-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
Force map rerender synchronously after props update #720
Conversation
this._map._render(); | ||
if (map.isStyleLoaded()) { | ||
map._render(); | ||
// cancel the scheduled update |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why cancel the scheduled update can force redraw?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
_render
contains the redraw logic. We only cancel the scheduled update because all changes have been flushed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe suggest to mapbox that they provide a public API for this? And add a link to that github issue in a comment here?
Also, do we gain much by canceling the next redraw? FPS when resizing/transition?
If not, seems the less assumptions we make (about how mapbox manages dirty state and schedules draws etc), the better, and allowing an occasional extra draw might not be too bad?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do we gain much by canceling the next redraw?
As far as I can tell mapbox's _render
method does not check the dirty state. Instead, they schedule a redraw when the dirty state changes.
Since we call this every time map prop updates, it can be fairly significant during transition and interaction.
this._map._render(); | ||
if (map.isStyleLoaded()) { | ||
map._render(); | ||
// cancel the scheduled update |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe suggest to mapbox that they provide a public API for this? And add a link to that github issue in a comment here?
Also, do we gain much by canceling the next redraw? FPS when resizing/transition?
If not, seems the less assumptions we make (about how mapbox manages dirty state and schedules draws etc), the better, and allowing an occasional extra draw might not be too bad?
The synchronization issue is illustrated below, where the "SF" text is rendered using the React Marker component: (exaggerated by throttling the CPU)
The problem: when the viewport updates, we call
Map.jumpTo()
insidecomponentDidUpdate()
, which schedules a Mapbox rerender in the next animation frame. This causes the canvas rerender to always occur one step behind the React updates.This is one step to fixing visgl/deck.gl#2458. Currently, there are three independent asynchronous update cycles: React, Mapbox and Deck. When props are updated inside a React lifecycle, Mapbox and Deck should immediately redraw instead of deferring to their own animation loops.