guard against past-end iterator invalidation

[ysiewappl]

Hi:

This PR fixes a crash we're seeing on some exports where layered shaders are involved, leading to a mistaken assumption in array sizes and resulting in past-the-end-iterator invalidation.

Please review and raise any concerns.

[JGamache-autodesk]

This code patches a symptom without addressing the root cause. The program will stop crashing at this exact location, but it will still produce corrupted output because we need to assign a UV set name to each one of the inputs declared in the material:

def Material "foo1SG_map1_uvSet1" (kind = "assembly")
{
    token inputs:file1:varname = "map1" // Potentially incorrect
    token inputs:file2:varname = "uvSet1" // Potentially incorrect
    token inputs:file3:varname =   // Missing
    ...
}

I would be very interested in seeing a sample scene that reproduces the crash. It is possible that we want to detect layered materials and save only the active layer if exporting for UsdPreviewSurface.

Generating a simple scene that reproduces the crash will also allow adding it as a unit test. There are a few materials in test\lib\usd\translators\UsdExportUVSetMappingsTest\UsdExportUVSetMappingsTest.ma that could probably be layered on a new plane to reproduce the issue quickly.

[ysiewappl]

Hi @JGamache-autodesk after a bit of investigation the following case will trigger the crash:

Basically having two file texture nodes that are named the same (but in different namespaces), along with the shading engine as well will cause the _nodesWithUVInput array to have a size of 2 while the largestSet array only has size 1.

I haven't yet looked into what exactly all the code surrounding material mappings is doing, so I can't speak to the issue of generating corrupted output. But I still think adding this condition (or an assert) makes sense since there isn't any reasonable way to continue if the array sizes are mismatched.

[JGamache-autodesk]

That is interesting. The export code is based on the expectations of names being unique. This expectation has been proven wrong in this scenario when using non-namespaced names.

The correct fix, if you want to attempt it, is to use the full namespaced name for nodeWithUVInput. The starting point for the fix would be to replace depNodeFn.name() in https://github.com/Autodesk/maya-usd/blob/dev/lib/usd/translators/shading/usdFileTextureWriter.cpp#L217 with depNodeFn.absoluteName().

This should work fine downstream since you already made a fix in PR #1062 to handle the extra : this will add in the attribute name so chances are that the fileTextureWriter change will be the only one required.

[ysiewappl]

The correct fix, if you want to attempt it, is to use the full namespaced name
for nodeWithUVInput. The starting point for the fix would be to replace
depNodeFn.name() in
https://github.com/Autodesk/maya-usd/blob/dev/lib/usd/translators/shading/usdFileTextureWriter.cpp#L217
with depNodeFn.absoluteName().

Hi, thanks for taking a look. I might be missing something here because even
without doing that, the names in the _nodesWithUVInput map already have the
absolute names in there anyway:

* thread #1, queue = 'com.apple.main-thread', stop reason = breakpoint 6.2
    frame #0: 0x00000001bf59130f libmayaUsd.dylib`pxrInternal_v0_20__pxrReserved__::(anonymous namespace)::_UVMappingManager::_UVMappingManager(this=0x00007ffeefbf88c8, material=0x00007ffeefbf8958, assignmentsToBind=size=1) at shadingModeExporterContext.cpp:546:52
   536 	        TfTokenVector largestSet;
   537 	        for (const auto& iter : mappingGroups) {
   538 	            if (iter.second.size() > largestSize) {
   539 	                largestSize = iter.second.size();
   540 	                largestSet = iter.first;
   541 	            }
   542 	        }
   543
   544 	        // Update the original material with the most common mapping:
   545 	        if (largestSize) {
-> 546 	            TfTokenVector::const_iterator itNode = _nodesWithUVInput.cbegin();
   547 	            TfTokenVector::const_iterator itName = largestSet.cbegin();
   548 	            for (; itNode != _nodesWithUVInput.cend(); ++itNode) {
   549 	                TfToken inputName(
   550 	                    TfStringPrintf("%s:%s", itNode->GetText(), _tokens->varname.GetText()));
   551 	                UsdShadeInput materialInput = material.GetInput(inputName);
   552 	                // NOTE: (yliangsiew) If we have a mismatch in array sizes, we default to assigning the
   553 	                // most common mapping to the materials that don't have inputs.
   554 	                if (itName != largestSet.cend()) {
   555 	                    materialInput.Set(*itName);
   556 	                    ++itName;
Target 0: (Maya) stopped.
(lldb) p _nodesWithUVInput
(pxrInternal_v0_20__pxrReserved__::TfTokenVector) $46 = size=2 {
  [0] = {
    _rep = {
      _ptrAndBits = 0x00000001e0bfa3c1
    }
  }
  [1] = {
    _rep = {
      _ptrAndBits = 0x000000017e2e5151
    }
  }
}
(lldb) p largestSet
(pxrInternal_v0_20__pxrReserved__::TfTokenVector) $47 = size=1 {
  [0] = {
    _rep = {
      _ptrAndBits = 0x00000001bdeb68b0
    }
  }
}
(lldb) p _nodesWithUVInput[0].GetText()
(const char *) $48 = 0x00000001e0bfa3c1 "file1"
(lldb) p _nodesWithUVInput[1].GetText()
(const char *) $49 = 0x000000017e2e5151 "NewNamespace1:file1"
(lldb) p largestSet[0].GetText()
(const char *) $50 = 0x00000001bdeb68b1 "map1"
(lldb)

Nevertheless your suggestion makes sense, so I went ahead and put the absolute
name in the inputName so that the inputs created will always use the absolute
name. However, that still doesn't address the original issue that the array
sizes will still end up being different in the example graph above.

The program will stop crashing at this exact location, but it will still
produce corrupted output because we need to assign a UV set name to each one
of the inputs declared in the material:

I updated the code in this case to always assign the most-common mapping to the
materials that don't have inputs in order to address this concern, rather than
exiting early. Is this "more" correct behaviour?

[JGamache-autodesk]

OK, I have built a scene that is exactly like yours by texturing a sphere, saving it, then re-importing it as a Maya file. this results in a perfect namespaced duplicate:

When exporting to USD I end up with 2 materials, each one having a single varname input without the iterators getting out of sync.
The only way I see to get the behavior you are describing is if the exporter tried to merge the two materials into a single one or if the _UVMappingManager class was re-used. This would require either an export option that I am not aware of, or some unpublished changes in your development branch.
BTW I tried the "Ignore namespace" option just in case it would be the one triggering the issue, but the export failed after ambiguity was discovered, so it was not the one causing the problem.
Can you help me reproduce the problem on my side?

[ysiewappl]

Hi @JGamache-autodesk:

I've uploaded a test scene that should reproduce the issue when exported with the following command:

mayaUSDExport -v -sl -f "/Users/yliangsiew/tmp/test_output.usda" -ignoreWarnings true  -skl "auto" -skn "auto" -defaultMeshScheme "catmullClark"

recreate_crash_adsk.tar.gz

I just double-checked against 991913f (current top of dev at time of writing) and the issue still occurs without the proposed fix in this PR.

Hopefully you are able to reproduce this! If not, let me know and I can try to investigate further.

[JGamache-autodesk]

I can reproduce the crash. Thanks for providing a file.
Printing the value of usdPath in the constructor of PxrUsdTranslators_FileTextureWriter returns the value "/group1/Looks/usdPreviewSurface1SG/NewNamespace1_file1" which might be the issue. I would have expected NewNamespace1_usdPreviewSurface1SG for the material name. This might be why we ended up reusing a single material UsdPrim with a second varname input added. Now I need to find out where that name gets generated and fix it so we end up creating the second material UsdPrim.

[JGamache-autodesk]

Remove the MNamespace::stripNamespaceFromName() call in UsdMayaShadingModeExportContext::MakeStandardMaterialPrim() and your scene will export correctly.

[ysiewappl]

Remove the MNamespace::stripNamespaceFromName() call in UsdMayaShadingModeExportContext::MakeStandardMaterialPrim() and your scene will export correctly.

Hi @JGamache-autodesk : thanks for looking into this! I've updated the PR accordingly by avoiding having the namespace removed and limited the earlier changes to just a TF_VERIFY instead.