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

KML error with non-earth ellipsoid #6176

Closed
tamarmot opened this issue Feb 1, 2018 · 14 comments
Closed

KML error with non-earth ellipsoid #6176

tamarmot opened this issue Feb 1, 2018 · 14 comments

Comments

@tamarmot
Copy link
Contributor

tamarmot commented Feb 1, 2018

I am loading some KML to render on my moon ellipsoid. When I look at the coordinate conversion in KmlDataSource, I see that it is not using any ellipsoid to calculate the coordinates.

For example, with:
longitude: 24.070617695061806
latitude:87.90173269295278
I get coordinates:
Cartesian3 {x: 213935.5635247161, y: 95566.36983235707, z: 6352461.425213023}
instead of lunar coordinates:
Cartesian3 {x: 58080.7702560248, y: 25945.04756005268, z: 1736235.0758562544}

I suggest the constructor for a KmlDataSource takes an option for an ellipsoid, which is then used for every single Cartesian3.fromDegrees call.

@hpinkos
Copy link
Contributor

hpinkos commented Feb 1, 2018

Thanks @tamarmot! While we do support non-earth ellipsoids, it's one of the areas that is least tested because most of our users are using WGS84.
If you find any more issues, let us know!

As a workaround, setting Cesium.Ellipsoid.WGS84 = new Cesium.Ellipsoid(x, y, z) should fix most problems

@tamarmot
Copy link
Contributor Author

tamarmot commented Feb 1, 2018 via email

@hpinkos hpinkos changed the title KmlDataSource coordinates converted using the WGS84 / earth ellipsoid KML error with non-earth ellipsoid Feb 1, 2018
@tamarmot
Copy link
Contributor Author

tamarmot commented Feb 1, 2018

For the record, making that change does not correct this issue ...

@tamarmot
Copy link
Contributor Author

tamarmot commented Feb 1, 2018

... because it is hardcoded in Cartesian3:
var wgs84RadiiSquared = new Cartesian3(6378137.0 * 6378137.0, 6378137.0 * 6378137.0, 6356752.3142451793 * 6356752.3142451793);

maybe not so flexible ...

@hpinkos
Copy link
Contributor

hpinkos commented Feb 1, 2018

Ah yeah. We had to do that because the Ellipsoid module requires Cartesian3, so we can't also have the Cartesian module require Ellipsoid because of the circular dependency.

If you have time, we'd be happy to review a PR with the KML fix!

@mramato
Copy link
Contributor

mramato commented Feb 1, 2018

Just wanted to stop in and say hi, @tamarmot!

But now that I'm here, that's definitely a bug (KmlDataSource should not be using Cartesian3.fromDegrees, or any of our internal code for that matter). We have those hardcoded values purely because it lets us provide that helper function for end users, but internally it's a no go.

@tamarmot
Copy link
Contributor Author

tamarmot commented Feb 1, 2018 via email

@tamarmot
Copy link
Contributor Author

tamarmot commented Feb 1, 2018 via email

@ggetz
Copy link
Contributor

ggetz commented Feb 1, 2018

Hi @tamarmot, we just pushed an npm fix about half an hour ago - https://www.npmjs.com/package/cesium

I'm running the build right now with our recommended webpack configuration to see if that is working.

@tamarmot
Copy link
Contributor Author

tamarmot commented Feb 1, 2018 via email

@tamarmot
Copy link
Contributor Author

tamarmot commented Feb 1, 2018 via email

@hpinkos
Copy link
Contributor

hpinkos commented Feb 1, 2018

Looking forward to your PR @tamarmot!

You can find a bunch of contributor documentation here: https:/AnalyticalGraphicsInc/cesium/tree/master/Documentation/Contributors
Here is some info about opening a PR: https:/AnalyticalGraphicsInc/cesium/blob/master/CONTRIBUTING.md#opening-a-pull-request

To run the tests, run npm start to start a local server, and navigate to localhost:8080. On that page, there is a link to run the unit tests
You don't need to npm-ify your fork. The problem we had with our npm package was a special case.

@tamarmot
Copy link
Contributor Author

fixed by my pull request ...

@cesium-concierge
Copy link

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

https://groups.google.com/forum/#!topic/cesium-dev/e_osjMgHD4E

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

The issue at #6176 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:/AnalyticalGraphicsInc/cesium.


I am a bot who helps you make Cesium awesome! Contributions to my configuration are welcome.

🌍 🌎 🌏

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants