Interesting!
Do you have renders done in Cycles already?

Did a quick pass of review. Generally seems fine, so far only cosmetic comments. BTW, do you have clang-format setup in your development environment?

The only thing that I do not feel like addressing is the #define SIMILARITY_LEVEL 9 into a const

Fine by me.

From reading the code the bigger missing part seems to be that it is not formatted b clang-format. We can do that when we'll be applying the patch. But it'll be great if you can look into setting it for your Cycles development as well. We have some quick notes at https://wiki.blender.org/wiki/Tools/ClangFormat and https://wiki.blender.org/wiki/Style_Guide/C_Cpp

Anyway, was playing around with the applied patch. Test test file with packed textures:

• The compatibility seems to be lost: as if the radius is way smaller with this path. While the difference is inevitable, we are always trying to do-version existing files to avoid to make them render as close as possible. Not sure yet whether we can keep compatibility here, but some tweaks should be possible. Or, it needs to be explicitly stated somewhere that things are expected to break existing setups.

• Not sure we really support Burley in the Cycles-X, but this patch seems to introduce artifacts related on non-initialized value somewhere. Easiest to see on my setup is to use CPU render.
  

• Is it possible to have anisotropy support for Subsurface Scattering node (currently it is only available via Principled node) ?

• There is something I don't understand in the anisotropy behavior. The model turns black for anistropy values range 0.8 - 0.96. Don't think this is expected?

In D11965#308980, @Sergey Sharybin (sergey) wrote:

• Not sure we really support Burley in the Cycles-X, but this patch seems to introduce artifacts related on non-initialized value somewhere. Easiest to see on my setup is to use CPU render.
  

I re-rendered your scene with latest cycles-x and this patch applied and I do not see the artifacts you mention (in both OptiX, Cuda or CPU). Weird. See attached. This is F12-render.

In D11965#308980, @Sergey Sharybin (sergey) wrote:

• There is something I don't understand in the anisotropy behavior. The model turns black for anistropy values range 0.8 - 0.96. Don't think this is expected?

I can confirm that here, but only with this scene. Not sure what is triggering that behavior. I will look into that.

In D11965#309163, @Christophe Hery (chery) wrote:

In D11965#308980, @Sergey Sharybin (sergey) wrote:

• There is something I don't understand in the anisotropy behavior. The model turns black for anistropy values range 0.8 - 0.96. Don't think this is expected?

I will look into that.

I think I have figured this one out. Your albedo is pretty high (at 0.8), and I guess the albedo inversion can produce slight numerical overflowing (result slightly above 1.0f) in this case.
The solution is simply to add a *alpha = clamp(*alpha, 0.0f, 0.99f) at line 271 in the new subsurface_random_walk_remap from integrator_subsurface.h.
Could you confirm that this solves it for you? Thanks.

I re-rendered your scene with latest cycles-x and this patch applied and I do not see the artifacts you mention (in both OptiX, Cuda or CPU).

You need to switch the node to Christensen-Burley mode. It currently falls-back to the Random-Walk in Cycles-X.
Seems that initializing IOR and anisotropy for BSSRDF in the svm_closure.h solves the artifacts.

The solution is simply to add a *alpha = clamp(*alpha, 0.0f, 0.99f) at line 271 in the new subsurface_random_walk_remap from integrator_subsurface.h.
Could you confirm that this solves it for you?

Testing the new state of the patch and can not reproduce the unexpected darkening anymore.

Lastly, I'd rather keep the "Subsurface Ior" parameter (as opposed to "Subsurface IOR"), otherwise we have to upgrade all our scenes.

If there are a lot of scenes it could be possible to deal with them as a do-version.

From the remaining parts I think we have:

• Make the Subsurface BSDF node to support anisotropy.
• Find a solution to preserve compatibility
• Run benchmark, make sure we don't loose a lot of performance

In D11965#309863, @Sergey Sharybin (sergey) wrote:

You need to switch the node to Christensen-Burley mode. It currently falls-back to the Random-Walk in Cycles-X.
Seems that initializing IOR and anisotropy for BSSRDF in the svm_closure.h solves the artifacts.

This is really weird, right? As subsurface ior and anisotropy are new parameters and of course not used in Christensen-Burley.
In any case, glad it is solved.

Testing the new state of the patch and can not reproduce the unexpected darkening anymore.

Great. Thanks for confirming.

Lastly, I'd rather keep the "Subsurface Ior" parameter (as opposed to "Subsurface IOR"), otherwise we have to upgrade all our scenes.

If there are a lot of scenes it could be possible to deal with them as a do-version.

Not sure what a do-version is. But, we had talked about it with a few users internally, and we had opted originally to "Subsurface Ior" rather than "Subsurface IOR", bringing less attention to that parameter (since no capitalization) compared to others in the UI.

From the remaining parts I think we have:

• Make the Subsurface BSDF node to support anisotropy.

I can do that.

• Find a solution to preserve compatibility

Not sure about this. The albedo inversion is much truer to the input color. This is a big advantage, since now an input in base color or in subsurface color will produce very much the same color (minus the softness and backlighting of translucency with sss).

• Run benchmark, make sure we don't loose a lot of performance

Because of the Similarity Theory thing I incorporated, you should not see major slow downs from it. But let me know.
Internally, we have used this method for 2 years and have been happy with its performance.

In D11965#309925, @Christophe Hery (chery) wrote:

In D11965#309863, @Sergey Sharybin (sergey) wrote:

• Make the Subsurface BSDF node to support anisotropy.

I can do that.

After looking at it, I wonder whether this is the right choice. That node is very simple. The albedo inversion and the changes in random walk will already work with it.
Default anisotropy is 0 and default ior 1.4. These seems reasonable in that context and, in my opinion, may not need to be exposed as parameters in that particular shader.
But let me know what you think.

Not sure what a do-version is

Is a versioning code. It allows to "patch" .blend file on loading so that it behaves same as in older Blender version. In this case it means that we can change socket name without loosing compatibility with files you have.

But, we had talked about it with a few users internally, and we had opted originally to "Subsurface Ior" rather than "Subsurface IOR", bringing less attention to that parameter (since no capitalization) compared to others in the UI.

To my knowledge all abbreviations in Blender are capitalized. You can also argue that making IOR less visible things become more confusing because of having two "Subsurface" sliders.
So far I don't really see a strong argument for deviating of Blender's consistency.

After looking at it, I wonder whether this is the right choice. That node is very simple. The albedo inversion and the changes in random walk will already work with it.
Default anisotropy is 0 and default ior 1.4. These seems reasonable in that context and, in my opinion, may not need to be exposed as parameters in that particular shader.

To me it does make sense. Is not so much for being a simple node, but more of a node dedicated to BSSRDF. Don't think we gain much by hiding parameters in it, and forcing to use Principled node when one needs to mix some SSS to their shader.

In D11965#310864, @Sergey Sharybin (sergey) wrote:

Not sure what a do-version is

Is a versioning code. It allows to "patch" .blend file on loading so that it behaves same as in older Blender version. In this case it means that we can change socket name without loosing compatibility with files you have.

Great. Could you show me an example / how to do that?

But, we had talked about it with a few users internally, and we had opted originally to "Subsurface Ior" rather than "Subsurface IOR", bringing less attention to that parameter (since no capitalization) compared to others in the UI.

To my knowledge all abbreviations in Blender are capitalized. You can also argue that making IOR less visible things become more confusing because of having two "Subsurface" sliders.
So far I don't really see a strong argument for deviating of Blender's consistency.

Ok then. Easy to do.

After looking at it, I wonder whether this is the right choice. That node is very simple. The albedo inversion and the changes in random walk will already work with it.
Default anisotropy is 0 and default ior 1.4. These seems reasonable in that context and, in my opinion, may not need to be exposed as parameters in that particular shader.

To me it does make sense. Is not so much for being a simple node, but more of a node dedicated to BSSRDF. Don't think we gain much by hiding parameters in it, and forcing to use Principled node when one needs to mix some SSS to their shader.

Alright. Will do.

Overall I agree with Sergey's comments.

For compatibility, I think we need to add a new option in the menu if the new behavior is desired. I can see no other practical way to keep a compatible result if a texture map is used for the base color.

Here's an example to show why. These 3 strips have albedo 0.8, 0.5 and 0.1.

Old	New

In the old mapping the effective scattering distance varies a lot with the albedo, in the new one not so much. From what I can tell this is (entirely?) due to this code in bssrdf_burley_setup:

const float3 A = bssrdf->albedo;
const float3 s = make_float3(
    bssrdf_burley_fitting(A.x), bssrdf_burley_fitting(A.y), bssrdf_burley_fitting(A.z));

bssrdf->radius = l / s;

Also to give some background, a reason the Subsurface Scattering node is useful to support is because we have always let users build their own uber-shaders. Since we added the Principled BSDF this is less important, but if we add MaterialX import in the future it will become more important again to have all the components available as individual nodes.

In D11965#311139, @Brecht Van Lommel (brecht) wrote:

Also to give some background, a reason the Subsurface Scattering node is useful to support is because we have always let users build their own uber-shaders. Since we added the Principled BSDF this is less important, but if we add MaterialX import in the future it will become more important again to have all the components available as individual nodes.

I am working on the Subsurface Scattering node right now. No worries.

In the old mapping the effective scattering distance varies a lot with the albedo, in the new one not so much. From what I can tell this is (entirely?) due to this code in bssrdf_burley_setup:

Yes, the albedo inversion is really what I am changing deep down. This bssrdf_burley_setup()/old subsurface_random_walk_remap() approach does not obviously account for anisotropy. You can try it, ie keep it called in subsurface_random_walk_coefficients, something like:

/* Support for anisotropy from:
 * "Path Traced Subsurface Scattering using Anisotropic Phase Functions
 * and Non-Exponential Free Flights".
 * Magnus Wrenninge, Ryusuke Villemin, Christophe Hery.
 * https://graphics.pixar.com/library/PathTracedSubsurface/ */

ccl_device void subsurface_monaco_walk_remap(
        const float albedo,
        const float d,
        const float eta,
        const float inv_eta,
        const float g,
        float *sigma_t,
        float *alpha)
{
  /* Compute attenuation and scattering coefficients from albedo. */
  /* the following #if should stay at 1: this is only meant to test/compare to the old albedo inversion (without anisotropy) */
#if 1
  const float g2 = g * g;
  const float g3 = g2 * g;
  const float g4 = g3 * g;
  const float g5 = g4 * g;
  const float g6 = g5 * g;
  const float g7 = g6 * g;

  const float A = 1.8260523782f + -1.28451056436f * g + -1.79904629312f * g2 +
                  9.19393289202f * g3 + -22.8215585862f * g4 + 32.0234874259f * g5 +
                  -23.6264803333f * g6 + 7.21067002658f * g7;
  const float B = 4.98511194385f +
                  0.127355959438f *
                      expf(31.1491581433f * g + -201.847017512f * g2 + 841.576016723f * g3 +
                           -2018.09288505f * g4 + 2731.71560286f * g5 + -1935.41424244f * g6 +
                           559.009054474f * g7);
  const float C = 1.09686102424f + -0.394704063468f * g + 1.05258115941f * g2 +
                  -8.83963712726f * g3 + 28.8643230661f * g4 + -46.8802913581f * g5 +
                  38.5402837518f * g6 + -12.7181042538f * g7;
  const float D = 0.496310210422f + 0.360146581622f * g + -2.15139309747f * g2 +
                  17.8896899217f * g3 + -55.2984010333f * g4 + 82.065982243f * g5 +
                  -58.5106008578f * g6 + 15.8478295021f * g7;
  const float E = 4.23190299701f +
                  0.00310603949088f *
                      expf(76.7316253952f * g + -594.356773233f * g2 + 2448.8834203f * g3 +
                           -5576.68528998f * g4 + 7116.60171912f * g5 + -4763.54467887f * g6 +
                           1303.5318055f * g7);
  const float F = 2.40602999408f + -2.51814844609f * g + 9.18494908356f * g2 +
                  -79.2191708682f * g3 + 259.082868209f * g4 + -403.613804597f * g5 +

  const float blend = powf(albedo, 0.25f);

  *alpha = (1.0f - blend) * A * powf(atanf(B * albedo), C) +
                   blend  * D * powf(atanf(E * albedo), F);
  *alpha = clamp(*alpha, 0.0f, 0.99f);  // because of numerical precision

  const float F_dr = inv_eta * (-1.440f * inv_eta + 0.710f) + 0.668f + 0.0636f * eta;
  const float fourthirdA = (4.0f / 3.0f)
                         * (1.0f + F_dr) / (1.0f - F_dr);  // from Jensen's Fdr ratio formula
  float alpha_prime = dipoleComputeAlphaPrime(albedo, fourthirdA);
  float z_r = sqrtf(3.0f * (1.0f - alpha_prime)) * d;

  float sigma_t_prime = 1.0f / fmaxf(z_r, 1e-16f);
  *sigma_t = sigma_t_prime / (1.0f - g);
#else
  /* Default Cycles' albedo inversion */
  /* From bssrdf.h: bssrdf_burley_setup() */
  /* Note that our radius d (for new anisotropic random walk) was scaled already with the bssrdf_burley_compatible_mfp to be "backwards-compatible" */
  *alpha = 1.0f - expf(albedo * (-5.09406f + albedo * (2.61188f - albedo * 4.31805f)));
  *sigma_t = 1.0f / fmaxf(d, 1e-16f); // d/s*s obviously simplifies out!
#endif
}

With anisotropy/g at 0, you will get a close result between the two inversion methods (though the new one is truer to the equivalent diffuse render, from same input albedo).
When you modify the anisotropy to go towards say 0.7, then you will see a strong energy loss with the old approach.

I didn't find a good way to preserve compatibility besides adding another mode. I'm thinking now perhaps it's useful anyway to be able to specify a manual radius (maybe from measurement), or to have it automatically adjusted based on albedo. So I might rename the option to make it a feature more than a compatibility thing.

For converting existing .blend files, renaming a socket can be done like this (untested, based on the updated patch): P2327

I tested this with some more .blend files besides our tests, and found that it does not preserve the color well for negative anisotropy or higher values like 0.8 (the value of skin from the paper). See the attached .blend file for example, the skin turns green at 0.8.

Is this something that can be improved? Otherwise it seems only anisotropy in the range 0.0..0.5 preserves albedo well?

In D11965#317236, @Brecht Van Lommel (brecht) wrote:

I tested this with some more .blend files besides our tests, and found that it does not preserve the color well for negative anisotropy or higher values like 0.8 (the value of skin from the paper). See the attached .blend file for example, the skin turns green at 0.8.
Is this something that can be improved? Otherwise it seems only anisotropy in the range 0.0..0.5 preserves albedo well?

We use routinely 0.7 to 0.8 for anisotropy (corresponding to measurements).
I think all the problems you seem to be facing (Sergey had a similar issue early on) is due to very high albedo inputs (for subsurface color). Here, the red channel goes above 0.9, even 0.99. These are not realistic values for organic albedos.
In any case, you can modify in subsurface_random_walk_remap the numerical protective clamp I had added, from:

*alpha = clamp(*alpha, 0.0f, 0.99f);  // because of numerical precision

into:

*alpha = clamp(*alpha, 0.0f, 0.999999f);  // because of numerical precision

This should take care of the hue shift.

(On the other hand, I am not sure about the negative anisotropy behavior. Probably our fits were never meant for that, since skin is very forward scattering. So maybe we should just clamp between 0 and 0.9 in svm_closure.h and in the interface)