search

CesiumGS / cesium-unreal

Bringing the 3D geospatial ecosystem to Unreal Engine
https://cesium.com/platform/cesium-for-unreal/
Apache License 2.0
902 stars 287 forks source link

Let `AGlobeAwareDefaultPawn` use `SimplePlanarEllipsoidCurve`, add more tests #1348

Closed azrogers closed 6 months ago

azrogers commented 6 months ago

Fixes #1122.

Currently, AGlobeAwareDefaultPawn only has a single test for a bug previously encountered. This change adds more tests that check its fly-to functionality.

csciguy8 commented 6 months ago

These tests look a lot like the tests in https://github.com/CesiumGS/cesium-native/pull/797.

What do you think about not bothering with the new Unreal tests, but just integrating with the native implementation, thus relying on the native tests instead?

(that said, Unreal side tests do have value, but I have a feeling most of the corner cases are in path calculation)

j9liu commented 6 months ago

My fault for not reading the issue closely when I passed this to @azrogers. @csciguy8 was there anything else you were thinking of testing when you wrote up the issue? One thing I could think of is testing that it adjusts its orientation as it moves across the Earth.

Regarding the fly-to behavior, though, perhaps we can simplify the tests just to "make sure it works" without going to deeply into the math?

csciguy8 commented 6 months ago

adjusts its orientation as it moves across the Earth

I like that, a test for orientation sounds reasonable. And maybe a test to make sure the flight path doesn't fly through the earth? (height stays >0).

As far as what goes into the Unreal side, maybe we can just check that we can start at X and arrive at Y when we expect? (verifying the integration code). All the subtleties in the middle can be a cesium-native test.

Open to discussion though. Just trying to avoid too much duplication and test coverage that won't really help us in the end.

azrogers commented 6 months ago

@csciguy8 Implemented SimplePlanarEllipsoidCurve for the fly-to controller, so all the tests from cesium-native should verify that. I've also added a test to make sure the path doesn't fly through the earth.

csciguy8 commented 6 months ago

Modified the title. We're doing a bit more than just adding tests :)