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

Add an ellipsoid tileset loader for flat terrain generation #908

Merged
merged 18 commits into from
Nov 6, 2024

Conversation

jqntn
Copy link
Contributor

@jqntn jqntn commented Jun 12, 2024

Hi,

This is a proposal to implement an ellipsoid tileset loader. It leverages the custom loader API and allows the Cesium3DTileset to generate terrain by creating an ellipsoidal surface.

I believe this is equivalent to EllipsoidTerrainProvider in CesiumJS.

In my opinion, this can be very useful, especially on low-end devices, or if you simply don't have a tileset at your disposal.

Here is a Unity example using the raster overlay, which you can try on this branch using this version of the cesium-unity package:

image
image

CC @carlopiersanti

@azrogers
Copy link
Contributor

azrogers commented Jun 12, 2024

Hi @jqntn - thanks for the pull request! Haven't dug through this yet, but I'd like to point you to PR #890, which is almost in, that adds custom ellipsoid support to cesium-native. For cesium-native, this means instead of passing the ellipsoid as a separate parameter, you can access it from TilesetOptions::ellipsoid. This will make sure it matches the ellipsoid that the georeference is using.

Alternatively, if you see a circumstance where you'd want to use a different ellipsoid for terrain generation than you use for georeferencing, I'll point you to CesiumGS/cesium-unity#465. With that PR, you can change "From WGS84 Ellipsoid" to "From Ellipsoid Asset" or something of the sort on Cesium3DTileset in Unity, and add a property to it that takes a CesiumEllipsoid asset (a ScriptableObject that contains the ellipsoid's radii).

Regardless, this looks good on a quick skim - please @ me when you're ready to have it reviewed!

@jqntn jqntn marked this pull request as ready for review June 14, 2024 14:40
@jqntn
Copy link
Contributor Author

jqntn commented Jun 14, 2024

Hi @azrogers, thanks for pointing me to #890. I'm ready to make appropriate changes on my end as soon as your PR is in👍

@azrogers
Copy link
Contributor

@jqntn Looks like #890 just made it in!

@kring kring added this to the July Release milestone Jun 24, 2024
@azrogers azrogers self-requested a review June 24, 2024 20:36
@azrogers azrogers self-assigned this Jun 24, 2024
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.

Thanks @jqntn! I have a few suggestions for changes below. Let us know if anything is unclear, or if you want some help with any of it. We really appreciate your work on it so far!

@kring
Copy link
Member

kring commented Jun 29, 2024

I'm moving this out of the July release for now, to give time to make a few more changes.

@kring kring removed this from the July Release milestone Jun 29, 2024
@jqntn jqntn requested a review from kring July 2, 2024 18:07
@jqntn
Copy link
Contributor Author

jqntn commented Sep 17, 2024

Hi @kring! Any update on this?

@j9liu
Copy link
Contributor

j9liu commented Sep 17, 2024

Hi @jqntn, sorry for the delay. Thank you for implementing @kring 's suggestions! We'll get a pair of eyes on the changes as soon as we can, thank you for your patience 🙏

@j9liu j9liu added this to the November 2024 Release milestone Oct 2, 2024
@kring
Copy link
Member

kring commented Oct 31, 2024

Really sorry, but we have to move this out of this month's release again.

@azrogers
Copy link
Contributor

azrogers commented Nov 6, 2024

Thank you for your patience on this, @jqntn. This PR is looking good, just one tiny change - can you move the additions you've made to CHANGES.md up to the "Not Released Yet" temporary section? It's currently under 0.37.0. I think that's the last bit needed to merge this in.

@azrogers
Copy link
Contributor

azrogers commented Nov 6, 2024

Scratch that, I've made that change myself. Once CI completes I'll make the merge!

@azrogers azrogers merged commit 544b2b3 into CesiumGS:main Nov 6, 2024
12 checks passed
@azrogers
Copy link
Contributor

azrogers commented Nov 6, 2024

Thanks for adding this feature, @jqntn!

@jqntn
Copy link
Contributor Author

jqntn commented Nov 6, 2024

Hi @azrogers, thank you for keeping me updated 🙂

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.

4 participants