AcademySoftwareFoundation / OpenPBR

Specification and reference implementation for the OpenPBR Surface shading model
Apache License 2.0
407 stars 18 forks source link

Subsurface refactor #171

Closed portsmouth closed 2 months ago

portsmouth commented 4 months ago

This PR refactors the subsurface discussion to be consistent with our (extensive) discussion in Slack about improving the meaning of the parametrization. None of the parameters have changed (names, types or ranges), but the interpretation of them is re-stated to avoid the issues we were seeing in implementations following the old text.

In summary:

With this refactor, the existing implementations we have in Arnold (for example) are reasonable approximate interpretations. Those implementations are production-proven, so are certainly safe to ship.

image

peterkutz commented 3 months ago

@portsmouth This looks great overall. It accurately conveys the conclusions of our last round of discussions. It also makes a number of useful clarifications.

I left a number of comments about specific concerns and suggestions for further improvements.

portsmouth commented 3 months ago

@peterkutz @fpsunflower Do you mind signing off that these formulas are stated correctly?

image

fpsunflower commented 3 months ago

LGTM!

peterkutz commented 2 months ago

Do you mind signing off that these formulas are stated correctly?

The formulas look good to me.

They match the formulas in @fpsunflower's 2017 physically based shading notes, posted here for future reference:

ScreenShotOf2017ImageworksPhysicallyBasedShadingSlides

They also match the implementation of these formulas in ASM.

peterkutz commented 2 months ago

Just to double-check, we want the anisotropy to be incorporated into the color parameterization, since that's a multiple-scattering perceptual parameterization, but we don't want the anisotropy to be incorporated into the scale parameterization, since that's a single-scattering non-perceptual parameterization. Is that right?

portsmouth commented 2 months ago

Just to double-check, we want the anisotropy to be incorporated into the color parameterization, since that's a multiple-scattering perceptual parameterization, but we don't want the anisotropy to be incorporated into the scale parameterization, since that's a single-scattering non-perceptual parameterization. Is that right?

I think it's not clear what "anisotropy ... incorporated into the scale parameterization" would do exactly, in terms of the desired effect on the appearance. Currently the anisotropy control will just do what it physically does (bias the scattering forwards or backwards), while we also automatically adjust the single-scatter albedo (taking into account the anisotropy) to achieve the desired multi-scatter color. Of course, as we discussed before this means the anisotropy mostly seems to have the effect of making the medium more or less transmissive (which we previously tried to compensate for), though it seems like a somewhat familiar effect perceptually so good to leave this behavior in.

peterkutz commented 2 months ago

Makes sense. I also reviewed your previous reply about this question here: https://github.com/AcademySoftwareFoundation/OpenPBR/pull/171/files#r1571187628

It seems clear that the color mapping should take the anisotropy and dielectric specular interface properties into account.

Of course, as we discussed before this means the anisotropy mostly seems to have the effect of making the medium more or less transmissive (which we previously tried to compensate for)

Right. While I don't see it explicitly mentioned, I suppose it's clear enough in the text that the anisotropy will not effect the radius->extinction mapping, which implies that the anisotropy will make the medium appear more or less translucent.

portsmouth commented 2 months ago

Makes sense. I also reviewed your previous reply about this question here: https://github.com/AcademySoftwareFoundation/OpenPBR/pull/171/files#r1571187628

It seems clear that the color mapping should take the anisotropy and dielectric specular interface properties into account.

Of course, as we discussed before this means the anisotropy mostly seems to have the effect of making the medium more or less transmissive (which we previously tried to compensate for)

Right. While I don't see it explicitly mentioned, I suppose it's clear enough in the text that the anisotropy will not effect the radius->extinction mapping, which implies that the anisotropy will make the medium appear more or less translucent.

It might be worth adding a sentence to make that explicit, along the lines of:

Note that the extinction $\boldsymbol{\mu}_t$ is determined only by the scaled subsurface radii $\mathbf{r}$, independent of the anisotropy $g$, thus the anisotropy has the familiar physical effect of allowing light to transmit more deeply into the medium when positive than when negative.

(Committed this, with a bit of polishing, in 02d942a59260b7e1a2d4f9d68b28fc71c4c65dbc).

portsmouth commented 2 months ago

These changes look good to me! Are there any corresponding updates required for our reference implementation, or are we ready to merge this work to main?

In https://github.com/AcademySoftwareFoundation/OpenPBR/pull/171/commits/eb28a25eadd0362531192bd285e2ef1141c03228 I updated the MaterialX file with descriptions for the subsurface parameters that match a bit better to the new text.