MAYA-127700 - Extend Viewport and Outliner menus to allowing assigning new or existing materials #2896

stefanminning-autodesk · 2023-02-23T13:34:58Z

Description

This PR extends the Outliner and Viewport menus with new entries that allow the user to assign new or existing materials to objects.
The difficulty here is the jumping between the different languages: Going from MEL via Python into C++ doesn't strike me as the most elegant approach, but I wasn't sure how else to handle it, since the viewport menu itself lives in MEL.

Simple unit tests are included for the newly added Commands.

TODOs

Currently, the renderers and their materials (MaterialX, USD, Arnold) are hard-coded; this was largely ported from existing code in UsdContextOps.cpp. A future iteration should feature a more robust solution that queries the loaded renderers (including third-party!) for their supported materials.

…ing Material Final fixes

stefanminning-autodesk · 2023-02-23T13:36:06Z

pr-build

seando-adsk · 2023-02-23T14:21:29Z

@stefanminning-autodesk pr-build doesn't work for github. To run a preflight here set the assignee to ecp-maya-devops-adsk

JGamache-autodesk · 2023-02-23T16:22:48Z

test/lib/mayaUsd/utils/testMaterialCommands.py

+                             ]
+
+        materials = cmds.mayaUsdGetMaterialsFromRenderers()
+        self.assertCountEqual(materials, expectedMaterials)


That test will fail if third party renderers are loaded. Might I suggest checking for subset instead of count equality?

JGamache-autodesk · 2023-02-23T16:44:53Z

lib/mayaUsd/ufe/UsdContextOps.cpp

-        } else if (itemPath.size() == 2u && itemPath[1] == kAssignNewArnoldMaterialItem) {
-            items.emplace_back(
-                kAssignNewAIStandardSurfaceMaterialItem, kAssignNewAIStandardSurfaceMaterialLabel);
+            MString script;


You should call mayaUsdGetMaterialsFromRenderers() and use it to populate the context ops.
The call to get a context op interface is separate from the location where this results in showing menus. If you look at this unit test, calling getItems(["Add New Material"]) in a Python script will result in a menu unexpectedly showing up.

Thanks Jerry. To clarify so I understand correctly: You're saying I need to separate the filling of the items for the top level menus (renderers) from the individual submenus (per-renderer materials)?

So we would have a structure that mimics the original code in the form:

Suggested change

MString script;

} else if (itemPath.size() == 1u && (itemPath[0] == kAssignNewMaterialItem || itemPath[0] == kAddNewMaterialItem)) {

// Get just the renderers -- these will be shown in top level menus.

...

} else if (itemPath.size() == 2u) {

// Get the materials for renderer specified in itemPath[1], which will be shown in the submenu.

...

}

Is that correct?

I'm having a little trouble with this. To split the population of the ContextOps items, I've changed my Command to have to two separate steps:

Get the list of available renderers

Get the list of materials associated with a given renderer

This works fine, except for one detail: To replicate the way in which we previously filled the material list ContextOps, the UsdContextOps::getItems function needs to know which renderers are available. It's easy enough to get that list from my command, but I'm not sure where to store it... getItems is a const function, so I can't set the variable on the class after I first query for the list of renderers.

Obviously I could hard-code it for now, but that's hardly ideal.

Here's what I have currently:

Suggested change

MString script;

} else if (itemPath.size() == 1u

&& (itemPath[0] == kAssignNewMaterialItem || itemPath[0] == kAddNewMaterialItem)) {

MStringArray renderers; // <- Should store this on the class, but can't because the function is const...

MGlobal::executeCommand("mayaUsdGetMaterialsFromRenderers -lr true", renderers, false, false);

for (const auto& renderer : renderers) {

items.emplace_back(renderer.asChar(), renderer.asChar(), Ufe::ContextItem::kHasChildren);

}

} else if (itemPath.size() == 2u

&& (itemPath[1] == "Arnold" || itemPath[1] == "USD" || itemPath[1] == "MaterialX")) // <- Shouldn't be hard-coded!

{

MString script;

script.format("mayaUsdGetMaterialsFromRenderers \"^1s\"", itemPath[1].c_str());

MStringArray materials;

MGlobal::executeCommand(script, materials, false, false)

// Fill items with materials

...

}

Guess a pointer MStringArray* would work, but seems a bit hacky... any suggestions how to best handle this?

OK, so mutable MStringArray renderers works, but again feels hacky.

Ah, nevermind: Found a much simpler solution that will also work for the "Existing Material" case.

JGamache-autodesk · 2023-02-23T16:47:01Z

lib/mayaUsd/ufe/UsdContextOps.cpp

+                Ufe::PathString::string(fItem->path()).c_str());
+            MString result = MGlobal::executeCommandStringResult(script, false, false);
+        } else if (itemPath.size() == 1u && itemPath[0] == kAssignExistingMaterialItem) {
+            MString script;


Same thing here. You need to populate the menus on the fly as requested by calling mayaUsdGetMaterialsInScene() and find out from the path which menu/submenu entries to add.

JGamache-autodesk · 2023-02-23T17:02:51Z

plugin/adsk/plugin/adskMaterialCommands.cpp

+
+void ADSKMayaUSDGetMaterialsForRenderersCommand::appendUsdMaterials() const
+{
+    // TODO: Replace hard-coded materials with dynamically generated list.


No need for a TODO here. There will always be only one preview material.

JGamache-autodesk · 2023-02-23T17:09:51Z

plugin/adsk/plugin/adskMaterialCommands.cpp

+    UsdStagePtr stage = ufe::getStage(ufePath);
+    if (stage) {
+        for (auto prim : stage->Traverse()) {
+            if (prim.GetTypeName() == materialType) {


I would prefer you use the stronger if (UsdShadeMaterial(prim)) as filter.

JGamache-autodesk · 2023-02-23T17:11:22Z

plugin/adsk/plugin/adskMaterialCommands.h

+// GetMaterialXMaterialsCommand
+//------------------------------------------------------------------------------
+
+struct MxShaderMenuEntry


I don't think this struct is used. Can you recheck and see it it can be removed?

JGamache-autodesk · 2023-02-23T17:15:39Z

plugin/adsk/scripts/USDMenuProc.mel

+                                ufePath = ufe.PathString.path('" + $ufePath + "');\
+                                item = ufe.Hierarchy.createItem(ufePath);\
+                                contextOps = ufe.ContextOps.contextOps(item);\
+                                cmd = contextOps.doOpCmd(['Assign New Material', 'USD', '" + $material + "']);\


The 'USD' part could be misleading. Looks like the context ops are only interested in items 0 and 2 in that list. Maybe you should leave that middle string blank?

JGamache-autodesk · 2023-02-23T17:18:54Z

test/lib/mayaUsd/utils/testMaterialCommands.py

+        materialsInStage = cmds.mayaUsdGetMaterialsInScene(si="|stage|stageShape,/cube")
+        self.assertEqual(materialsInStage, expectedMaterials)
+
+


You could add a test for ContextOp::getItems() to check if the subtrees of add new/existing material are correctly populated.

plugin/adsk/plugin/adskMaterialCommands.h

seando-adsk · 2023-02-23T16:37:31Z

plugin/adsk/plugin/adskMaterialCommands.h

+//! renderers.
+//! \todo: The list of materials and renderers is currently hard-coded. We need to make
+//! it dynamic so that third-party renderers can hook in to provide their own materials.
+class MAYAUSD_CORE_PUBLIC ADSKMayaUSDGetMaterialsForRenderersCommand : public MPxCommand


The name of this command (and one below) doesn't really match the brief description. From the command name it it sounds like this returns a list. This command doesn't actually fill a menu - right?

seando-adsk · 2023-02-23T16:43:21Z

lib/mayaUsd/ufe/UsdContextOps.cpp

+            script.format(
+                "MayaUsdMenuAddNewMaterialsForRenderers \"^1s\"",
+                Ufe::PathString::string(fItem->path()).c_str());
+            MString result = MGlobal::executeCommandStringResult(script, false, false);


Why are you calling "xxxStringResult"? You don't use the return result. I think you can just call executeCommand(str). Both default args for this one are false.

executeCommandStringResult was what was used in the example I randomly chose to follow. But you're right, executeCommand is the correct one to use here.

seando-adsk · 2023-02-23T16:51:01Z

test/lib/mayaUsd/utils/testMaterialCommands.py

+import unittest
+
+
+class testMaterialCommands(unittest.TestCase):


Could you also modify the existing testContextOps.py to test your new menu items? Such as adding executing one of the menu items which adds a new material and then verify that new material?

seando-adsk · 2023-02-23T19:42:48Z

plugin/adsk/plugin/adskMaterialCommands.cpp

+MSyntax ADSKMayaUSDGetMaterialsInStageCommand::createSyntax()
+{
+    MSyntax syntax;
+    syntax.addFlag("-si", "-sceneItem", MSyntax::kString);


I don't think you need or should use a flag like this. Its not really a "sceneitem". It is actually a string which is interpreted as a Ufe PathString.

Instead look at calling addArg(MSyntax::kString) then simply get that string arg. There are examples of this in our code already. Plus you should probably set the syntax.enableQuery/enableEdit as well.

stefanminning-autodesk · 2023-03-07T13:43:38Z

Undo probably doesn't work. Currently looking into a fix as part of https://github.com/Autodesk/maya-usd/pull/2910.

JGamache-autodesk · 2023-03-07T14:03:15Z

lib/mayaUsd/ufe/UsdContextOps.cpp

+
+            for (const auto& material : materials) {
+                MStringArray pathAndMaterial;
+                MStatus      status = material.split('/', pathAndMaterial);


Why not a reverse find for the last "/" that will allow you to split the string in two at the right location?

Good point. Looks like MString::rindex will help there.

JGamache-autodesk · 2023-03-07T14:05:09Z

lib/mayaUsd/ufe/UsdContextOps.cpp

+            for (const auto& material : materials) {
+                MStringArray pathAndMaterial;
+                MStatus      status = material.split('/', pathAndMaterial);
+                // Expects a string in the format "/path1/path2|Material".


Is that comment correct? I don't see any splitting on the "|" in the below code.

Typo, thanks :)

JGamache-autodesk · 2023-03-07T16:32:13Z

plugin/adsk/scripts/USDMenuProc.mel

+                                item = ufe.Hierarchy.createItem(ufePath);\
+                                contextOps = ufe.ContextOps.contextOps(item);\
+                                cmd = contextOps.doOpCmd(['Assign New Material', '', '" + $material + "']);\
+                                cmd.execute();")`;


This is where you need to use Ufe::UndoableCommandMgr::instance().executeCmd(cmd);

Works perfectly, thank you!

JGamache-autodesk · 2023-03-07T16:32:29Z

plugin/adsk/scripts/USDMenuProc.mel

+                                item = ufe.Hierarchy.createItem(ufePath);\
+                                contextOps = ufe.ContextOps.contextOps(item);\
+                                cmd = contextOps.doOpCmd(['Assign Existing Material', '', '" + $material + "']);\
+                                cmd.execute();")`;


Same thing here.

…er not allowed"

…wport-menu

Viewport and Outliner menu entries: Assign New Material, Assign Exist…

3cd2c3a

…ing Material Final fixes

stefanminning-autodesk added the do-not-merge-yet Development is not finished, PR not ready for merge label Feb 23, 2023

stefanminning-autodesk requested review from seando-adsk and JGamache-autodesk February 23, 2023 13:34

stefanminning-autodesk assigned ecp-maya-devops-adsk Feb 23, 2023

Fix formatting

d936496

JGamache-autodesk assigned ecp-maya-devops-adsk and unassigned ecp-maya-devops-adsk Feb 23, 2023

JGamache-autodesk requested changes Feb 23, 2023

View reviewed changes

seando-adsk requested changes Feb 23, 2023

View reviewed changes

Changes for Code Review

73b76ad

stefanminning-autodesk assigned ecp-maya-devops-adsk and unassigned ecp-maya-devops-adsk Mar 7, 2023

stefanminning-autodesk requested review from JGamache-autodesk and seando-adsk March 7, 2023 13:25

stefanminning-autodesk assigned ecp-maya-devops-adsk and unassigned ecp-maya-devops-adsk Mar 7, 2023

JGamache-autodesk approved these changes Mar 7, 2023

View reviewed changes

JGamache-autodesk requested changes Mar 7, 2023

View reviewed changes

Fix undo, second level menus in Outliner

dd2ce21

stefanminning-autodesk requested a review from JGamache-autodesk March 8, 2023 08:26

stefanminning-autodesk assigned ecp-maya-devops-adsk and unassigned ecp-maya-devops-adsk Mar 8, 2023

Attempt to fix build error: "definition of dllimport static data memb…

63f3dcf

…er not allowed"

stefanminning-autodesk assigned ecp-maya-devops-adsk and unassigned ecp-maya-devops-adsk Mar 8, 2023

Formatting

ad5854c