-
Notifications
You must be signed in to change notification settings - Fork 360
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
Fix the call to anisotropic_vdf closure in OSL #2016
Fix the call to anisotropic_vdf closure in OSL #2016
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks great to me, thanks @niklasharrysson!
@niklasharrysson I just ran new GLSL/OSL render tests with this code, and there's a new difference that seem worthwhile to highlight, just to make sure it's expected: Here's the GLSL/OSL comparison for LamaDielectric before this change: And here's the same GLSL/OSL comparison after this change: LamaDielectric is one of the nodes that makes use of a BSDF/VDF layering operation, so it's worthwhile to consider whether this change would be expected with the latest code. |
@jstone-lucasfilm That change in the OSL result was not expected. Since volumetric rendering is not supported yet in OSL And even if it was supported this result looks wrong.. It looks like the refraction is changed. Perhaps there is a bug in testrender where somehow the IOR of the top closure is modified when there is a vdf closure layered as base? |
I'm CC'ing @fpsunflower, in case he has thoughts on why this visual change might occur on the OSL side. |
Actually sorry, yes, this closure does set the medium IOR to 1.0 so it will impact the result. The |
It could also be argued that the way |
Yes, since |
@fpsunflower @niklasharrysson That proposed change on the OSL side sounds like a good resolution to me. For now, should we proceed with this update to OSL shader generation in MaterialX, with the expectation that |
Yes, I will submit a PR to fix the OSL side. |
Thanks @fpsunflower, and let's go ahead and merge this change from @niklasharrysson. |
80e1b56
into
AcademySoftwareFoundation:main
The MaterialX team noticed this issue here: AcademySoftwareFoundation/MaterialX#2016 Basically, if you add a medium through the anisotropic_vdf closure, it should not force the ior back to 1.0 as this IOR might have been changed by one of the microfacet closures. The IOR already defaults to 1.0 so cases that do not involve a microfacet closure are not affected. Signed-off-by: Chris Kulla <ckulla@gmail.com>
…ation#1870) The MaterialX team noticed this issue here: AcademySoftwareFoundation/MaterialX#2016 Basically, if you add a medium through the anisotropic_vdf closure, it should not force the ior back to 1.0 as this IOR might have been changed by one of the microfacet closures. The IOR already defaults to 1.0 so cases that do not involve a microfacet closure are not affected. Signed-off-by: Chris Kulla <ckulla@gmail.com>
…ation#1870) The MaterialX team noticed this issue here: AcademySoftwareFoundation/MaterialX#2016 Basically, if you add a medium through the anisotropic_vdf closure, it should not force the ior back to 1.0 as this IOR might have been changed by one of the microfacet closures. The IOR already defaults to 1.0 so cases that do not involve a microfacet closure are not affected. Signed-off-by: Chris Kulla <ckulla@gmail.com>
Update the implementation of
anisotropic_vdf
node to call the corresponding closure in OSL.