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

Make tokens more closely follow their leader #4817

Merged
merged 10 commits into from
Jun 4, 2024

Conversation

kwvanderlinde
Copy link
Collaborator

@kwvanderlinde kwvanderlinde commented May 31, 2024

Identify the Bug or Feature request

Fixes #4813

Description of the Change

Bug fix

A new approach is taken for deriving follower paths from leader paths. The general idea is that the waypoints in the follower path should follow the waypoints of the leader path, regardless of which tokens are STG or not. In most cases this is exact, but in the case of a STG token following a non-STG leader the waypoints are snapped to grid cells after being calculated. When both tokens are STG or both are not, the follower path will precisely follow the leader path even between waypoints (so pathfinding is accounted for). In the other cases, only the waypoints are considered, and intervening points may be interpolated.

Refactoring

There is a supporting refactor to Path to make the above idea solid. Path now has an invariant that makes sure the waypoint list and cell list make sense together:

  1. if either list is empty, both are empty
  2. if not empty, the lists have the same first and last point.
  3. All waypoints are guaranteed to appear in the cell list in the same order, but possibly with extra cells interposed.

To do this, callers are only allowed to append to the path, not otherwise modify it. And they no longer add cells and waypoints separately. Instead they can either:

  1. Append a single point to be treated as both cell and waypoint.
  2. Append a partial path whose last point is treated as a waypoint.

Only SelectionSet and MeasureTool really had a problem with that, and specifically for gridless paths. They were easily fixed by having them handle their own current point instead of hijacking the Path to do it for them.

New unit tests are included that fully cover Path aside from readResolve().

Possible Drawbacks

Serialized paths will not obey the new rules. This shouldn't really be an issue, but it does mean that if a user loads up an campaign, hit "Show Path" on a follower token, the paths they see won't match what they see if they tried the same drag that produced it originally.

Documentation Notes

Follower tokens try to follow the leader token as closely as possible when dragging.

Release Notes

  • Fixed a bugs where follower tokens would take different paths to the destination than the leader.

This change is Reviewable

`ZoneWalker.replaceLastWaypoint()` does not need to return a value, so that has been removed.

`ZoneWalker.getPath()` and `SelectionSet.getGridlessPath()` never return `null`, so mark them as such and clean up
callers to not check for it.

`AbstractPoint` is now sealed with `ZonePoint` and `CellPoint`. We do a lot of casework that already assumes only those
two possibilities, and now that is formal. It will also allow for pattern matching.
Avoid some case work and unnecessary `Path<>` creation.
Instead it keeps its own last point that it can freely modify but that is kept out of the path until committed as a
waypoint.

Gridless path rendering is also cleaned up by using a Path2D. Even better would be to delegate to
`ZoneRenderer.renderPath()` like we do for gridded paths, but that current assumes it needs to adjust for a token
footprint, which `MeasureTool` does not require.
Also change DTO translation to work with the field rather than the public API.
Just like `MeasureTool`, `SelectionSet` keeps its own current point that it modifies and only adds to the path when set
as a waypoint. If the final path is requested, the path is copied and the current point is added as the final waypoint.

A subtle aspect of this change is that all points in the `Path` are waypoints and there are no "dangling" cell points.
Now that all users of `Path` do not modify the `Path` and are guaranteed to not have dangling cell points, we can have a
new mutating API for Path:
- One method for appending a waypoint (implies adding a cell point too)
- Oen method for appending a partial path, the last point of which is treated as a waypoint.

This gives the following invariants for `Path`:
1. If there are no waypoints, then there are no cell points.
2. The first cell point in the path is a waypoint.
3. The last cell point in the path is a waypoint.
4. The waypoints are a subsequence of the cell points.

Before this change, there we no invariants for `Path` - the cell points and waypoints could vary arbitrarily depending
on the caller. The downside we still have is that the representation does not reflect this invariant. E.g., there is no
way accurately pull the path back apart into its partial paths.
First of all, we copy the leader token before making any changes to the movement set. Otherwise we run the risk of
modifying the leader partway and getting inconsistent results depending on iteration order.

Next we pull the responsibility for derive the path out of `Token`. Previously, `Token.applyMove()` had to be aware of a
bunch of information that is really specific to the renderer state, only to pass it along to `Path.derive()`. We now
instead just make the renderer derive the path, then set it on the token. This also removes the need for
`Token.applyMove()`.

Finally, we push the calculation of the "cell offset" into `Path.derive()`. There is casework in the renderer and in
`Path.derive()` that has to line up or else things don't work. So it is simpler to just have `Path.derive()` do the
calculation in those cases where it is needed.
The calculation for how to derive a follower path is quite different now (and simpler). When both tokens are STG or both
are not STG, the calculation is the same: simply apply the offset to all path points. This lets the follower's path have
the exact same shape as the leader.

When a non-STG token is following a STG leader, we reflect the waypoints of the leader in the follower path (offset, of
course). This does not keep the full pathfinding shape when pathfinding around terrain, but at least the path is not
arbitrarily chosen and does not suffer issues with zig-zagging.

When a STG token is following a STG leader, we again reflect the waypoints. But in this case we have to snap each
resulting point to the grid, and need to fill in partial paths between each waypoint. As before, this pathfinding is
simple, i.e., it is not an A*-based algorithm.
This is not visibly different from before, though it ensures that STG followers have their position snapped to the
grid, just as they would be had they been the leader.
Includes tests for all four derived path cases, and a test for waypoints in a self-crossing snap-to-grid path. The
latter case used to include extraneous waypoints.
@kwvanderlinde kwvanderlinde changed the title Clean up walker and points Make tokens more closely follow their leader May 31, 2024
@kwvanderlinde kwvanderlinde self-assigned this May 31, 2024
@kwvanderlinde kwvanderlinde added bug refactor Refactoring the code for optimal awesomeness. labels May 31, 2024
@kwvanderlinde kwvanderlinde changed the title Make tokens more closely follow their leader Follower tokens do not follow the leader Jun 3, 2024
@kwvanderlinde kwvanderlinde changed the title Follower tokens do not follow the leader Make tokens more closely follow their leader Jun 3, 2024
@cwisniew cwisniew added this pull request to the merge queue Jun 4, 2024
Merged via the queue into RPTools:develop with commit 58127f6 Jun 4, 2024
5 checks passed
@kwvanderlinde kwvanderlinde deleted the bugfix/4813-follower-paths branch June 28, 2024 16:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug refactor Refactoring the code for optimal awesomeness.
Projects
Status: Merged
Development

Successfully merging this pull request may close these issues.

[Bug]: Follower paths don't make sense
2 participants