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

Fix flyTo rectangle in 2D #3695

Merged
merged 9 commits into from
Apr 5, 2016
Merged

Fix flyTo rectangle in 2D #3695

merged 9 commits into from
Apr 5, 2016

Conversation

novacto2
Copy link
Contributor

This is for #3688

Zoom limits check leads to a change of coordinates.

@novacto2 novacto2 mentioned this pull request Mar 10, 2016
Closed
@hpinkos
Copy link
Contributor

hpinkos commented Mar 10, 2016

Thanks @novacto2! Can you please send in a Contributor License Agreement so we can review this?

@pjcozzi
Copy link
Contributor

pjcozzi commented Mar 10, 2016

We now have a CLA from @novacto2. Thanks!

@hpinkos
Copy link
Contributor

hpinkos commented Mar 10, 2016

I'm not sure if deleting that code is the correct solution here.
@bagnell, can you look at this?

@novacto2
Copy link
Contributor Author

I've made some changes @hpinkos

It seems that the transformation of the camera position from cartesian to cartographic gives a negative height (in this specific case - rectangle in 2D). The computed height is stored in the 'z' component of cartesian, so the zoom limits should be tested on this number.

@hpinkos
Copy link
Contributor

hpinkos commented Mar 21, 2016

Thanks for the update!
I can confirm this fixes the issue. And all of the tests pass.
We'll probably want to add a unit test for this. @novacto2 would you mind doing that? See our testing guide for information

@bagnell, does this code look good to you?

bagnell
bagnell reviewed Mar 21, 2016
View reviewed changes
Source/Scene/Camera.js
if (destinationCartographic.height === sscc.minimumZoomDistance || destinationCartographic.height === sscc.maximumZoomDistance) {
destination = ellipsoid.cartographicToCartesian(destinationCartographic);
}
if (mode === SceneMode.SCENE2D && isRectangle) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This is true for Columbus view as well. I would change this to mode !== SceneMode.SCENE3D.

@bagnell
Copy link
Contributor

bagnell commented Mar 21, 2016

Looks good besides the one comment and formatting. @hpinkos Can you merge when you're ready?

@novacto2
Copy link
Contributor Author

@bagnell you're right, thanks:) I changed the condition and formatting.

@hpinkos unit tests were added

@hpinkos
Copy link
Contributor

hpinkos commented Mar 28, 2016

Alright, looks great @novacto2! Thanks!
Can you merge master? It looks like there's a conflict.
Also, add a line to CHANGES.md for this bug fix, then we'll get this merged in.

@novacto2
Copy link
Contributor Author

novacto2 commented Apr 4, 2016

Is it ok @hpinkos ?

@hpinkos
Copy link
Contributor

hpinkos commented Apr 4, 2016

Thanks @novacto2! Just a few tiny things:
Please add yourself to CONTRIBUTORS.md

And it looks like your formatting is still off. Please see the Coding Guide
If you are coding in Eclipse or WebStorm, highlighting your changes and running the autoformatter should fix the problem.

@novacto2
Copy link
Contributor Author

novacto2 commented Apr 4, 2016

I'm sorry about the mistakes. Hope it's ok now.

Thanks for going easy on me @hpinkos :)

@hpinkos
Copy link
Contributor

hpinkos commented Apr 5, 2016

Looks great! Thanks @novacto2!

@hpinkos hpinkos merged commit fb8193c into CesiumGS:master Apr 5, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@novacto2 @hpinkos @pjcozzi @bagnell