Assign new material to multiple objects #2641

stefanminning-autodesk · 2022-10-05T11:47:05Z

This PR adds functionality to the existing UsdUndoAssignNewMaterialCommand, allowing for a new material to be assigned to multiple objects.

The command's original functionality remains untouched, as I wanted to avoid breaking API changes.

seando-adsk · 2022-10-05T11:56:00Z

@stefanminning-autodesk Before we start the code review and preflight process can you please clean this up by squashing the 11 commits?

stefanminning-autodesk · 2022-10-05T13:12:14Z

Sure, I've pushed with squashed history.

seando-adsk · 2022-10-06T17:31:11Z

@womanyoyoyo-adsk Natalia can you please have a look at this from a design perspective?
@stefanminning-autodesk From the code changes it looks to me as if you are always applying the material the selection, regardless of the item the context menu was opened on. Normally the behavior would be:
Q: "is the item on which the context menu was opened in the selection or not"?
Yes - apply operation to selection
No - apply operation only to context item

stefanminning-autodesk · 2022-10-07T07:21:47Z

Right, I've coded it to match the current behaviour, where a new material would be assigned to an object when the right-click operation is applied upon it, regardless of its selection state. Additional selected objects will then also receive the new material.

But what you're describing also seems like a perfectly valid option. I'll leave it to the UX team to decide what the best workflow is. I've asked in our team for clarification. Should be an easy change, either way.

seando-adsk · 2022-10-07T13:02:27Z

@stefanminning-autodesk Actually sorry I don't think that is correct. With your changes it applies the material to all the selected objects. If the object you right click on is not selected, then the material is not applied to it. The normal context menu behavior would be "if the context object is selected?" then apply to selection, otherwise apply only to context object.

stefanminning-autodesk · 2022-10-07T13:28:06Z

Ah yes, you're right: Looking at the code again, it should not be ignoring fItem. Could have sworn I'd accounted for that at some point... must have slipped through the cracks.

Anyway, guess I'll have to have another look at this when the designers are back from holidays and they've clarified the exact behaviour they'd like to see.

womanyoyoyo-adsk · 2022-10-13T17:05:14Z

Hi, myself and Lookdevx designer are looking into this. Keep you posted.

stefanminning-autodesk · 2022-10-18T10:00:44Z

As discussed with the UX team, the new material will be assigned to all of the following:

The selected object(s)
The object which received the right-click (regardless of its selection state)

The designers would also like to see additional UI feedback for the right-clicked item, but that will come as part of another task.

hodoulp

LGTM

lib/mayaUsd/ufe/UsdUndoMaterialCommands.h

hodoulp · 2022-10-19T21:38:24Z

lib/mayaUsd/ufe/UsdUndoMaterialCommands.h

@@ -105,6 +111,11 @@ class MAYAUSD_CORE_PUBLIC UsdUndoAssignNewMaterialCommand : public Ufe::InsertCh
    //! sdrShaderIdentifier and assigns it to \p parentItem
    static UsdUndoAssignNewMaterialCommand::Ptr
    create(const UsdSceneItem::Ptr& parentItem, const std::string& sdrShaderIdentifier);
+    //! Create a UsdUndoAssignNewMaterialCommand that creates a new material based on \p


Not clear about Doxygen but I would have move \p close to the parameter name sdrShaderIdentifier

hodoulp · 2022-10-19T21:40:39Z

lib/mayaUsd/ufe/UsdUndoMaterialCommands.cpp

+    _stagesAndPaths[stage].push_back(parentPath);
+}
+
+UsdUndoAssignNewMaterialCommand::UsdUndoAssignNewMaterialCommand(


Does that code need UFE_PREVIEW_XX defines protection?

Good question, honestly not sure.

These changes are entirely contained in MayaUSD, with no UFE update required. What UFE version would I be looking for?

hodoulp · 2022-10-19T21:41:23Z

lib/mayaUsd/ufe/UsdUndoMaterialCommands.h

+    //! sdrShaderIdentifier and assigns it to multiple \p parentItems
+    static UsdUndoAssignNewMaterialCommand::Ptr 
+    create(const std::vector<const UsdSceneItem::Ptr>& parentItems,
+           const std::string&                          sdrShaderIdentifier);


Should the new method be protected by UFE_PREVIEW_XX defines?

hodoulp · 2022-10-19T21:41:38Z

lib/mayaUsd/ufe/UsdUndoMaterialCommands.h

@@ -93,6 +96,9 @@ class MAYAUSD_CORE_PUBLIC UsdUndoAssignNewMaterialCommand : public Ufe::InsertCh
    UsdUndoAssignNewMaterialCommand(
        const UsdSceneItem::Ptr& parentItem,
        const std::string&       sdrShaderIdentifier);
+    UsdUndoAssignNewMaterialCommand(
+        const std::vector<const UsdSceneItem::Ptr>& parentItems,
+        const std::string&                          sdrShaderIdentifier);


Should the new method be protected by UFE_PREVIEW_XX defines?

lib/mayaUsd/ufe/UsdContextOps.cpp

seando-adsk · 2022-11-01T17:42:51Z

lib/mayaUsd/ufe/UsdUndoMaterialCommands.h

@@ -116,8 +126,8 @@ class MAYAUSD_CORE_PUBLIC UsdUndoAssignNewMaterialCommand : public Ufe::InsertCh
    void connectShaderToMaterial(Ufe::SceneItem::Ptr shaderItem, PXR_NS::UsdPrim materialPrim);
    void markAsFailed();

-    const Ufe::Path   _parentPath;
-    const std::string _nodeId;
+    std::map<pxr::UsdStageWeakPtr, std::vector<const Ufe::Path>> _stagesAndPaths;


Sorry I didn't catch this earlier. We shouldn't use the namespace name directly, but instead the define. So "pxr::" should be "PXR_NS::" (see line 126).

Once this and the merge conflict is fixed, you can run another preflight and if it passes add the label "ready for merge" and I'll merge it. I confirmed with our designer that the multiple selection workflow is okay.

stefanminning-autodesk · 2022-11-02T11:57:21Z

Please ignore for now, I botched the merge apparently.

stefanminning-autodesk · 2022-11-02T12:37:54Z

Alright, merge issue should be fixed.

seando-adsk · 2022-11-03T13:04:49Z

@stefanminning-autodesk There are clang-format errors that need to be fixed. Please see codingGuidelines for info on clang-format.

stefanminning-autodesk · 2022-11-03T13:24:00Z

@seando-adsk Sorry about that. I've now let clang-format do its thing and it fixed two issues. Fingers crossed!

stefanminning-autodesk · 2022-11-11T09:40:00Z

@seando-adsk @hodoulp I have added a unit test to test the multiple material assignment test case. Please have a look.

seando-adsk · 2022-11-11T14:43:31Z

test/lib/ufe/testContextOps.py

+        os.putenv("MAYAUSD_MATERIALS_SCOPE_NAME", "test_scope")
+        ufeCmd.execute(cmdSS)
+        checkMaterial(self, rootHier, 5, 1, 1, 0, "standard_surface", "mtlx", "out", "/test_scope")
+
+        # Clear the envvar, otherwise we're affecting the results of following tests.
+        os.putenv("MAYAUSD_MATERIALS_SCOPE_NAME", "")


I think we should have a way to make sure this is automatically restored using a context manager object. Look at TemporaryDirectory. You can create one that will restore an env var. The input would be the env var and the new value. The init will get the current value, store it and set the env var to input and the exit will restore to stored value. Then you can wrap this section of code in a with ... statement.

Good idea, will do.

seando-adsk

Thanks for the test. If you could just create a new context object and use it the two places that env var is set/restored that would be great. The new context object can be in testUtils.py

…s less error prone

seando-adsk · 2022-11-14T14:34:39Z

test/testUtils/testUtils.py

+    Context manager that creates a temporary environment variable and deletes it on exit,
+    so it's usable with "with" statement.


Rather than deleting it upon exit, you should save the current env var value (if any) in the init and restore it in the exit (if one was saved).

Alright, I've changed it to restore the original value.

seando-adsk

Everything looks good now. Thanks.

seando-adsk · 2022-11-16T18:50:07Z

@stefanminning-autodesk FYI, any new commit (such as your latest one merging in dev) will invalidate the preflight check and it must be run again. So I've removed the ready for merge label and re-started the preflight for you. When it passes, you may add back the label and I'll merge.

stefanminning-autodesk · 2022-11-17T09:08:28Z

Thanks @seando-adsk. The preflight succeeded, but then it was complaining about a tiny merge issues which I resolved right here in Github. I've kicked off another preflight just to be sure.

stefanminning-autodesk requested review from hodoulp and pierrebai-adsk October 5, 2022 11:47

stefanminning-autodesk force-pushed the minnins/assign-new-material-to-multiple-objects branch from cadd76e to 8dd50e8 Compare October 5, 2022 13:11

seando-adsk added the ufe-usd Related to UFE-USD plugin in Maya-Usd label Oct 5, 2022

stefanminning-autodesk force-pushed the minnins/assign-new-material-to-multiple-objects branch 2 times, most recently from a90833f to 66dd552 Compare October 18, 2022 09:56

hodoulp reviewed Oct 19, 2022

View reviewed changes

seando-adsk reviewed Oct 26, 2022

View reviewed changes

lib/mayaUsd/ufe/UsdContextOps.cpp Outdated Show resolved Hide resolved

seando-adsk reviewed Nov 1, 2022

View reviewed changes

stefanminning-autodesk added the do-not-merge-yet Development is not finished, PR not ready for merge label Nov 2, 2022

hodoulp changed the title ~~Assign new material to multiple objects~~ WIP - Assign new material to multiple objects Nov 2, 2022

stefanminning-autodesk force-pushed the minnins/assign-new-material-to-multiple-objects branch from 3619924 to 656a49c Compare November 2, 2022 12:34

stefanminning-autodesk removed the do-not-merge-yet Development is not finished, PR not ready for merge label Nov 2, 2022

seando-adsk previously approved these changes Nov 3, 2022

View reviewed changes

seando-adsk assigned ecp-maya-devops-adsk Nov 3, 2022

stefanminning-autodesk dismissed seando-adsk’s stale review via 5e6e4b1 November 3, 2022 13:22

seando-adsk previously approved these changes Nov 3, 2022

View reviewed changes

seando-adsk unassigned ecp-maya-devops-adsk Nov 3, 2022

Add unit test for multiple material assignment

1b03a5d

stefanminning-autodesk force-pushed the minnins/assign-new-material-to-multiple-objects branch from b7124b4 to 1b03a5d Compare November 11, 2022 09:35

stefanminning-autodesk requested review from seando-adsk and hodoulp November 11, 2022 09:39

seando-adsk reviewed Nov 11, 2022

View reviewed changes

seando-adsk requested changes Nov 11, 2022

View reviewed changes

Add TemporaryEnvironmentVariable utility to make working with env var…

f8389c5

…s less error prone

seando-adsk reviewed Nov 14, 2022

View reviewed changes

Restore original environment variable value, if any

dbb7315

seando-adsk previously approved these changes Nov 15, 2022

View reviewed changes

seando-adsk assigned ecp-maya-devops-adsk and unassigned ecp-maya-devops-adsk Nov 15, 2022

Merge branch 'dev' into minnins/assign-new-material-to-multiple-objects

d50fa23

stefanminning-autodesk added the ready-for-merge Development process is finished, PR is ready for merge label Nov 16, 2022

seando-adsk assigned ecp-maya-devops-adsk and unassigned ecp-maya-devops-adsk Nov 16, 2022

seando-adsk removed the ready-for-merge Development process is finished, PR is ready for merge label Nov 16, 2022

Merge branch 'dev' into minnins/assign-new-material-to-multiple-objects

7f3ddcd

stefanminning-autodesk dismissed seando-adsk’s stale review via 7f3ddcd November 17, 2022 09:06

stefanminning-autodesk assigned ecp-maya-devops-adsk and unassigned ecp-maya-devops-adsk Nov 17, 2022

seando-adsk self-requested a review November 21, 2022 14:58

seando-adsk approved these changes Nov 21, 2022

View reviewed changes

stefanminning-autodesk added the ready-for-merge Development process is finished, PR is ready for merge label Nov 21, 2022

seando-adsk merged commit 346fdd9 into dev Nov 21, 2022

seando-adsk deleted the minnins/assign-new-material-to-multiple-objects branch November 21, 2022 15:18

JGamache-autodesk mentioned this pull request Dec 13, 2022

Outliner rework of Material items: #2775

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Assign new material to multiple objects #2641

Assign new material to multiple objects #2641

stefanminning-autodesk commented Oct 5, 2022

seando-adsk commented Oct 5, 2022

stefanminning-autodesk commented Oct 5, 2022

seando-adsk commented Oct 6, 2022

stefanminning-autodesk commented Oct 7, 2022 •

edited

Loading

seando-adsk commented Oct 7, 2022

stefanminning-autodesk commented Oct 7, 2022

womanyoyoyo-adsk commented Oct 13, 2022

stefanminning-autodesk commented Oct 18, 2022

hodoulp left a comment

hodoulp Oct 19, 2022

hodoulp Oct 19, 2022

stefanminning-autodesk Oct 21, 2022

hodoulp Oct 19, 2022

hodoulp Oct 19, 2022

seando-adsk Nov 1, 2022

seando-adsk Nov 1, 2022

stefanminning-autodesk commented Nov 2, 2022

stefanminning-autodesk commented Nov 2, 2022

seando-adsk commented Nov 3, 2022

stefanminning-autodesk commented Nov 3, 2022

stefanminning-autodesk commented Nov 11, 2022

seando-adsk Nov 11, 2022

stefanminning-autodesk Nov 14, 2022

seando-adsk left a comment

seando-adsk Nov 14, 2022

stefanminning-autodesk Nov 15, 2022

seando-adsk left a comment

seando-adsk commented Nov 16, 2022

stefanminning-autodesk commented Nov 17, 2022

		Context manager that creates a temporary environment variable and deletes it on exit,
		so it's usable with "with" statement.

Assign new material to multiple objects #2641

Assign new material to multiple objects #2641

Conversation

stefanminning-autodesk commented Oct 5, 2022

seando-adsk commented Oct 5, 2022

stefanminning-autodesk commented Oct 5, 2022

seando-adsk commented Oct 6, 2022

stefanminning-autodesk commented Oct 7, 2022 • edited Loading

seando-adsk commented Oct 7, 2022

stefanminning-autodesk commented Oct 7, 2022

womanyoyoyo-adsk commented Oct 13, 2022

stefanminning-autodesk commented Oct 18, 2022

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

Choose a reason for hiding this comment

stefanminning-autodesk commented Nov 2, 2022

stefanminning-autodesk commented Nov 2, 2022

seando-adsk commented Nov 3, 2022

stefanminning-autodesk commented Nov 3, 2022

stefanminning-autodesk commented Nov 11, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

seando-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

seando-adsk left a comment

Choose a reason for hiding this comment

seando-adsk commented Nov 16, 2022

stefanminning-autodesk commented Nov 17, 2022

stefanminning-autodesk commented Oct 7, 2022 •

edited

Loading