USDGeomScope support in Proxy Shape #185

murphyeoin · 2019-12-20T06:29:49Z

Description

Currently any Prim of type USDGeomScope will get translated to an AL_USDMaya_Transform on any relevant Proxy Shape related operation e.g;

AL_usdmaya_ProxyShapeImportAllTransforms
AL_usdmaya_TranslatePrim
transiently on selection
etc.

We want to avail of 2 characteristics of Scopes when translating to maya:

Their maya transforms are locked
They cannot be animated and therefore don't need a time input

We now translate those to a new MPxTransform derived node called "Scope"

What was done?

created Scope MPxNode
created BasicTransformationMatrix which is used by the Scope MPxNode
have the original Transform and TransformationMatrix inherit from them as they are relatively simple and lightweight
updated any code which makes assumptions about AL_usdmaya_transform being the only transform type (most of this is ProxyShape/Selection related) and ideally use the base Scope class pointer
update code to create Scope nodes where appropriate (i.e mostly when translating from USDGeomScope nodes)
refactored the transform node creation in ProxyShapeSelection - there were 2 methods performing more or less the same node creation
add/update some tests etc

Doing this work made me remember we have a weird hack in AL_USDMaya which allows creation of arbitrary transform nodes. This works by tagging an Xform in USD with the
"al_usdmaya_transformType" metadatum... It allows you to create custom transform types without needing to add a proper factory/plugin system or create an explicit dependency on the type - basically a poor man's factory. How this stuff interacts with AL_usdmaya transforms and scopes is anyone's guess really. At least the code is mostly in one place now

Future work

add standard import/export functionality for Scopes - wasn't needed by us, but should be done to make the feature complete
as mentioned above the effort could be made to refactor the code to make transforms a plugin - currently the code for supporting AL_USDMaya_Transform, AL_USDMaya_Scope, and user-defined types is smeared a bit all over the codebase - although this refactor has made it a bit more general (most code should work with any type that derives from Scope).
Consider having an interface for the custom Transform types (currently Scope, Transform) and matrices (BasicTransformationMatrix, TransformationMatrix) rather than having one inherit from the other

* create Scope MPxNode, MTransformationMatrix, add/update some tests etc * filled in a few missing methods and conditionals - tests are passing now * fleshing out scope behaviour * scope tests now working as expected * selection store support for scope * add todos * basic bounding box working - spliced in Dan Englessons improvements to boundingBox() and a mixed hierarchy of scope/xforms/geom seems to frame in maya-2019 * support time * rename transform() method to getTransformNode() to make it easier to find * Transform now subclasses from Scope * refactored Scope/Transform nodes and their TransformationMatrix classes to an inheritance relationship to support nicer handling of new/multiple types * addressed code review comments * replace transform2() method with more sensible getTransMatrix() * show node type when print refcounts * addressed hopefully final review comnents * refactor proxy select node creation logic, prioritise custom node types over AL_USDMaya scope/transform * extract mayaNodeCreation from makeUsdTransformChain for eventual reuse * update makeUsdTransformsInternal to use new createMayaNode method * dynamic cast when requesting transform type to avoid problems with custom types * remove todos, update unit test * A slight change so that Transform now inherits the attributes from Scope, rather than making new copies of them. * Check for both Scope::kTypeId and Transform::kTypeId * reordered the node registration

kxl-adsk · 2020-01-21T17:40:08Z

@pilarmolinalopez can you help review this change?

fowlertADSK · 2020-01-22T17:24:17Z

Hey Eoin. I started writing up some comments/questions as part of a review and I think the direction most of my thoughts were going were towards things like how general the transform classes are and if some of this might belong in a common area instead of just being in the AL plugin (similar to the discussion in PR #158). I’ve been slowed down by a bunch of other stuff, but maybe you can answer that question before I spend too much time on it. Do we think that there is functionality here that would be nice to have down in the core lib next to or as part of the proxy base class that is shared with the other plugins?

I think the answer is “Yes, eventually”, but right now it’s only the AL plugin that would use it.

murphyeoin · 2020-01-23T02:39:12Z

Hi Tim, in this case the functionality is very AL Proxy Shape live interaction workflow specific - there's not even a general maya->USD Scope importer/exporter..

kxl-adsk · 2020-01-23T14:53:02Z

Totally makes sense, Eoin.

fowlertADSK · 2020-01-24T02:58:02Z

plugin/al/lib/AL_USDMaya/AL/usdmaya/nodes/ProxyShape.cpp

@@ -1637,7 +1637,9 @@ bool ProxyShape::lockTransformAttribute(const SdfPath& path, const bool lock)
  r.setLocked(lock);
  s.setLocked(lock);

-  if (lock && MFnDependencyNode(lockObject).typeId() == AL_USDMAYA_TRANSFORM)
+  MFnDependencyNode fn(lockObject);
+  Scope* transformNode = static_cast<Scope*>(fn.userNode());


Should this one and the one on line 2144 be dynamic_cast?

I think you're right -It would be OK to use static_cast if the data had been pre-filtered to only allow Scopes or classes derived from it, or we were doing some kind of checking earlier on in the method, but as "lockObject" can be any maya object we should either be doing those checks or using a dynamic_cast to check that it's a valid Scope at run -time... (add some minor performance cost). Will update. Thanks!

fowlertADSK · 2020-01-24T03:05:53Z

plugin/al/lib/AL_USDMaya/AL/usdmaya/nodes/Scope.cpp

+      // that has ANY attribute that connected to rotateAxis, it will cause the rotateAxis to evaluate INCORRECTLY,
+      // even on the BASE transform class!
+      // See this gist for full reproduction details:
+      //   https://gist.github.com/elrond79/f9ddb277da3eab2948d27ddb1f84aba0


Do you happen to know if we have an internal bug for this? If not we should get one logged. It does sound familiar.

Not sure...(If that was addressed to me)

fowlertADSK · 2020-01-24T03:30:21Z

Appreciate the added tests for the new class, have you only been able to run them on Linux? If not, I'll build it and run them locally tomorrow morning.

murphyeoin · 2020-01-24T05:40:07Z

Hi Tim, yes have only been able to run on Linux. This code has been in production for a while though, so I think it's mostly error free

pilarmolinalopez

I have the same question as Tim has regarding static_cast vs dynamic_cast in those lines. Everything else looks good, I'll mark it as approved on my part waiting on that.

I was also able to build it and run the unit tests in Linux.

fowlertADSK · 2020-01-24T19:42:17Z

plugin/al/lib/AL_USDMaya/AL/usdmaya/nodes/Transform.cpp

-    m_primPath = addStringAttr("primPath", "pp", kReadable | kWritable | kStorable | kConnectable | kAffectsWorldSpace, true);
-    m_inStageData = addDataAttr("inStageData", "isd", MayaUsdStageData::mayaTypeId, kWritable | kStorable | kConnectable | kHidden | kAffectsWorldSpace);
+    addInheritedAttr("primPath");
+    addInheritedAttr("inStageData");


FYI I am spending some time today trying to get it built and see if the tests pass. This is a line you'll have problems with. Paul removed this function and replaced it with something slightly different. See commit 476826f

fowlertADSK · 2020-01-24T20:15:18Z

I managed to pull your changes into the branch where Hamed and I were working on getting the tests to run/pass on Windows and I think I got a successful run (after changing a couple lines where the functions you were using had been removed). You'll have to update those but then I'd be ready to close this one.

murphyeoin · 2020-01-28T04:44:54Z

@fowlertADSK @kxl-adsk addressed comments, merged latest dev

kxl-adsk

Please fix compilation errors in strict mode

kxl-adsk · 2020-01-28T14:15:26Z

plugin/al/lib/AL_USDMaya/AL/usdmaya/nodes/TransformationMatrix.h

+
+  /// \brief  return the prim this transform matrix is attached to
+  /// \return the prim this transform matrix is controlling
+  inline const UsdPrim& prim() const


/git/usd/maya-usd/plugin/al/lib/AL_USDMaya/AL/usdmaya/nodes/TransformationMatrix.h:310:25: error: 'prim' overrides a member function but is not marked 'override' [-Werror,-Winconsistent-missing-override]
inline const UsdPrim& prim() const
^
/git/usd/maya-usd/plugin/al/lib/AL_USDMaya/AL/usdmaya/nodes/BasicTransformationMatrix.h:73:26: note: overridden virtual function is here
virtual const UsdPrim& prim() const

In addition, there is no value in making the virtual method inlined.

kxl-adsk · 2020-01-28T14:19:54Z

plugin/al/lib/AL_USDMaya/AL/usdmaya/nodes/Scope.h

+  //--------------------------------------------------------------------------------------------------------------------
+  /// Data members
+  //--------------------------------------------------------------------------------------------------------------------
+  bool updateTransformInProgress = false;


/git/usd/maya-usd/plugin/al/lib/AL_USDMaya/AL/usdmaya/nodes/Scope.h:110:8: error: private field 'updateTransformInProgress' is not used [-Werror,-Wunused-private-field]
bool updateTransformInProgress = false;

kxl-adsk · 2020-01-28T14:23:06Z

plugin/al/plugin/AL_USDMayaTestPlugin/AL/usdmaya/nodes/test_ScopeMatrix.cpp

+      if (ptr->typeId()==AL::usdmaya::nodes::Transform::kTypeId)
+      {
+        //It's a transform
+        AL::usdmaya::nodes::BasicTransformationMatrix* matrix = ptr->transform();


/git/usd/maya-usd/plugin/al/plugin/AL_USDMayaTestPlugin/AL/usdmaya/nodes/test_ScopeMatrix.cpp:130:56: error: unused variable 'matrix' [-Werror,-Wunused-variable]
AL::usdmaya::nodes::BasicTransformationMatrix* matrix = ptr->transform();

kxl-adsk · 2020-01-28T14:48:26Z

@murphyeoin attaching patch I used to fix compilation errors.
strict_mode_patch.txt

murphyeoin · 2020-01-28T20:37:23Z

Thanks @kxl-adsk can you confirm that running the compiler in strict mode is part of your standard testing process? If so should we just make it part of the build? (perhaps as an additional target? )

kxl-adsk · 2020-01-28T21:08:16Z

Yes, we run the compiler in strict mode on all platforms. You can pass it to build.py like this --build-args=",-DPXR_STRICT_BUILD_MODE=1"

murphyeoin · 2020-01-29T03:50:07Z

Hi @kxl-adsk I fixed them in our fork and have updated. I will add the strict mode compile to our build script as the aim is that we're all testing the same thing. Any other flags in your build-args that we should know about? Thanks

kxl-adsk · 2020-01-29T10:01:01Z

At the moment, no, there is nothing else.

kxl-adsk

Looks good. Thank you!

murphyeoin added 5 commits December 20, 2019 09:31

fix merge conflicts

25ee4d1

conform merge

4f2ada4

conform merge 2

454ce84

last cleanup

ced08d1

kreppene pushed a commit to kreppene/maya-usd that referenced this pull request Jan 7, 2020

MAYA-99904 respect proxy transformation and visibility (Autodesk#185)

0e23a73

fowlertADSK self-requested a review January 21, 2020 18:20

kxl-adsk requested a review from pilarmolinalopez January 22, 2020 18:33

fowlertADSK reviewed Jan 24, 2020

View reviewed changes

pilarmolinalopez approved these changes Jan 24, 2020

View reviewed changes

fowlertADSK reviewed Jan 24, 2020

View reviewed changes

murphyeoin added 3 commits January 28, 2020 15:25

address PR comments

8c86363

Merge branch 'dev' into Scope

074f9a5

Merge branch 'dev' into Scope

ecb6a77

fowlertADSK approved these changes Jan 28, 2020

View reviewed changes

kxl-adsk suggested changes Jan 28, 2020

View reviewed changes

fix errors shown in strict mode compile

ff2d17d

kxl-adsk approved these changes Jan 29, 2020

View reviewed changes

kxl-adsk merged commit bda180b into Autodesk:dev Jan 29, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

USDGeomScope support in Proxy Shape #185

USDGeomScope support in Proxy Shape #185

murphyeoin commented Dec 20, 2019 •

edited

Loading

kxl-adsk commented Jan 21, 2020

fowlertADSK commented Jan 22, 2020

murphyeoin commented Jan 23, 2020

kxl-adsk commented Jan 23, 2020

fowlertADSK Jan 24, 2020

murphyeoin Jan 25, 2020

fowlertADSK Jan 24, 2020

murphyeoin Jan 24, 2020

fowlertADSK commented Jan 24, 2020

murphyeoin commented Jan 24, 2020

pilarmolinalopez left a comment

fowlertADSK Jan 24, 2020

fowlertADSK commented Jan 24, 2020

murphyeoin commented Jan 28, 2020

kxl-adsk left a comment

kxl-adsk Jan 28, 2020

kxl-adsk Jan 28, 2020

kxl-adsk Jan 28, 2020

kxl-adsk Jan 28, 2020

kxl-adsk commented Jan 28, 2020

murphyeoin commented Jan 28, 2020

kxl-adsk commented Jan 28, 2020

murphyeoin commented Jan 29, 2020

kxl-adsk commented Jan 29, 2020

kxl-adsk left a comment

USDGeomScope support in Proxy Shape #185

USDGeomScope support in Proxy Shape #185

Conversation

murphyeoin commented Dec 20, 2019 • edited Loading

Description

Future work

kxl-adsk commented Jan 21, 2020

fowlertADSK commented Jan 22, 2020

murphyeoin commented Jan 23, 2020

kxl-adsk commented Jan 23, 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

fowlertADSK commented Jan 24, 2020

murphyeoin commented Jan 24, 2020

pilarmolinalopez left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fowlertADSK commented Jan 24, 2020

murphyeoin commented Jan 28, 2020

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

kxl-adsk commented Jan 28, 2020

murphyeoin commented Jan 28, 2020

kxl-adsk commented Jan 28, 2020

murphyeoin commented Jan 29, 2020

kxl-adsk commented Jan 29, 2020

kxl-adsk left a comment

Choose a reason for hiding this comment

murphyeoin commented Dec 20, 2019 •

edited

Loading