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

Fully support specifying a custom ellipsoid #890

Merged
merged 21 commits into from
Jun 20, 2024
Merged

Fully support specifying a custom ellipsoid #890

merged 21 commits into from
Jun 20, 2024

Conversation

azrogers
Copy link
Contributor

@azrogers azrogers commented May 24, 2024

cesium-native already allows specifying a custom ellipsoid with most of its operations. This change removes the few places where the WGS84 ellipsoid was hardcoded, allowing all of cesium-native to be used with other ellipsoids. This is necessary to support CesiumGS/cesium-unreal#366.

@csciguy8 csciguy8 self-requested a review May 29, 2024 16:01
Copy link
Member

@kring kring left a comment

Choose a reason for hiding this comment

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

Looks great @azrogers, just a few comments below.

Cesium3DTilesSelection/src/BoundingVolume.cpp Outdated Show resolved Hide resolved
Cesium3DTilesSelection/src/BoundingVolume.cpp Outdated Show resolved Hide resolved
Cesium3DTilesSelection/src/LayerJsonTerrainLoader.cpp Outdated Show resolved Hide resolved
Copy link
Contributor

@csciguy8 csciguy8 left a comment

Choose a reason for hiding this comment

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

Thanks for the changes @azrogers !

From what I can tell, all review comments have been resolved. I'll give @kring another chance to chime in though...

One clarifying question from me, do we still need CESIUM_DISABLE_DEFAULT_ELLIPSOID? If not, can we remove it? And if so, is it useful enough to put in our release notes so that other developers can use it?

(Btw, I appreciate the sheer number of files you needed to change for this PR, quite impressive!)

@kring
Copy link
Member

kring commented Jun 11, 2024

One clarifying question from me, do we still need CESIUM_DISABLE_DEFAULT_ELLIPSOID? If not, can we remove it? And if so, is it useful enough to put in our release notes so that other developers can use it?

I asked Ashley to add this because I think it's very useful, such as when adding alternate ellipsoid support to a new product that uses cesium-native. Adding it to the release notes seems like a good idea.

Copy link
Member

@kring kring left a comment

Choose a reason for hiding this comment

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

Getting close now!

Cesium3DTilesSelection/src/CesiumIonTilesetLoader.h Outdated Show resolved Hide resolved
Cesium3DTilesSelection/src/CesiumIonTilesetLoader.h Outdated Show resolved Hide resolved
Cesium3DTilesSelection/src/CesiumIonTilesetLoader.h Outdated Show resolved Hide resolved
Cesium3DTilesSelection/src/Tileset.cpp Outdated Show resolved Hide resolved
CesiumGeospatial/include/CesiumGeospatial/Projection.h Outdated Show resolved Hide resolved
CesiumGeospatial/src/BoundingRegion.cpp Outdated Show resolved Hide resolved
CesiumGeospatial/src/S2CellBoundingVolume.cpp Outdated Show resolved Hide resolved
@azrogers
Copy link
Contributor Author

@kring Updated the PR based on the review. BoundingRegion, S2CellBoundingVolume and Tileset no longer store copies of the ellipsoid, which required a good number of changes.

Copy link
Member

@kring kring left a comment

Choose a reason for hiding this comment

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

A few more small changes, I think this should be the last of it!

CMakeLists.txt Outdated Show resolved Hide resolved
Cesium3DTilesSelection/src/ImplicitQuadtreeLoader.cpp Outdated Show resolved Hide resolved
Cesium3DTilesSelection/src/ImplicitQuadtreeLoader.cpp Outdated Show resolved Hide resolved
Cesium3DTilesSelection/src/LayerJsonTerrainLoader.cpp Outdated Show resolved Hide resolved
Cesium3DTilesSelection/src/RasterOverlayUpsampler.cpp Outdated Show resolved Hide resolved
Cesium3DTilesSelection/src/RasterOverlayUpsampler.cpp Outdated Show resolved Hide resolved
@kring
Copy link
Member

kring commented Jun 20, 2024

Thanks @azrogers!

@kring kring merged commit ea161fa into main Jun 20, 2024
24 checks passed
@kring kring deleted the custom-ellipsoid branch June 20, 2024 21:51
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