fix(map): correct basemap symbol occlusion under draped rasters in 3D… - #1703
fix(map): correct basemap symbol occlusion under draped rasters in 3D…#1703RohithPariki wants to merge 2 commits into
Conversation
… terrain ## Summary This PR fixes a visual bug where vector basemap labels and symbols were incorrectly rendering on top of overlying opaque raster layers when the 3D Terrain plugin was active. ## Problem In Issue opengeos#781, users observed that the layer stack hierarchy was broken when using 3D terrain. A designated "Background" basemap layer would incorrectly draw its labels, buildings, and vector footprints directly over top-level, fully opaque user rasters (e.g., Esri Wayback, USGS Topo). ## Root Cause This was caused by a known rendering limitation in our underlying engine, `maplibre-gl-js`. In 3D terrain mode, MapLibre uses a two-pass rendering system: draped layers (rasters, fills) are drawn to a texture and draped on the terrain mesh, while non-draped symbols and extrusions are drawn "live" in a separate pass afterwards to avoid clipping. This decoupled rendering meant that all basemap symbols floated above the previously drawn rasters, effectively ignoring the 2D layer list hierarchy. ## Solution Instead of attempting a fragile workaround within GeoLibre's UI layer-sync logic, this PR applies a surgical patch directly to our `maplibre-gl` dependency using `patch-package`. The patch backports an open upstream fix (MapLibre PR #7852) which modifies `RenderToTexture.ts` to identify the highest opaque draped raster layer and explicitly skips rendering for symbol layers placed below it. *Testing Note:* Verified locally on Windows. The frontend UI test suite passes successfully. The 8 subtest failures observed locally were confirmed to be WSL/bash path resolution issues in `metainfo-generated.test.ts` (Linux AppImage packaging) running on a Windows host, and are unrelated to this UI fix. ## References Closes opengeos#781
📝 WalkthroughWalkthroughThe package now applies a MapLibre patch after installation. The patch detects opaque draped raster layers and skips symbols below them during render-to-texture processing. Tests cover symbols, raster opacity, and fill-extrusions. ChangesOpaque raster render ordering
Estimated code review effort: 3 (Moderate) | ~20 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint install failed: dependency version conflict. Check your lock file or package.json. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
patches/maplibre-gl+5.24.0.patch (1)
88-118: 🩺 Stability & Availability | 🔴 Critical | ⚡ Quick winRegenerate the patch-package artifact without appending the email-formatted patch.
patches/maplibre-gl+5.24.0.patchends with anothergit format-patchblock starting at line 88 (From ...,Subject: [PATCH 2/2], and anotherdiff --git).patch-packageruns frompostinstall, so this malformed patch makesnpm cifail. Replace it with a single regenerated patch that includes all required changes.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@patches/maplibre-gl`+5.24.0.patch around lines 88 - 118, Regenerate the patches/maplibre-gl+5.24.0.patch artifact from the intended maplibre-gl changes using patch-package, ensuring it contains one valid patch only and no appended git format-patch email block. Preserve the required isRasterStyleLayer guard change in RenderToTexture.Source: Pipeline failures
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@patches/maplibre-gl`+5.24.0.patch:
- Around line 88-118: Regenerate the patches/maplibre-gl+5.24.0.patch artifact
from the intended maplibre-gl changes using patch-package, ensuring it contains
one valid patch only and no appended git format-patch email block. Preserve the
required isRasterStyleLayer guard change in RenderToTexture.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 86c31540-4dc7-423e-a1ea-4198c6526fd2
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (2)
package.jsonpatches/maplibre-gl+5.24.0.patch
|
@RohithPariki Thank you for looking into it. The issue is being fixed in the upstream. I would prefer not to apply a surgical patch in GeoLibre. |
|
Makes complete sense! I'll close this PR and we can wait for the upstream MapLibre release to bump the dependency. |
… terrain
Summary
This PR fixes a visual bug where vector basemap labels and symbols were incorrectly rendering on top of overlying opaque raster layers when the 3D Terrain plugin was active.
Problem
In Issue #781, users observed that the layer stack hierarchy was broken when using 3D terrain. A designated "Background" basemap layer would incorrectly draw its labels, buildings, and vector footprints directly over top-level, fully opaque user rasters (e.g., Esri Wayback, USGS Topo).
Root Cause
This was caused by a known rendering limitation in our underlying engine,
maplibre-gl-js. In 3D terrain mode, MapLibre uses a two-pass rendering system: draped layers (rasters, fills) are drawn to a texture and draped on the terrain mesh, while non-draped symbols and extrusions are drawn "live" in a separate pass afterwards to avoid clipping. This decoupled rendering meant that all basemap symbols floated above the previously drawn rasters, effectively ignoring the 2D layer list hierarchy.Solution
Instead of attempting a fragile workaround within GeoLibre's UI layer-sync logic, this PR applies a surgical patch directly to our
maplibre-gldependency usingpatch-package. The patch backports an open upstream fix (MapLibre PR #7852) which modifiesRenderToTexture.tsto identify the highest opaque draped raster layer and explicitly skips rendering for symbol layers placed below it.Testing Note: Verified locally on Windows. The frontend UI test suite passes successfully. The 8 subtest failures observed locally were confirmed to be WSL/bash path resolution issues in
metainfo-generated.test.ts(Linux AppImage packaging) running on a Windows host, and are unrelated to this UI fix.References
Closes #781
Summary by CodeRabbit
Bug Fixes
Chores