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

Frame lag between (React) marker and GeoJSON update #940

Closed
wafisher opened this issue Nov 13, 2019 · 9 comments
Closed

Frame lag between (React) marker and GeoJSON update #940

wafisher opened this issue Nov 13, 2019 · 9 comments

Comments

@wafisher
Copy link
Contributor

(I'm still having issues related to what was supposed to be addressed in #720.)

The code we have will update the position of a train and then propagate that update both directly to GeoJSON and via a Popup:

<ReactMap
    points={this.state.points}
    onClick={this.onClick}
    onLoad={this.onMapLoad}
    viewport={this.state.viewport}
    updateViewport={this.updateViewport}
>
    <TrainPopup
        train={this.state.selectedTrain}
        close={() => this.setState({ selectedTrain: undefined })}
    />
</ReactMap>

The problem is that the React update happens immediately but the GeoJSON update may happen over a couple frames. I've found that large GeoJSON style changes, whether by the old method of setting the style object or the new method of Source/Layer will be processed on a worker thread by Mapbox. When it's done (perhaps a few frames later), it will send messages back to the main thread where the change is actually displayed. So the popup will move immediately, but the train icon, which is done in Mapbox via a Source doesn't show until later.

Point being here that even calling render on Mapbox synchronously may not cause an update in the same frame. The update is actually incredibly small so I'm not sure why Mapbox is taking so long to "think" but regardless, this would happen with a larger update so could still be an issue.

This architecture seems to be confirmed by https://github.com/mapbox/mapbox-gl-native/wiki/Threading-structure but some issues in mapbox-gl-js imply this work hasn't been done. Not sure how to solve this from here. As the user, I don't have any insight into when Mapbox will actually render the new updates so it's impossible to time it the same with the popup changes.

ezgif-5-9f7af3888070

@wafisher
Copy link
Contributor Author

mapbox/mapbox-gl-js#4904 confirms that setData is indeed done on another thread. If I check for the event sent back to the main thread, I can probably time this right. Would be nice for this to be built into react-map-gl though.

I'm going to try and write a workaround for now.

@RobJacobson
Copy link

RobJacobson commented Nov 21, 2019

I'm developing something similar, except that it's for ferry boats in the Seattle area with a boat icon and a popup marker. I use a separate Marker component for each. (Note that Marker and Popup both derive from the BaseControl parent component, so the performance should be comparable.)

Since I'm using the same component for both, they move in synchronization with one another. However, there seem to be performance issues with using markers, as discussed in issues #750 and #909, especially on mobile.

For the sake of asking, have you considered using GeoJSON for both the icon and the popup? They they should move in sync together and avoid the performance issues mentioned above.

@wafisher
Copy link
Contributor Author

I don't think we can use GeoJSON for the popup, unless I'm missing something? Not sure how you would do that.

@RobJacobson
Copy link

RobJacobson commented Nov 21, 2019

You're probably correct. I'm inexperienced with GeoJSON. It would require some hacking around at a minimum.

The alternative would be to use Marker for the train icon, so that you're essentially using BaseControl for positioning both elements (the icon and the popup) simultaneously. This is equivalent to how I'm currently doing it, which keeps the elements in sync. If your typical use-case is just showing a single marker and a single popup at any given time (or a few), this should avoid most of the performance issues. There might still be some lag when the user drags the map.

In your app, are you always showing the popup marker, or are you only showing it if the user clicks on the icon? I'm doing the former.

I'm looking for a more-performant solution using GeoJSON. I'll update if I find one.

@RobJacobson
Copy link

RobJacobson commented Nov 21, 2019

I looked at the example code for this mapbox-gl demo:
https://docs.mapbox.com/help/tutorials/building-a-store-locator

This demonstrates how to create a popup from a GeoJSON source using mapbox-gl. The relevant code is in the "stores.features.foreach(...)" function. This does the following:

  1. Iterates through each feature.
  2. Creates a mapboxgl.Marker instance for each feature.
  3. Conditionally renders a popup for any particular feature (in createPopUp).

For whatever reason, the mapboxgl.Marker instances do not seem to have the performance issues associated with the markers in react-map-gl. (I need to investigate that issue further.)

However, this demonstrates that it's definitely possible to render a popup from GeoJSON using mapbox-gl. We just need to translate this mapbox-gl code into equivalent react-map-gl code (or switch from react-map-gl to mapbox-gl instead).

@wafisher
Copy link
Contributor Author

Ah yes. I forget why I never went down that route. I haven't looked at the react-map-gl code recently but IIRC, it just renders a normal <div> at the pixel point representing the lat/long you specify (i.e. it queries Mapbox for that point on the canvas and puts the div there).

This is definitely an option except that react-map-gl doesn't "natively" support it. Also, how would you specify the contents of the popup? I suppose this could also be done by using the react-map-gl reference to the underlying Mapbox instance, but it seems hacky. Better that react-map-gl perf on popups were better sync'd with Mapbox updates.

@RobJacobson
Copy link

RobJacobson commented Nov 21, 2019

Agreed. I'm going to spend some time trying to convert markers and popups from react-map-gl to their mapbox-gl equivalents. It would be a hack, but I want to compare the performance. I'll send an update in a few days.

Your recollection is spot-on. In the marker code for react-map-gl, the code first converts lon/lat coordinates to pixels using the viewport.project function. It then just renders a plain old div (with some CSS styles) at that position in the render function. The popup is similar. That's why (performance wise) there isn't a tight coupling between the map and the markers.

In contrast, in mapbox-gl land, the examples involve creating elements from the mapboxgl.Marker class and then adding them with their lon/lat coodinates to the map. The map appears to be responsible for rendering them.

@RobJacobson
Copy link

RobJacobson commented Jan 3, 2020

@wafisher This is off-topic, but I opened another issue (#977) concerning mobile app development with react-map-gl. Would you mind sharing, in that thread, what framework/tech stack you're using for incorporating react-map-gl in your iOS app?

@Pessimistress
Copy link
Collaborator

This is being addressed in v7.0. Please follow #1646

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

3 participants