Fix offset placement for Place Marker & Replace with Other

[riverwanderer]

… This change explicitly bypasses Place Marker's stack operations when an offset is specified.

[riverwanderer]

This PR makes PlaceMarker comply to the documented behaviour "Any value other than zero will prevent the new piece from stacking with the original piece.".

[jonathanrwatts]

Is this going to cause a problem if the offset causes the new piece to overlap an existing stack? You now have a supposedly stack-able piece overlapping a stack without being added to the stack (I have no idea if that is actually a cause for concern or not)....

[riverwanderer]

Is this going to cause a problem if the offset causes the new piece to overlap an existing stack? You now have a supposedly stack-able piece overlapping a stack without being added to the stack

A fair question, thanks. The documentation effectively advises that's exactly what will happen for the piece's current stack, so it would seem consistent for that same behaviour to apply to any stack. I do have a nagging question as to whether SnapToGrid negates that, however.

Going deeper, another question might be, why can't PlaceMarker apply its Stack setting in combination with the Offset action. My novice conclusion is that for PlaceMarker to do that would require a lot of extra overhead (code) to handle remote placement. By which I mean, PlaceMarker knows about its piece's own stack but nothing about other locations.

One solution for the module designer is to use a Movement trait to wobble the piece at its new location and also do any positioning with the stack. This seems a reasonable accommodation to me.

[BrentEaston]

Is this going to cause a problem if the offset causes the new piece to overlap an existing stack? You now have a supposedly stack-able piece overlapping a stack without being added to the stack (I have no idea if that is actually a cause for concern or not)....

This begs the question of whether we actually want to allow this behaviour, or whether the documentation is needs to be corrected. The title of the original issue is actually incorrect. Place Marker does not actually 'Ignore' an offset, it is just that a small offset that does not displace the counter far enough away to be out of range of counter or hex snapping gets over-ridden. In EXACTLY the same way that drag'n drop of a counter in that same location would. This PR is creating a special case to over-ride standard behaviour.

What is the actual use case for having a slightly offset piece, which generates 2 very close stacks? This can be achieved using Game Piece Layers or Non-stacking pieces.

[BrentEaston]

Querying the need for this PR at all. Should it be a documentation fix? See extended comment above.

[riverwanderer]

Querying the need for this PR at all. Should it be a documentation fix? See extended comment above.

Not disputing that some small offsets maybe should behave like a drag and drop, but the issue in question manifests regardless of the offset, providing the piece is Stackable it WILL stack at the original location regardless.

[BrentEaston]

Not disputing that some small offsets maybe should behave like a drag and drop, but the issue in question manifests regardless of the offset, providing the piece is Stackable it WILL stack at the original location regardless.

Ah, I had not quite realised the seriousness.

So currently, if the marker and the source stack are all stackable (lines 240-284), then the calculated destination for the piece (p) is completely ignored and the marker is inserted into the same stack as the source, There is some special case code to handle top/bottom/up/down.

The second else (lines 285-295) is activated when either the source piece is non-stacking, or the pieces cannot be merged. It does a placeOrMerge(), to the correct destination, which may find a valid Stack there, in which case it has some special case code to place the piece at the bottom of that stack if Bottom or Below was selected.

The final Else (lines 296-298) is activated when we have a non-stacking marker. We just place it directly at the destination. In you solution, this path will be taken by all markers with an offset specified.

I can see a couple of issues with your proposed solution.

1. Your change means that markers with an offset will be placed using placeAt(), not placeOrMerge(), I think stackable markers should always use placeOrMerge(). Using placeAt() will cause two issues. Firstly, we potentially end up with overlapping stacks that can't be created in any normal way via be created with drag and drop. Secondly, I believe using placeAt() will result in a subsequent stackable marker being placed in the same offset location forming a second unrelated Stack in the same location. Using placeOrMerge() should do the equivalent of dropping the new marker on that new location, letting it find a nearby Stack if one exists, or creating a new Stack if not.

2. When using an offset, the top/bottom/above/below really becomes irrelevant unless the piece happens to merge with the same Stack as it's source. In your current solution, the top/bottom/above/below option is ignored anyway when an offset is specified. I think this is fine. but the option should probably be disabled when you select a non-zero offset. And the doco improved.

[riverwanderer]

Thanks @BrentEaston. With your guidance, I now wonder if step 1 is actually causing the problem.

Would this simplified logic work better (assume rotation matching preserved)?

If the marker is non-stacking, place it directly at the destination (i.e. the current step 3).
Else
 Determine if marker and source piece can be merged and store marker's X/Y
 Do a placeOrMerge(), to the correct destination. 
 If marker is at the stored X/Y and marker and source piece were mergeable; process stacking options per current step 1
 else
    if the marker is part of a stack & Bottom or Below* option is set process those options per current step 2

*I question if the Below option should have any meaning away from the source piece's stack but as this is doubtless now a backward compatibility requirement, it's enough to clarify in the documentation if need be.

[riverwanderer]

Not working yet for Above & Below within parent stack.

[BrentEaston]

I am liking the direction you have taken. I was thinking it could be simplified.

I am trying to think what the impact will be.

1. Where the offsets are both currently 0, there should be no impact.
2. Where offsets have been specified, there is now going to be different behaviour because the offsets are currently being ignored. Modules may have this set incorrectly, perhaps as an experiment that caused no effect, so the non-zero values where left in. If the offsets are large, the marker will suddenly start jumping somewhere unexpected. If the offsets are small, then above/below will stop working and become top/bottom instead.

How many modules are likely to be effected do we think?

We could add a compatibility preference, but I am not a huge fan of these as a) It's not an obvious fix if the module suddenly stops working and b) Each player has to do this in their own preferences.

Another way to handle this is to add a version number to the trait, and add code so that the old behaviour continues until the module is next edited and re-saved. New traits will be always use the new behaviour. This doesn't stop the 'surprise' happening, but it means it happens when the developer next releases an update, not when Vassal updates.

[riverwanderer]

Trying to implement my proposed "simplification" was educational. Now that I can see that the Above/Below options rely on the new piece remaining in the stack with the parent piece, I understand why the code is structured as it is and that there's extra complexity to add back in if I continue down that path. So, I've reverted to my original solution but got rid of the offset check at the second else (lines 285-295), which I can see was an error on my part. The impact of this on trait outcomes is just the one change highlighted below:

1. New marker that can remain in its parent stack will do so. Offsets must be zero. All four placement options work.
2. Any other stackable situations start by moving the marker to the Offset, potentially stacking. Above/Below equate to Top/Bottom.
3. Non-stackable markers will be placed as now. Placement options ignored.

@jonathanrwatts

Is this going to cause a problem if the offset causes the new piece to overlap an existing stack?

• I think this correction addresses your point too.

I've also updated the Place Marker & Replace with Other help sections to clarify the impact of specifying an Offset and its effect on the Place Marker options.

I stripped out and modified a module to test both traits in the various combinations under consideration:
PlaceMarkerStack Test.zip
(updated: fix to test pieces, buttons to set offset values)

@BrentEaston - this still leaves the compatibility gap that you mention. How would I go about trait versioning? Is there an example I can look at?

[BrentEaston]

this still leaves the compatibility gap that you mention. How would I go about trait versioning? Is there an example I can look at?

Have a look at ActionButton and ReturnToDeck.

[riverwanderer]

Continued via #13801.