search

vassalengine / vassal

VASSAL, the open-source boardgame engine
https://vassalengine.org
GNU Lesser General Public License v2.1
426 stars 102 forks source link

UnDo triggers a deck "send hotkey when empty"; so corrupting the game state #11344

Closed riverwanderer closed 2 years ago

riverwanderer commented 2 years ago

A deck has a "when empty" hotkey configured. During a game, when the deck is already empty, an action is taken that puts pieces back into that deck.

If that action is UnDone, as soon as the deck becomes empty again, the Hotkey triggers! This is not correct - we now have a different game state from the one that existed before the Action was taken.

Demo of this occurring in Liberté during an election phase: Use this module: https://www.dropbox.com/s/hbflrqdbv83ts30/Libertev1-0a30.vmod?dl=0 Replay this log file: https://www.dropbox.com/s/wrfdspbvj5866i2/Liberte%20UnDo%20Bug%20demo.vlog?dl=0

riverwanderer commented 2 years ago

Workaround (partial): For some games, Module developers will be able to protect the game state by ensuring that the "send hotkey when empty" Hotkey cannot execute unless valid.

BrentEaston commented 2 years ago

Hi Mark,

Please try the UndoNoDeckEmpty build. Note that this build will prevent a 3.6.7+ client from triggering the hotkey, but if a 3.6.6- client plays the log or is online with a 3.6.7+, they will still trigger the hotkey.

riverwanderer commented 2 years ago

Tested using Empty HK test module (attached). Test shows that the fix works. Also tested with two players; test still passed. I observed that two UnDo actions are required to backtrack past the Empty Deck event.

To repeat a simple test and observe the hidden UnDo action (if this is relevant);

  1. Run up the test module: Empty HK test.zip

  2. Start a New Game, connecting as P1 or P2.

  3. Ignore the introductory chat log message except for the last line. If this indicates that Alert() mode is enabled, you can disable it in Preferences...Test Options.

  4. Draw the two cards from the deck. Note the message in chat. Place one card back on the deck.

  5. Test UnDo step by step as follows 5.1 UnDo - note card moves off the deck. No deck empty event occurs. 5.2 UnDo - a card now moves back on the deck. 5.3 UnDo - this UnDo appears to have no effect. 5.4 UnDo - now the "Empty Deck" chat log message is "undone". 5.5 UnDo - second card goes back on the deck End of UnDo history

BrentEaston commented 2 years ago

The two UnDo actions to undo a Deck Empty event is a symptom of #11429 which is not included in this fix. I'll do a build containing both fixes for you to test.

BrentEaston commented 2 years ago

Try the CombinedUndoFixes which is just building now.

riverwanderer commented 2 years ago

Tried combined fix; seems still to require an extra undo.

Test: Run up the demo module game.

  1. Remove top card then Remove second (bottom) card. Message appears in chat log.
  2. UnDo - card moves back to deck.
  3. UnDo - only now does the chat log message "undo".
BrentEaston commented 2 years ago

That does not happen for me. The Undo of the second card produces both of these messages:

Are you definitely running the CombinedUndoFixes version?

riverwanderer commented 2 years ago

Yes, I just checked again.

I am running this version of the demo module, different from the one you used, though I am not sure why that would make a difference to the observation: Empty HK test.zip

BrentEaston commented 2 years ago

I found an issue where Hotkeys where still not being batched into a single Undo properly. Please try the latest version of the CombinedUndoFixes build

riverwanderer commented 2 years ago

Tested snapshot 9833a65. Still seeing multiple steps in UnDoing a single empty deck action

  1. Move the last card off a deck - note Empty Deck hotkey message appears in chat
  2. Undo#1 - card returns to deck
  3. Undo#2 - no apparent effect
  4. Undo#3 - chat message Undone.
BrentEaston commented 2 years ago

Hi Mark, there is a new build of CombinedUndoFixes going up now.

riverwanderer commented 2 years ago

In my test, the extra UnDo seemed to be fixed but I have a couple of other issues show up with normal actions in my test module that seem related to the Empty Hotkey itself rather than UnDo:

  1. My test module has the Deck Empty hotkey trigger an Action Button that reports to the chat window. In the test, if I put a card back and then draw it (to empty the deck again), no report shows up. UPDATE: Ignore this, it was an artefact of my test module. Deck Empty hotkey is working always in the test.
  2. When I repeat the discard / draw sequence some more times, I eventually get this exception: 
    
2022-05-28 11:29:54,972 [64259-main] INFO  VASSAL.launch.StartUp - Starting
2022-05-28 11:29:54,978 [64259-main] INFO  VASSAL.launch.StartUp - OS Mac OS X 12.4 x86_64
2022-05-28 11:29:54,978 [64259-main] INFO  VASSAL.launch.StartUp - Java version 18.0.1
2022-05-28 11:29:54,978 [64259-main] INFO  VASSAL.launch.StartUp - Java home /Applications/VASSAL-3.6.7-SNAPSHOT-0da792f-CombinedUndoFixes.app/Contents/MacOS/jre
2022-05-28 11:29:54,978 [64259-main] INFO  VASSAL.launch.StartUp - VASSAL version 3.6.7-SNAPSHOT-0da792f-CombinedUndoFixes
2022-05-28 11:29:54,978 [64259-main] INFO  VASSAL.launch.Launcher - Editor
2022-05-28 11:29:56,326 [64259-AWT-EventQueue-0] INFO  VASSAL.build.GameModule - Test Empty Deck Hotkey version 0.2
2022-05-28 11:31:17,337 [64259-AWT-EventQueue-0] ERROR VASSAL.tools.ErrorDialog - 
java.lang.NullPointerException: Cannot invoke "VASSAL.command.Command.append(VASSAL.command.Command)" because "move" is null
at VASSAL.build.module.map.PieceMover.performDrop(PieceMover.java:1337)
at VASSAL.build.module.map.PieceMover.mouseReleased(PieceMover.java:1298)
at java.desktop/java.awt.AWTEventMulticaster.mouseReleased(AWTEventMulticaster.java:298)
at java.desktop/java.awt.AWTEventMulticaster.mouseReleased(AWTEventMulticaster.java:298)
at java.desktop/java.awt.AWTEventMulticaster.mouseReleased(AWTEventMulticaster.java:297)
at java.desktop/java.awt.AWTEventMulticaster.mouseReleased(AWTEventMulticaster.java:297)
at VASSAL.build.module.Map.mouseReleased(Map.java:1940)
at java.desktop/java.awt.AWTEventMulticaster.mouseReleased(AWTEventMulticaster.java:297)
at java.desktop/java.awt.AWTEventMulticaster.mouseReleased(AWTEventMulticaster.java:297)
at java.desktop/java.awt.Component.processMouseEvent(Component.java:6616)
at java.desktop/javax.swing.JComponent.processMouseEvent(JComponent.java:3398)
at java.desktop/java.awt.Component.processEvent(Component.java:6381)
at java.desktop/java.awt.Container.processEvent(Container.java:2266)
at java.desktop/java.awt.Component.dispatchEventImpl(Component.java:4991)
at java.desktop/java.awt.Container.dispatchEventImpl(Container.java:2324)
at java.desktop/java.awt.Component.dispatchEvent(Component.java:4823)
at VASSAL.build.module.Map.drop(Map.java:2076)
at VASSAL.build.module.map.PieceMover$AbstractDragHandler.drop(PieceMover.java:2026)
at java.desktop/java.awt.dnd.DropTarget.drop(DropTarget.java:455)
at java.desktop/sun.awt.dnd.SunDropTargetContextPeer.processDropMessage(SunDropTargetContextPeer.java:548)
at java.desktop/sun.lwawt.macosx.CDropTargetContextPeer.processDropMessage(CDropTargetContextPeer.java:129)
at java.desktop/sun.awt.dnd.SunDropTargetContextPeer$EventDispatcher.dispatchDropEvent(SunDropTargetContextPeer.java:864)
at java.desktop/sun.awt.dnd.SunDropTargetContextPeer$EventDispatcher.dispatchEvent(SunDropTargetContextPeer.java:788)
at java.desktop/sun.awt.dnd.SunDropTargetEvent.dispatch(SunDropTargetEvent.java:48)
at java.desktop/java.awt.Component.dispatchEventImpl(Component.java:4856)
at java.desktop/java.awt.Container.dispatchEventImpl(Container.java:2324)
at java.desktop/java.awt.Component.dispatchEvent(Component.java:4823)
at java.desktop/java.awt.LightweightDispatcher.retargetMouseEvent(Container.java:4948)
at java.desktop/java.awt.LightweightDispatcher.processDropTargetEvent(Container.java:4649)
at java.desktop/java.awt.LightweightDispatcher.dispatchEvent(Container.java:4511)
at java.desktop/java.awt.Container.dispatchEventImpl(Container.java:2310)
at java.desktop/java.awt.Window.dispatchEventImpl(Window.java:2780)
at java.desktop/java.awt.Component.dispatchEvent(Component.java:4823)
at java.desktop/java.awt.EventQueue.dispatchEventImpl(EventQueue.java:775)
at java.desktop/java.awt.EventQueue$4.run(EventQueue.java:720)
at java.desktop/java.awt.EventQueue$4.run(EventQueue.java:714)
at java.base/java.security.AccessController.doPrivileged(AccessController.java:399)
at java.base/java.security.ProtectionDomain$JavaSecurityAccessImpl.doIntersectionPrivilege(ProtectionDomain.java:86)
at java.base/java.security.ProtectionDomain$JavaSecurityAccessImpl.doIntersectionPrivilege(ProtectionDomain.java:97)
at java.desktop/java.awt.EventQueue$5.run(EventQueue.java:747)
at java.desktop/java.awt.EventQueue$5.run(EventQueue.java:745)
at java.base/java.security.AccessController.doPrivileged(AccessController.java:399)
at java.base/java.security.ProtectionDomain$JavaSecurityAccessImpl.doIntersectionPrivilege(ProtectionDomain.java:86)
at java.desktop/java.awt.EventQueue.dispatchEvent(EventQueue.java:744)
at java.desktop/java.awt.EventDispatchThread.pumpOneEventForFilters(EventDispatchThread.java:203)
at java.desktop/java.awt.EventDispatchThread.pumpEventsForFilter(EventDispatchThread.java:124)
at java.desktop/java.awt.EventDispatchThread.pumpEventsForHierarchy(EventDispatchThread.java:113)
at java.desktop/java.awt.EventDispatchThread.pumpEvents(EventDispatchThread.java:109)
at java.desktop/java.awt.EventDispatchThread.pumpEvents(EventDispatchThread.java:101)
at java.desktop/java.awt.EventDispatchThread.run(EventDispatchThread.java:90)
BrentEaston commented 2 years ago

Hi Mark,

I have gone back a step to the original fix to prevent Undo from triggering Deck Empty Hotkeys in the latest UndoNoDeckEmpty. This does not attempt to fix any multiple undos. Can you give this a go. Thanks.

riverwanderer commented 2 years ago

Tested b1e8704 and it UnDo does not trigger the deck empty Hotkey. I need to remind myself what the other issue was that this build is fixing. There is something odd about the test though, repeated Undoes seem to break through the startup gkc (which just puts out messages to the chat log) and also the Vassal-generated room-joining messages:

- Sending game info to MarkBx...
- <MarkBx> Joining room test.
- VASSAL and Module versions match, enjoy your game!
*To replicate bug;: 1. Start a game online as P1; connect another player as P2. 2. Drag the cards from the deck. 3. Observe chat log when the last card is drawn. Also works with observers (vary PlayerNames to show which instance is producing the message). Try with more than 2 players/observers...each will echo the hotkey. Vassal v3.5.8 supplementary: Place a card back on the deck. Observe that the other player instance executes the Deck Empty Hotkey again! This seems to have been fixed in v3.6.x
*Alert will appear on player screens when deck empties. To disable go to Preferences...Test Options **
* MarkB (P1) brings you this message courtesy of the Deck Empty Hotkey (1)
* MarkBx (P2) brings you this message courtesy of the Deck Empty Hotkey (2)
* UNDO: * MarkBx (P2) brings you this message courtesy of the Deck Empty Hotkey (2)
* UNDO: * MarkB (P1) brings you this message courtesy of the Deck Empty Hotkey (1)
* UNDO: * UNDO: * MarkB (P1) brings you this message courtesy of the Deck Empty Hotkey (1)
* UNDO: * UNDO: * MarkBx (P2) brings you this message courtesy of the Deck Empty Hotkey (2)
* UNDO: * MarkBx (P2) brings you this message courtesy of the Deck Empty Hotkey (2)
* UNDO: * MarkB (P1) brings you this message courtesy of the Deck Empty Hotkey (1)
* UNDO: *Alert will appear on player screens when deck empties. To disable go to Preferences...Test Options **
* UNDO: *To replicate bug;: 1. Start a game online as P1; connect another player as P2. 2. Drag the cards from the deck. 3. Observe chat log when the last card is drawn. Also works with observers (vary PlayerNames to show which instance is producing the message). Try with more than 2 players/observers...each will echo the hotkey. Vassal v3.5.8 supplementary: Place a card back on the deck. Observe that the other player instance executes the Deck Empty Hotkey again! This seems to have been fixed in v3.6.x
* UNDO: -! VASSAL and Module versions match, enjoy your game!
* UNDO: -! <MarkBx> Joining room test.
riverwanderer commented 2 years ago

Reminded myself; the other issue being reliably finishing Empty Deck hotkey execution before passing it on to other players. As far as I can tell, that fix is working, bringing us to the position @BrentEaston described in this post.