MAYA-99006 Filter out objects to export by hierarchy. #657

fowlertADSK · 2020-07-14T11:17:49Z

Extracted the code to figure out the set of objects to export from the translator and commands into a single utility function.
Added code to filter out any objects that also have parent objects in the list. This was previously an error in the write job and would error out.

Extracted the code to figure out the set of objects to export from the translator and commands into a single utility function. Added code to filter out any objects that also have parent objects in the list. This was previously an error in the write job and would error out.

fowlertADSK · 2020-07-14T11:18:34Z

lib/mayaUsd/commands/baseExportCommand.cpp

-    if (argData.isFlagSet(kSelectionFlag)) {
-        MGlobal::getActiveSelectionList(objSelList);
-    }
-    else {
-        argData.getObjects(objSelList);
-
-        // If no objects specified, then get all objects at DAG root
-        if (objSelList.isEmpty()) {
-            objSelList.add("|*", true);
-        }
-    }
-
-    // Convert selection list to jobArgs dagPaths
    UsdMayaUtil::MDagPathSet dagPaths;
-    for (unsigned int i=0; i < objSelList.length(); i++) {
-        MDagPath dagPath;
-        status = objSelList.getDagPath(i, dagPath);
-        if (status == MS::kSuccess)
-        {
-            dagPaths.emplace(dagPath);
-        }
+    bool exportSelected = argData.isFlagSet(kSelectionFlag);
+    if (!exportSelected) {
+        argData.getObjects(objSelList);
    }
+    GetFilteredSelectionToExport(exportSelected, objSelList, dagPaths);


This bit where we get an object list and add to a set for export has been moved to util.h/.cpp as a utility function shared with the translator. The one thing that the command can do differently is get a list of objects explicitly passed to the command to work on.

fowlertADSK · 2020-07-14T11:19:12Z

plugin/adsk/plugin/exportTranslator.cpp

+    // If we are in neither of these modes then there won't be anything to do
+    if (mode != MPxFileTranslator::kExportActiveAccessMode && 
+        mode != MPxFileTranslator::kExportAccessMode) {
+        return MS::kSuccess;
+    }


The logic farther down used to be that we would only fill the input list if one of these two flags were set. If neither were set then we had nothing to export and just returned kSuccess. Moving this check to the start since there is no point in continuing if we are in a different mode.

fowlertADSK · 2020-07-14T11:19:54Z

test/lib/usd/translators/testUsdExportSelectionHierarchy.py

+            self.assertFalse(prim.IsValid())
+
+if __name__ == '__main__':
+    unittest.main(verbosity=2)


The test file just has a bunch of spheres parented in a hierarchy. The test selects things in a certain order where some objects lower in the hierarchy come first, which tests that we are correctly sorting the hierarchy in order to determine the top level ancestors to export. This was previously an error case with no file exported, now it correctly filters out the children, exports the file, and then we make sure we see the expected objects read back in from usd.

fowlertADSK · 2020-07-14T11:20:46Z

lib/mayaUsd/utils/util.h

-        return strcmp(lhs.fullPathName().asChar(), rhs.fullPathName().asChar()) < 0;
+        int pathCountDiff = lhs.pathCount() - rhs.pathCount();
+        return (0 != pathCountDiff) ? (pathCountDiff < 0) :
+                (strcmp(lhs.fullPathName().asChar(), rhs.fullPathName().asChar()) < 0);


Changing the sorting function to more explicitly sort by hierarchy depth, and then alphabetically within the same depth. End result is the same because of the path separator used in the fullPathName, but adding the pathCount check makes it more obvious that it’s doing this. It would also be faster this way since the path count is a stored value internally so it’s a quick int compare in a lot of cases, avoiding a bunch of string compares.

fowlertADSK · 2020-07-14T11:21:23Z

lib/mayaUsd/utils/util.cpp

@@ -2101,3 +2133,48 @@ UsdMayaUtil::nameToDagPath(const std::string& name)
    CHECK_MSTATUS(status);
    return dag;
 }
+
+void 
+UsdMayaUtil::GetFilteredSelectionToExport(bool exportSelected, MSelectionList& objectList, UsdMayaUtil::MDagPathSet& dagPaths)


Had hoped that there was an easy API call or Maya command to do what I want here but I never found anything. Most of the code I found that did something similar ended up traversing the entire Maya scene using various dag iterators, sometimes multiple times, and it didn’t fit what we want to do here very well.

What I ended up doing was:

Gather input in a MSelectionList

Move the selection list into a set, sorted by hierarchy depth.

Iterate over that set, so kind of depth first traversal of the input, not the entire Maya scene, moving objects we want to keep into a second set that will be returned to the caller.

ppt-adsk

Looks fine as it is, and I realize you didn't add a lot of code in utils/util.cpp, but I wonder if it wouldn't have been simpler to build up a Ufe::Selection and just use Ufe::Selection::containsAncestor().

ppt-adsk · 2020-07-14T14:30:06Z

lib/mayaUsd/utils/util.cpp

+namespace {
+    bool shouldAddToSet(const MDagPath& toAdd, const UsdMayaUtil::MDagPathSet& dagPaths)
+    // Utility function to check if an object should be added to the set of objects to
+    // export.  An object should not be added if it's invalid, or if any of it's parent


"it's" --> "its"

fowlertADSK · 2020-07-14T16:05:02Z

Looks fine as it is, and I realize you didn't add a lot of code in utils/util.cpp, but I wonder if it wouldn't have been simpler to build up a Ufe::Selection and just use Ufe::Selection::containsAncestor().

Yes, you did mention using a Selection and I took a look at it. The one thing Selection has for it would be performance, the lookup in that function should be faster than a set lookup, but it felt like the amount of code required to use it could be quite a bit more. We already have to go from MSelectionList -> MDagPathSet, so the addition of an intermediate MDagPathSet doesn’t add much code that seems out of place (i.e. dag path is the core of both of those).

To use a Selection…I’d have to convert dag paths to Ufe::Paths, create Ufe::SceneItems, fill up a Ufe::Selection…at that point, yes the lookup while iterating over SceneItems would be faster. I wouldn’t be against using it if performance was an issue, but this is already a relatively slow operation.

ppt-adsk · 2020-07-14T16:56:06Z

I think it's fine as is, you had a look at using Ufe::Selection and decided against it, the resulting code size is small and fairly easy to understand, I doubt that we'll revisit this.

mattyjams

Just a few small comments, but otherwise looks good to me!

mattyjams · 2020-07-14T17:17:10Z

lib/mayaUsd/commands/baseExportCommand.cpp

    }
+    GetFilteredSelectionToExport(exportSelected, objSelList, dagPaths);


Maybe a dumb question, but how are we finding GetFilteredSelectionToExport() without the UsdMayaUtil namespace on the front?

Hmm...all I know is it compiles and works somehow. I think I assumed that it was one of the "using" macros we add at the top of some of these files, but in this case it isn't that. I'll add it.

mattyjams · 2020-07-14T17:25:15Z

lib/mayaUsd/utils/util.h

+/// If \p exportSelected is true then the active selection list will be used to fill \p dagPaths
+/// with the objects to be exported.


Might also mention here that the selection passed in with objectList is ignored and it is re-populated with the active selection, just to clarify that objectList is still mutated in this case.

I'll update the comments to add something that mentions it.

mattyjams · 2020-07-14T17:33:00Z

lib/mayaUsd/utils/util.cpp

+    if (exportSelected) {
+        MGlobal::getActiveSelectionList(objectList);
+    } else if (objectList.isEmpty()) {
+        objectList.add("|*", true);


I hadn't noticed this before, but will this miss any top-level objects that have a namespace? Is there a syntax that would catch objects with arbitrarily deep namespaces?

|* will catch all top-level objects, including objects in a namespace. I could add a test for something like this though, can you give an example of what you mean by arbitrarily deep namespaces?

Oh, sorry, nevermind this then. Apparently selection lists work differently from the ls command. I remembered this being an issue with the latter, so it made me think the former might be affected as well.

I did this in the script editor to setup a test scene:

cmds.polyCube(name='Cube') cmds.namespace(addNamespace=':NS_One') cmds.namespace(setNamespace=':NS_One') cmds.polyCube(name='Cube') cmds.namespace(addNamespace=':NS_One:NS_Two') cmds.namespace(setNamespace=':NS_One:NS_Two') cmds.polyCube(name='Cube') cmds.namespace(addNamespace=':NS_One:NS_Two:NS_Three') cmds.namespace(setNamespace=':NS_One:NS_Two:NS_Three') cmds.polyCube(name='Cube')

That yields four top-level nodes:

[n for n in cmds.ls(long=True) if n.endswith('Cube')] # Result: [u'|Cube', u'|NS_One:Cube', u'|NS_One:NS_Two:Cube', u'|NS_One:NS_Two:NS_Three:Cube'] #

If I try to find those nodes using ls though, I seem to have to explicitly add the right level of namespacing:

cmds.ls('|*') # Result: [u'Cube', u'front', u'persp', u'side', u'top'] # cmds.ls('|*:*') # Result: [u'NS_One:Cube'] # cmds.ls('|*:*:*') # Result: [u'NS_One:NS_Two:Cube'] # cmds.ls('|*:*:*:*') # Result: [u'NS_One:NS_Two:NS_Three:Cube'] #

But the OpenMaya API indeed seems to behave the way you're describing:

from maya.api import OpenMaya as OM objectList = OM.MSelectionList() objectList.add('|*', True) for i in range(objectList.length()): print(objectList.getDagPath(i))

That yields (ignore the default cameras):

Cube front persp side top NS_One:Cube NS_One:NS_Two:Cube NS_One:NS_Two:NS_Three:Cube

mattyjams · 2020-07-14T17:35:42Z

lib/mayaUsd/utils/util.cpp

+        filterInput = false;
+    }
+
+    unsigned int nbObj = objectList.length(), i;


Any particular benefit to declaring i here versus in the for loop? It looks a little odd to my eye hanging off the end of this.

Ah, at one point I had two for loops instead of just one that both used i. I'll put it back in the loop.

mattyjams · 2020-07-14T17:41:20Z

lib/mayaUsd/utils/util.cpp

+    // Easiest way to filter by hierarchy is to:
+    // 1. Put the input into a set that is sorted by distance from the world root.
+    // 2. For each input object we iterate up its hierarchy checking if any parent is in the set.
+    // 3. If no parent is in the set then we can add it.
+    UsdMayaUtil::MDagPathSet sortedInput;
+    for (i=0; i < nbObj; i++) {
+        MDagPath dagPath;
+        status = objectList.getDagPath(i, dagPath);
+        if (status == MS::kSuccess)
+            sortedInput.emplace(dagPath);
+    }
+
+    for(auto pIter : sortedInput) {
+        if (!filterInput || shouldAddToSet(pIter, dagPaths))
+            dagPaths.emplace(pIter);
+    }
+}


I wonder whether this would be a useful utility in and of itself, sort of like SdfPath::RemoveDescendentPaths()?

I think if I wanted to spend any more time on this then I'd probably choose to add some utility functions that make it easier to convert a Maya selection list to a Ufe Selection and then use its functionality to implement this. TBH I might leave it for now and come back to it if we ever find another reason to need it.

mattyjams · 2020-07-14T17:43:12Z

lib/mayaUsd/utils/util.cpp

@@ -49,7 +49,6 @@
 #include <maya/MPlug.h>
 #include <maya/MPlugArray.h>
 #include <maya/MPoint.h>
-#include <maya/MSelectionList.h>


We still use MSelectionList in this file below as a param to GetFilteredSelectionToExport()?

mattyjams · 2020-07-14T17:44:08Z

lib/mayaUsd/utils/util.h

@@ -39,6 +38,7 @@
 #include <maya/MPlug.h>
 #include <maya/MStatus.h>
 #include <maya/MString.h>
+#include <maya/MSelectionList.h>


Move up before MStatus :)

These MSelectionList changes actually came in during a rebase + fixing some merge conflicts...didn't even realize they'd moved.

mattyjams · 2020-07-14T17:49:36Z

test/lib/usd/translators/UsdExportSelectionHierarchy/UsdExportSelectionHierarchy.ma

+requires maya "2020";
+currentUnit -l centimeter -a degree -t film;
+fileInfo "application" "maya";
+fileInfo "product" "Maya 2021";
+fileInfo "version" "Preview Release 117";


Sorry, this is probably a pain, but any chance this test scene could be reconstructed using Maya 2019? If we're still claiming to support that version, that would ensure that people building for 2019 can still run this test.

Ah, I was actually trying to fire up an older version of Maya to do exactly this but the two machines I have Maya on can't see each other when I'm on VPN and I forgot to come back to it. I meant to save it in Maya 2018.

mattyjams · 2020-07-14T17:51:06Z

test/lib/usd/translators/testUsdExportSelectionHierarchy.py

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


New Autodesk copyright.

yep, will update

mattyjams · 2020-07-14T17:52:36Z

test/lib/usd/translators/testUsdExportSelectionHierarchy.py

+
+    @classmethod
+    def setUpClass(cls):
+        cls.inputPath = fixturesUtils   .setUpClass(__file__)


Extra whitespace after fixtureUtils?

Hmm...not sure how that happened.

fowlertADSK commented Jul 14, 2020

View reviewed changes

fowlertADSK requested a review from ppt-adsk July 14, 2020 11:21

fowlertADSK assigned ecp-maya-devops-adsk Jul 14, 2020

kxl-adsk requested a review from mattyjams July 14, 2020 12:39

kxl-adsk added the import-export Related to Import and/or Export label Jul 14, 2020

ppt-adsk approved these changes Jul 14, 2020

View reviewed changes

mattyjams approved these changes Jul 14, 2020

View reviewed changes

MAYA-99006 code review changes

7298adf

kxl-adsk assigned ecp-maya-devops-adsk and unassigned ecp-maya-devops-adsk Jul 15, 2020

kxl-adsk merged commit a7f2feb into dev Jul 16, 2020

kxl-adsk deleted the fowlert/MAYA-99006/filter_input_for_export branch July 16, 2020 11:41

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

MAYA-99006 Filter out objects to export by hierarchy. #657

MAYA-99006 Filter out objects to export by hierarchy. #657

fowlertADSK commented Jul 14, 2020

fowlertADSK Jul 14, 2020

fowlertADSK Jul 14, 2020

fowlertADSK Jul 14, 2020

fowlertADSK Jul 14, 2020

fowlertADSK Jul 14, 2020

ppt-adsk left a comment

ppt-adsk Jul 14, 2020

fowlertADSK commented Jul 14, 2020

ppt-adsk commented Jul 14, 2020

mattyjams left a comment

mattyjams Jul 14, 2020

fowlertADSK Jul 14, 2020

mattyjams Jul 14, 2020

fowlertADSK Jul 14, 2020

mattyjams Jul 14, 2020

fowlertADSK Jul 15, 2020

mattyjams Jul 15, 2020

mattyjams Jul 14, 2020

fowlertADSK Jul 14, 2020

mattyjams Jul 14, 2020

fowlertADSK Jul 14, 2020

mattyjams Jul 14, 2020

mattyjams Jul 14, 2020

fowlertADSK Jul 14, 2020

mattyjams Jul 14, 2020

fowlertADSK Jul 14, 2020

mattyjams Jul 14, 2020

fowlertADSK Jul 14, 2020

mattyjams Jul 14, 2020

fowlertADSK Jul 14, 2020

		}
		GetFilteredSelectionToExport(exportSelected, objSelList, dagPaths);

		/// If \p exportSelected is true then the active selection list will be used to fill \p dagPaths
		/// with the objects to be exported.

MAYA-99006 Filter out objects to export by hierarchy. #657

MAYA-99006 Filter out objects to export by hierarchy. #657

Conversation

fowlertADSK commented Jul 14, 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

Choose a reason for hiding this comment

ppt-adsk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fowlertADSK commented Jul 14, 2020

ppt-adsk commented Jul 14, 2020

mattyjams 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

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