Add contextual menu to create material nodes #2466

JGamache-autodesk · 2022-07-07T22:19:10Z

Requirements:

User right-clicks a prim and selects "Standard Surface" under "Assign New Material" menu
"mtl" scope is created at the root level of the stage in the outliner (if it wasn't there before)
New "standard_surface<#>" material is listed under this "materials" scope
This standard surface material is assigned to the selected prim
The material should be selected in the outliner and the tab should be displayed in the attribute editor

(prim creation also benefits from auto-selection)

nodes.

- Puts the material in the right location - Supports undo/redo - Added unit test

…l_node

JGamache-autodesk · 2022-07-07T22:21:11Z

lib/mayaUsd/ufe/UsdContextOps.cpp

@@ -500,148 +650,6 @@ class ClearAllReferencesUndoableCommand : public Ufe::UndoableCommand
 const std::string ClearAllReferencesUndoableCommand::commandName("Clear All References");
 const MString     ClearAllReferencesUndoableCommand::cancelRemoval("No");

-#if PXR_VERSION >= 2108
-class BindMaterialUndoableCommand : public Ufe::UndoableCommand


Moved to UsdUndoMaterialCommands.cpp

JGamache-autodesk · 2022-07-07T22:22:19Z

lib/mayaUsd/ufe/UsdContextOps.cpp

@@ -892,77 +900,90 @@ Ufe::ContextOps::Items UsdContextOps::getItems(const Ufe::ContextOps::ItemPath&

        // Top level item - Add New Prim (for all context op types).
        items.emplace_back(kUSDAddNewPrimItem, kUSDAddNewPrimLabel, Ufe::ContextItem::kHasChildren);
-
+        if (!fIsAGatewayType) {


Moved upwards yo keep the Materials menu in last position.

hodoulp

LGTM. Made some comments/questions.

lib/mayaUsd/ufe/UsdContextOps.cpp

lib/mayaUsd/ufe/UsdUndoMaterialCommands.cpp

Tested against Maya 2022

lib/mayaUsd/ufe/UsdContextOps.cpp

feldstj · 2022-07-08T14:00:24Z

lib/mayaUsd/ufe/UsdUndoMaterialCommands.cpp

+            return false;
+        }
+        materialOutput
+            = UsdShadeMaterial(materialPrim).CreateSurfaceOutput(shaderNodeDef->GetSourceType());


If I understand correctly this will add the appropriate surface output to the material node, but shouldn't you also be adding the displacement and volume outputs?

Why do that if there is nothing to be connected to those outputs?

I agree that the material node won't be connected to those outputs, but can't a user use the editor to connect shaders to displacement if you created it?

That would be the editor's responsibility to either automatically create the right ports, or offer options to create them when dealing with displacement shaders. The editor should not dictate what this command is doing.

But shouldn't all materials have surface, displacement, volume ports? I'm not following why you think this is a special case. It's not like in the future users will create displacement shaders as a different type, they will still just create a standard surface and connect shaders to the displacement port.

Displacement and volume attributes appear in your editor because UFE will allow browsing unauthored attributes. They do not exist in the UFE stage until you explicitly set their values or connect them.

The outputs for MaterialX context are another problem, because they are not in the default schema and escape UFE detection. But this is an issue for a later time, when we add knowledge on how to connect to a material port. I suspect the attempt to connect to outputs:displacement will be redirected to outputs:mtlx:displacement directly in the MayaUSD implementation of the connect command after looking at the return value of GetSourceType() on the shader we will be trying to connect to the material.

ppt-adsk

I'm a bit concerned about the additional complexity caused by attempting to recover from failed undo. I think that handling undo failure at this level is not buying us much, and we should let the DCC app (Maya) deal with it, which hopefully should be by clearing the undo stack.

ppt-adsk · 2022-07-08T15:04:27Z

lib/mayaUsd/ufe/UsdContextOps.cpp

+    }
+
+protected:
+    Ufe::UndoableCommand::Ptr _creationCmd;


Should be const, i.e.
const Ufe::UndoableCommand::Ptr

lib/mayaUsd/ufe/UsdUndoMaterialCommands.cpp

ppt-adsk · 2022-07-08T15:16:08Z

lib/mayaUsd/ufe/UsdUndoMaterialCommands.cpp

+static const std::string kDefaultMaterialScopeName("mtl");
+} // namespace
+
+void UsdUndoAssignNewMaterialCommand::execute()


Feels like this should be a Ufe::CompositeUndoableCommand.

I used has a because is a leads to a diamond shape inheritance with InsertChildCommand.

hodoulp · 2022-07-08T16:29:49Z

Managing the undo of a composite command could be challenging. But something must be done; otherwise, the composite undo() can try to undo a step which was never done i.e. failure in the undo composite step. A good approach is that the composite command only keeps a state of what was done.

For example, there is a composite command with three steps. Let say that the composite do() fails but only successfully did the first two steps (i.e. the third step failed). The composite undo() should not try to undo the third step; otherwise, it will also fail.

JGamache-autodesk · 2022-07-08T17:36:16Z

test/lib/mayaUsd/nodes/testProxyShapeBase.py

@@ -79,8 +79,10 @@ def testDuplicateProxyStageAnonymous(self):
        self.assertEqual(True, "-session" in stage.GetSessionLayer().identifier)
        self.assertEqual(True, "anonymousLayer1" in stage.GetRootLayer().identifier)

-        # add proxyShapeItem to selection list
-        ufe.GlobalSelection.get().append(proxyShapeItem)


Worked because Add Prim left the selection empty. No longer the case.

ppt-adsk · 2022-07-08T17:59:22Z

Managing the undo of a composite command could be challenging. But something must be done; otherwise, the composite undo() can try to undo a step which was never done i.e. failure in the undo composite step. A good approach is that the composite command only keeps a state of what was done.

For example, there is a composite command with three steps. Let say that the composite do() fails but only successfully did the first two steps (i.e. the third step failed). The composite undo() should not try to undo the third step; otherwise, it will also fail.

I would distinguish initial execution (execute()) from undo() and redo(). If initial execution fails, it should be dealt with outside of undo / redo, so that the undo stack is preserved. If a step in the composite undo fails, it should throw an exception, because the state of the undo stack is unknown and possibly corrupt, and the undo stack should be cleared.

hodoulp · 2022-07-08T18:55:38Z

If a step in the composite undo fails, it should throw an exception, because the state of the undo stack is unknown and possibly corrupt, and the undo stack should be cleared.

I don't think. The behaviour must be the same in both situation (single or composite commands). A user, the Maya undo queue, etc. must work exactly the same way without knowing if it's a single command or a composite command. That's the composite pattern -- or -- like a transaction (perhaps too much for now) i.e. all steps succeeds (or all steps are reverted).

seando-adsk · 2022-07-11T13:08:29Z

@JGamache-autodesk Did you discuss the changes (especially the selecting prim part) with Natalia?

JGamache-autodesk · 2022-07-11T13:40:35Z

@seando-adsk Yes, I checked this morning and Natalia is OK with auto-selecting prims.

feldstj and others added 4 commits July 5, 2022 10:13

LOOKDEVX-730 | Added new interface for creating materials/material

19efb65

nodes.

Merge branch 'dev' into feldstj/create_material_node

f1cb54d

Completed implementation

a2140f9

- Puts the material in the right location - Supports undo/redo - Added unit test

Merge remote-tracking branch 'github/dev' into feldstj/create_materia…

c8bf6ad

…l_node

JGamache-autodesk requested review from ppt-adsk, hodoulp and feldstj July 7, 2022 22:19

JGamache-autodesk assigned ecp-maya-devops-adsk Jul 7, 2022

JGamache-autodesk commented Jul 7, 2022

View reviewed changes

Fix builds on older Ufe versions

2e1cb2c

JGamache-autodesk assigned ecp-maya-devops-adsk and unassigned ecp-maya-devops-adsk Jul 8, 2022

hodoulp requested changes Jul 8, 2022

View reviewed changes

Properly check for UFE versions

3d2862a

Tested against Maya 2022

feldstj requested changes Jul 8, 2022

View reviewed changes

Rework from review comments.

5667d0b

JGamache-autodesk requested review from feldstj and hodoulp July 8, 2022 15:10

JGamache-autodesk assigned ecp-maya-devops-adsk and unassigned ecp-maya-devops-adsk Jul 8, 2022

ppt-adsk reviewed Jul 8, 2022

View reviewed changes

feldstj previously approved these changes Jul 8, 2022

View reviewed changes

Use CompositeCmd everywhere

e82bc40

JGamache-autodesk dismissed feldstj’s stale review via e82bc40 July 8, 2022 17:28

JGamache-autodesk assigned ecp-maya-devops-adsk and unassigned ecp-maya-devops-adsk Jul 8, 2022

JGamache-autodesk commented Jul 8, 2022

View reviewed changes

ppt-adsk approved these changes Jul 8, 2022

View reviewed changes

feldstj approved these changes Jul 8, 2022

View reviewed changes

JGamache-autodesk added the ready-for-merge Development process is finished, PR is ready for merge label Jul 8, 2022

seando-adsk added do-not-merge-yet Development is not finished, PR not ready for merge and removed ready-for-merge Development process is finished, PR is ready for merge labels Jul 11, 2022

seando-adsk added ufe-usd Related to UFE-USD plugin in Maya-Usd ready-for-merge Development process is finished, PR is ready for merge and removed do-not-merge-yet Development is not finished, PR not ready for merge labels Jul 11, 2022

seando-adsk merged commit 50e2fd3 into dev Jul 11, 2022

seando-adsk deleted the feldstj/create_material_node branch July 11, 2022 14:04

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add contextual menu to create material nodes #2466

Add contextual menu to create material nodes #2466

JGamache-autodesk commented Jul 7, 2022

JGamache-autodesk Jul 7, 2022

JGamache-autodesk Jul 7, 2022

hodoulp left a comment

feldstj Jul 8, 2022

JGamache-autodesk Jul 8, 2022

feldstj Jul 8, 2022

JGamache-autodesk Jul 8, 2022

feldstj Jul 8, 2022

JGamache-autodesk Jul 8, 2022

ppt-adsk left a comment

ppt-adsk Jul 8, 2022

ppt-adsk Jul 8, 2022

JGamache-autodesk Jul 8, 2022

hodoulp commented Jul 8, 2022 •

edited

Loading

JGamache-autodesk Jul 8, 2022

ppt-adsk commented Jul 8, 2022

hodoulp commented Jul 8, 2022

seando-adsk commented Jul 11, 2022

JGamache-autodesk commented Jul 11, 2022

Add contextual menu to create material nodes #2466

Add contextual menu to create material nodes #2466

Conversation

JGamache-autodesk commented Jul 7, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hodoulp 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ppt-adsk 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

hodoulp commented Jul 8, 2022 • edited Loading

Choose a reason for hiding this comment

ppt-adsk commented Jul 8, 2022

hodoulp commented Jul 8, 2022

seando-adsk commented Jul 11, 2022

JGamache-autodesk commented Jul 11, 2022

hodoulp commented Jul 8, 2022 •

edited

Loading