-
Notifications
You must be signed in to change notification settings - Fork 2.2k
POC: fix(mapbox) MapboxOverlay Resize Handling in Interleaved Mode #9877
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Draft
chrisgervang
wants to merge
7
commits into
master
Choose a base branch
from
chr/fix-9666
base: master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
+108
−1
Conversation
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
The issue was that in interleaved mode, deck.gl shares the WebGL context with Mapbox/Maplibre, but the drawing buffer sizes were getting out of sync
during resize events.
The solution involved three changes:
1. Set autoResize: true and useDevicePixels: false (mapbox-overlay.ts:143-146) - This allows luma.gl to handle resizing, but prevents DPR scaling
issues.
2. Listen to map resize events (mapbox-overlay.ts:152) - Added map.on('resize', this._handleInterleavedResize) to handle when the map canvas resizes.
3. Manually synchronize drawing buffer dimensions (mapbox-overlay.ts:246-267) - In the resize handler:
- Set device.canvasContext.drawingBufferWidth/Height to match the map's canvas dimensions
- Call _updateCanvasSize() to update deck's internal size tracking
- Call redraw() to render with the new size
Replaces manual assignment of drawing buffer dimensions with a call to syncDrawingBufferSize for luma.gl compatibility. Removes explicit redraw call, relying on map's render event to trigger deck redraw.
Adds a ResizeObserver to track canvas size changes, including device pixel ratio changes that do not trigger Mapbox's 'resize' event. Synchronizes luma.gl and Deck.gl internal size tracking with the actual canvas dimensions, ensuring correct rendering and fixing issues with white-out and redraws in interleaved mode.
Replaces the call to syncDrawingBufferSize with direct updates to drawingBufferWidth and drawingBufferHeight on the canvasContext. This ensures luma.gl's internal state matches the externally managed canvas size in interleaved mode.
Introduces a canvas element to the Map mock for testing purposes, including getCanvas and getPixelRatio methods. This enhances the mock's compatibility with code expecting these Mapbox GL JS API features.
Adds a check to ensure that a viewport exists before attempting to activate it in MapboxOverlay. Prevents potential errors when the viewports array is empty.
Collaborator
|
Collaborator
|
This is an example of where we have significant duplication |
Actually, maybe the problem isn't with activateViewport but something else. Maybe calling activateViewport is wrong in the first place during resize. Looking back at deck.gl's internal _updateCanvasSize, it only calls activateViewport but deck.gl controls its own canvas, whereas in interleaved mode we don't. Maybe we shouldn't be calling activateViewport at all?
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.
Summary
This is a POC to demonstrate what the problems were and how they can be fixed without any API changes. I focused testing on Maplibre in PureJS, since the issues are always reproducible there. I still need to test the react environment, mapbox, and prune out unnecessary code.
And, once the dust has settled, I'd like to discuss if there are sensible changes to make in luma or deck's API, or if this integration complexity is just unavoidable, and our best choice is to just isolate it to the mapbox module with integration test coverage.
This comment was a big clue #9856 (comment) and it may be that
GoogleMapsOverlayneeds a similar resize handler... consider both fixes when considering API changes.(Potentially) Closes #9666
Problem
When using
MapboxOverlaywithinterleaved: true, deck.gl layers would get out of sync with the underlying Mapbox/Maplibre map during:This occurred because deck.gl wasn't properly tracking canvas size changes when the map controlled the canvas.
Solution
Modified
modules/mapbox/src/mapbox-overlay.tsto synchronize dimensions between the map and deck.gl.Key Changes:
Disabled autoResize (line 147): Set
autoResize: falsein deviceProps since the map controls canvas size, not deck.glAdded ResizeObserver (lines 161-169): Watches for canvas size changes including DPR changes that don't trigger map resize events
Implemented
_handleInterleavedResize()(lines 269-337): Core resize handler that:canvas.width/heightas the drawing buffer size (lines 286-287)drawingBufferWidth/Heighttracking (lines 301-306)cssToDeviceRatiocalculation (lines 309-316)width/heightto CSS pixels (lines 321-323)viewManagerandlayerManager(lines 327-329)Fixed event handler cleanup (lines 155-156, 209-213): Properly stores and removes map resize handler reference
Added private fields (lines 48-50): Track resize observer, last canvas size, and map resize handler
Key Insight
In interleaved mode, Mapbox/Maplibre controls the canvas and may use either CSS pixels or device pixels depending on configuration. The fix works by:
canvas.width/height(the true drawing buffer size)cssToDeviceRatio()handle the scaling factorTesting
Verified working for:
Files Modified
modules/mapbox/src/mapbox-overlay.ts- Core resize handling fix