Fallback, multi-matrix parent absolute. #1201

ppt-adsk · 2021-02-22T04:29:12Z

No description provided.

ppt-adsk · 2021-02-22T16:46:55Z

lib/mayaUsd/ufe/UsdTransform3dFallbackMayaXformStack.cpp

@@ -143,7 +144,10 @@ void setXformOpOrder(const UsdGeomXformable& xformable)
    xformable.SetXformOpOrder(newOrder, resetsXformStack);
 }

-Ufe::Transform3d::Ptr createTransform3d(const Ufe::SceneItem::Ptr& item)
+Ufe::Transform3d::Ptr createEditTransform3dImp(


Factored out common code between createTransform3d() and editTransform3d().

Would be good to add documentation for parameters. As it is now, it's unclear without reading the implementation to know which arguments are input vs output. Adding a prefix is another nice way of helping developers that have to work with your code.

Will add documentation. As for input versus output, I usually stick to
http://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#f17-for-in-out-parameters-pass-by-reference-to-non-const
so that non-const ref parameters will be modified inside the function, but I will add that information in the documentation.

I would prefer to see more usage of Doxygen formatted documentation with proper usage of direction [in] and [out] to make this clear - https://www.doxygen.nl/manual/commands.html#cmdparam. I won't block the review on it.

ppt-adsk · 2021-02-22T16:48:52Z

lib/mayaUsd/ufe/UsdTransform3dFallbackMayaXformStack.cpp

+
+    // The Maya fallback transform stack is the last group of transform ops in
+    // the complete transform stack, so Mr and thus inv(Mr), are the identity.
+    return UsdTransform3dSetObjectMatrix::create(editTransform3d, ml.GetInverse(), GfMatrix4d(1));


transform3d() interface is used to set the object position (under parent -absolute) using the complete transform stack. To do this, use the new UsdTransform3dSetObjectMatrix wrapper that takes an editTransform3d() interface, along with the matrices to perform the matrix algebra to change only the fallback transform ops.

ppt-adsk · 2021-02-22T16:49:37Z

lib/mayaUsd/ufe/UsdTransform3dMatrixOp.cpp

@@ -45,6 +46,52 @@ VtValue getValue(const UsdAttribute& attr, const UsdTimeCode& time)

 const char* getMatrixOp() { return std::getenv("MAYA_USD_MATRIX_XFORM_OP_NAME"); }

+std::vector<UsdGeomXformOp>::const_iterator


Factor out common code between the editTransform3d() and transform3d() interfaces.

ppt-adsk · 2021-02-22T16:50:12Z

lib/mayaUsd/ufe/UsdTransform3dMatrixOp.cpp

+        // Use editTransform3d() to set a single matrix transform op.
+        // transform3d() returns a whole-object interface, which may include
+        // other transform ops.
+        auto t3d = Ufe::Transform3d::editTransform3d(sceneItem());


editTransform3d() sets the matrix for a single matrix op, which is what we want.

ppt-adsk · 2021-02-22T16:50:59Z

lib/mayaUsd/ufe/UsdTransform3dMatrixOp.cpp

+    // pivot edit was done on a matrix op stack.  Since matrix ops don't
+    // support pivot edits, a fallback Maya stack will be added, and from that
+    // point on the fallback Maya stack must be used.
+    if (findNonMatrix(i, xformOps)) {


Use factored-out code --- no change in behavior.

ppt-adsk · 2021-02-22T17:34:25Z

lib/mayaUsd/ufe/UsdTransform3dSetObjectMatrix.h

+//
+// Certain Maya operations (such as parent -absolute) require the capability to
+// edit the matrix transform of the whole object.  This class wraps a
+// UsdTransform3dMatrixOp or UsdTransform3dFallbackMayaXformStack and converts


For UsdTransform3dMatrixOp targeting a single matrix op in a multi-matrix stack, and unconditionally for UsdTransform3dFallbackMayaXformStack, use this new UsdTransform3dSetObjectMatrix object to set the local transform of the object (e.g. for parent -absolute).

You wrote a lot of great comments and documentation...but it still took me some time to figure out what exactly maps to Ml, Mw, Mr. Now looking at it back, what would help me to understand the code quicker is an example of a xform stack and how you break it down into these three transformations given a specific op we want to edit. It's all in the code, but having it described in one place (including pull request description) would help a lot.

I was hoping that the one place would be this header. Perhaps a pointer in the pull request description to this file would help? I can certainly add examples for the Maya fallback stack and a multi-matrix stack to this header.

ppt-adsk · 2021-02-22T17:36:23Z

lib/mayaUsd/ufe/wrapUtils.cpp

@@ -213,6 +225,7 @@ void wrapUtils()
    def("usdPathToUfePathSegment",
        usdPathToUfePathSegment,
        (arg("usdPath"), arg("instanceIndex") = UsdImagingDelegate::ALL_INSTANCES));
+    def("getTime", getTime);


Reading transform op values directly through the USD interface requires time (whereas the UFE interface uses proxy time). Expose the time accessor in Python.

ppt-adsk · 2021-02-22T17:37:31Z

test/lib/ufe/testComboCmd.py

@@ -706,8 +706,15 @@ def testFallback(self):
        mayaSphereItem        = ufe.Hierarchy.createItem(mayaSpherePath)
        usdSphereItem         = ufe.Hierarchy.createItem(usdSpherePath)
        usdFallbackSphereItem = ufe.Hierarchy.createItem(usdFallbackSpherePath)
-        usdSphere3d         = ufe.Transform3d.transform3d(usdSphereItem)
-        usdFallbackSphere3d = ufe.Transform3d.transform3d(usdFallbackSphereItem)
+        # For scene items with fallback transform ops, the transform3d()


Fix latent bug exposed with new transform3d() implementation for Maya fallback stacks. It is inappropriate to use transform3d() for reading and writing fallback transform ops; editTransform3d() must be used.

ppt-adsk · 2021-02-22T17:39:25Z

test/lib/ufe/testParentCmd.py

+        checkParentDone()
+
+    @unittest.skipUnless(mayaUtils.previewReleaseVersion() >= 123, 'Requires Maya fixes only available in Maya Preview Release 123 or later.') 
+    def testZParentAbsoluteMultiMatrixOp(self):


Evil test order dependence is back. testParentAbsoluteMultiMatrixOp() causes testParentAbsoluteSingleMatrixOp() to fail if the former is run first, so rename it.

ppt-adsk · 2021-02-22T17:40:01Z

test/lib/ufe/testParentCmd.py

+                                usdUtils.createUfePathSegment('/Capsule1')])
+        capsuleItem = ufe.Hierarchy.createItem(capsulePath)
+
+        # Add 3 matrix transform ops to the capsule.


Create a stack of 3 matrices, and test editing of each one, in turn.

kxl-adsk

Left bunch of comments...mostly requests for improvements to the documentation.

kxl-adsk · 2021-02-23T10:28:14Z

lib/mayaUsd/ufe/UsdTransform3dFallbackMayaXformStack.cpp

@@ -143,7 +144,10 @@ void setXformOpOrder(const UsdGeomXformable& xformable)
    xformable.SetXformOpOrder(newOrder, resetsXformStack);
 }

-Ufe::Transform3d::Ptr createTransform3d(const Ufe::SceneItem::Ptr& item)
+Ufe::Transform3d::Ptr createEditTransform3dImp(


Would be good to add documentation for parameters. As it is now, it's unclear without reading the implementation to know which arguments are input vs output. Adding a prefix is another nice way of helping developers that have to work with your code.

kxl-adsk · 2021-02-23T10:44:25Z

lib/mayaUsd/ufe/UsdTransform3dMayaXformStack.cpp

@@ -466,6 +469,9 @@ Ufe::Vector3d UsdTransform3dMayaXformStack::rotation() const
    }
    UsdGeomXformOp r = getOp(NdxRotate);
    TF_AXIOM(r);
+    if (!r.GetAttr().HasValue()) {


This pattern is possibly repeated in every transform 3d implementation. Have you looked for it?

kxl-adsk · 2021-02-23T10:58:25Z

test/lib/ufe/testParentCmd.py

+        # Redo: capsule still hasn't moved, Maya fallback ops are back.
+        cmds.redo()
+
+        checkParentDone()


kxl-adsk · 2021-02-23T11:05:06Z

test/lib/ufe/testParentCmd.py

+            cmds.undo()
+
+        # Restore initial conditions.
+        del os.environ['MAYA_USD_MATRIX_XFORM_OP_NAME']


How about we spare other developers debugging and put an assert in setUp to detect when another test left this env variable set?

Excellent idea.

kxl-adsk · 2021-02-23T11:23:37Z

lib/mayaUsd/ufe/UsdTransform3dSetObjectMatrix.h

+//
+// Certain Maya operations (such as parent -absolute) require the capability to
+// edit the matrix transform of the whole object.  This class wraps a
+// UsdTransform3dMatrixOp or UsdTransform3dFallbackMayaXformStack and converts


You wrote a lot of great comments and documentation...but it still took me some time to figure out what exactly maps to Ml, Mw, Mr. Now looking at it back, what would help me to understand the code quicker is an example of a xform stack and how you break it down into these three transformations given a specific op we want to edit. It's all in the code, but having it described in one place (including pull request description) would help a lot.

kxl-adsk · 2021-02-23T19:46:09Z

lib/mayaUsd/ufe/UsdTransform3dFallbackMayaXformStack.cpp

@@ -143,7 +144,10 @@ void setXformOpOrder(const UsdGeomXformable& xformable)
    xformable.SetXformOpOrder(newOrder, resetsXformStack);
 }

-Ufe::Transform3d::Ptr createTransform3d(const Ufe::SceneItem::Ptr& item)
+Ufe::Transform3d::Ptr createEditTransform3dImp(


I would prefer to see more usage of Doxygen formatted documentation with proper usage of direction [in] and [out] to make this clear - https://www.doxygen.nl/manual/commands.html#cmdparam. I won't block the review on it.

kxl-adsk · 2021-02-23T19:46:58Z

lib/mayaUsd/ufe/UsdTransform3dSetObjectMatrix.h

@@ -58,6 +58,36 @@ namespace ufe {
 // matrices inv(Ml) and inv(Mr).  In the case of the Maya fallback transform
 // stack, Mr is the identity by definition, as the Maya fallback transform
 // stack must be the last group of transform ops in the transform stack.
+//
+// Here is an example given a Maya fallback transform stack:


kxl-adsk · 2021-02-23T19:47:20Z

test/lib/ufe/testParentCmd.py

+    def tearDown(self):
+        # If there was no 'MAYA_USD_MATRIX_XFORM_OP_NAME' environment variable,
+        # make sure there is none at the end of the test.
+        if self.mayaUsdMatrixXformOpName is None:


Fallback, multi-matrix parent absolute.

bc6f29c

ppt-adsk assigned ecp-maya-devops-adsk Feb 22, 2021

Fix pre-flight errors.

c8e1e83

ppt-adsk assigned ecp-maya-devops-adsk and unassigned ecp-maya-devops-adsk Feb 22, 2021

ppt-adsk requested a review from kxl-adsk February 22, 2021 16:46

ppt-adsk commented Feb 22, 2021

View reviewed changes

Fix more pre-flight errors.

5c281ba

ppt-adsk assigned ecp-maya-devops-adsk and unassigned ecp-maya-devops-adsk Feb 22, 2021

Fix clang-format error.

fe35686

ppt-adsk assigned ecp-maya-devops-adsk and unassigned ecp-maya-devops-adsk Feb 22, 2021

Restore initial conditions after multi-matrix parent test.

b31c057

ppt-adsk assigned ecp-maya-devops-adsk and unassigned ecp-maya-devops-adsk Feb 23, 2021

kxl-adsk reviewed Feb 23, 2021

View reviewed changes

Addressed code review feedback.

3892d75

ppt-adsk assigned ecp-maya-devops-adsk and unassigned ecp-maya-devops-adsk Feb 23, 2021

kxl-adsk approved these changes Feb 23, 2021

View reviewed changes

ppt-adsk added the ready-for-merge Development process is finished, PR is ready for merge label Feb 24, 2021

kxl-adsk merged commit ca457f8 into dev Feb 24, 2021

kxl-adsk deleted the tremblp/MAYA-109273/multi_matrix_and_fallback_parent_absolute branch February 24, 2021 02:28

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fallback, multi-matrix parent absolute. #1201

Fallback, multi-matrix parent absolute. #1201

ppt-adsk commented Feb 22, 2021

ppt-adsk Feb 22, 2021

kxl-adsk Feb 23, 2021

ppt-adsk Feb 23, 2021

kxl-adsk Feb 23, 2021

ppt-adsk Feb 22, 2021

ppt-adsk Feb 22, 2021

ppt-adsk Feb 22, 2021

ppt-adsk Feb 22, 2021

ppt-adsk Feb 22, 2021

kxl-adsk Feb 23, 2021

ppt-adsk Feb 23, 2021

ppt-adsk Feb 22, 2021

ppt-adsk Feb 22, 2021

ppt-adsk Feb 22, 2021

ppt-adsk Feb 22, 2021

kxl-adsk left a comment

kxl-adsk Feb 23, 2021

kxl-adsk Feb 23, 2021

kxl-adsk Feb 23, 2021

kxl-adsk Feb 23, 2021

ppt-adsk Feb 23, 2021

kxl-adsk Feb 23, 2021

kxl-adsk Feb 23, 2021

kxl-adsk Feb 23, 2021

kxl-adsk Feb 23, 2021

		@@ -45,6 +46,52 @@ VtValue getValue(const UsdAttribute& attr, const UsdTimeCode& time)

		const char* getMatrixOp() { return std::getenv("MAYA_USD_MATRIX_XFORM_OP_NAME"); }

		std::vector<UsdGeomXformOp>::const_iterator

Fallback, multi-matrix parent absolute. #1201

Fallback, multi-matrix parent absolute. #1201

Conversation

ppt-adsk commented Feb 22, 2021

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

kxl-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