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

Unlit surface shader node implemenation #858

Conversation

niklasharrysson
Copy link
Contributor

Implementation of the "unlit" surface shader, as proposed in issue #839.

This change list adds the definitions and implementations for GLSL, OSL and MDL.

Some tests added to the test suite, testing visualization of a simple procedural with/without cutout opacity and with/without colored transmission:
unlit_surface_tests

Note that MDL rendering is not part of the testsuite yet and are here renderer separately with the MDL DXR sdk sample. Hence the difference in environment and geometry. Also note that for GLSL our renderer does not support colored transmission yet.

@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Mar 7, 2022

CLA Missing ID CLA Not Signed

Signed-off-by: Niklas Harrysson <[email protected]>
@niklasharrysson
Copy link
Contributor Author

/easycla

mx::GenContext materialContext = context;
materialContext.getOptions().hwTransparency = hasTransparency;

mx::ShaderPtr shader = createShader(elem->getNamePath(), materialContext, elem);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This code path was missing the check for transparency, so saved GLSL shaders would never include transparency handling.

@niklasharrysson
Copy link
Contributor Author

niklasharrysson commented Mar 7, 2022

@jstone-lucasfilm I'm not sure why my EasyCLA is not picked up here, as it's signed and approved in the materialx-test repo.

@niklasharrysson
Copy link
Contributor Author

@jstone-lucasfilm I'm not sure why my EasyCLA is not picked up here, as it's signed and approved in the materialx-test repo.

It seems I had my old Autodesk email address set locally on my laptop that I made commits from. Will try to rebase or create a new PR to make sure all commits are made from my current user / email.

const ShaderInput* emissionColor = node.getInput("emission_color");
shadergen.emitLine(outColor + " = " + shadergen.getUpstreamResult(emission, context) + " * " + shadergen.getUpstreamResult(emissionColor, context), stage);

if (context.getOptions().hwTransparency)
Copy link
Contributor

@kwokcb kwokcb Mar 8, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @niklasharrysson, I don't think you want to emit this based on the option as this makes it inconsistent between language implementations (this and mdl). Also, it would make it so you can't change the values which affect "outTransparency" without recompiling the shader.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @kwokcb! Those are good points, but this convention has been used for a long time for our surface shading in GLSL., where transparency handling is not generated if the option is not set.

For example, see the PBR version of the surface constructor node here:
https://github.com/AcademySoftwareFoundation/MaterialX/blob/main/source/MaterialXGenGlsl/Nodes/SurfaceNodeGlsl.cpp#L201

And also when emitting the final alpha blending of the fragment result here:
https://github.com/AcademySoftwareFoundation/MaterialX/blob/main/source/MaterialXGenGlsl/GlslShaderGenerator.cpp#L643

So shader regeneration has always been required to introduce transparency to a surface shader in GLSL. While I agree it would be nice to improve on this, I think that should be done as part of a more holistic look at our transparency rendering for GLSL. Maybe it's worth adding a separate issue to track this.

Copy link
Contributor

@kwokcb kwokcb Mar 8, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @niklasharrysson ,
Yes, this makes sense to separate out.
I didn't know that we already required regeneration for other cases.
I can have added an issue ( #859) with the current logic as a "baseline" to consider.

kwokcb and others added 2 commits March 8, 2022 09:30
- Disable shadow maps in virtual framebuffer renders.
- Remove duplication of render tests between GCC and Clang.

Supporting changes:
- Add a 'shadowMap' command-line option to MaterialXView.
- Align render settings conventions between MaterialXRender and MaterialXView.

Signed-off-by: Niklas Harrysson <[email protected]>
@niklasharrysson
Copy link
Contributor Author

Closing this in favor of a new PR with proper EasyCLA setup: #860)

@niklasharrysson niklasharrysson deleted the unlit_surface branch October 17, 2022 17:15
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.

3 participants