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

Rotate back across the globe when points cross the IDL #4507

Merged
merged 3 commits into from
Oct 24, 2016

Conversation

lasalvavida
Copy link
Contributor

Fix for #3874.

@lasalvavida
Copy link
Contributor Author

Corrected output:

capture

@hpinkos
Copy link
Contributor

hpinkos commented Oct 21, 2016

I don't think that's quite right @lasalvavida. One of the blue rectangles should be rotated. The result should look like the yellow rectangles.

@hpinkos
Copy link
Contributor

hpinkos commented Oct 21, 2016

Can you add a unit test for this?

@lasalvavida
Copy link
Contributor Author

As discussed offline, this is the expected output:

var western2 = createWesternRectangle(0.005);

The blue rectangle is only rotated a small amount.

@lasalvavida
Copy link
Contributor Author

Adding a test now. Just to be clear, the crux of this issue is that the rectangle has its northwest corner on the opposite of the IDL from its center. So, when we do the following:

 nwCartesian = Cartesian3.subtract(nwCartesian, centerCartesian, nwCartesian);

Instead of getting the very small difference between them, we subtract a negative value, producing a large offset which blows up small rotations to the point where it causes a translation.

@@ -118,6 +118,10 @@ define([
granYSin = granularityY * sinRotation;
granXSin = granularityX * sinRotation;

if (center.longitude < nwCorner.longitude) {
center.longitude += 2 * Math.PI;
Copy link
Contributor

Choose a reason for hiding this comment

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

do CesiumMath.TWO_PI

@hpinkos
Copy link
Contributor

hpinkos commented Oct 21, 2016

Looks good to me @lasalvavida! I just had that one comment.

@lasalvavida
Copy link
Contributor Author

Test added!

@hpinkos
Copy link
Contributor

hpinkos commented Oct 24, 2016

Great, thanks @lasalvavida!

@hpinkos hpinkos merged commit 8ba579b into CesiumGS:master Oct 24, 2016
@pjcozzi
Copy link
Contributor

pjcozzi commented Oct 25, 2016

@hpinkos please update CHANGES.md in master.

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.

3 participants