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

Polyline entity updates with correct ellipsoid #3174

Merged
merged 2 commits into from
Nov 9, 2015

Conversation

hpinkos
Copy link
Contributor

@hpinkos hpinkos commented Nov 6, 2015

Reported on the forum: https://groups.google.com/forum/?hl=en#!topic/cesium-dev/Zkt3n4_4lI8

The ellipsoid wasn't being sent to PolylinePipeline.generateCartesianArc in the polyline's DynamicGeometryUpdater so it was using WGS84 instead.

@mramato
Copy link
Contributor

mramato commented Nov 9, 2015

Thanks @hpinkos, I updated CHANGES since this was a user-reported issue.

mramato added a commit that referenced this pull request Nov 9, 2015
…ipsoid

Polyline entity updates with correct ellipsoid
@mramato mramato merged commit 6e6fecb into master Nov 9, 2015
@mramato mramato deleted the polylineEntityEllipsoid branch November 9, 2015 14:42
positions : undefined,
granularity : undefined,
height : undefined
generateCartesianArcOptions.ellipsoid = geometryUpdater._scene.globe.ellipsoid;
Copy link
Contributor

Choose a reason for hiding this comment

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

scene.globe is not guaranteed to exist, generally we prefer scene.mapProjection.ellipsoid for this kind of thing.

Copy link
Contributor

Choose a reason for hiding this comment

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

@emackey, can you submit a separate issue for this. A quick search for scene.globe. shows that there are quite a few places we are access a property on globe without checking. For the specific case of scene.globe.ellipsoid, we should probably add an ES5 getter for scene.ellipsoid so that this mistake is harder to make.

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, thanks.

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