Simplify AreaMeta and move vision blocking algorithm into a dedicated class #4577
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
Given that topology trees are alternating islands and oceans, and since vision is only blocked at boundaries of each, all VBL types follow a common pattern in their case work: 1. Find containers that vision is allowed to pass through (oceans for Wall and Cover, islands for Hill and Pit). 2. Use part of those containers' boundaries to block vision. 3. Use part of those containers' childrens' boundaries to block vision. The new `blockVisionBeyondContainer(AreaIsland)` and `blockVisionBeyondContainer(AreaOcean)` helpers give direct support to this pattern and reduces noise in the case work. The `AreaIsland` overload was previously called `addIslandForHillBlocking(AreaIsland)` and used for Hill and Pit VBL, but the new naming makes it clearer that it is not specific to Hill VBL and is consistent with the `AreaOcean` version. Another pattern relies on the fact that the result of `AreaTree.getDeepestContainerAt(Point2D)` is either an ocean or has an ocean as a parent (the same is not true of islands). The case work for Hill and Cover VBL is identical in both cases, so they can be flattened by anchoring the implemetations at this nearest ocean then mixing in the presence of islands as needed. As a convenience, it is now possible to call `VisionBlockingAccumulator.add(TopologyType, AreaTree)` to add topology of a given type, rather than having separate public methods for each. This makes `FogUtil` a bit more natural to read.
The quartet of `AreaTree`, `AreaContainer`, `AreaIsland`, and `AreaOcean` have been condensed into `AreaTree` for a clearer structure and smaller code size. `AreaTree` no longer allows modifications after construction, has a more efficient construction process using JTS' `Polygonizer`, and more directly supports the patterns established in cbf91d1. Each node in the tree is now represented by a simple `AreaTree.Node` type which only contains references to its children and the associated `AreaMeta`. All logic is either in `AreaTree` (for traversing the tree) or in `AreaMeta` (for operations on the region itself). `AreaMeta` has been simplified as well. It is no longer mutable, and doesn't possess or build an AWT `Area`. It is represented as a list of vertices which defines the polygonal boundary of the region it represents. There are other fields, but they are derived entirely from the vertices and exist solely for efficiency. `AreaMeta` is also now capable of representing the root ocean, which simplifies the usage of `AreaTree` nodes as they no longer have to check for possibly-null `AreaMeta` for just this one case. `AreaTree.getArea()` has been removed since it only existed to support `VisibilityInspector`. Since that is just a debugging tool, and the method is kind of dangerous (doesn't protect against changes), it doesn't make sense to keep it around. Instead `VisibilityInspector` keeps track of its own `Area` objects, alongside some other minor simplifications.
A new class `VisibilityProblem` is responsible for collecting blocking segments and generating a visibility polygon from them. No meaningful changes have been made to the algorithm itself, though it does support some new conveniences such as progressively building up the problem set before solving.
cwisniew
approved these changes
Dec 23, 2023
cwisniew
added
the
code-maintenance
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
First part of #4506
Description of the Change
This change is mainly for the complexity & organization aspects of #4506. There is a very small performance improvement in complex cases due to
AreaTreeconstructor changes, but there are no big wins - only ~300ms was saved over the ~30s test case referenced in the issue. Another PR will include performance improvements, but would add too much noise to include here.AreaTree, etc.This is the bulk of the PR. The quartet of
AreaTree,AreaContainer,AreaIsland, andAreaOceanhave been condensed intoAreaTreewith nodes represented by the very plainAreaTree.Node. The main mutually-recursive algorithmAreaTree.getContainerAt(Point2D)has been replaced by the iterativeAreaTree.locate(Coordinate), which additionally returns more context to better supportVisionBlockingAccumulator.AreaTreeconstruction is simplified and more performant due to using JTS'Polygonizerwhich associates islands with child oceans for us. It also plays well with the new representation ofAreaMetaas a list ofCoordinatevertices forming a ring, which is sufficient for all uses ofAreaMeta.VisionBlockingAccumulatorA small refactoring in
VisionBlockingAccumulatorunifies a common case across all VBL types where blocking needs to be added for the boundaries of a container and its children. Hill VBL and Pit VBL already had this case factored out for islands, but now Wall VBL and Cover VBL have the same case factored out for oceans. Also included is a newadd(TopologyType, AreaTree)method so that we dont need to call distinct public methods for each type (helpsFogUtilout a bit).FogUtilThe final change is pretty trivlal: the visibility sweep algorithm (
FogUtil.calculateVisibleArea()and related) has been moved out ofFogUtilinto a dedicatedVisibilityProblemclass that doesn't depend so heavily on static methods. A future PR will further refine this with some algorithmic improvements.Possible Drawbacks
Should be none.
Documentation Notes
N/A
Release Notes
This change is