Fix various issues with transfering map topology to and from token topology - #5019
Merged
cwisniew merged 5 commits intoOct 24, 2024
Merged
Conversation
The `ServerCommand.addTopology()` and `removeTopology()` have been merged into a single `updateTopology()` that handles both drawing and erasing. It also applies the changes to the local client so caller's don't have to remember to do that. All callers were already well-behaved here, but this change just cleans them all up and avoids the repetition conditions.
There's only a couple places that need to do it, but both of them forgot to sync to remote clients.
The way to get rid of topology from a token is to set it to `null`. This change allows it to actually be sent over the wire, in the form of an empty area.
The results of a few calculations were not used, so a good chunk of the method could just be deleted. Beyond that, the method was written to be as condensed as possible, and avoiding unnecessary `Area` copies. The method `TokenVBL.getMapTopology_transformed()` was removed because it had to do everything that `TokenVBL.getTopology_underToken()` plus transforming it back into token space. A new method `TokenVBL.transformTopology_toToken()` takes its place, and can transform an `Area` (no matter the source) into token space. This process was taken from `TokenVBL.getMapTopology_transformed()`, but cleaned up to produce a single transformation and avoid any intermediate `Area`.
Collaborator
Author
|
This will conflict with #5011 because of the changes to tools. I suggest for any reviewer's sake that this PR be done first. |
cwisniew
approved these changes
Oct 24, 2024
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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 #5018
Description of the Change
There are a few issues fixed in this PR.
First is the fix for #5018 itself, which is done by avoiding any direct modification of a token's topology. Instead, the server command is used to update both local and remote clients in one place. The same pattern is done for map topology, though in all cases it was at least done correctly. This used to be the role of
TokenVBL.renderTopology(), but the server commands is a much more natural place for it. The add/remove operations and commands have also been merged into a single update operation that can draw and erase.Next is the fix for the NPE mentioned in the comments. Our protobuf messages do not support sending null token topology, so instead we send empty areas along. On the other end these empty areas are turned back into nulls since there are a few places that rely on null and I wasn't confident about nailing all those down.
Finally is the fix for the misaligned token topology also mentioned in the comments. This was simple a matter of incorporating the token anchor in the rotation used by
TokenVBL.getTopology_underToken()andTokenVBL.getMapTopology_transformed(). These methods were also refactored to be remove unused calculations, to not duplicate each other, and to avoid unnecessary intermediateAreacreation.Possible Drawbacks
All sunshine.
Documentation Notes
N/A
Release Notes
transferVbl()on a token with no VBL would print a NullPointerException in chat.This change is