Ah, interesting that it works with Send To Location, I didn’t think of using the X/Y offset option. I should be able to Fix the ‘Move Fixed Distance’ issue by making it do the same thing as the ‘Send to Location’. They are both essentially doing the same thing.
Thus spake Brent Easton:
There are some other approached to this problem.
This problem is being caused by a ‘Apply after Move’ key causing a drag
and dropped unit to move again before the first move has completed.Are we aware of any other Undo problems that are NOT caused by this? I
suspect not.
I have a collection of messages dealing with undo problems (and also at
least one test module) them in my inbox. I don’t know if they have the
same cause. I was already planning to (attempt to) deal with my mail
backlog today; I’ll see about digging them up.
–
J.
More analysis results.
The main “move piece” magic happens in Map.placeAt().
For the regular move that the user initiates by drag and drop, the code in PieceMover.movePieces() does the handling and calls Map.placeAt() to do the actual move.
The undo move OTOH, uses the command MovePiece and its executeCommand() method, which does roughly the same as PieceMover.movePieces(), and calls Map.placeAt() to do the actual moving.
And if I understand correctly, the same MovePiece.executeCommand() is also used in a network scenario, where the clients other than the initial user move the piece by going through MovePiece.executeCommand().
So there are two completely different ways of doing the same thing, one in PieceMover.movePieces() and the other in MovePiece.executeCommand(). This is asking for trouble. A clean design would be for instance, when PieceMover.movePieces() would only build the MovePiece command, and not do any actual moving. Then all of the actual moving would happen in MovePiece.executeCommand().
Another issue is that Map.placeAt() does two different things, it does the actual moving, and also creates the MovePiece command and returns it. In the PieceMover.movePieces() scenario, this command is used for logging and for sending it to the other clients. In the MovePiece.executeCommand() scenario, this command is discarded.
I only did some refactoring so far, split up the long methods into smaller pieces in order to understand what is going on.
Is Map.placeAt() part of the public API? Do some or many of the modules use that method? Because the idea right now is to decouple the 2 functions, the creation of the command by placeAt() and the actual command execution.
Oh you’re right, that’s of the old-school “(a) do stuff, (b) try to package up a command that would also do the stuff, (c) send it” category of stuff. Definitely better structurally if everybody executes-from-command. Though in this case I don’t think it would be sufficient to fix the bug. But possibly a necessary step along the way.
And another, only slightly related question, what exactly is the public API, is there a way to find out what all the modules use? I am no license lawyer, but if Vassal is open source, shouldn’t all the modules that use the Vassal API also be open source? Where is the code for all the modules? I have even seen one online store that sells Vassal modules for money, is that legally possible without making the modules’ sources public?
And another question, related to the bug - I understand a single Counter, and Stack, but what is a Deck exactly?
Hey if you’ve got that compiling and stuff then I’d be fascinated to know if doing a remove piece before the move and add does anything to the repro. Because starting with a remove should make the undo end with an add.
A deck is a different flavor of collection of pieces, originally built with decks of cards in mind, but occasionally useful (or at least used) for other pieces.
There is no Public API as such, module designers (generally) do not write code. Modules are not code, they are essentially descriptions of behaviours that are implemented at run-time by the Vassal engine.
Rule 2 of Vassal development is that every bug is a feature. No matter what bug you want to fix, there will be a module out there that depends on that bug existing to work correctly, so we have to turn the ‘bug’ into a ‘feature’ that can be turned off by a preference.
I STRONGLY suggest you don’t try to refactor the current PieceMover, we will not accept major surgery to such a core component, it’s just too risky. Instead, write a completely new PieceMover that extends the existing PieceMover and over-rides all of its methods. You can test it by editing the Buildfile and swap out the existing PieceMover reference to your new PieceMover. If we decide it is a goer, then we add it as an option ‘Use Advanced Piecemover’ that will default to Yes, but users will be able to turn it off to use the old PieceMover.
Again, as this particular problem only manifests when an Apply After Move key initiates a second move ‘inside’ the first move, I suspect it will be far easier to just make sure the ‘Apply after move’ command is appended after the first move has completed appending all of its commands, instead of being inserted into the middle of the Command sequence.
A Deck is a special case of a Stack that has loads of additional dodgy code to try and emulate a Deck of Cards.
Try moving this
// Apply key after move to each moved piece
if (map.getMoveKey() != null) {
applyKeyAfterMove(allDraggedPieces, comm, map.getMoveKey());
}
out of Map.movePieces(), into Map.performDrop() after the line
final Command move = movePieces(map, p);FWIW, as far as I can tell, the entire apply-after-move sequence IS already being appended at the end of the command sequence. The original move item already gets run first when I trace through. Unless I’m missing something?
That’s interesting, because as yet, no-one has actually put their finger on exactly where this is going wrong. Yes, we are seeing some strange Command sequences, but what is it about the Application of the Move After Key that is causing the problem? Presumably this is a Send To Location Trait, so we may be back to the problem being in Send To Location, rather than the PieceMover as such.
No major surgery, instead leave the old way and put the new way at its side and allow user to select between old and new way: yes this is what I’m aiming for, this is what I know as common practice in dealing with legacy code.
Now, forgive me for being direct.
Too risky: yes, I understand why the code deteriorated so much, instead of crying about how bad things are and making plans for a complete rewrite in this glorious so-much-better-than-java language, it would have been your job to DO major surgery, and if you don’t get rid of this “boohoo it’s too risky we are shitting our pants” mindset the exact same deterioration will happen to your new Vassal.
Stop messing around, just try this and that: if you know how to solve it, or want to try a certain fix, why not do it yourself, it is open source code. I haven’t understood the problem yet, and I am not ready to solve it at this point. A wise man once said, in order to solve a problem, one has to understand the problem first. This is why I am doing the refactoring, the major surgery, I don’t do it for keeping the results, for now I just do it to understand the problem. Once the problem is understood, we can still talk about how to solve it and how to package the fix.
“No-one has actually put their finger on exactly where this is going wrong” - either I am going to put a finger on this, or I will not fix this bug at all.
So effectively move the “apply key” part from before tracker.repaint() to after tracker.repaint() and hope this is a map repaint problem? And the clean way of doing this would be to simply move the tracker.repaint() 6 lines up and put it before the “apply key” part, not to extract the whole “apply key” into the caller method.
I even tried it, nothing changed. Any more shots in the dark or can I proceed with a proper analysis? It is hard and time-consuming as it is, the code seems to not have been touched since Java’s pre-1.2 times, since 1998, I just almost got a heart attack when I saw what kind of manual array acrobatics the Stack does with its contents. Was is that hard to replace this with a simple ArrayList which has existed for 22 years…
Ah Well. I’ll just shut up now. You’ve been working on the project for what, 5 minutes and have it all nutted? Good luck to you.
I agree - closest I’ve come so far was tracing through and discovering that upon undo the commands are being processed in the apparently-correct order – that the piece is being “moved back” to the correct set of coordinates, but for some reason doesn’t appear to us as being back.
Which does suggest that Send To Location could be “missing something” – for example it isn’t doing an “add piece” the way the PieceMover does. Add Piece (and here is where my understanding of the architecture isn’t complete enough yet) seems to relate to the stacking functions, and so the fact that it’s being left out certainly raises a suspicious flag.
Hopefully I will have time to dive in another around before the weekend is done.
Brian
Thus spake Flint1b:
No major surgery, instead leave the old way and put the new way at its
side and allow user to select between old and new way: yes this is what
I’m aiming for, this is what I know as common practice in dealing with
legacy code.Now, forgive me for being direct.
Too risky: yes, I understand why the code deteriorated so much, instead
of crying about how bad things are and making plans for a complete
rewrite in this glorious so-much-better-than-java language, it would
have been your job to DO major surgery, and if you don’t get rid of this
“boohoo it’s too risky we are shitting our pants” mindset the exact same
deterioration will happen to your new Vassal.
This “deterioriation” you’re talking about never happened. Going on about
it makes it hard to take seriously what you’re saying. As I have already
explained, we inherited the code you’re referring to, and it was mostly
written that way to begin with. What you’re seeing now is the where we’ve
reached after many, many attempts to rectify things. You didn’t see what
it looked like in 2006.
I was extremely reluctant to come around to the idea that we needed to
start over. There are old posts in the forum where I explained a plan for
doing model-view separation. Then I tried it, and realzied that keeping
things working while doing that was far, far more work than writing
something new with tests validating that it does what it is intended to
do.
Furthermore, there’s no way to ensure you haven’t broken existing modules,
saves, and logs when you don’t have a spec for what anything is intended to
do. We’ve fixed many bugs over the years without that because some bugs are
so obviouslly wrong that they violate any reasonable intent, so we don’t
need to know exacty what the actual intent was in order to fix them.
On a few occasions, we’ve fixed something to find that so many modules
dependend on it being broken that we had to leave it as it was. You might
be surprised by how angry users can be when their saves and logs stop
working.
Stop messing around, just try this and that: if you know how to solve
it, or want to try a certain fix, why not do it yourself, it is open
source code. I haven’t understood the problem yet, and I am not ready to
solve it at this point. A wise man once said, in order to solve a
problem, one has to understand the problem first. This is why I am doing
the refactoring, the major surgery, I don’t do it for keeping the
results, for now I just do it to understand the problem. Once the
problem is understood, we can still talk about how to solve it and how
to package the fix.“No-one has actually put their finger on exactly where this is going
wrong” - either I am going to put a finger on this, or I will not fix
this bug at all.
I want to be clear that we’re talking about the same thing here:
Change whavever you need to change while debugging. It’s local to you. If
refactoring some class helps you figure out what’s wrong, great.
We have to be more conservative about what goes into releases. It is a
massive waste of time and goodwill to put out releases which break things
for users. If we’re going to do refactoring which could affect module
behavior and have that go into master, we need to be very sure that it
won’t cause a mess.
And, in general: Chill a bit. It’s not necessary to be so aggro. You
think we’ve done a bad job. I’ve explained that we’re a long way ahead
of where we started. We don’t agree on these points and I suspect that
we won’t. That need not prevent us from fixing this bug.
–
J.
Possibly relevant other Undo issues:
https://forum.vassalengine.org/t/undo-problem-captured-in-log-file/7622/1
vassalengine.org/tracker/sho … i?id=12951
https://forum.vassalengine.org/t/test-builds-for-3-3-0/10056/91
https://forum.vassalengine.org/t/test-builds-for-3-3-0/10056/170
https://forum.vassalengine.org/t/test-builds-for-3-3-0/10056/195
Several of these links are for a problem which was reported against some earlier 3.3.0 test builds C&C which had a fix for Bug 12538 that had some unintended consequences for Undo. (I still don’t grasp why these changes cause a problem, as they look right to me.)
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
I’m coming to realize that the “AddPiece” we’re getting is actually the creation of the “parent” piece for a stack. But I’m looking at those first two code fragments above and baffled why one of them is creating a parent for a stack and the other isn’t?!?!
This is the second time I’ve composed a message that disappeared in the ether. I have some information that may be helpful.
Earlier I had reported my version of the undo bug where I send-to pieces from zone to zone. My problem is not at undo time. Mine is when I go forward. The undo poisons later behavior. But it might provide a clue. I created a small 6k mod that 100% shows a problem if anyone cares to see it.
In my mod, I have 6 numbered zones. If I press a button, pieces advance to the next zone, so a piece in ‘2’ goes to ‘3’. I must have at least 2 pieces in zones to have a problem.
After the undo, a piece in ‘2’ is supposed to go to ‘3’, but instead it jumps to ‘4’, joining its brother who came from ‘3’. I query the piece, and it tells me its OldZone was ‘3’. No, it’s supposed to be ‘2’. OldZone is wrong.
So I surmise the undo caused the bad piece to use the other piece’s OldZone. Since state variables tend to be clustered in classes and structures, it suggests to me that not just OldZone is wrong. I surmise that during the undo, the bad piece’s state got linked to someone else’s. If true, then my observation is just a tip of an iceberg, and it be a door to other undo problem explanations.
If it were me (and I’m retired and keep bees), I’d run it in a symbolic debugger and watch specifically what happens.
Oh, but meanwhile I’m getting even more convinced that it is a Bad Thing (or at least “an Unintended Thing that is now causing this problem, and we can all begin our Futile Prayers That Some Modules Won’t Turn Out To Depend On This”) that when a piece move runs through an Undo sequence that there is NOT a new “stack parent” being created to receive it back at its old position.