PixarAnimationStudios / OpenUSD

Universal Scene Description
http://www.openusd.org
Other
5.98k stars 1.18k forks source link

Incremental improvement requests for the USD MaterialX parser #3177

Open JGamache-autodesk opened 1 month ago

JGamache-autodesk commented 1 month ago

Description of Issue

We have the following issues we (Autodesk) would like to fix in the usdMtlx parser code:

  1. Shaders should retain the names they had in the MaterialX document. This is already addressed by @dgovil in #3147.
  2. Surface shaders should use the output port name declared in the node definition instead of a hardcoded outputs:surface.
  3. The loader should traverse connections from surface shader to other top-level nodes, allowing loading a flattened material.
  4. The loader should not systematically promote all input ports of the surface shader to the UsdMaterial.
  5. Nodes and NodeGraphs should only be created as references if they are shared between multiple materials in the same document.
  6. The resulting layer should declare /MaterialX as the defaultPrim.

Now, that is a lot of behavior changes calling for 6 extra environment variables and/or SDF_FORMAT_ARGS. Can these be grouped somehow? Any other changes you (@dgovil , @spiffmon) would like to see while I am working on that codebase?

I will try to submit one PR per issue to insure easy things stay easy while complex things get discussed and reviewed accordingly.

jesschimein commented 1 month ago

Filed as internal issue #USD-9887

spiffmon commented 1 month ago

I'd like more information on 3 and 4, specifically:

If the answer to both of those is Yes, then from my POV none of those changes are particularly contentious and seem unlikely to cause problems for people (as Dhruv already argued for #1)... most of them are in the "composition details" to which people adding overs on a UsdStage should be mostly oblivious. And it seems uncommon for people to be changing the generated/default targets of a Material's outputs:surface, and if someone already made an override for a Material input, well then it will remain regardless of what specs the underlying Material contains.

So could we be so bold as to say it all goes behind one env var? (which could still be deployed incrementally with PR-per-behavior-change)?

JGamache-autodesk commented 1 month ago

For 3, yes, we stop putting them in a separate scope and referencing them by default. We will do that only if the node is shared between multiple materials.

For 4, that one is a difficult question. We definitely want a material interface exposed to lookdev artists that contains only the recommended set of attributes they should tweak. We think the default of promoting every attribute of the surface material node is not the best solution. Maybe we need a few iterations on 4 so let's start by looking at what MaterialX recommends.

This can be explored on the web by looking at the "Property Editor" of the MaterialXWeb Viewer:

From this I can deduce the following rules: The material interface is composed of:

Would this be OK with you too?

JGamache-autodesk commented 1 month ago

Reproducing the MaterialX property editor look also means that the promoted inputs on the Material interface will require UsdShade metadata (uimin, uimax, uilabel, uifolder, enum labels and values) to be added to get a correct display. I shall use the Sdr metadata names when they exist, and revert to MaterialX ones for those unknown to USD.

spiffmon commented 1 month ago

So what you are describing for 4 seems like a reasonable fallback behavior, but looking at standard_surface_brick_procedural.mtlx, I would have assumed that the Material interface would be constructed from all of the inputs that have interfacename set... but on rereading the spec I don't see any indication that that is the intention of interfacename.

It sounds like the implicit rules (which I didn't find in the spec?) are "authored value for the surface inputs, or inputs anywhere having (any?) ui metadata provided" ? Both of those rules seem problematic in general, and that the spec would benefit from having a more intentional way of crafting an interface on the surfacematerial node (or at the least, suppressing application of those two rules to any inputs within the network, since sometimes you'll want to hide things that need authored values, or suppress elements of an more general-purpose nodegraph you are referencing).

But in the absence of such additions to the spec, emulating the behavior of the MaterialXWeb viewer does sound like a good position for usdMtlx to adopt!

Thanks, @JGamache-autodesk !

JGamache-autodesk commented 1 month ago

Thanks to Nikola's very sharp mind I happen to have the answer to that one. He asked what happens if we put the standard_surface node itself inside a NodeGraph. So I would like to propose a third clause to item 4:

4c. If the surfacematerial node is connected directly to a NodeGraph, then this NodeGraph becomes a UsdShadeMaterial node, using the exact interface declared on that NodeGraph.

If we have this NodeGraph: image Connected directly to the surface material: image Then we can directly take the ImageMaterial NodeGraph and transform that into a UsdShadeMaterial with two inputs on its material interface.

JGamache-autodesk commented 1 month ago

Also, about

all of the inputs that have interfacename set

I don't think I need to explicitly check that these inputs are connected to something using interfacename. I will probably iterate the list of inputs of the NodeGraph, which would make these brick inputs become input attributes of the UsdShadeMaterial node:

    <input name="brick_color" type="color3" value="0.661876, 0.19088, 0" uiname="Brick Color" uifolder="Color" />
    <input name="hue_variation" type="float" value="0.083" uimin="0" uimax="1" uiname="Hue Variation" uifolder="Color" />
    <input name="value_variation" type="float" value="0.787" uimin="0" uimax="1" uiname="Value Variation" uifolder="Color" />
    <input name="roughness_amount" type="float" value="0.853" uimin="0" uimax="1" uiname="Roughness Amount" uifolder="Roughness" />
    <input name="dirt_color" type="color3" value="0.56372, 0.56372, 0.56372" uiname="Dirt Color" uifolder="Dirt" />
    <input name="dirt_amount" type="float" value="0.248" uimin="0" uimax="1" uiname="Dirt Amount" uifolder="Dirt" />
    <input name="uvtiling" type="float" value="3" uisoftmin="1" uisoftmax="16" uiname="UV Tiling" uifolder="Texturing" />

Letting an unconnected input show up on the interface and do nothing is annoying, but I don't think it is fatal. I can always add a check to skip unconnected inputs if you feel strongly about this though.

ld-kerley commented 1 month ago

When @spiffmon mentioned the specification above, it immediately made me wish that we weren't trying to reverse engineer heuristics here, but instead just reading the (possibly yet to be written) spec.

I wonder if we should take this back to the MaterialX project itself, and see if there is a canonical and explicit way we can describe what we need in MaterialX itself, then UsdMtlx can just follow whatever convention is determined by the project - and UsdMtlx will also then play nice with any other integrations that may not use USD - or have taken the path to re-implement the specification themselves.

I would also be interested to hear if the folks from sideFX have any thoughts here @chrisrydalch - and I'm guessing they have already had to broach this problem in their MaterialX/USD integration in Solaris.

I will highlight this section of the current MaterialX specification - https://github.com/AcademySoftwareFoundation/MaterialX/blob/main/documents/Specification/MaterialX.Specification.md#example-pre-shader-compositing-material - which appears to have an example that might be a good template for solidifying these ideas, though it does leverage the creation of a new <nodedef> element to surface the material interface - and thats not something that UsdMtlx supports.