Closed j9liu closed 1 year ago
The original issue suggests a solution that involves safeguarding against invalid values and warning the user.
Here's a proposal for an alternate solution, let's remove the FlyToGranularityDegrees
property completely.
Here are supporting reasons why:
FlyToGranularityDegrees
is not user intuitive. It's essentially a user-controlled variable controlling the smoothness of the approximated flight path of the camera. It defaults to 0.01, which is not obvious whether this produces a good or bad result. FlyToGranularityDegrees
can be a performance sink. In AGlobeAwareDefaultPawn::FlyToLocationECEF
, this value is inversely related to the number of keypoints generated. A value of zero results in a divide by 0, which should be avoided. It defaults to 0.01, but a progressively smaller value results in more iterations and more memory used. For example, traversing an arc of 90 degrees at a 0.00001 granularity results in 9,000,000 iterations / key points to be stored. What is the minimum value that is acceptable here?Ellipsoid::scaleToGeodeticSurface
. Admittedly, this is a non-trivial function with an internal iteration loop of its own. From the book "3D Engine Design for Virtual Globles", section 2.3 Math Transformation, "Patrick says" that the scaleToGeodeticSurface function has an internal iteration of count of 1-2 for Earth sized ellipses. This function may have a reasonable execution time...Ellipsoid::scaleToGeodeticSurface
, with an expectation of 1-2 iterations, it looks reasonable to call this once per frame (AGlobeAwareDefaultPawn::Tick
). The base logic uses 21 multiplications, 2 divides, and 1 square root. Each iteration pass uses an additional 19 multiplications and 4 divides. This means that a single call for an Earth-shaped ellipsoid would use at most 59 multiplications, 10 divides, and 1 square root.The final solution would replace the logic that looks up the current position (_keypoints, AGlobeAwareDefaultPawn::_handleFlightStep
) with the logic that initially generates the cached position (AGlobeAwareDefaultPawn::FlyToLocationECEF
). The _keypoints
member will be removed completely as well as the FlyToGranularityDegrees
property.
Comments and suggestions welcome! @kring @j9liu
Thanks @csciguy8, that sounds reasonable to me. I wouldn't be concerned about executing a scaleToGeodeticSurface
every frame. As you demonstrated, it's not very expensive at all.
This was discovered in https://github.com/CesiumGS/cesium-unity/pull/154. The code for computing the flight path of
GlobeAwareDefaultPawn
does not account for the case whenflyToGranularityDegrees
is 0. This results in a divide by zero error and an infinitefor
loop that stalls the editor.https://github.com/CesiumGS/cesium-unreal/blob/8e65171bca1dbae5e57e60c9efa75f87f60ab0a9/Source/CesiumRuntime/Private/GlobeAwareDefaultPawn.cpp#L110-L112
For Cesium for Unity, we logged an error whenever
FlyToLocation
was called withflyToGranularityDegrees
set to 0. Then, the function would return early. This should be done similarly in Cesium for Unreal. It may also help to log a warning if the user sets the value to zero.