Revalidate fast-clip spike repairs#893
Open
Symmetricity wants to merge 1 commit into
Open
Conversation
When fast_clip produces a polygon reported with failure_spikes, the existing code removes spikes and then continues with that geometry without checking whether it became valid. That can skip the existing Boost intersection fallback if the repaired geometry still has self-intersections or intersecting interiors. Run Boost's validity check again after remove_spikes so remaining self-intersection failures use the existing slower intersection path instead of being cached and returned as invalid clipped geometry.
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
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.
This PR is AI generated.
Summary
This rechecks polygon validity after
geom::remove_spikes()repairs afast_clipresult.The existing code already falls back to
boost::geometry::intersectionwhenfast_clipleaves self-intersections or intersecting interiors. However, if thefirst validity check reports
failure_spikes, tilemaker removes the spikes andthen continues without checking whether the resulting geometry is now valid.
That can bypass the existing Boost fallback and cache/return a still-invalid
clipped polygon.
This keeps the existing fast path unchanged for valid clipped geometry. It only
adds a second validity check after spike removal, and uses the already-existing
fallback if that second check reports self-intersections or intersecting
interiors.
Background
fast_clipwas added in #482 to avoid the cost of callingboost::geometry::intersectionfor every polygon clip. The PR notes largespeedups from the specialized rectangular clipper.
#597 later documented that
fast_clipcan create zero-width borders aroundtile margins and added a fallback to
boost::geometry::intersectionwhenself-intersections persist. That fallback deliberately keeps most of the
fast_clipperformance win while using Boost for the pathological cases.The remaining gap is the
failure_spikespath:fast_clipclips the polygon.geom::correct()normalizes it.geom::is_valid()reportsfailure_spikes.geom::remove_spikes()modifies the polygon.If step 4 removes the spikes but leaves another invalidity, such as
self-intersections, the #597 fallback is skipped.
Implementation
The patch is intentionally small:
geom::is_valid()call.failure_spikes, callgeom::remove_spikes()as before.geom::is_valid()again.interiors, use the existing Boost intersection fallback.
No new geometry repair strategy is introduced.
Boost Fallback Version Sensitivity
This PR does not change the Boost fallback algorithm itself. It can increase
use of that fallback only for this narrow path:
fast_clip()produces an invalid polygon.failure_spikes.geom::remove_spikes()is run.failure_self_intersectionsorfailure_intersecting_interiors.Before this patch, step 4 was never checked, so those cases kept the
spike-removed
fast_clipoutput. After this patch, those cases use theexisting
boost::geometry::intersection(input, box, output)fallback added by#597.
That matters because Boost.Geometry's overlay traversal changed substantially
between Boost 1.88 and newer Boost versions. Boost.Geometry's Boost 1.89
release notes list "Rewrite of traversal" as a major improvement
(https://www.boost.org/doc/libs/latest/libs/geometry/doc/html/geometry/release_notes.html).
The corresponding Boost.Geometry PR is
boostorg/geometry#1369.
That upstream PR changed 86 files with about 4,000 insertions and 6,700
deletions. Its summary says the overlay code started using Boost.Graph to
determine biconnected components for traversal instead of the previous
isolation-detection approach. In the headers this replaces the old
traversal_switch_detector/traversal_ring_creator/backtrack_check_self_intersectionspath inboost/geometry/algorithms/detail/overlay/traverse.hppwith the graph pathusing
detect_biconnected_componentsandtraverse_graph.Prior local investigation found at least one clipped polygon where this
fallback output differed between Boost builds. For OSM way
438997805(
natural=scrub) in tile z14/8626/10626, the system Boost 1.88 build kept twoclipped fragments while the vcpkg/CI Boost build kept one fragment. In that
investigation,
fast_clip()produced the same invalid geometry in both buildsand both builds entered the existing Boost intersection fallback, so the
difference was isolated to Boost.Geometry fallback behavior rather than
tilemaker's
fast_clipoutput.This PR therefore trades an invalid cached polygon for the repository's
existing fallback behavior, but it also means this narrow class of
spike-repaired-and-still-invalid geometries may become subject to the same
Boost-version-sensitive fallback output as other self-intersection cases.
Evidence
I reproduced this against a clean upstream master build and this branch using
the Liechtenstein Geofabrik fixture with the OpenMapTiles profile.
include_idswas enabled only to make the affected OSM feature easy to identify in the
generated vector tiles.
Affected object:
landuse=residential2659Observed clean-master behavior:
landusetile.Observed behavior with this patch:
landusetile.Across the clean fixture output:
landusefeatures changed from 3,762 to 3,763.poifeatures changed from 2,337 to 2,343.clipped geometry for affected polygons and any derived features that depend on
that geometry.
Runtime on the small fixture was within noise:
The fallback remains more expensive than
fast_clip, but it is only reachedwhen a spike repair is still invalid in one of the same ways already handled by
the existing fallback.
Screenshots
The visual comparison screenshots show the affected relation missing before the
patch and restored after the patch. The restored shape closely follows the
source OSM geometry clipped to the relevant tile.
Related Issues And PRs
fast_clipself-intersectionfailures. This PR extends that same fallback to the case where removing spikes
does not actually produce a valid polygon.
related to clipping correctness, but I have not verified that it fixes the
specific examples in that issue.
This PR is related to that class of issue, but I have not verified that it
fixes those specific examples.
Possible Regressions
The expected behavior change is that some polygons which previously used the
spike-removed
fast_clipoutput will now use the existing Boost intersectionfallback if the spike-removed output is still invalid.
That can change geometry near tile boundaries for affected polygons. In the
verified case, the change restores a missing source-backed polygon. Small edge
differences are possible where Boost and
fast_clipproduce slightly differentclipped shapes.
The performance impact should be limited to polygons that hit this narrow
invalidity path. The small fixture did not show a measurable slowdown.
Testing
Code checks:
Fixture checks:
The fixture was generated once with clean upstream master and once with this
branch, then decoded and compared semantically.