Apply Restriction Rules for Parenting #673

HamedSabri-adsk · 2020-07-21T12:44:09Z

This PR applies the same restriction rules that we currently have for "Rename" to "Re-parent" operation.

…deline.

…the operations.

HamedSabri-adsk · 2020-07-21T12:45:40Z

lib/mayaUsd/ufe/UsdUndoInsertChildCommand.h

+    SdfLayerHandle _childLayer;
+    SdfLayerHandle _parentLayer;
+
+    Ufe::Path _ufeDstPath;



Cleaned up this class according to our guidelines.

HamedSabri-adsk · 2020-07-21T12:48:57Z

lib/mayaUsd/ufe/UsdUndoInsertChildCommand.cpp

 {
+    const auto& childPrim = child->prim();
+    const auto& parentPrim = parent->prim();
+


cleaned up initializing class member variables.

HamedSabri-adsk · 2020-07-21T12:50:46Z

lib/mayaUsd/ufe/UsdUndoInsertChildCommand.cpp

+    // Apply restriction rules
+    ufe::applyCommandRestriction(childPrim, "reparent");
+    ufe::applyCommandRestriction(parentPrim, "reparent");
+


@ppt-adsk here I am applying the restriction rules very early. These rules are applied both to the child and parent prims.

Is this really the only change/fix here? Other than name changes I see a couple places where we've removed nullptr checks and then this is the only real change, do I have that right?

Hey @fowlertADSK I have broken down my PR into 3 parts to make it easier to review:

1- c358550
2- 0202eec
3- baf1595

The 2 line changes that you pointed out are part of section 2. There are also 2 important changes in section 2 which are related to cleaning up _childLayer and _parentLayer.

Section 3 (Fix failing unit tests) is what affects your earlier changes. If I recall correctly you told me last time that you were planning to make changes to testGroupCmd. I wanted to make sure we are aligned on this.

HamedSabri-adsk · 2020-07-21T12:59:18Z

lib/mayaUsd/ufe/UsdUndoInsertChildCommand.cpp

-    _childLayer = MayaUsdUtils::defPrimSpecLayer(childPrim);
-    if (!_childLayer) {
-        std::string err = TfStringPrintf("No child prim found at %s", childPrim.GetPath().GetString().c_str());
-        throw std::runtime_error(err.c_str());
-    }
+    _childLayer = _stage->GetEditTarget().GetLayer();

    // If parent prim is the pseudo-root, no def primSpec will be found, so
    // just use the edit target layer.
-    _parentLayer = parentPrim.IsPseudoRoot() ?
-        _stage->GetEditTarget().GetLayer() :
-        MayaUsdUtils::defPrimSpecLayer(parentPrim);
-
-    if (!_parentLayer) {
-        std::string err = TfStringPrintf("No parent prim found at %s", parentPrim.GetPath().GetString().c_str());
-        throw std::runtime_error(err.c_str());
-    }
+    _parentLayer = parentPrim.IsPseudoRoot() 
+        ? _stage->GetEditTarget().GetLayer() 
+        : MayaUsdUtils::defPrimSpecLayer(parentPrim);
 }


Both "_childLayer" and "_parentLayer" should be now in the correct target layers, therefore they can simply take "_stage->GetEditTarget().GetLayer()".

@ppt-adsk does "_parentLayer" still need to check for "parentPrim.IsPseudoRoot()"? I would like to understand when this happens.

HamedSabri-adsk · 2020-07-21T13:08:52Z

test/lib/ufe/testParentCmd.py

+        stage = mayaUsd.ufe.getStage(str(shapeSegment))
+
+        # check GetLayerStack behavior
+        self.assertEqual(stage.GetLayerStack()[0], stage.GetSessionLayer())
+        self.assertEqual(stage.GetEditTarget().GetLayer(), stage.GetSessionLayer())
+
+        # set the edit target to the root layer
+        stage.SetEditTarget(stage.GetRootLayer())
+        self.assertEqual(stage.GetEditTarget().GetLayer(), stage.GetRootLayer())


@ppt-adsk We now need to make sure we are in the correct target layer before making any edits.

Also please note that this logic would soon change once #661 is in. I will refactor this chunk of code since the same logics is repeated for many of our test cases.

HamedSabri-adsk · 2020-07-21T13:18:55Z

test/lib/ufe/testParentCmd.py

-            # path strings.
-            cmds.parent("|mayaUsdProxy1|mayaUsdProxyShape1,/pCylinder1/pCube1/pSphere1", "|mayaUsdProxy1|mayaUsdProxyShape1")
-
-            # Confirm that the sphere is now a child of the proxy shape.
-            shapeChildren = shapeHier.children()
-            self.assertIn("pSphere1", childrenNames(shapeChildren))
-
-            # Undo: the sphere is no longer a child of the proxy shape.
-            cmds.undo()
-
-            shapeChildren = shapeHier.children()
-            self.assertNotIn("pSphere1", childrenNames(shapeChildren))
-
-            # Redo: confirm that the sphere is again a child of the proxy shape.
-            cmds.redo()
-
-            shapeChildren = shapeHier.children()
-            self.assertIn("pSphere1", childrenNames(shapeChildren))
-
-            # Confirm that the sphere's world transform has not changed.  Must
-            # re-create the item, as its path has changed.
-            sphereChildPath = ufe.Path(
-                [shapeSegment, usdUtils.createUfePathSegment("/pSphere1")])
-            sphereChildItem = ufe.Hierarchy.createItem(sphereChildPath)
-            sphereChildT3d = ufe.Transform3d.transform3d(sphereChildItem)
-
-            sphereWorld = sphereChildT3d.inclusiveMatrix()
-            assertVectorAlmostEqual(
-                self, sphereWorldListPre, matrixToList(sphereWorld), places=6)
-
-            # Undo.
-            cmds.undo()
-
-            shapeChildren = shapeHier.children()
-            self.assertNotIn("pSphere1", childrenNames(shapeChildren))
+            # path strings.Expect the exception happens
+            with self.assertRaises(RuntimeError):
+                cmds.parent("|mayaUsdProxy1|mayaUsdProxyShape1,/pCylinder1/pCube1/pSphere1", "|mayaUsdProxy1|mayaUsdProxyShape1")


@ppt-adsk This test would have passed prior to applying the rules but now it fails because of third rule we have in place:

Cannot reparent with definitions or opinions on other layers. Opinions exist in [simpleHierarchy.usda],[simpleHierarchy-session.usda]

I am going to create a new PR to move the deleted part under with self.assertRaises(RuntimeError):. I really shouldn't have removed the code now that I think more about it.

HamedSabri-adsk · 2020-07-21T13:35:35Z

test/lib/ufe/testGroupCmd.py

-        self.assertTrue(ball35Path not in childPathsRedo)
-
+        self.assertTrue(ball5Path not in childPathsRedo)
+        self.assertTrue(ball3Path not in childPathsRedo)


Hey @fowlertADSK , prior to applying the restriction rules, one could use top_layer.ma scene for writing this test case. However, with the new rules in place, we can't really use it:

The first error you would hit is this:

Cannot reparent [Ball_25] defined on another layer. Please set [Assembly_room_set.usda] as the target layer to proceed

And once you go to the correct target layer, you would hit this.

Cannot reparent [Ball_25] with definitions or opinions on other layers. Opinions exist in [Ball.usd],[edits.usda],[Assembly_room_set.usda],[Ball.maya.usd],[Ball.shadingVariants.usda]

Hence, creating a simpler version of ball_set scene and applying your previous group logics to it.

fowlertADSK · 2020-07-27T19:48:22Z

test/lib/ufe/test-samples/groupBalls/ballset.ma

@@ -0,0 +1,189 @@
+//Maya ASCII 2020 scene


Can you resave this in something like Maya 2019 or 2018 if you have it? If not, I can grab the file and send an updated version to you (not sure if for this file it's as easy as typing in "2018" by hand). Matt caught that in one of my recent changes and I installed 2018 so I'd have it around.

Sure thing. Yes just changing the text should do it since we are dealing with the ASCII. I have been doing it many times before.

The important part to change is this: requires maya "2020";

We have other Maya test scenes that are still 2020. I think we should change them as well ( something for another PR ). I wonder how they could load in older Maya cut since @kxl-adsk usually runs tests against 2019.

There is an option to ignore the version, it might be on by default these days? Changing the requires line usually works, especially in simple files that we create for automated tests that don't have much in them, but it can definitely fail depending on what exactly is in there.

Something else I usually do when creating automated test files is to now save the UI state in the file. It's usually not needed for the test, it makes the file a lot bigger, and it's something that can mess up interactive tests occasionally.

@fowlertADSK please see e123bad

Hamed Sabri added 3 commits July 20, 2020 14:25

Clean up member variable names, header orders, etc.. according to gui…

c358550

…deline.

Apply restriction rules for child and parent prims before performing …

0202eec

…the operations.

Fix failing unit tests ( testGroupCmd, testParentCmd ).

baf1595

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

HamedSabri-adsk requested review from fowlertADSK and ppt-adsk July 21, 2020 12:44

HamedSabri-adsk commented Jul 21, 2020

View reviewed changes

put back the test codes that I removed earlier.

23d2f9b

HamedSabri-adsk assigned ecp-maya-devops-adsk Jul 21, 2020

Make MacOS/Linux compiler happy by changing the initializing order .

54ed18a

HamedSabri-adsk assigned ecp-maya-devops-adsk and unassigned ecp-maya-devops-adsk Jul 21, 2020

ppt-adsk approved these changes Jul 23, 2020

View reviewed changes

fowlertADSK reviewed Jul 27, 2020

View reviewed changes

change required version to 2018.

e123bad

fowlertADSK approved these changes Jul 28, 2020

View reviewed changes

kxl-adsk assigned ecp-maya-devops-adsk and unassigned ecp-maya-devops-adsk Jul 28, 2020

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

kxl-adsk merged commit f14af0e into dev Jul 28, 2020

kxl-adsk deleted the sabrih/MAYA-105765/apply_restriction_rules_parenting branch July 28, 2020 14:46

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Apply Restriction Rules for Parenting #673

Apply Restriction Rules for Parenting #673

HamedSabri-adsk commented Jul 21, 2020

HamedSabri-adsk Jul 21, 2020

HamedSabri-adsk Jul 21, 2020

HamedSabri-adsk Jul 21, 2020

fowlertADSK Jul 27, 2020

HamedSabri-adsk Jul 27, 2020 •

edited

Loading

HamedSabri-adsk Jul 21, 2020

HamedSabri-adsk Jul 21, 2020

HamedSabri-adsk Jul 21, 2020

HamedSabri-adsk Jul 21, 2020

fowlertADSK Jul 27, 2020

HamedSabri-adsk Jul 27, 2020

fowlertADSK Jul 27, 2020

HamedSabri-adsk Jul 28, 2020

Apply Restriction Rules for Parenting #673

Apply Restriction Rules for Parenting #673

Conversation

HamedSabri-adsk commented Jul 21, 2020

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 Jul 27, 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

Choose a reason for hiding this comment

HamedSabri-adsk Jul 27, 2020 •

edited

Loading