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

Interleaved support with DeckGL #8

Open
padawannn opened this issue Sep 28, 2023 · 21 comments
Open

Interleaved support with DeckGL #8

padawannn opened this issue Sep 28, 2023 · 21 comments

Comments

@padawannn
Copy link

I would like to open a discussion about Interleaved support.

Imagine that you have a DeckGL project where you want to change between MapLibre and Google Maps with interleave control. For MapLibre you would have something like this

<DeckGL
    initialViewState={INITIAL_VIEW_STATE}
    layers={layers}
    controller={true}
    onViewStateChange={...}>
    <MapLibre />
  </DeckGL>

where the camera control is done by Deck but if you change it to Google Maps with interleave support you would have something like this example where the map camera control is done by the Google Maps component instead DeckGL.

It's difficult to maintain and it can also be difficult to integrate with libraries like NebulaGL that interacts directly with the map events. So my question is if there would be a way to support interleave while keeping Deck as a wrapper of the map, for example:

<DeckGL
    initialViewState={INITIAL_VIEW_STATE}
    layers={layers}
    controller={true}
    onViewStateChange={...}>
    <Map interleaved={true} {...GOOGLE_MAPS_MAP_OPTIONS} />
  </DeckGL>
@felixpalmer
Copy link

Just to add some implementation thoughts to this:

In order for the interleaving to work, the React wrapper would need to implement the google.maps.WebGLOverlayView in order to allow deck.gl (or other libraries) to draw to the shared WebGL canvas.

This is how it works in the GoogleMapsOverlay in deck.gl, the google.maps.WebGLOverlayView.onDraw method triggers the render of DeckGL:

https://github.com/visgl/deck.gl/blob/master/modules/google-maps/src/google-maps-overlay.ts#L246-248

The React wrapper is already special-casing deck.gl in some places, for example here to update the view state, so it doesn't seem unreasonable to support the render callback

@usefulthink
Copy link
Collaborator

I'm not sure how that would work, and I certainly have to look into how that is done for maplibre and mapbox. My current understanding is that we need to use the GoogleMapsOverlay implementation to be able to do interleaved rendering due to the webgl context-sharing and I'm very curious how it is even possible to do that in map(box|libre) when there's a component boundary between deck.gl and the basemap.

@usefulthink
Copy link
Collaborator

@felixpalmer can you maybe provide some hints how you achieved that in react-map-gl? I can't seem to find any mention of that in the docs, so have to dive into the implementation..

@ibgreen
Copy link

ibgreen commented Sep 28, 2023

Just curious, is it possible to create a working example right now that supports all basemap variants (maplibre/maplibre interleaved/google maps/google maps interleaved)?

Even if it requires lots of ugly conditional cases, it could be instructive to help us see where simplification/API unification opportunities are.

I am not really up to date on how all the different variants look, hard to weigh in without seeing where we are at today.

@Pessimistress
Copy link

The <DeckGL><Map/></DeckGL> pattern does not support interleaving. To use interleaving, deck must be created as a MapboxOverlay/GoogleMapsOverlay, and used as a child of the map. Any pattern otherwise is misleading the user to believe that they can just replace the base map on the fly, which is not true.

Here is an example of how react-map-gl does it: https://github.com/visgl/react-map-gl/blob/master/examples/deckgl-overlay/src/app.tsx

@felixpalmer
Copy link

@Pessimistress I'm aware it isn't currently supported in the <DeckGL><Map/></DeckGL> pattern, but would it not be technically possible to use the same integration approach used in the XXXOverlay classes to add the interleave support?

@usefulthink
Copy link
Collaborator

usefulthink commented Oct 2, 2023

I've been thinking about this for a bit, and I think there's a viable solution. This is just a rough sketch, so everything is subject to change and there might be a lot of details I glossed over or don't yet know about. Still, maybe worth putting this here for discussion anyway:

In case of DeckGL > MapRenderer (think type MapRenderer = google.maps.Map | mapbox.Map | ...), the DeckGL is fully in control of rendering and the view data is passed from DeckGL to the map via the viewport / viewState props. This relies on the assumption that all map renderers will instantly update with changes to the view-props. (Tangentially related: those props are historically mapbox-specific and don't necessarily map well to other renderers. A solution for this could also be integrated into what I outlined below.)

For interleaved rendering the control of rendering is the other way around: the MapRenderer has to control rendering, since it has to coordinate rendering the different passes with the overlays being drawn somewhere in the middle.

To get this to work in the DeckGL > MapRenderer structure, a bit more coordination is needed.

The DeckGL component could create the overlay classes needed (GoogleMapsOverlay / MapboxOverlay / (arcGis.DeckRenderer?)), but it needs access to the map-instance to do that. For this, a MapRendererInterface could be defined that would be returned by the basemap-renderer via useImperativeHandle() and forwarded refs. The DeckGL component could then use that interface to create the overlay.

A schematic implementation for this would roughly look like this:

// a class implementing this interface is returned via ref from all 
// map-renderers
interface MapRendererInterface<T = any> {
  rendererType: string;
  mapReadyPromise: Promise<T>;
  map?: T; // google.maps.Map | mapbox.Map | maplibre.Map | ...
}

const DeckGL = props => {
  const mapRenderer = findMapRendererComponent(props.children);
  const [rendererApi, setRendererApi] = useState(null);
  const rendererRef = useCallback(api => {
    setRendererApi(ref);
  });

  useEffect(() => {
    if (!rendererApi) return;

    rendererApi.mapReadyPromise.then((map) => {
      createOverlay(rendererApi.rendererType, map);
    });
  }, [rendererApi]);

  // omitted: handling of all other children
  return <>{React.cloneElement(mapRenderer, {ref: rendererRef})}</>;
};

So what would happen here?

  • DeckGL component renders,
    • injects the ref for the map renderer
    • doesn't yet create the Deck instance, waits for map renderer to initialize
  • Map component renders
    • assigns the renderer-api to the imperativeHandle
  • after mounting the map, the ref-callback in DeckGL is called, making the
    "renderer api" available and triggering the effect to create the overlay

A couple of things to note:

  • the map instance might not be ready to create an overlay right away
    (I think in mapbox a layer can only be added after the style was loaded
    and with Google Maps we have to wait for the api to load),
    so we'd need the mapReadyPromise to signal the map being ready to accept
    an overlay.
  • The typical use-case for <MapRenderer ref={mapRef} /> would be to access
    the map instance in the application (although for both react-map-gl and
    react-google-maps the same can be achieved with the useMap hook).
    If the ref is also used externally, we'd have to somehow combine the refs.
  • With an implementation like this, the DeckGL component has to know about
    all the possible map-implementations. This is probably fine as there are
    only 3 supported renderers at the moment, but it would probably be better
    to use a common interface and a registry for all overlay types (e.g. the @deck.gl/google-maps registers the GoogleMapsOverlay in a global registtry where the DeckGL component will look for overlay implementations).

What do you think of this approach? I could go ahead and write a very minimalistic implementation of such a DeckGL component to see how those parts would work together.

@ibgreen
Copy link

ibgreen commented Oct 2, 2023

I like the big ideas here.

Can we make it so that deck.gl core doesn't need to know about the different renderer overlays (its just the applications that needs to know them, import them from their modules etc)? Maybe adding any required import statements to your code snippet could help shed more light on that?

As a total nit, I prefer shorter names:

interface MapRenderer<T = any> {
  type: string;
  mapLoaded: Promise<T>;
  map?: T; // google.maps.Map | mapbox.Map | maplibre.Map | ... 
}

@usefulthink
Copy link
Collaborator

usefulthink commented Oct 2, 2023

I was thinking about having some sort of global registry (effectively a Map<string, OverlayTypeConstructor>) where @deck.gl/mapbox and @deck.gl/google-maps could register their respective XxxOverlay constructors, like this

// in @deck.gl/mapbox
import {registerOverlayType} from '@deck.gl/core';

registerOverlayType('mapbox', MapboxOverlay);
// in @deck.gl/googlemaps
import {registerOverlayType} from '@deck.gl/core';

registerOverlayType('google-maps', GoogleMapsOverlay);

This is assuming a common interface for all overlays, but since they are very close already this shouldn't be a huge problem (it looks like map.addControl(overlay); (mapbox) vs. overlay.setMap(map) (google) is the only real difference).

The DeckGL component would then only need the type from the MapRenderer to retrieve the compatible overlay implementation.

Bonus benefit: If at some point someone wants to write react components for e.g. openlayers they could implement all the deck.gl integration without needing any changes to deck.gl itself.

@ibgreen
Copy link

ibgreen commented Oct 2, 2023

I was thinking about having some sort of global registry

We have had a few examples of global registries in the vis.gl frameworks (for instance loaders.gl allows for global loader registration), but usually (like all things global) these end up eventually being more pain than they are worth, and kind of promote debatable coding practices.

The above is not a veto but rather just a soft vote against it.

Would asking the application to pass in an overlays: [MapboxOverlay, GoogleMapsOverlay] prop to deck.gl work?

@usefulthink
Copy link
Collaborator

Would asking the application to pass in an overlays: [MapboxOverlay, GoogleMapsOverlay] prop to deck.gl work?

Sure, that would work as well.

@padawannn
Copy link
Author

With this approach, Deck would be responsible for creating the Overly (MapboxOverlay or GoogleMapOverlay) which is a great abstraction but the camera control would be still in the MapRenderer, right? I mean:

<DeckGL>
    < MapRenderer viewState={viewState} />
  </DeckGL>

It could be a bit confusing for the user why sometimes you send the viewState in the MapRenderer and other times you can send it in the DeckGL component

<DeckGL viewState={viewState}>
    < MapRenderer />
  </DeckGL>

I'm asking this to make sure I understand the approach correctly.

@usefulthink
Copy link
Collaborator

@padawannn I'll keep that in mind, thanks! Which component is actually controlling the rendering should just be an implementation detail, and isn't relevant to the outside. When rendering via Overlay, it can still be the DeckGL component that receives the view-props. In that case the values would only be forwarded to the map-renderer since the Deck instance will receive the values to use via the overlay.

@felixpalmer
Copy link

Thanks for taking the time to think about this @usefulthink and the detailed writeup!

What do you think of this approach? I could go ahead and write a very minimalistic implementation of such a DeckGL component to see how those parts would work together.

I would be very interested in seeing such an implementation 👍

@Pessimistress
Copy link

Pessimistress commented Oct 5, 2023

I just want to point out that:

  • @deck.gl/react does not depend on @deck.gl/mapbox or @deck.gl/google-maps
  • react-map-gl and react-google-maps do not depend on any deck.gl modules
  • assuming we want to keep things this way (and I think we should), we will have to ask the user to import the corresponding overlay class themselves and pass that into either DeckGL or the Map component. I'm afraid it will bot be as clean as you imagined.

Having the parent component render after the child is an anti-pattern in React and will lead to all sorts of unexpected problems. What if there are multiple map components in the children? What if the child map is later removed, shouldn't it behave like using DeckGL standalone? The component is already complicated as is, do you really want to sacrifice stability to save a few lines of client code?

@felixpalmer
Copy link

Not to downplay the issues you mentioned @Pessimistress but there is value beyond saving lines of code. By having deck control the camera and user input we make it possible to have a consistent experience across the basemaps and enable other libraries that work with deck, like nebula.gl integrating with the interleaved basemaps.

Ideally we would support adding/removing the child map and transitioning to DeckGL standalone, in effect this permits swapping between basemaps. We could use types to limit the number of children to one, and at runtime simply ignore additional children.

@Pessimistress
Copy link

Are we still talking about the same problem? Non of the existing interleaved options use Deck's camera control. This is not really up to us.

@chrisgervang
Copy link

chrisgervang commented Nov 20, 2023

I think a consistent experience is a great goal to work towards, however I don't think it's currently possible to have total consistency (but we can get close!).

In my experience interleave implementations are only as good as the least-featured hook for any given integration. Deck tends to have the more feature-ful hook compared to basemap libraries (historically this included webgl 2, many view types, multi-view, devicePixelRatio override, to name a few) for reasons ranging from performance to preference. We could encourage basemaps to make public apis and support more features, and until then Deck needs to be conform a bit to interleave properly.

Deck v8 straightened out an ownership model with maplibre/mapbox so that they always control deck through the IControl interface. We removed bug-prone usage of private apis where control was sometimes the other way around at the cost of some small features. It's been a lot more reliable and functional since that change.

I would love to see a similar pattern in place for google maps too, and like a lot of the ideas in here. My main piece of feedback would be to use a basemap -> deck control flow, make consistency a non-goal in the initial implementation, and encourage the basemaps to prioritize a consistent and featureful api for deck to integrate with in the long term.

Publishing a deck interface/spec for integration, something like MapRenderer, sounds like a good step towards this.

As a sidenote, hubble.gl and nebula.gl have the same challenge in wanting to configure both the deck and basemap integration. I don't think registerOverlayType is a '@deck.gl/_core_' module for the same dependency reasons @Pessimistress pointed out, however, I think something along the same lines in a non-core module could make sense. Whatever it is, I think of the integrator as independent best-effort bridge between a basemap and deck's core.

@ibgreen
Copy link

ibgreen commented Nov 21, 2023

Lots of deep thoughts here. Just wanted to mention that in the last open governance meeting we floated the idea of setting up an Open Visualization "basemaps working group" - the idea would be similar to the binary data, 3D and widgets working groups - to create more focused collaboration here, perhaps with some meetings where we could discuss in detail, hash out a common roadmap etc.

@usefulthink
Copy link
Collaborator

I would be very interested in joining that working-group once it is formed.

@ibgreen
Copy link

ibgreen commented Nov 22, 2023

The key enabler is to have someone step up and take the WG lead role, at least temporarily. Currently we have

If you are interested I could spend some time to help you set up the roadmap and the working group slack channel etc.

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

No branches or pull requests

6 participants