Stabilize polylabel placement#899
Open
Symmetricity wants to merge 2 commits into
Open
Conversation
Compute the polygon centroid seed relative to a local ring origin before translating it back to map coordinates. This isolates the centroid-origin change from the distance-key ordering candidate so it can be tested independently against the submitted PR stack.
Break exact priority-queue ties with existing cell geometry values so equivalent cells are not left to implementation-specific heap ordering. This keeps the local-origin centroid candidate isolated from the broader distance-key ordering change while testing whether the remaining macOS versus Linux/Windows point drift is caused by queue ordering alone.
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 makes
polylabelpoint placement deterministic across the tested nativebuilds.
Two small changes are included:
ring before translating it back to map coordinates.
std::priority_queueties with existing cell geometry valuesinstead of leaving equal-priority cells to implementation-specific heap
ordering.
The intended effect is stable generated point placement for
LayerAsCentroid()users such as the OpenMapTiles-compatiblehousenumberandpoilayers.Background
The generated-tile CI work found repeatable semantic differences between
native runners. macOS 14 and macOS latest matched each other, while Ubuntu
CMake, Ubuntu Makefile, and Windows CMake matched each other, but the two
runner groups placed some generated points at different MVT coordinates.
Focused diagnostics on one affected building showed that the source polygon
coordinates, envelope, and initial grid were identical across platforms. The
first divergence was the centroid seed calculated by
getCentroidCell():(9.5072016807699118, 53.476525214138611)with a lower distance score, sothe bbox seed won.
(9.5072058467781453, 53.476548647062621)with a higher distance score, sothe centroid seed won.
Those slightly different seed choices were enough to move some generated
points by one or two MVT coordinate units.
The centroid calculation was using absolute lon/lat-style coordinates directly
in shoelace sums for very small polygons. Shifting the arithmetic to a local
origin reduces cancellation in those sums. The queue tie-break then handles the
remaining case where cells have exactly equal priority.
Implementation
The patch is intentionally limited to
include/polylabel.h.getCentroidCell()now uses the first outer-ring point as a local arithmeticorigin:
The
polylabel()priority queue comparator still orders primarily bymax,the existing "potential" value. It now adds deterministic tie-breaks using
values already stored on each
Cell:d,h,x, andy.This does not change the public
polylabel()API, requested precision, or thegeometry algorithm used by callers.
Evidence
The candidate passed generated-tile verification in the native CI matrix used
for this investigation. Decoded MBTiles content matched across:
A 30-run repeat check with the OpenMapTiles profile and the Liechtenstein
Geofabrik extract also produced semantically identical output for every run
against repeat 01 (
changed_tiles: 0).The failed control case was the local-origin centroid seed without the queue
tie-break. That version still had a native CI semantic difference: macOS
matched macOS and Ubuntu/Windows matched Ubuntu/Windows, but one
housenumberpoint differed between those runner groups.
Performance
Performance was measured with matched RelWithDebInfo builds, once without this
patch and once with this patch.
Configuration:
resources/config-openmaptiles.jsonresources/process-openmaptiles.lua--threads 4I used alternating
/usr/bin/time -vruns to check wall time and peak RSS withboth builds running under the same conditions:
This paired sample suggests the memory footprint is effectively unchanged and
the patch was about 0.7% slower on this fixture.
The likely reason is that
polylabelpriority queues can contain many cellswith equal
maxvalues. The new comparator only does extra work for exact ties,but those ties appear common enough on this fixture to be measurable.
Related Issues And PRs
Related centroid/polylabel context:
polylabelbehavior and housenumber placement throughLayerAsCentroid()/Centroid().LayerAsCentroid()behavior around Lua interop and relationmembers.
I did not find an existing issue or PR that directly reports this exact
cross-runner generated-point determinism bug.
Possible Regressions
This can change generated centroid/label point coordinates for polygonal
features that use
polylabel, especially tiny polygons where absolutecoordinate cancellation affected the centroid seed or where queue cells had
equal priority.
Expected behavior:
polylabelalgorithm;
byte-for-byte deterministic by this patch;
The queue comparator performs a few additional comparisons when
maxvaluesare exactly equal. On the Austria fixture, that was measurable as a slowdown in
timing tests, so this should be treated as a determinism/correctness tradeoff
rather than a performance-neutral change.
Testing
git diff --check origin/master..fix/deterministic-polylabel-placement cmake -S . -B build -DCMAKE_BUILD_TYPE=RelWithDebInfo cmake --build build --target tilemaker -j2Generated-tile checks: