MAYA-105277: Initial work to support None-destructive reorder ( move ) of prims relative to their siblings. #867

HamedSabri-adsk · 2020-10-25T16:45:43Z

This PR is the initial work to support None-destructive reorder (move) of USD prim(s) relative to their siblings based on Maya's native reorder command.

Supported features:

Drag/Drop in outliner
Undo/Redo single/multiple items

To see how reorder command works: https://download.autodesk.com/us/maya/2011help/CommandsPython/reorder.html

Demo:

ppt-adsk

Looking good! A few minor things to fix up, but otherwise really close.

ppt-adsk · 2020-10-27T13:49:03Z

lib/mayaUsd/ufe/ProxyShapeHierarchy.cpp

-    // Ufe::Hierarchy::insertChildCmd() returns a base class
-    // Ufe::UndoableCommand::Ptr object, from which we can't retrieve the added
-    // child.  PPT, 13-Jul-2020.
+    #ifdef UFE_V2_FEATURES_AVAILABLE


You sure about this macro? There is no Hierarchy::insertChild() in UFE v1.

Good catch. Yes this #ifdef is redundant. Addressed in 0977cf1

ppt-adsk · 2020-10-27T13:58:04Z

lib/mayaUsd/ufe/ProxyShapeHierarchy.cpp

+	// in order to get the parent out of it.
+    const auto& childPrim = downcast((*orderedList.begin()))->prim();
+
+	return UsdUndoReorderCommand::create(childPrim, orderedTokens);


This is confusing to me: in the UsdUndoReorderCommand, all we do with this childPrim is to get its parent, as far as I can tell. Then why not give the parent directly, especially here since we know the parent is the pseudo-root.

Can't agree more... passing parent is more appropriate. Addressed in 0977cf1

ppt-adsk · 2020-10-27T13:59:40Z

lib/mayaUsd/ufe/UsdHierarchy.cpp

-    // Ufe::Hierarchy::insertChildCmd() returns a base class
-    // Ufe::UndoableCommand::Ptr object, from which we can't retrieve the added
-    // child.  PPT, 13-Jul-2020.
+    #ifdef UFE_V2_FEATURES_AVAILABLE


Again, insertChild() doesn't exist in UFE v1, so pretty sure this ifdef is incorrect.

Addressed in 0977cf1

ppt-adsk · 2020-10-27T14:13:50Z

lib/mayaUsd/ufe/UsdHierarchy.cpp

+        orderedTokens.emplace_back(downcast(item)->prim().GetPath().GetNameToken());
+    }
+
+    // TODO HS Oct 23, 2020, grab any child and pass it to UsdUndoReorderCommand


Since the first argument is the parent, it's the scene item of the Hierarchy interface, so this is just

return UsdUndoReorderCommand::create(downcast(sceneItem()), orderedTokens);

@ppt-adsk
But downcast(sceneItem()) doesn't give me the parent and rather the item that I am moving.

e.g /Xform1/pSphere1

What I want is the parent (/Xform1) since that's the one whose spec is going to call the SetNameChildrenOrder.

see 0977cf1

Hmm, I'm confused! Isn't this a method that is called on the parent, to reorder its children? I.e. you're calling it on /Xform1's hierarchy interface, to reorder its children, so Hierarchy::sceneItem() will in fact give you the parent.

Ahhh.. sorry about the confusion. Now I see it... :) I was calling the the command on a wrong hierarchy interface.

I have it fixed soon on the data model side and then make this change.

@ppt-adsk addressed in 520b36d

ppt-adsk · 2020-10-27T14:14:33Z

lib/mayaUsd/ufe/UsdHierarchy.h

-    ) override;
+
+    Ufe::SceneItem::Ptr insertChild(const Ufe::SceneItem::Ptr& child,const Ufe::SceneItem::Ptr& pos) override;
+	Ufe::InsertChildCommand::Ptr insertChildCmd(const Ufe::SceneItem::Ptr& child, const Ufe::SceneItem::Ptr& pos) override;


Funky indentation?

Yup... all of the code base under lib/mayaUsd/ufe has a mix of spaces and tabs. I can't wait for @kxl-adsk to apply the clang format on the repo :) .

ppt-adsk · 2020-10-27T14:16:04Z

lib/mayaUsd/ufe/UsdUndoReorderCommand.h

+#include <ufe/undoableCommand.h>
+
+#include <mayaUsd/base/api.h>
+#include <mayaUsd/ufe/UsdSceneItem.h>


Do we need to include UsdSceneItem.h?

Good point. Not at all. This header came from my copy-paste of another class. UsdSceneItem.h brings in #include <pxr/usd/usd/prim.h> and that what we need here.

Addressed in 0977cf1

ppt-adsk · 2020-10-27T14:18:24Z

test/testSamples/reorderCmd/primitives.usda

@@ -0,0 +1,53 @@
+#usda 1.0


Nit-pick, but seems like such a simple file, we could just create the scene on the fly in the test, I would think.

I agree with you. primitives.usda has a very simple hierarchy and I could have created it on the fly. I admit being lazy here :)

ppt-adsk · 2020-10-27T14:21:43Z

test/testUtils/mayaUtils.py

@@ -164,6 +164,10 @@ def openGroupBallsScene():
    filePath = os.path.join(os.path.dirname(os.path.realpath(__file__)), "..", "testSamples", "groupBalls", "ballset.ma" )
    cmds.file(filePath, force=True, open=True)

+def openPrimitivesScene():


Have a look at the recent improvement that SPI put in for data file path access.

And I can't help to re-iterate that if you created the scene on the fly, you wouldn't need to do this :)

Addressed in 0977cf1

ppt-adsk · 2020-10-27T18:04:12Z

test/lib/ufe/testReorderCmd.py

+        ''' Reorder (a.k.a move) objects relative to their siblings.
+           For relative moves, both positive and negative numbers may be specified.
+        '''
+        shapeSegment = mayaUtils.createUfePathSegment("|world|Primitives_usd|Primitives_usdShape")


Because this test won't be executed for UFE v1, it would be much clearer and more maintainable to use UFE path strings:
cylinderPath = ufe.PathString.path('Primitives_usd|Primitives_usdShape,/Xform1/Cylinder1')

Thanks. Great point.

Addressed in 0977cf1

ppt-adsk · 2020-10-27T18:04:54Z

test/lib/ufe/testReorderCmd.py

+            Reorder (a.k.a move) to front and back of the sibling list
+        '''
+        shapeSegment = mayaUtils.createUfePathSegment("|world|Primitives_usd|Primitives_usdShape")
+        cylinderPath = ufe.Path([shapeSegment, usdUtils.createUfePathSegment("/Xform1/Cylinder1")])


Use path strings.

Addressed in 0977cf1

ppt-adsk · 2020-10-27T20:45:26Z

lib/mayaUsd/ufe/ProxyShapeHierarchy.cpp


-	return UsdUndoReorderCommand::create(childPrim, orderedTokens);
+	return UsdUndoReorderCommand::create(parentPrim, orderedTokens);


Of course there's nothing wrong with this, but you can avoid the downcast and the call to childPrim.GetParent(), because here you know the parent prim is the pseudo-root, so you can just pass it:

return UsdUndoReorderCommand::create(getUsdRootPrim(), orderedTokens);

@ppt-adsk addressed in 520b36d

Hamed Sabri added 3 commits October 25, 2020 12:15

MAYA-105277 : implement UsdUndoReorderCommand

a95a659

MAYA-105277 : add testReorderCmd.

d38fcde

MAYA-105277: add guards around reorder command.

13330c4

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

HamedSabri-adsk requested a review from ppt-adsk October 25, 2020 16:45

HamedSabri-adsk assigned ecp-maya-devops-adsk Oct 25, 2020

MAYA-105277 : fix the bad merge.

66e7ab5

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

MAYA-105277: react to ufe interface changes.

e502399

ppt-adsk requested changes Oct 27, 2020

View reviewed changes

Hamed Sabri added 2 commits October 27, 2020 15:15

Merge branch 'dev' into sabri/MAYA-105277/reorder_support_ufe

688b46a

MAYA-105277: address feedbacks.

0977cf1

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

ppt-adsk reviewed Oct 27, 2020

View reviewed changes

Address feedback.

520b36d

ppt-adsk approved these changes Oct 28, 2020

View reviewed changes

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

kxl-adsk merged commit f144c43 into dev Oct 28, 2020

kxl-adsk deleted the sabri/MAYA-105277/reorder_support_ufe branch October 28, 2020 19:42

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

MAYA-105277: Initial work to support None-destructive reorder ( move ) of prims relative to their siblings. #867

MAYA-105277: Initial work to support None-destructive reorder ( move ) of prims relative to their siblings. #867

HamedSabri-adsk commented Oct 25, 2020

ppt-adsk left a comment

ppt-adsk Oct 27, 2020

HamedSabri-adsk Oct 27, 2020

ppt-adsk Oct 27, 2020

HamedSabri-adsk Oct 27, 2020

ppt-adsk Oct 27, 2020

HamedSabri-adsk Oct 27, 2020

ppt-adsk Oct 27, 2020

HamedSabri-adsk Oct 27, 2020

ppt-adsk Oct 27, 2020

HamedSabri-adsk Oct 27, 2020

HamedSabri-adsk Oct 27, 2020

ppt-adsk Oct 27, 2020

HamedSabri-adsk Oct 27, 2020

ppt-adsk Oct 27, 2020

HamedSabri-adsk Oct 27, 2020

ppt-adsk Oct 27, 2020

HamedSabri-adsk Oct 27, 2020

ppt-adsk Oct 27, 2020

ppt-adsk Oct 27, 2020

HamedSabri-adsk Oct 27, 2020

ppt-adsk Oct 27, 2020

HamedSabri-adsk Oct 27, 2020

ppt-adsk Oct 27, 2020

HamedSabri-adsk Oct 27, 2020

ppt-adsk Oct 27, 2020

HamedSabri-adsk Oct 27, 2020


		return UsdUndoReorderCommand::create(childPrim, orderedTokens);
		return UsdUndoReorderCommand::create(parentPrim, orderedTokens);

MAYA-105277: Initial work to support None-destructive reorder ( move ) of prims relative to their siblings. #867

MAYA-105277: Initial work to support None-destructive reorder ( move ) of prims relative to their siblings. #867

Conversation

HamedSabri-adsk commented Oct 25, 2020

Supported features:

Demo:

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment