Cycles: Fix inconsistencies in Principled Subsurface Scattering SVM/OSL #113192

Alaska · 2023-10-03T10:36:46+02:00

Alaska commented

2023-10-03 10:36:46 +02:00

The Subsurface Scattering part of the Principled BSDF SVM was using
the wrong IOR in some situations (Random Walk when IOR level != 0.5).

OSL for Subsurface Scattering was also setup in a way that made
comparisons between SVM and OSL confusing/inconsistent.

The Subsurface Scattering part of the Principled BSDF SVM was using the wrong IOR in some situations (Random Walk when IOR level != 0.5). OSL for Subsurface Scattering was also setup in a way that made comparisons between SVM and OSL confusing/inconsistent.

Alaska added the

Module

Render & Cycles

label 2023-10-03 10:36:46 +02:00

Alaska added 1 commit 2023-10-03 10:37:02 +02:00

Fix incorrect Subsurface Scattering in Principled BSDF OSL

buildbot/vexp-code-patch-coordinator Build done.

Details

31ca3190b1

Alaska requested review from Brecht Van Lommel 2023-10-03 10:37:13 +02:00

Alaska requested review from Lukas Stockner 2023-10-03 10:37:20 +02:00

Alaska reviewed 2023-10-03 10:40:52 +02:00

intern/cycles/kernel/osl/closures_setup.h Outdated

						
				@ -827,7 +827,7 @@ ccl_device void osl_closure_bssrdf_setup(KernelGlobals kg,

				  /* create one closure per color channel */

				  bssrdf->albedo = closure->albedo;

				  bssrdf->N = closure->N;

				  bssrdf->alpha = sqr(closure->roughness);

Alaska commented

2023-10-03 10:40:52 +02:00

No other closure does sqr(closure->roughness) so I removed it here for consistency sake, both with other OSL closures and SVM. Comment if you'd like this change reverted. I mainly did this because when investigating the code, I was confused why SVM used sqr(roughness) while OSL didn't.

Note: Only the Principled BSDF and Subsurface Scattering nodes use the bssrdf closure.
Both of them are unaffected by this change (Principled now passes through sqr(roughness) instead of roughness (similar to SVM which passes through sqr(roughness)), and the Subsurface Scatting node always sets roughness to 1.0)

No other closure does `sqr(closure->roughness)` so I removed it here for consistency sake, both with other OSL closures and SVM. Comment if you'd like this change reverted. I mainly did this because when investigating the code, I was confused why SVM used `sqr(roughness)` while OSL didn't. Note: Only the Principled BSDF and Subsurface Scattering nodes use the `bssrdf` closure. Both of them are unaffected by this change (Principled now passes through `sqr(roughness)` instead of `roughness` (similar to SVM which passes through `sqr(roughness)`), and the Subsurface Scatting node always sets roughness to 1.0)

Alaska changed title from ~~Fix incorrect Subsurface Scattering in Principled BSDF OSL~~ to Cycles: Fix incorrect Subsurface Scattering in Principled BSDF OSL

2023-10-03 10:41:41 +02:00

Brecht Van Lommel commented

2023-10-03 18:01:22 +02:00

@blender-bot build

Lukas Stockner commented

2023-10-04 02:52:59 +02:00

I originally left the Roughness as-is since theoretically a user-defined OSL shader could also use bssrdf. But yeah, we probably want this to be consistent, and we're making breaking changes anyways.

As for the IOR: Hmm, I'm actually not sure whether IOR Level should have influence here. Arguably the main specular reflection comes from the same layer interface that also defines the SSS entry, so they probably should be coupled together? In that case, it'd be the SVM code that needs to be changed.

No strong opinion from me either way, anyone else?

I originally left the Roughness as-is since theoretically a user-defined OSL shader could also use `bssrdf`. But yeah, we probably want this to be consistent, and we're making breaking changes anyways. As for the IOR: Hmm, I'm actually not sure whether IOR Level should have influence here. Arguably the main specular reflection comes from the same layer interface that also defines the SSS entry, so they probably should be coupled together? In that case, it'd be the SVM code that needs to be changed. No strong opinion from me either way, anyone else?

Alaska commented

2023-10-04 03:56:39 +02:00

As for the IOR: Hmm, I'm actually not sure whether IOR Level should have influence here. Arguably the main specular reflection comes from the same layer interface that also defines the SSS entry, so they probably should be coupled together?

The "Specular IOR Level" name suggests it only impacts the specular aspect of the material. As such I feel it's better if SSS isn't impacted by the "Specular IOR level".

But you are right. If the Specular and SSS entry are the same layer interface, then for physical correctness, they should have the same IOR.

What about using the Subsurface IOR instead of re-using the standard IOR?

> As for the IOR: Hmm, I'm actually not sure whether IOR Level should have influence here. Arguably the main specular reflection comes from the same layer interface that also defines the SSS entry, so they probably should be coupled together? The "Specular IOR Level" name suggests it only impacts the specular aspect of the material. As such I feel it's better if SSS isn't impacted by the "Specular IOR level". But you are right. If the Specular and SSS entry are the same layer interface, then for physical correctness, they should have the same IOR. What about using the Subsurface IOR instead of re-using the standard IOR?

Brecht Van Lommel commented

2023-10-04 17:33:24 +02:00

If we think of IOR Level as just a way to texture the IOR, then I think it should affect the SSS entry direction. And then SVM should be changed.

The "Specular IOR Level" name suggests it only impacts the specular aspect of the material. As such I feel it's better if SSS isn't impacted by the "Specular IOR level".

Layers affect other layers below, I think that is expected. With transmission it's justifiable to have no effect because it's blending with a different component instead of layering, but here I think there should be an impact.

What about using the Subsurface IOR instead of re-using the standard IOR?

I prefer to keep the basic Random Walk compatible with Standard Surface and OpenPBR.

If we think of IOR Level as just a way to texture the IOR, then I think it should affect the SSS entry direction. And then SVM should be changed. > The "Specular IOR Level" name suggests it only impacts the specular aspect of the material. As such I feel it's better if SSS isn't impacted by the "Specular IOR level". Layers affect other layers below, I think that is expected. With transmission it's justifiable to have no effect because it's blending with a different component instead of layering, but here I think there should be an impact. > What about using the Subsurface IOR instead of re-using the standard IOR? I prefer to keep the basic Random Walk compatible with Standard Surface and OpenPBR.

Brecht Van Lommel requested changes 2023-10-04 17:33:41 +02:00

Brecht Van Lommel left a comment

Requesting changes.

Alaska changed title from ~~Cycles: Fix incorrect Subsurface Scattering in Principled BSDF OSL~~ to Cycles: Fix incorrect Subsurface Scattering in Principled BSDF SVM

2023-10-05 01:29:21 +02:00

Alaska changed title from ~~Cycles: Fix incorrect Subsurface Scattering in Principled BSDF SVM~~ to Cycles: Fix inconsistencies in Principled Subsurface Scattering SVM/OSL

2023-10-05 01:35:38 +02:00

Alaska added 3 commits 2023-10-05 01:45:04 +02:00

Update SVM to match OSL instead of OSL to match SVM d161cc4675

Merge branch 'blender-v4.0-release' into fix-osl-subsurface bb4def6647

Fix roughness after merge with 4.0

buildbot/vexp-code-patch-coordinator Build done.

Details

d2a4cdc3eb

Alaska requested review from Brecht Van Lommel 2023-10-05 01:45:13 +02:00

Brecht Van Lommel approved these changes 2023-10-05 21:27:04 +02:00

Brecht Van Lommel commented

2023-10-05 21:27:13 +02:00