visgl / react-google-maps

React components and hooks for the Google Maps JavaScript API
https://visgl.github.io/react-google-maps/
MIT License
1.17k stars 88 forks source link

Interleaved support with DeckGL #8

Open padawannn opened 11 months ago

padawannn commented 11 months ago

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 commented 11 months ago

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 commented 11 months ago

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 commented 11 months ago

@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 commented 11 months ago

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 commented 11 months ago

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 commented 11 months ago

@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 commented 11 months ago

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?

A couple of things to note:

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 commented 11 months ago

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 commented 11 months ago

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 commented 11 months ago

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 commented 11 months ago

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

Sure, that would work as well.

padawannn commented 11 months ago

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 commented 11 months ago

@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 commented 11 months ago

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 commented 11 months ago

I just want to point out that:

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 commented 11 months ago

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 commented 11 months ago

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 commented 9 months ago

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 commented 9 months ago

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 commented 9 months ago

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

ibgreen commented 9 months ago

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.