Fix fill-extrusion heights for features with a non-numeric building_id - #13715
Open
enersis-pst wants to merge 1 commit into
Open
Fix fill-extrusion heights for features with a non-numeric building_id#13715enersis-pst wants to merge 1 commit into
enersis-pst wants to merge 1 commit into
Conversation
|
Hey, @enersis-pst 👋 Thanks for your contribution to Mapbox GL JS! Important: This repository does not accept direct merges. All changes go through our internal review process. What happens next:
Please respond to any review comments on this PR. For more details, see CONTRIBUTING.md. |
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.
Fixes #13712.
The bug
A
fill-extrusionlayer whose features carry abuilding_idstring (a UUID, in thereporter's case) renders with wrong heights over terrain. Removing the property makes
the heights correct again.
Root cause
building_idgroups the parts of one building so that they share a centroid. It wasread with
Number(feature.properties['building_id'])and never validated, so anynon-numeric value became
NaN.NaNis where this turns into a visible bug:MapandSetkeys compare withSameValueZero, under which
NaNequals itself. So thebuildingGroupsmap collapsedevery feature with a non-numeric
building_idinto a single group._finalizeBuildingGroups()then gave each of them one shared centroid and the boundingbox merged across all of them. Since
fill-extrusion-height-alignmentdefaults toflat, that centroid is exactly what decides the roof elevation over terrain — so eachbuilding got a roof height derived from the average of all unrelated buildings.
The same
NaNpattern was latent inBuildingBucket, where aNaNid in thedisabledBuildingsset would have disabled every other building along with it.The fix
resolveBuildingId()insrc/data/building_id.tsreports unusable values as absentinstead of returning
NaN. Both buckets fall back to their existing "nobuilding_id"path, which for fill extrusions means grouping by feature id — the behaviour the reporter
already confirmed as correct. A single
warnOncemakes the cause visible.Numeric strings such as
"42"keep working, which was the intent of a60f5c6(
building_idvalues are compared against feature ids, and those are always numbers).Note this does not add native support for string
building_idvalues, one of the optionsraised in the issue.
buildingIdis packed into aFloat64Arrayand into aSet<number>used for conflation, so a string would need a stable hash with a collisionrisk — a feature decision rather than part of this bug fix.
Tests
test/unit/data/building_id.test.ts— the resolver's return values.test/unit/data/fill_extrusion_bucket.test.ts— bucket-level behaviour: two featureswith distinct non-numeric ids keep separate centroids and bounding boxes, features
sharing a numeric id still get merged, and non-numeric ids fall back to the feature id.
Without the fix both UUID buildings share the centroid
26006,26006.test/integration/render-tests/fill-extrusion-terrain/flat-roof-non-numeric-building-id/— two boxes on a slope with ~38m of terrain between them. Verified in both directions:
it passes with the fix and fails without it (without it the downhill box is stretched
and the uphill box is squashed).
Launch Checklist
@mapbox/map-design-team@mapbox/static-apisif this PR includes style spec API or visual changes. — no style spec change@mapbox/gl-nativeif this PR includes shader changes or needs a native port. — no shader change, but the sameNumber()conversion likely exists on the native side@mapbox/gl-nativeif this PR disables any test because it also needs to be disabled on their side. — no test disabledgl-native... — not applicable