[patch] Everybody shoots

[MatGB]

Something is definitely off with visibility after combat. I just lost two ships to a sentry and two Mu Ursh homeworlds in the same system, it was according to the combat log fairly close, but neither the sentry nor either planet are showing any damage (it's scanlined now, I have nothing else nearby. I want to get another ship in ASAP but don't want to lose that, I can do the maths from the combat log to work out where things are at but given I know the damage done it should be showing.

I don't recall if this is a change in behaviour or it's always been this way, but regardless I don't like it, if the empire knows something, especially a monster, has been damaged, then that damage should surely show.

[Mitten.O]

I think I got the information gained in battle be considered gotten last turn, now.
That is, if you saw something in battle, but don't see it anymore,
it should show up grayed out, and with the health you know from the battle.

Tried to convince git to generate an SVN compatible patch instead of
generating the patch with SVN. It should work.

EDIT: There are still 150 lines of code in PostCombatProcessTurns,
during which the visibility of things does not decrease,
whereas before it was computed from scratch thrice.
(The lines between the first call to UpdateEmpireObjectVisibilities(false)
until the first call to UpdateEmpireObjectVisibilities without parameters.)
This was the most fluent way I could come up with to stop the
visibility gained in battle from being forgotten.

Arguably it makes sense for the empire not to forget what is saw during the turn while the turn is
still going on, but I cannot know that this won't affect something adversely.

[Dilvish]

for anyone else wanting to test this out, here's an svn diff relative to /trunk/FreeOrion. This current state of this visibility issue seems like a bug to me and I think it would be great if this works & could make it into 0.4.4.

I haven't tested this yet, but looking it over it looks pretty good to me.

I was wondering a bit about the visibility resets & the possibility of some further efficiency gain & a possible small bug. The post-movement-pre-combat call to UpdateEmpireObjectVisibilities() includes a reset as before, makes total sense to me. At the beginning of PostCombatProcessTurns(), it seems to me that changing the call to UpdateEmpireObjectVisibilities to be without a reset means in this case it wouldn't actually do anything. I'm thinking it could probably be dropped entirely, but if I'm overlooking some reason it's needed then perhaps it should swap places to be after the adjacent call to UpdateEmpireLatestKnownObjectsAndVisibilityTurns() and then stay with a reset. Also, later in PostCombatProcessTurns after updating meters, there is another call to UpdateEmpireObjectVisibilities around patched line 2947 which has been changed to be without a reset, which isn't seeming quite right to me-- if an ion field or something has moved such that it would reduce an empire's visibility of some object, I don't think that would have been taken into consideration with the initial resetting call to UpdateEmpireObjectVisibilities since meters hadn't been updated then; it seems that this call at patched line 2947, the first after meter adjustment, should include a reset. And if that's the case, then the call at patched line 2967 (the 3rd call to UpdateEmpireObjectVisibilities() from within PostCombatProcessTurns) seems unnecessary.

Of course that last point would seem to apply to the current setup as well since all the calls include resets-- Geoff, do you have any comments on that?

[Geoff the Medio]

Geoff, do you have any comments on that?

I don't like the method used to fix the issue, I don't like how reset is a parameter (would be better as a separate function, I think), and I don't want to make this type of change right before a release.

[Dilvish]

a reminder for us to follow up on this...

[Geoff the Medio]

After all the other stuff...

[Bigjoe5]

a reminder for us to follow up on this...

Has this been followed up on?

[Geoff the Medio]

Has this been followed up on?

No.

[Dilvish]

Mitten, could you please put this latest combat visibility patch up on GitHub as a pull request? In the meantime, I have put it up as a fought_visible branch on my GitHub fork to facilitate people being able review it and test it out, but I didn't have author info to use for Mitten (no public email for him it appears) so this branch is just an interim thing.

Geoff, do you have any comments on that?

I don't like the method used to fix the issue, I don't like how reset is a parameter (would be better as a separate function, I think), and I don't want to make this type of change right before a release.

When you have a chance, please take a look again and give any more particular advice/requests that come to mind. I think this is one of the things we should make sure we get fixed for 0.4.5.

p.s.: I'll note a couple things I just saw about it worth discussion-- my attacked ships were destroyed so I no longer had visibility into the system, but the MapWnd monster icon was not hashed out (probably because the visibility was considered to be from the current turn); it would probably be best hashed out in that case. Also, when clicking on the monster icon in that case, the fleetwnd pulled up had some kind of blank/error icon for the monster.

[Mitten.O]

Ok, here: https://github.com/freeorion/freeorion/pull/29.

The code merged cleanly and at least compiles still, but I do not have time to create the situation where it matters. I see no reason why it should have stopped working since it was made, though.

[Dilvish]

Ok, thanks for putting that up, I think that should help this progress now.

I see no reason why it should have stopped working since it was made, though.

I would say that it works, but there appear to be what I would call a couple of rough edges. On the 2 issues I mentioned (in the case of no surviving observer, lack of scanlines for the attacker, and if the log link for the attacker is clicked, then the fleetwnd for the attacker shows a blank/bad icon), do you recall the behavior? I'll double check with your PR code tough, perhaps something was off with the old code I had.

Let me also note, it is fairly simple to create conditions to test it-- just start a game with high specials and monsters and send a single scout out exploring past your homeworld visibility.

[Dilvish]

A heads-up to anyone following the forums more closely than the GitHub repo: I posted a derivation of this for discussion as PR 52