Skip to content

Fix fill-extrusion heights for features with a non-numeric building_id - #13715

Open
enersis-pst wants to merge 1 commit into
mapbox:mainfrom
enersis-pst:fix/non-numeric-building-id
Open

Fix fill-extrusion heights for features with a non-numeric building_id#13715
enersis-pst wants to merge 1 commit into
mapbox:mainfrom
enersis-pst:fix/non-numeric-building-id

Conversation

@enersis-pst

Copy link
Copy Markdown
Contributor

Fixes #13712.

The bug

A fill-extrusion layer whose features carry a building_id string (a UUID, in the
reporter's case) renders with wrong heights over terrain. Removing the property makes
the heights correct again.

Root cause

building_id groups the parts of one building so that they share a centroid. It was
read with Number(feature.properties['building_id']) and never validated, so any
non-numeric value became NaN.

NaN is where this turns into a visible bug: Map and Set keys compare with
SameValueZero, under which NaN equals itself. So the buildingGroups map collapsed
every feature with a non-numeric building_id into a single group.
_finalizeBuildingGroups() then gave each of them one shared centroid and the bounding
box merged across all of them. Since fill-extrusion-height-alignment defaults to
flat, that centroid is exactly what decides the roof elevation over terrain — so each
building got a roof height derived from the average of all unrelated buildings.

The same NaN pattern was latent in BuildingBucket, where a NaN id in the
disabledBuildings set would have disabled every other building along with it.

The fix

resolveBuildingId() in src/data/building_id.ts reports unusable values as absent
instead of returning NaN. Both buckets fall back to their existing "no building_id"
path, which for fill extrusions means grouping by feature id — the behaviour the reporter
already confirmed as correct. A single warnOnce makes the cause visible.

Numeric strings such as "42" keep working, which was the intent of a60f5c6
(building_id values are compared against feature ids, and those are always numbers).

Note this does not add native support for string building_id values, one of the options
raised in the issue. buildingId is packed into a Float64Array and into a
Set<number> used for conflation, so a string would need a stable hash with a collision
risk — 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 features
    with 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

  • Make sure the PR title is descriptive and preferably reflects the change from the user's perspective.
  • Add additional detail and context in the PR description (with screenshots/videos if there are visual changes).
  • Manually test the debug page.
  • Write tests for all new functionality and make sure the CI checks pass.
  • Document any changes to public APIs. — no public API change
  • Post benchmark scores if the change could affect performance. — parsing path only, no measurable effect
  • Tag @mapbox/map-design-team @mapbox/static-apis if this PR includes style spec API or visual changes. — no style spec change
  • Tag @mapbox/gl-native if this PR includes shader changes or needs a native port. — no shader change, but the same Number() conversion likely exists on the native side
  • Tag @mapbox/gl-native if this PR disables any test because it also needs to be disabled on their side. — no test disabled
  • Create a ticket for gl-native ... — not applicable

@enersis-pst
enersis-pst requested a review from a team as a code owner August 14, 2026 11:06
@enersis-pst
enersis-pst requested review from mourner and removed request for a team August 14, 2026 11:06
@github-actions

Copy link
Copy Markdown

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:

  1. A team member will review your PR here first
  2. If it looks good, they will import it to our internal repository for further review
  3. If approved, changes will be synced back here via our release process

Please respond to any review comments on this PR. For more details, see CONTRIBUTING.md.

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.

"building_id" property will show fill-extrusion layer with wrong heights

1 participant