MaterialX icon indicating copy to clipboard operation
MaterialX copied to clipboard

The `default` value for <image> does not correctly update until Graph Editor restart

Open hybridherbst opened this issue 9 months ago • 11 comments

This is a weird issue – I was trying to understand why changing the default value of didn't work and realized that somehow the value is "stuck" in the Graph Editor until restart.

Steps to reproduce

  1. Save this graph as "myGraph.mtlx":
<?xml version="1.0"?>
<materialx version="1.39" colorspace="lin_rec709">
  <surfacematerial name="Marble_3D" type="material" xpos="17.391304" ypos="0.000000">
    <input name="surfaceshader" type="surfaceshader" nodename="standard_surface_surfaceshader" />
  </surfacematerial>
  <standard_surface name="standard_surface_surfaceshader" type="surfaceshader" xpos="12.724638" ypos="-3.362069">
    <input name="base_color" type="color3" nodename="image_color3" />
  </standard_surface>
  <image name="image_color3" type="color3" xpos="9.942029" ypos="-3.232759">
    <input name="default" type="color3" value="0.0264807, 0.0813303, 0.570401" />
    <output name="out" type="color3" />
  </image>
</materialx>
  1. Open MaterialXGraphEditor
  2. Load "myGraph.mtlx"
  3. Note you get some blueish result: Image
  4. Change the default color to yellow
  5. Note there's no change in the visualization
  6. Save the file (override the existing one)
  7. Click File > Reload
  8. Note it's still the blue color despite the default being yellow Image
  9. Quit GraphEditor, launch it again, and load the file
  10. Now it's yellow!
  11. You can repeat this – the value can only ever be changed once and then a restart is needed.

hybridherbst avatar Jun 03 '25 20:06 hybridherbst

It's a bit worse actually:

  • ALL defaults from all graphs will be on that first value until restart of Graph Editor
  • MaterialXView doesn't show defaults at all (stays black)
  • MaterialXWebViewer also doesn't show defaults (stays black)

Should I open separate issues for these?

hybridherbst avatar Jun 03 '25 21:06 hybridherbst

@hybridherbst This sounds like a familiar set of issues in MaterialXRenderGlsl, and I think the ideal would be to post a single GitHub Issue with an overview of the problem across our GLSL renderers.

jstone-lucasfilm avatar Jun 03 '25 21:06 jstone-lucasfilm

This is probably the decision made at then time that improvements where made for default color with respect to USD handling -- in that value changes for this input do not trigger shader rebuilds.

Background: There was a related talk at the time -- I believe spearheaded by @JGamache-autodesk on what things should automatically be considered to require shader rebuilds (branches, string/enum changes). I don't think there is a formal way to set an input as "requiring a shader rebuild / topological change etc" via the data model so all that what was added was to add more cases which always caused rebuilds. A possible data model approach could be to add more hints -- but these could be renderer / shading language specific so wasn't fully hashed out AFAIK.

kwokcb avatar Jun 04 '25 15:06 kwokcb

This is because we ask the DCC to upload the "default" color as a 1x1 texture because adding GLSL code to discover that a texture is missing is very costly.

Some DCC will do nothing and you get the OpenGL default value of (0,0,0) when sampling an empty texture. Some other DCC will create a single "default" texture and re-use it everywhere. Which means the first one seen sticks. Fixing that completely requires keeping one texture per RGB value and uploading the right one based on the current "default" value.

JGamache-autodesk avatar Jun 04 '25 16:06 JGamache-autodesk

Which makes it unrelated to the question of rebuilding the shader or not. The generated GLSL code does not change to follow the current value of the "default" parameter.

JGamache-autodesk avatar Jun 04 '25 16:06 JGamache-autodesk

Thanks for the explanations!

But surely it's a bug that even when a shader recompiles, the value stays the same or is not applied at all? I would hope that there's some language in the spec to ensure that default values are useful at all. If a viewer can decide to simply not generate any default values at all, they don't feel very useful.

For the current graphs I'm working on I had to resort to shipping my own 1x1px textures with the same default values as the "default" property should apply, because it's so inconsistent between viewers – that works, but feels like it defeats the purpose of having the property in the first place.

hybridherbst avatar Jun 04 '25 18:06 hybridherbst

Sorry for not explaining correctly. It is not your job to create and ship these fallback textures. They should be auto-generated by the application.

In the case of the MaterialX viewer and graph editor, the 1x1 fallback image is created in the ImageHandler. If you look at that code you will see that is uses resolvedFilePath to cache the fallback image. This is really really bad since: 1- It will not update if the default color changes. 2- If you were to copy the missing image to the right location on the filesystem, a refresh would still fail to load it.

To fix that requires using a name like "ImageHandler_FallbackImage_0xF356F1" where 0xF356F1 is the image RGB converted to a hex value instead of using resolvedFilePath in the cache. Would first need to check if there is already an image by that name in the cache.

JGamache-autodesk avatar Jun 04 '25 19:06 JGamache-autodesk

I see, thank you for the explanation. That might also explain why I’m not seeing any refresh at all until restart, even for other files – the value in my case is just “” (no image bound, with the idea of it being selectable in the viewer/editor) – so until restart all textures resolved to “” get the same first-time fallback.

hybridherbst avatar Jun 04 '25 21:06 hybridherbst

Hi @JGamache-autodesk , I guess I was out-of-date with the current implementation which does not pass defaultcolor to the shader at all for GLSL, unlike say OSL (*). I was assuming you could branch on color vs image conditional which would always evaluate both branches.

I guess I worry that if your constantly creating new textures per colour that you could have a lot of these -- though I hope these will be flushed when they are dereferenced. However, I would have assumed that a new file load should have flushed the image cache so at least this workflow would work -- guess it's worth to check cache dereferencing / clearing.

(*) By the way the current GLSL code does not check for constant color addressing mode so that will not return the defaultcolor when you have a valid image path.

kwokcb avatar Jun 04 '25 22:06 kwokcb

Hi @kwokcb. Nothing prevents us from changing the implementation and testing new ideas. One avenue to explore is to change the ND_image... code from:

void mx_image_vector3(sampler2D tex_sampler, int layer, vec3 defaultval, vec2 texcoord, int uaddressmode, int vaddressmode, int filtertype, int framerange, int frameoffset, int frameendaction, vec2 uv_scale, vec2 uv_offset, out vec3 result)
{
    vec2 uv = mx_transform_uv(texcoord, uv_scale, uv_offset);
    result = texture(tex_sampler, uv).rgb;
}

to

void mx_image_vector3(sampler2D tex_sampler, int layer, vec3 defaultval, vec2 texcoord, int uaddressmode, int vaddressmode, int filtertype, int framerange, int frameoffset, int frameendaction, vec2 uv_scale, vec2 uv_offset, out vec3 result)
{
    vec2 uv = mx_transform_uv(texcoord, uv_scale, uv_offset);
    if (textureSize(tex_sampler, 0) == ivec2(0, 0)) {
        result = defaultval;
    } else {
        result = texture(tex_sampler, uv).rgb;
    }
}

This was a definite "don't do that" in 2010, but maybe video cards are now better at handling these kind of calls. Would definitely solve the issue on OpenGL pipelines.

JGamache-autodesk avatar Jun 05 '25 13:06 JGamache-autodesk

Thanks for the details Jerry. This was what I assumed was occurring or desired to be occurring. ( In any case adding defaultval into the function arguments will allow for constant address mode to be fixable as well. )

Texture size query should be quite low cost and supported by all target hardware languages including ESSL (3+), Vulkan, Metal etc.

kwokcb avatar Jun 05 '25 14:06 kwokcb