Move fixed distance bug/fix

Technical Support & Bugs
#1

I’m very new to this sort of thing, so I’m not sure what the process is for bug fixes/code modification suggestions.

Recently the Malifaux module updated from Vassal 3.2.7 to 3.4.3. This introduced an issue with the ‘Move Fixed Distance’(MFD) trait on the models, as they have 2 MFD traits - one above the ‘Can Rotate’ trait which is designed to follow the piece rotation and one below it which is designed to remain unrotated.

Between the versions, the Translate code was modified to retrieve the highest FreeRotator decorator if it failed to find one below it, thus breaking the trait ordering:
// Handle rotation of the piece, movement is relative to the current facing of the unit.
// Use the first Rotator trait below us, if none, use the highest in the stack.
FreeRotator myRotation = (FreeRotator) Decorator.getDecorator(this, FreeRotator.class);
if (myRotation == null) {
myRotation = (FreeRotator) Decorator.getDecorator(Decorator.getOutermost(this), FreeRotator.class);
}

Additionally, after applying the rotation, the floating point values are truncated with an (int) cast without being rounded first, causing models to drift up and left as they’re moved, and breaking the inverse relationship of forward/backward movement

if (myRotation != null) {
  Point2D myPosition = getPosition().getLocation();
  Point2D p2d = p.getLocation();
  p2d = AffineTransform.getRotateInstance(myRotation.getCumulativeAngleInRadians(), myPosition.getX(), myPosition.getY()).transform(p2d, null);
  p = new Point((int) p2d.getX(), (int) p2d.getY());
}

I would like to add a request that the following lines be removed:
if (myRotation == null) {
myRotation = (FreeRotator) Decorator.getDecorator(Decorator.getOutermost(this), FreeRotator.class);
}

And that this line:
p = new Point((int) p2d.getX(), (int) p2d.getY());
be modified to this:
p = new Point((int) Math.round(p2d.getX()), (int) Math.round(p2d.getY()));

How do I go about suggesting code changes? Do I need to make a bug report describing the issue, and then submit a pull request? Also, how does one go about submitting a pull request?

#2

Hi, thanks for the report and for going to the trouble of looking deeper into the issue. I did have your original post on this issue on a TODO list somewhere, but am absolutely swamped and have not had a chance to look at it.

Yes, definitely start by creating a bugzilla bug report on the Vassalengine.org tracker and include all of this information. We work off the Bugzilla issue numbers as our primary reference.

By the looks of things you know your way around code, so we are more than happy for you to submit a PR for the changes. Check out the Engine Development section here vassalengine.org/wiki/Main_Page which has tutorials on how to get the source, build and test Vassal and use Git. We have a Discord server for dev chat.

Alternatively, I am happy to apply these changes for you, which will get things moving faster as I made the changes and know where the bodies are buried.

Out of interest, you can try setting the Compatibility preference ‘Use Classic Move Fixed Distance trait move batching?’ which I think will solve your problem at the expense of screwing up undo and sensible Triggered sequence MFD’s. The ‘classic’ option uses the complete original MFD code.

The rounding suggestion make sense, it should have been that way in the first place.

The interaction with the Rotators is more complex. The original behaviour actually makes no sense at all. Traits should only be affected by other traits that are ABOVE them on the Stack, not below them. A MFD with a rotator trait below it should not be affected by that Rotator at all, the counter should effectively rotate below the MFD, but the MFD will still move orthogonally (ie Up = towards top of board). A Rotator above the MFD should rotate the direction of the MFD movement.

I have to admit I had not anticipated someone including 2 MFD’s in the one stack. I left the check for the rotator below for compatibility reasons, but add the check for the rotator above since this is the sensible place for it to be.

I believe the correct fix is to remove the check for a rotator below completely and ONLY use the first rotator in the Stack that is ABOVE the current pieces location. This will begin the MFD behaviour in line with standard trait behaviour but will essentially break your module further, but with the workaround that you can swap the position of your two MFD’s above/below the rotator.

Since this will be a change from old behaviour, we could add a compatibility preference ‘MFD’s use Rotator below’ to turn on the original ‘Only use a rotator from below’ behavior, but with the other bug fixes that you lose if you use the current preference.

Thoughts?

#3

Thanks for the assistance - I’m not sure how I missed the entire developer section on the homepage initially, I’ll have to check that out.

I’m happy to go through and set up the bug report and pull request - it’ll give me a chance to practice my github fu.

I still find the trait ordering confusing frequently, but I had to think through the old trait ordering again because it had initially made sense to me that it worked as it did.

I think the model I had in my head for how it was working is that rather than the ‘can rotate’ trait modifying the translation, they both were applied relative to the gamepiece. Hence, if the movement is listed before the rotation, the piece has not yet had a rotation applied to it and it’s therefore moving an unrotated piece. If the movement is after the rotation, the movement directions are now relative to a rotated piece, and therefore need to have a rotation transform applied to them.

I can definitely see how the alternative model would apply though, with the rotation trait modifying the translation trait above it but not below.

I’m not really sure which model better fits the trait ordering paradigm, and honestly am pretty indifferent between the two - whichever better fits the trait ordering model seems like the one to go with just for consistency sake, and we can reorder the traits to match the model.

#4

Hi,

After reviewing this issue and consulting our super new in-app Reference Manual, I have come to the conclusion that you are absolutely correct and we need to make the changes you suggest and revert completely to the original behaviour as this agrees with the behaviour described in the reference manual (trait ordering discussion). I have to admit I still get confused by trait ordering!

That removes the need for any compatibility settings or for you to change your module. I will also include the rounding change to fix the drift problem.

I have created bug report vassalengine.org/tracker/sho … i?id=13542. This fix will be in version 3.4.7, but a test snapshot be ready before then and I will get you to test it when it is ready to validate the fix.

Regards,
Brent.

#5

Awesome, thanks for the assistance - I’ll look forward to seeing the test release.

-David

#6

Try VASSAL-3.4.7-SNAPSHOT-d65487454:

vassalengine.org/~uckelman/tmp/

Does that fix the problems you’re seeing?

#7

I won’t be at my computer to test them for the next few days, but if the changes match what I described in the original post, they resolved the issue when I tested them on the current github branch, so I’m confident they’ll be resolved in this version as well.

I’ll report back once I get a chance to test the build however.

-David

#8

Looks like everything works perfectly. Thanks again.

-David