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

[Doc] Update Mapbox docs regarding deck v9 and react-map-gl v7 #8146

Merged
merged 18 commits into from Mar 13, 2024

Conversation

jonenst
Copy link
Contributor

@jonenst jonenst commented Sep 28, 2023

For #8541

Background

Hi, this started as a very light pull request but I've now included many changes which I think are improvement over the current docs. Let me know what you think. Please see #8147 for some background

Copy link
Collaborator

@Pessimistress Pessimistress left a comment

Choose a reason for hiding this comment

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

Thanks for taking time to update the docs. I think the "three options" section in "Using with Mapbox" is getting quite confusing, and I would like to offer some further clarifications.

  • MapboxLayer is no longer recommended. Its functionality can be fully replaced by MapboxOverlay with interleaved: true. So the three options are:
    1. Using <DeckGL> as the parent component of <Map>
    2. Using <Map> as the parent of MapboxOverlay, with interleaved: false
    3. Using <Map> as the parent of MapboxOverlay, with interleaved: true
  • You should use option 2|3 if you want to use more mapbox-gl features, such as controls and terrain.
  • You should use option 1 if you want to use more Deck features, such as multi-view and effects.

docs/developer-guide/base-maps/using-with-mapbox.md Outdated Show resolved Hide resolved
docs/developer-guide/base-maps/using-with-mapbox.md Outdated Show resolved Hide resolved
@jonenst
Copy link
Contributor Author

jonenst commented Oct 4, 2023

Thanks for looking into this ! Do you want me to try and make a new version taking your remarks into account ? Or do you prefer to take it from here ? Even if I can do it if you want, Feel free to use everything I wrote if you prefer, I don't need to be the author or anything.

I was puzzled about MapboxLayer as well, and was actually going to open a separate issue once this one was finished to ask about it; good to know you already have the information.

Am i correct to understand that you do not want to keep two separate examples of navigationcontrol : deck as root + react-map-gl 5 + NavigationControl, as well as Map as root + react-map-gl 7 + deck through MapboxOverlay ? And only keep the second one?

Finally, one last question but maybe we can talk about this one in a separate issue (I was holding back like for the question of MapboxLayer): I think the following code is a bit weird, can it be simplified by offering a different API for mapboxOverlay ? Or does every project really need the few lines calling useControl and seemingly passing the props twice ?

import {useControl} from 'react-map-gl';

function DeckGLOverlay(props: MapboxOverlayProps & {
  interleaved?: boolean;
}) {
  const overlay = useControl<MapboxOverlay>(() => new MapboxOverlay(props));
  overlay.setProps(props);
  return null;
}

// in jsx
    <Map  ...   >
      <DeckGLOverlay layers={[scatterplotLayer]} />
...
    </Map>

Cheers,
Jon

@Pessimistress
Copy link
Collaborator

Do you want me to try and make a new version taking your remarks into account?

If you don't mind, your effort is much appreciated.

Am i correct to understand that you do not want to keep two separate examples of navigationcontrol

We don't need to keep the react-map-gl v5 example.

does every project really need the few lines calling useControl and seemingly passing the props twice?

You are hyper focused on your own use case here. MapboxOverlay is not React dependent. It can be used in vanilla JS. On the other hand, react-map-gl does not depend on deck.gl. If you want to use MapboxOverlay in React, you have to use react-map-gl's useControl hook to convert it from a generic mapbox-gl IControl to a React component.

@jonenst
Copy link
Contributor Author

jonenst commented Oct 7, 2023

MapboxOverlay is not React dependent. It can be used in vanilla JS. On the other hand, react-map-gl does not depend on deck.gl. If you want to use MapboxOverlay in React, you have to use react-map-gl's useControl hook to convert it from a generic mapbox-gl IControl to a React component.

Hi,
so on the question of having a definition of <DeckGLOverlay> in this repository instead of explaining in the docs how every project should define it, you are saying that @deck.gl/react shouldn't depend on mapbox and @deck.gl/mapbox shouldn't depend on react, so there's no place to host the code ? And for such small code it's not worth finding a place for it ?

I'm not sure but it feels like it's worth it to find a way to host this code somewhere, because using a simple deckgl data display on top of a basic map (and users expect basic maps to have navigation controls..) feels like a very common use case, not just my hyper focused use case (which is why you feel like I'm insisting I guess). I think it would be worth it to make <Map><DeckControl/> be just as easy to code as <Deck><Map/> . Do you have any ideas ? (create a new module ? put it in a existing module (are there technical reasons or only philosophical reasons not do to it ? Do optional dependencies help ? not an expert sorry)

Also, maybe a better implementation for DeckGLOverlay is

function DeckGLOverlay(props) {
  const overlay = useControl(() => new MapboxOverlay(props));
  useEffect(() => {
    overlay.setProps(props);
  }, [props])
  return null;
}

Using useEffect makes it easier to see what's happening and avoids calling setProps (even if it is meant to be cheap I guess) when nothing has changed and components are just rendered because the parent has rendered.

Looking at https://github.com/visgl/react-map-gl/blob/e11b41eefe73ff773bbec1db73277e4aca1f1bef/src/components/attribution-control.ts#L32, they also add in a memo() call (even though for this kind of component it's not very useful..), maybe add it too so that the DeckGLOverlay behaves more like other mapbox controls ?

as a note for myself, if you decide to modify anything regarding DeckGLOverlay, your example https://github.com/visgl/react-map-gl/blob/master/examples/deckgl-overlay/src/app.tsx should be updated as well as the examples in this repo.

Cheers,
Jon

Make v7 control compatibility use cases more prominent
@jonenst
Copy link
Contributor Author

jonenst commented Oct 8, 2023

Which example with a react-map-gl basemap would you use to illustrate "You should use option 1 if you want to use more Deck features, such as multi-view and effects.". Or should we link to an example not using any of Deck's advanced features but still using DeckGL as the root and Map as the child ? Should it be in get-started or can we like to website examples ?

Maybe this one ? examples/website/radio/app.jsx

For the full list, I see

examples/experimental/h3-grid/src/app.jsx
examples/website/contour/app.jsx
examples/website/carto-sql/app.jsx
examples/website/trips/app.jsx
examples/website/scenegraph/app.jsx
examples/website/text/app.jsx
examples/website/arc/app.jsx
examples/website/mask-extension/app.jsx
examples/website/brushing/app.jsx
examples/website/highway/app.jsx
examples/website/collision-filter/app.jsx
examples/website/3d-tiles/app.jsx
examples/website/heatmap/app.jsx
examples/website/geojson/app.jsx
examples/website/scatterplot/app.jsx
examples/website/line/app.jsx
examples/website/data-filter/app.jsx
examples/website/radio/app.jsx
examples/website/screen-grid/app.jsx
examples/website/3d-heatmap/app.jsx
examples/website/icon/app.jsx

Should some of them be converted to deckOverlay if they are not using advanced deck features ?

Remove all mentions of react-map-gl v5/v6, assume v7
use relative links for ../../api-reference
no longer recommend MapboxLayer unless specific constraints require it
@jonenst
Copy link
Contributor Author

jonenst commented Oct 8, 2023

I've pushed a version taking your remarks into account, feel free to add your own ideas to it

@vsg24
Copy link

vsg24 commented Feb 18, 2024

Why is this PR not merged yet? the current version of docs is very confusing since it's out of date...


Mapbox provides an example of [finding the first label layer](https://www.mapbox.com/mapbox-gl-js/example/geojson-layer-in-stack/). For more sophisticated injection point lookups, refer to Mapbox' documentation on the format of Mapbox style layers, see [Mapbox Style Spec](https://www.mapbox.com/mapbox-gl-js/style-spec/#layers).


In some cases, the application wants to add a deck.gl 3D layer (e.g. ArcLayer, HexagonLayer, GeoJsonLayer) on top of a Mapbox basemap, while seamlessly blend into the z-buffer. This will interleave the useful visualization layers from both the deck.gl and Mapbox layer catalogs. In this case, a `beforeId` is not needed.

## Compatibility
Copy link
Collaborator

Choose a reason for hiding this comment

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

I looked for the first release the WebGL2 compatibility commit appeared to make this table:

docs/README.md Outdated Show resolved Hide resolved
@chrisgervang
Copy link
Collaborator

Which example with a react-map-gl basemap would you use to illustrate "You should use option 1 if you want to use more Deck features, such as multi-view and effects.". Or should we link to an example not using any of Deck's advanced features but still using DeckGL as the root and Map as the child ? Should it be in get-started or can we like to website examples ?

Maybe this one ? examples/website/radio/app.jsx

For the full list, I see

examples/experimental/h3-grid/src/app.jsx
examples/website/contour/app.jsx
examples/website/carto-sql/app.jsx
examples/website/trips/app.jsx
examples/website/scenegraph/app.jsx
examples/website/text/app.jsx
examples/website/arc/app.jsx
examples/website/mask-extension/app.jsx
examples/website/brushing/app.jsx
examples/website/highway/app.jsx
examples/website/collision-filter/app.jsx
examples/website/3d-tiles/app.jsx
examples/website/heatmap/app.jsx
examples/website/geojson/app.jsx
examples/website/scatterplot/app.jsx
examples/website/line/app.jsx
examples/website/data-filter/app.jsx
examples/website/radio/app.jsx
examples/website/screen-grid/app.jsx
examples/website/3d-heatmap/app.jsx
examples/website/icon/app.jsx

Should some of them be converted to deckOverlay if they are not using advanced deck features ?

I like the examples you've included in the documentation. As for the rest of the React website examples, they should be written without @deck.gl/mapbox (meaning, written with ) where ever possible. Examples are meant to show the bare minimum necessary to work.

Comment on lines 11 to 13
- If you don't use the most advanced features of Deck, such as multi-view and effects, and want to use all the features of mapbox-gl, such as controls like `NavigationControl` or plugins, then you have to use Mapbox as the root element. The recommended approach is then to use the Deck canvas as an overlay on top of the basemap, inserted into the map container using [MapboxOverlay](../../api-reference/mapbox/mapbox-overlay#using-with-react-map-gl) from the [@deck.gl/mapbox](../../api-reference/mapbox/overview.md) module in its default mode (overlaid, which corresponds to `interleaved: false`). The [react get-started example](https://github.com/visgl/deck.gl/tree/master/examples/get-started/react/mapbox/) illustrates this pattern.
- Otherwise, if you need the more advanced features of Deck, then the recommended approach is to use Deck as the root element with its canvas as an overlay on top of the child Mapbox map. The [Minimap example](https://deck.gl/examples/multi-view) illustrates the basic pattern. This is the most tested and robust use case with respect to Deck's functionality, but you can't use all the features of mapbox-gl like controls (e.g. `NavigationControl`) and plugins.
- Finally, if you need to mix deck.gl layers with base map layers, e.g. having deck.gl surfaces below text labels or objects occluding each other correctly in 3D, then you have to use deck.gl layers interleaved with Mapbox layers in the same WebGL context. In addition to using Mapbox as the root element (option 1), you have to use [MapboxOverlay](../../api-reference/mapbox/mapbox-overlay#using-with-react-map-gl) in interleaved mode (`interleaved: true`). Be cautious that this feature subjects to bugs and limitations of mapbox-gl's custom layer interface, and is only compatible with WebGL2 (See [compatibility](../../api-reference/mapbox/overview#compatibility)). Here's an [interactive example](https://deck.gl/examples/mapbox), and in the following image, notice that the yellow circles (first deck.gl layer) are between the ground (first mapbox layer) and the labels (second mapbox layer) and also below the buildings (third mapbox layer) which correctly occlude the arcs (second deck.gl layer)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Summary of Current Examples:

Option 1: Mapbox/Maplibre as root, Deck as overlay

Option 2: Deck as root, Mapbox/Maplibre as child

Option 3: Mapbox/Maplibre as root, Deck layers interleaved in shared WebGL2 context.

  • React: Using react-map-gl <Map> as the parent of MapboxOverlay, with interleaved: true
    • Mapbox: Mapbox Website example.
    • Maplibre: Set interleave: true in the React getting-started examples in Option 1.
  • Pure JS: Using vanilla mapbox-gl as the root element for MapboxOverlay, with interleaved: true
  • Scripting: Interleaved rendering not supported.

docs/README.md Outdated Show resolved Hide resolved
docs/api-reference/mapbox/mapbox-overlay.md Outdated Show resolved Hide resolved
docs/api-reference/mapbox/overview.md Outdated Show resolved Hide resolved
docs/api-reference/mapbox/overview.md Outdated Show resolved Hide resolved
docs/api-reference/mapbox/overview.md Outdated Show resolved Hide resolved
docs/api-reference/mapbox/overview.md Outdated Show resolved Hide resolved
docs/api-reference/mapbox/overview.md Outdated Show resolved Hide resolved
docs/api-reference/mapbox/overview.md Outdated Show resolved Hide resolved
docs/developer-guide/base-maps/using-with-mapbox.md Outdated Show resolved Hide resolved
docs/developer-guide/base-maps/using-with-mapbox.md Outdated Show resolved Hide resolved
Co-authored-by: Ib Green <ib@foursquare.com>
@chrisgervang chrisgervang mentioned this pull request Feb 26, 2024
25 tasks
@coveralls
Copy link

coveralls commented Mar 7, 2024

Coverage Status

coverage: 83.892% (-0.01%) from 83.904%
when pulling aeed153 on jonenst:patch-2
into ada54fc on visgl:master.

Signed-off-by: Chris Gervang <chris@gervang.com>
@chrisgervang
Copy link
Collaborator

chrisgervang commented Mar 9, 2024

Just pushed up a bunch of changes taking feedback and the deprecation of MapboxLayer into account. Thank you for your feedback in advance!

Signed-off-by: Chris Gervang <chris@gervang.com>
@chrisgervang chrisgervang changed the title [Doc] Update using-with-mapbox.md regarding react-map-gl v7 [Doc] Update using-regarding deck v9 and react-map-gl v7 Mar 9, 2024
@chrisgervang chrisgervang changed the title [Doc] Update using-regarding deck v9 and react-map-gl v7 [Doc] Update Mapbox docs regarding deck v9 and react-map-gl v7 Mar 9, 2024
chrisgervang and others added 3 commits March 11, 2024 15:11
Signed-off-by: Chris Gervang <chris@gervang.com>
Signed-off-by: Chris Gervang <chris@gervang.com>
@chrisgervang chrisgervang merged commit 7b2c25e into visgl:master Mar 13, 2024
3 checks passed
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

6 participants