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

Expose event for camera change #3746

Open
Pessimistress opened this issue Dec 6, 2016 · 11 comments
Open

Expose event for camera change #3746

Pessimistress opened this issue Dec 6, 2016 · 11 comments

Comments

@Pessimistress
Copy link

Our libraries react-map-gl and deck.gl both use Mapbox GL as a controlled React component. All viewport states are synced with a data store. Currently there is no event that is:

  • reliably fired for every camera change as a result of user interaction or flyTo()
  • fired before update (a controlled component does not rerender until props change)

As a result, we have been implementing our own mouse/touch interaction in order to use Mapbox in a React/Redux paradigm. It would be nice if there is an event that we can subscribe to to dispatch requests for camera change.

@mourner
Copy link
Member

mourner commented Dec 7, 2016

We typically use the move event for things like this. Can you elaborate on why you need a similar event rendered before the map render?

@lucaswoj
Copy link
Contributor

lucaswoj commented Dec 7, 2016

@mourner I think I've heard mention of some cases in which the map moves but a move event isn't fired. These should be considered bugs and fixed. I believe this would eliminate the need for any new events.

@lucaswoj
Copy link
Contributor

lucaswoj commented Dec 7, 2016

Hopefully @Pessimistress can help us understand if my understanding is correct and what the situations are when we don't fire move.

@Pessimistress
Copy link
Author

@mourner @lucaswoj
My understanding is that move only gets fired when map center changes, not on zoom/rotate

More importantly: we use React/Redux to build applications on top of Mapbox. The Redux state container is the single source of truth for all app states, which we bind the Mapbox camera to. Otherwise we risk the map going out of sync with the overlays and other UI components. It means that the order of events is like:

  • camera change requested
  • state container updated
  • map viewport updated
  • move event fired

Basically, I'm wishing for the ability to use a state manager that is external to Mapbox. This is critical to our tech stack as it allows us to manage the increasing complexity of our mapping applications.

@lucaswoj
Copy link
Contributor

lucaswoj commented Dec 7, 2016

The move event is fired for any camera mutation, not just when the map center changes.

There are a number of camera improvements coming in #2801 that will help your use case.

@vicapow
Copy link
Contributor

vicapow commented Jan 18, 2017

I think the key problem here is with syncing the render of two seperate components and most noticeable when the frame rate is low. If we just listen for "move" I think by that time, Mapbox has already done a render. I'll be digging into this more but if anyone else has any ideas, let me know!

@Pessimistress
Copy link
Author

I think the key here is react-map-gl was designed to be a Controlled Component, where React state holds the single source of truth. If we start subscribing to the move event after the change happens, then we are switching to an Uncontrolled Component, where Mapbox is the single source of truth.

The latter will likely break many of our use cases since you can no longer (reliably) control the viewport by updating the Redux store.

@vicapow
Copy link
Contributor

vicapow commented Jan 18, 2017

That's true but I don't think this is unique to react-map-gl. I would expect it to be a problem for any third party library wanting to stay in sync with mapboxGL

@mcrawshaw
Copy link

mcrawshaw commented Apr 8, 2017

Maybe the best way forward would be suggest a couple of possible solutions @Pessimistress.

@lucaswoj there is no harm cleaning up the API, but I don't think it solves the issue presented by @Pessimistress. The reliability of move is a minor concern compared with the need to completely re-implement the user interaction logic when CameraOptions are externally controlled.

Before I go on, just to note that the 2 libraries in question have almost 5,000 stars combined. Sorry, not trying to be a pain, but just saying they represent a large user base. At the moment all user interaction for these libraries is left completely up to them (out of Mapbox's control).

Solution 1
Add a new event called beforeUserMove or even shouldUserMove that receives the translated CameraOptions object and accepts a return indicating if the library should execute the user input (i.e. update the matrix transforms). This would solve @Pessimistress's libraries major architectural issues and also provide more advanced maxBounds restrictions for other users. If that isn't possible due to issues with your eventing system, maybe solution 2...

Solution 2
Add an additional property to MapOptions that indicates if user actions should be executed, maybe interactiveExecuted. In this case the events described above would still be sent, but no action would be taken regardless of return.

Obviously in both cases, userland would then take the CameraOptions object and (in most cases) call your new setCamera function to update the camera.

Separating the command for camera movement from execution may require some dev/testing but it seems now is the time do it with all the other work going on in the camera class...

@anandthakker
Copy link
Contributor

Thanks for articulating this pain point / use case. I definitely like the idea of decoupling the handling of user input from the consequent changes to the camera state. In terms of internal architecture, I think this is already very likely going to be a by-product of the refactor that @lucaswoj mentioned (#2801).

Once that work starts getting closer to complete, we'll be in a better position to design in detail the ideal API by which client code can subscribe to user actions that (attempt to) change the camera state.

@anandthakker
Copy link
Contributor

anandthakker commented May 18, 2018

If we just listen for "move" I think by that time, Mapbox has already done a render.

Noting here that this syncing issue should now be resolved via #5743 and #6005 (and some similar follow-ups).

The broader issue of decoupling interaction handling from state updates remains.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

7 participants