Apple: Use Node name instead of NodeDef name for SurfaceShaders

[dgovil]

Description of Change(s)

Change the translated name of surface shaders from the node defs name to the Node's name, toggleable by setting an env var of PXR_MTLX_SUPPORT_MATERIAL_INHERITS

UsdMtlx was using the node defs name for surface shaders as an attempt to support material inheritance, the logic was using the most generic name possible to allow easier overriding.

Unfortunately this causes issues where the imported names for just the surface shader does not match the input mtlx file.

While I understand the original rationale, I think that until we find a better heuristic, there is more value in keeping the naming consistent and intuitive. I do not believe material inheritance is a common feature in MtlX libraries, and certainly does not feature in the set of mtlx files provided by the MaterialX library or with OpenUSD.

For pipelines that require this, I suggest they instead set the PXR_MTLX_SUPPORT_MATERIAL_INHERITS boolean env var, until a better heuristic can be found.

Fixes Issue(s)

• usdMtlx does not consistently preserve MaterialX node names #3100

• I have verified that all unit tests pass with the proposed changes

• I have submitted a signed Contributor License Agreement

[jesschimein]

Filed as internal issue #USD-9816

[jesschimein]

/AzurePipelines run

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[dgovil]

I confirmed with the MaterialX project that Material Inherits are not a supported feature in MaterialX currently.
Based on that, I wonder if I should remove the env-var switch here and just use the new proposed behaviour? Or should we keep the switch in case people need to transition to the new naming scheme?

[spiffmon]

I personally like the idea of just improving the behavior unconditionally, but we're also not using MaterialX in production yet, so no archival issues or overrides to worry about. Maybe ask the question on the forum and the slack channel Dhruv, with the alternative being to default the env var to the new behavior, still allowing folks to disable it if they do have important overrides on older translations of still-live mtlx files?

[dgovil]

Will do, though just to clarify, the PR does have the default for the env var to OFF, in which case the new behaviour is picked by default.

[JGamache-autodesk]

I plan to go have a look as well in that usdMtlx codebase because we came up with a list of things we want to either fix or implement differently, and in the pre-investigations I have checked to see if the registry and or syntax allows multiple readers (it doesn't) and if there was a way to pass parameters to the loader to make this explicit instead of implicit (there is, look for SDF_FORMAT_ARGS).

I think having these arguments saved in the USD data would solve the issue when loading a stage without having set the right combination of env vars, and would solve the composition problem of assembling assets from different studios requiring different combination of usdMtlx env vars.

So, my recommendation on this PR would be to allow passing a materialInherits format argument in the referencing call itself. I think there is an implementation example in the Alembic reader, and I have seen it used also in some of the file format plugins managed by Adobe.

@spiffmon, does this make sense?

[dgovil]

@JGamache-autodesk in general I like your idea but for this specific issue I have a few thoughts.

I think we should aim to remove the old behaviour in this case and use the env var to help people transition over a predetermined period.

I worry that if it's a file format arg, that means we're encouraging keeping the old behaviour around for longer.

Since MaterialX itself doesn't support this feature, my thinking is that we do the following:

1. Switch to the new behaviour by default, and have the env variable to help people transition.

2. Eventually remove the environment variable all together.

3. If MaterialX introduces material inherits for real, we evaluate then whether it needs a change to the conversion and if so, introduce a file format argument at that time.

[spiffmon]

Deciding which approach to take seems to depend on whether we think the MaterialX-USD-integration is mature enough that people are expecting the "archival document" promise of MaterialX to apply to MaterialX-consuming-USD documents, as well? I don't know the answer to that question...

[tcauchois]

If we're hoping to fully delete the old behavior, it seems better to keep this out of assets, and avoid plumbing it through.

While I appreciate the utility of environment variables in facilitating a soft transition, they also complicate deployment and so I'm trying to crack down on some of ours internally. Two questions then:
(1) Is there a process that would give us confidence in a variant of this change where we just switch to the new behavior with no rollback flag? i.e. asking relevant companies, posting on the forums, bringing it up in an ASWF meeting?
(2) If #1 feels too risky, can we change the env var to USDMTLX_OLD_SURFACE_NODE_NAMES (or something) and put in a // deprecated label and a warning that it will go away in 25.02 or 25.05?

It seems like the benefit of the nodedef names is that, whenever material inheritance is added to MaterialX that will facilitate that feature in UsdMtlx, right? Is the benefit of node names just that they are slightly less arcane in the shadergen? I wonder if we could pass some off-label node data with the node name and add that as a comment somewhere in the shader or something instead. (Please excuse my shallow understanding of the technical meat here...)

[dgovil]

We had discussed it in the MaterialX meetings and the consensus seems to be that the new behaviour is good.

Nobody could talk to whether the old behaviour needed to stay for deprecation reasons. It would only come up if someone was referencing a MTLX file and also applying an override.

I think pragmatically, it's safe to remove the env var and just move to the new behaviour. UsdMtlx currently doesn't have a wide adoption rate in pipelines, and the deprecation period is something people would ignore.

If that sounds reasonable, I can update the PR to remove the old behaviour.

[tcauchois]

This is definitely personal judgement here, and I'm fine going with your change either way/going with a consensus, now that I have a better understanding of the context, but I'd feel slightly better dropping the env var and am also happy to be on record about that and assume the risk. Let me know if you intend to rev it and I can hold off on checking it in.

[dgovil]

Sounds good to me. I just updated with a commit to remove the env var and previous behaviour.