LOOKDEVX-862- Rename ports #2644

alicedegirolamo · 2022-10-11T12:20:48Z

Description

Added the command for renaming an attribute.
I modified UFE in this PR in order to add the new command to the interface.

Type of change

New feature (non-breaking change which adds functionality)

Checklist:

My code follows the style guidelines of this project

- delete test code

ppt-adsk

I will let Jerry review the shading network part of this pull request. It does need an automated test.

ppt-adsk · 2022-10-11T15:58:19Z

lib/mayaUsd/ufe/UsdAttributes.h

@@ -62,6 +62,8 @@ class UsdAttributes : public Ufe::Attributes
    Ufe::AddAttributeCommand::Ptr
                              addAttributeCmd(const std::string& name, const Ufe::Attribute::Type& type) override;
    Ufe::UndoableCommand::Ptr removeAttributeCmd(const std::string& name) override;
+    Ufe::UndoableCommand::Ptr


This will be available in UFE version 0.4.33; it isn't available in 0.4.24.

Yes, I've updated the version.

ppt-adsk · 2022-10-11T16:01:22Z

lib/mayaUsd/ufe/UsdAttributes.cpp

+    const std::string&       newName)
+{
+    if (!sceneItem || !sceneItem->prim().IsActive()
+        || !UsdAttributes(sceneItem).hasAttribute(targetName)) {


Don't understand this line. We can't rename if if don't have an attribute with the targetName?

The name of the variable creates confusion. Maybe something like originalName would be less ambiguous and a better match to newName? When I see targetName I look for sourceName, which does not fit the logic of this code.

I've renamed the variables everywhere (also in PR for UFE) so now should be more clear.

ppt-adsk · 2022-10-11T16:02:04Z

lib/mayaUsd/ufe/UsdAttributes.cpp

+    }
+
+    // Do not rename if an attribute already exists with the newName
+    if (UsdAttributes(sceneItem).hasAttribute(newName)) {


This is the opposite condition of line 469.

I suspect we want to integrate the work you did in #2622 so a new name gets automatically generated on collision. This would most probably require updating the UFE API to return the renamed Ufe::Attribute instead of a boolean. Might not be too late to change that Ufe API.

Yes, right now if we want to create a unique name when there is a collision we need #2622.

JGamache-autodesk · 2022-10-11T18:56:06Z

lib/mayaUsd/ufe/UsdAttributes.cpp

+    }
+
+    // Do not rename if an attribute already exists with the newName
+    if (UsdAttributes(sceneItem).hasAttribute(newName)) {


I suspect we want to integrate the work you did in #2622 so a new name gets automatically generated on collision. This would most probably require updating the UFE API to return the renamed Ufe::Attribute instead of a boolean. Might not be too late to change that Ufe API.

JGamache-autodesk · 2022-10-11T19:01:06Z

lib/mayaUsd/ufe/UsdAttributes.cpp

+    const std::string&       newName)
+{
+    if (!sceneItem || !sceneItem->prim().IsActive()
+        || !UsdAttributes(sceneItem).hasAttribute(targetName)) {


The name of the variable creates confusion. Maybe something like originalName would be less ambiguous and a better match to newName? When I see targetName I look for sourceName, which does not fit the logic of this code.

JGamache-autodesk · 2022-10-11T19:07:19Z

lib/mayaUsd/ufe/UsdAttributes.cpp

+    auto            prim = sceneItem->prim();
+    auto            attribute = prim.GetAttribute(nameAsToken);
+
+    PXR_NS::UsdShadeNodeGraph      ngPrim(prim);


Looks like some of this code could be replaced by a call to canRemoveAttribute(targetName) since the restrictions are exactly the same between removing and renaming.

Yes exactly, I delete that function

JGamache-autodesk · 2022-10-11T19:10:33Z

lib/mayaUsd/ufe/UsdAttributes.cpp

+    PXR_NS::TfToken           nameAsToken(targetName);
+    auto                      prim = sceneItem->prim();
+    auto                      attribute = prim.GetAttribute(nameAsToken);
+    PXR_NS::UsdShadeNodeGraph ngPrim(prim);


This unused variable will most probably cause the preflight to fail on Linux and OS/X.

I removed it also in the new version since I don't use it.

JGamache-autodesk · 2022-10-11T19:16:55Z

lib/mayaUsd/ufe/UsdAttributes.cpp

+    if (propertyHandle) {
+        return propertyHandle->SetName(newName);
+    }
+


Almost there, but the hardest part goes here. If the attribute was connected, then it must stay connected. This means finding and updating all connection sources that used the old name and changing that to the new name. This is where ngPrim will come handy as you only have to do that if the attribute is a NodeGraph boundary. You do not have to traverse the whole stage, only the nodes in the enclosing NodeGraph, if any, and the nodes in ngPrim.

I should also have completed this part.

JGamache-autodesk · 2022-10-11T19:22:47Z

lib/mayaUsd/ufe/UsdAttributes.cpp

@@ -319,6 +319,12 @@ Ufe::UndoableCommand::Ptr UsdAttributes::removeAttributeCmd(const std::string& n
 {
    return UsdRemoveAttributeCommand::create(fItem, name);
 }
+
+Ufe::UndoableCommand::Ptr


As Pierre mentioned somewhere else, this code is new to UFE 0.4.33; it isn't available in 0.4.24 where add/remove was added. You will have to move this new code into separate block that will be guarded with #if (UFE_PREVIEW_VERSION_NUM >= 4033) (see line 311).

ppt-adsk · 2022-10-13T14:26:50Z

lib/mayaUsd/ufe/UsdAttributes.cpp

-Ufe::UndoableCommand::Ptr
-UsdAttributes::renameAttributeCmd(const std::string& targetName, const std::string& newName)
+#endif
+//#if (UFE_PREVIEW_VERSION_NUM >= 4033)


Commented-out code.

yes I forgot to remove the comment.

The #if condition is needed.

@JGamache-autodesk The code is not consistent in this file. Does it need the two conditions UFE_V4_FEATURES_AVAILABLE and UFE_PREVIEW_VERSION_NUM >= 4033 -- or -- only the latter?

ppt-adsk · 2022-10-13T14:29:54Z

lib/mayaUsd/ufe/UsdUndoAttributesCommands.h

@@ -96,14 +96,14 @@ class UsdRemoveAttributeCommand : public UsdUndoableCommand<Ufe::UndoableCommand
 }; // UsdRemoveAttributeCommand

 //! \brief Implementation of RenameAttributeCommand
-class UsdRenameAttributeCommand : public UsdUndoableCommand<Ufe::UndoableCommand>
+class UsdRenameAttributeCommand : public UsdUndoableCommand<Ufe::AttributeValuedUndoableCommand>


Is AttributeValuedUndoableCommand in a checked-in version of UFE? I can't find it in UFE master.

Yes, it is here: https://git.autodesk.com/media-and-entertainment/ufe/pull/308

Update, following this discussion for UFE, I've updated the names.

alicedegirolamo · 2022-10-17T08:53:33Z

I will let Jerry review the shading network part of this pull request. It does need an automated test.

I also added and tested the new test.

hodoulp · 2022-10-18T18:08:36Z

@alicedegirolamo did you address all the reviewer's comments? if done, please ping reviewers to finalize & merge the pull request.

alicedegirolamo · 2022-10-21T09:26:51Z

@alicedegirolamo did you address all the reviewer's comments? if done, please ping reviewers to finalize & merge the pull request.

Yes, I did.

@ppt-adsk @JGamache-autodesk could you please check if my corrections are ok? Thank you.

hodoulp

There are points:

The code is not consistent in the changed files and with the current changes. Does it need the two conditions UFE_V4_FEATURES_AVAILABLE and UFE_PREVIEW_VERSION_NUM >= 4033 -- or -- only the latter?
Being in maya-usd the pull request must have at least one unit test to demonstrate that the usd implementaion is working.

hodoulp · 2022-10-25T20:35:37Z

lib/mayaUsd/ufe/UsdAttributes.cpp

-Ufe::UndoableCommand::Ptr
-UsdAttributes::renameAttributeCmd(const std::string& targetName, const std::string& newName)
+#endif
+//#if (UFE_PREVIEW_VERSION_NUM >= 4033)


The #if condition is needed.

hodoulp · 2022-10-25T20:37:38Z

lib/mayaUsd/ufe/UsdAttributes.cpp

-Ufe::UndoableCommand::Ptr
-UsdAttributes::renameAttributeCmd(const std::string& targetName, const std::string& newName)
+#endif
+//#if (UFE_PREVIEW_VERSION_NUM >= 4033)


@JGamache-autodesk The code is not consistent in this file. Does it need the two conditions UFE_V4_FEATURES_AVAILABLE and UFE_PREVIEW_VERSION_NUM >= 4033 -- or -- only the latter?

alicedegirolamo · 2022-11-02T07:51:27Z

Hi @alicedegirolamo, you can take me off the reviewer's list for this pull request, I think others can fully take over. Cheers!

Hi @ppt-adsk , yes, sure. Thanks!

# Conflicts: # lib/mayaUsd/ufe/UsdUndoAttributesCommands.cpp

JGamache-autodesk

New code looks good, but is untested. Fastest way is to import test scene test\testSamples\MaterialX\MayaSurfaces.usda, and rename /pCube2/Looks/standardSurface2SG/MayaNG_standardSurface2SG.inputs:file2:varnameStr to inputs:file2:varname and make sure the connection from /pCube2/Looks/standardSurface2SG/MayaNG_standardSurface2SG/place2dTexture2.inputs:geomprop is fixed. Then you rename /pCube2/Looks/standardSurface2SG/MayaNG_standardSurface2SG.outputs:baseColor to outputs:MyColor and make sure the input connection from /pCube2/Looks/standardSurface2SG/standardSurface2.inputs:base_color stays connected.

alicedegirolamo · 2022-11-08T17:38:39Z

New code looks good, but is untested. Fastest way is to import test scene test\testSamples\MaterialX\MayaSurfaces.usda, and rename /pCube2/Looks/standardSurface2SG/MayaNG_standardSurface2SG.inputs:file2:varnameStr to inputs:file2:varname and make sure the connection from /pCube2/Looks/standardSurface2SG/MayaNG_standardSurface2SG/place2dTexture2.inputs:geomprop is fixed. Then you rename /pCube2/Looks/standardSurface2SG/MayaNG_standardSurface2SG.outputs:baseColor to outputs:MyColor and make sure the input connection from /pCube2/Looks/standardSurface2SG/standardSurface2.inputs:base_color stays connected.

I've updated the test and checked it's passing for the renaming in test/lib/ufe/testAttributes.py

alicedegirolamo added 8 commits September 26, 2022 11:33

Add first version for renameAttribute function

c7a5db0

Add RenameAttributeCommand

465b089

Add canRenameAttribute

2dbce29

- delete test code

Add missing check for material outputs

78ec066

Merge branch 'dev' into degiroa/LOOKDEVX-862/rename-and-delete-ports

3cae843

Add override renameAttributeCmd function

542290c

Merge branch 'dev' into degiroa/LOOKDEVX-862/rename-and-delete-ports

5b3df2e

Add missing braces

0788a0b

alicedegirolamo requested a review from JGamache-autodesk October 11, 2022 12:20

alicedegirolamo assigned alicedegirolamo and ecp-maya-devops-adsk Oct 11, 2022

ppt-adsk requested changes Oct 11, 2022

View reviewed changes

JGamache-autodesk requested changes Oct 11, 2022

View reviewed changes

alicedegirolamo added 3 commits October 12, 2022 15:53

Start recreating connections

cf792ac

Use alias for AddAttributeCommand

ff741b9

Recreate connections for renamed attribute

14ea1fd

ppt-adsk reviewed Oct 13, 2022

View reviewed changes

alicedegirolamo added 4 commits October 13, 2022 16:43

Rename bool

781c8e6

Move connectedApi check

3702743

Merge branch 'dev' into degiroa/LOOKDEVX-862/rename-and-delete-ports

1f643bb

Handle name uniqueness

2066d6a

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

seando-adsk unassigned alicedegirolamo Oct 17, 2022

Merge branch 'dev' into degiroa/LOOKDEVX-862/rename-and-delete-ports

8bf3540

hodoulp requested review from seando-adsk and neilh-adsk October 25, 2022 19:15

hodoulp requested changes Oct 25, 2022

View reviewed changes

alicedegirolamo requested review from ppt-adsk and removed request for ppt-adsk November 2, 2022 07:50

alicedegirolamo added 2 commits November 2, 2022 11:55

Add missing #ifdef for new attribute cmd typedef

1cba9b8

Merge branch 'dev' into degiroa/LOOKDEVX-862/rename-and-delete-ports

25a2f4d

seando-adsk previously approved these changes Nov 4, 2022

View reviewed changes

alicedegirolamo added 2 commits November 4, 2022 14:28

Merge branch 'dev' into degiroa/LOOKDEVX-862/rename-and-delete-ports

69563dd

# Conflicts: # lib/mayaUsd/ufe/UsdUndoAttributesCommands.cpp

Update to ufe 0.4.34

9fc697d

alicedegirolamo dismissed seando-adsk’s stale review via 9fc697d November 4, 2022 14:13

alicedegirolamo added the ready-for-merge Development process is finished, PR is ready for merge label Nov 7, 2022

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

JGamache-autodesk requested changes Nov 7, 2022

View reviewed changes

alicedegirolamo added 2 commits November 8, 2022 17:27

Fix compound renaming connections

dfb9353

Update test for renaming attribute and testing connections

cfac06c

alicedegirolamo requested a review from ppt-adsk November 8, 2022 17:51

alicedegirolamo assigned ecp-maya-devops-adsk and unassigned ecp-maya-devops-adsk Nov 8, 2022

Merge branch 'dev' into degiroa/LOOKDEVX-862/rename-and-delete-ports

226b5c5

alicedegirolamo assigned ecp-maya-devops-adsk and unassigned ecp-maya-devops-adsk Nov 8, 2022

JGamache-autodesk approved these changes Nov 8, 2022

View reviewed changes

hodoulp approved these changes Nov 8, 2022

View reviewed changes

seando-adsk approved these changes Nov 8, 2022

View reviewed changes

Merge branch 'dev' into degiroa/LOOKDEVX-862/rename-and-delete-ports

92e5955

alicedegirolamo assigned ecp-maya-devops-adsk and unassigned ecp-maya-devops-adsk Nov 9, 2022

alicedegirolamo added the ready-for-merge Development process is finished, PR is ready for merge label Nov 9, 2022

seando-adsk merged commit ed25078 into dev Nov 9, 2022

seando-adsk deleted the degiroa/LOOKDEVX-862/rename-and-delete-ports branch November 9, 2022 15:28

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

LOOKDEVX-862- Rename ports #2644

LOOKDEVX-862- Rename ports #2644

alicedegirolamo commented Oct 11, 2022 •

edited

Loading

ppt-adsk left a comment

ppt-adsk Oct 11, 2022

alicedegirolamo Oct 13, 2022

ppt-adsk Oct 11, 2022

JGamache-autodesk Oct 11, 2022

alicedegirolamo Oct 13, 2022

ppt-adsk Oct 11, 2022

JGamache-autodesk Oct 11, 2022

alicedegirolamo Oct 13, 2022 •

edited

Loading

JGamache-autodesk Oct 11, 2022

JGamache-autodesk Oct 11, 2022

JGamache-autodesk Oct 11, 2022

alicedegirolamo Oct 13, 2022

JGamache-autodesk Oct 11, 2022

alicedegirolamo Oct 13, 2022

JGamache-autodesk Oct 11, 2022

alicedegirolamo Oct 21, 2022

JGamache-autodesk Oct 11, 2022

ppt-adsk Oct 13, 2022

alicedegirolamo Oct 17, 2022

hodoulp Oct 25, 2022

hodoulp Oct 25, 2022

ppt-adsk Oct 13, 2022

alicedegirolamo Oct 17, 2022

alicedegirolamo Oct 26, 2022

alicedegirolamo commented Oct 17, 2022

hodoulp commented Oct 18, 2022

alicedegirolamo commented Oct 21, 2022

hodoulp left a comment

hodoulp Oct 25, 2022

hodoulp Oct 25, 2022

alicedegirolamo commented Nov 2, 2022

JGamache-autodesk left a comment

alicedegirolamo commented Nov 8, 2022

LOOKDEVX-862- Rename ports #2644

LOOKDEVX-862- Rename ports #2644

Conversation

alicedegirolamo commented Oct 11, 2022 • edited Loading

Description

Type of change

Checklist:

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

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

alicedegirolamo Oct 13, 2022 • 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

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

alicedegirolamo commented Oct 17, 2022

hodoulp commented Oct 18, 2022

alicedegirolamo commented Oct 21, 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

alicedegirolamo commented Nov 2, 2022

JGamache-autodesk left a comment

Choose a reason for hiding this comment

alicedegirolamo commented Nov 8, 2022

alicedegirolamo commented Oct 11, 2022 •

edited

Loading

alicedegirolamo Oct 13, 2022 •

edited

Loading