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

Don't redo placement for zoom-only changes of an unpitched map #5248

Closed
jfirebaugh opened this issue Sep 6, 2017 · 2 comments · Fixed by #5284
Closed

Don't redo placement for zoom-only changes of an unpitched map #5248

jfirebaugh opened this issue Sep 6, 2017 · 2 comments · Fixed by #5284
Assignees
@ChrisLoer
Labels
performance ⚡ Speed, stability, CPU usage, memory usage, or power usage

Comments

@jfirebaugh
Copy link
Contributor

jfirebaugh commented Sep 6, 2017 •
edited
Loading

While profiling #5208 (https://jsfiddle.net/w2frv0ns/), I noticed frequent calls to redoPlacement happening on the worker during the zoom animation.

image

Redoing placement was not previously necessary for zoom-only changes, and according to @ChrisLoer, still shouldn't be necessary if the map is not pitched. Therefore this is a performance regression we should fix.
@jfirebaugh jfirebaugh added the performance ⚡ Speed, stability, CPU usage, memory usage, or power usage label Sep 6, 2017
@jfirebaugh
Copy link
Contributor Author

@ChrisLoer Can you take a look? I think this may be a significant performance regression.

@ChrisLoer
Copy link
Contributor

@jfirebaugh Yeah, I can put together a quick PR that just skips the zoom-triggered placement in when pitch is low-to-zero. The code will be superseded by #5150 when that lands.

ChrisLoer added a commit that referenced this issue Sep 13, 2017
@ChrisLoer
Fix issue #5248: don't redo placement for zoom changes of low-pitch m…
36caf14 
…aps.
@ChrisLoer ChrisLoer mentioned this issue Sep 13, 2017
Merged
ChrisLoer added a commit that referenced this issue Sep 14, 2017
@ChrisLoer
Fix issue #5248: don't redo placement for zoom changes of low-pitch m…
28a6418 
…aps.
mollymerp pushed a commit that referenced this issue Sep 18, 2017
@ChrisLoer
Fix issue #5248: don't redo placement for zoom changes of low-pitch m…
8a78d74 
…aps.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@ChrisLoer ChrisLoer

Labels
performance ⚡ Speed, stability, CPU usage, memory usage, or power usage
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Fix issue #5248: don't redo placement for zoom changes of low-pitch m… mapbox/mapbox-gl-js
2 participants
@jfirebaugh @ChrisLoer