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

Implement a class to load and sample an EGM96 grid #835

Open
wants to merge 15 commits into
base: main
Choose a base branch
from
Open

Conversation

azrogers
Copy link
Contributor

Closes #637.

Users often have data with heights referenced to mean sea-level (MSL), but Cesium uses the WGS84 ellipsoid internally, which can differ from MSL by dozens of meters in some locations. Accurately determining MSL requires an external geopotential model. This change allows users to load the EGM96 model and sample heights from it, allowing for MSL heights to be converted to ellipsoid heights. Converting from MSL to the ellipsoid becomes as simple as:

const std::optional<EarthGravitationalModel96Grid> grid = EarthGravitationalModel96Grid::fromFile("WW15MGH.DAC");
assert(grid.has_value());
const Cartographic mappedPosition = Cartographic(
    position.longitude,
    position.latitude,
    position.height + grid->sampleHeight(position));

@azrogers
Copy link
Contributor Author

@timoore Updated review based on your comments. Latitude values are now clamped.

@timoore
Copy link
Contributor

timoore commented Apr 16, 2024

I'm ready to merge this, but I notice that the WW15MGH.DAC file is only test data. What's the plan to make this more official? Egm96 height will very useful to users and should be easy to use from cesium-native. Perhaps the source data file should be transformed into C++ source by a python script or something and linked in?

@azrogers
Copy link
Contributor Author

We could definitely include WW15MGH.DAC in the binary, by generating a header file either manually or as part of the build process. However, I wonder if it doesn't just make more sense to have users bring their own WW15MGH.DAC? After all, most users probably won't touch the EGM96 grid, so it would save on 2MB of extra dependencies for people who don't. We'll definitely have to do this if we decide to add other models (the EGM08 grid is 128MB) and it makes sense to me to keep it consistent, even if 2MB isn't that much.

@timoore
Copy link
Contributor

timoore commented Apr 17, 2024

We could definitely include WW15MGH.DAC in the binary, by generating a header file either manually or as part of the build process. However, I wonder if it doesn't just make more sense to have users bring their own WW15MGH.DAC? After all, most users probably won't touch the EGM96 grid, so it would save on 2MB of extra dependencies for people who don't. We'll definitely have to do this if we decide to add other models (the EGM08 grid is 128MB) and it makes sense to me to keep it consistent, even if 2MB isn't that much.

I see a lot of forum posts that boil down to ellipsoid vs geoid confusion, so I think this functionality is useful; on the other hand, I don't see users bringing their own WW15MGH.DAC file; where would they even get it? In other words, the set of users helped by this new functionality is a lot larger than the set than know how to help themselves.

I don't care much if the data is a built-in array or read from a file. Do we have any external data files in cesium-native that need to be installed and used by the clients? The clients e.g. cesium-omniverse already have external data dependencies.

@kring
Copy link
Member

kring commented Apr 17, 2024

For our integrations (Unreal, Unity, Omniverse), we should include the WW15MGH.DAC file with the distribution so users don't have to think about it.

For cesium-native, users should supply the file or its contents when constructing an instance of the type, as is already done in this PR. But we should make it easy for them to do so by telling people in the reference docs (doxygen comments) where to get it. It used to be really easy to get this file directly from NGA, but that doesn't seem to be true anymore. So we should probably also include it in a data directory (but maybe not under test?)._

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.

Lots of little comments, but it looks great overall @azrogers! Also please update CHANGES.md.

@kring
Copy link
Member

kring commented Jun 4, 2024

@azrogers bump! Not sure if you saw that I reviewed this.

@azrogers
Copy link
Contributor Author

Looks like I forgot to @ you @kring on Friday when I updated this PR. Should be good for another round of review!

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.

Add a function to convert between WGS84 and EGM96/EGM2008 heights
3 participants