Post combat visibility #52
Conversation
…isibility, so that Basic Visibility info gleaned during combat would not necessarily be considered 'stale' (which was causing the human client MapWnd to suppress display of such)
Vezzra
added
the
category:bug
|
Geoff, if you have suggestions where I should look to try figuring out why the "Ship" link fails to zoom on click, please let me know. |
| // Also ensure that attacker (and their fleet if attacker was a ship) are | ||
| // revealed with at least BASIC_VISIBILITY to the taget empire | ||
| combat_state.combat_info.empire_object_visibility[target->Owner()][attacker->ID()] = | ||
| std::max(GetUniverse().GetObjectVisibilityByEmpire(attacker->ID(), target->Owner()), VIS_BASIC_VISIBILITY); |
There was a problem hiding this comment.
Why is it necessary to poll the main universe visibility info here? Doesn't the container you're modifying have the relevant info about the best visibility level empires have for each object?
There was a problem hiding this comment.
As far as I could tell, it appeared these combat_info visibility maps had never been populated at all before. Even if I overlooked that somewhere, this was the safest approach. I had been trying to decide about putting in a warning comment about that, but decided to wait and do some more searching, but I suppose I should have added some kind of advisory/todo comment somewhere. Mitten's version hadn't needed to do this checking because he was setting visibility to Partial, and so didn't have to worry that a non-owner might already have had a higher visibility. If we change the later dissemination stage to check visibility and go with the greater, rather than just overwriting, then we wouldn't need to query it here.
|
Re: the Ship link, do any of the combatant or system links work for you? The CombatLogWnd class is a CUILinkTextMultiEdit but I don't see anywhere that its link clicked signals are connected to anything which would react to the clicking of a link in the text. See line 442 of EncyclopediaDetailPanel.cpp for the needed connection(s). |
|
Re: setting the stale object info, I don't think it would be difficult to add an extra case for things that got visibility updates during combat but otherwise aren't visible. But I suppose granting knowledge of object stealth is fine until that is done. |
|
You are right about none of those links working, odd I hadn't noticed it before. I'll keep your tip in mind for following up on that, but since it now appears unrelated to these changes it is fairly distinct and I won't plan to work on it until sometime after the rest of this is working. |
|
Links should be working in master. |
|
That's great about the links, thanks. I adjusted the handling of visibility info as you suggested a few comments up. I think this is all ready now. I put a bit more thought into adjusting the stale object algorithm, but then I realized that this is something that has to be repeatable every turn after the combat, not just on the immediately next time when the stale info algorithm could be passed some info about the most recent combats. So to make it lasting we'd either have to keep a long term log of the combat info for use here, or else add some kind of flag information to universe objects. At least that's all that came to mind for me, so I'm glad you are wiling to accept the stealth meter handling at least until we can think of something better. |
…f pointless copying of visibility info
|
doh! I had borrowed the use of Objects() from other nearby uses, but hadn't paid quite enough attention as I did that, and hadn't tested long enough to notice any resulting lag from all the extra copying. Changed now to use system->ContainedObjectIDs(). |
|
Some observations: To avoid this, perhaps objects that have only ever been basically visible should report large stealth values (not 0 stealth) so they will fail the should-be-visible-based-on-last-known-state test, and thus not be marked as stale. Or, the staleness test could check if the object has only ever been basically visible, and if that's the case, never mark it as stale. |
|
Made a commit to return LARGE_VALUE for stealth when basically visible, but to not overwrite previously-observed legit meter values... This might let a one-turn-effective SetObjectVisibilityForEmpire effect work reasonably as well... |
|
Your change to the basic visibility stealth meter worked fine to replace the somewhat different approach I had originally taken here. I combined the parts of this PR we are keeping and committed as 84d07d8, so will now close this PR. |
adrianbroher
added
status:merged
So, this PR is just submitted for discussion; it is a derivation from Mitten's PR #29. I only wound up using a portion of lines from that & so these commits just start from scratch rather than start from his and then revert 70% of it. Perhaps I should do just that, though, or perhaps just make a new commit in his name with the portion of his that I kept-- someone with more experience with standard git practice please advise about recommended protocol here.
Anyway, I think this accomplishes 95% of what we want for this. With this, stealthed attackers reveal Basic Visibility of themselves to their targets (as opposed to the Partial Vis granted in the Mittens PR). It is broken into two commits in case Geoff prefers some other way than what I have in the second commit to deal with the MapWnd suppression issue, perhaps by adjusting the Universe::UpdateEmpireStaleObjectKnowledge() algorithm. Geoff, I think this does address all your concerns mentioned regarding PR #29 (in our forum thread on it).
Below is a screenshot immediately post-combat after a stealthed Asteroid Snail killed my scout.
. Once I research Active Radar and observe the snail, its icon on the map, etc., gets updated properly. The reason I say this is currently only 95% of what we want is because if the "Ship" link in the combat log is not working as an active link, and I haven't figured out why.