Large refactor of the C++ code

lilleyse commented 7 months ago

Fixes https://github.com/CesiumGS/cesium-omniverse/issues/539 Fixes https://github.com/CesiumGS/cesium-omniverse/issues/584 Fixes https://github.com/CesiumGS/cesium-omniverse/issues/600 Fixes https://github.com/CesiumGS/cesium-omniverse/issues/588 Fixes https://github.com/CesiumGS/cesium-omniverse/issues/616

This is a wide reaching cleanup PR that has both functional changes and C++ style changes. Ideally this would have been spit into multiple PRs, but here we are 🤕

Globe Anchors

The Python API for adding globe anchors has been removed. The Python <-> C++ syncing issues have been fixed and globe anchors can now be defined directly in USD at the Python layer.
Globe anchors no longer add a cesium:transform op to the attached prim. Instead they modify the translate, rotate (all variants), and scale ops directly.
Several bugs were fixed
The naming of transforms is now more consistent throughout the code

Asset Registry

Every Cesium prim type now has a corresponding Omni class. All Omni objects are stored in and accessed from AssetRegistry. A lot of time was spent making sure UsdNotificationHandler was accurately updating AssetRegistry and the various Omni classes.
OmniPolygonImagery and OmniIonImagery now own the underlying cesium-native raster overlay object. This will allow multiple tilesets to use the same imagery resource once #585 is implemented.
GlobeAnchorRegistry and SessonRegistry have been consolidated into AssetRegistry

Context

Most singletons and global variables have been removed. Anything that was global state is now stored in Context.
Multiple Context objects can now be created, if desired, such as in unit tests
Most ion related code has moved into CesiumIonServerManager. I hoped to do more refactoring in this PR but ran out of time.

File changes

Created CppUtil for various C++ or standard library related helper functions
Renamed Fabric***Definition to Fabric***Descriptor
Created separate files for many of the Fabric structs

Style changes

++i instead of i++ for the slight performance gain and to match cesium-native conventions
Use const more frequently
Helper functions in ***Util.h files are now scoped in sub namespaces of cesium::omniverse
Add p prefix to pointers, similar to cesium-native style
More aggressively deleted copy constructors and copy assignment operators to prevent coding errors
Consistently using brace initialization for member variables in header files
Renamed srw and isrw to fabricStage and iFabricStage
More aggressive about using forward declarations in header files
std::string_view instead of const char* in certain places
~pxr:: -> PXR_NS:: which is more consistent with other projects like kit-extension-template-cpp~
Tried to fix most errors reported by clang-tidy but there are still a few remaining

Other changes

The code is much more resilient if /Cesium or /CesiumGeoreference prims aren't defined or rel's are empty
Added ability to log oneTimeWarning, which fixes #588
Updated FabricTexture to handle more channels and bit depths, which fixes #616
CesiumOmniverse.h no longer depends on GfMatrix4d. This is one step closer towards towards https://github.com/CesiumGS/cesium-omniverse/issues/3
Reduced the amount of Usd::Gf math. glm is used almost everywhere now.

timoore commented 7 months ago

When I write a mega-change like this, I try to break it up after the fact into commits that could at least compile and might even work. I'm a fan of the workflow described in https://git-scm.com/docs/git-stash#Documentation/git-stash.txt-Testingpartialcommits for going about this. I do this for the obvious reasons of:

Telling a story about the design and progression of the changes via the commits;
Providing a better surface for git bisect if that is needed.

I have no idea if it is practical to do this for this commit, or even a good use of time, so this is just a noob suggestion!

lilleyse commented 7 months ago

Thanks for the suggestion @timoore. It could be tricky to detangle things at this point, but I agree with everything you said. I haven't tried that specific workflow but I'll give it a shot the next time this happens (which hopefully won't be very often).

corybarr commented 7 months ago

This isn't related to this PR, but it is making me ask the question of if there's a better approach than having a CesiumCartographicPolygon contain one BasisCurves and one GlobeAnchor. I'm wondering if we want to have CesiumCartographicPolygon prims potentially share a GlobeAnchor. If we ever wanted to shift all CartographicPolygons' GlobeAnchors, we'd need to do each individually. Maybe that's rare in workflows, or could be something in PowerTools.

corybarr commented 7 months ago

There's an issue in this branch that doesn't reproduce in main. If you de-parent a CesiumPolygonImageryPrim from a CesiumTilesetPrim by dragging it in the Stage Window, then when you re-parent the CesiumPolygonImageryPrim its polygon won't clip. To reproduce:

Open the USDA file. You'll see two clipped polygons:

Create a Scope
Drag a CesiumPolygonImageryPrim to the scope :

Drag the CesiumPolygonImageryPrim back to the CesiumTilesetPrim
Refresh the tileset:

clippingTest.twoPolygonLayers.cleanupBranch.usda.zip

lilleyse commented 7 months ago

@corybarr that was fixed in https://github.com/CesiumGS/cesium-omniverse/pull/626/commits/be1024cac2de62f0a5e770136eab40530804dc0a. We must have both independently discovered the issue around the same time.

corybarr commented 7 months ago

@corybarr that was fixed in be1024c. We must have both independently discovered the issue around the same time.

Already fixed. Awesome.

lilleyse commented 7 months ago

I disabled building tests on CI - I was seeing linker errors that I couldn't reproduce locally and don't really have the energy to figure out. The test infrastructure needs to be rethought in order to be CI compatible.

[100%] Linking CXX shared module ../../lib/CesiumOmniverseCppTestsPythonBindings.cpython-310-x86_64-linux-gnu.so
lto-wrapper: warning: using serial compilation of 2 LTRANS jobs
/usr/bin/ld: ../../lib/libCesiumOmniverseCore.a(UsdTokens.cpp.o): in function `carb::cpp::atomic<carb::detail::CachedInterface<omni::fabric::IToken, (char const*)0>::RequestState>::wait(carb::detail::CachedInterface<omni::fabric::IToken, (char const*)0>::RequestState, std::memory_order) const':
UsdTokens.cpp:(.text._ZNK4carb3cpp6atomicINS_6detail15CachedInterfaceIN4omni6fabric6ITokenELPKc0EE12RequestStateEE4waitESA_St12memory_order[_ZNK4carb3cpp6atomicINS_6detail15CachedInterfaceIN4omni6fabric6ITokenELPKc0EE12RequestStateEE4waitESA_St12memory_order]+0xbd): undefined reference to `carb::thread::detail::ParkingLot::WaitBucket::bucket(void const*)::waitBuckets'
/usr/bin/ld: ../../lib/libCesiumOmniverseCore.a(UsdTokens.cpp.o): relocation R_X86_64_PC32 against undefined hidden symbol `_ZZN4carb6thread6detail10ParkingLot10WaitBucket6bucketEPKvE11waitBuckets' can not be used when making a shared object
/usr/bin/ld: final link failed: bad value
collect2: error: ld returned 1 exit status
gmake[2]: *** [tests/bindings/CMakeFiles/CesiumOmniverseCppTestsPythonBindings.dir/build.make:159: lib/CesiumOmniverseCppTestsPythonBindings.cpython-310-x86_64-linux-gnu.so] Error 1
gmake[1]: *** [CMakeFiles/Makefile2:678: tests/bindings/CMakeFiles/CesiumOmniverseCppTestsPythonBindings.dir/all] Error 2
gmake: *** [Makefile:166: all] Error 2

lilleyse commented 7 months ago

@corybarr I don't think I can get to the sign out issue in this PR, though I would like to address it before the release.

I'm marking the PR as non-draft now. I think I've tested as much as I can and it would be good to get this finally merged.

CesiumGS / cesium-omniverse