Possible fix for Detach UnDo issue#14298
Open
riverwanderer wants to merge 5 commits intovassalengine:release-3.7from
Open
Possible fix for Detach UnDo issue#14298riverwanderer wants to merge 5 commits intovassalengine:release-3.7from
riverwanderer wants to merge 5 commits intovassalengine:release-3.7from
Conversation
…h dice roll. Creating a single masterCommand in DR(). Appending all actions (Hotkeys, Chat Report, Property Set, and the ShowResults command) to this single master command. Calling GameModule.getGameModule().sendAndLog(masterCommand) only once at the end of DR(). This process ensures that a single user action (the dice roll) generates one log entry, which then only requires one undo step to revert the entire sequence, correctted in previous Commit.
…sync with dice roll." This reverts commit 5873501. Accidentally pushed.
riverwanderer
added
bug
Collaborator
Author
|
The fixed snapshot has been used in several games with the module that exhibited the bug. No recurrences or other issues so far. |
riverwanderer
added
Ready for Review
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
2 participants
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.
Replacing #14297.
AI-mediated, passed an initial test.
This fix addresses a critical bug in the Deselect trait (when configured with the "Remove piece from stack" / unstack option) where a piece would disappear from the map after the Detach action was undone.
AI-described fix:
Root Cause
The root cause was the implementation within the Deselect.myKeyEvent() method when unstack was true. The command chain generated for the detach action was structurally flawed:
It relied on Map.placeAt(outer, pos) to unstack the piece, which generated a Command that did not reliably restore the piece's parent (the original Stack) upon undo, causing the piece to lose its Map/Zone reference.
It immediately followed the unstack with unnecessary and confusing logic that attempted to create and place a new, empty stack at the same location. This compounded the state corruption during the undo operation.
The Fix
The fix involves simplifying the command chain and removing the faulty logic to rely on the VASSAL engine's core capability to manage piece container changes.
Faulty Logic Removal: The code that called m.getStackMetrics().createStack(outer) and subsequently placed this new, empty stack was removed.
Command Isolation: The resulting command (c) is now isolated to only the single necessary action: c = m.placeAt(outer, pos);. This command removes the piece from the original stack and places it directly on the map.
By simplifying the command and removing the conflicting state changes, the Command generated by placeAt is now correctly reversed by the VASSAL engine's undo mechanism, ensuring the piece's parentage is fully restored and the piece reappears in its original stack upon undo.
File(s) Modified:
VASSAL/counters/Deselect.java