feat: Support two render modes: Standard/deck.gl-first and MapboxOverlay #921
Conversation
kylebarron
changed the title
| Indicates if a click handler has been registered. | ||
| """ | ||
|
|
||
| render_mode = t.Unicode(default_value="deck-first").tag(sync=True) |
There was a problem hiding this comment.
This will be further improved on from the Python side to connect with #908
src/renderers/overlay.tsx
Outdated
| mapStyle, | ||
| customAttribution, | ||
| initialViewState, | ||
| // deckRef, |
There was a problem hiding this comment.
I'm not exactly sure how to pass through the deckRef to get a deck.gl reference. I figure I probably have to modify DeckGLOverlay somehow?
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
src/renderers/overlay.tsx
Outdated
| mapStyle, | ||
| customAttribution, | ||
| initialViewState, | ||
| // deckRef, |
There was a problem hiding this comment.
[nitpick] Remove commented-out code or add a TODO comment explaining why it's temporarily disabled. Dead code reduces readability.
| // deckRef, | |
Copilot uses AI. Check for mistakes.
src/renderers/overlay.tsx
Outdated
| style={{ width: "100%", height: "100%" }} | ||
| > | ||
| <DeckGLOverlay | ||
| // ref={deckRef} |
There was a problem hiding this comment.
[nitpick] Remove commented-out code or add a TODO comment explaining why it's temporarily disabled. Dead code reduces readability.
| // ref={deckRef} |
Copilot uses AI. Check for mistakes.
| // @ts-expect-error useDevicePixels should allow number | ||
| // https://github.com/visgl/deck.gl/pull/9826 | ||
| useDevicePixels: isDefined(useDevicePixels) ? useDevicePixels : true, |
There was a problem hiding this comment.
[nitpick] Consider using a more specific type assertion instead of @ts-expect-error to make the intent clearer and safer.
| // @ts-expect-error useDevicePixels should allow number | |
| // https://github.com/visgl/deck.gl/pull/9826 | |
| useDevicePixels: isDefined(useDevicePixels) ? useDevicePixels : true, | |
| // useDevicePixels should allow number (see https://github.com/visgl/deck.gl/pull/9826) | |
| useDevicePixels: (isDefined(useDevicePixels) ? useDevicePixels : true) as boolean | number, |
Copilot uses AI. Check for mistakes.
There was a problem hiding this comment.
@kylebarron this is working well and looks good to merge. It would be good to add Playwright test coverage for the overlay mode, but I haven’t found the right way to handle pointer events yet. If we figure that out, we can implement it separately. The lockfile is outdated, we should probably run npm i and update it before merging. Also, we can remove the read code from the overlay component for now and reintroduce it later once we have a proper solution.
|
Thanks!
Oh I missed that; I made #927 to validate on CI; and then I can bump the lockfile in another PR. |
## Summary Adds e2e test coverage for bbox selection in overlay render mode, validating the functionality introduced in #921. ## Changes - Add `bbox-select-overlay.spec.ts`: test suite for overlay mode bbox selection - Add `maplibre.ts` helper for MapLibre-specific bbox drawing - Adjust timing: increase overlay init wait (2s → 5s) ## How to verify ```bash npm run test:e2e ``` Ready for review.
…#935) ### Change list - Add `basemap` parameter to `Map`. This now manages a class with more information than just the basemap style. In particular, this holds the render mode `"interleaved"`, `"overlaid"`, or `"reverse-controlled"` that was implemented on the JS side in #921 - Deprecate `basemap_style` parameter to `Map`. This is superseded by the `style` parameter to `MaplibreBasemap` - Rename `CartoBasemap` to `CartoStyle` - Add `before_id` parameter to the core `Layer`. When passed and the basemap render mode is `"interleaved"`, - Pass `interleaved` prop down to MapboxOverlay - Remove temporary `render_mode` from #921 **Rendering layer interleaved in maplibre layer stack:** <img width="817" height="505" alt="image" src="https://github.com/user-attachments/assets/c63148a9-daa0-4dd4-bb65-88cc13805060" /> TODO: - [ ] Validation for `beforeId`. If `beforeId` is passed to a layer, try to resolve the basemap style URL to a dict, and then validate that the string value of `beforeId` exists in that layer stack. Keep a cache of fetched styles so that you're not fetching them repeatedly. - [ ] Warn if `beforeId` is set when the basemap style is not `"interleaved"`? - [ ] (next PR): integration with https://github.com/geopandas/xyzservices (see #494) - [ ] (possibly future PR): examples of rendering different basemap modes For #494 --------- Co-authored-by: Vitor George <[email protected]>
2 participants
As described in the upstream deck.gl docs, there are three ways to support using deck.gl with Maplibre: interleaved, overlaid, and reverse-controlled. The first two are supported via
MapboxOverlaywith a propinterleaved: true|false, while the latter is implemented by having Maplibre be a child of the deck.gl Map.There are worthwhile reasons to support all of these modes. We need to use interleaved or overlaid to support globe view, while reverse-controlled better supports multiple deck.gl views.
This PR refactors the map component in
index.tsxinto two separate React components: a deck.gl-first renderer (i.e. "reverse-controlled") and a MapboxOverlay renderer.The idea is that this will pair with #908 to give users more control over various ways of rendering maps.
This is backwards-compatible because we default to reverse-controlled, the existing default.
Closes #890, closes #437, for #886, for #718