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

Camera improvement when moving to a rectangle which crosses the antimeridian #7909

Merged
merged 4 commits into from
Sep 18, 2019

Conversation

michaelhayes-dev
Copy link
Contributor

  • Previously, if a user attempted to call camera.setView({ destination: rectangle }) or camera.flyTo({ destination: rectangle }) in the Cesium 2D View with a rectangle that crossed over the International Date Line, the camera would zoom out to show the entire map often making it difficult to see what the user intended the camera to zoom to.
  • This has been resolved, and in the Cesium 2D View, flyTo and setView will now work as intended.

Discussed in Issue #6936
Discussed in this Cesium forum post

@cesium-concierge
Copy link

Thanks for the pull request @michaelhayes-dev!

  • ❌ Missing CLA.
  • CHANGES.md was not updated.
    • If this change updates the public API in any way, please add a bullet point to CHANGES.md.
  • ❔ Unit tests were not updated.
    • Make sure you've updated tests to reflect your changes, added tests for any new code, and ran the code coverage tool.

Reviewers, don't forget to make sure that:

  • Cesium Viewer works.
  • Works in 2D/CV.
  • Works (or fails gracefully) in IE11.

@michaelhayes-dev
Copy link
Contributor Author

CLA has been sent in

@michaelhayes-dev
Copy link
Contributor Author

Re-ran the tests on my machine and I reproduced the same error Travis threw, but I also got that error on my unmodified master branch too. It appears to be an issue with some of the node_modules; I will investigate further this evening.

@pjcozzi
Copy link
Contributor

pjcozzi commented Jun 9, 2019

Thanks again for contributing, @michaelhayes-dev! We received your CLA.

@michaelhayes-dev
Copy link
Contributor Author

Not sure if this was lost amidst everything else, but this is ready to be merged!

@cesium-concierge
Copy link

Thanks again for your contribution @michaelhayes-dev!

No one has commented on this pull request in 30 days. Maintainers, can you review, merge or close to keep things tidy?

I'm going to re-bump this in 30 days. If you'd like me to stop, just comment with @cesium-concierge stop. If you want me to start again, just delete the comment.

@OmarShehata
Copy link
Contributor

Thanks for the contribution @michaelhayes-dev ! @hpinkos are you the best person to look at this? Seems like a straightforward PR for an issue multiple users have reported.

@cesium-concierge
Copy link

Thanks again for your contribution @michaelhayes-dev!

No one has commented on this pull request in 30 days. Maintainers, can you review, merge or close to keep things tidy?

I'm going to re-bump this in 30 days. If you'd like me to stop, just comment with @cesium-concierge stop. If you want me to start again, just delete the comment.

@hpinkos
Copy link
Contributor

hpinkos commented Sep 18, 2019

Thanks for the pull request @michaelhayes-dev! This fix is great. I'll merge this as soon as CI passes

@hpinkos hpinkos merged commit 84631de into CesiumGS:master Sep 18, 2019
@cesium-concierge
Copy link

Congratulations on closing the issue! I found these Cesium forum links in the comments above:

https://groups.google.com/forum/#!msg/cesium-dev/p5ygxIQA490/mbAYcjUlBgAJ

If this issue affects any of these threads, please post a comment like the following:

The issue at #7909 has just been closed and may resolve your issue. Look for the change in the next stable release of Cesium or get it now in the master branch on GitHub https://github.com/AnalyticalGraphicsInc/cesium.

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.

5 participants