[al] Performance improvements when processing prim meta data #245

robthebloke · 2020-02-04T05:28:10Z

Previously we a mishmash of code buried within the ProxyShape that would handle 3 separate meta data tags we could apply to a prim within a scene. These are:

al_usdmaya_excludeFromProxyShape

If this boolean value is true, the prim would be added to an excluded geometry list that we then pass to UsdImaging to disable the rendering of these prims within Hydra.

al_usdmaya_lock

This meta data tag affects the transform chains created when importing maya nodes via either the translator framework, or the translate prim command. It controls whether the transform attributes (scale, rotate, etc) are locked after creation. There are 3 possible values for this meta data:

inherited : This is the default. If set, the transform will be locked or unlocked based on the lock status of it's parent nodes. If the prim has no parent, then the default state is unlocked.
transform : A poor choice of name, a better name would be "locked". If this is set, the transform (and any child transforms which are set to inherited) will have their transform attributes locked.
unlocked : If set, the transform attributes of this node (and any child transforms with the 'inherited' lock status) will be unlocked and free to be modified.

al_usdmaya_selectability

This meta data tag controls whether it's possible to select a prim (and it's children) within the Maya viewport. We use this at AL to say, prevent animators from accidentally selecting the environments whilst animating. This can have two values:

selectable : This is the default. If set, the prim can be selected in the maya viewport.
unselectable : If set, the prim (and all child prims) will be unselectable in the viewport.

Problems this PR addreses

The motivating factor for the PR was that we found the previous code used to handle the meta data did not scale to very large set sizes. This PR primarily addresses those performance problems, has fixed a few bugs and inconsistencies along the way, and additional tests have been added. In general, these are the main aims:

Removed the performance bottlenecks that occurred after a variant switch when the lock and selectability states needed to be updated.
The TranslatePrim command did not respect the meta data tags.
Ensured that changes to the meta data on a prim within a proxy shape, will correctly be reflected within Maya.

Code changes

Previously the code used to build up and modify the internal meta data lists the was generalised behind the HierarchyLogics interface. Sadly there are subtle differences in the way each of those meta data items need to be handled, and so generalising the code simply lead to a fairly inefficient process needed to update and maintain the internal databases. We've removed the base class, and have split the three meta data tags into their own classes (with an eye on maximising performance of each one)
Added the optionVar AL_usdmaya_ignoreLockPrims, to disable all of the above features (If you are not using the above functionality, I'd suggest enabling the optionvar - it somewhat improves performance!)
Added a new LockManager to maintain the locked/unlocked status of prims. We went through a number of iterations of this class, eventually settling on a version that stores two SdfPath arrays: locked and unlocked. These lists are sorted internally so that we can quickly determine any lock prim state quickly with 2x std::lower_bound calls, and then a simple check to see which path is the longest (i.e. the nearest parent).
Added a 3rd argument for the TransformIterator which allows it to stop iterating into an instance (we can't select prims within a master prim, only the highest instance level, so there isn't any point us recursing further down the tree). By default the iterator will iterate into instances (the same behaviour as before)
Changed the way that paths were being stored within the SelectabilityDB to improve the performance of querying and modifying the list of paths.
Added extra tests to cover some of the edge cases we found during testing.

+ Added a new LockManager to maintain the locked/unlocked status of prims in the Scene + Added ability for TransformIterator to stop when encountering a prim + Refactored the selectabilityDB to simplify the way the internal data is modified

ppt-adsk

Apologies for the somewhat superficial review. Aside from what I believe is a duplicate parentTransform() implementation, I trust the rest performs as required... And there appears to be sufficient testing to prove it, thanks a lot for that.

ppt-adsk · 2020-02-18T18:35:25Z

plugin/al/lib/AL_USDMaya/AL/usdmaya/SelectabilityDB.h

+  bool containsPath(const SdfPath& path) const
+  {
+    auto foundPathEntry = std::lower_bound(m_unselectablePaths.begin(), m_unselectablePaths.end(), path);
+    if(foundPathEntry != m_unselectablePaths.end() && path == *foundPathEntry)


Nit-pick: could just return the result of the boolean expression and avoid the conditional altogether.

return (foundPathEntry != m_unselectablePaths.end() && path == *foundPathEntry);

Fine as it is, of course.

ppt-adsk · 2020-02-18T18:41:13Z

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

@@ -77,6 +77,16 @@ typedef void (*proxy_function_prototype)(void* userData, AL::usdmaya::nodes::Pro

 const char* ProxyShape::s_selectionMaskName = "al_ProxyShape";

+//----------------------------------------------------------------------------------------------------------------------
+MDagPath ProxyShape::parentTransform()


You sure you really need this? You're inheriting from MayaUsdProxyShapeBase, which has the exact same implementation of parentTransform():

https://github.com/Autodesk/maya-usd/blob/dev/lib/nodes/proxyShapeBase.cpp#L921

ppt-adsk · 2020-02-18T18:43:31Z

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

+  {
+    bool ignoreLockPrims = MGlobal::optionVarIntValue("AL_usdmaya_ignoreLockPrims");
+    //The lock Prim functionality is a UI thing - no need to have it on in batch mode
+    //However, this also causes the tests to fail which is bad


Why do the tests fail? What's the interaction?

When we run the tests on our build server, MGlobal::mayaState() == MGlobal::kInteractive will return false, which would end up disabling the meta-data processing entirely. Not overly useful when you've written tests to test said functionality.

kxl-adsk

Please fix compilation error in strict mode + resolve conflicts with latest dev branch.

kxl-adsk · 2020-02-18T19:32:21Z

plugin/al/lib/AL_USDMaya/AL/usdmaya/fileio/TransformIterator.h

@@ -128,14 +128,15 @@ class TransformIterator

  std::vector<StackRef> m_primStack;
  UsdStageRefPtr m_stage;
+  size_t m_currentItem;


/git/usd/maya-usd/plugin/al/lib/AL_USDMaya/AL/usdmaya/fileio/TransformIterator.h:131:10: error: private field 'm_currentItem' is not used [-Werror,-Wunused-private-field]
size_t m_currentItem;

As well, for variables like this (and m_stopOnInstance) below, use in-class member initializers instead (https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#Rc-default)

kxl-adsk · 2020-02-24T10:52:51Z

@robthebloke please resolve conflicts.

kxl-adsk

Commit 5d0612f has change made to lib/usd/translators/CMakeLists.txt which doesn't come from the dev branch. Not sure why it was added, but it makes this PR not compile:

[ 77%] Linking CXX shared library libmayaUsd_Translators.dylib
ld: library not found for -lOpenMayaFX

Please remove this change from lib/usd/translators/CMakeLists.txt since this dependency is not needed.

robthebloke · 2020-02-27T10:47:57Z

It is not required to build the plug in, but it is required to load the plug in successfully.

kxl-adsk · 2020-02-27T12:25:43Z

It’s not something we observed, nor we have any issues logged for it. Additionally, most of Pixar tests rely on this plugin, so I really don’t see how we could have missed it. Given that this change doesn’t belong to PR you have open, let’s not sneak changes in hidden behind “merge from dev”.

kxl-adsk · 2020-02-27T12:40:46Z

@robthebloke Let's get this PR in by removing the unrelated change which causes compilation problems. Then please open a new PR with only this new change and description of the problem it fixes.

robthebloke changed the title ~~Performance improvements when processing prim meta data~~ [al] Performance improvements when processing prim meta data Feb 4, 2020

kxl-adsk requested a review from ppt-adsk February 18, 2020 18:25

ppt-adsk approved these changes Feb 18, 2020

View reviewed changes

kxl-adsk suggested changes Feb 18, 2020

View reviewed changes

code review changes

381d375

kxl-adsk approved these changes Feb 24, 2020

View reviewed changes

Merge branch 'dev' into AL20-improve_meta_data_performance

5d0612f

kxl-adsk suggested changes Feb 27, 2020

View reviewed changes

remove lib from build

b3a1059

kxl-adsk approved these changes Feb 28, 2020

View reviewed changes

kxl-adsk merged commit 8b2ebb4 into Autodesk:dev Feb 28, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[al] Performance improvements when processing prim meta data #245

[al] Performance improvements when processing prim meta data #245

robthebloke commented Feb 4, 2020

ppt-adsk left a comment

ppt-adsk Feb 18, 2020

ppt-adsk Feb 18, 2020

ppt-adsk Feb 18, 2020

robthebloke Feb 18, 2020 •

edited

Loading

kxl-adsk left a comment

kxl-adsk Feb 18, 2020

kxl-adsk commented Feb 24, 2020

kxl-adsk left a comment

robthebloke commented Feb 27, 2020

kxl-adsk commented Feb 27, 2020 via email

kxl-adsk commented Feb 27, 2020

[al] Performance improvements when processing prim meta data #245

[al] Performance improvements when processing prim meta data #245

Conversation

robthebloke commented Feb 4, 2020

al_usdmaya_excludeFromProxyShape

al_usdmaya_lock

al_usdmaya_selectability

Problems this PR addreses

Code changes

ppt-adsk left a comment

Choose a reason for hiding this comment

ppt-adsk Feb 18, 2020

Choose a reason for hiding this comment

ppt-adsk Feb 18, 2020

Choose a reason for hiding this comment

ppt-adsk Feb 18, 2020

Choose a reason for hiding this comment

robthebloke Feb 18, 2020 • edited Loading

Choose a reason for hiding this comment

kxl-adsk left a comment

Choose a reason for hiding this comment

kxl-adsk Feb 18, 2020

Choose a reason for hiding this comment

kxl-adsk commented Feb 24, 2020

kxl-adsk left a comment

Choose a reason for hiding this comment

robthebloke commented Feb 27, 2020

kxl-adsk commented Feb 27, 2020 via email

kxl-adsk commented Feb 27, 2020

robthebloke Feb 18, 2020 •

edited

Loading