Add simply connected interior validation and test suite #1472
Conversation
These tests document the missing validation check for simply connected polygon interiors, as noted in polygon.rs:13-14. The OGC Simple Feature Specification requires that "the interior of every Surface is a connected point set." Test cases: - Two L-shaped holes sharing 2 vertices (disconnects interior) - Checkerboard level 0: 13 holes sharing vertices at grid points - Checkerboard level 1: Nested checkerboard with 26 holes - Valid cases: holes sharing 1 vertex, non-touching holes The checkerboard pattern demonstrates that a simple "two holes sharing 2+ vertices" check is insufficient - four holes meeting at single vertices can also disconnect the interior. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]>
Add validation to detect when a polygon's interior is not simply connected, which violates the OGC Simple Feature Specification requirement that "the interior of every Surface is a connected point set." The validation detects two types of disconnection: - Multi-touch: Two rings sharing 2+ vertices at different coordinates - Cycle detection: Rings forming a cycle through distinct single-touch points Key implementation details: - Build a touch graph where nodes are rings and edges connect touching rings - Detect vertex-vertex touches (same coordinate on both rings) - Detect vertex-on-edge touches (vertex lies on another ring's edge) - Use DFS to find cycles through distinct coordinates The error message now includes the problematic coordinates to aid debugging, e.g., "polygon interior is not simply connected at coordinate(s): (2, 2), (3, 3)" Tests cover: - Two L-shaped holes sharing two vertices (invalid) - Checkerboard patterns with shared vertices (invalid) - Four holes forming a cycle of single touches (invalid) - Three holes meeting at one vertex (valid - interior still connected) - Holes with vertex-on-edge touches to exterior (invalid) 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
bretttully
force-pushed
the
fix-simply-connected-interior-validation
branch
from
dfa491f to
7398dfe
Compare
michaelkirk
left a comment
michaelkirk
left a comment
There was a problem hiding this comment.
Hello! Thank you for the pull request. This would be a great feature to incorporate, but I've got some questions to work through.
| } | ||
|
|
||
| /// Translate a ring by an offset. | ||
| pub fn translate_ring(ring: &LineString<f64>, dx: f64, dy: f64) -> LineString<f64> { |
There was a problem hiding this comment.
See the Translate::translate / Translate::translate_mut
| /// Returns a vector of (name, polygon) tuples. | ||
| fn load_benchmark_geometries() -> Vec<(String, Polygon<f64>)> { | ||
| let mut path = PathBuf::from(env!("CARGO_MANIFEST_DIR")); | ||
| path.push("fixtures/rust-geo-booleanop-fixtures/benchmarks/validate.geojson"); |
There was a problem hiding this comment.
What's the providence of fixtures/rust-geo-booleanop-fixtures/benchmarks/validate.geojson
It's seems like ultimately we just want a vector of polygons. If it's an arbitrary test fixture generated for this test, can you simplify the test data rather than complicating the parsing code?
| let edge = RefCell::borrow(edge); | ||
| edge.edge_intersections() | ||
| .iter() | ||
| .filter_map(|ei| coord_to_bits(&ei.coordinate())) |
There was a problem hiding this comment.
re: f64::to_bits
Is this in pursuit of getting the floating points to fit into a HashSet? There's a reason you can't put floats in a HashSet, and it's not clear to me that this usage is valid.
Can you talk more about this? Maybe it's OK, but I've just never used floats this way.
The only problematic examples I can think of off the top of my head are:
-
NaN coords which have many representations, but we don't make any guarantees for NaN coords anyway, so that's probably fine
-
-0.0 vs. +0.0 - topologically those are the same number, but they'd appear as distinct entries, which seems like it could confound this algorithm.
I'm not sure if there are any others.
| RingRole::Exterior, | ||
| RingRole::Interior(0) | ||
| )] | ||
| let errors = polygon.validation_errors(); |
There was a problem hiding this comment.
Please use the assert_validation_errors macro. If you're having trouble I can help, but I'm not sure if the issue is you tried and it didn't work or if this is just what the AI spit out.
| ## 0.32.0 - 2025-12-05 | ||
|
|
||
| - Add simply connected interior validation for polygons. Polygons with holes that touch at vertices in ways that disconnect the interior (e.g., two holes sharing 2+ vertices, or cycles of holes each sharing a vertex) are now detected as invalid via `Validation::is_valid()`. This aligns with OGC Simple Features and matches PostGIS behavior. | ||
| - Performance: Polygon validation now uses `PreparedGeometry` to cache R-tree structures, improving validation speed by 26-45% for polygons with many holes. |
There was a problem hiding this comment.
PreparedGeometry is just used for this newly added validation right? So there's no world in which people validating before would see a speedup, right?
2 participants
CHANGES.mdif knowledge of this change could be valuable to users.Add validation to detect when a polygon's interior is not simply connected, which violates the OGC Simple Feature Specification requirement that "the interior of every Surface is a connected point set."
The validation detects two types of disconnection:
Key implementation details:
The error message now includes the problematic coordinates to aid debugging, e.g., "polygon interior is not simply connected at coordinate(s): (2, 2), (3, 3)"
Tests cover: