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 missing basis orthogonalizations in GLSL backend #1177

Conversation

pablode
Copy link
Contributor

@pablode pablode commented Dec 28, 2022

I've encountered shading artifacts in normal mapped MaterialX materials, and upon closer inspection I believe that these are caused by the missing basis orthogonalizations @Tellusim brought to attention in PR #1049.

However, instead of using a second normalize(cross(..)) call, I've applied the Gram-Schmidt algorithm.

Here are some comparisons from the soon-to-be-released test suite for my glTF to USD+MaterialX converter:

Name Old New Diff
NormalTangentTest NormalTangentTest_ref NormalTangentTest_test NormalTangentTest_diff
DamagedHelmet DamagedHelmet_ref DamagedHelmet_test DamagedHelmet_diff
IridescentDishWithOlives IridescentDishWithOlives_ref IridescentDishWithOlives_test IridescentDishWithOlives_diff
ClearCoatTest ClearCoatTest_ref ClearCoatTest_test ClearCoatTest_diff

The changes can be tested using following model:
NormalTangentMirrorTest.zip

It requires a recent build of MaterialXView as it makes use of the support for explicit bitangents introduced in PR #1156.

@jstone-lucasfilm
Copy link
Member

jstone-lucasfilm commented Dec 29, 2022

Thanks @pablode, this looks like a great improvement to me.

Refining it one step further, perhaps we should wrap the newly-added step in an mx_gram_schmidt_orthogonalize helper function? Its input arguments could be the original X and N vectors, and its return value could be the modified X vector. This would also help to make the code more self-documenting for new readers.

One potential home for this new helper function would be:
https:/AcademySoftwareFoundation/MaterialX/blob/a345d134b865a6cdaf63e10cfc870df49ed841be/libraries/pbrlib/genglsl/lib/mx_microfacet.glsl

@jstone-lucasfilm
Copy link
Member

@pablode Let's plan to make that additional refinement in a future pull request, and I believe your existing change is worthwhile as it stands. I'll run some visual tests in a local build to confirm your results, and then I believe we can merge this pull request to main.

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.

Thanks for the fixes, @pablode!

@jstone-lucasfilm jstone-lucasfilm merged commit fefce08 into AcademySoftwareFoundation:main Jan 4, 2023
Michaelredaa pushed a commit to Michaelredaa/MaterialX that referenced this pull request Oct 21, 2023
…Foundation#1177)

I've encountered shading artifacts in normal mapped MaterialX materials, and upon closer inspection I believe that these are caused by the missing basis orthogonalizations @Tellusim brought to attention in PR AcademySoftwareFoundation#1049.

However, instead of using a second normalize(cross(..)) call, I've applied the Gram-Schmidt algorithm.
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