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

Synchronous redraw API #7893

Open
Pessimistress opened this issue Feb 11, 2019 · 19 comments
Open

Synchronous redraw API #7893

Pessimistress opened this issue Feb 11, 2019 · 19 comments

Comments

@Pessimistress
Copy link

Pessimistress commented Feb 11, 2019

Motivation

In react-map-gl, when the viewport updates, we call Map.jumpTo() inside componentDidUpdate(), which schedules a Mapbox rerender in the next animation frame. This causes the canvas rerender to always occur one step behind the React updates.

You can see this issue in visgl/react-map-gl#720 and visgl/deck.gl#2458.

Design

Provide a synchronous API e.g. redraw() that immediately flushes the dirty state.

Implementation

Here's how we are currently forcing rerender:

// map render will throw error if style is not loaded
if (map.style) {
  // cancel the scheduled update
  if (map._frame) {
    map._frame.cancel();
    map._frame = null;
  }
  map._render();
}

Private methods and properties are manipulated here, which is not a reliable solution.

@peterqliu
Copy link
Contributor

does triggerRepaint() address this need?

https://docs.mapbox.com/mapbox-gl-js/api/#map#triggerrepaint

@Pessimistress
Copy link
Author

triggerRepaint is asynchronous: https://github.com/mapbox/mapbox-gl-js/blob/master/src/ui/map.js#L1766

@mourner
Copy link
Member

mourner commented Feb 20, 2019

@Pessimistress would exposing the map._render method publicly fit the use case? Edit: we could then also move canceling the frame logic to it (if there's no ongoing animation).

@Pessimistress
Copy link
Author

We need to both call map._render and clear the scheduled rerender (map._frame) - see code snippet in the issue summary.

@mourner
Copy link
Member

mourner commented Feb 20, 2019

@Pessimistress yeah, I updated the comment — should we add the logic of clearing the frame if there's no ongoing animation to (potentially renamed) _render (instead of making another wrapper)?

@Pessimistress
Copy link
Author

Yeah that makes sense. Though I figure the naming of render and triggerRepaint might be confusing to a lot of users.

@mourner
Copy link
Member

mourner commented Feb 20, 2019

@Pessimistress yeah, maybe something like forceRepaint?

@ansis
Copy link
Contributor

ansis commented Feb 26, 2019

@Pessimistress would this be used for instantly jumping to a single new location or also animating a movement to a new location?

Also, what would be deciding when frames are rendered? Would there be an animation loop outside of mapbox-gl that uses requestAnimationFrame?

@Pessimistress
Copy link
Author

@ansis timing will be managed by React's component life cycle, on componentDidUpdate

@ansis
Copy link
Contributor

ansis commented Feb 26, 2019

I'm not that familiar with React so please correct me if I'm getting something wrong. React's component life cycle appears to not to use requestAnimationFrame. Using animation frames lets the map rendering be synced with browser repainting so that the map is rendered only once for each browser repaint which avoids doing extra work and dropping frames.

Would it be an option to have all the components that need to be synced with the map live in a separate react tree and have updates to those components be set after the map renders a frame?

I think just adding a synchronous render method wouldn't fix the underlying problem because you could still end up with multiple map renders for each browser repaint.

@Pessimistress
Copy link
Author

Pessimistress commented Feb 26, 2019

The root issue is that both React and Mapbox's render cycles are asynchronous. If we update component states on a render event, React would not immediately repaint either.

On a high level, when we are developing a React application, React is the source of truth for all app states, and the React clock should be driving all rerenders caused by app state change. In the context of mapbox, when the canvas is resized and/or the camera position is changed, the React root tells all child components when to update, not the other way around.

In your proposal, imagine if more than one sub-component require to be informed of an update before everyone else. They can kick off an infinite update loop that never ends.

With that said - It is only a description of why this API would solve our problem. I can understand your concern that such an API may get abused by the user and affect performance.

@ansis
Copy link
Contributor

ansis commented Feb 27, 2019

Thanks for the explanation!

I'm wondering if a Portal could be used to solve the marker syncing problem. For each react marker you could create a mapboxgl.Marker and render within that. This way the map would be responsible for positioning the elements while React would be responsible for rendering them.

render() {
  return ReactDOM.createPortal(this.props.children, this.mapboxmarker.getElement());
}

This example uses mapboxgl markers but something similar could be implemented for react so that it works without mapbox maps.

The positioning of containers would happen synchronously with respect to the map. The rendering of the markers within the containers would happen synchronously with respect to react.

This would allow the webgl rendering to be synced with browser repaints instead of react renders and could avoid unnecessary rendering.


Side note: I looked into the spec and it looks like animation frames should always be run just before a repaint. This should mean that triggering a repaint would update the map before the dom element changes are visible. But I'm guessing browsers don't follow that exactly and maybe break that sometimes :( https://html.spec.whatwg.org/multipage/webappapis.html#event-loop-processing-model

@Pessimistress
Copy link
Author

I'm wondering if a Portal could be used to solve the marker syncing problem.

Yes, portals might fix the rendering, though other issues would surface (e.g. React's event handling do not mix well with native event listeners).

I've been working on the React wrapper for over two years now. Many of mapbox-gl's APIs are incompatible with the pure reactive pattern that we are after, as illustrated in this old issue. Being able to manage map states and control render cycles outside of the Map component is essential to a lot of our applications. Another example would be rendering two maps side by side and synchronize their cameras, which you cannot do with mapbox's native API.

We have also built a lot more components than just marker, especially DeckGL. Admittedly some of the implementation decisions are legacy and could be revisited, but some are with good reason. I suspect either way there needs to be a lot of patching and hacking.

@ansis
Copy link
Contributor

ansis commented Mar 4, 2019

@Pessimistress thanks for all the feedback and explanations! They were very helpful for me. We'll add a synchronous render method.

ansis added a commit that referenced this issue Apr 8, 2019
This option allows the user to implement `requestAnimationFrame` and
`cancelAnimationFrame` in order to synchronize map rendering with their
external rendering.

fix #7893
@ansis
Copy link
Contributor

ansis commented Apr 8, 2019

@Pessimistress I'm finally getting around to implementing this.

An idea that popped up internally was to expose this functionality by letting users implement their own AnimationFrameProvider that we would request frames from. The pr has some examples, including one where you can synchronously dispatch frames: #8131

The main advantages of this approach seemed to be that:

  • It prevents frames from being rendered outside of your render loop. With something like map.synchronousRender() you could render within your loop, but the map might still occasionally render outside of it.
  • requestAnimationFrame() calls inform you that the map needs to be rerendered. There is no way to tell with existing events.
  • it might be harder for others to misuse

It is more complicated that a map.synchronousRender() though. Does the AnimationFrameProvider approach look like it would work for you? Would you still prefer a map.synchronousRender()?

Thanks!

@Pessimistress
Copy link
Author

Pessimistress commented Apr 11, 2019

I prefer map.synchronousRender(). External animation frame provider is a neat feature and I get why it's desirable, but it doesn't affect our particular use case.

I observed something weird with the above hack. Although map._render() is executed, the canvas does not update immediately. i.e. when render event is fired, the visuals do not yet reflect the new camera position. This seems more pronounced when tiles that are not cached are brought into view. Is this behavior expected? Will I be able to work around it with the proposed synchronousRender API?

jahow added a commit to jahow/openlayers that referenced this issue May 13, 2019
This uses an undocumented method to trigger a synchronous redraw
instead of using the standard schedule from Mapbox. The CSS previously used
to make the OL and MB views match is not necessary anymore.

Reference: mapbox/mapbox-gl-js#7893 (comment)
jahow added a commit to jahow/openlayers that referenced this issue May 13, 2019
This uses an undocumented method to trigger a synchronous redraw
instead of using the standard schedule from Mapbox. The CSS previously used
to make the OL and MB views match is not necessary anymore.

Reference: mapbox/mapbox-gl-js#7893 (comment)
@wesg52
Copy link

wesg52 commented Mar 22, 2020

I believe we also require a synchronous solution to implement this feature https://stackoverflow.com/questions/60803985/how-to-synchronously-update-mapbox-gl-style

The proposed implementation above did not work.

@charleyw
Copy link

charleyw commented Mar 23, 2020

Hi there,
I met a problem with jumpTo and _render() trick. We have some style like this:

"icon-image": { "stops": [[4, "Capital"], [9, ""]] },
"text-font": [
                  "step",
                    ["zoom"],
                    ["literal", ["noto sans"]],
                    9,
                    ["literal", ["noto sans bold"]]
                ],
"text-field": "{name}"

Which make the map show both icon and text before zoom 9 for a specific poi. And start from zoom9 hide icon and show bold text.

When current zoom is 8, we use jumpTo and _render to zoom 9, all these works., icon will disappear and text become bold.

But when current zoom is 9, and use jumpTo and _render change to zoom 8, the map will not update, icon not appear and font weight still keep bold. And it need another trigger (like move or even call jumpTo with same properties again), map will change, icon will disappear and font weight become normal.

@RyanMcManaman
Copy link

@Pessimistress I'm finally getting around to implementing this.

Did this ever get implemented? Couldn't find any documentation for it, looks like others are finding this webpage in search of it

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

Successfully merging a pull request may close this issue.

7 participants