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

Clean up and reorganize ACesiumGeoreference, and improve its transformation API #1179

Merged
merged 20 commits into from
Sep 11, 2023

Conversation

kring
Copy link
Member

@kring kring commented Jul 28, 2023

This is a PR into #1174 so merge that first.

Lots of (mostly small) changes to CesiumGeoreference:

  • Organized the class into named regions.
  • Removed all the glm functions from CesiumGeoreference. Now that UE's FMatrix and FVector are double-precision, there's little need for these.
  • Moved Ellipsoid-related functions that don't depend on the CesiumGeoreference at all to a new UCesiumWgs84Ellipsoid blueprint function library (also usable from C++). This is similar to how Cesium for Unity is organized. Unfortunately it doesn't seem possible to CoreRedirect an instance method to a static one, so deprecated methods still exist on CesiumGeoreference as well.
  • Implemented the best practices documented in Documentation/developing-uobjects.md.
    • Made previously public properties private and accessible from C++ via public get/set functions. That's important because setting these via C++ previously would have done nothing and left the georeference instance in an inconsistent state. In Blueprints, access is transparent via BlueprintGetter / BlueprintSetter.
  • Added a Doxyfile that can be used to generate C++ reference docs with Doxygen. This isn't "production ready", but it was useful to examine the public interface of CesiumGeoreference. We could eventually ship the docs with the plugin or publish them on our web site.
  • Improved a lot of the doc comments.
  • Renamed a bunch of things for consistency and clarity. I've added CoreRedirects so Blueprints should continue to work, but will be breaking changes for C++ code.
  • Added tests for a lot of the transform functions.
  • Made sure all Blueprint-accessible functions and properties are in sensible and consistent categories so that users can find them more easily.

Fixes #724
Related to #1170 but not a complete fix just yet

@cesium-concierge
Copy link

Thanks for the pull request @kring!

  • ✔️ Signed CLA found.

Reviewers, don't forget to make sure that:

@kring kring added this to the v2.0 milestone Jul 28, 2023
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 pretty good. Minor questions and comments about new and old code.

In addition to looking at the code, I verified blueprints access still looked sensible, and that the new test is running on CI. CesiumGeoreference was probably the hardest to review as the diff was probably the most confused with the .cpp file.

@csciguy8 csciguy8 merged commit 58d8461 into transformable-georeference Sep 11, 2023
25 checks passed
@csciguy8 csciguy8 deleted the transform-api branch September 11, 2023 15:53
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.

3 participants