Notify former owner when a unit is captured in combat (fixes BR#3370) - #167
Open
larrydgray wants to merge 1 commit into
Open
Notify former owner when a unit is captured in combat (fixes BR#3370)#167larrydgray wants to merge 1 commit into
larrydgray wants to merge 1 commit into
Conversation
csChangeOwner() only sent the ChangeSet update to the new owner, never to the player who lost the unit. Their client kept a stale copy (still theirs, still active, still with full moves), which resurfaced it as needing orders every End Turn even though the server had already reassigned it -- a stuck "ghost unit" that rejected all move orders since the server no longer recognized the client as its owner. Explicitly notify the former owner via csRemove(), mirroring the See.only()/See.perhaps() visibility split csSlaughterUnit() already uses for the settlement/open-field cases. Fixes https://sourceforge.net/p/freecol/bugs/3370/
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
When a unit loses a combat and the result is that it gets captured (rather than killed, losing equipment, or demoted), the losing player's client was never told that the unit changed hands.
ServerPlayer.csChangeOwner()transfers ownership correctly on the server, but the onlyChangeSetupdate it sends is to the new owner:The former owner's client keeps its stale copy of the unit -- still theirs, still active, still with full moves -- so it keeps resurfacing as "needing orders" at every End Turn even though the server has already reassigned it. Trying to give it any order fails with "The server can not do that.", since the server no longer recognizes the client as the unit's owner.
Fix
In
csCaptureUnit(), after the ownership transfer succeeds, explicitly notify the former owner to drop the unit viacs.addRemove(...), mirroring theSee.only()/See.perhaps()visibility splitcsSlaughterUnit()already uses for the settlement/open-field cases.Scoped to
csCaptureUnit()rather than the sharedcsChangeOwner()utility, sincecsChangeOwner()has several other callers (colony capture, native conversion, REF defection) I have not individually verified are safe to change the same way -- they may share the same missing-notification pattern and could be worth a follow-up.Verification
Combat attacker=... Attack LOSE CAPTURE_UNIT) and the subsequent client-side desync (client held the unit as[unit:8805 english veteranSoldier], server responded to a move attempt withUnit not owned by player:19: [unit:8805 russian freeColonist]).sf-auto-merge(ant compile, no errors).Fixes https://sourceforge.net/p/freecol/bugs/3370/