-
Notifications
You must be signed in to change notification settings - Fork 366
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
Simplify default color handling in GLSL #1452
Conversation
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.
Do we have a preference on default color being black, white or gray? |
@ashwinbhat For authored materials, we're still using the |
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 |
@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 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. |
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 |
@kwokcb Good point, and I think it's just an oversight that our image nodes don't yet have |
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. |
The elimination of conditional logic in the shaders, and the elimination of comparisons with a sentinel image, yield
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? |
Maybe we should extend the render test suite as part of the test suite here |
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:
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.