Implement OpenPBR's sheen BRDF #1819

fpsunflower · 2024-06-01T00:31:51Z

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:

I have read the contribution guidelines.
I have updated the documentation, if applicable.
I have ensured that the change is tested somewhere in the testsuite (adding new test cases if necessary).
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.

aconty · 2024-06-03T15:39:07Z

src/testrender/shading.cpp

+        const float len2     = dot(wiOriginal, wiOriginal);
+        const float det      = aInv * aInv;
+        const float jacobian = det / (len2 * len2);
+        pdf = jacobian * std::max(wiOriginal.z, 0.0f) * float(M_1_PI);


Is there a reason not to call eval(wo, wi) here?

It just seemed redundant to transform/untransform the vector and recompute the ltc factors several times. Since its just a few lines of code, it seemed easier to just do the small amount of work again.

aconty

Looks very good!

selfshadow

I think it would be better to reference the MaterialX implementation for now*, since the WebGL sandbox was just a test and may go away or move.

I'm planning to publish something about the fitting methodology later once I've settled on a final implementation, at which point this can be the canonical reference.

selfshadow · 2024-06-03T19:19:27Z

src/testrender/shading.cpp

+        float pdf;
+        Sampling::sample_cosine_hemisphere(Vec3(0, 0, 1), rx, ry, wi, pdf);
+
+        wi = Vec3(wi.x - wi.z * bInv, wi.y, wi.z * aInv).normalize();


Nice idea of scaling up by aInv here. I'll see if I can borrow that for the MaterialX implementation.

selfshadow · 2024-06-03T19:24:57Z

src/testrender/shading.cpp

+        // TODO: is there a cheaper way to get the jacobian here?
+        const Vec3 wiOriginal(aInv * wi.x + bInv * wi.z, aInv * wi.y, wi.z);
+        const float len2     = dot(wiOriginal, wiOriginal);
+        const float det      = aInv * aInv;


Here's what I'm doing, using the length of L / wi instead (before normalisation): https://github.com/AcademySoftwareFoundation/MaterialX/blob/main/libraries/pbrlib/genglsl/lib/mx_microfacet_sheen.glsl#L169-L183

Nice - I'll go through this and try to consolidate the logic. Didn't realize yours was added to the MaterialX side already.

I think if you retain the aInv multiplication of wi but otherwise follow what I'm doing then you'll end up with square(len2 / aInv) for the jacobian, which should still be a little bit cheaper than what you have here.

NB: I may end up changing to fitting to work with the non-inverse matrix coefficients (TBD, but it's possible that they could be more linear), but that shouldn't change the implementation all that much.

selfshadow · 2024-06-04T16:59:37Z

src/testrender/shading.cpp

+        const float NdotV = clamp(N.dot(V), 0.0f, 1.0f);
+        const Vec3 ltc = fetch_ltc(NdotV);
+
+        const Vec3 T1 = (V - N * NdotV).normalize();


There's a danger of generating NaNs here if V = N (I've certainly hit this in other LTC-related code in production). I didn't bother to handle that in the WebGL sandbox, but I'm doing so in the MaterialX version:
https://github.com/AcademySoftwareFoundation/MaterialX/blob/main/libraries/pbrlib/genglsl/lib/mx_microfacet_sheen.glsl#L117-L133

Things get a little simpler if vectors are already in local space:
https://github.com/shill-lucasfilm/OpenPBR-viewer/blob/main/glsl/fuzz_brdf.glsl#L30-L37

fpsunflower · 2024-06-06T05:47:59Z

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 · 2024-06-06T05:52:09Z

@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 · 2024-06-06T15:12:35Z

@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.

selfshadow · 2024-06-06T17:39:44Z

testsuite/render-mx-sheen/sheen.osl

@@ -16,7 +16,7 @@ sheen
  )
 {
    if (N.x < 0)
-        Ci = sheen_bsdf (N, Cs, roughness * 2, "mode", 1);
+        Ci = sheen_bsdf (N, Cs, sqrt(roughness), "mode", 1);


Are you sure this is correct? I thought the external parameterisation was roughness (alpha in the Zeltner implementation) and that only the underlying SGGX model uses sigma. The table itself (and my analytic refit) should be parameterised by alpha, not sigma.

When looking at the wedges of both sheen models, the sqrt seemed to bring them closer together. This part is just for the testsuite where its handy to see both models at once on the same sphere.

The materialx closure just exposes the "raw" parameter in any case (just like GGX).

Okay, makes sense.

selfshadow · 2024-06-06T17:44:13Z

src/testrender/shading.cpp

+        const float y = roughness;
+        const float A = ((2.58126f * x + 0.813703f * y) * y) / (1.0f + 0.310327f * x * x + 2.60994f * x * y);
+        const float B = sqrtf(1.0f - x) * (y - 1.0f) * y * y * y / (0.0000254053f + 1.71228f * x - 1.71506f * x * y + 1.34174f * y * y);
+        const float invs = (0.0379424f + y * (1.32227f + y)) / (y * (0.0206607f + 1.58491f * y));


Good idea! I had originally tried to fit 1/s directly but it made the optimisation unstable. I don't know why I didn't follow through on doing it as a post step but I'll bring this over to a future MaterialX GLSL version, assuming I don't replace the Gaussian fitting (I might try a logistic).

selfshadow · 2024-06-06T17:59:35Z

src/testrender/sampling.h

-        u = v.cross(w);
+    // fallsback to an arbitrary basis if the tangent is 0 or colinear with n
+    static TangentFrame from_normal_and_tangent(const Vec3& n, const Vec3& t) {
+        Vec3 y = n.cross(t);


Out of curiosity, was there a reason you went with computing y here rather than x (via t - n*dot(n, t))? Numerical stability or performance?

No real reason actually. Now that I look at both in the light of day, they should be equivalent, and the dot product way might be a few less operations (especially considering the dot product is needed for the LTC lookup anyway).

Okay, fair enough. I just wanted to check that I wasn't missing anything.

jstone-lucasfilm · 2024-06-10T19:00:59Z

@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:

AcademySoftwareFoundation/MaterialX#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 · 2024-06-11T04:09:27Z

Sounds good to me.

…osure Signed-off-by: Chris Kulla <[email protected]>

Signed-off-by: Chris Kulla <[email protected]>

…roughness Signed-off-by: Chris Kulla <[email protected]>

Signed-off-by: Chris Kulla <[email protected]>

…DF implementations Signed-off-by: Chris Kulla <[email protected]>

Signed-off-by: Chris Kulla <[email protected]>

…ngent vector Signed-off-by: Chris Kulla <[email protected]>

Signed-off-by: Chris Kulla <[email protected]>

fpsunflower · 2024-06-21T16:17:31Z

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.

selfshadow · 2024-06-21T16:18:07Z

src/testrender/shading.cpp

@@ -1529,7 +1529,7 @@ struct ZeltnerBurleySheen final : public BSDF, MxSheenParams {
        // for the implementation in MaterialX:
        // https://github.com/AcademySoftwareFoundation/MaterialX/blob/main/libraries/pbrlib/genglsl/lib/mx_microfacet_sheen.glsl
        const float x = NdotV;
-        const float y = roughness;
+        const float y = std::max(roughness, 1e-3f);


Ahh yes, I should've caught this, since I hit the same problem in the MaterialX GLSL implementation. I decided to clamp to the minimum used by Zeltner et. al for their table:
https://github.com/AcademySoftwareFoundation/MaterialX/blob/main/libraries/pbrlib/genglsl/mx_sheen_bsdf.glsl#L30

Note: I may come up with a new fit against a higher-resolution table in the future, at which point the minimum roughness may be lower.

Oops, I left this comment as pending earlier. Oh well!

* fix: reparameter string needed to be ustringhash (AcademySoftwareFoundation#1841) * fix: make backfacing shadeop indicate backfacing shader-global is needed (AcademySoftwareFoundation#1827) * Fix the subsurface_bssrdf parameters (AcademySoftwareFoundation#1823) * Implement OpenPBR's sheen BRDF in testrender (AcademySoftwareFoundation#1819) * cleanup: OIIO deprecations (AcademySoftwareFoundation#1834) See merge request spi/dev/3rd-party/osl-feedstock!61

fpsunflower changed the title ~~New sheen closure~~ Implement OpenPBR's sheen BRDF Jun 1, 2024

aconty reviewed Jun 3, 2024

View reviewed changes

aconty approved these changes Jun 3, 2024

View reviewed changes

selfshadow reviewed Jun 3, 2024

View reviewed changes

selfshadow reviewed Jun 4, 2024

View reviewed changes

selfshadow reviewed Jun 6, 2024

View reviewed changes

fpsunflower added 12 commits June 11, 2024 01:19

Add Zeltner-Burley sheen BSDF as a new mode of the MaterialX sheen cl…

fa3a46d

…osure Signed-off-by: Chris Kulla <[email protected]>

Run clang-format

95e1a0f

Signed-off-by: Chris Kulla <[email protected]>

Simplify sheen math a bit more and update shaders to use sqrt of the …

62d0c27

…roughness Signed-off-by: Chris Kulla <[email protected]>

Skip two divisions in the fit by evaluating 1/s instead of s

a18e16f

Signed-off-by: Chris Kulla <[email protected]>

Run clang-format

2626a07

Signed-off-by: Chris Kulla <[email protected]>

Fix tangent frame calculation from tangents and use it to simplify BS…

01233a4

…DF implementations Signed-off-by: Chris Kulla <[email protected]>

Refresh testsuite images to account for updated tangent frame math

1c421c3

Signed-off-by: Chris Kulla <[email protected]>

Run clang-format

0a74e00

Signed-off-by: Chris Kulla <[email protected]>

Simplify math for computing tangent frame aligned with a specified ta…

b80c946

…ngent vector Signed-off-by: Chris Kulla <[email protected]>

Update reference images

e8a17c8

Signed-off-by: Chris Kulla <[email protected]>

Run clang-format

31654dd

Signed-off-by: Chris Kulla <[email protected]>

Add alternative reference images on linux

58ec48b

Signed-off-by: Chris Kulla <[email protected]>

fpsunflower force-pushed the new-sheen-closure branch from e42de16 to 58ec48b Compare June 21, 2024 01:15

fpsunflower added 2 commits June 21, 2024 01:33

Add alternate image for testsuite

8a4baa6

Signed-off-by: Chris Kulla <[email protected]>

Clamp roughness in sheen closure to avoid NaNs

f72ac64

Signed-off-by: Chris Kulla <[email protected]>

fpsunflower merged commit 65024b0 into AcademySoftwareFoundation:main Jun 21, 2024
21 of 23 checks passed

fpsunflower deleted the new-sheen-closure branch June 21, 2024 16:18

selfshadow reviewed Jun 21, 2024

View reviewed changes

shill-lucasfilm mentioned this pull request Sep 14, 2024

Implement new oren nayar model from OpenPBR #1817

Merged

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement OpenPBR's sheen BRDF #1819

Implement OpenPBR's sheen BRDF #1819

fpsunflower commented Jun 1, 2024

aconty Jun 3, 2024

fpsunflower Jun 3, 2024

aconty left a comment

selfshadow left a comment

selfshadow Jun 3, 2024

selfshadow Jun 3, 2024

fpsunflower Jun 3, 2024

selfshadow Jun 4, 2024

selfshadow Jun 4, 2024

fpsunflower commented Jun 6, 2024

fpsunflower commented Jun 6, 2024

jstone-lucasfilm commented Jun 6, 2024

selfshadow Jun 6, 2024

fpsunflower Jun 6, 2024

selfshadow Jun 6, 2024

selfshadow Jun 6, 2024

selfshadow Jun 6, 2024

fpsunflower Jun 6, 2024

selfshadow Jun 6, 2024

jstone-lucasfilm commented Jun 10, 2024

fpsunflower commented Jun 11, 2024

fpsunflower commented Jun 21, 2024

selfshadow Jun 21, 2024

selfshadow Jun 21, 2024

Implement OpenPBR's sheen BRDF #1819

Implement OpenPBR's sheen BRDF #1819

Conversation

fpsunflower commented Jun 1, 2024

Description

Tests

Checklist:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aconty left a comment

Choose a reason for hiding this comment

selfshadow left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fpsunflower commented Jun 6, 2024

fpsunflower commented Jun 6, 2024

jstone-lucasfilm commented Jun 6, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jstone-lucasfilm commented Jun 10, 2024

fpsunflower commented Jun 11, 2024

fpsunflower commented Jun 21, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment