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

Update self-shadowing function for direct lighting #12063

Merged
merged 4 commits into from
Jul 3, 2024
Merged

Conversation

jjhembd
Copy link
Contributor

@jjhembd jjhembd commented Jul 2, 2024

Description

This PR updates the geometric shadowing function used for direct lighting in physically-based rendering.

Our existing code was using a Schlick-Smith approximation. The limitations of this approximation have been explained by Eric Heitz in a 2014 paper. This PR updates the approach to follow the recommendations of the paper, using code similar to the glTF Sample Viewer.

The effect of the change is subtle, but it makes specular reflections slightly brighter on models with medium roughness.

Issue number and link

Resolves #12010.

Testing plan

Load the glTF PBR Sandcastle, and select the lighting option "Direct Lighting Only". See especially the "Metal-Roughness Spheres" model. Compared to the same Sandcastle in main, the specular reflection should be slightly brighter and broader in this branch.

Author checklist

  • I have submitted a Contributor License Agreement
  • I have added my name to CONTRIBUTORS.md
  • I have updated CHANGES.md with a short summary of my change
  • I have added or updated unit tests to ensure consistent code coverage
  • I have updated the inline documentation, and included code examples where relevant
  • I have performed a self-review of my code

Copy link

github-actions bot commented Jul 2, 2024

Thank you for the pull request, @jjhembd!

✅ We can confirm we have a CLA on file for you.

@jjhembd jjhembd requested a review from ggetz July 2, 2024 16:57
@ggetz
Copy link
Contributor

ggetz commented Jul 2, 2024

I have added or updated unit tests to ensure consistent code coverage

I understand why unit test may not be an ideal way to test this, but is there any kind of test we can put in place to avoid regressions?

@ggetz
Copy link
Contributor

ggetz commented Jul 2, 2024

But overall the code looks good, and I can detect a slight change from main as described.

@jjhembd
Copy link
Contributor Author

jjhembd commented Jul 3, 2024

is there any kind of test we can put in place to avoid regressions?

@ggetz as discussed, unit testing is probably not the right choice here. Screenshot comparisons would work better.

These lighting effects can be pretty subtle. We will need to add some end-to-end testing with very tight tolerances to catch any changes or regressions. But this would be out of the scope of this PR. I opened #12065 to discuss further.

@ggetz
Copy link
Contributor

ggetz commented Jul 3, 2024

Great, thanks @jjhembd!

@ggetz ggetz merged commit a0ef08f into main Jul 3, 2024
9 checks passed
@ggetz ggetz deleted the self-shadowing branch July 3, 2024 16:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Verify geometric shadowing function in PBR shader
2 participants