Fix NPE on partially initialized square grids #4964
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Instead, the edge facing and vertex facing preference need to be provided by the caller, and the `Grid` will dynamically decide its response. There is no more static state that needs to be kept up-to-date - state which previously was shared between instances despite being treated like instance state.
- Avoid uninitialized state where reasonable. - Only remaining case is the footprint list which is loaded lazily. - `final`ize fields where possible. - Remove unused alternative cell highlight fields and associated assets
Tools no longer call `Grid.getFacingAngles()`, which requires each caller to interpret the resposne to get the nearest or next facing. Now there are two new methods in on `Grid`: - `int nearestFacing(int facing, boolean faceEdges, boolean faceVertices)`: snaps a facing to one of the standard facings. - `int nextFacing(int facing, boolean faceEdges, boolean faceVertices, boolean clockwise)`: snaps a facing as in `nearestFacing()`, but also advances one step clockwise or counterclockwise. There is no new behaviour introduced by this change. Also included are tests cases for interesting input facings. These make it clear there are some bugs in the current behaviour, such as -179° being considered closer to -135° than to 180° (these are 44° and 1° away, respectively).
Instead of a provide a hardcoded array of possible facings, we now calculate the nearest facing using some modular arithmetic and rounding. This fixes the bugs mentioned in the prior commit, and test cases have been updated to match. Internally to `Grid`, the use of `getFacingAngles(boolean faceEdges, boolean faceVertices)` and been replaced with `snapFacingInternal(int facing, boolean faceEdges, boolean faceVertices, int addedSteps)`, which is enough to provide the functionality of both `getNearestFacing()` and `getNextFacing()`.
kwvanderlinde
force-pushed
the
bugfix/3315-facing-uninitialized-static-state
branch
from
c2caaf6
to
399c740
Compare
cwisniew
approved these changes
Oct 2, 2024
kwvanderlinde
deleted the
bugfix/3315-facing-uninitialized-static-state
branch
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Identify the Bug or Feature request
Fixes #3315
Description of the Change
When our objects are deserialized by XStream, the constructors are not run. In the case of
Gridimplementations, the constructors are relied on to set static state such asSquareGrid.FACING_ANGLES. Without the constructor running, the state remainsnull. The bug is not likely reproducible in recent versions since we recreate anyGridinstances as part of our protobuf logic. But if there is any change there, the bug would resurface.This PR address this by:
Gridwith new methods so callers don't have to care about the facing arrays.Gridmethod to calculate results instead.The end result is:
Grids to keep the facing arrays up to dateTest cases were added for the new
nearestFacing()andnextFacing()methods.Possible Drawbacks
The new
snapFacingInternal()is not as easy to read as a facing array.Documentation Notes
N/A
Release Notes
This change is