Create account / Log in
Bug 12554 - UNDO only partially undoes certain moves - particularly if moving piece is moved a second time by the trigger it generates by moving on the map.
UNDO only partially undoes certain moves - particularly if moving piece is mo...
Status: ASSIGNED
Product: VASSAL
Classification: Applications
Component: Player
3.3.0-beta1
PC Windows
: unspecified normal
: 3.3.2
Assigned To: Brian Reynolds
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2020-04-23 02:51 CEST by Brian Reynolds
Modified: 2020-06-22 20:27 CEST (History)
2 users (show)

See Also:


Attachments
"Minimal Example Module" to reproduce bug (263.09 KB, application/zip)
2020-04-23 02:51 CEST, Brian Reynolds
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Brian Reynolds 2020-04-23 02:51:50 CEST
Created attachment 11641 [details]
"Minimal Example Module" to reproduce bug

I have attached a "minimal example module" that demonstrates the problem -- drag the piece to a few different spaces, then try the undo button, and you'll get the idea pretty quick.

(1) If you add "does not stack"/Immobilized to the piece, the problem goes away. So something to do with stacks and reversing their moves.
(2) The problem occurs specifically when a piece (or stack) is moved, and somewhere during the processing of the trigger caused by the move the piece (or stack) gets moved a second time.
(3) It is ONLY when a piece/stack is dragged somewhere as the initial part of the movement. A sequence initiated by send-to-location does not manifest the problem.

NOTE: although this particular example would be "fixed" by turning on Snap-To-Region for the zone, that's really only because that makes the module not generate the second follow-on movement command which is necessary to manifest the problem. Also there are plenty of times that secondary movement is useful that are not covered by turning on Snap To.

I think it is quite likely that the movement is being correctly wrapped up in the Command (else the initial move wouldn't be working right in logfile/multiplayer play, but it IS working right), and that therefore the error is somehow in the execution of the UNDO -- possibly some part of it is being refused/ignored as a duplicate-to-the-same-location or something like that. But alas I have not found it so far.
Comment 1 Brian Reynolds 2020-04-28 19:30:45 CEST
Updating version number to 3.3, per announcement to repost bugs in 3.3 even if we've already submitted against an earlier version.
Comment 2 Brian Reynolds 2020-05-17 23:49:00 CEST
Debugging report so far, in case someone has some ideas:

Outbound (i.e. forward executing) the following stuff happens:
•	Piece is moved to the “middle position” (literal spot I dragged it to)
•	Somewhere in here a Global Property gets set because that’s hooked into the piece
•	Piece is moved to the “final position” (centered in the space)
•	An “add piece” happens at the final position. 
•	At some point in here a Chatter message (report) is generated

Then, when we UNDO, we get the following sequence:
(1)	“Undo” of Chatter report message
(2)	“Remove Piece”
(3)	Move Piece to the “middle position” (to the spot I once dragged it to – this is the correct order for undoing the move)
(4)	Change Global Property back to what it had been before
(5)	Move Piece to the “original position” – w/ the correct coordinates for the original place the piece had been.

So that’s the RIGHT order of events! And then I stepped into the final MovePiece and watched it theoretically set everything to the right position! And then I stepped back out. And then as far as I could tell no other commands got executed (I had been thinking maybe the order of the MovePiece commands was going to be reversed, but no).

But then, when all was said and done… there was the piece still apparently “stuck at the middle position” and NOT actually moved to the original position, even though I’d theoretically just watched a MovePiece command move it there.

So possibly there was some snafu along the way in the final MovePiece, even though it “looked right”? 

If anyone can brainstorm up any likely scenarios here, by all means let me know:
•	Is there something in the “add piece” / “remove piece” that could be frigging things up?
•	Is it possible for a piece be to think it is at <original position> (i.e. its internal x,y position is correct) but the MAP to think it is at <middle position> -- i.e. one of those weird stacky things thinks it’s somewhere else?

Planning to debug some more and drill down further, of course, but would definitely be interested if anyone has any thoughts on what some of the flavors of failure are possible.

Ruled out (at least apparently):
#1 – ruled out that one of the Move Piece commands was failing to execute, or not being stored as part of the Undo sequence.
#2 – ruled out that Move Piece commands could be executing in wrong order
#3 – ruled out that Move Piece destination was being overwritten
Comment 3 Brian Reynolds 2020-05-18 17:31:53 CEST
More detail, in case anyone has any thoughts/clues to offer!

Outbound was:
1.	Move A-B
2.	PropertyChange (for a global property managed by the piece)
3.	Move B-C
4.	AddPiece (C)
5.	Chatter (report movement)

Undo was:
1.	“Undo” chatter
2.	RemovePiece (from C)
3.	MovePiece C-B
4.	PropertyChange back to old state
5.	MovePiece B-A

Some of these were in little [0] [1] arrays, others inward links, but that was the full tree according to the debugger. There’s nothing beyond the Command.seq tree is there? 

So I’m trying to figure out what the proper role of the AddPiece / RemovePiece pair is. Because it seems a little suspicious that there’s only one of them in each sequence (wouldn’t we theoretically need to add it to each space along the way? And maybe then remove it?). But then even if I contemplate the simpler version where there’s only one  “Move” in each pair, I’m still trying to figure out why it’s “okay” to only do an “AddPiece” on the outbound (no “Remove” for the original space), and but then when reversing it we ONLY “remove it” from where we’re starting from and DON’T add it to where we’re putting it. 

And of course the answer might be that it’s very much not okay to do it that way, and there’s actually some deeper flakiness in the creation of the undo sequence here, which was getting masked or “working well enough” in a simpler movement sequence and just happens to have been exposed by the double-move situation. 

I would be much more comfortable if the single-move sequence looked like Move-Add and the Undo was Move-Add. And even more comfortable with Remove-Move-Add undone by Remove-Move-Add. And then I’d be trying to figure out how to get a double move to generate Remove-Move-Add-Remove-Move-Add. 

But that’s about 70% just “plausible pattern matching” by my debugging spidey sense, because I haven’t quite figured out what “AddPiece” and “RemovePiece” are “for” (what they are intended to do, when are they necessary, when are they optional).
Comment 4 Brian Reynolds 2020-05-31 18:56:58 CEST
Check this out... I'm trying to figure out why PieceMover generates an "AddPiece" but the SendToLocation does not. Biggest mystery is in the last code fragment.

So in the 12554 instance, the PieceMover is running the following code to for its part of the move (line 570 of the 3.2.17 PieceMover):


      if (mergeWith == null) {
        comm = comm.append(movedPiece(dragging, p));
        comm = comm.append(map.placeAt(dragging, p));
        if (!(dragging instanceof Stack) &&
            !Boolean.TRUE.equals(dragging.getProperty(Properties.NO_STACK))) {
          final Stack parent = map.getStackMetrics().createStack(dragging);
          if (parent != null) {
            comm = comm.append(map.placeAt(parent, p));
          }
        }
      }


The second part of the move from SendToLocation calls into Map.placeOrMerge and runs:

    Command c = apply(new DeckVisitorDispatcher(new Merger(this, pt, p)));
    if (c == null || c.isNull()) {
      c = placeAt(p, pt);
      // If no piece at destination and this is a stacking piece, create
      // a new Stack containing the piece
      if (!(p instanceof Stack) &&
          !Boolean.TRUE.equals(p.getProperty(Properties.NO_STACK))) {
        final Stack parent = getStackMetrics().createStack(p);
        if (parent != null) {
          c = c.append(placeAt(parent, pt));
        }
      }
    }


So that's ALMOST the same logic. What's different is the existence of the call to "movedPiece" in the first version before the placedAt-ing starts.

The movedPiece does this:

    setOldLocation(p);
    Command c = null;
    if (!loc.equals(p.getPosition())) {
      c = markMoved(p, true);
    }
    if (p.getParent() != null) {
      final Command removedCommand = p.getParent().pieceRemoved(p);
      c = c == null ? removedCommand : c.append(removedCommand);
    }
    return c;


But I don't see anything in there that's raising red flags for this (although it's interesting that SendToLocation version apparently isn't checking to see if piece needs to be "removed" from an earlier parent -- but I don't think we're seeing a pieceRemoved command sequence in our final command string anyway)

So given that both versions seem to ultimately call into Map.placeAt(), what I'm now trying to figure out is why the call from PieceMover seems to generate an AddPiece command (as well as a MovePiece) command, but the SendToLocation call does NOT:

  public Command placeAt(GamePiece piece, Point pt) {
    Command c = null;
    if (GameModule.getGameModule().getGameState().getPieceForId(piece.getId()) == null) {
      piece.setPosition(pt);
      addPiece(piece);
      GameModule.getGameModule().getGameState().addPiece(piece);
      c = new AddPiece(piece);
    }
    else {
      MoveTracker tracker = new MoveTracker(piece);
      piece.setPosition(pt);
      addPiece(piece);
      c = tracker.getMoveCommand();
    }
    return c;
  }


See they both call Map.addPiece() but we're only actually getting an AddPiece command on the PieceMover version. Could we be seeing a problem in (gulp) MoveTracker? That's a rabbit hole I haven't been down yet, I've just been "assuming it works".

Or actually, the TOP part of the "if" DOES manually insert an AddPiece into the command, but the bottom part doesn't. So for some reason PieceMover apparently goes through the top part and the SendToLocation goes through the bottom part? That seems... odd. I would have expected them to both go through the bottom part.

And I'm still left wondering what the AddPiece is "for" and why it is only put in the sequence sometimes.

Any insights will be received with great gratitude!

Brian
Comment 5 Brian Reynolds 2020-05-31 18:58:06 CEST
AddPiece is actually a new "parent" piece being added for a stack. But now why does PieceMover end up adding one and SendToLocation doesn't?
Comment 6 Joel Uckelman 2020-06-15 23:19:45 CEST
Try VASSAL-3.3.1-test-68-g233784ce:

http://www.vassalengine.org/~uckelman/tmp/
Comment 7 Brian Reynolds 2020-06-22 17:50:26 CEST
Agree fixed.