fix simply connected interior validation#1522
Draft
michaelkirk wants to merge 23 commits intomainfrom
Draft
Conversation
Bumps i_overlay from 4.0 to 4.4 and enables `OverlayOptions::ogc()` for all boolean operations. This ensures output polygons have connected interiors per ISO 19125-1, fixing cases where holes sharing vertices produced invalid geometry. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Cache R-tree structures via PreparedGeometry when validating polygon ring relationships. This avoids rebuilding the geometry graph for each interior containment check against the exterior, and for each interior-interior intersection check. Also adds an early return when no non-empty interiors exist, and skips empty interior rings in pairwise comparisons. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
The prepared_interior_1 construction was moved upward so the exterior_vs_interior relate compares Polygon-vs-Polygon instead of Polygon-vs-LineString. This changes the DE-9IM semantics: a Polygon's boundary is the ring curve (not its interior), so the line-intersection check must use (OnBoundary, OnBoundary) instead of (OnBoundary, Inside). Also applies cargo fmt to bool_ops files. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
With PreparedGeometry wrapping interior rings as Polygons, an interior ring identical to the exterior now passes is_contains() (a polygon contains itself), so only IntersectingRingsOnALine is reported — not InteriorRingNotContainedInExteriorRing. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Detect polygons with disconnected interiors per OGC Simple Features (ISO 19125-1, section 6.1.11.1): "The interior of every Surface is a connected point set." The new InteriorNotSimplyConnected(RingRole, RingRole) error fires when: - Two rings share 2+ distinct touch coordinates (creating a corridor) - Rings form a cycle of single-vertex touches through distinct coordinates (enclosing interior area) Ring pairs already reported as IntersectingRingsOnALine or IntersectingRingsOnAnArea are skipped to avoid duplicate errors. Includes: - Checkerboard polygon fixture for stress-testing cycle detection - 9 new tests covering valid and invalid cases - Validation benchmarks (simple polygons, checkerboards, real fixtures) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Use Vec<Vec<usize>> instead of Vec<HashSet<usize>> for adjacency (only iterated, never queried) - Store edge_coords in one direction with canonical (min, max) keys - Flatten nested if-let chains into tuple-matching patterns - Replace unreachable if-let with .expect() for invariant - Hoist unique_edges allocation outside grouping loop - Minor: hoist coords() call, use |= operator Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Simplifies check_ring_touches_disconnect_interior by replacing the 45-line DFS with entry_coord tracking with a dual Union-Find approach. Also moves Case 1 (pair with 2+ touches) into the caller as an early exit using a HashSet, and builds coord_edges directly instead of going through an intermediate HashMap. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
The polygon is still considered invalid, but a different condition is reported. Some of these conditions are redundant - so it's somewhat arbitrary which one is surfaced.
My original intent was to be able to use GeometryGraph outside of the
context of PreparedGeometry without having to copy the setup code in
`prepare_geometry`.
The nice side side effect of this is an incidental speedup.
I think it's because we are rebuilding the geometry RTree in a couple places.
If we build it once up front, we don't have to worry about this cost.
```
relate overlapping 50-point polygons
time: [27.251 µs 27.693 µs 28.099 µs]
change: [-21.806% -19.731% -17.678%] (p = 0.00 < 0.05)
Performance has improved.
entire jts test suite time: [344.95 ms 346.44 ms 348.46 ms]
change: [-0.4298% +0.0268% +0.6061%] (p = 0.93 > 0.05)
No change in performance detected.
jts test suite matching *Relate*
time: [33.497 ms 33.642 ms 33.796 ms]
change: [-6.1824% -5.5581% -4.9284%] (p = 0.00 < 0.05)
Performance has improved.
disjoint polygons time: [19.555 µs 19.599 µs 19.656 µs]
change: [+0.1938% +0.4692% +0.9024%] (p = 0.00 < 0.05)
Change within noise threshold.
large rotated polygons time: [4.5333 ms 4.5564 ms 4.5896 ms]
change: [-28.900% -28.393% -27.871%] (p = 0.00 < 0.05)
Performance has improved.
offset polygons time: [4.2994 ms 4.3199 ms 4.3457 ms]
change: [-30.030% -29.516% -28.997%] (p = 0.00 < 0.05)
Performance has improved.
```
michaelkirk
referenced
this pull request
Apr 11, 2026
This is subjective, but I don't think we get much value here. This code is already quite complicated, so there is value in simplifying.
michaelkirk
force-pushed
the
mkirk/fix-simply-connected-interior-validation
branch
from
9c747d2 to
1555a94
Compare
This is subjective, but I don't think we get much value here. This code is already quite complicated, so there is value in simplifying.
There were several places in the flow where we were doing similar group_by/order_by. Because GeoFloat isn't Ord, we introduce a total_ord wrapper Also some renames for clarity. "edge" == "ring"
michaelkirk
force-pushed
the
mkirk/fix-simply-connected-interior-validation
branch
from
1555a94 to
9c30e16
Compare
2 tasks
michaelkirk
commented
Apr 11, 2026
| continue; | ||
| } | ||
| // Which rings are connected at *this* coordinate | ||
| // let mut local = UnionFind::new(rings.len()); |
Member
Author
There was a problem hiding this comment.
I don't have an intuitive grasp on what the local UnionFind accomplishes. I've read the description in #1472, and it sort of of makes sense.
Can you come up with a test case which would cover this @bretttully? Currently all the tests pass even with this commented out like this.
Contributor
|
No worries! I won't be able to take a look for a week or two, but will asap. |
…ected-interior-validation
2 tasks
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.
CHANGES.mdif knowledge of this change could be valuable to users.Draft as this depends on #1521
Built off of #1472, with some simplifications and clarity changes. I hope you don't mind that I've taken your code and run with it @bretttully. I would appreciate if you have any opinions on my changes/comments since you've studied the algorithm.