Skip to content

Revalidate fast-clip spike repairs#893

Open
Symmetricity wants to merge 1 commit into
systemed:masterfrom
Symmetricity:fix/revalidate-fast-clip-spikes
Open

Revalidate fast-clip spike repairs#893
Symmetricity wants to merge 1 commit into
systemed:masterfrom
Symmetricity:fix/revalidate-fast-clip-spikes

Conversation

@Symmetricity
Copy link
Copy Markdown

This PR is AI generated.

Summary

This rechecks polygon validity after geom::remove_spikes() repairs a
fast_clip result.

The existing code already falls back to boost::geometry::intersection when
fast_clip leaves self-intersections or intersecting interiors. However, if the
first validity check reports failure_spikes, tilemaker removes the spikes and
then 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_clip was added in #482 to avoid the cost of calling
boost::geometry::intersection for every polygon clip. The PR notes large
speedups from the specialized rectangular clipper.

#597 later documented that fast_clip can create zero-width borders around
tile margins and added a fallback to boost::geometry::intersection when
self-intersections persist. That fallback deliberately keeps most of the
fast_clip performance win while using Boost for the pathological cases.

The remaining gap is the failure_spikes path:

  1. fast_clip clips the polygon.
  2. geom::correct() normalizes it.
  3. geom::is_valid() reports failure_spikes.
  4. geom::remove_spikes() modifies the polygon.
  5. The modified polygon is cached and returned without revalidation.

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:

  • Store the result of the first geom::is_valid() call.
  • If the failure is failure_spikes, call geom::remove_spikes() as before.
  • Reset the failure code and run geom::is_valid() again.
  • If the geometry is still invalid due to self-intersections or intersecting
    interiors, use the existing Boost intersection fallback.
  • Otherwise preserve the existing behavior.

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:

  1. fast_clip() produces an invalid polygon.
  2. The first validity check reports failure_spikes.
  3. geom::remove_spikes() is run.
  4. The second validity check still reports failure_self_intersections or
    failure_intersecting_interiors.

Before this patch, step 4 was never checked, so those cases kept the
spike-removed fast_clip output. After this patch, those cases use the
existing 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_intersections path in
boost/geometry/algorithms/detail/overlay/traverse.hpp with the graph path
using detect_biconnected_components and traverse_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 two
clipped fragments while the vcpkg/CI Boost build kept one fragment. In that
investigation, fast_clip() produced the same invalid geometry in both builds
and both builds entered the existing Boost intersection fallback, so the
difference was isolated to Boost.Geometry fallback behavior rather than
tilemaker's fast_clip output.

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_ids
was enabled only to make the affected OSM feature easy to identify in the
generated vector tiles.

Affected object:

Observed clean-master behavior:

  • The relation is absent from the affected landuse tile.
  • Mask comparison against the OSM relation geometry clipped to the tile gives:
    • source area: 373,514 pixels
    • output area: 0 pixels
    • IoU: 0.000000

Observed behavior with this patch:

  • The relation is present in the affected landuse tile.
  • Mask comparison against the OSM relation geometry clipped to the tile gives:
    • source area: 373,514 pixels
    • output area: 377,500 pixels
    • IoU: 0.932346

Across the clean fixture output:

  • Tile count stayed the same: 192 tiles before and after.
  • Total decoded feature count changed from 65,462 to 65,469.
  • landuse features changed from 3,762 to 3,763.
  • poi features changed from 2,337 to 2,343.
  • 37 tiles had semantic differences, which is expected because the fix changes
    clipped geometry for affected polygons and any derived features that depend on
    that geometry.

Runtime on the small fixture was within noise:

  • Clean master: 2.08s wall time, 402,644 KB max RSS.
  • This patch: 1.99s wall time, 401,924 KB max RSS.

The fallback remains more expensive than fast_clip, but it is only reached
when 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.

Close comparison for relation 1839204

Context comparison for relation 1839204

Geometry mask comparison for relation 1839204

Related Issues And PRs

Possible Regressions

The expected behavior change is that some polygons which previously used the
spike-removed fast_clip output will now use the existing Boost intersection
fallback 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_clip produce slightly different
clipped 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:

git diff --check origin/master..fix/revalidate-fast-clip-spikes
cmake -S . -B build -DCMAKE_BUILD_TYPE=RelWithDebInfo
cmake --build build --target tilemaker -j2

Fixture checks:

tilemaker liechtenstein.osm.pbf \
  --output=liechtenstein-ids.mbtiles \
  --config=config-openmaptiles-ids.json \
  --process=process-openmaptiles.lua \
  --store=store

The fixture was generated once with clean upstream master and once with this
branch, then decoded and compared semantically.

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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant