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.

Sign up for GitHub

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

Jump to bottom

Force map rerender synchronously after props update #720

Merged
merged 2 commits into from
Feb 11, 2019
Merged

Force map rerender synchronously after props update #720

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
fcd367e
force map rerender after update
Feb 5, 2019
cf7332c
add comments
Feb 5, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 23 additions & 5 deletions src/mapbox/mapbox.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -211,11 +211,23 @@ export default class Mapbox {
// (e.g. until "componentDidUpdate")
resize() {
this._map.resize();
return this;
}

// Force redraw the map now. Typically resize() and jumpTo() is reflected in the next
// render cycle, which is managed by Mapbox's animation loop.
// This removes the synchronization issue caused by requestAnimationFrame.
redraw() {
const map = this._map;
// map render will throw error if style is not loaded
if (this._map.isStyleLoaded()) {
this._map._render();
if (map.isStyleLoaded()) {
map._render();
// cancel the scheduled update
Copy link

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?

Copy link
Collaborator Author

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.

Copy link
Contributor

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?

Copy link
Collaborator Author

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.

if (map._frame) {
map._frame.cancel();
map._frame = null;
}
}
return this;
}

// External apps can access map this way
Expand Down Expand Up @@ -360,8 +372,12 @@ export default class Mapbox {
newProps = Object.assign({}, this.props, newProps);
checkPropTypes(newProps, 'Mapbox');

this._updateMapViewport(oldProps, newProps);
this._updateMapSize(oldProps, newProps);
const viewportChanged = this._updateMapViewport(oldProps, newProps);
const sizeChanged = this._updateMapSize(oldProps, newProps);

if (viewportChanged || sizeChanged) {
this.redraw();
}

this.props = newProps;
}
Expand All @@ -374,6 +390,7 @@ export default class Mapbox {
this.height = newProps.height;
this.resize();
}
return sizeChanged;
}

_updateMapViewport(oldProps: any, newProps: Props) {
Expand All @@ -396,6 +413,7 @@ export default class Mapbox {
this._map.transform.altitude = newViewState.altitude;
}
}
return viewportChanged;
}

_getViewState(props: Props) : ViewState {
Expand Down