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

azrogers · 2024-03-21T18:43:17Z

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));

CesiumGeospatial/include/CesiumGeospatial/EarthGravitationalModel96Grid.h

CesiumGeospatial/src/EarthGravitationalModel96Grid.cpp

azrogers · 2024-03-22T17:22:01Z

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

timoore · 2024-04-16T12:31:53Z

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 · 2024-04-16T16:56:17Z

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 · 2024-04-17T09:59:46Z

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 · 2024-04-17T10:42:40Z

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

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

kring · 2024-05-24T07:04:11Z

CesiumGeospatial/include/CesiumGeospatial/EarthGravitationalModel96Grid.h

+ * @brief Allows elevation data in the format provided by the Earth
+ * Gravitational Model 1996 to be loaded and queried.


Active voice would be better:

Suggested change

* @brief Allows elevation data in the format provided by the Earth

* Gravitational Model 1996 to be loaded and queried.

* @brief Loads and queries heights from the Earth Gravity Model 1996 (EGM96).

Just call me the MS Word grammar checker. 😎

Also elevation -> height. The word elevation tends to imply a height above sea level, so we avoid it.

kring · 2024-05-24T07:08:43Z

CesiumGeospatial/include/CesiumGeospatial/EarthGravitationalModel96Grid.h

+   * @returns The height representing the difference in meters of mean sea-level
+   * from the surface of a WGS84 ellipsoid.
+   */
+  double sampleHeight(Cartographic& position) const;


Suggested change

double sampleHeight(Cartographic& position) const;

double sampleHeight(const Cartographic& position) const;

kring · 2024-05-24T07:12:57Z

CesiumGeospatial/include/CesiumGeospatial/EarthGravitationalModel96Grid.h

+   * @returns The height representing the difference in meters of mean sea-level
+   * from the surface of a WGS84 ellipsoid.


Can we state this a little more clearly? The current wording doesn't make it clear to me whether the returned value is positive or negative when the EGM96 surface is above the ellipsoid. Something like:

"The height (in meters) of the EGM96 surface above the WGS84 ellipsoid. A positive value indicates that the EGM96 surface is above the ellipsoid, while a negative value indicates that the MSL surface is below the ellipsoid."

(Please double check I have that right! I can never remember if it's that or the opposite. 😆)

kring · 2024-05-24T07:13:47Z

CesiumGeospatial/include/CesiumGeospatial/EarthGravitationalModel96Grid.h

+  double sampleHeight(Cartographic& position) const;
+
+private:
+  EarthGravitationalModel96Grid(const std::vector<int16_t>& gridValues);


Save a copy!

Suggested change

EarthGravitationalModel96Grid(const std::vector<int16_t>& gridValues);

EarthGravitationalModel96Grid(std::vector<int16_t>&& gridValues);

kring · 2024-05-24T07:19:44Z

CesiumGeospatial/src/EarthGravitationalModel96Grid.cpp

+    gridValues.push_back(gridValue);
+  }
+
+  return EarthGravitationalModel96Grid(gridValues);


Suggested change

return EarthGravitationalModel96Grid(gridValues);

return EarthGravitationalModel96Grid(std::move(gridValues));

kring · 2024-05-24T07:39:59Z

CesiumGeospatial/src/EarthGravitationalModel96Grid.cpp

+  const double longitude =
+      std::remainder(position.longitude - Math::OnePi, Math::TwoPi) +
+      Math::OnePi;


Use CesiumUtility::Math::zeroToTwoPi.

kring · 2024-05-24T07:44:03Z

CesiumGeospatial/src/EarthGravitationalModel96Grid.cpp

+  const double latitude =
+      std::clamp(position.latitude, -Math::PiOverTwo, Math::PiOverTwo);


Suggested change

const double latitude =

std::clamp(position.latitude, -Math::PiOverTwo, Math::PiOverTwo);

const double latitude =

Math::clamp(position.latitude, -Math::PiOverTwo, Math::PiOverTwo);

kring · 2024-05-24T07:47:49Z

CesiumGeospatial/src/EarthGravitationalModel96Grid.cpp

+    clampedVertical = NUM_ROWS - 1;
+  }
+
+  const std::vector<int16_t>::size_type index =


You can just use size_t instead of std::vector<int16_t>::size_type.

kring · 2024-05-24T07:48:23Z

CesiumGeospatial/src/EarthGravitationalModel96Grid.cpp

+  const std::vector<int16_t>::size_type index =
+      static_cast<std::vector<int16_t>::size_type>(
+          clampedVertical * NUM_COLUMNS + horizontal);
+  const double result = static_cast<double>(_gridValues[index]) / 100.0;


Is this static_cast needed? I think it shouldn't be.

kring · 2024-05-24T07:50:21Z

CesiumGeospatial/test/TestEarthGravitationalModel96Grid.cpp

+        57.79),
+    Egm96TestCase(
+        Cartographic::fromDegrees(179.78766535213848, -66.77911223257036, 0),
+        -57.37),


You're testing both positive and negative values, so my fears about sign extension are unfounded. Up to you if you like the code I suggested better anyway.

kring · 2024-06-04T07:19:28Z

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

azrogers added 8 commits March 21, 2024 14:33

EGM96 grid implementation

8e9713a

Merge branch 'main' of github.com:CesiumGS/cesium-native into egm96

486dabe

Format

175d538

Fix case of include

98d7ee6

Fix sign conversion errors

d1f9d16

Fix index conversion

bbab6b4

Fix more conversion errors

68124e2

Fix unused variable

3f866b3

timoore reviewed Mar 21, 2024

View reviewed changes

CesiumGeospatial/include/CesiumGeospatial/EarthGravitationalModel96Grid.h Outdated Show resolved Hide resolved

timoore reviewed Mar 21, 2024

View reviewed changes

CesiumGeospatial/src/EarthGravitationalModel96Grid.cpp Outdated Show resolved Hide resolved

timoore reviewed Mar 21, 2024

View reviewed changes

CesiumGeospatial/src/EarthGravitationalModel96Grid.cpp Outdated Show resolved Hide resolved

timoore reviewed Mar 21, 2024

View reviewed changes

CesiumGeospatial/src/EarthGravitationalModel96Grid.cpp Outdated Show resolved Hide resolved

Remove mapLatitude, mapLongitude methods

660436f

timoore mentioned this pull request Apr 15, 2024

Assistance In Implementing A New Frontend #823

Open

kring reviewed May 24, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

azrogers commented Mar 21, 2024

azrogers commented Mar 22, 2024

timoore commented Apr 16, 2024

azrogers commented Apr 16, 2024

timoore commented Apr 17, 2024

kring commented Apr 17, 2024

kring left a comment

kring May 24, 2024

kring May 24, 2024

kring May 24, 2024

kring May 24, 2024

kring May 24, 2024

kring May 24, 2024

kring May 24, 2024

kring May 24, 2024

kring May 24, 2024

kring May 24, 2024

kring commented Jun 4, 2024

		* @brief Allows elevation data in the format provided by the Earth
		* Gravitational Model 1996 to be loaded and queried.

	* @brief Allows elevation data in the format provided by the Earth
	* Gravitational Model 1996 to be loaded and queried.
	* @brief Loads and queries heights from the Earth Gravity Model 1996 (EGM96).

	double sampleHeight(Cartographic& position) const;
	double sampleHeight(const Cartographic& position) const;

		* @returns The height representing the difference in meters of mean sea-level
		* from the surface of a WGS84 ellipsoid.

	EarthGravitationalModel96Grid(const std::vector<int16_t>& gridValues);
	EarthGravitationalModel96Grid(std::vector<int16_t>&& gridValues);

	return EarthGravitationalModel96Grid(gridValues);
	return EarthGravitationalModel96Grid(std::move(gridValues));

		const double latitude =
		std::clamp(position.latitude, -Math::PiOverTwo, Math::PiOverTwo);

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

Are you sure you want to change the base?

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

Conversation

azrogers commented Mar 21, 2024

azrogers commented Mar 22, 2024

timoore commented Apr 16, 2024

azrogers commented Apr 16, 2024

timoore commented Apr 17, 2024

kring commented Apr 17, 2024

kring left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kring commented Jun 4, 2024