MaterialX icon indicating copy to clipboard operation
MaterialX copied to clipboard

ShaderPort::getPath() should be parented at surfacematerial

Open ashwinbhat opened this issue 2 years ago • 4 comments

Consider the following material document

<materialx version="1.38" colorspace="lin_rec709">
  <UsdPreviewSurface name="SR_plastic" type="surfaceshader">
    <input name="diffuseColor" type="color3" value="0.10470402, 0.24188282, 0.81800002" />
    <input name="roughness" type="float" value="0.32467532157897949" />
    <input name="ior" type="float" value="1.5" />
  </UsdPreviewSurface>
  <surfacematerial name="USD_PlasticOne" type="material">
    <input name="surfaceshader" type="surfaceshader" nodename="SR_plastic" />
  </surfacematerial>
  <surfacematerial name="USD_PlasticTwo" type="material">
    <input name="surfaceshader" type="surfaceshader" nodename="SR_plastic" />
  </surfacematerial>  
</materialx>

USD_PlasticOne and USD_PlasticTwo are two valid shaders. However, when we use the shader reflection

MaterialX::ShaderPort::getPath() will report SR_plastic/diffuseColor for both surfacematerial

Additional details on slack here

ashwinbhat avatar Sep 01 '23 01:09 ashwinbhat

I think the Slack details and this are unclear.

  1. diffuseColor path must point to SR_plastic/diffuseColor and not SR_plastic as it's an input. 1st issue in Slack.

  2. If it's the surfaceshader input on each surfacematerial and it's returning SR_plastic/diffuseColor then this needs to be looked at as it could be the port being looked at is the upstream one. This can be compared against what is returned from getNamePath() outside of code generation to see if there is a discrepancy.

kwokcb avatar Sep 01 '23 02:09 kwokcb

I created a test case to demonstrate USD/HdSt workflow here to highlight the issue. Input paths for both surface materials are identical though the generated shader code isn't. Shader one input paths: SR_plastic/diffuseColor SR_plastic/emissiveColor SR_plastic/useSpecularWorkflow SR_plastic/specularColor SR_plastic/metallic SR_plastic/roughness SR_plastic/clearcoat SR_plastic/clearcoatRoughness SR_plastic/opacity SR_plastic/opacityThreshold SR_plastic/ior SR_plastic/normal SR_plastic/displacement SR_plastic/occlusion

Shader two input paths: SR_plastic/diffuseColor SR_plastic/emissiveColor SR_plastic/useSpecularWorkflow SR_plastic/specularColor SR_plastic/metallic SR_plastic/roughness SR_plastic/clearcoat SR_plastic/clearcoatRoughness SR_plastic/opacity SR_plastic/opacityThreshold SR_plastic/ior SR_plastic/normal SR_plastic/displacement SR_plastic/occlusion

ashwinbhat avatar Sep 06 '23 23:09 ashwinbhat

I'd expect the reflection to be the same since the surfaceshader is a shared instance.

Each code can be updated independently, but they will point back to the same path on the shared instance. There is no "parent" concept as with Usd as materials in MTLX are not containers and hence do not affect pathing. I have raised this difference before.

I'm not sure of the workflow:

  1. If you want to override some values with the inputs specified at the USD material level. As each shader each source would be updated separately so I don't see the issue.

  2. If your trying to hash this shader base on path for some reason then you know which surfacematerial the shader is coming from so could include this.

  3. If you want to hash the source code, it's already different but if you want the actual uniforms to embed the surfacematerial as part of the surfaceshader's port naming than you could add a prefix to it via GenContext::addInputSuffix().

These are just possible guesses as I'm unsure what the actual issue is ? Is it one of these or something else ?

For code differences I see this (GLSL):

void main()
{
    surfaceshader SR_plastic_out = surfaceshader(vec3(0.0),vec3(0.0));
    IMP_UsdPreviewSurface_surfaceshader(SR_plastic_diffuseColor, SR_plastic_emissiveColor, SR_plastic_useSpecularWorkflow, SR_plastic_specularColor, SR_plastic_metallic, SR_plastic_roughness, SR_plastic_clearcoat, SR_plastic_clearcoatRoughness, SR_plastic_opacity, SR_plastic_opacityThreshold, SR_plastic_ior, SR_plastic_normal, SR_plastic_displacement, SR_plastic_occlusion, SR_plastic_out);
    material USD_PlasticTwo_out = SR_plastic_out;
    float outAlpha = clamp(1.0 - dot(USD_PlasticTwo_out.transparency, vec3(0.3333)), 0.0, 1.0);
    out1 = vec4(USD_PlasticTwo_out.color, outAlpha);
    if (outAlpha < u_alphaThreshold)
    {
        discard;
    }
}
void main()
{
    surfaceshader SR_plastic_out = surfaceshader(vec3(0.0),vec3(0.0));
    IMP_UsdPreviewSurface_surfaceshader(SR_plastic_diffuseColor, SR_plastic_emissiveColor, SR_plastic_useSpecularWorkflow, SR_plastic_specularColor, SR_plastic_metallic, SR_plastic_roughness, SR_plastic_clearcoat, SR_plastic_clearcoatRoughness, SR_plastic_opacity, SR_plastic_opacityThreshold, SR_plastic_ior, SR_plastic_normal, SR_plastic_displacement, SR_plastic_occlusion, SR_plastic_out);
    material USD_PlasticOne_out = SR_plastic_out;
    float outAlpha = clamp(1.0 - dot(USD_PlasticOne_out.transparency, vec3(0.3333)), 0.0, 1.0);
    out1 = vec4(USD_PlasticOne_out.color, outAlpha);
    if (outAlpha < u_alphaThreshold)
    {
        discard;
    }
}

The downstream naming of the material differs, but the inputs are not defined at that node. They are defined at the surfaceshader, thus the path reflects this.

kwokcb avatar Sep 08 '23 20:09 kwokcb

https://forum.aousd.org/t/instanced-shaders-and-connections-broken/666

Is this the same situation?

kwokcb avatar Sep 18 '23 23:09 kwokcb