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 flyTo not zooming to exact given zoom #6828

Merged
merged 1 commit into from
Jun 19, 2018
Merged

Conversation

mourner
Copy link
Member

@mourner mourner commented Jun 18, 2018

Currently, if you call flyTo with an integer zoom, you're not guaranteed to land on the exact zoom after the animation because of floating point errors. E.g. map.flyTo({zoom: 2}) when starting on zoom 0.6 will end up on zoom 1.999999999996, which is problematic if you wish to get on the exact zoom for corresponding tiles to load. Discovered this while implementing the getClusterExpansionZoom API.

Launch Checklist

  • briefly describe the changes in this PR
  • write tests for all new functionality
  • (n/a) document any changes to public APIs
  • (n/a) post benchmark scores
  • manually test the debug page

closes #6191

@mourner mourner requested a review from mollymerp June 18, 2018 10:39
Copy link
Contributor

@mollymerp mollymerp left a comment

Choose a reason for hiding this comment

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

looks good to me, but I would maybe add a comment above the new conditional explaining why its there 👍

@andrewharvey
Copy link
Collaborator

@mourner I edited your comment to include closes #6191, since I think it needs to be in the top comment to auto close when merged.

@mourner
Copy link
Member Author

mourner commented Jun 19, 2018

@andrewharvey great, I didn't realize that was already reported.

@mollymerp I couldn't come up with a comment that would make the lines clearer...

@mourner mourner merged commit 8cf4624 into master Jun 19, 2018
@mourner mourner deleted the fix-fly-to-round-zoom branch June 19, 2018 05:56
@mollymerp
Copy link
Contributor

oh I was just thinking something like "avoid floating point errors by using zoom directly when (whatever k represents)k===1"

mourner pushed a commit that referenced this pull request Sep 4, 2018
…7223)

* Fix flyTo when final zoom is not requested one (#7222)

This complete the fix for #6828 when the final zoom value is
different from the zoom requested.

* Fix final center position and add test case.
pirxpilot pushed a commit to pirxpilot/mapbox-gl-js that referenced this pull request Jun 15, 2019
…7222) (mapbox#7223)

* Fix flyTo when final zoom is not requested one (mapbox#7222)

This complete the fix for mapbox#6828 when the final zoom value is
different from the zoom requested.

* Fix final center position and add test case.
pirxpilot pushed a commit to pirxpilot/mapbox-gl-js that referenced this pull request Jun 16, 2019
pirxpilot pushed a commit to pirxpilot/mapbox-gl-js that referenced this pull request Jun 16, 2019
…7222) (mapbox#7223)

* Fix flyTo when final zoom is not requested one (mapbox#7222)

This complete the fix for mapbox#6828 when the final zoom value is
different from the zoom requested.

* Fix final center position and add test case.
pirxpilot pushed a commit to pirxpilot/mapbox-gl-js that referenced this pull request Jun 18, 2019
pirxpilot pushed a commit to pirxpilot/mapbox-gl-js that referenced this pull request Jun 18, 2019
…7222) (mapbox#7223)

* Fix flyTo when final zoom is not requested one (mapbox#7222)

This complete the fix for mapbox#6828 when the final zoom value is
different from the zoom requested.

* Fix final center position and add test case.
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.

flyTo doesn't reach integral zoom needed for cluster expansion
3 participants