-
Notifications
You must be signed in to change notification settings - Fork 359
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
Shader generation support for new OSL closures in MaterialXGenOsl #1039
Shader generation support for new OSL closures in MaterialXGenOsl #1039
Conversation
if (MATERIALX_OSL_LEGACY_CLOSURES) | ||
install(FILES "${CMAKE_CURRENT_SOURCE_DIR}/pbrlib/genosl/pbrlib_genosl_impl.legacy" | ||
DESTINATION "${MATERIALX_INSTALL_STDLIB_PATH}/pbrlib/genosl/" RENAME pbrlib_genosl_impl.mtlx) | ||
endif() |
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.
Some of the code snippets are different when using the legacy OSL closures. This will install a separate mtlx file with implementation elements pointing to the right code.
@@ -0,0 +1,5 @@ | |||
void mx_anisotropic_vdf(vector absorption, vector scattering, float anisotropy, output VDF vdf) |
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.
Old code snippets are move into a "legacy" directory.
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.
Great to see this!
@@ -1,6 +1,5 @@ | |||
void mx_subsurface_bsdf(float weight, color _color, vector radius, float anisotropy, normal N, output BSDF bsdf) | |||
{ | |||
// TODO: Subsurface closure is not supported by vanilla OSL. | |||
bsdf.response = _color * weight * diffuse(N); | |||
bsdf.throughput = color(0.0); | |||
bsdf = _color * weight * diffuse(N); |
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.
testrender
will recognize the subsurface closure (even if it happens to use diffuse in the implementation right now). So it would be safe to use it.
} | ||
else | ||
{ | ||
bsdf.response = tint * weight * comp * microfacet(distribution, N, U, safeAlpha.x, safeAlpha.y, ior, 2); | ||
bsdf = weight * dielectric_bsdf(N, U, tint, tint, roughness.x, roughness.y, ior, distribution); |
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.
For refraction to occur this has to be paired with a medium closure. Is it expected that the user will have a medium node in the graph if they want refraction?
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.
If refraction with absorption/scattering is requested then it is expected that the user sets a medium_vdf
below the dielectric_bsdf
, using the layer
operator, in the MaterialX graph.
But it would be good if the dielectric_bsdf
could support basic refraction without having the medium_vdf
present. In that case it would use the same ior for both the reflection and transmission parts. This would ensure backwards compatibility as this is the current behavior before we add the medium_vdf
to MaterialX, and it's the behavior used in MDL for example.
I'm imagining the following behavior could be used for the two dielectric BSDF's.
dielectric_bsdf
If transmission is enabled but no medium is present use the one and only ior for both reflection and transmission.generalized_schlick_bsdf
If transmission is enabled but no medium is present use ior=1.0 for transmission and hence get straight transmission without any refraction.
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 is a good point. Do you think generalized_schlick_bsdf
should always use 1.0? or try to guess the IOR from f0 ?
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.
To my mind it would be better to just have ior=1.0 and get no refraction for generalized_schlick_bsdf
if no medium is present. But this is mainly because I find it so unintuitive that f0 will control how light is refracted. It seems very hard to control for an artist and especially since refraction has such a drastic effect on the look. But this is my personal preference and I'm open to other suggestions.
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.
Perhaps we should start with the current MaterialX behavior, where IOR is derived from F0 in generalized_schlick_bsdf
, but discuss this further in the ASWF Slack channel for this topic. Ideally, I'd like to bring Naty Hoffman and Andre Mazzone from Lucasfilm/ILM into that conversation, since they've given that question additional thought in a production context.
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.
Sounds good. Thanks for the input @jstone-lucasfilm.
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 like a great foundation to build upon, thanks @niklasharrysson, and let's go ahead with the merge to main.
…ation#1039) This change list adds a build option to select between supporting either the old legacy OSL closures or the new OSL closures in development. By default the old closures are selected for backwards compatibility. To select the new closures one needs to disable the build option MATERIALX_OSL_LEGACY_CLOSURES and use a build of OSL that has the new closures supported in testrender. Also make sure to remove the ifdef MATERIALX_CLOSURE guards in stdosl.h to make the new closures visible when compiling the generated shaders.
This change list adds a build option to select between supporting either the old legacy OSL closures or the new OSL closures in development. By default the old closures are selected for backwards compatibility.
To select the new closures one needs to disable the build option MATERIALX_OSL_LEGACY_CLOSURES and use a build of OSL that has the new closures supported in testrender. Also make sure to remove the ifdef MATERIALX_CLOSURE guards in stdosl.h to make the new closures visible when compiling the generated shaders.