maplibre / maplibre-gl-js

MapLibre GL JS - Interactive vector tile maps in the browser
https://maplibre.org/maplibre-gl-js/docs/
Other
6.54k stars 697 forks source link

Ability to use Map as a stateless component #1545

Closed Pessimistress closed 1 year ago

Pessimistress commented 2 years ago

Motivation

Most modern web applications use a reactive/MVC UI framework, such as React, Angular and Vue. When building a complex application that contains a map component, it is very rare that the map library itself provides all the necessary functionalities. Some typical use cases include:

These scenarios all concern two-way state sharing between Map instances and other UI components. In the reactive programming paradigm, an exemplary implementation manages the camera state at the app's top level with a state store. Any UI component can request a change to that state. When the state does change, a rerender is triggered that flows down to the map and other components.

The design that maplibre-gl inherited from mapbox-gl does not make this easy. When the map receives user input, it immediately modifies the camera (transform) and rerenders. A move event is fired after the fact. Say the application updates its state based on a move event listener. When the other React components get rendered, they are always one animation frame behind the map.

I have maintained react-map-gl for over five years. There had been different attempts to address this issue, all concerning a) blocking the map from updating its own camera b) forcing the map to redraw when React rerenders. Prior to v6, the wrapper sets interactive: false and implements its own input handlers. In v7, the wrapper swaps out map.transform with a "shadow transform" that matches the React props just before repaint. Both approaches have painful shortcomings including performance and a creeping amount of hacks.

mapbox-gl issue regarding the stateful nature of input handling mapbox-gl issue regarding the state dependency of transform

Proposal

Whenever map.transform is about to be modified (any operation that leads to a move event), fire a premove event with the proposed changes: zoom, pitch, bearing, center. The event listeners are allowed to mutate the proposed camera parameters. map.transform is then updated to the "approved" values. In pseudo code:

/// Current
const tr = map.transform;
tr.bearing = newBearing;
tr.pitch = newPitch;
tr.zoom = newZoom;
tr.setLocationAtPoint(around, aroundPoint);

fireMoveEvents();
/// Proposed
const tr = map.transform;
const tr2 = tr.clone(); // this will resolve any incoming change and apply constraints

tr2.bearing = newBearing;
tr2.pitch = newPitch;
tr2.zoom = newZoom;
tr2.setLocationAtPoint(around, aroundPoint);

const proposedChanges = {
  center: tr2.center,
  bearing: tr2.bearing,
  pitch: tr2.pitch,
  zoom: tr2.zoom,
};
map.fire(new Event('premove'), {proposedChanges});

tr.bearing = proposedChanges.bearing;
tr.pitch = proposedChanges.pitch;
tr.zoom = proposedChanges.zoom;
tr.center = proposedChanges.center;

fireMoveEvents();

The current usage pattern and code path will stay the same if the event is not handled.

In an application (or wrapper library) where the Map instance is used as a stateless component, premove is used to block the camera from updating immediately. Rather, the transform will be modified when the state change propogates.

Some of the handler classes will need to be updated. They currently produce panDelta, pitchDelta, zoomDelta etc. for each input event and assume that map.transform holds the accumulated change. Once the transform state and user intention are decoupled, the handlers will need to track the accumulated changes themselves, so that interaction is still responsive and not dependent on when rerender happens.

Here is a very rough proof-of-concept, only enough changes for pointer inputs to work correctly: https://github.com/maplibre/maplibre-gl-js/compare/x/premove-poc

I'm open to alternative ideas.

Concerns

HarelM commented 2 years ago

Thanks for opening this discussion! I haven't looked deeply into the mapbox issues yet, but I was wondering why this should be an event and not a "registered" method:

map.registerPreMove(() => do something here...)

and then use it inside the code above, a bit similar to what we did in addProtocol.

Would that improve some of the concerns?

Not sure about the deltas, I'm not familiar with those unfortunately...

wipfli commented 2 years ago

I think if the usability of MapLibre GL JS in reactive frameworks would get much better, we could maybe justify a breaking change in the API...

birkskyum commented 2 years ago

Thank you for raising this issue

birkskyum commented 2 years ago

We have many handlers - I marked the ones currently adapted in the x/premove branch you mention, to make sure we don't miss something.

HarelM commented 2 years ago

I honestly don't like the fact that you fire an event to get a value for something. It's OK if you fire an event and listen to other event in order to be notified about something, but reading code that looks like fire('myEvent', (data) => use data) is prone to errors and not a good pattern, IMHO. If there's a need to introduce a message-bus in order to decouple events from who uses them it might be an option (although feels like an overkill for this small library).

birkskyum commented 2 years ago

Okay, so the premove should be made a middleware where we inject a function instead of fire an event.

HarelM commented 2 years ago

yup, this is my suggestion above :-)

Pessimistress commented 2 years ago

@birkskyum Sorry about the mixed up terms. When I said "breaking this library's API convention to have event handlers pass data back" I was referring to the premove event listener. I share the concern with everyone on this thread about that API.

Another option is to follow the convention of transformRequest and have something like transformCameraUpdate. Given the target use case it does not have to be dynamically modified once the Map is constructed.

HarelM commented 2 years ago

transformCameraUpdate sounds like a good direction :-)

birkskyum commented 2 years ago

We already have the synchronous redraw() that will let react drive the render cycle. https://github.com/maplibre/maplibre-gl-js/pull/300

https://github.com/maplibre/maplibre-gl-js/blob/b7beec3adec98fb0605468f9a769bf25490710fe/src/ui/map.ts#L2738-L2754

That is a good start. Maybe we should look into a mode where the asynchronous life cycle MapLibre has is completely disabled or replaced with the redraw() so we don't have to request and then stop the frames all the time.

birkskyum commented 2 years ago

This seems to be similar to Deck.onBeforeRender.

birkskyum commented 1 year ago

@Pessimistress , did you have a chance to look at this? I remember there was some talk in January about adding this to v3, which is getting close now, so it's a bit of a last call.

Pessimistress commented 1 year ago

I'll make another pass on the proposal this week.

birkskyum commented 1 year ago

Closed by #2535