Skip to content
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

fix(mapbox) leave additional views intact #6329

Merged
merged 17 commits into from Feb 22, 2022
Merged

Conversation

chrisgervang
Copy link
Collaborator

@chrisgervang chrisgervang commented Oct 27, 2021

Background

The deck/mapbox interleaved rendering needs to reproject the viewport to match mapbox when an external deck object is provided. Currently the extension assumes the first View is a MapView and deletes any other viewports, however deletion is unnecessary and not expected if provided multiple views (e.g. a minimap or info viz overlay).

Screen Shot 2021-10-23 at 10 38 49 PM

Result After Fix
TextLayer on OrthographicView renders on top of interleaved MapView.
Screen Shot 2021-10-23 at 10 42 05 PM

Change List

Related to interleaved mapbox rendering:

  • No viewId provided - same behavior as today
  • viewId provided and found - replace specified viewport
  • viewId provided and not found - do nothing

@coveralls
Copy link

coveralls commented Oct 27, 2021

Coverage Status

Coverage increased (+0.1%) to 82.635% when pulling 0b3fd1c on chr/fix-mapbox-viewports into d6c5619 on master.

docs/api-reference/mapbox/mapbox-layer.md Outdated Show resolved Hide resolved
@@ -14,7 +14,7 @@ Use deck.gl layers as custom Mapbox layers, enabling seamless interleaving of Ma

### Limitations

* deck.gl's multi-view system cannot be used.
* deck.gl's multi-view system only supports one MapView (e.g. minimaps are not supported).
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Technically speaking you can have multiple MapViews, the limitation is that you will always have a MapView that fills the entire canvas (what Mapbox draws into).

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, is that why minimap views can't use mapbox? I'd like to explain the limitation better in the overview in case its useful to someone hacking around it.

modules/mapbox/src/deck-utils.js Outdated Show resolved Hide resolved
modules/mapbox/src/deck-utils.js Outdated Show resolved Hide resolved
@Pessimistress Pessimistress removed the bug label Feb 21, 2022
@Pessimistress Pessimistress added this to the 8.7 milestone Feb 21, 2022
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.

None yet

3 participants