Yes, I will comment it better. Let me start by explaining here my intent. For anyone else not so familiar with the visibility handling, & so you, Geoff, can let me know if my understanding has pertinent gaps, I'll include some extra review here. Although we may still want to implement an audit process like you had mentioned the other day, at least for a while to see if it still catching things not otherwise handled correctly, I was hoping to clean up the underlying update mechanisms so that the audit wouldn't actually have anything to catch. As part of that, I was wanting to try coming up with a somewhat more consistent approach for the update containers in general (be that a system, or a fleet, etc), and how that interacts with the update of the things that can be contained. A key requirement is that it be robust to ordering-- whether the container gets updated before the contained objects. Redundant update checking can be ok, so long as one part of updating doesn't interfere with another, but I did want to try avoiding any such redundancy.Geoff the Medio wrote:This comment in Fleet.cpp confused me:A comment in Ship::Copy or similar functions when removing an object from a previous container might be helpful, to explain why this might be necessary.
Code: Select all
// removal from old fleet and setting of ship's m_fleet_id handled by the ship
The basic overall process is that the server determines which objects are visible to which empires, and then it updates the Objectmaps for those empires by using the Clone() (initially) or Copy() method (for updates) for each visible object. The Copy() function copies certain information from the server's copy of the object into the empire's copy of the object, dependent on visibility.
The simplest situation "(A)" to deal with is when the new container, old container, and contained objects all have Partial Visibility or better; then the updating is all fairly straightforward & we just need to not be doing conflicting updates.
Another update situation "(B)" is when the old container is observed but the (formerly contained) objects and new container are not observed at all. If the old container was observed with with Partial Visibility (and until now at least for Systems, even with Basic Visibility) then its contained objects list is overwritten during Copy(), and, I believe as part of another (earlier?) stage in the visibility processing the not-now-observed-but-previously-contained objects get flagged into the Universe::EmpireStaleKnowledgeObjectIDs(client_empire_id) list, but don't otherwise get updated. That StaleKnowledge list something that hasn't been exposed to the AI yet, and I'm just now reading up on it. I think currently its only use is to block stale objects from being shown on the Galaxy Map. The changes we're currently making to System and Fleet Copy() updates probably make it unnecessary for the client to use the StaleKnowledge list for making accurate predictions of blockades but I want to think about it a bit more, and whether any other updating of the underlying objects could/should be done, but I'll pass on that for now.
The revision we just settled on for Systems with only Basic Visibility is that their contained objects list would be updated to add any objects currently visible in that situation, but that previously observed objects would not necessarily be erased from it.
Yet another update situation "(C)" is if the new container (and objects ) are observed, but not the old container, in that situation the old container (if present in the object map) could/should be updated to no longer think it contains those objects, and the objects and new container get whatever update is suitable to their visibility.
The Approach in my Revisions
Removal from old Container
In looking this code over, one thing that stood out was situation (C). For the things I've looked at so far, it looked like the old container was not being updated at all (instead relying on EmpireStaleKnowledgeObjectIDs to eventually clear things up, I guess). To have an update, it needs to be handled either by the new container or the objects themselves, since the old container might not have current visibility (and therefor not be getting its own individual update). Since fleets could be visible inside of containers (Systems) or outside (on starlanes), it made sense to me to have this update triggered by the object itself rather than the container. For consistency I think that approach should be taken for all objects & containers, not just fleets & systems. Since systemID is an attribute of all UniverseObjects, I put the systemID checking & removal code into UniverseObject::Copy(). Removing Ships from their old fleet is likewise handled by Ship::Copy(). Once the Ship has made sure it is removed from any previous Fleet, it is ok to update its fleetID (that could possibly be done by new Fleet when adding this Ship, but would then need to also handle the removal process to not interfere with removal from the old Fleet, and since since not all removal can be handled by a new container I felt this approach was best). I will add a comment to this portion of both those Copy() methods, referring to this approach. I have not yet looked at Planets and Buildings, but since buildings don't go planet hopping, I think that's much less of an issue for us.
For both Fleets and Systems, if the old container has Partial Visibility then it will directly overwrite its contained objects list, which may be redundant with the above checks but should not cause any problems. **edit -- scratch this following suggestion re the extra checking -- if the objects are visible they will take care of the update with the old container, and if they are not visible then they should not be removed from the old container.** We could think about doing extra checking here in case the objects are not visible, and possibly doing some update on them to directly flag that they no longer appear in the old container, but the EmpireStaleKnowledgeObjectIDs list probably meets that same need. It seems we could perhaps instead add a 'not_currently_visible_in_container' flag to UniverseObjects to perform the same function but keeping that info with the other Object info (and also allowing it to be determined in a fashion more consistent with other object updates). Right now the blockades code has not been looking at the EmpireStaleKnowledgeObjectIDs list and so on the client side may be predicting some blockades when it (probably) should not; I'll be following up on that.
Adding to New Container (or updating within container)
Different containers have different ways of storing info about their contained objects -- Systems map orbits to sets of objectsIDs (and may nee dto update orbit for an object they already contain), whereas Fleets just keep a set of shipIDs, so it seemed best to have object insertions/updates handled by the containers. I don't think there is a way that an object could be visible & have an update without its container also having at least basic visibility and therefore getting an update.