Implement and test setMatrixCmd() for single matrix op prims. #1102

ppt-adsk · 2021-01-22T19:13:19Z

No description provided.

ppt-adsk · 2021-01-22T19:14:02Z

lib/mayaUsd/python/wrapUsdUndoManager.cpp

@@ -16,7 +16,6 @@

 #include <mayaUsd/undo/UsdUndoBlock.h>
 #include <mayaUsd/undo/UsdUndoManager.h>
-#include <mayaUsd/undo/UsdUndoableItem.h>


ppt-adsk · 2021-01-22T19:15:10Z

lib/mayaUsd/ufe/UsdTransform3dMatrixOp.cpp

-    // our UsdTransform3dBaseHandler base class does not know how to edit the
-    // argument either.  Simply delegate to the next handler in the list.
-    return _nextHandler->transform3d(item);
+    // Remove code duplication with editTransform3d().  PPT, 21-Jan-2021.


Copy-paste from editTransform3d() a bit of a shame, hopefully can fix later.

ppt-adsk · 2021-01-22T19:15:52Z

lib/mayaUsd/ufe/UsdTransform3dMayaXformStack.cpp

@@ -162,21 +162,13 @@ createTransform3d(const Ufe::SceneItem::Ptr& item, NextTransform3dFn nextTransfo
    return stackOps.empty() ? nextTransform3dFn() : UsdTransform3dMayaXformStack::create(usdItem);
 }

-// Class for setMatrixCmd() implementation.  Should be rolled into a future
-// command class with full undo / redo support
+// Class for setMatrixCmd() implementation.  UsdUndoBlock data member and


Changed to use new undo / redo framework.

ppt-adsk · 2021-01-22T19:17:14Z

lib/mayaUsd/ufe/UsdUndoSetKindCommand.h

@@ -26,6 +26,8 @@

 #include <memory>

+PXR_NAMESPACE_USING_DIRECTIVE


Was relying on UsdUndoableItem.h to define this, but UsdUndoableItem.h doesn't need this definition and was removed.

Hey @ppt-adsk! I noticed this pattern in some of the other UFE classes and I actually intentionally avoided doing the same here when I added this command in #1094. This macro effectively resolves to a using namespace ... statement which it seems like we would want to avoid in headers:
https://github.com/PixarAnimationStudios/USD/blob/dc710925675f7f58f7a37f6c18d046a687b09271/pxr/pxr.h.in#L52

Instead I had attempted to resolve the symbols by prefixing all of the USD types with PXR_NS::, but looks like I probably missed a few. Reading through that class again, I would guess it's at least these in the header:

maya-usd/lib/mayaUsd/ufe/UsdUndoSetKindCommand.h

Line 55 in a9f8119

UsdPrim _prim;

maya-usd/lib/mayaUsd/ufe/UsdUndoSetKindCommand.h

Line 57 in a9f8119

TfToken _kind;

And maybe this in the implementation:

maya-usd/lib/mayaUsd/ufe/UsdUndoSetKindCommand.cpp

Line 54 in a9f8119

UsdModelAPI(_prim).SetKind(_kind);

Though I think using the PXR_NAMESPACE_USING_DIRECTIVE macro in a .cpp file would be fair game if we wanted to do that instead.

Makes perfect sense, fully agree with you. Let me wait until a bit more feedback builds up and I'll fix this.

Sounds good, thanks! And sorry for leaving that trap for you. :)

Are we ready to set a course on getting rid of all using directives from the header files in the core modules? That would be fantastic.

ppt-adsk · 2021-01-22T19:18:05Z

lib/mayaUsd/undo/UsdUndoableItem.h

@@ -14,16 +14,14 @@
 // limitations under the License.
 //

-#ifndef MAYAUSD_UNDO_UNDOABLE_BLOCK_H


Fixed copy-paste damage... Was causing really, really weird compilation errors for my local changes...

ppt-adsk · 2021-01-22T19:18:27Z

lib/mayaUsd/undo/UsdUndoableItem.h


 #include <mayaUsd/base/api.h>

 #include <functional>
 #include <vector>

-PXR_NAMESPACE_USING_DIRECTIVE


No Pixar namespace declarations or definitions here, removed.

ppt-adsk · 2021-01-22T19:19:26Z

test/lib/ufe/testParentCmd.py

@@ -244,6 +244,136 @@ def testParentAbsolute(self):
        cylChildren = cylHier.children()
        self.assertEqual(len(cylChildren), 1)

+    def testParentAbsoluteSingleMatrixOp(self):


Test written with TDD approach: failed initially, now passes.

Leading by example!

kxl-adsk · 2021-01-23T11:43:52Z

lib/mayaUsd/ufe/UsdTransform3dMatrixOp.cpp

+    // Remove code duplication with editTransform3d().  PPT, 21-Jan-2021.
+    UsdSceneItem::Ptr usdItem = std::dynamic_pointer_cast<UsdSceneItem>(item);
+#if !defined(NDEBUG)
+    assert(usdItem);


I haven't really heard any good reason yet why to use these naked asserts over a rich number of possibilities to raise an error/warning via Tf diagnostic macros. Please switch over.

Agreed, this is simply a copy-paste of older code.

lib/mayaUsd/ufe/UsdTransform3dMatrixOp.cpp

kxl-adsk · 2021-01-23T12:01:11Z

lib/mayaUsd/ufe/UsdUndoSetKindCommand.h

@@ -26,6 +26,8 @@

 #include <memory>

+PXR_NAMESPACE_USING_DIRECTIVE


Are we ready to set a course on getting rid of all using directives from the header files in the core modules? That would be fantastic.

kxl-adsk · 2021-01-23T12:02:06Z

lib/mayaUsd/undo/UsdUndoableItem.h


 #include <mayaUsd/base/api.h>

 #include <functional>
 #include <vector>

-PXR_NAMESPACE_USING_DIRECTIVE


kxl-adsk · 2021-01-23T12:05:35Z

test/lib/ufe/testParentCmd.py

@@ -244,6 +244,136 @@ def testParentAbsolute(self):
        cylChildren = cylHier.children()
        self.assertEqual(len(cylChildren), 1)

+    def testParentAbsoluteSingleMatrixOp(self):


Leading by example!

kxl-adsk

Looks like the parent test is failing with PR122

FAIL: testParentAbsolute (testParentCmd.ParentCmdTestCase)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "testParentCmd.py", line 225, in testParentAbsolute
    self, cubeWorldListPre, matrixToList(cubeWorld), places=6)
  File "E:\Dev\usd\_workspaces_latest\Maya_USD_PR_PR\build\RelWithDebInfo\test\python\testUtils.py", line 31, in assertVectorAlmostEqual
    testCase.assertAlmostEqual(va, vb, places)
AssertionError: 0.8660254037844389 != 2.0762759672369597 within 6 places

======================================================================
FAIL: testParentAbsoluteSingleMatrixOp (testParentCmd.ParentCmdTestCase)
Test parent -absolute on prim with a single matrix op.
----------------------------------------------------------------------
Traceback (most recent call last):
  File "testParentCmd.py", line 367, in testParentAbsoluteSingleMatrixOp
    self, cubeWorldListPre, matrixToList(cubeWorld), places=6)
  File "E:\Dev\usd\_workspaces_latest\Maya_USD_PR_PR\build\RelWithDebInfo\test\python\testUtils.py", line 31, in assertVectorAlmostEqual
    testCase.assertAlmostEqual(va, vb, places)
AssertionError: 0.8660254037844389 != 2.0762759672369597 within 6 places

======================================================================
FAIL: testParentToProxyShape (testParentCmd.ParentCmdTestCase)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "testParentCmd.py", line 437, in testParentToProxyShape
    self, sphereWorldListPre, matrixToList(sphereWorld), places=6)
  File "E:\Dev\usd\_workspaces_latest\Maya_USD_PR_PR\build\RelWithDebInfo\test\python\testUtils.py", line 31, in assertVectorAlmostEqual
    testCase.assertAlmostEqual(va, vb, places)
AssertionError: 0.0 != -3.745325503251487 within 6 places

----------------------------------------------------------------------
Ran 9 tests in 1.947s

FAILED (failures=3)

kxl-adsk · 2021-01-28T11:22:58Z

@ppt-adsk Removing ready-for-merge label for now, since we still have to understand this test failure with the last Maya PR.

…trix

kxl-adsk · 2021-02-05T11:45:53Z

The only failure comes from our signing server hang. Marking this as ready for merge.

kxl-adsk · 2021-02-05T11:47:10Z

Also, the remaining test failures were fixed by 4a6c729

Implement and test setMatrixCmd() for single matrix op prims.

cdc1db9

ppt-adsk requested a review from kxl-adsk January 22, 2021 19:13

ppt-adsk commented Jan 22, 2021

View reviewed changes

kxl-adsk approved these changes Jan 23, 2021

View reviewed changes

kxl-adsk added the ufe-usd Related to UFE-USD plugin in Maya-Usd label Jan 23, 2021

kxl-adsk suggested changes Jan 25, 2021

View reviewed changes

Addressed code review feedback.

56c2188

kxl-adsk approved these changes Jan 26, 2021

View reviewed changes

kxl-adsk assigned ecp-maya-devops-adsk Jan 26, 2021

clang-format fix.

b04addb

ppt-adsk assigned ecp-maya-devops-adsk and unassigned ecp-maya-devops-adsk Jan 27, 2021

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

kxl-adsk removed the ready-for-merge Development process is finished, PR is ready for merge label Jan 28, 2021

ppt-adsk added 2 commits February 4, 2021 14:16

Add undo / redo workaround for PR122 support.

4a6c729

Merge branch 'dev' into tremblp/MAYA-108692/parent_absolute_single_ma…

ea3922f

…trix

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

kxl-adsk assigned ecp-maya-devops-adsk and unassigned ecp-maya-devops-adsk Feb 5, 2021

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

kxl-adsk merged commit c6179c0 into dev Feb 5, 2021

kxl-adsk deleted the tremblp/MAYA-108692/parent_absolute_single_matrix branch February 5, 2021 11:54

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement and test setMatrixCmd() for single matrix op prims. #1102

Implement and test setMatrixCmd() for single matrix op prims. #1102

ppt-adsk commented Jan 22, 2021

ppt-adsk Jan 22, 2021

ppt-adsk Jan 22, 2021

ppt-adsk Jan 22, 2021

ppt-adsk Jan 22, 2021

mattyjams Jan 22, 2021

ppt-adsk Jan 22, 2021

mattyjams Jan 22, 2021

kxl-adsk Jan 23, 2021

ppt-adsk Jan 22, 2021

ppt-adsk Jan 22, 2021

kxl-adsk Jan 23, 2021

ppt-adsk Jan 22, 2021

kxl-adsk Jan 23, 2021

kxl-adsk Jan 23, 2021

ppt-adsk Jan 25, 2021

kxl-adsk Jan 23, 2021

kxl-adsk Jan 23, 2021

kxl-adsk Jan 23, 2021

kxl-adsk left a comment

kxl-adsk commented Jan 28, 2021

kxl-adsk commented Feb 5, 2021

kxl-adsk commented Feb 5, 2021

		@@ -26,6 +26,8 @@

		#include <memory>

		PXR_NAMESPACE_USING_DIRECTIVE

Implement and test setMatrixCmd() for single matrix op prims. #1102

Implement and test setMatrixCmd() for single matrix op prims. #1102

Conversation

ppt-adsk commented Jan 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

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

kxl-adsk commented Jan 28, 2021

kxl-adsk commented Feb 5, 2021

kxl-adsk commented Feb 5, 2021