-
Notifications
You must be signed in to change notification settings - Fork 3.5k
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
Be consistent about using Ellipsoid.WGS84 in CzmlDataSource #11190
Conversation
Thanks for the pull request @malaretv!
Reviewers, don't forget to make sure that:
|
We've seen this in this repo as well as others. The The failure in CI was due to a separate configuration change on our part, which has since been fixed. This should be resolved if you merge in |
Thanks for the PR @malaretv! While this does certainly work around the issue when setting Consider this comment from #3543:
Would you have any interest in adding |
Certainly happy to at least take a look later this week and see the scale of work involved in adding
FWIW, this pull request would still be required even with an |
Understood! We would appreciate the contribution if you have the bandwidth! This would go a long way to improve usability and function for all those using CesiumJS for non-Earth cases.
Good point. One solution would be to pass in the ellipsoid as a parameter to the workers. For example,
Agreed. We could build in pre-computed optimizations like this into the
Yep! |
Thanks again for your contribution @malaretv! No one has commented on this pull request in 90 days. Maintainers, can you review, merge or close to keep things tidy? I'm going to re-bump this in 90 days. If you'd like me to stop, just comment with |
@malaretv What's the latest on this? If there's no bandwidth for the larger fix right now, would you be able to merge in |
@ggetz I've merged in As for the larger fix, indeed it was more than I could handle. I got a bit stuck trying to figure out how to best handle the hardcoded wgs84 defaults in Cartesian3. |
Thanks @malaretv, understood, no worries. I restarted your build to confirm the issues were transient. Looks good now. The last step here would be to update |
Thanks @malaretv! |
The current recommended path by maintainers to support non-wgs84 ellipsoids is to modify the Ellipsoid.WGS84 (see #11185 (comment) and #3543 (comment))
For CzmlDataSource this approach wasn't working because of the existence of Cartesian3 functions that default to other hardcoded WGS84 values if no ellipsoid is provided.
This change makes sure
Ellipsoid.WS84
is explicitly passed to those function calls in CzmlDataSource.