In a situation where first piece has been moved to point x,y by using placeOrMerge it is placed as a single GamePiece and not a stack.
When a second piece is moved to the same point by using placeOrMerge, a new stack is created, and an AddPiece command recorded for the stack followed by a MoveCommand for the second piece.
On undo this chain is reversed to opposite MoveCommand for the second piece and RemovePiece command for the stack. And this RemovePiece command behaves so that it removes the stack from game, but there is no command in the undo sequence that would place the first piece again in the original point, it is removed with the stack and good bye.
The developers receive the fora by email and and attend to all equally. The reason you have not got a response yet is undoubtably because everyone is busy.
Thanks very much for tracking down the source of this bug. If you can suggest a fix for the bug, it would be greatly appreciated.
I have been unable to reproduce the problem. Can you describe the exact conditions under which it occurs?
I’m not sure that line 661
fixedParent = createStack(fixed, true);
is the problem because this just creates a free standing Stack which in then passed to an AddPiece command to Add it.
One thing that conxerns me is that lines 662-663 do a GameState.addPiece(fixedParent), then Map.addPiece(fixedParent), but the AddPiece command it creates to reflect this to the logfile/opponent only does a GameState.addPiece(piece), it does not call Map.addPiece(piece).
So it looks to me like line 663
fixed.getMap().addPiece(fixedParent);
is not being reflected in the logfile. If you look at the Undo command AddPiece, it calls both GameState.removePiece(piece) and Map.removePiece(piece).
I may be barking up the wrong tree, but it looks suspicious. Which is a worry if AddPiece has a bug!
I’m away for a couple of days, good luck with this.
As I wrote earlier the problem will occur when you will undo a movement which, while executed, resulted in moving a GamePiece to a given point where another GamePiece (not a Stack) was present.
The method StackMetrics.merge has to create a Stack which will hold the GamePiece previously occupying the place and the newly moved GamePiece.
Since the movement of the GamePiece already in place to the newly created stack is not recorded as a command, it cannot be undone. Only the creation of the Stack is and of inserting the moved GamePiece into it. Those two commands during undo operation will remove the GamePiece that moved into the Stack and remove the Stack. And there is nothing that would restore the original GamePiece to it’s original location (the same Point but not inside a Stack, rather standalone).
Look at those lines:
fixedParent = createStack(fixed, true);
GameModule.getGameModule().getGameState().addPiece(fixedParent);
fixed.getMap().addPiece(fixedParent);
comm = comm.append(new AddPiece(fixedParent));
index = 1;
As you can see fixedParent is a new Stack object and the command AddPiece deals with this new object. Inside createStack there is new Stack(fixed) constructor which calls Stack.add(fixed), adding the fixed GamePiece object to the fixedParent Stack object, but this add is not covered by a corresponding command. This causes the fixed GamePiece object to become MIA on undo.
Luckily createStack in it’s two variants is only called in two places in the code: StackMetrics.merge and PieceMover.movePieces. Propably there is no bug in moving using PieceMover but since I wrote my own which moves pieces using Map.placeOrMerge (which in turns uses StackMetrics.merge) the bug occurs to me more often. And why there is no bug in PieceMover? I think it is because it deals differently with that matter:
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));
}
}
Here, both the dragged piece is “placed at” p, as is the newly created Stack. And placeAt uses MoveTracker to properly record that the dragged piece was not in stack before completing movement.
I will try to change the contents of StackMetrics.merge to something similar.
My first attempt to fix that problem, replace the lines 661-665 with:
MoveTracker fixedTracker = new MoveTracker(fixed);
fixedParent = createStack(fixed, true);
comm = comm.append(fixedTracker.getMoveCommand());
comm = comm.append(fixed.getMap().placeAt(fixedParent, fixed.getPosition()));
index = 1;
and now it works
This fixes the bug for all occurences of Map.placeOrMerge and it’s a lot of them - FreeRotator, Pivot, DrawPile, Clone, DynamicProperty, BasicPiece, SendToLocation - all use placeOrMerge!
Of course this would happen only in rare cases: when the first GamePiece was placed in an empty “location” using placeOrMerge AND then the second GamePiece again in the same location using placeOrMerge creating a Stack in bad way AND a Player would undo that move.
Since I have created my own PieceMover which used that bugged method for movement, it was more common for me and thus I was able to find it.
Yes, but you neglected to mention what sort of piece are required to achieve having a non-stacked single gamepiece. I have been trying various combinations of non-stacking pieces, but I cannot reproduce this error in order to verify your solution.
Again, Could you please give exact details of how to reproduce this problem including what sort of pieces you are using.
I cannot reproduce the problem. Normal, stacking GamePieces in the average module do not exist as single GamePieces not in stacks and the undo function works fine.
Can you post a module and a logfile showing the problem.
GameModule.getGameModule().getGameState().addPiece(fixedParent);
fixed.getMap().addPiece(fixedParent);
I think those commands are executed inside Map.placeAt invoked in next line:
comm = comm.append(fixedParent.getMap().placeAt(fixedParent, fixedParent.getPosition()));
in my version, lines 1980-1981:
addPiece(piece);
GameModule.getGameModule().getGameState().addPiece(piece);
so they are not needed, that’s why I had:
comm = comm.append(fixed.getMap().placeAt(fixedParent, fixed.getPosition()));
in place of those 3 lines.
Also, how do you track now that the fixed was added to fixedParent in createStack? That was what I used MoveTracker for. Anyway, if it works now, I will not argue
On the other hand, I’m a bit worried that the same action (placing a piece on map in given point) is coded differently in two places - Map.placeOrMerge (using StackMetrics.merge) and PieceMover.movePieces. Such basic action should be coded in only one place and used by all methods that move pieces. Map.placeOrMerge is a good candidate for that.
Yes, that executes the addPiece() commands in your client, but does not create a Command to cause them to be executed in other players clients. Remember, anything that changes anything in Vassal must be executed both in your own client, but also encapsulated in a Commands a transmitted to the log and/or other players.
If you look at the Map.PlaceAt() command, you can see it generates an AddPiece or a MovePiece command as appropriate.
Yes, Map.placeOrMerge is the main entry point that should be used to place pieces onto the map.
PieceMover.movePieces is specifically used by the Drag and Drop piece mover and handles the DragBuffer and other stuff that doesn’t apply to programatic movement.
I understand very well that you need to have a command for everything, that’s why I think only .placeAt will be sufficient since it sets the map and adds the piece to the game AND creates the command.
Anyway, glad this has been fixed, I will now be able to release the module without fear that on undo pieces will start to disappear. When do you plan to release 3.1.7?
Probably never. We might release 3.1.7 soon, though.
I’ll try to get together all of the patches since 3.1.6 tonight, and
then post a build for people to check.
(A lot of one-time things, like proofreading my wife’s dissertation, have
been eating up all of the time I usually spend on VASSAL in the past two
weeks. I’m guessing that this is not going to get a lot better until the
middle of August, when I’ll have handed my own dissertation to my committee.)
