-
Notifications
You must be signed in to change notification settings - Fork 20
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
Subsurface refactor #171
Subsurface refactor #171
Conversation
index.html
Outdated
|
||
This relationship was derived (via fitting) in the isotropic $g=0$ case. To account for anisotropy, similarity theory [#d'Eon2021] shows that a medium with anisotropy $g$ will have approximately similar overall light transport to an isotropic medium (while still exhibiting noticeably different directional effects, especially in low-order scattering) provided that the scattering coefficient is remapped to $\bar{\boldsymbol{\mu}}_s = \boldsymbol{\mu}_s / (1 - g)$, while keeping the absorption coefficient the same. Applying this, the net result is that the remapped single-scattering albedo $\bar{\boldsymbol{\alpha}}$ is given by | ||
Given the required multiple-scattering albedo $\mathbf{E}_\mathrm{multi-scatter}$ according to the formula above, then in principle, the single-scattering albedo $\boldsymbol{\alpha}$ which generates the required $\mathbf{E}_\mathrm{multi-scatter}$ can be determined. We do not require any particular theoretical formula for $\boldsymbol{\alpha}(\mathbf{C})$ be used, but the closer it is to satisfying equation [subsurface_albedo_constraint] the closer it is to the ground truth. Ideally, this formula should take into account all the properties of the medium as well as the dielectric interface. A number of such approximate formulas have been derived in the literature. A well-known approximation is due to van de Hulst ([#Kulla2017], [#d'Eon2021]), which assumes an index-matched boundary (i.e. $\mathbf{E}_\mathrm{spec} = 0$, thus $\mathbf{E}_\mathrm{multi-scatter} = \mathbf{C}$), where: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
all the properties of the medium as well as the dielectric interface
I think this part could potentially be misinterpreted to include the shape of the object, however clarifying the semi-infinite homogeneous slab assumption mentioned above could resolve this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hopefully the re-wording mentioned in #171 (comment) addresses this.
index.html
Outdated
This gives one approximate $\boldsymbol{\alpha}(\mathbf{C})$ mapping, but as noted it ignores the presence of the dielectric boundary. Other approximate forms which take into account the dielectric interface are presented in various sources (e.g. [#d'Eon2021], [#Burley2018]). | ||
We also note that the relevant approximation may depend on the specific assumptions made about the subsurface interface, for example in some cases renderers may approximate the boundary BSDF via a diffuse lobe, for efficiency and improved convergence. In that case, it would be more appropriate to use an $\boldsymbol{\alpha}(\mathbf{C})$ designed for this specific interface BSDF, in order to satisfy equation [subsurface_albedo_constraint]. | ||
|
||
The phase function anisotropy $g$ of the medium is taken to be given directly by the **`subsurface_anisotropy`** parameter [^anisotropy_g]. It is assumed that the phase function has the standard Henyey-Greenstein form. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Henyey-Greenstein
Minor: Many style guides would recommend using en dashes instead of hyphens ("–" instead of "-") for relationships and connections like this one. However it seems fine to use hyphens as long as we're consistent about it.
There's more information about this here if anyone is interested:
https://en.wikipedia.org/wiki/Dash#Relationships_and_connections
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In 9311dfd I fixed up the hyphens to en dashes, for Henyey-Greenstein. We can make similar fixes elsewhere as needed.
**`subsurface_radius`** | Radius | `float` | $ [0, \infty) $ | $ [0, 1] $ | $ 1 $ | Length scale of diffusion along the surface | ||
**`subsurface_radius_scale`** | Radius scale | `vector3` | $ [0, 1]^3 $ | | $ (1.0, 0.5, 0.25) $ | RGB multiplier to **`subsurface_radius`**, giving the per-channel diffusion length | ||
**`subsurface_color`** | Color | `color3` | $ [0, 1] $ | | $ (0.8, 0.8, 0.8) $ | Subsurface color, controls the observed reflection color of $V^\infty_\mathrm{subsurface}$ | ||
**`subsurface_radius`** | Radius | `float` | $ [0, \infty) $ | $ [0, 1] $ | $ 1 $ | Length scale of MFP |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Length scale of MFP
Using the word "scale" in the description of the parameter that doesn't include "scale" in the name seems a bit confusing. Maybe we could reword it. Here are some ideas:
"Length of MFP"
"Extinction MFP"
"Scale MFP"
"Base MFP"
"Average distance light travels along a ray, specifically the extinction MFP"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is maybe from habit, as "length scale" is a common phrase in physics (meaning the rough magnitude of the distance at which some effect is relevant).
Perhaps the term "scale" is confusing though since there is a parameter subsurface_radius_scale
as well. It may make more sense to call the scalar radius the "scale", and the color just "radius" -- which is what we did in Standard Surface (except subsurface_scale
was the name of the scalar length).
I do think it makes more sense for something called a "scale" to be a length scale (which in the physics terminology, is an actual length), and the scalar radius I would say is a genuine length scale in the physics sense, i.e. it gives the rough magnitude of the MFP, which is modulated per-channel by the color to give the final MFP.
**`subsurface_radius_scale`** | Radius scale | `vector3` | $ [0, 1]^3 $ | | $ (1.0, 0.5, 0.25) $ | RGB multiplier to **`subsurface_radius`**, giving the per-channel diffusion length | ||
**`subsurface_color`** | Color | `color3` | $ [0, 1] $ | | $ (0.8, 0.8, 0.8) $ | Subsurface color, controls the observed reflection color of $V^\infty_\mathrm{subsurface}$ | ||
**`subsurface_radius`** | Radius | `float` | $ [0, \infty) $ | $ [0, 1] $ | $ 1 $ | Length scale of MFP | ||
**`subsurface_radius_scale`** | Radius scale | `vector3` | $ [0, 1]^3 $ | | $ (1.0, 0.5, 0.25) $ | RGB multiplier to **`subsurface_radius`**, giving the per-channel MFPs |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(1.0, 0.5, 0.25)
Tangential: Just making sure we're going ahead with this default value as we no longer have any color inversion to compensate for.
(Incidentally, I'm going to miss the "Rayleigh scattering" parameter of the ASM, which let you achieve this type of appearance in a physically based way with a simple slider, without requiring a color to be manually entered. I suppose implementations could provide higher-level controls or presets if they want to use that concept.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The reasoning for this value is:
Note that the default value of subsurface_radius_scale is set at (1,0.5,0.25)
to approximate the effect of Rayleigh scattering. If we roughly approximate the wavelength corresponding to each of the RGB channels as$\lambda$ /nm≈650,550,450 and assume the radii scale like the reciprocal of the extinction, then since in Rayleigh scattering the extinction scales like$\lambda^{-4}$ for light of wavelength$\lambda$ the expected relative magnitudes of the radii are$(1,(550/650)^4, (450/650)^4) \approx (1,0.5,0.25)$ , hence the chosen default. This provides a slightly more realistic default look for the subsurface than resulting from gray radii.
Does it sound plausible to you? I noticed that the saturation of the resulting color does vary quite a bit depending on how dense the subsurface medium is.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(Incidentally, I'm going to miss the "Rayleigh scattering" parameter of the ASM, which let you achieve this type of appearance in a physically based way with a simple slider, without requiring a color to be manually entered. I suppose implementations could provide higher-level controls or presets if they want to use that concept.)
Yeah, it would be good to know what the logic for this higher level node should be to replicate the ASM control. We could then provide it e.g. in MaterialX.
index.html
Outdated
!!! Note: Future work | ||
The Henyey-Greenstein phase function is not ideal for representing media with both forward and back scattering lobes (as in the Mie phase function). A richer model such as [#Jendersie2023] could be considered in a future version. | ||
|
||
This formulation produces volumetric parameters which reproduce reasonably well the independently specified colors of the transmitted and single-scattered light. Finally the medium phase function anisotropy $g \in [-1, 1]$ is given by **`transmission_scatter_anisotropy`** (the standard Henyey-Greenstein phase function is assumed [^anisotropy_g]). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tangential: On the Adobe side we still don't find the interior volume parameterization particularly intuitive, although I'm not sure if there's time to address it for this first version, and I know there are various tradeoffs. It will be interesting to hear how users feel.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, of course I'm also uncomfortable having these two different parametrizations in the same model.
That said, it does make some logical sense that the parametrization should differ for the case of very dense and very thin media. The albedo remapping at least cannot be expected to work for very thin translucent volumes, so it seems inappropriate to use that when authoring e.g. colored glass.
(This could have been handled within the framework of a unified volume though, just it was too big a step to make that happen this time round).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I agree with all of that.
@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. |
Remove a TODO we might not do.
…penPBR into subsurface_refactor
…des microfacet multi-scatter. (By stating that the E_multi-scatter term includes only paths which "macroscopically transmitted", i.e. excluding paths which temporarily entered the medium during the microfacet scattering, but ultimately escaped into the reflection hemisphere.
…e given extinction and anisotropy.
Signed-off-by: Jamie Portsmouth <jamports@mac.com>
@peterkutz @fpsunflower Do you mind signing off that these formulas are stated correctly? |
LGTM! |
The formulas look good to me. They match the formulas in @fpsunflower's 2017 physically based shading notes, posted here for future reference: They also match the implementation of these formulas in ASM. |
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. |
Makes sense. I also reviewed your previous reply about this question here: It seems clear that the color mapping should take the anisotropy and dielectric specular interface properties into account.
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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for making all of the clarifications and improvements @portsmouth . This looks ready to merge to me!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
It might be worth adding a sentence to make that explicit, along the lines of:
(Committed this, with a bit of polishing, in 02d942a). |
In eb28a25 I updated the MaterialX file with descriptions for the subsurface parameters that match a bit better to the new text. |
# Conflicts: # index.html
Signed-off-by: Jamie Portsmouth <jamports@mac.com>
Co-authored-by: Jamie Portsmouth <jamports@mac.com> Signed-off-by: Jonathan Stone <stonej212@gmail.com>
Co-authored-by: Jamie Portsmouth <jamports@mac.com> Signed-off-by: Jonathan Stone <stonej212@gmail.com>
5dc2a16
into
AcademySoftwareFoundation:main
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:
subsurface_radius * subsurface_radius_scale
). I noted that this blows up at zero radius, so requires some "regularization".subsurface_color
C is defined to parametrize the "multi-scatter" albedo, which is defined being very explicit to disambiguate between the reflection due to Fresnel and due to the medium scattering. This then effectively defines the expected ground truth appearance unambiguously, without appealing to any particular approximation.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.