MAYA-106755: Support automatic updating of internal references when the path they are referencing has changed. #797

HamedSabri-adsk · 2020-09-24T15:49:32Z

Description

The goal of this PR is to allow clients to be able to rename prims that are internally referenced by other prims and not worry about breaking them. The same workflow can also later be applied to other composition arcs ( e.g inherits or specializes ).

…eferences if the path they are referencing has changed.

HamedSabri-adsk · 2020-09-28T13:44:13Z

lib/mayaUsd/ufe/UsdUndoRenameCommand.cpp

+    TfToken::HashSet childrenNames;
+    for (auto child : prim.GetParent().GetChildren()){
+        childrenNames.insert(child.GetName());
+    }
+    if (childrenNames.find(TfToken(_newName)) != childrenNames.end()){
+        _newName = uniqueName(childrenNames, _newName);
+    }
+
+    // names are not allowed to start to digit numbers
+    if(std::isdigit(_newName.at(0))){
+        _newName = prim.GetName();
+    }
+
+    // all special characters are replaced with `_`
+    const std::string specialChars{"~!@#$%^&*()-=+,.?`':{}|<>[]/ "};
+    std::replace_if(_newName.begin(), _newName.end(), [&](auto c){
+        return std::string::npos != specialChars.find(c);
+    }, '_');


Moved all the code for making the new name unique to the constructor. This is for sake of making the renameRedo clean and also there is no real benefit to keep calling them on every redo operation.

HamedSabri-adsk · 2020-09-28T13:57:16Z

lib/usd/utils/util.cpp

-    for (const auto& p : newPrim.GetStage()->TraverseAll()) {
-
-        // check is this prim has a spec
+    SdfChangeBlock changeBlock;


Added SdfChangeBlock to make the loop more performant and delay sending notification until after we exist this function.

Based on what we discussed internally the other day, having SdfChangeBlock on the stack while making calls into the Usd API's could be troublesome but I didn't see any issue in this case.

HamedSabri-adsk · 2020-09-28T14:00:07Z

lib/usd/utils/util.cpp

                        // add the new internal reference
-                        status = p.GetReferences().AddInternalReference(newPath);
+                        status = p.GetReferences().AddInternalReference(finalPath);


currently there is no APIs either at Sdf or Usd level to help querying the "append" or "prepend" operations for the entries in the list. Therefore, UsdListPosition is always set by the default value UsdListPositionBackOfPrependList passed to AddInternalReference.

This needs to be documented.

HamedSabri-adsk · 2020-09-28T14:19:17Z

lib/mayaUsd/ufe/UsdUndoRenameCommand.cpp

-        return false;
-    }
+    // send notification to update UFE data model
+    sendNotification<Ufe::ObjectRename>(_ufeDstItem, _ufeSrcItem->path());

    return true;


Updating the internal references paths should happen first before the primSpec->SetName.

Tidy up renameRedo a little bit and moved the unique name logics to the constructor.

…suggestion from Pixar.

HamedSabri-adsk · 2020-09-29T14:41:04Z

lib/usd/utils/util.cpp

+                itemVector.Replace(ref, newRef);
+            }
+        }
+    }
 }


@ppt-adsk Per conversation with Spiff, it was suggested that there would be a more efficient technique to approach how the internal references list ops can be updated.

In the new approach we directly replacing the old SdfReference with the new SdfReference and call the itemVector Replace() to switch them. This is more efficient than using the Usd API approach where I needed to clear the list first and add the new one reference.

HamedSabri-adsk · 2020-09-29T14:48:02Z

lib/usd/utils/util.cpp

+
+                // update append/prepend lists individually 
+                replaceReferenceItems(oldPrim, newPath, referencesList, SdfListOpTypeAppended);
+                replaceReferenceItems(oldPrim, newPath, referencesList, SdfListOpTypePrepended);
            }


@ppt-adsk We need to update each individual list separately. For now, I am only concerned about append and prepend list Ops.

As a side note, the word "list" could be a bit confusing. Each list edit operation ( e.g appen , prepend, etc.. ) are stored in std::vector like containers.

An SdfListProxy represents a single list of list editing operations, making it look like an STL vector (modeling a random access container and back insertion sequence).

https://github.com/PixarAnimationStudios/USD/blob/release/pxr/usd/sdf/listProxy.h

ppt-adsk · 2020-09-29T18:05:19Z

lib/mayaUsd/ufe/UsdUndoRenameCommand.cpp

-    std::replace_if(_newName.begin(), _newName.end(), [&](auto c){
-        return std::string::npos != specialChars.find(c);
-    }, '_');
+        auto newSceneItem = createSiblingSceneItem(_ufeSrcItem->path(), _newName);


Instead of doing the string manipulation, can't we just dynamic_cast to a UsdSceneItem and get the new prim's path, much like on (new) line 117?

Hey @ppt-adsk let's have a chat offline tomorrow. I am not sure if the dynamic_cast would work here but maybe I am missing something obvious.

ppt-adsk · 2020-09-29T18:06:23Z

lib/mayaUsd/ufe/UsdUndoRenameCommand.cpp

+        status = primSpec->SetName(_newName);
+        if (!status) {
+            return false;
+        }
    }

    // the renamed scene item is a "sibling" of its original name.
    _ufeDstItem = createSiblingSceneItem(_ufeSrcItem->path(), _newName);


This is a duplicate of line 119.

ppt-adsk · 2020-09-29T18:08:11Z

lib/mayaUsd/ufe/UsdUndoRenameCommand.cpp

-        return false;
+        auto newSceneItem = createSiblingSceneItem(_ufeSrcItem->path(), _ufeSrcItem->prim().GetName());
+        auto newUfePath = Ufe::Path(newSceneItem->path().getSegments()[1]);
+        bool status = MayaUsdUtils::updateInternalReferencesPath(prim, SdfPath(newUfePath.string()));


Same comment as redo(). In fact, undo() and redo() are so similar it would be good to factor out the code into a separate private method and call that from undo() and redo() with the proper arguments.

ppt-adsk · 2020-09-29T18:08:43Z

lib/mayaUsd/ufe/UsdUndoRenameCommand.cpp

    }

-    // shouldn't have to again create a sibling sceneItem here since we already have a valid _ufeSrcItem
-    // however, I get random crashes if I don't which needs furthur investigation.  HS, 6-May-2020.
+    // the renamed scene item is a "sibling" of its original name.
    _ufeSrcItem = createSiblingSceneItem(_ufeDstItem->path(), _ufeSrcItem->prim().GetName());


Same comment as redo, duplicate of line 161.

I agree, the duplicate can be cleaned up for sure.

ppt-adsk · 2020-09-29T18:10:37Z

lib/usd/utils/util.cpp

+                               SdfListOpType op)
+    {
+        // set the itemVector based on the SdfListOpType
+        auto itemVector = referencesList.GetAppendedItems();


Awkward that we assign to the itemVector unconditionally, then potentially squash it at lines 82-89.

itemVector is of type SdfListProxyand can't be created without initializing.

error C2512: 'pxrInternal_v0_20__pxrReserved__::SdfListProxy<pxrInternal_v0_20__pxrReserved__::SdfReferenceTypePolicy>': no appropriate default constructor available

This was what I originally had in mind.

// set the itemVector based on the SdfListOpType SdfReferencesProxy::ListProxy itemVector; if (op == SdfListOpTypeAppended) { itemVector = referencesList.GetAppendedItems(); } if (op == SdfListOpTypePrepended) { itemVector = referencesList.GetPrependedItems(); } else if (op == SdfListOpTypeOrdered) { itemVector = referencesList.GetOrderedItems(); } else if (op == SdfListOpTypeAdded) { itemVector = referencesList.GetAddedItems(); } else if (op == SdfListOpTypeDeleted) { itemVector = referencesList.GetDeletedItems(); }

I also went with auto to be on the safe side but the type here is SdfListProxy<SdfReferenceTypePolicy> or a.k.a ( SdfReferencesProxy::ListProxy )

std::cout << "type = " << typeid(itemVector).name() << '\n'; outputs: SdfListProxy<SdfReferenceTypePolicy>

Also the name "itemVector" is a bit misleading which is the return type for GetAppendedItems, GetPrependedItems, GetOrderedItems, GetAddedItems, GetDeletedItems in SdfListOp but what we have here is from SdfListEditorProxy. There is no Replace() in SdfListOp. I addressed this in 0618a8e to make it more clear.

See SdfListOp vs SdfListEditorProxy

https://github.com/PixarAnimationStudios/USD/blob/ebac0a8b6703f4fa1c27115f1f013bb9819662f4/pxr/usd/sdf/listOp.h

https://github.com/PixarAnimationStudios/USD/blob/ebac0a8b6703f4fa1c27115f1f013bb9819662f4/pxr/usd/sdf/listEditorProxy.h

O.K., you want ugly, I can give you ugly :)

SdfReferencesProxy::ListProxy itemVector = (op == SdfListOpTypeAppended) ? referencesList.GetAppendedItems() : (op == SdfListOpTypePrepended) ? referencesList.GetPrependedItems() : (op == SdfListOpTypeOrdered) ? referencesList.GetOrderedItems() : (op == SdfListOpTypeAdded) ? referencesList.GetAddedItems() : (op == SdfListOpTypeDeleted) ? referencesList.GetDeletedItems() : SdfReferencesProxy::ListProxy();

Per our offline conversation, I will leave things as is to avoid making broader changes.

HamedSabri-adsk · 2020-09-30T15:47:32Z

lib/mayaUsd/ufe/UsdUndoRenameCommand.cpp

-        auto newUfePath = Ufe::Path(newSceneItem->path().getSegments()[1]);
-        bool status = MayaUsdUtils::updateInternalReferencesPath(prim, SdfPath(newUfePath.string()));
+        auto ufeSiblingPath = _ufeSrcItem->path().sibling(Ufe::PathComponent(_newName));
+        bool status = MayaUsdUtils::updateInternalReferencesPath(prim, SdfPath(ufeSiblingPath.getSegments()[1].string()));


@ppt-adsk Thank you for pointing out to use sibling().

…ashes maya * Use the uniqueChildName() for the undo rename command.

kxl-adsk · 2020-10-02T13:55:56Z

@HamedSabri-adsk @seando-adsk Now that PF is back online, I re-run the check and Maya 2020 fails. Please have a look. For now, I'm removing ready-for-merge label.

HamedSabri-adsk · 2020-10-05T12:19:33Z

@HamedSabri-adsk @seando-adsk Now that PF is back online, I re-run the check and Maya 2020 fails. Please have a look. For now, I'm removing ready-for-merge label.

Thanks @kxl-adsk . It looks like a UFE1.0 issue after @seando-adsk change in my branch.

  S:\jenkins\workspace\ecg-mayausd-branch-preflight-2020-windows\ecg-maya-usd\maya-usd\lib\mayaUsd\ufe\UsdHierarchy.cpp(156): error C2039: 'nodeName': is not a member of 'Ufe_v1::SceneItem'
    p:\shared\artifactory\windows\rundev-2020\202004291615\runtime\df03b16\devkitbase\devkit\ufe\include\ufe\sceneItem.h(35): note: see declaration of 'Ufe_v1::SceneItem'

seando-adsk · 2020-10-05T13:53:14Z

@HamedSabri-adsk Darn UFE v1 support. I'll have a look.

…ashes maya * Fix to support UFE v1 (Maya 2020 and earlier).

seando-adsk · 2020-10-05T14:10:51Z

@HamedSabri-adsk / @kxl-adsk I pushed a fix and restarted the Preflight.

…ave 'IsInternal' routine.

HamedSabri-adsk · 2020-10-05T18:50:05Z

lib/usd/utils/util.cpp

+    #else
+    return ref.GetAssetPath().empty();
+    #endif
+}


IsInternal() is a new routine that I added to SdfReference as part of a PR and is supported as of 20.08.

https://github.com/PixarAnimationStudios/USD/blob/71b4baace2044ea4400ba802e91667f9ebe342f0/pxr/usd/sdf/reference.h#L155

Added a guard to make sure I am not breaking older USD version ( e.g 19.11 )

HamedSabri-adsk · 2020-10-05T18:50:09Z

lib/usd/utils/util.cpp

-    }
-
-    return isInternalRef;
-}


This piece of code didn't make much sense and it is not used anymore, therefore removing it.

MAYA-106755: initial work to support automatic updating of internal r…

944c4bb

…eferences if the path they are referencing has changed.

HamedSabri-adsk added ufe-usd Related to UFE-USD plugin in Maya-Usd workflows Related to in-context workflows labels Sep 24, 2020

HamedSabri-adsk requested a review from ppt-adsk September 24, 2020 15:49

MAYA-106755: Automatic update of internal references paths working.

efe42c5

HamedSabri-adsk commented Sep 28, 2020

View reviewed changes

HamedSabri-adsk marked this pull request as ready for review September 28, 2020 14:19

HamedSabri-adsk assigned ecp-maya-devops-adsk and unassigned ecp-maya-devops-adsk Sep 28, 2020

HamedSabri-adsk changed the title ~~MAYA-106755: initial work to support automatic updating of internal references when the path they are referencing has changed.~~ MAYA-106755: Support automatic updating of internal references when the path they are referencing has changed. Sep 28, 2020

MAYA-106755: use Sdf APIs to update internal references list Ops per …

e8c2a23

…suggestion from Pixar.

HamedSabri-adsk assigned ecp-maya-devops-adsk and unassigned ecp-maya-devops-adsk Sep 29, 2020

HamedSabri-adsk commented Sep 29, 2020

View reviewed changes

ppt-adsk requested changes Sep 29, 2020

View reviewed changes

Hamed Sabri added 2 commits September 29, 2020 16:57

address feedback: minor name clean up for clarity.

0618a8e

MAYA-106755: address feedback.

3b4eda4

HamedSabri-adsk commented Sep 30, 2020

View reviewed changes

HamedSabri-adsk requested a review from ppt-adsk September 30, 2020 15:50

HamedSabri-adsk assigned ecp-maya-devops-adsk and unassigned ecp-maya-devops-adsk Sep 30, 2020

ppt-adsk approved these changes Sep 30, 2020

View reviewed changes

HamedSabri-adsk added the ready-for-merge Development process is finished, PR is ready for merge label Sep 30, 2020

MAYA-106377 - renaming a prim to the same name as an inactive prim cr…

b96663d

…ashes maya * Use the uniqueChildName() for the undo rename command.

ppt-adsk approved these changes Oct 1, 2020

View reviewed changes

kxl-adsk assigned ecp-maya-devops-adsk and unassigned ecp-maya-devops-adsk Oct 2, 2020

kxl-adsk removed the ready-for-merge Development process is finished, PR is ready for merge label Oct 2, 2020

MAYA-106377 - renaming a prim to the same name as an inactive prim cr…

9d3c98a

…ashes maya * Fix to support UFE v1 (Maya 2020 and earlier).

seando-adsk assigned ecp-maya-devops-adsk and unassigned ecp-maya-devops-adsk Oct 5, 2020

HamedSabri-adsk added the ready-for-merge Development process is finished, PR is ready for merge label Oct 5, 2020

MAYA-106755: Fix compiler error with older USD version that doesn't h…

6003130

…ave 'IsInternal' routine.

HamedSabri-adsk commented Oct 5, 2020

View reviewed changes

HamedSabri-adsk assigned ecp-maya-devops-adsk and unassigned ecp-maya-devops-adsk Oct 5, 2020

ppt-adsk approved these changes Oct 5, 2020

View reviewed changes

kxl-adsk merged commit fab6650 into dev Oct 5, 2020

kxl-adsk deleted the sabrih/MAYA-106755/automatic_rename_update_internal_references branch October 5, 2020 20:24

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

MAYA-106755: Support automatic updating of internal references when the path they are referencing has changed. #797

MAYA-106755: Support automatic updating of internal references when the path they are referencing has changed. #797

HamedSabri-adsk commented Sep 24, 2020 •

edited

Loading

HamedSabri-adsk Sep 28, 2020

HamedSabri-adsk Sep 28, 2020

HamedSabri-adsk Sep 28, 2020

HamedSabri-adsk Sep 28, 2020

HamedSabri-adsk Sep 29, 2020

HamedSabri-adsk Sep 29, 2020

ppt-adsk Sep 29, 2020

HamedSabri-adsk Sep 29, 2020

ppt-adsk Sep 29, 2020

ppt-adsk Sep 29, 2020

ppt-adsk Sep 29, 2020

HamedSabri-adsk Sep 29, 2020

ppt-adsk Sep 29, 2020

HamedSabri-adsk Sep 29, 2020

ppt-adsk Sep 29, 2020

HamedSabri-adsk Sep 30, 2020 •

edited

Loading

HamedSabri-adsk Sep 30, 2020

kxl-adsk commented Oct 2, 2020

HamedSabri-adsk commented Oct 5, 2020

seando-adsk commented Oct 5, 2020

seando-adsk commented Oct 5, 2020

HamedSabri-adsk Oct 5, 2020

HamedSabri-adsk Oct 5, 2020 •

edited

Loading

MAYA-106755: Support automatic updating of internal references when the path they are referencing has changed. #797

MAYA-106755: Support automatic updating of internal references when the path they are referencing has changed. #797

Conversation

HamedSabri-adsk commented Sep 24, 2020 • edited Loading

Description

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

HamedSabri-adsk Sep 30, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kxl-adsk commented Oct 2, 2020

HamedSabri-adsk commented Oct 5, 2020

seando-adsk commented Oct 5, 2020

seando-adsk commented Oct 5, 2020

Choose a reason for hiding this comment

HamedSabri-adsk Oct 5, 2020 • edited Loading

Choose a reason for hiding this comment

HamedSabri-adsk commented Sep 24, 2020 •

edited

Loading

HamedSabri-adsk Sep 30, 2020 •

edited

Loading

HamedSabri-adsk Oct 5, 2020 •

edited

Loading