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 when the final zoom value is not the requested one (#7222) #7223

Merged
merged 2 commits into from
Sep 4, 2018
Merged

Fix flyTo when the final zoom value is not the requested one (#7222) #7223

merged 2 commits into from
Sep 4, 2018

Conversation

benoitbzl
Copy link
Contributor

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

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

@mourner mourner left a comment

Choose a reason for hiding this comment

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

Instead of adjusting the scale, could we do the k === 1 condition later in newCenter, setting it to center? That way we would make sure the center is exactly the one as passed at the end of the animation.

Also, let's add a unit test that fails without the fix and passes with it to demonstrate that the change is necessary.

src/ui/camera.js Outdated
const scale = 1 / w(s);
tr.zoom = k === 1 ? zoom : startZoom + tr.scaleZoom(scale);
const scale = k === 1 ? tr.zoomScale(zoom - startZoom) : 1 / w(s);
tr.zoom = startZoom + tr.scaleZoom(scale);
Copy link
Member

Choose a reason for hiding this comment

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

Let's keep the old k === 1 condition here because zoomScale+scaleZoom may introduce loss of floating point precision.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point. I will update the PR with your recommendation.

@benoitbzl
Copy link
Contributor Author

I found example code in the tests to simulate the animation for flyTo, as I think the problem I saw requires the computation of a fly path over several points. I will give it a try.

@mourner mourner merged commit 3978b6f into mapbox:master Sep 4, 2018
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
…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
…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.

2 participants