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

Simplify SelectionSet null checks case work #5098

Merged

Conversation

kwvanderlinde
Copy link
Collaborator

@kwvanderlinde kwvanderlinde commented Dec 8, 2024

Identify the Bug or Feature request

Fixes #5089

Description of the Change

Whenever the walker is used, we check it for null now, and same for the renderPathTask, and when both are needed we consistently check the walker first and only bother with renderPathTask if it is set.

We also avoid checking zone and token properties to decide whether we are using the walker or not, and instead only use the above null checks. The zone and token properties can change over time, and modifying them while a token is in transit used to also result in error (this is a long-standing issue with SelectionSet). Changing these avoids these errors, though it means SelectionSet still doesn't react to the changes. This is something that can be handled separately and is not urgent.

Possible Drawbacks

None

Documentation Notes

N/A

Release Notes

  • Fixed a bug where non-snap-to-grid tokens would generate an error when moved.
  • Fixed a bug where toggline snap-to-grid on a moving token would generate an error.

This change is Reviewable

The presence of the walker is used to indicated whether we are currently using pathfinding and snap-to-grid
behaviour. We no longer check that at every opportunity. Also, the task requires a non-null walker, which was not
checked.

This enables us to easily check against a null walker when it matters.
@kwvanderlinde kwvanderlinde self-assigned this Dec 8, 2024
@github-actions github-actions bot added the bug label Dec 8, 2024
Copy link
Collaborator

@bubblobill bubblobill left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems legit. No obvious down-sides.

@cwisniew cwisniew added this pull request to the merge queue Dec 11, 2024
Merged via the queue into RPTools:develop with commit f8d0672 Dec 11, 2024
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug]: Got NPE when moving a token disabled "Snap to Grid"
3 participants