About bug 12554

Development
41

Oh yeah actually from the code I just traced through they would be TOTALLY wrong. Because it isn’t appending command sequences to set the “OldLocationName” and “OldZone” flags – it’s just writing them right there on the fly. Meaning that they DON’T get undone properly (or at all) when a unit is un-done to a different location, and therefore if it then checked its OldZone and stuff they would be incorrect for its current position.

Now mind you, I always thought of OldZone (et al) as things I could only count on being right “immediately after a move”, in other words while processing the key-after-move or in an auto-report string. So IF you’re checking them in a situation where they haven’t just been moved (e.g. you’re looking at them during a right-click menu item, when you haven’t just done a Send To Location on them in this command sequence) then I think you may be using them “off spec”.

So that tells me you’re seeing A Separate Bug (or a Separate Alleged Bug if it turns out you’re using the traits a bit off-spec). The fix here would be we’d need to incorporate the changes to the OldZone (et al) into the Command sequence (so that they would undo properly) rather than just writing them directly.

Bugs 2, Me 0…

Brian

42

Okay, I think I’ve stumbled upon a new bug that may or may not be related to undo. In my test module, I decided to dump OldX and OldY too. I started moving my pieces around from zone to zone before I undo’d, and it failed! At some point, two pieces in adjacent zones stacked. The one in ‘2’ decided to skip ‘3’ where it was supposed to go, and it jumped to ‘4’ to stack with its brother. No undo involved. WTH!? These zones are squares, 200 pixels on a side! The stacking area of effect can’t be 200 pixels, can it?

To check, I turned off stacking in the map, and now it works, and it works even after an undo. In my main module, it always failed after an undo, but perhaps it has to do with this stacking bug, and I just read it wrong. I’ll check in a moment.

So my apologies for leading everyone astray. I was convinced this was an undo problem. Undo may have its hands in it, but I can’t prove it. Sorry. I’ll go now.

43

Okay, I’m back. One last time, I promise. I revisited my main module and disabled stacking in my player map. When tested, it all worked correctly. Undo did not fail. Perfecto.
SOoooo, if you have a test case that fails undo, just for laughs, try disabling stacking and see what happens.

44

The 12554 case ALSO works right w/o stacking. (The fact that the stacking code is WAY more complicated than the non-stacking version makes it ultimately small surprise that it is implicated in all this). Good information though, thanks! Brian

45

Brian, The main difference is that SendToLocation is calling Map.placeOrMerge() which is supposed to be a one-stop shop to move a piece and handle placement/merging/new stack creation etc. on the destination map at the destination map co-ords, rather than having to handle all of that ‘manually’ as the PieceMover does.

The fact that turning off stacking fixes the problem points to placeOrMerge() not handling the new stack creation correctly. On a map with stacking enabled, non-Do-Not-Stack pieces are not allowed to exist in a raw state, they have to be part of a Stack, so any movement to an empty location on a stacking map needs to first create and place an empty Stack at the location, then merge the moving piece into the new empty stack.

46

But Map.placeOrMerge() is the second of the two code-fragments I listed above – it appears to have code near-identical in meaning to the first code fragment. And specifically it looks like it (should(?)) be doing a createStack() and should(?) be appending a placeAt() result to the command, which would mean another AddPiece in the command sequence.

47

I think I’m giving up on this one guys. I’ve already got well over 40 hours of research and debug-tracing into it and I’m not really making headway anymore toward being able to propose some kind of fix.

I’ve left most of the research I’ve done on it attached to the bug, and if someone who understands the underlying mechanics of stacks & commands better wants to have a crack at it then I’m certainly happy to bounce ideas back and forth if useful.

It’s a bug I would really really like to see fixed, because all of the modules I use to play suffer from it, and I was really really hoping to be able to contribute to it, but I just can’t find a way forward.

48

Thanks for your help Brian, you’ve helped narrow down where the problem is.

49

Brian, just as an aside, is there a reason you don’t just turn ‘Snap to defined point?’ on in the irregular grid you’ve created? You don’t need the Send To Location then.

Alternatively, I usually handle these by creating a Rectangular grid instead of an irregular grid, adding a gridnumbering, aligning the Column number with the Track number and setting the Location Format to $column$ and turn on Grid Snapping. Save loads of work.

Rgds.

50

Quick report on the current state of things. I traced the source of the bug up to the DragBuffer.

The “thing” being actually moved is a ReportState object.

The very first move by the user puts the wrong thing into the DragBuffer, it puts the Stack there, which contains the ReportState. It looks like things work fine if the actual ReportState is in the DragBuffer, and not the Stack that contains the ReportState.

If I replace this Stack in the DragBuffer with the ReportState contained in the Stack, then everything seems to work. And this has to be done only once, afterwards all further move and undo actions work perfectly fine.

Example, the log is from the method DragBuffer.add() which fires when the user clicks and holds the counters:

  • The very first move by the user, observe how the DragBuffer contains a Stack:
2020-06-01 09:52:28,048 [0-AWT-EventQueue-0] DEBUG VASSAL.counters.DragBuffer - dragBuffer.add: VASSAL.counters.Stack@6db2a5c0[Blockade Marker]
  • I correct this during debugging by just replacing the Stack in the dragbuffer with the ReportState object that is inside the Stack
  • Then comes the undo which works fine
  • Then I can move the counter forward and undo, as many times as I want, every time the DragBuffer contains the correct object and nothing needs to be fixed anymore, observe how the DragBuffer contains the ReportState:
2020-06-01 09:55:21,425 [0-AWT-EventQueue-0] DEBUG VASSAL.counters.DragBuffer - dragBuffer.add: VASSAL.counters.ReportState@51a27fc[name=Blockade Marker,blablamorestuff]

Next questions I want to solve:

  • who puts the wrong object into the DragBuffer for the initial move, and why
  • why, once fixed, do the moves 2-N work correctly, why is the correct object put into the DragBuffer, what is different between move 1 and moves 2-N
  • is there any unwanted side effects by putting the ReportState into the DragBuffer instead of the Stack

Also, I browsed the other posts in this thread, sorry for going a bit over the edge, I have very high standards where code automatically deteriorates if it is not cleaned up and refactored into a manageable state, and thanks a lot for the input from whoever else analyzed this issue, and I completely understand why fixing this in the form of a “dirty hack” is better, safer and more backward-compatible than a full heart surgery. Once I narrowed down the source of the bug and found a way to fix it, I will present the solution and then we can still think about how we package it, how much we change and how much we leave as it is. I think I am almost there, I can already fix the bug when running inside the debugger, now I just have to follow the call hierarchy and find the exact source of the bug.

One other observation I have made - Stacks seem to be created dynamically whenever they are needed, and this creating of the Stacks and the whole merging functionality seems to work well (enough), and this Stack stuff is not related to the undo bug.

51

Hi Flint,

I have got to about the same place as you.

As GamePiece’s exist as a stack of Decorator’s, a GamePiece always appears to be the Decorator that happens to be ‘outermost’ or ‘on top’. That particular unit just happens to have a ReportState trait as the last defined trait. Another unit will appear to be a different Decorator.

The fact that the Stack is going into the DragBuffer instead if the GamePiece is not wrong. If you select and drag an entire Stack, then the stack goes into the Dragbuffer. Expand a stack a select single piece and drag it, and that single piece will go into the DragBuffer. Raw pieces do not exist on maps unless stacking is disabled, or they have a Do Not Stack trait with stacking disabled.

Using Brian’s undobug module, I have been trying to trace the exact commands generated by a move+apply key and the exact commands when undoing that Command. A good place to trap this at HybridClient.sendToOthers().

Something seems to be going wrong when the initial move is being undone, the piece is not being linked back into a Stack at the starting position correctly and is being left at the intermediate drop. The final MovePiece of the undo seems to be going to the correct location, but the piece doesn’t make it.

If you see strings like ‘1591009191005’, these are piece id numbers, basically a guid assigned to each Piece as used to identify it when the piece is serialised. Stacks also have these id’s.

Yes, Stacks are created as required. A Stack of 0 pieces may exist on the map, but it will be ignored (unless you move a piece on to it) and will not be saved to a save game.

52

Yes the last MovePiece of the undo tries to move an empty stack, if I manually move the actual counter, the ReportState object, into that stack, then everything works fine.

I am still not sure about the DragBuffer though, if the DragBuffer only contains the ReportState object then everything also works fine, the last MovePiece of the undo then moves a stack with the ReportState inside.

I will look further into this when time allows.

The IDs, yes I have seen them in use and used them myself, at some point I had to write down the IDs for a Stack and a ReportState object, then get the objects for them using the static GameState.blablasomething.getById().

53

Hi Brent –

I realize this particular example could be easily corrected with snap-to, but I alas have many many applications where that’s not possible. The “UndoBug” module is really just a simplistic stand-in for the case.

Just by way of example, in For The People there are three main types of unit that can be in a space (army, strength-point, and generals) but huge numbers of pieces can end up in the space. So it’s super helpful if the module can “auto-arrange” the stacks – basically as units arrive in the “space” it helpfully puts the generals all in one pile, the armies all in another pile, etc, and splays them helpfully so you can see just enough of each counter stack to know exactly what’s going on). It all “should” work, completely vanilla Vassal stuff, and it does, except for… undo.

The most poignant Paths of Glory example… I need units (armies, corps) to snap to their board spaces – not just for neatness but because armies arriving from the reinforcement card otherwise won’t be properly added to the “stack” and therefore can get lost. But I need the markers (trenches, control markers, fort besieged) and stuff to NOT snap. I suppose that now-knowing-about-this-bug I could look for some way to custom-class that, but at the time I was doing all this it was before custom classes and it all seemed like straightforward vanilla Vassal – and it worked! (And when I eventually noticed Undo wasn’t working I of course didn’t associate it with because-I-did-this-vanilla-Vassal-thing, I just thought Undo was probably broken in some random way).

It sounds from your recent message that you’ve gotten to about the same point I was able to get to (although with the more sophisticated analysis that it isn’t being added back into a stack). Trying to figure out how to get it to BE added into a stack was where I kept losing the thread.

Brian

54

This is it. The fix was to never add the whole Stack to the DragBuffer, this fixed the whole undo chain that follows the move. Adding the whole Stack to the DragBuffer contradicts the model anyways, either the whole stacks are moved around, or the contents of the stacks are moved and stacks are created dynamically. Doing both is asking for trouble.

The fix is here: github.com/vassalengine/vassal/pull/5 , have a look.

55

Another question: “a map with stacking disabled”, what exactly is this, I assume it is some kind of setting for a module or a single map of a module which means that there can be no stacks at all on the map, only single counters?

Can anyone recommend a module that has such a map? I have only used 2 modules so far, both have stacks on their map, so I guess these maps are not of the type “map with stacking disabled”.

56

In every Map Window there is a tickbox for disabling stacking (in the same place you configure horizontal/vertical offsets for pieces in stacks).

[attachment=0]Screen Shot 2020-06-02 at 10.12.57 AM.png[/attachment]

57

Flint – Great job! Thanks SO much! Brian

58

So exactly what did you fix?

I ask because while exploring my own undo problems, I discovered that “undo” was actually only inflaming a different bug. The top bug I see involves stacks of pieces and moving those stacks of pieces.

59

Yes, that. Before the fix, the whole stack was moved. The fix was to not move the stack itself but just its contents, and relying on the code to dynamically create new stacks, which seems to work perfectly fine.

I am not fully done with the testing though, still looking for a module that has a map with stacking disabled. Does anyone know such a module? I am a bit afraid of making my own test module for that, I would much rather use a “real” module that is written by someone who knows how to use the module editor.

60

Shilinksi – The bug under direct consideration here occurs when the player drags a stackable piece (or a stack of stackable pieces) somewhere, and then in processing the “apply-on-move” keystroke for that piece (or pieces), a Send-to-Location trait moves the piece or pieces to a different location.

Ah and I see Flint and I posted at the same time.