EMSUSD-549 Fixes unneeded nodes getting exported on materials #3366

AramAzhari-adsk · 2023-10-04T21:20:23Z

When exporting prims to USD that have Lambert materials , some extra nodes were being exported that would later cause a crash due to the mismatch between input nodes and uv sets. This fixes the issue by changing to depth first when traversing the node graph and only exporting nodes that are supposed to be part of the graph.

vlasovi · 2023-10-05T14:03:07Z

lib/mayaUsd/fileio/shading/shadingModeUseRegistry.cpp

@@ -180,7 +185,7 @@ class UseRegistryShadingModeExporter : public UsdMayaShadingModeExporter
            rootPlugCopy,
            MFn::kInvalid,
            MItDependencyGraph::Direction::kUpstream,
-            MItDependencyGraph::Traversal::kDepthFirst,


I don't understand in which way switching to kBreadthFirst helps here and what it solves.

This is a suggestion by @JGamache-autodesk to improve traversing for complex networks with cascading connections. Without this the example materials in question still work. What fixes the issue is Line 263

Lets have this scenario:

When traversing depth first the connections will be returned in this order:

file1.outColor -> standardSurface2.base_color <== Exports file1 file3.outColor -> file1.colorGain <== Exports file3 file3.outColor -> file2.colorGain <== Incorrect since file2 is not yet exported

Traversing breadth first will produce this order:

file1.outColor -> standardSurface2.base_color <== Exports file1 file2.outColor -> standardSurface2.specular_color <== Exports file2 file3.outColor -> file1.colorGain <== Exports file3 file3.outColor -> file2.colorGain <== Correct now since we have already exported file2

In my mind, we should not make a move file3.outColor -> file2.colorGain because this traverses the graph in the opposite direction. Since file2 is connected to standardSurface2, it will be processed in its turn. So, preventing the backward move in the graph will solve the actual bug whether we use kDepthFirst or kBreadthFirst.

Here is the order of the connections returned by a breadth first MItDependencyGraph:

As you can see we end up traversing "file3.outColor -> file2.colorGain" at step 5. Since we already processed "file2.outColor -> standardSurface.pecularColor" at step 2 then we are OK.
Still in full agreement that browsing all the destinations of a source port is bad. I think removing that part would indeed work perfectly fine since the traversal will eventually hit all connections.

vlasovi · 2023-10-05T14:06:30Z

lib/mayaUsd/fileio/shading/shadingModeUseRegistry.cpp

@@ -255,7 +260,7 @@ class UseRegistryShadingModeExporter : public UsdMayaShadingModeExporter
                }

                auto dstShaderInfo = _GetExportedShaderForNode(
-                    dstPlug.node(), materialExportPath, context, shaderWriterMap);
+                    dstPlug.node(), materialExportPath, context, shaderWriterMap, false);


As far as I can see, the problem here is that we take destination plugs that we shouldn't. And it seems to me that it is at line 231 (if (!iterPlug.destinations(dstPlugs, &status) || status != MS::kSuccess) {) that we do that. So maybe the good solution would be to remove this code from there.

It looks like this code there makes us move in the opposite direction, which I think is wrong.

Yes, that would be a more correct fix. We need to process only the connections returned by MItDependencyGraph and should not be looking for the full list of destination plugs. I lacked time to implement this complete solution and resorted to the quickest fix that produced correct results. If you have time to rewrite the function then it would improve the codebase.

Make sure to add unit test for both the issue in EMSUSD-549 and the other one I put in a comment above.

@vlasovi @JGamache-autodesk Thanks for the feedback. I'll update the code and will add test cases to cover the above more scenarios.

I should mention even though this correctly exports as USD surface preview, it still doesn't have the support needed to make the ColorGain link survive. For this reason, the imported usd still does not fully reflect the exported ones.

Furthermore, if we switch to MaterialX when exporting, the original issue of EMSUSD-549 doesn't occur at all and the graph is exported and imported and the user's graph is preserved.

For that reason, the tests to compare export and import will automatically fail if added and we would have to skip that for the time being until we add support for fanned out nodes in USD Surface Preview scenarios, if at all possible.

vlasovi

I think we need a better solution to make sure we don't include unneeded destination plugs. Please see my comment in the code.

AramAzhari-adsk · 2023-10-05T16:05:12Z

I think we need a better solution to make sure we don't include unneeded destination plugs. Please see my comment in the code.

@vlasovi @JGamache-autodesk Removing the change at Line231 results in a correct export but causes multiple current tests to fail:

     84 - testShaderWriter (Failed)
    109 - testVP2RenderDelegateMaterialX (Failed)
    132 - testVP2RenderDelegateDisplayLayers (Failed)
    146 - testPxrUsdPreviewSurfaceExport (Failed)
    182 - testUsdExportUsdPreviewSurface (Failed)
    225 - testUsdExportImportRoundtripPreviewSurface (Failed)
    241 - testUsdExportRfMShaders (Failed)
    244 - testUsdExportUVSetMappings (Failed)
    252 - testUsdExportMaterialX (Failed)
    254 - testUsdExportMultiMaterial (Failed)

May I suggest that we check the changes in this PR in for the time being to unblock the crashing issue, and have another ticket made for a refactor of the algorithm that involves writing more tests as well as refactor the above tests.

vlasovi · 2023-10-05T17:24:48Z

The proposed fix with kBreadthFirst traversal may not fix all such crashes depending on the shader graph layout, so I would still like to see another approach implemented. I don't expect the alternative approach to require changes to the existing autotests. The failures that you see must be due to a bug.

JGamache-autodesk · 2023-10-05T18:54:02Z

That code is currently the best that can be done using MItDependencyGraph because it iterates on output plugs instead of connections.
Feel free to rewrite, but you will have to implement the attribute and connection traversal manually.
Before doing that, it would be nice if you could provide an example scene where this code (still traversing all destinations) fails to export the shader graph correctly.

vlasovi · 2023-10-05T19:12:34Z

I agree. Traversing the plugs is the right way to go here. I don't think we should rewrite anything. We just need to filter out unnecessary connections, which should be a minor adjustment here.

JGamache-autodesk · 2023-10-05T19:30:01Z

That filter would be skip any destination node not found in shaderWriterMap, which is why I added the createIfMissing flag to _GetExportedShaderForNode. It will return nullptr if the node was never exported.

vlasovi · 2023-10-05T19:37:06Z

The filter with 'createIfMissing' flag would rely on the order of graph traversal and may fail even with kBreadthFirst traversal, depending on the graph layout. The final filter should be similar to your one but without this issue.

vlasovi

Looks good. Nice work, @AramAzhari-adsk

JGamache-autodesk

1- Can you try using a UsdMayaUtil::MObjectHandleUnorderedSet instead of a MObjectArray? The performance will be way better!
2- Can you add unit tests for the two cases discussed where a file is connected multiple times?

AramAzhari-adsk · 2023-10-12T21:00:22Z

1- Can you try using a UsdMayaUtil::MObjectHandleUnorderedSet instead of a MObjectArray? The performance will be way better! 2- Can you add unit tests for the two cases discussed where a file is connected multiple times?

I made a separate ticket (EMSUSD-692) to address the improvement as well as the unit cases multiple times.

EMSUSD-549 Fixes unneeded nodes getting exported on materials

611b999

AramAzhari-adsk added bug Something isn't working adsk Related to Autodesk plugin materials labels Oct 4, 2023

AramAzhari-adsk requested a review from JGamache-autodesk October 4, 2023 21:20

AramAzhari-adsk assigned ecp-maya-devops-adsk Oct 4, 2023

AramAzhari-adsk requested a review from vlasovi October 4, 2023 21:20

Fixing lint issues

b52fedd

AramAzhari-adsk assigned ecp-maya-devops-adsk and unassigned ecp-maya-devops-adsk Oct 4, 2023

vlasovi reviewed Oct 5, 2023

View reviewed changes

vlasovi requested changes Oct 5, 2023

View reviewed changes

AramAzhari-adsk requested a review from vlasovi October 5, 2023 16:05

AramAzhari-adsk assigned ecp-maya-devops-adsk and unassigned ecp-maya-devops-adsk Oct 5, 2023

AramAzhari-adsk added the do-not-merge-yet Development is not finished, PR not ready for merge label Oct 6, 2023

Pruning plug selection to only include the needed ones.

a3c3de0

AramAzhari-adsk assigned ecp-maya-devops-adsk and unassigned ecp-maya-devops-adsk Oct 6, 2023

Changed the allowNodes type to MObject to preserve namespaces

24e89db

AramAzhari-adsk unassigned ecp-maya-devops-adsk Oct 6, 2023

AramAzhari-adsk assigned ecp-maya-devops-adsk and unassigned ecp-maya-devops-adsk Oct 6, 2023

Fixes a linting space.

e582572

AramAzhari-adsk assigned ecp-maya-devops-adsk and unassigned ecp-maya-devops-adsk Oct 12, 2023

vlasovi approved these changes Oct 12, 2023

View reviewed changes

JGamache-autodesk requested changes Oct 12, 2023

View reviewed changes

JGamache-autodesk approved these changes Oct 12, 2023

View reviewed changes

AramAzhari-adsk added ready-for-merge Development process is finished, PR is ready for merge and removed do-not-merge-yet Development is not finished, PR not ready for merge ready-for-merge Development process is finished, PR is ready for merge labels Oct 12, 2023

seando-adsk added import-export Related to Import and/or Export and removed bug Something isn't working adsk Related to Autodesk plugin materials labels Oct 19, 2023

seando-adsk merged commit 8749805 into dev Oct 19, 2023
13 of 14 checks passed

seando-adsk deleted the azharia/EMSUSD-549/Fixes-Exporting-Unneeded-Nodes-In-Materials branch October 19, 2023 20:55

AramAzhari-adsk mentioned this pull request Oct 20, 2023

Add test cases for material fanned out file nodes #3403

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

EMSUSD-549 Fixes unneeded nodes getting exported on materials #3366

EMSUSD-549 Fixes unneeded nodes getting exported on materials #3366

AramAzhari-adsk commented Oct 4, 2023

vlasovi Oct 5, 2023

AramAzhari-adsk Oct 5, 2023

JGamache-autodesk Oct 5, 2023

vlasovi Oct 5, 2023 •

edited

Loading

JGamache-autodesk Oct 5, 2023

vlasovi Oct 5, 2023 •

edited

Loading

JGamache-autodesk Oct 5, 2023

AramAzhari-adsk Oct 5, 2023

vlasovi left a comment

AramAzhari-adsk commented Oct 5, 2023

vlasovi commented Oct 5, 2023

JGamache-autodesk commented Oct 5, 2023

vlasovi commented Oct 5, 2023

JGamache-autodesk commented Oct 5, 2023

vlasovi commented Oct 5, 2023

vlasovi left a comment

JGamache-autodesk left a comment

AramAzhari-adsk commented Oct 12, 2023

EMSUSD-549 Fixes unneeded nodes getting exported on materials #3366

EMSUSD-549 Fixes unneeded nodes getting exported on materials #3366

Conversation

AramAzhari-adsk commented Oct 4, 2023

vlasovi Oct 5, 2023

Choose a reason for hiding this comment

AramAzhari-adsk Oct 5, 2023

Choose a reason for hiding this comment

JGamache-autodesk Oct 5, 2023

Choose a reason for hiding this comment

vlasovi Oct 5, 2023 • edited Loading

Choose a reason for hiding this comment

JGamache-autodesk Oct 5, 2023

Choose a reason for hiding this comment

vlasovi Oct 5, 2023 • edited Loading

Choose a reason for hiding this comment

JGamache-autodesk Oct 5, 2023

Choose a reason for hiding this comment

AramAzhari-adsk Oct 5, 2023

Choose a reason for hiding this comment

vlasovi left a comment

Choose a reason for hiding this comment

AramAzhari-adsk commented Oct 5, 2023

vlasovi commented Oct 5, 2023

JGamache-autodesk commented Oct 5, 2023

vlasovi commented Oct 5, 2023

JGamache-autodesk commented Oct 5, 2023

vlasovi commented Oct 5, 2023

vlasovi left a comment

Choose a reason for hiding this comment

JGamache-autodesk left a comment

Choose a reason for hiding this comment

AramAzhari-adsk commented Oct 12, 2023

vlasovi Oct 5, 2023 •

edited

Loading

vlasovi Oct 5, 2023 •

edited

Loading