Bring back some old AreaTree concepts when preparing BL and FoW - #5355
Merged
cwisniew merged 4 commits intoApr 8, 2025
Conversation
These new test cases focus on extremely precise geometry that includes self intersections with small and large loops, interior rings being near or touching exterior rings, and also a test that bounding boxes aren't used as the only determinant for whether one ring is contained in another.
- `s/areaProvider2/areaProvider/`
- Avoid `Arguments.of(Named.of("name", first), ...)`, use flatter `Arguments.argumentSet("name", ...)`.
This is inspired by and based on our original `AreaTree` / `AreaMeta` construction from 1.14 and earlier. For each clockwise ring, we construct a `GeometryUtil.Island` to represent the solid portion of the geometry. For each counterclockwise ring, we treat it as an ocean. After retrieving all islands and oceans, we stitch each ocean to each island as an interior polygonal ring. Once all oceans are connected to islands, we convert each island to a JTS `Polygon`. Differences from the original `AreaTree` construction are: 1. No need to build an entire tree. We only connect oceans to parent islands, but we do not care about connecting islands to parent oceans. Each island with their child oceans becomes a polygon. 2. The resulting polygons are run through a `GeometryPrecisionReducer` so callers will not need to worry about ultra-precise points. This reducer internally ensures the results remain valid, e.g., by eliminating mistaken self-intersections that would cause confusion about interior and exterior points.
`PreparedGeometry` does not handle empty geometries at all, despite no documentation suggesting this should be anything special. We now make sure to filter out empty geometries and won't bother preparing them.
cwisniew
approved these changes
Apr 8, 2025
github-project-automation
Bot
moved this from Awaiting-Review
to To-Be-Merged
in MapTool 1.18
kwvanderlinde
deleted the
bugfix/5159-bring-back-AreaTree-for-polygonizing-Area
branch
April 8, 2025 03:50
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 #5159
Description of the Change
First, some history:
Geometryin the same way as MBL was already.GeometryusingGeometryUtil.toJts(), which was based on JTSPolygonizerfor simplicity.AreaTreerepresentation of *BL for vision and lighting that was based on a hierarchy of oceans (clear space) and islands (filled space). Converting fromAreatoAreaTreewas done through custom logic that detected the orientation of each ring in theArea.AreaTreefor *BL, but built it using JTS'sPolygonizeras it was faster and simpler. This is the same polygonizer used for pathfinding since 1.13.AreaTree, but let each island live on its own as a JTSPolygon. Buillding these polygons was still based on thePolygonizeras in 1.15.Since 1.15 especially, we've had various reports of existing VBL and FoW breaking pathfinding and vision. Despite trying to tune the
Polygonizerto fix these reports, cases have continued to be reported..In an attempt to fix these issues, I've added several new test cases that are based on real-world issues encountered in recent MT versions. I tried a wide range of techniques to produce acceptable results for all of these cases, but I could not get any
Polygonizer-based solution to work - different cases needed different incompatible fixes to be applid. And JTS'sPolygonizeritself is not interested in (or is not capable of) preserving the specific structures that we rely on (nested polygons).In the end, I just removed the
PolygonizerfromGeometryUtil.toJts()andGeometryUtil.toJtsPolygons()and went back to the custom logic we used to use when buildingAreaTree. We don't build an entire tree as that would be quite wasteful, and we're not reintroducing any of the related types that have long been removed. We are only incorporating just enough of the concept to producePolygonobjects from anArea:Once all oceans are associated with their parent islands, we convert each island into a
Polygon. At this point, the resulting geometry is structurally valid, but there can still be small defects with the boundary of any given polygon or its holes. To address this, we pipe the polygon through aGeometryPrecisionReducerthat will remove any ultra-precise defects from a polygon, possibly dividing an invalid polygon into more than one valid polygon.There is finally a change to
MovementBlockingTopologyto filter out empty geometry as this can cause issues witthPreparedGeometry.Possible Drawbacks
Hopefully none as this seems to work for more cases than ever. But it's possible there is some other existing combination of VBL, MBL and FoW that will still cause issues.
Documentation Notes
N/A
Release Notes
This change is