Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Simplify default color handling in GLSL #1452

Merged

Conversation

jstone-lucasfilm
Copy link
Member

This changelist simplifies default color handling in GLSL, making the renderer responsible for generating fallback textures when images are not found on disk, and removing associated dynamic branches in GLSL shader code.

Two advantages of this simplification are:

  • Hydra Storm and other MaterialXGenGlsl integrations can now render materials that reference 1x1 images without needing special-case logic.
  • The render performance of MaterialXGenGlsl shaders that reference images is slightly improved, as dynamic branches exact a performance cost in some situations.

This new logic doesn't yet handle color space differences between missing images and default colors, and we should address this across languages in a future improvement.

This changelist simplifies default color handling in GLSL, making the renderer responsible for generating fallback textures when images are not found on disk, and removing associated dynamic branches in GLSL shader code.

Two advantages of this simplification are:
- Hydra Storm and other MaterialXGenGlsl integrations can now render materials that reference 1x1 images without needing special-case logic.
- The render performance of MaterialXGenGlsl shaders that reference images is slightly improved, as dynamic branches exact a performance cost in some situations.

This new logic doesn't yet handle color space differences between missing images and default colors, and we should address this across languages in a future improvement.
@ashwinbhat
Copy link
Contributor

Do we have a preference on default color being black, white or gray?
Setting to black usually makes it harder to debug problems.

@jstone-lucasfilm
Copy link
Member Author

@ashwinbhat For authored materials, we're still using the default input on each image node as the fallback color when that image is missing. Following the specification, this input has a default value of zero in all channels, but material authors can always set it to another color for better visualization:

https://github.com/AcademySoftwareFoundation/MaterialX/blob/main/documents/Specification/MaterialX.Specification.md#texture-nodes

@kwokcb
Copy link
Contributor

kwokcb commented Aug 17, 2023

Hi @jstone-lucasfilm, it seems this is grabbed at bind time of the texture but seems there still needs some extra logic to handle when the defaut changes to rebind a new texture otherwise no update will occur ? Not sure if it's there, so checking.

@jstone-lucasfilm
Copy link
Member Author

@kwokcb You're correct about this, and for completeness we would need to invalidate the cached image when the default color changes, allowing it to be regenerated on the next render.

There's another valid perspective here, though, which is that the default input of an image node will now follow the behavior of other image sampling inputs such as uaddressmode, vaddressmode, and filtertype, which are only accessed in GLSL when a texture is first bound, and will not visually update if the user modifies these properties during real-time rendering.

I wonder if the best approach here would simply be to document this behavior of image sampling inputs in real-time engines, and not to attempt to invalidate textures automatically in MaterialXRenderGlsl, since this would create surprising behaviors for real-time clients of MaterialX.

@kwokcb
Copy link
Contributor

kwokcb commented Aug 17, 2023

The latter sounds reasonable. I don't think folks would generally ever change the default.

I would still like to suggest we add in the defaultinput attribute on to the output of <image>, <tiledImage>, and <triplanarmapping> as there is no hint as to which actual input to use to perform the update. I think it's better than the hard-coded default values anyways which aren't in sync with default. A minor change that shouldn't affect anything but serve as a useful hint.

@jstone-lucasfilm
Copy link
Member Author

@kwokcb Good point, and I think it's just an oversight that our image nodes don't yet have defaultinput settings. Let's plan to address that in a separate pull request.

@kwokcb
Copy link
Contributor

kwokcb commented Aug 17, 2023

Sure thing. I can log a new issue.

BTW, May want to do a a sanity check with the Web viewer as ESSL derives from GLSL code gen and don't recall if anything is done explicitly in the renderer there.

@meshula
Copy link

meshula commented Aug 18, 2023

The elimination of conditional logic in the shaders, and the elimination of comparisons with a sentinel image, yield

  • a simplification that makes me very happy ;)
  • an elimination of an unintuitive heuristic behavior
  • shader execution efficiency

I do wonder if there also needs to be a unit test introduced that demonstrates the validity of a 1x1 texture, and the fallback behavior? Or is this sort of validation strictly in the domain of viewer/engine conformance, so there's nothing we can do on the matx side of the fence?

@kwokcb
Copy link
Contributor

kwokcb commented Aug 18, 2023

Maybe we should extend the render test suite as part of the test suite here
Just need to set to load a image that does not exist. Can do each "image" node type.

@jstone-lucasfilm
Copy link
Member Author

Agreed, @meshula, and I was thinking along the same lines as @kwokcb. We could add an example material that intentionally references a missing texture, so that we can easily verify that default color rendering is working as expected.

@jstone-lucasfilm jstone-lucasfilm merged commit 79433cc into AcademySoftwareFoundation:main Aug 18, 2023
13 checks passed
Michaelredaa pushed a commit to Michaelredaa/MaterialX that referenced this pull request Oct 21, 2023
This changelist simplifies default color handling in GLSL, making the renderer responsible for generating fallback textures when images are not found on disk, and removing associated dynamic branches in GLSL shader code.

Two advantages of this simplification are:
- Hydra Storm and other MaterialXGenGlsl integrations can now render materials that reference 1x1 images without needing special-case logic.
- The render performance of MaterialXGenGlsl shaders that reference images is slightly improved, as dynamic branches exact a performance cost in some situations.

This new logic doesn't yet handle color space differences between missing images and default colors, and we should address this across languages in a future improvement.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants