broadcast_city_info has apparently illogical code
lexxie9952 After more careful checking I think the fact that info is sent to powner instead of pplayer is the only actual bug. The code could be better arranged, and the variable 'send_city_suppressed' more accurately named (my assumption about is was causing me to think there to be other bugs)
- 'send_city_suppressed' name does not make it clear it applies to send to *owner* only. One would assume that if info is sent to anyone despite that flag, it would owner who still gets latest information
- It's a bit weird that 'send_city_suppressed' check is only inside 'can_player_see_city_internals()' block, not affecting else and 'can_player_see_city_externals()' block. This is not an actual bug simply because the city owner always 'can see city internals', so owner-specific 'send_city_suppressed' is never relevant in the else branch.
I may open a new ticket about renaming 'send_city_suppressed'. In this ticket I plan to change information to be sent to pplayer, and to move "if (!send_city_suppressed ..." check outside whole 'if (can_player_see_city_internals()) {} else {}' construct. Both for clarity and performance (no point to figure out can_player_see_city_internals() if the simpler checks are about to prevent the send anyway.
- Going to push also to S2_6
- Master/S3_1 patch has the refactoring I described earlier, S3_0 & S2_6 patches implement the minimum change to fix the bug.
lexxie9952 Thanks! It turned out to be almost exactly same as my fix, but I noticed your changing the "if" might be faster performance.
lexxie9952 Hello. After I implemented a small difference between my fix and your fix, a week later we noticed a bug and finally tracked it back to this patch. It turns out the "performance enhancement" of changing the order of the outer and inner IF statements, causes a bug. The bug can be found by attacking a city with killcitizen unitclass, poison city, etc. You'll see it doesn't update the client. The cause is because the "else" formerly responded to the not-true condition of the outer if, which was then made to an inner if, which caused the "else" statement to trigger itself under different logical contexts, and this bug is a "falling through the cracks" in the adjusted logic of the otherwise-case condition to the first outer if statement!
See https://github.com/Lexxie9952/fcw.org-server/commit/17bc5abe49aa435a4e1ca1ed08073611cd3ad785 After doing this fix, the bug disappeared for us.
Reply To lexxie9952
See https://github.com/Lexxie9952/fcw.org-server/commit/17bc5abe49aa435a4e1ca1ed08073611cd3ad785 After doing this fix, the bug disappeared for us.
That's not complete revert of the rearrangement I made. You would need to move the "else" block too, to match correct "if"
Not that I yet can see what the exact problem with my patch is.
Reply To cazfi
That's not complete revert of the rearrangement I made. You would need to move the "else" block too, to match correct "if" Not that I yet can see what the exact problem with my patch is.
Checking the FCW code it seems that something had gone wrong when you applied the original patch, so you really had only that part of it. Which explains the issues you saw. I think my patch is ok after all.
lexxie9952 Hello. Hopefully I'm not annoying on this. But I am finding trouble with what you said. The current FCW code compared to current upstream code looks like this:
[https://gyazo.com/b51dd247004aaa9c235b14e77ea2476b
To me it seems the only difference in this is the powner was changed to pplayer in the right spots. So far, so good?
Correct me where I go wrong, please!
This is how it compares to your patch to the upstream code:
https://gyazo.com/bbe2968357dc102b8dcccc8bb8107430
If there is any action I should take please let me know. I am just confused because it was buggy and now I got it working with that change.
Reply To lexxie9952
Hello. Hopefully I'm not annoying on this. But I am finding trouble with what you said. The current FCW code compared to current upstream code looks like this: [https://gyazo.com/b51dd247004aaa9c235b14e77ea2476b To me it seems the only difference in this is the powner was changed to pplayer in the right spots. So far, so good?
Yes, that's the situation before applying my patch to freeciv.org code.
Now, look at the patch. You had ever applied only part of it:
- if (can_player_see_city_internals(pplayer, pcity)) {
- if (!send_city_suppressed || pplayer != powner) {
+ if (!send_city_suppressed || pplayer != powner) {
+ if (can_player_see_city_internals(pplayer, pcity)) {
Having only that is wrong as it changes what "if" the "else" block is part of. The latter part of the patch begins with:
- }
- } else {
- if (player_can_see_city_externals(pplayer, pcity)) {
+ } else if (player_can_see_city_externals(pplayer, pcity)) {
The crucial bit being the very first removal of a "}"
in freeciv/freeciv master branch, citytools.c, broadcast_city_info Line 2182, https://github.com/freeciv/freeciv/blob/5bcfc01b1cd0a721764ff64b8706a037826294d1/server/citytools.c#L2182
We iterate all players to see if they can see inside the city, presumably so they can "see" the internal city data. But each time they qualify to see it, we make that info accessible to the city **owner.** So, for every pplayer who can see inside the city, we repeatedly and redundantly allow the city owner access to see inside their city. Not the pplayer who is supposedly able to see.
This ticket submitted by request... copy-paste of cazfi response: "Can you open a ticket about that? The code makes no sense, while it's less clear what exactly is wrong with it (seems to me that it has actually multiple problems, but need to confirm that with more time)"
NOTE: for the purposes FCW was able to achieve, changing powner to pplayer worked. If there are other issues when all this gets rewritten, please send me a courtesy notification. Thanks!