Improve performance of the visibility sweep #4642
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
The sweep is now based on `LineSegment` instead of `LineString` as it is a bit easer to work with and avoids some of the cost associated with `Geometry`. The intersection check for whether to include a blocking segments is now against the vision bounding box rather than the precise vision geometry. There is a tradeoff here as more segments can be excluded the more precise we are, but in practice the extra precision does not gain us nearly as much as it costs. `VisibilityProblem` is now responsible for collecting and organizing segments. `VisionBlockingAccumulator` only decides how to treat each topology type, but otherwise forwards to `VisibilityProblem`. This avoids some expense around building large lists of segments.
A new `EndpointSet` class is responsible for maintaining the endpoints and walls for the visibility sweep. It avoids any use of endpoint hashing in favour of retroactively merging the few duplicated endpoints we receive. `VisibilitySweepEndpoint` now references other `VisibilitySweepEndpoint` to define walls instead of using `LineSegment`, which saves on some redundancy and make it really easy to validate the graph.
This change moves much closer to Asano's original algorithm as described in section 3.2 of "Efficient Computation of Visibility Polygons" ([arXiv:1403.3905](https://arxiv.org/abs/1403.3905)). The critical difference is in how we find the nearest wall, in particular: 1. Keeping the open walls ordered by distance to the origin, so that the nearest one is always at the front. 2. Using a cheaper orientation-based comparator for the open walls that does not depend on any finding any intersections. Since we don't have to intersect walls in order to find the nearest one, we can now limit ourselves to a single nearest wall check per iteration, as opposed to the two that used to be used. Intersections are only calculated when we need to add a point to the result that does not lie on one of the input endpoints.
There are plenty of cases where a wall is completely hidden behind other open walls and therefore cannot contribute to the result. Instead of marking these as open, we can skip them completely and avoid a lot of expense during the sweep. In order to keep the check cheap, walls are only checked against the nearest wall, not against all open walls that might hide them.
A lot of things have been documented, names improved, and `VisibilityProblem` has been rearraanged to keep the core public stuff first. There are also fewer assumptions and `assert` checks made in several places, instead opting for being able to handle a wider variety of situations. If the sweep ever fails, an exception is thrown rather than returning a null (no blocking) result - `FogUtil` handles this and simply does not expose any area in this case while logging the error.
The new comparator is based on point orientation around the origin rather than directly depending on angle. This is much cheaper, and means we don't have to cache calculations on `VisibilitySweepEndpoint` thus making it simpler. `EndpointSet` also buckets endpoints according to which eighth of the plane the point is in. Sorting can be done on one bucket at a time which reduces the number of comparisons needed. The buckets also remove the need for some special cases in the comparator, further streamlining the sort.
cwisniew
approved these changes
Jan 17, 2024
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
Final piece of #4506
Description of the Change
A number of changes were made to the visibility sweep to improve performance:
Geometrywhere it isn't needed. Often we weren't even relying on the generalGeometrybehaviour but were still paying a cost for it.VisibilitySweepEndpointis the nodes, and edges are merely references to otherVisibilitySweepEndpoint.Altogether this brought the test case in #4506 down from ~15.5 seconds to ~2 seconds on my test machine.
Possible Drawbacks
Should be none.
Release Notes
This change is