search

CesiumGS / cesium-unity

Bringing the 3D geospatial ecosystem to Unity
https://cesium.com/platform/cesium-for-unity/
Apache License 2.0
358 stars 83 forks source link

Rewrite CesiumFlyToController to match improvements in cesium-unreal #395

Closed azrogers closed 8 months ago

azrogers commented 9 months ago

Fixes #390 and #334.

The CesiumFlyToController had a jump at the end of its flight path, similar to the bug in the Unreal plugin. However, the fix that we used for the Unreal plugin couldn't just be dropped in as the way the two implementations work was quite different. This change rewrites CesiumFlyToController to match the implementation of the Unreal equivalent, fixing the jump bug in the process.

azrogers commented 9 months ago

Updated review! @csciguy8. This one is pretty hefty. It's now blocked by CesiumGS/cesium-native#797 - the cesium-native implementation of GlobeFlightPath - as well as #380, which is required to get this project up to date with the latest version of cesium-native. Because of that, it'll probably take a while to get this one merged!

With that being said, I've moved the math behind CesiumFlyToController onto the cesium-native side so we can take advantage of glm's math functions to get the precision we need. More info about that side of things is in the cesium-native issue linked in the paragraph above. Then there's the glue in the cesium-unity native plugin to connect things up between C++ and Unity.

Speaking of which, I'd like some input from @kring on that part if possible. CeisumGlobeFlightPath isn't supposed to be constructed directly - you're supposed to go through FromEarthCenteredEarthFixedCoordinates or FromLongituteLatitudeHeight. The reason for this is because the creation of the GlobeFlightPath can fail (if the coordinates couldn't be mapped to the WGS84 geodetic surface) and needs to be able to return an empty value (the C++ side uses std::optional for this). But it doesn't look like Reinterop supports making a non-public constructor for a class, meaning you can happily call new CesiumGlobeFlightPath and you won't know you've done something wrong until an assertion on the C++ side crashes Unity. How can I solve this? Is there a different paradigm I should use for constructing objects where that construction can fail?

kring commented 9 months ago

But it doesn't look like Reinterop supports making a non-public constructor for a class, meaning you can happily call new CesiumGlobeFlightPath and you won't know you've done something wrong until an assertion on the C++ side crashes Unity.

Some confusion here I think. Your CesiumGlobeFlightPath does have a public default constructor. The C# compiler has generated it because you haven't explicitly declared any constructors at all. And because you're using it from ConfigureReinterop.cs, Reinterop has made it accessible from C++.

If you don't want any public constructors, you need to define a private one (even if it has no implementation). Then the line you added to ConfigureReinterop.cs will be a compiler error, and you'll want to simply remove it. Then I think everything will be working as expected, but let me know if not.

azrogers commented 9 months ago

Thanks, Kevin! With that, I was able to make the constructor private and get everything working!

azrogers commented 9 months ago

@kring Resolved your comments!

azrogers commented 9 months ago

@kring Changed to std::optional!

kring commented 9 months ago

Looks great, just one last nitpick! :)

azrogers commented 8 months ago

Resolved that last item!

csciguy8 commented 8 months ago

Looks good @azrogers!.

Checked all remaining comments, retested, and a did a sanity check over all the file diffs once more.

Will merge once CI completes...