Fix offset placement for Place Marker & Replace with Other

vassal-app/src/main/java/VASSAL/counters/PlaceMarker.java

@@ -91,11 +91,13 @@
 import static VASSAL.counters.MatCargo.CURRENT_MAT_OFFSET_Y;
 
 /**
- * This Decorator defines a key command to places another counter on top of this one.
+ * This Decorator defines a key command to places another counter on the same map as this one, normally on top
+ * but stack adjustment and offsets may be specified too.
  */
 public class PlaceMarker extends Decorator implements TranslatablePiece, RecursionLimiter.Loopable {
   public static final String ID = "placemark;"; // NON-NLS
   public static final String PARENT_ID = "ParentID";
+  public static final int PLACEMARKER_VERSION = 1;
 
   protected KeyCommand command;
   protected NamedKeyStroke key;
@@ -118,6 +120,7 @@ public class PlaceMarker extends Decorator implements TranslatablePiece, Recursi
   protected static final int BELOW = 3;
   protected int placement = STACK_TOP;
   protected boolean above;
+  protected int version;
 
   protected String descString;
   protected boolean copyDPsByName;
@@ -175,7 +178,8 @@ public String myGetType() {
       .append(placement)
       .append(above)
       .append(copyDPsByName)
-      .append(ParameterListConfigurer.encode(parameterList));
+      .append(ParameterListConfigurer.encode(parameterList))
+      .append(PLACEMARKER_VERSION);
     return ID + se.getValue();
   }
 
@@ -237,10 +241,12 @@ protected Command placeMarker(boolean clearParentId) {
       p = getMap().snapTo(p);
     }
 
-    if (m.getStackMetrics().isStackingEnabled() &&
-        !Boolean.TRUE.equals(marker.getProperty(Properties.NO_STACK)) &&
-        !Boolean.TRUE.equals(outer.getProperty(Properties.NO_STACK)) &&
-        m.getPieceCollection().canMerge(outer, marker)) {
+    if ((version == 0 || (xOffset == 0 && yOffset == 0)) // Offset negation fix is backward compatible.
+            && m.getStackMetrics().isStackingEnabled() &&
+            !Boolean.TRUE.equals(marker.getProperty(Properties.NO_STACK)) &&
+            !Boolean.TRUE.equals(outer.getProperty(Properties.NO_STACK)) &&
+            m.getPieceCollection().canMerge(outer, marker)) {
+      // The new piece will remain in the same stack as the parent piece
       GamePiece target = outer;
       int index = -1;
       Stack parent = getParent();
@@ -283,6 +289,8 @@ protected Command placeMarker(boolean clearParentId) {
       m.repaint();
     }
     else if (m.getStackMetrics().isStackingEnabled() && !Boolean.TRUE.equals(marker.getProperty(Properties.NO_STACK))) {
+      // The new piece is stackable, albeit not on the parent piece or at an offset from that piece.
+      // The Above and Below options will fallback to Top and Bottom respectively.
       c = m.placeOrMerge(marker, p);
       final Stack parent = marker.getParent();
       if ((parent != null) && (parent.pieceCount > 1)) {
@@ -294,6 +302,8 @@ else if (m.getStackMetrics().isStackingEnabled() && !Boolean.TRUE.equals(marker.
       }
     }
     else {
+      // The new piece is not Stackable.
+      // The Above and Below options will have no effect.
       c = m.placeAt(marker, p);
     }
 
@@ -544,6 +554,7 @@ public void mySetType(String type) {
     copyDPsByName = st.nextBoolean(false);
     gpidSupport = GameModule.getGameModule().getGpIdSupport();
     parameterList = ParameterListConfigurer.decode(st.nextToken(""));
+    version = st.nextInt(0);
   }
 
   @Override
@@ -785,8 +796,8 @@ public String getState() {
     @Override
     public String getType() {
       final SequenceEncoder se = new SequenceEncoder(';');
-      se.append(commandInput.getValueString());
-      se.append(keyInput.getValueString());
+      se.append(commandInput.getValueString())
+      .append(keyInput.getValueString());
       if (pieceInput.getPiece() == null) {
         se.append("null"); // NON-NLS
       }
@@ -797,18 +808,19 @@ else if (markerSlotPath != null) {
         final String spec = GameModule.getGameModule().encode(new AddPiece(pieceInput.getPiece()));
         se.append(spec);
       }
-      se.append("null"); // Older versions specified a text message to echo. Now performed by the ReportState trait, // NON-NLS
+      se.append("null") // Older versions specified a text message to echo. Now performed by the ReportState trait, // NON-NLS
                           // but we remain backward-compatible.
-      se.append(xOffsetConfigEXP.getValueString());
-      se.append(yOffsetConfigEXP.getValueString());
-      se.append(matchRotationConfig.getValueString());
-      se.append(afterBurner.getValueString());
-      se.append(descConfig.getValueString());
-      se.append(slotId);
-      se.append(placementConfig.getSelectedIndex());
-      se.append(aboveConfig == null ? "false" : aboveConfig.getValueString()); // NON-NLS
-      se.append(copyConfig == null ? "false" : copyConfig.getValueString()); // NON-NLS
-      se.append(parameterListConfig.getValueString());
+      .append(xOffsetConfigEXP.getValueString())
+      .append(yOffsetConfigEXP.getValueString())
+      .append(matchRotationConfig.getValueString())
+      .append(afterBurner.getValueString())
+      .append(descConfig.getValueString())
+      .append(slotId)
+      .append(placementConfig.getSelectedIndex())
+      .append(aboveConfig == null ? "false" : aboveConfig.getValueString()) // NON-NLS
+      .append(copyConfig == null ? "false" : copyConfig.getValueString()) // NON-NLS
+      .append(parameterListConfig.getValueString())
+      .append(PLACEMARKER_VERSION);
       return ID + se.getValue();
     }
     public static class ChoosePieceDialog extends ChooseComponentPathDialog {

vassal-doc/src/main/readme-referencemanual/ReferenceManual/Marker.adoc

@@ -43,14 +43,14 @@ If left blank, this trait will not produce a context menu item but may still be
 This acts as a pointer to an existing definition: if the original piece is changed, the piece created by the _Place Marker_ trait will also change.
 
 *Horizontal offset:*::  The new piece will be placed this many pixels to the right of the original piece.
-Any value other than zero will prevent the new piece from stacking with the original piece.
 
 *Vertical offset:*::  The new piece will be placed this many pixels above the original piece.
-Any value other than zero will prevent the new piece from stacking with the original piece.
+
+NOTE: A non-zero value in either Offset field will remove the piece from its parent stack and restrict the placement options to the Top or Bottom of a Stack, if joined (see below).
 
 *Match Rotation:*::  If selected, and both the original piece and the new piece have the <<Rotate.adoc#top,Can Rotate>> trait, then the rotation angle of the new piece will be adjusted to match that of the original piece.
 
-*Place marker:*::  Choose whether the new piece should be placed on the top of this piece's stack, on the bottom, or directly above/below the triggering piece.
+*Place marker:*::  Choose whether the new piece should be placed on the top of this piece's stack, on the bottom, or directly above/below this piece. If the new piece is to join a different stack, the above/below options are interpreted as top/bottom respectively. No effect if the new piece is not stackable.
 
 *Set Dynamic Properties in marker:*::
 Allows you to set the values of the named <<DynamicProperty.adoc#top,Dynamic Properties>> in the created marker. +

vassal-doc/src/main/readme-referencemanual/ReferenceManual/Replace.adoc

@@ -31,9 +31,11 @@ If left blank, no context menu item will appear, but the trait can still be acti
 Click the *_Select_* button to use the definition from an existing piece in the module.
 This acts as a pointer to an existing definition: if the original piece is changed, the piece created by the _Replace with Other_ trait will also change.
 
-*Horizontal offset:*::  The center of the replacement piece will be placed this many pixels to the right of the center of the original piece.
+*Horizontal offset:*::  The new piece will be placed this many pixels to the right of the original piece.
 
-*Vertical offset:*::  The center of the replacement piece will be placed this many pixels above the center of the original piece.
+*Vertical offset:*::  The new piece will be placed this many pixels above the original piece.
+
+NOTE: A non-zero value in either Offset field will remove the piece from its parent stack and restrict the placement options to the Top or Bottom of a Stack, if joined (see below).
 
 *Match current state:*::  If selected, VASSAL will attempt to put the replacement piece in the same state as the original piece.
 Layers will be set to the same level, labels will be given the same value, rotation angles will match, etc.
@@ -45,16 +47,14 @@ NOTE: _Markers_ are never state matched. A _Marker_ defined in the replacement p
 *Only match states above this trait:*::  If selected, VASSAL will only replace states in traits that appear above this one in the list of traits in the <<GamePiece.adoc#top,game piece editor>>. For example, the state of a _Layer_ that's above this one _will_ change if the state in the new game piece changes.
 If it's below, then it _will not_ change if the new game piece has the same _Layer_ property.
 
-*Copy Dynamic Property values whre Property name matches:*::
+*Copy Dynamic Property values where Property name matches:*::
 The default option for <<DynamicProperty.adoc#top,Dynamic Properties>> is that the trait must be defined exactly in the replacement as it is in the original (including description, minimum and maximum values and Key Commands).
 +
 Selecting this option causes the match process to just compare the name of the Dynamic Property between the original and the replacement, regardless of the other settings.
 +
 This makes it easier to simply copy the value of  <<DynamicProperty.adoc#top,Dynamic Properties>> into the matching properties replacement piece where you may need to define them differently in the replacement piece.
 
-
-*Place marker:*::  Choose whether the new piece should be placed on the top of this piece's stack, on the bottom of the stack, or directly above/below the triggering piece.
-
+*Place marker:*::  Choose whether the new piece should be placed on the top of this piece's stack, on the bottom, or directly above/below (i.e. replacing) this piece. If the new piece is to join a different stack, the above/below options are interpreted as top/bottom respectively. No effect if the new piece is not stackable.
 
 *Set Dynamic Properties in marker:*::
 Allows you to set the values of the named <<DynamicProperty.adoc#top,Dynamic Properties>> that exist in the created marker. +
@@ -63,7 +63,7 @@ Properties used in the expression reference values in the newly created marker.
  +
 The expression used to set the value can reference existing property values in both the piece creating the marker (Using $$ variables) and in the newly created marker. +
  +
-The values are set into the new marker after any propert values are copied based on State matching, but before the Keystroke after placement is applied. +
+The values are set into the new marker after any property values are copied based on State matching, but before the Keystroke after placement is applied. +
 +
 See <<PassingValues.adoc#marker,Passing values to pieces>> for more detailed information on using this feature.