MAYA-104214 - As a user, I'd like to add a USD prim to an existing stage #489

seando-adsk · 2020-05-08T17:33:31Z

MAYA-104214 - As a user, I'd like to add a USD prim to an existing stage

Added new AE templates for basic prim types.
New ProxyShapeContextOpsHandler to decorate the one from Maya.
Send new Ufe::ObjectDestroyed notif when USD prim is deleted.
New context item sub-menu: "Add New Prim" with various prim types. Full undo/redo support thur new command. Added new context ops test "testAddNewPrim".
Fixed a bug where "visbility" context menu item was added to prims that don't support visibility attribute.

seando-adsk · 2020-05-08T17:34:34Z

lib/mayaUsd/ae/CMakeLists.txt

@@ -8,7 +8,7 @@ if (CMAKE_UFE_V2_FEATURES_AVAILABLE)
    install(FILES __init__.py DESTINATION ${CMAKE_INSTALL_PREFIX}/lib/python/ufe_ae/usd/nodes)

    # Maya Attribute Editor python template files
-    foreach(_SUBDIR base camera material mesh scope shader xform)
+    foreach(_SUBDIR base camera capsule cone cube cylinder material mesh scope shader sphere xform)


Created AE templates for the missing prim types that are part of the menu. They are all just copies of the existing templates that have everything in the "AEBaseTemplate"

seando-adsk · 2020-05-08T17:35:39Z

lib/mayaUsd/ufe/Global.cpp

+	g_MayaContextOpsHandler = Ufe::RunTimeMgr::instance().contextOpsHandler(g_MayaRtid);
+	auto proxyShapeContextOpsHandler = ProxyShapeContextOpsHandler::create(g_MayaContextOpsHandler);
+	Ufe::RunTimeMgr::instance().setContextOpsHandler(g_MayaRtid, proxyShapeContextOpsHandler);


Decorate the Maya context ops handler, just like we do with the Maya hier handler. Note: currently the Maya context ops handler is not used so it's null.

seando-adsk · 2020-05-08T17:37:12Z

lib/mayaUsd/ufe/ProxyShapeContextOpsHandler.cpp

+			auto usdItem = UsdSceneItem::create(item->path(), stage->GetPseudoRoot());
+			auto usdContextOpsHandler = UsdContextOpsHandler::create();
+			auto cOps = usdContextOpsHandler->contextOps(usdItem);
+			UsdContextOps::Ptr usdCOps = std::dynamic_pointer_cast<UsdContextOps>(cOps);


From Maya the input Ufe::SceneItem will be of an internal type to Maya. We need to create a UsdSceneItem (that has a prim).

seando-adsk · 2020-05-08T17:38:16Z

lib/mayaUsd/ufe/StagesSubject.cpp

+#if UFE_PREVIEW_VERSION_NUM >= 2015
+		else if (!prim.IsValid())
+		{
+			auto notification = Ufe::ObjectDestroyed(ufePath);
+			Ufe::Scene::notifyObjectDelete(notification);
+		}
+#endif


New scene notification added in Ufe. When this message is sent the object is no longer valid. Thus we don't use the scene item, but instead just the path.

seando-adsk · 2020-05-08T17:39:16Z

lib/mayaUsd/ufe/UsdContextOps.cpp

+    void redo() override
+    {
+        if (_stage) {
+            auto newPrim = _stage->DefinePrim(_primPath, _primToken);


Create a prim with the specified path and of the specified type.

seando-adsk · 2020-05-08T17:40:39Z

lib/mayaUsd/ufe/UsdContextOps.cpp

-                    std::string("Make Visible") : std::string("Make Invisible");
-                items.emplace_back("Toggle Visibility", l);
+        // Top-level items.  Variant sets and visibility. Do not add for gateway type node.
+        if (!fIsAGatewayType) {


Lines 159-174 were indented inside this new if test. The gateway node will be the mayausd proxy shape.

As I discussed with you, I'm still trying to figure out how we could have done this with a base class, and avoided the type-based conditional...

If we had a base class for the context ops that did not have variant sets and visibility, and a derived class that would have variant sets and visibility first, then would call the base class, would that work? Ordering would be preserved.

Maybe I'm too attached to that idea, and maybe it's just a minor improvement, but my feeling is that it would help with separation of concerns. The base class would take care of the proxy shape-only context ops, and the derived class would add in visibility and variant sets. Let me know what you think.

It would work for now only because in our ordering we have the two non-gateway node stuff first and then a single item that applies to all. But as we add more context menu items this won't work. Example: my next task is to add menu item for "Layer Editor" as the first menu item. It will be added for all types. Once we add that I think the complexity of using a base/derived class outweighs the benefit.

I agree with you, the base class approach isn't flexible enough to accomodate the ordering we'll need. Case closed.

seando-adsk · 2020-05-08T17:41:56Z

lib/mayaUsd/ufe/UsdContextOps.cpp

+                    "Variant Sets", "Variant Sets", Ufe::ContextItem::kHasChildren);
+            }
+            auto attributes = Ufe::Attributes::attributes(sceneItem());
+            if (attributes && attributes->hasAttribute(UsdGeomTokens->visibility)) {


Added the hasAttribute() test here to fix a bug with prims that don't have a visibility attribute to toggle.

Thanks. For my own education, which ones don't? The typeless defs?

Typeless = yes, but also shaders, materials

Ah, of course. Can you add that in as a code comment, to avoid further questions?

seando-adsk · 2020-05-08T17:42:49Z

lib/mayaUsd/ufe/UsdContextOps.cpp

+            if (itemPath[1] == "Def")
+                _primToken = TfToken();     // create typeless prim
+            else
+                _primToken = TfToken(itemPath[1]);


Type of prim to create. Def is typeless.

* Added new AE templates for basic prim types. * New ProxyShapeContextOpsHandler to decorate the one from Maya. * Send new Ufe::ObjectDestroyed notif when USD prim is deleted. * New context item sub-menu: "Add New Prim" with various prim types. Full undo/redo support thur new command. Added new context ops test "testAddNewPrim". * Fixed a bug where "visbility" context menu item was added to prims that don't support visibility attribute.

ppt-adsk · 2020-05-08T19:29:24Z

lib/mayaUsd/ufe/ProxyShapeContextOpsHandler.h

+public:
+	typedef std::shared_ptr<ProxyShapeContextOpsHandler> Ptr;
+
+	ProxyShapeContextOpsHandler(Ufe::ContextOpsHandler::Ptr mayaContextOpsHandler);


Nit-pick: should pass in the shared_ptr by const ref.

Agreed. I had originally copied this from the ProxyShapeHierarchyHandler. It wasn't const ref either so I changed it.

ppt-adsk · 2020-05-08T19:45:09Z

lib/mayaUsd/ufe/ProxyShapeContextOpsHandler.h

+	ProxyShapeContextOpsHandler& operator=(ProxyShapeContextOpsHandler&&) = delete;
+
+	//! Create a ProxyShapeContextOpsHandler from a UFE hierarchy handler.
+	static ProxyShapeContextOpsHandler::Ptr create(Ufe::ContextOpsHandler::Ptr mayaContextOpsHandler);


Pass by const ref.

Agreed - same as above.

ppt-adsk · 2020-05-08T20:05:38Z

lib/mayaUsd/ufe/UsdContextOps.cpp

+        _stage = MayaUsd::ufe::getStage(Ufe::Path(dagSegment));
+        if (_stage) {
+            // Rename the new prim for uniqueness, if needed.
+            Ufe::Path newUfePath = ufePath + itemPath[1];


You've tried this, so clearly it works, but I'm just not sure how :) In the parenting code, when I parent just below the proxy shape, I needed to take care of the fact that the newly-added component is in a different run-time:
https://github.com/Autodesk/maya-usd/blob/dev/lib/mayaUsd/ufe/UsdUndoInsertChildCommand.cpp#L53
Why don't you have the same problem? If you just concatenate the prim name to the existing proxy shape UFE path, you'll get a path that is in the Maya run-time, which is incorrect.

You are right, I didn't think about this. But I just tested it again and it works. The reason is that in "uniqueChildName" it just does:

std::string childName = childPath.back().string();

which in my case is fine, because it will just return the "itemPath[1]" that I appended. Then it loops thru all the existing child names to see if any match.

Do you think it is okay like this? Alternatively I could pass in (to the command) if I have a gateway node. When I do, I would know that the new prim name I need to append will be in a different runtime and I could handle that in a manner similar to what you did.

I think it's O.K. like this, but I'd add in a comment somehow to say something like "Hey I know this isn't 100% correct, but all uniqueName does is take the back() of the path, so it's fine."

Done. I also added another code comment for the visibility.

ppt-adsk · 2020-05-08T20:08:18Z

lib/mayaUsd/ufe/UsdContextOps.cpp

+{
+public:
+    AddNewPrimUndoableCommand(const MayaUsd::ufe::UsdSceneItem::Ptr& usdSceneItem,
+                              const Ufe::ContextOps::ItemPath& itemPath)


Looks like you're only using itemPath[1]. If that's the case, couldn't we just pass that in, and give it a more readable name, i.e. const std::string& primType, or something similar?

Yes I am, but this way it more matches the other undoable command which also takes in the itemPath.

ppt-adsk · 2020-05-08T20:09:45Z

lib/mayaUsd/ufe/UsdContextOps.cpp

+
+            // The type of prim we were asked to create.
+            if (itemPath[1] == "Def")
+                _primToken = TfToken();     // create typeless prim


Nit-pick: could be in the constructor's initializer list as

_primToken(itemPath[1] == "Def" ? TfToken() : TfToken(itemPath[1]))

I'll use the single line version, but I prefer to keep it here rather that in init list because I have some comments.

ppt-adsk · 2020-05-08T20:14:31Z

lib/mayaUsd/ufe/UsdContextOps.cpp

-                    std::string("Make Visible") : std::string("Make Invisible");
-                items.emplace_back("Toggle Visibility", l);
+        // Top-level items.  Variant sets and visibility. Do not add for gateway type node.
+        if (!fIsAGatewayType) {


As I discussed with you, I'm still trying to figure out how we could have done this with a base class, and avoided the type-based conditional...

If we had a base class for the context ops that did not have variant sets and visibility, and a derived class that would have variant sets and visibility first, then would call the base class, would that work? Ordering would be preserved.

Maybe I'm too attached to that idea, and maybe it's just a minor improvement, but my feeling is that it would help with separation of concerns. The base class would take care of the proxy shape-only context ops, and the derived class would add in visibility and variant sets. Let me know what you think.

ppt-adsk · 2020-05-08T20:15:11Z

lib/mayaUsd/ufe/UsdContextOps.cpp

+                    "Variant Sets", "Variant Sets", Ufe::ContextItem::kHasChildren);
+            }
+            auto attributes = Ufe::Attributes::attributes(sceneItem());
+            if (attributes && attributes->hasAttribute(UsdGeomTokens->visibility)) {


Thanks. For my own education, which ones don't? The typeless defs?

ppt-adsk · 2020-05-08T20:16:22Z

lib/mayaUsd/ufe/UsdContextOps.cpp

+
+        // At this point we know we have 2 arguments to execute the operation.
+        // itemPath[1] contains the new prim type to create.
+        return std::make_shared<AddNewPrimUndoableCommand>(fItem, itemPath);


As per above I'd just pass in itemPath[1] 'cause that's all you need.

test/lib/ufe/testContextOps.py

* Code review comments.

* Added a couple of code comments.

ppt-adsk · 2020-05-11T15:52:07Z

lib/mayaUsd/ufe/UsdContextOps.cpp

+        _stage = MayaUsd::ufe::getStage(Ufe::Path(dagSegment));
+        if (_stage) {
+            // Rename the new prim for uniqueness, if needed.
+            Ufe::Path newUfePath = ufePath + itemPath[1];


* Fix problem with new Ufe::ObjectDestroyed() and invalid prim.

seando-adsk · 2020-05-11T20:17:03Z

I discovered a problem while doing some more local testing. We want to ignore resycn paths that prim paths. See commit 17c9a05

seando-adsk · 2020-05-11T20:18:31Z

lib/mayaUsd/ufe/StagesSubject.cpp

+		if (!changedPath.IsPrimPath())
+			continue;


At line 221 we specifically ask for the prim at path. So if this changed path is not a prim path we will get an invalid prim back.

pilarmolinalopez · 2020-05-11T20:48:14Z

lib/mayaUsd/ufe/UsdContextOps.cpp

@@ -184,6 +260,17 @@ Ufe::UndoableCommand::Ptr UsdContextOps::doOpCmd(const ItemPath& itemPath)
        return visibility->setCmd(
            current == UsdGeomTokens->invisible ? UsdGeomTokens->inherited : UsdGeomTokens->invisible);
    } // Visibility
+    else if (!itemPath.empty() && (itemPath[0] == "Add New Prim")) {


Super minor comment: it may be a good idea to keep these operation strings (like "Add New Prim") in a unique place. That way it'd be easier to change if we ever need to. I've seen the "Add New Prim" string already three times during this review.

Yes good idea. See commit 14d8d4b

pilarmolinalopez

Except for a minor comment, it looks good to me. Thanks!

* Code review comment - use string constants.

…outliner * Added context menu item for opening USD Layer Editor.

seando-adsk · 2020-05-12T15:53:53Z

Since this all deals with context menu changes, I've also added one more small change in this commit for opening the USD Layer Editor.

ppt-adsk · 2020-05-12T16:06:56Z

lib/mayaUsd/ufe/UsdContextOps.cpp

+// Ufe::ContextItem strings
+// - the "Item" describe the operation to be performed.
+// - the "Label" is used in the context menu (can be localized).
+static constexpr char kUSDLayerEditorItem[] = "USD Layer Editor";


Nit-pick: is this some MSVC-specific requirement? Because otherwise, you're in an unnamed namespace, you don't need the static linkage specifier.

…ting_stage

pilarmolinalopez

thanks! looks good to me

* Added missing PYTHONPATH for test.

* testAddNewPrim requires changes not in Maya yet.

seando-adsk commented May 8, 2020

View reviewed changes

seando-adsk force-pushed the donnels/MAYA-104214/add_a_usd_prim_to_an_existing_stage branch from cb4eddb to ffc9e5c Compare May 8, 2020 17:48

seando-adsk requested review from ppt-adsk and pilarmolinalopez May 8, 2020 17:49

ppt-adsk reviewed May 8, 2020

View reviewed changes

kxl-adsk added ufe-usd Related to UFE-USD plugin in Maya-Usd workflows Related to in-context workflows labels May 10, 2020

seando-adsk added 2 commits May 11, 2020 10:05

MAYA-104214 - As a user, I'd like to add a USD prim to an existing stage

8d3153f

* Code review comments.

MAYA-104214 - As a user, I'd like to add a USD prim to an existing stage

5557632

* Added a couple of code comments.

ppt-adsk approved these changes May 11, 2020

View reviewed changes

MAYA-104214 - As a user, I'd like to add a USD prim to an existing stage

17c9a05

* Fix problem with new Ufe::ObjectDestroyed() and invalid prim.

seando-adsk commented May 11, 2020

View reviewed changes

ppt-adsk approved these changes May 11, 2020

View reviewed changes

pilarmolinalopez reviewed May 11, 2020

View reviewed changes

pilarmolinalopez approved these changes May 11, 2020

View reviewed changes

seando-adsk added 2 commits May 12, 2020 11:47

MAYA-104214 - As a user, I'd like to add a USD prim to an existing stage

14d8d4b

* Code review comment - use string constants.

MAYA-104397 - Users can launch the Layer Editor by right-clicking in …

2045683

…outliner * Added context menu item for opening USD Layer Editor.

ppt-adsk reviewed May 12, 2020

View reviewed changes

Merge branch 'dev' into donnels/MAYA-104214/add_a_usd_prim_to_an_exis…

6078142

…ting_stage

pilarmolinalopez approved these changes May 12, 2020

View reviewed changes

seando-adsk added 2 commits May 12, 2020 14:29

MAYA-104214 - As a user, I'd like to add a USD prim to an existing stage

bf97826

* Added missing PYTHONPATH for test.

MAYA-104214 - As a user, I'd like to add a USD prim to an existing stage

7918a30

* testAddNewPrim requires changes not in Maya yet.

seando-adsk force-pushed the donnels/MAYA-104214/add_a_usd_prim_to_an_existing_stage branch from 5744a52 to 7918a30 Compare May 12, 2020 21:19

ppt-adsk approved these changes May 13, 2020

View reviewed changes

kxl-adsk merged commit dac8880 into dev May 13, 2020

kxl-adsk deleted the donnels/MAYA-104214/add_a_usd_prim_to_an_existing_stage branch May 13, 2020 01:11

MAYA-104214 - As a user, I'd like to add a USD prim to an existing stage #489

MAYA-104214 - As a user, I'd like to add a USD prim to an existing stage #489

Conversation

seando-adsk commented May 8, 2020

MAYA-104214 - As a user, I'd like to add a USD prim to an existing stage

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

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

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

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

seando-adsk commented May 11, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pilarmolinalopez left a comment

Choose a reason for hiding this comment

seando-adsk commented May 12, 2020

Choose a reason for hiding this comment

pilarmolinalopez left a comment

Choose a reason for hiding this comment