Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Bypass Place Marker stack ops in favour of offset placement #13720

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

riverwanderer
Copy link
Collaborator

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

…Place Marker's stack operations when an offset is specified.
@riverwanderer riverwanderer added this to the 3.7.16 milestone Nov 29, 2024
@riverwanderer riverwanderer self-assigned this Nov 29, 2024
@riverwanderer riverwanderer linked an issue Nov 29, 2024 that may be closed by this pull request
@riverwanderer riverwanderer changed the title Possible fix for https://github.com/vassalengine/vassal/issues/13719.… Bypass Place Marker stack ops in favour of offset placement Nov 29, 2024
@riverwanderer
Copy link
Collaborator Author

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
Copy link

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
Copy link
Collaborator Author

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.

@riverwanderer riverwanderer added bug Something isn't working Ready for Review Ready to be reviewed for Merging labels Dec 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working Ready for Review Ready to be reviewed for Merging
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Place Marker Offset ignored
2 participants