fix(3d-buildings): grow tile culling bounds near the horizon#7932
fix(3d-buildings): grow tile culling bounds near the horizon#7932clement-igonet wants to merge 11 commits into
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #7932 +/- ##
==========================================
+ Coverage 93.36% 93.38% +0.02%
==========================================
Files 288 288
Lines 24422 24429 +7
Branches 6454 6454
==========================================
+ Hits 22801 22813 +12
+ Misses 1621 1616 -5 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
Interesting approach, can you add a render test? I think there are a few tiles with buildings already part of this repo so you can reuse those. |
…e#7633) Without terrain, getTileBoundingVolume() hardcoded a tile's bounding box to a flat 0 elevation, so it had no way to represent extruded features (3D buildings) on that tile. As the camera pitches up toward the near- horizontal cap, the frustum's bottom edge sweeps past these flat boxes and tiles get culled even though their buildings would still be visibly poking into view - reported as buildings disappearing when looking up while walking around a city in first person. Rather than a flat elevation-based workaround (the maintainer's option 2 in the issue thread - a fixed buffer applied everywhere, even when pitch is low and current culling is already correct), this ties the extra bounds to FOV: they only grow as the frustum's bottom edge - computed from both pitch and fov, not pitch alone - approaches horizontal, via getElevationForTileCulling() in covering_tiles.ts. At normal pitch the buffer is ~0 and culling is unchanged; it only kicks in for the steep look-up case this issue is about. Applies to both the mercator and globe covering-tiles providers. The globe provider caches bounding volumes keyed only by tile z/x/y (plus whether terrain is set) - since elevation now varies continuously with pitch/fov instead of being static, it's added to that cache key too, so frames don't get served a stale, pre-fix bounding volume. Includes dedicated tests for getElevationForTileCulling at pitch 80/90/100 - the range the reported bug was observed across - plus pitch 60 (partial ramp) and 45 (unaffected baseline).
2109560 to
a42c50e
Compare
|
Reviewer verification aid: a standalone repro/test page, walking a real vector-tile city (OpenFreeMap's Happy to share the actual file directly if useful - didn't want to clutter the PR with a large inline code dump. Ping me and I'll get it to you however's easiest. |
First off all, can you please not commuincate trough an LLM? That is a bit rude 😉 Here is how we do render tests -> https://github.com/maplibre/maplibre-gl-js/blob/main/test/integration/README.md |
|
Fair enough, point taken 😄 Trimmed the earlier comment down too. Added a render test at |
Reuses the existing 14-8802-5374.mvt building tile fixture at steep pitch (80°, maxPitch raised to 90 so it isn't clamped) with a tall extrusion height, so tile-covering bounds that ignore building height would visibly cut buildings off near the top of frame. Confirmed it fails against the pre-fix covering_tiles.ts/mercator_covering_tiles_details_provider.ts/ bounding_volume_cache.ts and passes with the fix.
CommanderStorm
left a comment
CommanderStorm
left a comment
There was a problem hiding this comment.
Promising, but needs cleanup
There was a problem hiding this comment.
before, for reference:
There was a problem hiding this comment.
Right, that is the pre-fix rendering; the committed expected.png is the after state, with the buildings kept near the frustum edge.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
| // At pitch 80 (close to the 89.25° horizon cap once fov/2 is | ||
| // added), getElevationForTileCulling grows the assumed | ||
| // elevation so tiles whose extruded features could still be | ||
| // visible near the horizon aren't dropped - see #7633. |
There was a problem hiding this comment.
| // At pitch 80 (close to the 89.25° horizon cap once fov/2 is | |
| // added), getElevationForTileCulling grows the assumed | |
| // elevation so tiles whose extruded features could still be | |
| // visible near the horizon aren't dropped - see #7633. |
Added contributor information for the bug fix regarding 3D buildings disappearing.
| * the frustum's bottom edge approaches the horizon, where a ground-level bounding | ||
| * box would cull tiles whose extruded features are still visible. | ||
| */ | ||
| export function getElevationForTileCulling(transform: IReadonlyTransform): number { |
There was a problem hiding this comment.
Is this exported only to be unit tests? If this is the case I think better tests should be used.
Since this is a relatively small method I think it should be easy to cover it using existing tests.
|
Looks good over all, I've added some comments. |
3 participants
Launch Checklist
covering_tiles.test.ts'spitchedcase, and added dedicated tests forgetElevationForTileCullingat pitch 45/60/80/90/100.)CHANGELOG.mdunder the## mainsection.Assisted-By: Claude Sonnet 5 (claude-sonnet-5)
What
Fixes #7633 - 3D buildings disappearing when the camera looks up.
Without terrain,
getTileBoundingVolume()hardcoded a tile's bounding box to a flat0elevation, so it had no way to represent extruded features (3D buildings) standing on that tile. As the camera pitches up toward the near-horizontal cap, the frustum's bottom edge sweeps past these flat boxes and tiles get culled even though their buildings would still be visibly poking into view.@NathanMOlson sketched two options in the issue thread:
elevation(already a parameter, but only used inside the terrain branch), outside the terrain branch too - "might be" mergeable, but a fixed buffer applied everywhere, even when pitch is low and current culling is already correct.This PR takes a third approach discussed on the issue: tie the extra bounds to FOV, so they only grow as the frustum's bottom edge - computed from both
pitchandfov, not pitch alone - approaches horizontal (maxMercatorHorizonAngle, 89.25°). At normal pitch the buffer is~0and culling is unchanged from today; it only kicks in for the steep look-up case this issue is about. SeegetElevationForTileCulling()incovering_tiles.tsfor the implementation and reasoning.Applies to both the mercator and globe covering-tiles providers. The globe provider caches bounding volumes keyed only by tile
z/x/y(plus whether terrain is set) - since elevation now varies continuously with pitch/fov instead of being static, it's added to that cache key too, so frames don't get served a stale, pre-fix bounding volume.Testing
npm run typecheck,npm run lint: clean.npm run test-unit: full suite passing (2820 tests).covering_tiles.test.ts > pitched), and added dedicated tests forgetElevationForTileCullingcovering pitch 45 (baseline, unaffected), 60 (partial ramp), and 80/90/100 (the range the reported bug was observed across - fully buffered at the default FOV).liberty-style buildings, walking/pitching 80-100° in first person): nearby buildings that previously vanished around pitch ~90 now stay rendered throughout the range. The exact assumed-height constant (ASSUMED_MAX_FEATURE_HEIGHT_METERS = 500) and onset threshold (TILE_CULLING_HORIZON_ONSET_DEGREES = 15) are the main things worth discussing/tuning if reviewers have thoughts.npm run benchmarkif a reviewer wants numbers; expect negligible impact away from the near-horizon case, since the buffer is 0 there and the code path is unchanged.