[maya] factor out matching of xforms into PxrUsdMayaXformStack #287

pmolodo · 2017-09-19T01:44:00Z

Description of Change(s)

This factors out the code for defining standardized transform stacks into a new class, xformStack. This class is also responsible for checking compatibility with existing usd xform stacks. This code is also exposed through the API for use externally.

There's actually nothing in the code which is specific to maya, so in theory it could be promoted to somewhere in the usd core, but it may be premature if it's not used anywhere else yet... and, of course, I'm sure that would warrant a higher level of scrutiny.

Speaking of scrutiny - I'm planning to add some unit tests, but wanted to give a chance for comment now!

jtran56 · 2017-09-19T17:40:48Z

Filed as internal issue #151051.

pmolodo · 2017-10-06T01:32:48Z

Just pushed an update for this PR - mostly just adds python wrappers and a bunch of tests (though also renames one of the class aliases).

The commit in this PR is the consolidated version, if you want to see the incremental changes since the last push, you can see them here:

d663105
14028e0
00b102e
2148a40
8c17e76

sunyab

Hi @elrond79, passing along code review notes from the sets team. We'd be happy to chat more about the comments re: FirstMatchingSubstack if that'd be helpful. Thanks!

sunyab · 2017-10-11T17:31:51Z

third_party/maya/lib/usdMaya/CMakeLists.txt

+        MAYA_PLUG_IN_PATH=${CMAKE_INSTALL_PREFIX}/third_party/maya/plugin
+        MAYA_SCRIPT_PATH=${CMAKE_INSTALL_PREFIX}/third_party/maya/share/usd/plugins/usdMaya/resources
+        MAYA_DISABLE_CIP=1
+        MAYA_APP_DIR=./maya_profile


Could we remove the MAYA_APP_DIR in this test registration for now, pending a resolution to the issues in #209?

Oops, that was accidentally included. I'll get rid of it.

sunyab · 2017-10-11T20:16:15Z

third_party/maya/lib/usdMaya/xformStack.h

+        return std::vector<OpClassPtr>();
+    }
+
+   /// \brief Runs MatchingSubstack against the given list of stacks


The variadic template stuff here is neat, but we had some concerns about it because of the duplicated code in the Python wrapping. We'd really like to find a way to avoid that if possible.

Are there performance concerns or other reasons why this method couldn't accept something like a vector? I noticed PxrUsdMayaXformStack objects are non-copyable, but maybe we could relax that?

Well, haven't done any benchmarking, but copying the entire stare object would end up copying multiple vectors, so I'd like to avoid it. I could make it take a vector of pointers, but then I'd either have to have it take raw pointers, or switch PxrUsdMayaXformStack to use either weak or ref ptrs.

I guess right now I'm leaning toward just making PxrUsdMayaXformStack derive from TfRefBase, switch everything to use refPtrs instead of refs, and then make FirstMatchingSubstack take a vector of those.

I'm also thinking I may just switch PxrUsdMayaXformOpClassification to use refPtrs, instead of weak ptrs... both for consistency with FirstMatchingSubstack, and because when I first chose weak ptrs, and because I've since realized that refPtrs are slightly lighter-weight (other than the initial copy from static memory to heap-allocated, but that will only be done once per stack). Again, will have to update everything to use refptrs... but will probably simplify things, and don't have to make any assumptions about how things are allocated.

So, after having found that you can't (or at least, it's difficult) to python-wrap const-ref-ptrs, I think I'm just going to make FirstMatchingSubstack take a vector of raw ptrs. It feels slightly yucky to use raw pointers, but it also feels like overkill to introduce weak pointers purely so they can be used with this function. Let me know if you'd prefer weak pointers, though (or any other approach), and I'll change it.

sunyab · 2017-10-11T20:16:49Z

third_party/maya/lib/usdMaya/xformStack.h

+private:
+    PxrUsdMayaXformStack(
+            const OpClassList ops,
+            const std::vector<std::pair<size_t, size_t> > inversionTwins,


Did you mean to pass a const-ref in here instead?

Oops, yes, that should be a const-ref. Good catch.

sunyab · 2017-10-11T20:32:59Z

third_party/maya/lib/usdMaya/testenv/testUsdMayaXformStack.py

+from maya.api import OpenMaya as OM
+from maya import standalone
+
+from pxr import Usd, UsdMaya


Could you change the UsdMaya import here to look like they do in other tests, like:
https://github.com/PixarAnimationStudios/USD/blob/496775539c324af7cb2ea12ddbb80720865b6cb2/third_party/maya/lib/usdMaya/testenv/testUsdMayaReferenceAssemblyEdits.py#L26-L31

This is unfortunately an internal build issue that affects the USD repository; we'd like to fix the issue so this won't be needed in the future, but in the meantime we'd appreciate the change here

Sure, easy fix...

sunyab · 2017-10-11T20:33:54Z

third_party/maya/lib/usdMaya/CMakeLists.txt

@@ -164,6 +166,7 @@ pxr_test_scripts(
        testenv/testUsdMayaProxyShape.py
        testenv/testUsdMayaReferenceAssemblyEdits.py
        testenv/testUsdMayaUserExportedAttributes.py
+        testenv/testUsdMayaXformStack.py


Thank you for adding a test!

pmolodo · 2017-10-13T16:50:16Z

Addressed the code review notes, as well as converted over both classes to be "value types", that store a refPtr to the actual data. Since both are immutable classes, this seemed a good compromise between safety / performance (ie, refPtr is lighter weight than weakPtr) / and ease-of-python-wrapping. Thanks to Alex Mohr for the suggestion!

Once again, this is the collapsed PR - the (new) incremental changesets can be seen here:

LumaPictures@728dfed
LumaPictures@711a547
LumaPictures@40ad81b
LumaPictures@c6db468
LumaPictures@5aa4fa8

sunyab · 2017-10-17T17:19:07Z

Thanks! I've pulled these changes over again and am running them through builds now

sunyab · 2017-10-20T20:45:40Z

Unfortunately this change is causing some internal test failures for usdExport. I'm looking at this further now, I will see if I can package up the test and send it your way to get your take on it.

pmolodo · 2017-10-25T18:09:01Z

Ok... so the test break is due to specifying rotateAxis using a single-axis (rotateX) xformOp. I had set rotateAxis to only accept rotateXYZ ops... because, unlike the "rotate" xform, it has no support for changing the rotation order (and, in fact, under the hood it's stored as a quaternion anyway!)... but it should at least be able to accept single-axis rotates, if not things like rotateZXY.

However... on thinking about it a bit more, I think this highlights one of the current shortcomings of my design - the maya-centric way in which it's handling rotation order. Granted, it's currently only used for Maya... but it was designed in a way that should be usable by other applications. For rotation order, though, it assumes that is set based off of the "rotate" op... and in fact, it sets a maya-only class (MTransformationMatrix::RotationOrder) to indicate what it what set to.

In a more general case, it's entirely possible for an xform stack to have 2 (or more) rotation ops, all of which may have their own independent rotation orders... so I think I'm going to get rid of handling of rotation-order as part of the API. Instead, the paradigm will be:

any 3-axis rotate in a stack is compatible with any-other 3-axis rotate (or 1-axis rotate)... since, at minimum, it is always possible to convert between them
it will be the responsibility of the user of the API to check the rotate-order of the matched xforms, and either set a rotate order in their app, or convert, as appropriate to their context
therefore, the optional RotationOrder* pointer will be dropped from the API for MatchingSubstack / FirstMatchingSubstack

I should have these changes (and some more tests) by EOD

spiffmon · 2017-10-25T19:40:46Z

Hi Paul, Apologies as I haven't reviewed the code, but based on the last set of comments about rotationOrder, I just wanted to confirm that it will still be possible, using the new API, to encode triple-axis rotations in USD, as they will be three times faster to read than three single-axis, which adds up at scale.

…

--Spiff

On Oct 25, 2017, at 11:09 AM, Paul Molodowitch ***@***.***> wrote: Ok... so the test break is due to specifying rotateAxis using a single-axis (rotateX) xformOp. I had set rotateAxis to only accept rotateXYZ ops... because, unlike the "rotate" xform, it has no support for changing the rotation order (and, in fact, under the hood it's stored as a quaternion anyway!)... but it should at least be able to accept single-axis rotates, if not things like rotateZXY. However... on thinking about it a bit more, I think this highlights one of the current shortcomings of my design - the maya-centric way in which it's handling rotation order. Granted, it's currently only used for Maya... but it was designed in a way that should be usable by other applications. For rotation order, though, it assumes that is set based off of the "rotate" op... and in fact, it sets a maya-only class (MTransformationMatrix::RotationOrder) to indicate what it what set to. In a more general case, it's entirely possible for an xform stack to have 2 (or more) rotation ops, all of which may have their own independent rotation orders... so I think I'm going to get rid of handling of rotation-order as part of the API. Instead, the paradigm will be: any 3-axis rotate in a stack is compatible with any-other 3-axis rotate (or 1-axis rotate)... since, at minimum, it is always possible to convert between them it will be the responsibility of the user of the API to check the rotate-order of the matched xforms, and either set a rotate order in their app, or convert, as appropriate to their context therefore, the optional RotationOrder* pointer will be dropped from the API for MatchingSubstack / FirstMatchingSubstack I should have these changes (and some more tests) by EOD — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub, or mute the thread.

pmolodo · 2017-10-25T22:56:36Z

So, made the changes I mentioned above. rotateAxis can now handle single-axis rotations, as well as will automatically cross-convert from non-XYZ-ordered triple-axis rotates.

@spiffmon absolutely, triple-axis rotates are still recorded as such. I just meant that if you have a USD file that records it's rotateAxis as rotateYZX, this new version will automatically convert that to rotateXYZ on import, which is the only format that transform nodes will accept as input. It means that the maya stack is now slighly more tolerant (though an xformOp:rotateYZX:rotateAxis will not round-trip - ie, it will import fine, and maintain the same transformation / rotation, but when you re-ouput a new USD, it will be xformOp:rotateXYZ:rotateAxis).

pmolodo · 2017-10-25T22:58:48Z

Oh, also, in case you want to see incremental commits:

LumaPictures@9bb528b
LumaPictures@3215095

sunyab

Our sets team had a few more notes in review, mostly just cosmetic cleanup and documentation updates. There's one larger note about not using exceptions in the codebase, but this should be hopefully easily replaced with TF_VERIFY.

Thanks!

sunyab · 2017-11-11T00:25:08Z

third_party/maya/lib/usdMaya/testenv/testUsdMayaXformStack.py

@@ -0,0 +1,1161 @@
+#!/pxrpythonsubst
+#
+# Copyright 2016 Pixar


sunyab · 2017-11-11T00:27:44Z

third_party/maya/lib/usdMaya/xformStack.h

+
+    PxrUsdMayaXformStack(
+            const OpClassList& ops,
+            const std::vector<std::pair<size_t, size_t> >& inversionTwins,


Should this be const std::vector<IndexPair>& ...?

sunyab · 2017-11-11T00:29:08Z

third_party/maya/lib/usdMaya/xformStack.h

+    OpClassList const & GetOps() const;
+
+    PXRUSDMAYA_API
+    std::vector<std::pair<size_t, size_t> > const & GetInversionTwins() const;


Should we use const std::vector<IndexPair>& ... here as well?

sunyab · 2017-11-11T00:30:14Z

third_party/maya/lib/usdMaya/xformStack.h

+    /// \param  opName the name of the operator classification we  wish to find
+    /// \param  isInvertedTwin the returned op classification object must match
+    ///         this param for it's IsInvertedTwin() - if an op is found that matches
+    ///         the name, but has the wrong invertedTwin status, nullptr is returned


Looks like this should be "NO_INDEX is returned" instead of "nullptr is returned"

sunyab · 2017-11-11T00:30:27Z

third_party/maya/lib/usdMaya/xformStack.h

+    /// \param  isInvertedTwin the returned op classification object must match
+    ///         this param for it's IsInvertedTwin() - if an op is found that matches
+    ///         the name, but has the wrong invertedTwin status, nullptr is returned
+    /// return  Undex to the op classification object with the given name (and


Minor typo: "Undex" instead of "Index"

sunyab · 2017-11-11T00:36:34Z

third_party/maya/lib/usdMaya/xformStack.cpp

+
+    _PxrUsdMayaXformStackData(
+            const OpClassList& ops,
+            const std::vector<std::pair<size_t, size_t> >& inversionTwins,


const std::vector<IndexPair>& ... here?

sunyab · 2017-11-11T00:36:54Z

third_party/maya/lib/usdMaya/xformStack.cpp

+    return _sharedData->m_ops;
+}
+
+std::vector<std::pair<size_t, size_t> > const &


const std::vector<IndexPair>& ... here?

sunyab · 2017-11-11T00:43:25Z

third_party/maya/lib/usdMaya/xformStack.cpp

+                // Insert, and check if it already existed
+                if (!result.insert({attrName, indexPair}).second)
+                {
+                    // The attrName was already in the lookup map...


Our coding convention is to not use exceptions in our code, so we'd prefer not to throw here. If this is something that should never happen (i.e., it's an invariant in the code and not reliant on user input), we prefer to use TF_VERIFY to indicate that something is wrong, like:

TF_VERIFY(result.insert({attrName, indexPair}).second, "AttrName %s already found in attrName lookup map", attrName.GetText());

This will cause the error output to be emitted if the condition is false, but allows the code to continue executing so that it doesn't cause the program to crash.

sunyab · 2017-11-11T00:44:37Z

third_party/maya/lib/usdMaya/xformStack.cpp

+                std::string msg = TfStringPrintf(
+                        "Op classification name %s already found in op lookup map",
+                        op.GetName().GetText());
+                throw std::invalid_argument(msg);


See note about exceptions and TF_VERIFY above

sunyab · 2017-11-11T00:49:55Z

third_party/maya/lib/usdMaya/xformStack.cpp

+        {
+            const PxrUsdMayaXformOpClassification& first = m_ops[pair.first];
+            const PxrUsdMayaXformOpClassification& second = m_ops[pair.second];
+            if (first.GetName() != second.GetName())


See note about exceptions and TF_VERIFY above

pmolodo · 2017-11-16T23:32:12Z

Ok, addressed the notes. Incremental commit can be found here:

LumaPictures@bf06845

Also made one other small tweak - marking the python wrapper for XformStack as noncopyable:

LumaPictures@cdac1b8

[maya] factor out matching of xforms into PxrUsdMayaXformStack

sunyab · 2017-11-18T19:35:09Z

Just wanted to note I had added a follow-up commit ddd583f to fix a build issue on Windows. I also had to remove the noncopyable change to the XformStack's Python wrapping, as it caused the associated unit test to fail.

pmolodo · 2017-11-21T01:20:26Z

Doh - my mistake. Completely forgot that I had switched to the shared-data-ref-ptr paradigm, and that copies were now ok / preferred! In any case, thanks!

pmolodo force-pushed the pr/xformStack branch from d663105 to edf5ab4 Compare October 6, 2017 01:29

pmolodo force-pushed the pr/xformStack branch from edf5ab4 to 45172c6 Compare October 6, 2017 02:15

pmolodo mentioned this pull request Oct 6, 2017

Adds option to euler filter output transform data #299

Merged

fabal mentioned this pull request Oct 10, 2017

Extra transforms being introduced on selection AnimalLogic/AL_USDMaya#24

Open

sunyab requested changes Oct 11, 2017

View reviewed changes

pmolodo added a commit to LumaPictures/USD that referenced this pull request Oct 13, 2017

[maya] xformStack - address simple notes from PixarAnimationStudios#287

728dfed

pmolodo force-pushed the pr/xformStack branch from 45172c6 to c0f2461 Compare October 13, 2017 16:44

pmolodo force-pushed the pr/xformStack branch from c0f2461 to afa18ee Compare October 25, 2017 22:51

sunyab requested changes Nov 11, 2017

View reviewed changes

pmolodo force-pushed the pr/xformStack branch from afa18ee to 129d38b Compare November 16, 2017 20:56

[maya] factor out matching of xforms into PxrUsdMayaXformStack

007e6c0

pmolodo force-pushed the pr/xformStack branch from 129d38b to 007e6c0 Compare November 16, 2017 23:29

pixar-oss merged commit 007e6c0 into PixarAnimationStudios:dev Nov 18, 2017

pixar-oss added a commit that referenced this pull request Nov 18, 2017

Merge pull request #287 from elrond79/pr/xformStack

11d9332

[maya] factor out matching of xforms into PxrUsdMayaXformStack

chadrik mentioned this pull request Dec 20, 2017

Change transform translation to use PxrUsdMayaXformStack, the same object used in usd plugin AnimalLogic/AL_USDMaya#43

Merged

pmolodo deleted the pr/xformStack branch March 12, 2019 23:03

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[maya] factor out matching of xforms into PxrUsdMayaXformStack #287

[maya] factor out matching of xforms into PxrUsdMayaXformStack #287

pmolodo commented Sep 19, 2017

jtran56 commented Sep 19, 2017

pmolodo commented Oct 6, 2017 •

edited

Loading

sunyab left a comment

sunyab Oct 11, 2017

pmolodo Oct 11, 2017

sunyab Oct 11, 2017

pmolodo Oct 11, 2017

pmolodo Oct 12, 2017

sunyab Oct 11, 2017

pmolodo Oct 11, 2017

sunyab Oct 11, 2017

pmolodo Oct 11, 2017

sunyab Oct 11, 2017

pmolodo commented Oct 13, 2017

sunyab commented Oct 17, 2017

sunyab commented Oct 20, 2017

pmolodo commented Oct 25, 2017

spiffmon commented Oct 25, 2017 via email

pmolodo commented Oct 25, 2017 •

edited

Loading

pmolodo commented Oct 25, 2017

sunyab left a comment

sunyab Nov 11, 2017

sunyab Nov 11, 2017

sunyab Nov 11, 2017

sunyab Nov 11, 2017

sunyab Nov 11, 2017

sunyab Nov 11, 2017

sunyab Nov 11, 2017

sunyab Nov 11, 2017

sunyab Nov 11, 2017

sunyab Nov 11, 2017

pmolodo commented Nov 16, 2017

sunyab commented Nov 18, 2017

pmolodo commented Nov 21, 2017

[maya] factor out matching of xforms into PxrUsdMayaXformStack #287

[maya] factor out matching of xforms into PxrUsdMayaXformStack #287

Conversation

pmolodo commented Sep 19, 2017

Description of Change(s)

jtran56 commented Sep 19, 2017

pmolodo commented Oct 6, 2017 • edited Loading

sunyab 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

pmolodo commented Oct 13, 2017

sunyab commented Oct 17, 2017

sunyab commented Oct 20, 2017

pmolodo commented Oct 25, 2017

spiffmon commented Oct 25, 2017 via email

pmolodo commented Oct 25, 2017 • edited Loading

pmolodo commented Oct 25, 2017

sunyab 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

pmolodo commented Nov 16, 2017

sunyab commented Nov 18, 2017

pmolodo commented Nov 21, 2017

pmolodo commented Oct 6, 2017 •

edited

Loading

pmolodo commented Oct 25, 2017 •

edited

Loading