Only send bombardment combat when targets are visible #2363
Conversation
|
Note that with this PR, there won't be any animation when bombarding. |
|
With this PR I don't see an animation when bombarding a city, but I still get a message for each unit bombarded inside the city, so that information is still leaking through?
edit: Confirmed even with a newly created server. |
|
I didn't touch messages because they are handled in #2362 |
|
but shouldn't these messages not be triggered if the server isn't sending the information to anyone but the defender? If the attacker receives this information, then the attacker can hack the client to make it visible. |
|
The messages are created and sent by the server and the client only displays them. I think your PR already removes them |
|
ooooh ok, my bad |
| @@ -3592,7 +3606,8 @@ static void see_combat(struct unit *pattacker, struct unit *pdefender) | |||
| Send combat info to players. | |||
| */ | |||
| static void send_combat(struct unit *pattacker, struct unit *pdefender, | |||
| int att_veteran, int def_veteran, int bombard) | |||
There was a problem hiding this comment.
You've removed int bombard but #2362 needs it
There was a problem hiding this comment.
This seems to be the only compatibility issue between the two PRs - as far as I can tell
There was a problem hiding this comment.
Oh, I see that #2362 uses this to not route bombardment towards the combat HUD. We may need to have a discussion about the client-side display of bombardment before we merge.
lmoureaux
force-pushed
the
bugfix/bombard-sees-inside-cities
branch
from
7a72b5c to
4aceed4
Compare
Newly added packets shouldn't be sent to old clients. In order to permit adding new packets, add the possibility to send a packet only when a capability is available. The packet is silently ignored otherwise. This is needed to add a packet for bombardment information.
lmoureaux
force-pushed
the
bugfix/bombard-sees-inside-cities
branch
from
4aceed4 to
4d9312a
Compare
This will allow reusing it to display bombardment.
Bombarding cities was very powerful because it was showing every unit inside the city, something only diplomats and spies can normally do. Stop sending this around. Bombing a tile no longer shows every unit on the tile being bombarded individually. Instead, an explosion is shown on top of the tile without more information. This required adding a dedicated packet.
lmoureaux
force-pushed
the
bugfix/bombard-sees-inside-cities
branch
from
4d9312a to
461585a
Compare
|
Tested: Player C experience when player A bombs player B, both for a unit stack sitting outside and for units inside a city. |
|
Tested: New server with both old and new client. When the new client bombards: When the old client bombards: |
|
Just noticed one of the client (or both) were complaining about I'll have to run them on separate consoles to see who's complaining. Currently running the old server with the two clients and noticing a lot more of those than before. |
|
Testing: Old server with both new and old clients When new client bombards: When the old client bombards: Again, some of these popped up every now and again, but didn't appear to affect the game |
|
On the PR client, destroying a city caused this: Bombarding is not related. However, this only happened for the first city that was destroyed... Destroying other cities did not bring those up again. The old client destroying a city of another nation didn't cause this either. Old client didn't show assertion errors at all. |
|
nevermind, the master client can pop up the same kind of assertion errors too. Happened when destroying a city as well. It happened after I disconnected and reconnected the new client - maybe related? Disconnecting and reconnecting both clients and then destroying a city with the old client didn't cause any warnings. Not clear what is behind this - but it does not seem related to the bombard updates. |
Bombarding cities was very powerful because it was showing every unit inside the city, something only diplomats and spies can normally do. Stop sending this around.