Large refactor of the C++ code

[lilleyse]

Fixes #539
Fixes #584
Fixes #600
Fixes #588
Fixes #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
  • Orientation and scale incorrect when using anchor_xform_at_path #539
  • Adding a Globe Anchor to a scaled prim changes size or position in the stage #584
  • Globe anchor position out of sync after programmatic creation #600
• 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 Use rel for imagery layers #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 C API #3
• Reduced the amount of Usd::Gf math. glm is used almost everywhere now.

[timoore]

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]

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).

[timoore]

Drive-by review

[timoore]

This is pretty hairy stuff. I think that you are calling a lambda or other function on a list of arbitrary types, and returning the results in an array?

Someday you are going to regret returning a static array 🥲 I think you could use std:array here and return it safely. https://stackoverflow.com/questions/12024304/c11-number-of-variadic-template-function-parameters getting the length.

I don't think the type of the lambda needs to be a template argument. You could write auto&& lambda instead.

Perhaps this is all bikeshedding; I don't see where you're using this function in your PR.

[lilleyse]

I borrowed this solution from stack overflow, but I can't seem to find the post right now...

This function is called by CALL_TEMPLATED_FUNCTION_WITH_RUNTIME_DATA_TYPE and related macros. When used in that context, dispatchImpl returns an array storing function pointers for all enumerations, and dispatch calls the function for the given enumeration.

I had tried to switch to std::array before, and thought I was using sizeof...(I) correctly, but got blocked by something. I just tried it again and it worked🤷‍♂️. I pushed the change: 61f2698

I did just try auto&& lambda, but that only seems possible with C++ 20.

[timoore]

This function is called by CALL_TEMPLATED_FUNCTION_WITH_RUNTIME_DATA_TYPE and related macros. When used in that context, dispatchImpl returns an array storing function pointers for all enumerations, and dispatch calls the function for the given enumeration.

Ah, OK. I searched for uses of dispatch() but didn't realize that dispatchImpl is actually the one used.

I had tried to switch to std::array before, and thought I was using sizeof...(I) correctly, but got blocked by something. I just tried it again and it worked🤷‍♂️. I pushed the change: 61f2698

I did just try auto&& lambda, but that only seems possible with C++ 20.

My bad, that only works in lambda argument lists pre C++20.

[timoore]

I imagine that this nugget of joy is dealt with in every Cesium program that uses cesium-native. I wonder if there's a way to deal with it there, or at least come up with a standard idiom for killing the OPAQUE macro.

[lilleyse]

I opened a cesium-native PR for that: CesiumGS/cesium-native#796

[corybarr]

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]

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:

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

2. Create a Scope
3. Drag a CesiumPolygonImageryPrim to the scope :

4. Drag the CesiumPolygonImageryPrim back to the CesiumTilesetPrim
5. Refresh the tileset:
   

clippingTest.twoPolygonLayers.cleanupBranch.usda.zip

[lilleyse]

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

[corybarr]

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

Already fixed. Awesome.

[lilleyse]

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]

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

[corybarr]

This codebase feels considerable more manageable now.