Implement OpenPBR's sheen BRDF

AcademySoftwareFoundation / OpenShadingLanguage

Advanced shading language for production GI renderers

BSD 3-Clause "New" or "Revised" License

2.09k stars 356 forks source link

Implement OpenPBR's sheen BRDF #1819

Closed fpsunflower closed 3 months ago

fpsunflower commented 4 months ago

Description

This PR adds an implementation of the Zeltner-Burley sheen closure which is the new default in OpenPBR. I've exposed it via a new (optional) mode keyword arg in the existing closure, though it would be easy to switch the interface if needed.

The implementation is based on a fit done by Stephen Hill here: https://blog.selfshadow.com/sheen/ltc_sheen.html

Tests

I've modified the existing sheen render unit tests to use the new closure on half of the sphere so we can keep testing old and new modes.

Checklist:

[x] I have read the contribution guidelines.
[ ] I have updated the documentation, if applicable.
[x] I have ensured that the change is tested somewhere in the testsuite (adding new test cases if necessary).
[x] My code follows the prevailing code style of this project. If I haven't already run clang-format v17 before submitting, I definitely will look at the CI test that runs clang-format and fix anything that it highlights as being nonconforming.

fpsunflower commented 3 months ago

I've made the suggested changes. I also fixed the TangentFrame class so it could be used here and replace the ad-hoc one (which wasn't robust as you pointed out).

This did invalidate all the reference images with small noise differences, but I think its worth the cleanup.

fpsunflower commented 3 months ago

@jstone-lucasfilm Do you have any preference for the keyword argument used to pick the new sheen variant?

As I've implemented it so far, using the new sheen would look like this:

Ci = sheen_bsdf (N, SheenColor, Roughness "mode", 1);

jstone-lucasfilm commented 3 months ago

@fpsunflower This is a great question, and my initial thinking is that the MaterialX node would have a mode input with two enumerated options: conty_kulla and zeltner_burley. This would allow for additional techniques to be added in the future, and the two enumerated options would simply become 0 and 1 in hardware shading languages such as GLSL and MSL.

Since we have a great set of subject-matter experts on the thread, I'd be very interested in what your thoughts are, and I'm happy to defer to the group.

jstone-lucasfilm commented 3 months ago

@fpsunflower @shill-lucasfilm I've had a chance to write a pull request for the mode input of sheen_bsdf in MaterialX, and I wanted to post a link here to compare notes between these two implementations:

https://github.com/AcademySoftwareFoundation/MaterialX/pull/1880

Following additional discussion with Stephen, our current thinking is that the two modes should be named conty_kulla and zeltner. Since the Zeltner paper has three authors (Zeltner, Burley, and Chiang), it seemed incorrect to mention just two of them in the sheen option, so we're instead leaning towards zeltner as a parallel construction to "Zeltner, et al".

Let me know how this sounds to you, and I'm very open to making adjustments based on feedback from this group.

fpsunflower commented 3 months ago

Sounds good to me.

fpsunflower commented 3 months ago

Finally got the testsuite passing. Just needed a handful of extra reference images and had to clamp the roughness to a small value to avoid generating NaNs for the R term.

I will squash and merge since I don't believe there are any outstanding comments.

The two remaining failures also happen in main and are udim related -- I am guessing that is coming from the latest OIIO.