CesiumGS / cesium-native

Apache License 2.0
402 stars 205 forks source link

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

Open azrogers opened 5 months ago

azrogers commented 5 months ago

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 commented 4 months ago

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

timoore commented 4 months ago

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 commented 4 months ago

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 commented 4 months ago

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 commented 4 months ago

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?)._

kring commented 2 months ago

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

azrogers commented 1 month ago

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