Improve rename logics #483

HamedSabri-adsk · 2020-05-06T15:26:15Z

This change simplifies the rename operation by simply renaming a prim in place via SdfPrimSpec's SetName routine and removes the old work-flow involving SdfCopySpec.

This change simplifies the rename operation by simply renaming a prim in place via SdfPrimSpec's SetName routine and removes the old work-flow involving SdfCopySpec.

HamedSabri-adsk · 2020-05-06T15:29:45Z

lib/usd/utils/util.h

+    //! Return a PrimSpec for the argument prim in the layer containing the stage's current edit target.
+    MAYA_USD_UTILS_PUBLIC
+    SdfPrimSpecHandle getPrimSpecAtEditTarget(UsdStageWeakPtr, const UsdPrim&);
+
 } // namespace MayaUsdUtils


added a utility function for getting a PrimSpec from a prim path in the layer containing the stage's current edit target.

HamedSabri-adsk · 2020-05-06T15:29:48Z

lib/mayaUsd/ufe/UsdUndoRenameCommand.h

+
+    const UsdPrim& _prim;
+    UsdStageWeakPtr _stage;
+    std::string _newName;



Simplified member variables. We no longer need _usdSrcPath, _usdDstPath, and _layer.

HamedSabri-adsk · 2020-05-06T15:30:05Z

lib/mayaUsd/ufe/UsdUndoRenameCommand.cpp

+    , _ufeDstItem(nullptr)
+    , _prim(srcItem->prim())
+    , _stage(_prim.GetStage())
+    , _newName(newName.string())


Using initializer list.

lib/mayaUsd/ufe/UsdUndoRenameCommand.cpp

HamedSabri-adsk · 2020-05-06T15:36:42Z

lib/mayaUsd/ufe/UsdUndoRenameCommand.cpp

    }

-    return status;
+    return true;


On Undo:
1- Get the SdfPrimSpec to the _ufeDstItem->prim() and check it's validity.
2- Set it's name to _prim's original name: primSpec->SetName(_prim.GetPath().GetElementString());
3- Set _ufeSrcItem and send UFE scene notification
4- Set _ufeDstItem = nullptr

ppt-adsk

I think to best demonstrate that this change is robust I would like to see an addition to the rename test where you access a child of the renamed prim, and try to do something with it e.g. use the hierarchy interface on the child to get its parent, which should now be the renamed prim. This way we would know that the subtree of prims under the renamed prim is in a good state.

ppt-adsk · 2020-05-06T19:55:44Z

lib/mayaUsd/ufe/UsdUndoRenameCommand.cpp

-            UsdEditContext ctx(_stage, _layer);
-            status = _stage->RemovePrim(_ufeSrcItem->prim().GetPath());
-        }
+    auto primSpec = MayaUsdUtils::getPrimSpecAtEditTarget(_stage, _prim);



Nit-pick: wonder if it wouldn't be clearer to just early-out false if !primSpec:

if (!primSpec) {
return false;
}

ppt-adsk · 2020-05-06T19:58:52Z

lib/mayaUsd/ufe/UsdUndoRenameCommand.cpp

-                assert(newPrim);
-            #endif
+        // set prim's name
+        primSpec->SetName(_prim.GetPath().GetElementString());


How is this setting the name back to the original name? Also, is prim still valid, since you've changed its name?

ppt-adsk · 2020-05-06T20:00:39Z

lib/mayaUsd/ufe/UsdUndoRenameCommand.cpp

-            _ufeSrcItem = createSiblingSceneItem(_ufeDstItem->path(), _usdSrcPath.GetElementString());
+        // 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.
+        _ufeSrcItem = createSiblingSceneItem(_ufeDstItem->path(), _prim.GetPath().GetElementString());


Isn't the prim in the original _ufeSrcItem now invalid?

…operations in undo/redo. - To read the code better, I added a new member variable _oldName that holds the prim's original name.

HamedSabri-adsk · 2020-05-06T21:57:25Z

lib/mayaUsd/ufe/UsdUndoRenameCommand.cpp

+    // XXX: SetName successfuly returns true but when you examine the _prim.GetName()
+    // after the rename, the prim name shows the original name HS, 6-May-2020.
+    bool status = primSpec->SetName(_newName);
+    if (!status) {
        return false;
    }


I do an early check for primSpec and SetName result.

@ppt-adsk there is something troubling happening here which I am trying to understand:

On Redo, I get the primspec for _prim object and rename it's name to the _newName. This operation return successful BUT when I query the _prim's name right after, I still get the _prim's original name. Huh???

I then set _ufeDstItem and when I print it's USD name and it gives me the _newName which is expected.

HamedSabri-adsk · 2020-05-06T21:57:33Z

lib/mayaUsd/ufe/UsdUndoRenameCommand.cpp

@@ -47,6 +47,7 @@ UsdUndoRenameCommand::UsdUndoRenameCommand(const UsdSceneItem::Ptr& srcItem, con
    , _prim(srcItem->prim())
    , _stage(_prim.GetStage())
    , _newName(newName.string())
+    , _oldName(_prim.GetName().GetString())


Added _oldName that holds the _prim name before it is changed.

HamedSabri-adsk · 2020-05-06T21:59:43Z

I think to best demonstrate that this change is robust I would like to see an addition to the rename test where you access a child of the renamed prim, and try to do something with it e.g. use the hierarchy interface on the child to get its parent, which should now be the renamed prim. This way we would know that the subtree of prims under the renamed prim is in a good state.

@ppt-adsk Very valid point. I will update the rename test.

…sd file for testing purposes. - Renamed testRename to testRenameUndo - Added new logics for testing new way of renaming a prim name via SdfPrimSpec.

HamedSabri-adsk · 2020-05-07T00:02:03Z

test/lib/ufe/testRename.py

+
+        # HS, 6-May-2020. defualtPrim in null?? This can't be null....
+        # defualtPrim = stage.GetDefaultPrim()
+        # self.assertEqual(defualtPrim.GetName(), 'TreeBase_potato')


@ppt-adsk Unless I am missing something really obvious, defualtPrim can not be null in line 138??

Both assert passed in line 134,135 which to me indicates the subtree under the renamed prim is in a good state.

@ppt-adsk OK, I did a little more digging into this and the good news is that the tree/subtree graph is in a good state after the rename. It is just that we can't no longer rely on the stage which looks like a bug on USD side to me or perhaps there is a rational reason for this behavior that I don't know.

# set primspec name primspec.name = "TreeBase_potato" potatoPrim = stage.GetPrimAtPath('/TreeBase_potato') self.assertEqual(potatoPrim.GetName(), 'TreeBase_potato') // Pass

Traverse the stage

/TreeBase_potato /TreeBase_potato/leavesXform /TreeBase_potato/leavesXform/leaves /TreeBase_potato/trunk

HamedSabri-adsk · 2020-05-07T00:03:05Z

test/lib/ufe/testRename.py

+        # defualtPrim = stage.GetDefaultPrim()
+        # self.assertEqual(defualtPrim.GetName(), 'TreeBase_potato')
+
+    def testRenameUndo(self):


renamed original testRename to testRenameUndo.

…s around the stage behavior that deserves an explanation.

- Add a simple testRenameRestrictionExternalReference unit test.

lib/mayaUsd/ufe/UsdUndoRenameCommand.cpp

HamedSabri-adsk · 2020-05-11T20:07:25Z

test/lib/ufe/testRename.py

+        with self.assertRaises(RuntimeError):
+           newName = 'leaf_ref_2_renaming'
+           cmds.rename(newName)
+


Added a simple unit test around rename restriction for an external reference.

NOTE: I originally didn't want to add treeRef.usda and treeRef.ma and my plan was to use already added tree.ma and programmatically add internal reference, however AddInternalReference didn't seem to give me what I wanted.

What I wanted was to add a typeless prim, append an internal reference, author bunch of attributes and be done.
e.g

def "leaf_ref_1" ( append references = </TreeBase/leavesXform/greenLeaf> ) { double3 xformOp:translate = (2.0, 2.5, 0) uniform token[] xformOpOrder = ["xformOp:translate"] double radius = 0.5 }

Here is how I originally planned to add in code:

refPrim = stage.DefinePrim('/TreeBase/leaf_ref_1') refPrim.GetReferences().AddInternalReference('/TreeBase/leavesXform/greenLeaf') # author refPrim's xform and radius xform = UsdGeom.Xformable(refPrim) xform.AddTranslateOp().Set(Gf.Vec3d(2.0, 2.5, 0)) radiusAttr = refPrim.GetAttribute('radius') radiusAttr.Set(0.5)

Instead I get something like this:

def Sphere "leaf_ref_1" { float3[] extent = [(-2, -2, -2), (2, 2, 2)] color3f[] primvars:displayColor = [(0, 1, 0)] double radius = 0.5 double3 xformOp:translate = (2, 2.5, 0) uniform token[] xformOpOrder = ["xformOp:translate"] }

lib/mayaUsd/ufe/UsdUndoRenameCommand.cpp

ppt-adsk · 2020-05-12T15:46:33Z

lib/mayaUsd/ufe/UsdUndoRenameCommand.cpp

+        // account for internal vs external references
+        if (_prim.HasAuthoredReferences()) {
+            auto primSpec = MayaUsdUtils::getPrimSpecAtEditTarget(_stage, _prim);
+            for (const SdfReference& ref : primSpec->GetReferenceList().GetAddedOrExplicitItems()) {


Feels strange that this is going through a loop... Because in the loop body, the if part of the conditional returns, and the else part of the conditional throws, so there is no way you'll get anything beyond the first item in the loop!

Also, is it possible for _prim.HasAuthoredReferences() to be true, but primSpec->GetReferenceList().GetAddedOrExplicitItmes() to return an empty list?

@ppt-adsk yes that was my first impression about the loop but that's what we have got.

Because in the loop body, the if part of the conditional returns, and the else part of the conditional throws, so there is no way you'll get anything beyond the first item in the loop!

Yes I agree. This is a first pass and I am sure I am going to modify this code as I understand more edge cases.

Also, is it possible for _prim.HasAuthoredReferences() to be true, but primSpec->GetReferenceList().GetAddedOrExplicitItmes() to return an empty list?

I don't think that would be the case at least by looking at the low-level code. I could be wrong.

ppt-adsk · 2020-05-12T15:49:33Z

lib/mayaUsd/ufe/UsdUndoRenameCommand.cpp

-        auto layers = MayaUsdUtils::layersWithPrimSpec(prim);
+        // account for internal vs external references
+        if (_prim.HasAuthoredReferences()) {
+            auto primSpec = MayaUsdUtils::getPrimSpecAtEditTarget(_stage, _prim);


Given that you've submitted a pull request to Pixar to better handle this functionality, wouldn't it be good to encapsulate this behind a utility function so that this code won't have to change when internal reference testing improves? The encapsulation would be specially useful since you'll have to support different implementations, depending on the version of USD that's being used. E.g. something like
bool isInternalReference(primSpec)

Good point, will make one in the next commit.

@ppt-adsk please see a305af9

…ternal reference and create a new utility for it.

ppt-adsk

Almost there, just a couple of trivial things and we're done!

ppt-adsk · 2020-05-12T18:18:11Z

lib/mayaUsd/ufe/UsdUndoRenameCommand.cpp

-                   throw std::runtime_error(err.c_str());
-                }
+
+            if(MayaUsdUtils::isInternalReference(primSpec)) {


Nit-pick: could just be
if (!MayaUsdUtils::isInternalReference(primSpec)) {
throw blablabla;
}

@ppt-adsk see 80aa35e

lib/usd/utils/util.cpp

… we do still want the opinion detection in other layers happens.

…things more readable. - change the container type for layersWithContribution method from std::vector to std::set to guarantee having unique layers.

HamedSabri-adsk · 2020-05-13T15:40:52Z

@ppt-adsk please see 3cf2acc

Improve rename logics:

ae2a7e0

This change simplifies the rename operation by simply renaming a prim in place via SdfPrimSpec's SetName routine and removes the old work-flow involving SdfCopySpec.

HamedSabri-adsk requested a review from ppt-adsk May 6, 2020 15:26

HamedSabri-adsk commented May 6, 2020

View reviewed changes

lib/mayaUsd/ufe/UsdUndoRenameCommand.cpp Show resolved Hide resolved

HamedSabri-adsk commented May 6, 2020

View reviewed changes

HamedSabri-adsk added core Related to core library ufe-usd Related to UFE-USD plugin in Maya-Usd labels May 6, 2020

ppt-adsk reviewed May 6, 2020

View reviewed changes

- Added early checks to make sure we are in a valid state when doing …

bb1da2e

…operations in undo/redo. - To read the code better, I added a new member variable _oldName that holds the prim's original name.

HamedSabri-adsk commented May 6, 2020

View reviewed changes

- Added tree.ma and tree.usda to the test-samples. This is a useful u…

c4f247a

…sd file for testing purposes. - Renamed testRename to testRenameUndo - Added new logics for testing new way of renaming a prim name via SdfPrimSpec.

HamedSabri-adsk commented May 7, 2020

View reviewed changes

Hamed Sabri added 2 commits May 7, 2020 12:27

Updated test prim. tesRename does pass but there are still some issue…

a45779c

…s around the stage behavior that deserves an explanation.

- Add new logics for handling internal vs external references.

b207895

- Add a simple testRenameRestrictionExternalReference unit test.

HamedSabri-adsk commented May 11, 2020

View reviewed changes

lib/mayaUsd/ufe/UsdUndoRenameCommand.cpp Show resolved Hide resolved

HamedSabri-adsk commented May 11, 2020

View reviewed changes

HamedSabri-adsk added the unit test Related to unit tests (both python or c++) label May 11, 2020

ppt-adsk requested changes May 12, 2020

View reviewed changes

Hamed Sabri added 2 commits May 12, 2020 12:46

Eliminate the need to stores _prim and _oldName member variables.

890372d

Address feedback: Re-factored the logics for detecting internal vs ex…

a305af9

…ternal reference and create a new utility for it.

ppt-adsk requested changes May 12, 2020

View reviewed changes

Hamed Sabri added 2 commits May 12, 2020 14:34

Address feedback: break out of the loop once the condition is met.

610f2ce

Address feedback: Don't return if we have an internal reference since…

80aa35e

… we do still want the opinion detection in other layers happens.

HamedSabri-adsk requested a review from ppt-adsk May 12, 2020 22:24

- change the wording form opinion to contribute/contribution to make …

3cf2acc

…things more readable. - change the container type for layersWithContribution method from std::vector to std::set to guarantee having unique layers.

ppt-adsk approved these changes May 13, 2020

View reviewed changes

HamedSabri-adsk merged commit 7bb77b3 into sabrih/MAYA-104215/improve_usd_rename_operation May 13, 2020

HamedSabri-adsk deleted the sabrih/MAYA-104215/improve_rename_logics branch May 13, 2020 17:11

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve rename logics #483

Improve rename logics #483

HamedSabri-adsk commented May 6, 2020

HamedSabri-adsk May 6, 2020

HamedSabri-adsk May 6, 2020

HamedSabri-adsk May 6, 2020 •

edited

Loading

HamedSabri-adsk May 6, 2020 •

edited

Loading

ppt-adsk left a comment

ppt-adsk May 6, 2020

ppt-adsk May 6, 2020

ppt-adsk May 6, 2020

HamedSabri-adsk May 6, 2020

HamedSabri-adsk May 6, 2020

HamedSabri-adsk commented May 6, 2020

HamedSabri-adsk May 7, 2020

HamedSabri-adsk May 7, 2020 •

edited

Loading

HamedSabri-adsk May 7, 2020

HamedSabri-adsk May 11, 2020

ppt-adsk May 12, 2020

HamedSabri-adsk May 12, 2020

ppt-adsk May 12, 2020

HamedSabri-adsk May 12, 2020

HamedSabri-adsk May 12, 2020

ppt-adsk left a comment

ppt-adsk May 12, 2020

HamedSabri-adsk May 12, 2020

HamedSabri-adsk commented May 13, 2020

Improve rename logics #483

Improve rename logics #483

Conversation

HamedSabri-adsk commented May 6, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

HamedSabri-adsk May 6, 2020 • edited Loading

Choose a reason for hiding this comment

HamedSabri-adsk May 6, 2020 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

HamedSabri-adsk commented May 6, 2020

Choose a reason for hiding this comment

HamedSabri-adsk May 7, 2020 • 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

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

HamedSabri-adsk commented May 13, 2020

HamedSabri-adsk May 6, 2020 •

edited

Loading

HamedSabri-adsk May 6, 2020 •

edited

Loading

HamedSabri-adsk May 7, 2020 •

edited

Loading