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

Fix issue #5248: don't redo placement for zoom changes of low-pitch m… #5284

Merged
merged 1 commit into from
Sep 14, 2017

Conversation

ChrisLoer
Copy link
Contributor

Fixes #5248.

aefb3ee introduced redoPlacement calls in response to any map move, since any movement could potentially affect collision detection with the new pitch-scaling behavior. To prevent incurring extra/unnecessary placement costs, I did a couple things:

  • Don't trigger placement at all if none of the transform parameters had changed
  • Don't trigger placement if the only thing that had changed was the relative position of the tile (i.e. a pan operation), and the pitch was low (at low pitch, pan operations have very small effects on collision detection).
  • Trigger placement at most once per 300 ms: there's no point in maxing out the CPU on the worker threads to deliver placement faster than collisions are actually perceived, and in fact going too quickly can exacerbate some "flicker" problems.

However, I forgot that the transform parameter cameraToCenterDistance would be changing with every zoom change, and that zoom changes didn't previously trigger placement. While zoom changes (and anything else that affects cameraToCenterDistance, like changing the field of view) do affect collision detection, the effects are minimal at low pitch and can be safely ignored.

When the cost of placement is relatively low, this is not a very substantial difference, because the throttling limits placement to once every 300ms anyway. When placement is significantly more expensive (as in #5208), the placement can end up running continuously during zoom operations, which is a waste (we should only have to re-do placement when we cross integer zoom levels).

This PR fixes the oversight and introduces one unit test to verify that redoPlacement is actually being suppressed.

This will hopefully all be superseded soon by #5150.

bench results

The benchmarks don't really exercise the use case this PR addresses, so this is just a sanity check.

benchmark master 7d452ea cloer_5248 6e2fbf4
map-load 125 ms 89 ms
style-load 127 ms 132 ms
buffer 1,147 ms 1,128 ms
tile_layout_dds 1,259 ms 1,258 ms
fps 60 fps 60 fps
frame-duration 6.2 ms, 0% > 16ms 6.4 ms, 1% > 16ms
query-point 0.84 ms 0.96 ms
query-box 72.64 ms 66.96 ms
geojson-setdata-small 9 ms 11 ms
geojson-setdata-large 130 ms 148 ms
filter n/a n/a

/cc @jfirebaugh @ansis

Copy link
Contributor

@jfirebaugh jfirebaugh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

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

Successfully merging this pull request may close these issues.

Don't redo placement for zoom-only changes of an unpitched map
2 participants