Pixar Mesh read translator/utilities refactoring #343

HamedSabri-adsk · 2020-03-09T14:14:38Z

Problem Description and Objectives

Translation between USD and Maya schemas is a key technology to delivering amazing artist experiences across diverse workflows. Unfortunately, there is currently significant duplication of schema translators and relevant utilities across different USD plug-ins. This is hard to maintain and manage.

To enable code reuse across different workflows, accelerate community contributions, ease maintenance, and let others build upon advanced capabilities (e.g., Animal Logic in-context modeling), we are refactoring translation code. The goal is to centralize common functionality into a core library while still allowing individual plug-ins to customize behavior, based on specific needs.

PR Description

To ease reviewing of this work, we’re aiming to split up a large pull request into several smaller pull requests. The below description relates to the first of these pull requests:

This PR is based off of Pixar's Mesh read translators which provides a common set of mesh utilities and translators that can be used by other frameworks/plug-ins.

Specifically, this PR provides utility functions and translators for reading and translating UsdGeomMesh prim into a Maya mesh object. To achieve this, some architectural changes were required for the UsdMayaTranslatorMesh class:

Create a new class interfaces with no internal dependency to UsdMayaPrimReaderArgs and UsdMayaPrimReaderContext.
Remove private static functions, extract their operations and provide a new set of utilities under meshUtils (e.g., assignPrimvarsToMesh, assignInvisibleFaces, assignSubDivTagsToMesh ).
Provide some getter functions to easily get access to Maya Mesh object as well as deforming meshes (e.g blendshape, PointBasedDeformer) objects.

…ons and translator for reading and translating UsdGeomMesh prim into a Maya mesh object.To make this happen I had to make some architectural changes in UsdMayaTranslatorMesh class: - Create a new class interfaces with no internal dependency to UsdMayaPrimReaderArgs and UsdMayaPrimReaderContext. - Remove private static functions, extract their operations and provide as a new set of utilities under meshUtils ( e.g assignPrimvarsToMesh, assignInvisibleFaces, assignSubDivTagsToMesh ). - Provide getter functions to get access Maya Mesh object as well as deforming meshes (e.g blendshape, PointBasedDeformer) objects.

mattyjams

Code review-wise, the changes mostly look good to me, as it's mainly just moving things around. I noticed two things about the point-based deformer node handling that I think need attention though:

I think it should probably not be enabled by default when constructing the TranslatorMeshRead.
If we succeed in setting up the point-based deformer, we should skip doing any blend shape deformer stuff.

But following that, I ran the unit tests and hit a whole bunch of test failures:

82% tests passed, 15 tests failed out of 85

Total Test time (real) = 397.60 sec

The following tests FAILED:
	 25 - testUsdExportOverImport (Failed)
	 29 - testUsdExportPointInstancer (Failed)
	 41 - testUsdImportAsAssemblies (Failed)
	 43 - testUsdImportColorSets (Failed)
	 46 - testUsdImportMesh (Failed)
	 47 - testUsdImportNestedAssemblyAnimation (Failed)
	 49 - testUsdImportShadingModeDisplayColor (Failed)
	 50 - testUsdImportShadingModePxrRis (Failed)
	 51 - testUsdImportSkeleton (Failed)
	 53 - testUsdImportUVSets (Failed)
	 54 - testUsdImportUVSetsFloat (Failed)
	 55 - testUsdImportXforms (Failed)
	 58 - testUsdMayaAdaptorMetadata (Failed)
	 64 - testUsdMayaProxyShape (Failed)
	 68 - testUsdReferenceAssemblyChangeRepresentations (Failed)

I haven't looked into them in much detail yet, but it seems like some of them may have to do with the node naming and/or transform and shape node handling.

mattyjams · 2020-03-17T00:55:18Z

lib/fileio/utils/meshUtil.cpp

+MStatus 
+UsdMayaMeshUtil::setName(const MObject& meshObj, const std::string& shapeName)
+{
+    if(meshObj.apiType() != MFn::kMesh){
+        return MS::kFailure;
+    }
+
+    MStatus status;
+
+    MFnMesh meshFn(meshObj);
+    meshFn.setName(MString(shapeName.c_str()), false, &status);
+
+    return status;
+}
+


I'm a little unsure about how useful this function is generally. As far as I can tell from these changes, it's currently only called in one place in meshReader.cpp?

Maybe it would make more sense if it was moved to UsdMayaUtil::setName() and operated on nodes as MFnDependencyNode?

Good point. Please see 7be630d

I originally stick this here because it was needed for this PR. I have a larger PR locally for cleaning up the utility functions and didn't want bring those changes here.

mattyjams · 2020-03-17T00:56:44Z

lib/fileio/utils/meshUtil.cpp

+
+    MStatus status;
+
+    MFnMesh meshFn(meshObj);


This constructor can fail and return an error MStatus too, can't it? Should we be capturing and checking that before calling setName()?

Please see 7be630d.

mattyjams · 2020-03-17T01:14:36Z

lib/fileio/translators/translatorMesh.cpp

+#include "../utils/meshUtil.h"
+#include "../utils/readUtil.h"


Are we ready to stop using relative paths for these includes? I'd love it if we could stop digging ourselves deeper into that hole. I know there are a lot of existing ones to clean up, but it would be great not to be adding more.

On my TODO to make the final changes we voted on and commit our coding guidelines to the repo. So yes, we should start using them now.

@mattyjams Believe me I don't like the dots as much as you don't but I didn't want to spend time doing this in this PR since they are going to be cleanup in a serrate PR.

mattyjams · 2020-03-17T01:20:01Z

lib/fileio/translators/translatorMesh.cpp

@@ -252,7 +76,7 @@ UsdMayaTranslatorMesh::Create(
                "faceVertexCounts), which isn't currently supported. "
                "Skipping...",
                prim.GetPath().GetText());
-        return false;
+        stat = MS::kFailure;


Are we sure we want to continue attempting to translate the mesh after we encounter an error? Seems like we could run into trouble both on the Maya side and maybe the USD side as well if we try to barrel ahead with a degenerate or otherwise invalid mesh.

This is covered in 64ccefa. Both faceVertexIndices and faceVertexCounts are passed to UsdGeomMesh::ValidateTopology and if we don't have a valid topology mesh, we simply set the status and return.

mattyjams · 2020-03-17T01:23:32Z

lib/fileio/translators/translatorMesh.cpp

-            _SetupPointBasedDeformerForMayaNode(meshObj, prim, context)) {
-        return true;
+    if (m_wantCacheAnimation){
+        stat = setPointBasedDeformerForMayaNode(m_meshObj, stageNode, prim);


Do we need to check status here and return if it was successful? The intention of this point based deformer thing was as an alternative to the blend shape deformer, since that can be hugely expensive both in translation and execution time. If we succeeded in setting up the point based deformer, we want to skip setting up a blend shape deformer.

Duh... That's definitely an overlook on my part. Sorry about that. See 684c5b4
I also have done some extra clean up around the status in the same commit. If we have a bad USD or Maya mesh, I simply set the status and return. There is no point to do any further operation if the data is bad in a first place.

mattyjams · 2020-03-17T01:32:49Z

lib/fileio/translators/translatorMesh.h

+                       const MObject& transformObj,
+                       const MObject& stageNode,
+                       const GfInterval& frameRange,
+                       bool wantCacheAnimation = true,


I'm not sure whether we want to enable this by default. Turning this on, and using the point based deformer as a result, effectively bypasses doing a bunch of translation, so you then end up with a Maya node in your scene that depends on the USD file you're importing. We never enabled that option by default for usdImport since it seemed like the expected behavior would be that you could import a USD file and then delete the file if you wanted to. There would be no connection between the Maya scene resulting from a usdImport, and the USD file that was imported.

Ugh I see... Makes sense. Please see 64ccefa

…ed deformer" is selected. - clean up some of status logics. If a critical operation during the TranslatorMeshRead construction fails, we set the *status and simply return.

HamedSabri-adsk · 2020-03-18T20:31:08Z

Hi @mattyjams , apologies for the late reply. It has been a hectic week here for me :)

I have created 3 separate commits to address some of your feedback.

The unit tests are still failing which I am investigating next. I still don't have full understanding why they fail but I believe I can find out once looking into them in more detail.

… name wasn't set properly.

HamedSabri-adsk · 2020-03-19T05:18:26Z

lib/fileio/translators/translatorMesh.cpp

+    }
+
+	// store the path
+    m_shapePath = prim.GetPath().AppendChild(TfToken(shapeName));


After the mesh function set is created, we set it's name and then store the SdfPath that is created off of it.

HamedSabri-adsk · 2020-03-19T05:18:32Z

lib/usd/translators/meshReader.cpp

-    // shape name
-    const auto& primName = prim.GetName().GetString();
-    status = UsdMayaUtil::setName(meshRead.meshObject(), primName);
-    CHECK_MSTATUS_AND_RETURN(status, false);


This was just wrong! UsdMayaUtil::setName was creating a new object of type MFnMesh and set it's name.

HamedSabri-adsk · 2020-03-19T05:18:35Z

lib/usd/translators/meshReader.cpp

-    const auto shapeName = TfStringPrintf("%sShape", primName.c_str());
-    const auto shapePath = prim.GetPath().AppendChild(TfToken(shapeName));
-    context->RegisterNewMayaNode(shapePath.GetString(), meshRead.meshObject());
+    context->RegisterNewMayaNode(meshRead.shapePath().GetString(), meshRead.meshObject());


We now get the path from the meshRead and pass it in to be registered with the Maya mesh object.

HamedSabri-adsk · 2020-03-19T05:18:38Z

lib/utils/util.cpp

-    MFnMesh meshFn(meshObj);
-    meshFn.setName(MString(shapeName.c_str()), false, &status);
-
-    return status;


This was indeed a weird and almost useless function that I introduced. Sigh....

mattyjams

Cool, and all of our tests are now passing internally.

Just one tiny thing to possibly consider is whether the TranslatorMeshRead could be passed nullptr for the status parameter.

Other than that, looks good to me! Thanks @HamedSabri-adsk!

mattyjams · 2020-03-20T23:33:23Z

lib/fileio/translators/translatorMesh.cpp

+                                       const MObject& stageNode,
+                                       const GfInterval& frameRange,
+                                       bool wantCacheAnimation,
+                                       MStatus * status)


It looks like status could be passed nullptr so the sites where we dereference it below should probably check it first?:

if (status != nullptr) {

Or we could store it into the local stat first, and then check before any early return.

Awesome! Thanks @mattyjams

Good point, I initialized the status before passing it in to both meshRead and UsdMayaTranslatorUtil::CreateTransformNode. Please see 11b49c5

HamedSabri-adsk requested review from mattyjams and fowlertADSK March 9, 2020 14:14

HamedSabri-adsk added import-export Related to Import and/or Export pxr Related to Pixar plugin labels Mar 9, 2020

kxl-adsk added core Related to core library and removed pxr Related to Pixar plugin labels Mar 9, 2020

Remove unnecessary check!

86980e1

mattyjams suggested changes Mar 17, 2020

View reviewed changes

Hamed Sabri added 3 commits March 18, 2020 12:14

Don't enable cacheAnimation by default.

64ccefa

- Make sure to skip setting up a "blend shape deformer" if "point bas…

684c5b4

…ed deformer" is selected. - clean up some of status logics. If a critical operation during the TranslatorMeshRead construction fails, we set the *status and simply return.

move setName utility function to util.h

7be630d

Fix all failing unit tests. This was due the fact that the shape node…

42500cc

… name wasn't set properly.

HamedSabri-adsk commented Mar 19, 2020

View reviewed changes

HamedSabri-adsk requested a review from mattyjams March 19, 2020 05:19

mattyjams approved these changes Mar 20, 2020

View reviewed changes

Hamed Sabri and others added 3 commits March 20, 2020 22:00

make sure status is initialized when created.

11b49c5

Merge branch 'dev' into sabrih/MAYA-101846/pxr_mesh_translator_read

230afb0

Fix a bad merge.

da213c6

kxl-adsk merged commit ff90ae5 into dev Mar 21, 2020

kxl-adsk deleted the sabrih/MAYA-101846/pxr_mesh_translator_read branch March 21, 2020 17:52

HamedSabri-adsk mentioned this pull request Apr 14, 2020

Pixar Mesh write translator/utilities refactoring #420

Merged

ppt-adsk pushed a commit that referenced this pull request Feb 28, 2023

MAYA-127403: readd removed callback removals (#343)

bcc1152

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Pixar Mesh read translator/utilities refactoring #343

Pixar Mesh read translator/utilities refactoring #343

HamedSabri-adsk commented Mar 9, 2020

mattyjams left a comment

mattyjams Mar 17, 2020

HamedSabri-adsk Mar 18, 2020

mattyjams Mar 17, 2020

HamedSabri-adsk Mar 18, 2020

mattyjams Mar 17, 2020

kxl-adsk Mar 17, 2020

HamedSabri-adsk Mar 18, 2020

mattyjams Mar 17, 2020

HamedSabri-adsk Mar 18, 2020

mattyjams Mar 17, 2020

HamedSabri-adsk Mar 18, 2020 •

edited

Loading

mattyjams Mar 17, 2020

HamedSabri-adsk Mar 18, 2020 •

edited

Loading

HamedSabri-adsk commented Mar 18, 2020

HamedSabri-adsk Mar 19, 2020

HamedSabri-adsk Mar 19, 2020

HamedSabri-adsk Mar 19, 2020

HamedSabri-adsk Mar 19, 2020

mattyjams left a comment

mattyjams Mar 20, 2020

HamedSabri-adsk Mar 21, 2020

		#include "../utils/meshUtil.h"
		#include "../utils/readUtil.h"

Pixar Mesh read translator/utilities refactoring #343

Pixar Mesh read translator/utilities refactoring #343

Conversation

HamedSabri-adsk commented Mar 9, 2020

Problem Description and Objectives

PR Description

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

HamedSabri-adsk Mar 18, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

HamedSabri-adsk Mar 18, 2020 • edited Loading

Choose a reason for hiding this comment

HamedSabri-adsk commented Mar 18, 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

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

HamedSabri-adsk Mar 18, 2020 •

edited

Loading

HamedSabri-adsk Mar 18, 2020 •

edited

Loading