Repository navigation
Add per-layer invalid_polygon_repair_scope option to repair clipped-and-quantised polygons - #929
geoneutrino wants to merge 2 commits into
Conversation
|
|
I measured this PR against three of the open "broken polygon" issues, counting output polygons that are invalid in integer tile coordinates (crossing or overlapping edges, spikes, zero-area rings, first ring not an outer ring). Same data and configs for all builds:
So all three issues share the cause this PR addresses: polygons that are not simplified are never repaired after quantisation. With For comparison, the scaleRing fixes (#933 and the scaleRing part of #941) don't change these numbers. They fix a different problem, polygons that disappear. Two things that might help this get merged:
If this gets merged, #697, #403 and the z11–12 part of #191 can probably be closed with it (#697 is locked, so that needs a maintainer). |
e361e64 to
5a84652
Compare
|
rebased to current master @dktue "all" as default: i used a defensive approach in activating this because in #908 we had a discussion on performance impacts. But at least activate it by default in ocean and water_polygons sounds good (maybe landuse/forerst?) (The conflict was with #939, which added a knownInvalid parameter to repair_multi_polygon() in the same position as this PR’s strictArea. I kept knownInvalid as the second parameter so existing callers are unchanged, and appended strictArea. I also dropped the commit “Only check output polygon validity when it can change the output”, because #939 implements the same guard. The only change left to that guard is adding the opt-in, since otherwise "invalid_polygon_repair_scope": "all" would never trigger for unsimplified layers.) |
5a84652 to
b60cd0e
Compare
|
@geoneutrino, now that #949 is merged, #929 conflicts with master in Checked on the new master ( If it looks right to you, |
Large clipped polygons such as coastline/ocean shapefiles are written to tiles invalid: scaling them onto the integer tile grid introduces self-intersections. Until now only SIMPLIFIED geometry was repaired, so those layers stayed invalid and can be mis-rendered by strict renderers - as missing water, or as land hidden behind an ocean polygon. New layer option invalid_polygon_repair_scope: "simplified_only" (default, unchanged behaviour) repairs only invalid polygons that simplification may have broken; "all" is a superset and additionally repairs polygons that were merely clipped and quantised. For those, repair_multi_polygon runs with a strict area guard that also rejects any repair GROWING a polygon - growing means a hole was filled, which would make an island disappear. That bound must not apply to simplified geometry, where legitimate repairs do change area; applying it globally measurably made things worse (5 -> 20 invalid features in one tile). The bound is passed as a scoped enum, AreaBound, rather than a bool: other changes also append parameters to repair_multi_polygon (systemed#939 a bool, systemed#949 a bool*), and with a bool here a merge that puts the parameters in the wrong order would still compile, silently. Verified: with the default the output is unchanged (Bangladesh z7 96/55: 166 features, 5 invalid, 319 holes). With "all" on the ocean layers, that tile goes to 4 invalid / 318 holes and the ocean feature behind a reported rendering artefact becomes valid; Denmark z5 16/10 improves 5 -> 1 invalid while Funen correctly stays land; Philippines z5 26/14 improves 7 -> 1 invalid with no land wrongly covered by ocean.
b60cd0e to
0beaab1
Compare
|
@dktue thanks a lotI took your commit unchanged and only re-applied it on the current master, because #961 landed after your rebase (no conflicts with it). I think we can merge without the decision if the "all" option should be default on for ocean and water. This could be a seperte 2 line PR when decided also showing own history / comment in commits |
|
This is great, thank you. I think we can enable it in the I guess the only thing I'd suggest is that the option name is quite verbose - we could just call it |
As suggested in review: "invalid_polygon_repair_scope": "all" becomes "repair_polygons": true (default false, i.e. unchanged behaviour), in line with options such as combine_points. Enable it for the water and ocean layers of the OpenMapTiles profile.
|
Thanks. changed/renamed it (also in docs) and enabled on the 2 layers |
(This code was created with AI assistance)
Problem
Repair polygon PR 908 corrected polygon errors after simplification but still there could be issues im complex geometries like holes in river deltas or coastal regions like Philippines mentioned in discussion 909
Polygons written to a tile can be GEOS-invalid even when the source geometry is perfectly valid and no simplification is applied.
Root cause
It is not the clipper. Feeding real ocean shapefile polygons through
fast_clipand testing the result with GEOS, every sampled polygon came back valid. The invalidity is introduced afterwards, inTileBbox::scaleGeometry/scaleRing:snapping float coordinates onto the integer tile grid (
floor, plus the 4-point backtracking dedup) collapses near-coincident vertices and creates self-intersections and touching rings.Today
repair_multi_polygon()runs only whensimplifyLevel > 0, i.e. only for geometry that simplification may have broken. Layers that are merely clipped and quantised — typically ocean/coastline shapefiles — are never repaired, so thedamage goes straight into the tile.
What this PR adds
As a defnsive approach a new config option per layer is introduced "invalid_polygon_repair_scope" with default "simplified_only" (current code flow) and the new option "all" as superset: additionally repair polygons that were only clipped and quantised
extend the area guard
repair_multi_polygon() rejects a repair whose result is less than 50% of the original area, to avoid the catastrophic collapse that buffer(0) can cause on large multipolygons. For clipped-but-unsimplified geometry an upper bound is needed as well: a repair that grows the polygon has filled a hole, which means an island silently disappears from the map.
That upper bound must not be applied to simplified geometry — simplification legitimately changes area in both directions, and enforcing the bound globally made things worse in testing (one Bangladesh z7 tile went from 5 to 20 invalid features). So repair_multi_polygon() takes a strictArea flag, set only when simplifyLevel == 0.
Results
verified with a complete planet build. All known effects, mostly holes in very complex river deltas, are fixed. no new visual effects introduced. Tests with setting the new option on the ocean layer fixes
Tests with activating the option on ocean + water_polygons fixes also very minor effects like in a harbor in a Switzerland lake
No mentionable effect on performance when active for ocean and water_polygon layers
Scope / limitations
This is a repair pass, not a fix of the quantisation itself. It does not reach full validity in every case — the remaining class of failures needs true snap-rounding at tile-grid precision (GEOS GEOSGeom_setPrecision or an equivalent)
Touching quantisation would have been very big task and alternatively i didn't want to introduce libgeos, - not as a new dependeny and also not because of maybe licence implications as it is lgpl
Fixes #861
Fixes #580