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

Fix bug in BSDF code gen for HW languages #1399

Conversation

niklasharrysson
Copy link
Contributor

This change list fixes a bug in code gen for HW languages, where under certain conditions the function call for a BSDF node could be generated multiple times in the same scope.

For example, if a pure reflection BSDF node, like oren_nayar_bsdf, where referenced multiple times in a graph. Then during generation of the transmission section of a GLSL surface shader the node's output variable would be emitted multiple times, giving erroneous code.

A workaround was previously to duplicate the node in the graph, as for example done in the GlftPbr graph. With this fix duplication is no longer needed.

Copy link
Member

@jstone-lucasfilm jstone-lucasfilm left a 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 improvement, thanks @niklasharrysson, and I had just one question about a remaining comment in the graph definition.

@@ -148,11 +148,6 @@
<!-- Thin-film + Dielectric
Note: Due to limitations in codegen, the base layer BSDF is duplicated (#1035). -->
Copy link
Member

Choose a reason for hiding this comment

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

Can this comment now be removed as well?

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 Jonathan! I think this comment is meant for the dielectric base layer under thin-film layering. The specular reflection and transmission BSDF's below are also duplicated, in order to have the dielectric layer both with and without thin-film, to blend between. That duplication is needed for another code gen limitation. You can't have the same BSDF node both with and without thin-film (even if it's possible to create such a graph).

However, I have another PR coming up after this one, with the change to thin-film handling that we discussed, where thin-film is set by parameters instead of an extra layer operator. That change will modify this graph as well, and I will remove the comment since it's then no longer valid.

Copy link
Member

Choose a reason for hiding this comment

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

Thanks for the clarification, @niklasharrysson!

Copy link
Member

@jstone-lucasfilm jstone-lucasfilm left a comment

Choose a reason for hiding this comment

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

Looks good to me!

@jstone-lucasfilm jstone-lucasfilm changed the title Fix bug with code gen of BSDF nodes for HW languages. Fix bug in BSDF code gen for HW languages Jul 10, 2023
@jstone-lucasfilm jstone-lucasfilm merged commit 2c50d91 into AcademySoftwareFoundation:main Jul 10, 2023
13 checks passed
Michaelredaa pushed a commit to Michaelredaa/MaterialX that referenced this pull request Oct 21, 2023
…1399)

This change list fixes a bug in code gen for HW languages, where under certain conditions the function call for a BSDF node could be generated multiple times in the same scope.

For example, if a pure reflection BSDF node, like oren_nayar_bsdf, where referenced multiple times in a graph. Then during generation of the transmission section of a GLSL surface shader the node's output variable would be emitted multiple times, giving erroneous code.

A workaround was previously to duplicate the node in the graph, as for example done in the GltfPbr graph. With this fix duplication is no longer needed.
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.

2 participants