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 GlobeFlightPath helper class to handle math for fly-to components. #797

Merged
merged 10 commits into from
Feb 5, 2024

Conversation

azrogers
Copy link
Contributor

Required for CesiumGS/cesium-unity#395.

Both our Unreal and Unity implementations implement a fly-to class that animates a camera movement from one position on the globe to another. For one, the math between these two implementations is identical, and it would be nice to share code between the two implementations. But more importantly, Unity lacks double-precision implementations of the math functions we need to calculate this path, and trying to do the math with floats results in precision errors that cause jumping at the end of the flight (see CesiumGS/cesium-unity#390). By moving this implementation to the C++ side, we can use glm's double-precision math to solve these issues, while also eventually allowing us to unify the Unreal and Unity implementations.

This is implemented as a class that can take a normalized value representing position along the line and return ECEF coordinates for that point. This allows the path to be sampled continuously for each frame, while also allowing each implementation to choose how that normalized value is computed. This keeps things like the _flyToProgressCurve on the implementation side, letting us use the engine's curve classes with the side effect of allowing use-cases beyond just the fly-to controller. For example, you could use this class to place markers along the flight path, or to turn the curve into a set of vertices to display the line an object is moving along.

double _destinationHeight;
double _length;

glm::dvec3 _sourceDirection;
Copy link
Contributor

Choose a reason for hiding this comment

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

The private members should be const.

Copy link
Contributor

@lilleyse lilleyse Jan 29, 2024

Choose a reason for hiding this comment

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

Not sure if relevant to this class, but the C++ Core Guidelines advises against this for copyable / movable types: C.12

@timoore
Copy link
Contributor

timoore commented Jan 29, 2024

This approach is an approximation to the geodesic (cough jargon cough), and the approximation has the problem that the velocity over the ellipsoid isn't constant as one changes the angle between the start and end vectors. If this is going into cesium-native, I wonder if it would be better to have real solution to the geodesic problem? Either follow the methods in the wikipedia page and the linked reference https://doi.org/10.1007%2Fs00190-012-0578-z, or punt and use libPROJ.

@csciguy8
Copy link
Contributor

I'm not aware of any requirement for constant velocity for Unreal or Unity. The "fly to" animations have other aesthetic aspects, like height and progress curves, that make their use less than scientific.

I do like the simplicity and performance of this class. It's at least a good place to start if you're starting from scratch.

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.

This looks good, and I think it's very reasonable to a) use this class for our camera flights, at least for now, and b) include this class in cesium-native. I think we should give it a different name, though.

The trouble is it's a bit hard to describe what this curve actually is. It's not a geodesic. It's not a rhumb line. It's not even the curve described in 3D Engine Design for Virtual Globes in section 2.4, because that curve scales each point to the geocentric surface.

I don't think if there's a name for this curve. It's basically, you know, some random curve that mostly looks fine for camera flights and is easy to compute. 😆

But now that I think about it, I think I made a mistake in CesiumGS/cesium-unreal#1333. We had a jump in the camera flight at the end. I realized that was caused by the start and end points being incorrect because taking the end direction, scaling it to the geodetic surface and adding the end height does not yield the original end point. I fixed that by computing the start and end directions after scaling to the geodetic surface. However, I should have gone in the geocentric direction instead. Leave the direction computation as-is, and instead change the step computation to scale to the geocentric surface before adding the geocentric height. This would make the math consistent with that section 2.4 in 3D Engine Design for Virtual Globes. And it would make all the points on the path lie in a plane. It still wouldn't actually be a geodesic, but it'd be pretty close. And it's a faster and simpler computation, too.

So, at the risk of drawing this task out further, let's do this:

  1. Add scaleToGeocentricSurface to our Ellipsoid class. You can port the implementation from CesiumJS, which is only a few lines of code: https://github.com/CesiumGS/cesium/blob/50373e66f455a01b9428cb3320db229b552a6a36/packages/engine/Source/Core/Ellipsoid.js#L550
  2. No need to scale the source and destination points to the geodetic surface before computing the rotation axis and angle. Effectively, revert my change in Fix jump at end of flights. cesium-unreal#1333.
  3. The source and destination heights shouldn't be computed with a cartesianToCartographic anymore. Instead, they should be magnitude(sourceEcef - scaledSourceEcef) (and similar for destination).
  4. At each step, scale to the geocentric surface and then add the height. But don't add the height along the geodeticUp vector. Instead, add it along the geocentric up vector, which is simply normalize(position).
  5. Rename GlobeFlightPath to perhaps SimplePlanarEllipsoidCurve. The documentation should say that it produces points that lie in a plane going through the center of the Earth, and that their height above the ellipsoid will be a linear interpolation between the source and destination heights.

This turned into a lot of words. I hope it's not totally confusing. Let me know if anything doesn't make sense or if you disagree, because I'm not 100% sure I've thought through it completely.

The bigger picture here is still that CesiumJS's flight implementation is probably the better way to go. So if what I just described is a lot of work, it's probably not worth doing. And feel free to consider jumping right to the CesiumJS flight implementation instead if you prefer!

CesiumGeospatial/test/TestGlobeFlightPath.cpp Outdated Show resolved Hide resolved
CesiumGeospatial/test/TestGlobeFlightPath.cpp Outdated Show resolved Hide resolved
@azrogers
Copy link
Contributor Author

@kring I've updated the PR based on your feedback, and implemented the geocentric calculation you recommended. As far as I can tell I still think the CesiumJS solution is just lerping between cartographic coordinates - see here. I think our solution is probably more accurate.

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.

Looks great @azrogers! Reviewed the code and tested in cesium for unreal.

Have a couple comments that I could go either way on.

Let me know what you think, then we can merge.

@csciguy8 csciguy8 merged commit aa561e3 into main Feb 5, 2024
14 checks passed
@csciguy8 csciguy8 deleted the globe-flight-path branch February 5, 2024 16:25
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.

5 participants