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

Let AGlobeAwareDefaultPawn use SimplePlanarEllipsoidCurve, add more tests #1348

Merged
merged 8 commits into from
Feb 6, 2024

Conversation

azrogers
Copy link
Contributor

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
Copy link
Contributor

These tests look a lot like the tests in CesiumGS/cesium-native#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
Copy link
Contributor

j9liu commented Jan 30, 2024

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
Copy link
Contributor

csciguy8 commented Jan 30, 2024

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
Copy link
Contributor Author

azrogers commented Feb 1, 2024

@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 csciguy8 changed the title Add more tests for AGlobeAwareDefaultPawn Let AGlobeAwareDefaultPawn use SimplePlanarEllipsoidCurve, add more tests Feb 5, 2024
@csciguy8
Copy link
Contributor

csciguy8 commented Feb 5, 2024

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

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. Thanks @azrogers .

Reviewed the code with the new SimplePlanarEllipsoidCurve integration and tested.

Will merge once CI finishes...

@csciguy8 csciguy8 merged commit a4e2941 into main Feb 6, 2024
25 checks passed
@csciguy8 csciguy8 deleted the globe-aware-tests branch February 6, 2024 15:38
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.

Create tests for AGlobeAwareDefaultPawn
3 participants