Support loading pre-existing USD files #205

lilleyse · 2023-03-08T00:16:10Z

Fixes #174. Review #204 first.

I tested a variety of situations while working on this PR:

Drag and drop .usda with CesiumTileset and CesiumImagery prims in it
Rename tileset
Remove tileset
Remove imagery
Parent tileset to a different prim
Parent tileset to a different tileset
Parent imagery to a different tileset
Parent imagery to a non-tileset prim. It should have no effect.
Remove prim that has child tileset prims
Save stage

lilleyse · 2023-03-08T00:17:20Z

src/core/include/cesium/omniverse/UsdNotificationHandler.h

@@ -13,11 +13,15 @@ enum class ChangedPrimType {
    OTHER,
 };

-enum class ChangeType { PROPERTY_CHANGED, PRIM_ADDED [[maybe_unused]], PRIM_MOVED [[maybe_unused]], PRIM_REMOVED };


I removed PRIM_MOVED from this list. USD gives us two resync notifications: first a remove, then an add. Treating them as separate events is actually easier in our code.

lilleyse · 2023-03-08T00:32:08Z

src/core/src/Context.cpp

@@ -153,7 +154,6 @@ pxr::SdfPath Context::addTilesetUrl(const std::string& name, const std::string&

    tilesetUsd.GetUrlAttr().Set<std::string>(url);

-    AssetRegistry::getInstance().addTileset(tilesetPath);


Here and below: these functions are now only responsible for adding the USD prims. The USD notification system will let us know when the prims are added at which point the tileset/imagery will be added to the asset registry and the OmniTileset/OmniImagery objects will be created.

Should we be worried about any race conditions during the very brief time that the tileset has been added and the notification hasn't been handled?

I don't think so... right now the update loop is the only thing that cares about OmniTileset objects and it's happy to wait until they are created.

lilleyse · 2023-03-08T00:34:35Z

src/core/src/UsdNotificationHandler.cpp

+bool inSubtree(const pxr::SdfPath& path, const pxr::SdfPath& descendantPath) {
+    if (path == descendantPath) {
+        return true;
+    }
+
+    for (const auto& ancestorPath : descendantPath.GetAncestorsRange()) {
+        if (ancestorPath == path) {
+            return true;
+        }
+    }
+
+    return false;
+}


I was hoping there would be a path.isDescendant(otherPath) but couldn't find it. Maybe I'm overlooking it?

There isn't. The only way to collect all of the descendant paths is to traverse the stage. There are a few helper functions for taking action on descendant paths but they all assume you have a vector of all of the paths you care about rather than actually handling traversal themselves and just check for a particular prefix to say "yup, this is descendant" in that list you provide.

weegeekps

Looks great but I have one minor question before I approve.

weegeekps · 2023-03-08T16:59:18Z

src/core/src/Context.cpp

@@ -153,7 +154,6 @@ pxr::SdfPath Context::addTilesetUrl(const std::string& name, const std::string&

    tilesetUsd.GetUrlAttr().Set<std::string>(url);

-    AssetRegistry::getInstance().addTileset(tilesetPath);


Should we be worried about any race conditions during the very brief time that the tileset has been added and the notification hasn't been handled?

weegeekps · 2023-03-08T17:10:32Z

src/core/src/UsdNotificationHandler.cpp

+bool inSubtree(const pxr::SdfPath& path, const pxr::SdfPath& descendantPath) {
+    if (path == descendantPath) {
+        return true;
+    }
+
+    for (const auto& ancestorPath : descendantPath.GetAncestorsRange()) {
+        if (ancestorPath == path) {
+            return true;
+        }
+    }
+
+    return false;
+}


There isn't. The only way to collect all of the descendant paths is to traverse the stage. There are a few helper functions for taking action on descendant paths but they all assume you have a vector of all of the paths you care about rather than actually handling traversal themselves and just check for a particular prefix to say "yup, this is descendant" in that list you provide.

weegeekps · 2023-03-08T17:13:24Z

src/core/src/UsdNotificationHandler.cpp

+    for (const auto& path : resyncedPaths) {
+        if (path.IsPrimPath()) {
+            if (UsdUtil::primExists(path)) {
+                onPrimAdded(path);
+            } else {
+                onPrimRemoved(path);
+            }
+        }
+    }


Much cleaner. I like it.

lilleyse · 2023-03-08T19:05:34Z

I squeezed in one commit that fixes #62: 29825a7

weegeekps · 2023-03-08T20:00:04Z

I squeezed in one commit that fixes #62: 29825a7

That was a super simple fix.

weegeekps · 2023-03-08T20:50:50Z

src/core/src/UsdNotificationHandler.cpp

+        // This comes up when a tileset with imagery is moved or renamed.
+        const auto stage = UsdUtil::getUsdStage();
+        const auto prim = stage->GetPrimAtPath(primPath);
+        for (const auto& descendant : prim.GetAllDescendants()) {


I didn't think of this before, but do we have to use GetAllDescendants() here instead of something more performant like GetAllChildren()? Ultimately, anything that relies on subtree range iteration is going to be less performant than direct traversal, but that may be what we actually need here.

I think we need to use GetAllDescendants() here

Say the tree looks like

RandomXForm_1

RandomXForm_2

Tileset_1

Imagery_1

If RandomXForm_1 is renamed to MyXForm we need to handle all tilesets and imagery in the subtree.

Makes sense. Although I'm wondering what the difference is between GetAllChildren() and GetAllDescendants() then. Not something to worry about for this PR.

Though I just realized the scenario above won't work at all due to a bug. The type check shouldn't be surrounding the whole body.

Also, you're totally right, this should be GetAllChildren. At one point this function wasn't recursive and GetAllDescendants made more sense. I pushed a fix for that and my comment above.

weegeekps

Dismissing my approval pending a bug fix.

weegeekps

Good catch with that bug. 🚢

lilleyse requested review from weegeekps and r-veenstra as code owners March 8, 2023 00:16

lilleyse force-pushed the stage-reload branch from 413e77b to 9f0e285 Compare March 8, 2023 00:30

Check for prim added notifications

5ba046f

lilleyse force-pushed the stage-reload branch from 9f0e285 to 5ba046f Compare March 8, 2023 00:39

lilleyse commented Mar 8, 2023

View reviewed changes

lilleyse added 2 commits March 7, 2023 20:55

Remove unused function

551a41e

Check for url changes

29825a7

weegeekps reviewed Mar 8, 2023

View reviewed changes

lilleyse mentioned this pull request Mar 8, 2023

Support tilesets on the local filesystem #62

Closed

lilleyse force-pushed the stage-reload branch from c2f1320 to 29825a7 Compare March 8, 2023 20:19

Merge branch 'cleanup' into stage-reload

ac97da9

Base automatically changed from cleanup to main March 8, 2023 20:46

weegeekps reviewed Mar 8, 2023

View reviewed changes

weegeekps previously approved these changes Mar 8, 2023

View reviewed changes

weegeekps self-requested a review March 8, 2023 22:42

Fix onPrimAdded

8bc72ea

weegeekps approved these changes Mar 9, 2023

View reviewed changes

weegeekps merged commit 362a36a into main Mar 9, 2023

weegeekps deleted the stage-reload branch March 9, 2023 15:12

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support loading pre-existing USD files #205

Support loading pre-existing USD files #205

lilleyse commented Mar 8, 2023 •

edited

Loading

lilleyse Mar 8, 2023 •

edited

Loading

lilleyse Mar 8, 2023

weegeekps Mar 8, 2023

lilleyse Mar 8, 2023

lilleyse Mar 8, 2023

weegeekps Mar 8, 2023

weegeekps left a comment

weegeekps Mar 8, 2023

weegeekps Mar 8, 2023

weegeekps Mar 8, 2023

lilleyse commented Mar 8, 2023

weegeekps commented Mar 8, 2023

weegeekps Mar 8, 2023

lilleyse Mar 8, 2023

weegeekps Mar 8, 2023

lilleyse Mar 8, 2023

lilleyse Mar 8, 2023

weegeekps left a comment

weegeekps left a comment

		@@ -153,7 +154,6 @@ pxr::SdfPath Context::addTilesetUrl(const std::string& name, const std::string&

		tilesetUsd.GetUrlAttr().Set<std::string>(url);

		AssetRegistry::getInstance().addTileset(tilesetPath);

Support loading pre-existing USD files #205

Support loading pre-existing USD files #205

Conversation

lilleyse commented Mar 8, 2023 • edited Loading

lilleyse Mar 8, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

weegeekps left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lilleyse commented Mar 8, 2023

weegeekps commented Mar 8, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

weegeekps left a comment

Choose a reason for hiding this comment

weegeekps left a comment

Choose a reason for hiding this comment

lilleyse commented Mar 8, 2023 •

edited

Loading

lilleyse Mar 8, 2023 •

edited

Loading