Instance import, part 1 #794

JGamache-autodesk · 2020-09-23T16:04:12Z

Brings in the original @sirpalee / @pmolodo instancing code from the following Luma branch.

This code adds an Instance Mode option on import to either build instances or flatten the scene.

Updates:

Merged with latest dev
masters are loaded recursively and on demand, allowing import of instances inside other instances.
added unit tests for instance imports based on USD instancing scenarios by @pmolodo

Bug fixes:

Saves the Maya shape name instead of using the generic name "Shape"
XformOp errors on import could result in geometries with (0, 0, 0) scaling. Will now default to (1, 1, 1).

Fixes #340, fixes #723, fixes #310

Still todo:
The following issues have been found and will likely be fixed in a follow-up update:

Extra transforms get inserted between the instantiation transform and the instanced shape (also happens for regular non-instanced hierarchies where shapes in the middle of the hierarchy also gain a transform).
Instanced hierarchies will export as piecewise instances. A transform where all children are instances should generate a single USD master.

…instances # Conflicts: # lib/mayaUsd/fileio/jobs/jobArgs.cpp # lib/mayaUsd/fileio/jobs/jobArgs.h # lib/mayaUsd/fileio/jobs/readJob.cpp # lib/mayaUsd/fileio/jobs/readJob.h # plugin/pxr/maya/lib/usdMaya/importCommand.cpp

mattyjams

I think I'll have to defer to @pmolodo for the final approval since he's much more knowledgable on this subject than I am, but it generally looks good to me. I had just a few mostly minor notes.

lib/mayaUsd/fileio/jobs/readJob.cpp

mattyjams · 2020-09-25T18:56:06Z

lib/mayaUsd/fileio/jobs/readJob.cpp

-        const UsdPrimRange range = UsdPrimRange::PreAndPostVisit(rootPrim);
+        _PrimReaderMap primReaderMap;
+        const UsdPrimRange range =
+            mArgs.instanceMode == UsdMayaJobImportArgsTokens->flatten ?


Do we expect that there might some day be more instanceMode options, or could this equality be replaced with buildInstances from above?

Not sure about that one. With the current code, using instanceMode=anything_but_flatten_or_build would end up not loading any instance, which was the old behavior before this patch.

Is that a valid mode that should be preserved in the Import options? I would call it "ignore"...

No, I don't think it makes sense to preserve the behavior where instances in USD are ignored. If I'm reading the changes in jobArgs.cpp correctly, I think we'll fallback to building instances if instanceMode was given something bogus?

My main thought here was just whether we could re-use the buildInstances boolean from above. No big deal though.

lib/mayaUsd/fileio/jobs/readJob.h

test/lib/usd/translators/UsdImportInstancesTest/ComplexScenario_optimized.usda

test/lib/usd/translators/UsdImportInstancesTest/ComplexScenario_unoptimized.usda

mattyjams · 2020-09-25T20:02:17Z

test/lib/usd/translators/testUsdImportInstances.py

+            pCube1_pCubeShape1 [Scope]
+                pCubeShape1 [Mesh]
+
+        The goal would be for Maya to skip all [Scope] items and end up with:


Ahh... so maybe this explains the missing type names in the USD above. Prims with the Scope type name are different from the typeless prims we have at the moment though. Do we really mean Scope here and we should add that type name above, or should this read typeless here instead?

mattyjams · 2020-09-25T20:21:39Z

test/lib/usd/translators/testUsdImportInstances.py

+                           primPath="/",
+                           instanceMode="buildInstances")


I think this setting of primPath can be removed, since that should be the default? Similarly, instanceMode=buildInstances looks like it was made the default as well, but I'm ok with that staying since I think it makes the intent of the test clear.

Isn't primPath the defaultPrim, which would be pCube1 in the test scenes?

test/lib/usd/translators/testUsdImportInstances.py

pmolodo

So, I don't have too much additional to add to what @mattyjams said - mostly looks good.

My one concern is that, if we do release as is, and later want to support "cleaning up" the "extra" Scope args, it will obviously change the resulting maya scene. If we release this as is, people may become dependent on the behavior implemented in this PR... and I'd hate to lock us into having the unnecessary xforms. Are we ok with making a potentially breaking change to eliminate them in the future?

pmolodo · 2020-10-05T17:13:18Z

test/lib/usd/translators/UsdImportInstancesTest/ComplexScenario_optimized.usda

+    }
+}
+
+over "InstanceSources"


I think Prototypes would be bad too, because in the latest updates to USD, they're replacing "master" with "Prototype". (Which is going to require a some API changes here, but there's already post-20.08-incompatibilities in the existing maya_usd codebase, so we can punt on that for now).

And if we can't use Prototypes... then I might prefer the current InstanceSources over something which is essentially generic, like InstanceModels or InstanceAssets. I see where you're coming from, but:

sources is NOT the same term as the "official" term (which was masters, and is now prototypes)

conceptually they DO convey something similar... but the basic problem is that what we're trying to do IS something conceptually similar to what the "official" USD-created masters do. So it's a bit of a catch 22 - any name that accurately describes or invokes what the purpose here is automatically going to be treading pretty close to "masters"/"prototypes". So I'd rather go with the option that has some descriptive value, as long as it avoids the "official" term.

@mattyjams , would it help if it were named something more maya-centric, like say, MayaExportedInstanceSources? If we eventually want to implement special behavior for prims located here (which we will, if we ever do the eliminate-Scopes thing later), then it would be good to have it named a bit less generically, anyway.

test/lib/usd/translators/UsdImportInstancesTest/ComplexScenario_optimized.usda

lib/mayaUsd/fileio/jobs/readJob.cpp

…instances

Also investigated and fixed a few asserts poppping up in the test suite.

…instances # Conflicts: # lib/mayaUsd/fileio/jobs/jobArgs.cpp # lib/mayaUsd/fileio/jobs/jobArgs.h

pmolodo

Looks good to me. I'd still like to change the name of "InstanceSources" to be more maya-specific, but I suppose we can make that a separate PR if you'd prefer.

pmolodo · 2020-10-14T17:56:18Z

lib/mayaUsd/fileio/jobs/readJob.cpp

+                if (status) {
+                    while (masterNode.childCount()) {
+                        masterNode.removeChildAt(masterNode.childCount() - 1);
+                    }


Is there some sort of error or something that happens if you don't remove the children before deleting the parent? Since these child removals aren't done via a MDGModifier, will this undo properly? (Though - not sure if the the import command as a whole is even undoable...)

With the special builds we use, we get asserts about incorrect children counts on file->new because the instance count and children count differ. It is not something you will see with a shipped Maya cut.

pmolodo · 2020-10-14T17:58:17Z

test/lib/usd/translators/UsdImportInstancesTest/ComplexScenario_optimized.usda

+    }
+}
+
+over "InstanceSources"


Yeah, I think it would, but I don't think it should be a problem to change it there too...

mattyjams

Looks good to me!

mattyjams · 2020-10-16T16:40:39Z

lib/mayaUsd/fileio/jobs/readJob.cpp

-        const UsdPrimRange range = UsdPrimRange::PreAndPostVisit(rootPrim);
+        _PrimReaderMap primReaderMap;
+        const UsdPrimRange range =
+            mArgs.instanceMode == UsdMayaJobImportArgsTokens->flatten ?


No, I don't think it makes sense to preserve the behavior where instances in USD are ignored. If I'm reading the changes in jobArgs.cpp correctly, I think we'll fallback to building instances if instanceMode was given something bogus?

My main thought here was just whether we could re-use the buildInstances boolean from above. No big deal though.

mattyjams · 2020-10-16T16:57:01Z

test/lib/usd/translators/UsdImportInstancesTest/ComplexScenario_optimized.usda

+    upAxis = "Y"
+)
+
+def Xform "pCube1" (


One small nit-pick with the type names here, but it looks like we ended up with type names everywhere. I would think the majority of the time when you have a prim referencing other prims, you likely want a typeless def for the prim doing the referencing. I think in most cases below, the type names between the referencer and the referenced agree, so I don't think it's a practical issue here. Typeless defs let the type of the referenced prim shine through to the referencer whatever that type may be, so you don't have to match the types yourself. Specifying a different type on the referencer will actually change the type of the composed prim, which is pretty rarely what you intend.

sirpalee and others added 3 commits February 26, 2020 11:32

[usdMaya] Importing instances.

5582d35

MAYA-106489 - Handle more complex instancing scenarios

393ec92

JGamache-autodesk requested review from mattyjams and pmolodo September 23, 2020 16:06

JGamache-autodesk assigned ecp-maya-devops-adsk Sep 23, 2020

JGamache-autodesk added the import-export Related to Import and/or Export label Sep 23, 2020

mattyjams reviewed Sep 25, 2020

View reviewed changes

pmolodo reviewed Oct 5, 2020

View reviewed changes

JGamache-autodesk added 2 commits October 8, 2020 16:41

Merge remote-tracking branch 'github/dev' into pr/pxr/maya_importing_…

10b006c

…instances

Responded to review comments.

be52a7a

Also investigated and fixed a few asserts poppping up in the test suite.

JGamache-autodesk assigned ecp-maya-devops-adsk and unassigned ecp-maya-devops-adsk Oct 13, 2020

JGamache-autodesk requested review from mattyjams and pmolodo October 13, 2020 21:08

Merge remote-tracking branch 'github/dev' into pr/pxr/maya_importing_…

fc65a32

…instances # Conflicts: # lib/mayaUsd/fileio/jobs/jobArgs.cpp # lib/mayaUsd/fileio/jobs/jobArgs.h

pmolodo approved these changes Oct 14, 2020

View reviewed changes

JGamache-autodesk assigned ecp-maya-devops-adsk and unassigned ecp-maya-devops-adsk Oct 15, 2020

mattyjams approved these changes Oct 16, 2020

View reviewed changes

JGamache-autodesk added the ready-for-merge Development process is finished, PR is ready for merge label Oct 16, 2020

kxl-adsk merged commit 56b29e2 into dev Oct 16, 2020

kxl-adsk deleted the pr/pxr/maya_importing_instances branch October 16, 2020 19:39

This was referenced Oct 20, 2020

MAYA-106489 - Import instances is a boolean option #856

Merged

MAYA-106489 - Rename InstanceSources to be Maya-specific #859

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Instance import, part 1 #794

Instance import, part 1 #794

JGamache-autodesk commented Sep 23, 2020

mattyjams left a comment

mattyjams Sep 25, 2020

JGamache-autodesk Oct 13, 2020 •

edited

Loading

mattyjams Oct 16, 2020

mattyjams Sep 25, 2020

mattyjams Sep 25, 2020

JGamache-autodesk Oct 13, 2020

pmolodo left a comment •

edited

Loading

pmolodo Oct 5, 2020

pmolodo left a comment

pmolodo Oct 14, 2020

JGamache-autodesk Oct 14, 2020

pmolodo Oct 14, 2020

mattyjams left a comment

mattyjams Oct 16, 2020

mattyjams Oct 16, 2020

Instance import, part 1 #794

Instance import, part 1 #794

Conversation

JGamache-autodesk commented Sep 23, 2020

mattyjams left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JGamache-autodesk Oct 13, 2020 • edited Loading

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 left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pmolodo 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

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

JGamache-autodesk Oct 13, 2020 •

edited

Loading

pmolodo left a comment •

edited

Loading