Shader: Only clamp undefined or unsupported inputs of Principled BSDF #112895

Merged
Lukas Stockner merged 39 commits from Alaska/blender:clamp-tint into blender-v4.0-release 2023-10-31 03:14:14 +01:00
Member

Adjust clamping of inputs in the Principled BSDF to avoid errors and
inconsistencies between render engines, while trying to leave as many
inputs as possible unclamped for artisitc purposes.

Adjust clamping of inputs in the Principled BSDF to avoid errors and inconsistencies between render engines, while trying to leave as many inputs as possible unclamped for artisitc purposes.
Alaska added the
Module
Render & Cycles
label 2023-09-26 11:33:48 +02:00
Author
Member

This was missed as part of #112774. I forgot about clamping colored inputs and wasn't available at the time the Principled BSDF 4.0 changes pull request was made and reviewed.

Clamping the sheen tint input fixes an issue where EEVEE supported negative values and rendered weird as a result.
Clamping coat tint fixes issues with negative values producing different results between Cycles SVM and Cycles OSL.

We might want to also clamp Emission Color and possibly Base Color?

This was missed as part of #112774. I forgot about clamping colored inputs and wasn't available at the time the Principled BSDF 4.0 changes pull request was made and reviewed. Clamping the sheen tint input fixes an issue where EEVEE supported negative values and rendered weird as a result. Clamping coat tint fixes issues with negative values producing different results between Cycles SVM and Cycles OSL. We might want to also clamp Emission Color and possibly Base Color?
Alaska requested review from Brecht Van Lommel 2023-09-26 11:39:36 +02:00
Alaska requested review from Lukas Stockner 2023-09-26 11:39:46 +02:00
Alaska requested review from Weizhen Huang 2023-09-26 11:39:55 +02:00
Lukas Stockner approved these changes 2023-09-26 13:50:26 +02:00
Lukas Stockner left a comment
Member

Clamping Base Color makes sense, for Emission I wouldn't do it - Emission naturally supports any positive value, while reflectivity (in a physically based renderer) is inherently limited to 0..1.

Clamping Base Color makes sense, for Emission I wouldn't do it - Emission naturally supports any positive value, while reflectivity (in a physically based renderer) is inherently limited to 0..1.
Author
Member

for Emission I wouldn't do it - Emission naturally supports any positive value

I should of clarified. Clamping Emission colour with max(emission_color, 0.0) so we don't get negative values.

Clamping Base Color makes sense ... reflectivity (in a physically based renderer) is inherently limited to 0..1.

Then we should clamp all of the tints to 0..1 as well. Because a lot of the tints at high values reflect more light than comes in.

> for Emission I wouldn't do it - Emission naturally supports any positive value I should of clarified. Clamping Emission colour with `max(emission_color, 0.0)` so we don't get negative values. > Clamping Base Color makes sense ... reflectivity (in a physically based renderer) is inherently limited to 0..1. Then we should clamp all of the tints to 0..1 as well. Because a lot of the tints at high values reflect more light than comes in.
Brecht Van Lommel changed title from Shader: Clamp tint properties in Principled BSDF to Shader: Clamp tint properties in Principled BSDF 2023-09-27 15:56:58 +02:00
brecht changed target branch from main to blender-v4.0-release 2023-09-27 15:57:00 +02:00

@blender-bot build linux

@blender-bot build linux
Alaska force-pushed clamp-tint from 47a1e5b3f9 to 833409cd06 2023-09-28 02:15:22 +02:00 Compare
Author
Member
  • Rebased against Blender-v4.0-release.
  • Clamped Sheen and Coat tint to [0..inifinity]
    • Avoid negative values which behave differently between different SVM, OSL, and EEVEE.
  • Clamped Base color to [0..1]
    • Avoid base colors that reflect more light than what's coming in.
  • Clamped emission color to [0..infinity]
    • If negative Emission strengths aren't supported, then negative emission colors shouldn't be supported.
- Rebased against Blender-v4.0-release. - Clamped Sheen and Coat tint to [0..inifinity] - Avoid negative values which behave differently between different SVM, OSL, and EEVEE. - Clamped Base color to [0..1] - Avoid base colors that reflect more light than what's coming in. - Clamped emission color to [0..infinity] - If negative Emission strengths aren't supported, then negative emission colors shouldn't be supported.
Alaska requested review from Lukas Stockner 2023-09-28 02:18:39 +02:00
Alaska changed title from Shader: Clamp tint properties in Principled BSDF to Shader: Clamp color inputs in Principled BSDF 2023-09-28 02:18:56 +02:00
Lukas Stockner reviewed 2023-10-02 02:33:11 +02:00
Lukas Stockner left a comment
Member

Mostly LGTM, just one minor comment.

Regarding tint clamping: Yeah, that one's a bit tricky. This sort of restriction is always a balance between preventing users from completely breaking the shader and preserving the ability to perform non-physical tweaks. For now, I think max(0.0, tint) is good enough.

Mostly LGTM, just one minor comment. Regarding tint clamping: Yeah, that one's a bit tricky. This sort of restriction is always a balance between preventing users from completely breaking the shader and preserving the ability to perform non-physical tweaks. For now, I think `max(0.0, tint)` is good enough.
@ -60,6 +60,7 @@ void node_bsdf_principled(vec4 base_color,
out Closure result)
{
/* Match cycles. */
base_color = clamp(base_color, vec4(0.0), vec4(1.0));
Member

I think this should be saturate(base_color).

I think this should be `saturate(base_color)`.
Alaska changed title from Shader: Clamp color inputs in Principled BSDF to Shader: Clamp extra inputs in Principled BSDF 2023-10-02 14:52:53 +02:00
Author
Member

I really should of thought of this earlier, but should we be clamping some of these values inside the individual materials instead of the Principled BSDF?

For example should we clamp roughness inside bsdf_microfacet_setup_fresnel_f82_tint and materials functions instead of the Principled BSDF?


Also, I've gotten a few negative reactions from people from my original clamping pull request. People want to be able to play with physically incorrect material properties for "artistic" effect.

I understand the desire for this, but a lot of the parameters we're clamping are to avoid unsupported/not well defined situations. And due to the unsupported or not well defined behavior, things can change in this area between updates, annoying these people using the settings for "artistic" effect.

So what are your thoughts on this? Should we be clamping the inputs?

I really should of thought of this earlier, but should we be clamping some of these values inside the individual materials instead of the Principled BSDF? For example should we clamp roughness inside `bsdf_microfacet_setup_fresnel_f82_tint` and materials functions instead of the Principled BSDF? --- Also, I've gotten a few negative reactions from people from my original clamping pull request. People want to be able to play with physically incorrect material properties for "artistic" effect. I understand the desire for this, but a lot of the parameters we're clamping are to avoid unsupported/not well defined situations. And due to the unsupported or not well defined behavior, things can change in this area between updates, annoying these people using the settings for "artistic" effect. So what are your thoughts on this? Should we be clamping the inputs?
Member

I really should of thought of this earlier, but should we be clamping some of these values inside the individual materials instead of the Principled BSDF?

In general, we should only be clamping in cases where the implementation actually requires it. Therefore, it'd be nicer to do so in the actual closure implementation (specifically, the _setup functions), yes. That also avoids the danger of overlooking it when using the same closure elsewhere, and e.g. allows deduplicating the clamping logic between SVM and OSL.

There are two counter-arguments that I can think of. First, there's the risk of duplicated calculations, if the same parameter is used by multiple closures. For a simple clamp that should be negligible, but if we apply the same logic to e.g. the ensure_valid_normal stuff, it might be noticeable. Second, it makes it harder to ensure that EEVEE uses the same clamping.

So what are your thoughts on this? Should we be clamping the inputs?

As said above - in general, I think we should clamp only if the implementation requires it. For example, negative colors are going to cause all sorts of issues, so that's definitely warranted. However, clamping BaseColor to <= 1.0 is already questionable. The same probably goes for the coat/sheen weights. Metallic/Transmission weights, meanwhile, actually need to be between 0 and 1.

We probably should create a table with all parameters and properly work out the hard and soft min/max values for each?

> I really should of thought of this earlier, but should we be clamping some of these values inside the individual materials instead of the Principled BSDF? In general, we should only be clamping in cases where the implementation actually requires it. Therefore, it'd be nicer to do so in the actual closure implementation (specifically, the `_setup` functions), yes. That also avoids the danger of overlooking it when using the same closure elsewhere, and e.g. allows deduplicating the clamping logic between SVM and OSL. There are two counter-arguments that I can think of. First, there's the risk of duplicated calculations, if the same parameter is used by multiple closures. For a simple clamp that should be negligible, but if we apply the same logic to e.g. the `ensure_valid_normal` stuff, it might be noticeable. Second, it makes it harder to ensure that EEVEE uses the same clamping. > So what are your thoughts on this? Should we be clamping the inputs? As said above - in general, I think we should clamp only if the implementation requires it. For example, negative colors are going to cause all sorts of issues, so that's definitely warranted. However, clamping BaseColor to <= 1.0 is already questionable. The same probably goes for the coat/sheen weights. Metallic/Transmission weights, meanwhile, actually need to be between 0 and 1. We probably should create a table with all parameters and properly work out the hard and soft min/max values for each?
Author
Member

We probably should create a table with all parameters and properly work out the hard and soft min/max values for each?

I removed many of the clamps from the Principled BSDF and tested different values. I tested both Cycles SVM and EEVEE. All the images are from Cycles, but if I noticed a difference in EEVEE, I put it in the notes. I used the Filmic view transform.

Note: When something has a multi-channel input (Color, scattering radius, etc), I tested both the color(X) and color(X, Y, Y) case. E.G. Color(-1.0) and Color(-1.0, 0.0, 0.0) case.

Note 2: I have included recommendations on how I believe the values should be clamped. Please comment if you disagree. Also if you think something should be clamped differently for software interoperability (OpenPBR), just say.


-1.0 -0.5 0.0 0.5 1.0 2.0
Base Color - Diffuse Same as 0.0 Same as 0.0 Base Color - Diffuse - 0.0.jpg Base Color - Diffuse - 0.5.jpg Base Color - Diffuse - 1.0.jpg Base Color - Diffuse - 2.0.jpg

Notes: EEVEE's specular layer behaves weirdly when the colour is below 0 (changing colour or disappearing). Cycles behaves like color is clamped to max(color, 0.0).

My recommendation: Clamp Base Color to [0..infinity].

  • Values below 0 behave weirdly with EEVEE and different from Cycles. And values above 1.0 are physically incorrect, but useful for artistic purposes.

-1.0 -0.5 0.0 0.5 1.0 2.0 10.0
Base Color - Metallic Same as -0.5 Base Color - Metallic - -0.5.jpg Base Color - Metallic - 0.0.jpg Base Color - Metallic - 0.5.jpg Base Color - Metallic - 1.0.jpg Base Color - Metallic - 2.0.jpg Base Color - Metallic - 10.0.jpg

Notes: The specular reflection changes colour if only one of the color values is below 0.0

My recommendation: Clamp the Base Color input to Metallic to [0..1].

  • Values below 0.0 behave weirdly, and at a certain point above 1.0, the material renders incorrectly.

Note: If we clamp the Base Color to the diffuse component to [0..infinity] while clamping the Base Color to the Metallic component to [0..1], this could lead to unexpected behaviour for end users. How should we handle this? Should we try fixing the Metallic closure so it renders more predictably when using values above 1.0?


-1.0 -0.5 0.0 0.5 1.0 2.0
Base Color - Subsurface Base Color - SSS - -1.0.jpg Base Color - SSS - -0.5.jpg Base Color - SSS - 0.0.jpg Base Color - SSS - 0.5.jpg Base Color - SSS - 1.0.jpg Base Color - SSS - 2.0.jpg

Notes: With Random Walk and Random Walk (Skin), values below 0 introduced noise. With Christensen-Burley, the result starts noisy, then converges to a negative value. EEVEE behaves like max(color, 0.0).

My recommendation: Clamp the Base Color input to SSS to [0..1].

  • Values above 1.0 are darker than 1.0 with the Random Walk methods, but also reflects more light than is coming in. This is weird behaviour. Christensen-Burley and EEVEE works as expected. Values above 1.0 increase the brightness.

Note: Similar to the Base Color Metallic, if we clamp Base Color SSS to [0..1] and Base Color diffuse to [0..infinity], this could cause confusion for the end user. HOw do you want to handle this? Do we want to try fixing the issue that causes the darkening with Random Walk?


Edit: Base Color Transmission. I forgot to test this initially. Since I've run out of upload spots. I'll just describe it.

My recommendation: Clamp the Base Color input to Transmission to [0..infinity].

  • Values below 0 just render black with no reflections (EEVEE still renders with reflections). Values higher than 1 can be used for artistic purposed.

Metallic - It's a mix between closures. Clamp to [0..1].


-1.0 -0.5 0.0 0.5 1.0 2.0
Roughness Roughness - -1.0.jpg Roughness - -0.5.jpg Roughness - 0.0.jpg Roughness - 0.5.jpg Roughness - 1.0.jpg Roughness - 2.0.jpg

Note: In Cycles, negative roughness is equal to positive roughness because of the sqr(roughness) used in most places. Cycles also clamps the roughness to [0..1] inside bsdf_microfacet_ggx_setup because the behavior above 1.0 is weird. Along with that, Metallic, Specular, and Transmission behave the same, so I only included images for the Metallic case. EEVEE does not support negative roughnesses, and seems to support roughnesses higher than 1.0.

My recommendation: Clamp roughness to [0..1]. Other options include adding support for negative roughnesses in EEVEE with abs(roughness)

  • Values below 0.0 are not supported by EEVEE, and values below 1.0 are not well supported by Cycles.

IOR - Clamp to [0..Infinity].

  • Negative values are not supported.

-1.0 -0.5 0.0 1.0 2.0
Alpha Alpha - -1.0.jpg Alpha - -0.5.jpg Alpha - 0.0.jpg Alpha - 1.0.jpg Alpha - 2.0.jpg

Note: EEVEE doesn't support the negative alpha case.

My recommendation: Clamp Alpha to [0..1].

  • Values outside that range are unsupported.

Subsurface Weight - It's a mix between closures, clamp to [0..1]


Subsurface Radius - Clamp to [0..Infinity], Negative values are not physically correct, and behaviour can be a bit weird depending on what settings you combine it with.


Subsurface Scale - Clamp to [0..Infinity], Negative values are not physically correct, and behaviour can be a bit weird depending on what settings you combine it with.


Subsurface IOR - Clamp to [1.01..3.8], Already occurs in bssrdf_setup, values below 1.0 are not supported and values above 3.8 probably behave weirdly.


Subsurface Aniostropy - Clamp to [0..0.9], Already occurs in bssrdf_setup, probably due to weird behaviour outside that range.


0.0 0.5 1.0 10.0
Specular IOR Level IOR Level - 0.0.jpg IOR Level - 0.5.jpg IOR Level - 1.0.jpg IOR Level - 10.0.jpg

My recommendation: Clamp Specular IOR Level to [0..infinity].

  • Values below 0 are not supported (it results in f0 with negative values and the ior_from_F0() function doesn't support negative f0 values).

-1.0 0.0 1.0 10.0
Specular Tint - Specular Layer Same as 0.0 Specular Tinit - Specular - 0.0.jpg Specular Tinit - Specular - 1.0.jpg Specular Tinit - Specular - 10.0.jpg

Note: It seems like negative values are not supported. Probably clamped somewhere inside closures.

My recommendation: Clamp Specular Tint to [0..infinity].

  • Values below 0 don't appear to do anything. And values above 1.0 can be used for artisitc purposes.

Continued in next comment. I reached the upload limit.

> We probably should create a table with all parameters and properly work out the hard and soft min/max values for each? I removed many of the clamps from the Principled BSDF and tested different values. I tested both Cycles SVM and EEVEE. All the images are from Cycles, but if I noticed a difference in EEVEE, I put it in the notes. I used the Filmic view transform. Note: When something has a multi-channel input (Color, scattering radius, etc), I tested both the color(X) and color(X, Y, Y) case. E.G. Color(-1.0) and Color(-1.0, 0.0, 0.0) case. Note 2: I have included recommendations on how I believe the values should be clamped. Please comment if you disagree. Also if you think something should be clamped differently for software interoperability (OpenPBR), just say. --- ||-1.0|-0.5|0.0|0.5|1.0|2.0| |-|-|-|-|-|-|-| |Base Color - Diffuse|Same as 0.0|Same as 0.0|![Base Color - Diffuse - 0.0.jpg](/attachments/6b290147-3094-4765-ac40-6500f23947c4)|![Base Color - Diffuse - 0.5.jpg](/attachments/9210ad55-b2a4-4fff-a804-b2571b5da134)|![Base Color - Diffuse - 1.0.jpg](/attachments/550ead68-7493-4bf1-b932-29d694f56e33)|![Base Color - Diffuse - 2.0.jpg](/attachments/3f9b4330-5965-4f74-a6e1-b30b751f52b1)| Notes: EEVEE's specular layer behaves weirdly when the colour is below 0 (changing colour or disappearing). Cycles behaves like color is clamped to max(color, 0.0). My recommendation: Clamp Base Color to [0..infinity]. - Values below 0 behave weirdly with EEVEE and different from Cycles. And values above 1.0 are physically incorrect, but useful for artistic purposes. --- ||-1.0|-0.5|0.0|0.5|1.0|2.0|10.0| |-|-|-|-|-|-|-|-| |Base Color - Metallic|Same as -0.5|![Base Color - Metallic - -0.5.jpg](/attachments/86648354-d380-40e9-b8e2-93b0a3489072)|![Base Color - Metallic - 0.0.jpg](/attachments/ad87d46b-4fb1-4d52-95c1-a74ab5fbabf6)|![Base Color - Metallic - 0.5.jpg](/attachments/8c09e98d-22c0-402d-9615-2bfba0c984d5)|![Base Color - Metallic - 1.0.jpg](/attachments/3aa14ac4-4001-4ac2-9a31-33c5604a670d)|![Base Color - Metallic - 2.0.jpg](/attachments/5fd3cb8a-4f1c-4bf3-b72c-dc02bbc6d271)|![Base Color - Metallic - 10.0.jpg](/attachments/ddb758e5-d341-4557-840a-2c3e495d9473)| Notes: The specular reflection changes colour if only one of the color values is below 0.0 My recommendation: Clamp the Base Color input to Metallic to [0..1]. - Values below 0.0 behave weirdly, and at a certain point above 1.0, the material renders incorrectly. Note: If we clamp the Base Color to the diffuse component to [0..infinity] while clamping the Base Color to the Metallic component to [0..1], this could lead to unexpected behaviour for end users. How should we handle this? Should we try fixing the Metallic closure so it renders more predictably when using values above 1.0? --- ||-1.0|-0.5|0.0|0.5|1.0|2.0| |-|-|-|-|-|-|-| |Base Color - Subsurface|![Base Color - SSS - -1.0.jpg](/attachments/ab57f323-6532-41ab-b13c-31018783373e)|![Base Color - SSS - -0.5.jpg](/attachments/dbcf3bdf-de6a-4fad-ba43-6ba2dd338d8c)|![Base Color - SSS - 0.0.jpg](/attachments/6d85d272-b158-4536-925e-57a4612e84ff)|![Base Color - SSS - 0.5.jpg](/attachments/a056ad96-8532-4ac6-bf57-6503e9f7c557)|![Base Color - SSS - 1.0.jpg](/attachments/f44803d3-177d-4e05-9a34-5c34602b818e)|![Base Color - SSS - 2.0.jpg](/attachments/1812d01c-f2c2-4c15-9254-071d295efc28)| Notes: With Random Walk and Random Walk (Skin), values below 0 introduced noise. With Christensen-Burley, the result starts noisy, then converges to a negative value. EEVEE behaves like max(color, 0.0). My recommendation: Clamp the Base Color input to SSS to [0..1]. - Values above 1.0 are darker than 1.0 with the Random Walk methods, but also reflects more light than is coming in. This is weird behaviour. Christensen-Burley and EEVEE works as expected. Values above 1.0 increase the brightness. Note: Similar to the Base Color Metallic, if we clamp Base Color SSS to [0..1] and Base Color diffuse to [0..infinity], this could cause confusion for the end user. HOw do you want to handle this? Do we want to try fixing the issue that causes the darkening with Random Walk? --- Edit: Base Color Transmission. I forgot to test this initially. Since I've run out of upload spots. I'll just describe it. My recommendation: Clamp the Base Color input to Transmission to [0..infinity]. - Values below 0 just render black with no reflections (EEVEE still renders with reflections). Values higher than 1 can be used for artistic purposed. --- Metallic - It's a mix between closures. Clamp to [0..1]. --- ||-1.0|-0.5|0.0|0.5|1.0|2.0| |-|-|-|-|-|-|-| |Roughness|![Roughness - -1.0.jpg](/attachments/3a5b24f8-d0af-4460-8138-32e8466b78d0)|![Roughness - -0.5.jpg](/attachments/7b27c4e1-88c5-47e2-88c7-21a19cac2324)|![Roughness - 0.0.jpg](/attachments/8b1ee4d9-4465-4632-b2d0-b52a875428d0)|![Roughness - 0.5.jpg](/attachments/bec51460-9d0d-481b-9891-db2e55c4da99)|![Roughness - 1.0.jpg](/attachments/3904aef0-731b-4792-bb9d-ef6672807638)|![Roughness - 2.0.jpg](/attachments/1fd249a2-1865-4c39-85ea-d54fd8a8f102)| Note: In Cycles, negative roughness is equal to positive roughness because of the `sqr(roughness)` used in most places. Cycles also clamps the roughness to [0..1] inside `bsdf_microfacet_ggx_setup` because the behavior above 1.0 is weird. Along with that, Metallic, Specular, and Transmission behave the same, so I only included images for the Metallic case. EEVEE does not support negative roughnesses, and seems to support roughnesses higher than 1.0. My recommendation: Clamp roughness to [0..1]. Other options include adding support for negative roughnesses in EEVEE with `abs(roughness)` - Values below 0.0 are not supported by EEVEE, and values below 1.0 are not well supported by Cycles. --- IOR - Clamp to [0..Infinity]. - Negative values are not supported. --- ||-1.0|-0.5|0.0|1.0|2.0| |-|-|-|-|-|-| |Alpha|![Alpha - -1.0.jpg](/attachments/45fd0783-fb19-4771-aef4-2932e5d478eb)|![Alpha - -0.5.jpg](/attachments/06e1759a-e5fe-49b9-b484-c68c99c03473)|![Alpha - 0.0.jpg](/attachments/724f802e-df64-45d4-95ad-fd94ec501c58)|![Alpha - 1.0.jpg](/attachments/db871baf-503c-44fa-a526-eea0a80ec3af)|![Alpha - 2.0.jpg](/attachments/73c34a8c-9ec0-401b-b689-fa071c95b6b7)| Note: EEVEE doesn't support the negative alpha case. My recommendation: Clamp Alpha to [0..1]. - Values outside that range are unsupported. --- Subsurface Weight - It's a mix between closures, clamp to [0..1] --- Subsurface Radius - Clamp to [0..Infinity], Negative values are not physically correct, and behaviour can be a bit weird depending on what settings you combine it with. --- Subsurface Scale - Clamp to [0..Infinity], Negative values are not physically correct, and behaviour can be a bit weird depending on what settings you combine it with. --- Subsurface IOR - Clamp to [1.01..3.8], Already occurs in `bssrdf_setup`, values below 1.0 are not supported and values above 3.8 probably behave weirdly. --- Subsurface Aniostropy - Clamp to [0..0.9], Already occurs in `bssrdf_setup`, probably due to weird behaviour outside that range. --- ||0.0|0.5|1.0|10.0| |-|-|-|-|-|-| |Specular IOR Level|![IOR Level - 0.0.jpg](/attachments/d2a28e3c-ae07-45b6-9564-212887f4acb7)|![IOR Level - 0.5.jpg](/attachments/d066cc27-431c-4fed-9f41-0384f88db514)|![IOR Level - 1.0.jpg](/attachments/9d49b1d0-77b5-450a-bf11-c417e08feaa7)|![IOR Level - 10.0.jpg](/attachments/39e0e08c-49cd-4050-8fe2-3fe14b1f19bb)| My recommendation: Clamp Specular IOR Level to [0..infinity]. - Values below 0 are not supported (it results in `f0` with negative values and the `ior_from_F0()` function doesn't support negative `f0` values). --- ||-1.0|0.0|1.0|10.0| |-|-|-|-|-| |Specular Tint - Specular Layer|Same as 0.0|![Specular Tinit - Specular - 0.0.jpg](/attachments/0eba4913-72b7-40e7-aa7e-dd258f66d6e0)|![Specular Tinit - Specular - 1.0.jpg](/attachments/7089b109-fc73-406e-9b3a-52ea1027b769)|![Specular Tinit - Specular - 10.0.jpg](/attachments/e667a911-b926-45ab-9c97-2cc5dd1b9bcc)| Note: It seems like negative values are not supported. Probably clamped somewhere inside closures. My recommendation: Clamp Specular Tint to [0..infinity]. - Values below 0 don't appear to do anything. And values above 1.0 can be used for artisitc purposes. --- Continued in next comment. I reached the upload limit.
Author
Member
-10.0 -1.0 0.0 1.0 10.0
Specular Tint - Metallic Layer Specular Tinit - Metallic - -10.0.jpg Specular Tinit - Metallic - -1.0.jpg Specular Tinit - Metallic - 0.0.jpg Specular Tinit - Metallic - 1.0.jpg Specular Tinit - Metallic - 10.0.jpg

Note: When using negative values, the render does not produce any negative values, it's just 0,0,0 in the dark regions.

My recommendation: I don't know what to recommend here. What are your thoughts?


-1.0 0.0 1.0 10.0
Specular Tint - Transmission Layer Specular Tinit - Transmission - -1.0.jpg Specular Tinit - Transmission - 0.0.jpg Specular Tinit - Transmission - 1.0.jpg Specular Tinit - Transmission - 10.0.jpg

Note: EEVEE behaves differently when using negative values. Specifically showing negative reflections.

My recommendation: Clamp to [0..infinity].

  • Values below 0 don't appear to be supported in Cycles anyway, and values below 0 behvae weirdly in EEVEE. Values greater than 1 can be used for artisitc purposes

-1.0 0.0 1.0 1.111 1.112
Anisotropic Anisotropic - -1.0.jpg Anisotropic - 0.0.jpg Anisotropic - 1.0.jpg Anisotropic - 1.111.jpg Anisotropic - 1.112.jpg

My recommendation: Clamp to [0..1].

  • Values below 0.0 are not supported. Values above 1.0 should not be supported due to weird behaviour.

Ansiotropic Rotation - Don't clamp. All values are supported due to cosf(angle) and sinf(angle) being repeating functions.


Transmission Weight - It's a mix between closures, clamp to [0..1]


-1.0 0.0 1.0 10.0 100.0
Coat Weight Coat Weight - -1.0.jpg Coat Weight - 0.0.jpg Coat Weight - 1.0.jpg Coat Weight - 10.0.jpg Coat Weight - 100.0.jpg

Note: The coat was applied on top of a emission material with a color(1.0) and strength 1.0. This is because some of the issues only appear when applying the coat layer on top of an emission layer.

My recommendation: Clamp Coat Weight to [0..1].

  • Values below 0.0 don't do anything. Values above 1.0 can cause weird renders.

Coat Roughness - Clamp to [0..1], values below 0 are not supported on EEVEE, and values above 1.0 are not properly supported with Cycles.


Coat IOR - Clamp to [1.0..infinity], values below 1.0 are not supported.


-1.0 -0.5 0.0 1.0 2.0 10.0
Coat Tint Same as -0.5 Coat Tint - -0.5.jpg Coat Tint - 0.0.jpg Coat Tint - 1.0.jpg Coat Tint - 2.0.jpg Coat Tint - 10.0.jpg

Note: EEVEE behaves differently than Cycles if only some of the channels are below 0.0.

My recommendation: Clamp Coat Tint to [0..infinity].

  • Values below 0.0 produce weird results. Values above 1.0 can be used for artistic effects.

-1.0 0.0 1.0 10.0
Sheen Weight Sheen Weight - -1.0.jpg Sheen Weight - 0.0.jpg Sheen Weight - 1.0.jpg Sheen Weight - 10.0.jpg

Note: The sheen was applied on top of a emission material with a color(1.0) and strength 1.0. This is because some of the issues only appear when applying the sheen layer on top of an emission layer.

My recommendation: Clamp Sheen weight to [0..1].

  • Values below 0.0 does nothing. Values above 1.0 can produce weird results.

Sheen roughness - Clamp to [0..1]. Values below 0 are not supported. And values above 1.0 are not supported in Cycles (but are supported in EEVEE)


Sheen Tint - Behaves similar to Sheen weight and as such, produces weird results above 1.0.

My recommendation: Clamp Sheen Tint to [0..1].

  • Values below 0.0 does nothing. Values above 1.0 produce weird results.

Emission color and emission strength - Don't clamp these values. Negative values can be used for artistic effects, and it supports all positive values.




The coat was applied on top of a emission material with a color(1.0) and strength 1.0. This is because some of the issues only appear when applying the coat layer on top of an emission layer.

The sheen was applied on top of a emission material with a color(1.0) and strength 1.0. This is because some of the issues only appear when applying the sheen layer on top of an emission layer.

Both of these issues seem to stem from the weight being negative, and the emission layer seems to react to this case differently from every other closure, resulting in the emission layer emitting negative light. This issue can be fixed with a simple change, giving us the ability to "safely" clamp Coat and Sheen weight, and Sheen Tint, to [0..infinity].

Simple change to fix this issue: a546e4779e (This is in my fork of Blender)
Note: The change is only for SVM since OSL already handled this case correctly. I haven't looked into EEVEE.

||-10.0|-1.0|0.0|1.0|10.0| |-|-|-|-|-|-| |Specular Tint - Metallic Layer|![Specular Tinit - Metallic - -10.0.jpg](/attachments/edbb13cc-da89-4362-9304-1b3c532166bb)|![Specular Tinit - Metallic - -1.0.jpg](/attachments/a0869590-ffd8-41b0-b0ea-b595db809b10)|![Specular Tinit - Metallic - 0.0.jpg](/attachments/a2e4986a-5917-475e-99fa-dcc942525bfa)|![Specular Tinit - Metallic - 1.0.jpg](/attachments/5cecd523-7f39-4e4e-95b8-100affacd74c)|![Specular Tinit - Metallic - 10.0.jpg](/attachments/a1a511d7-dfcb-465a-b8af-517c1a21236a)| Note: When using negative values, the render does not produce any negative values, it's just 0,0,0 in the dark regions. My recommendation: I don't know what to recommend here. What are your thoughts? --- ||-1.0|0.0|1.0|10.0| |-|-|-|-|-| |Specular Tint - Transmission Layer|![Specular Tinit - Transmission - -1.0.jpg](/attachments/d271b8c7-8f67-477c-8826-be64513def8c)|![Specular Tinit - Transmission - 0.0.jpg](/attachments/24503f6d-b405-4ac2-8f6f-efdefa21b12d)|![Specular Tinit - Transmission - 1.0.jpg](/attachments/1e9db29f-c54f-4690-a085-74f3978a811b)|![Specular Tinit - Transmission - 10.0.jpg](/attachments/d4d9f897-c6de-4f88-8cdf-7a3364b6e759)| Note: EEVEE behaves differently when using negative values. Specifically showing negative reflections. My recommendation: Clamp to [0..infinity]. - Values below 0 don't appear to be supported in Cycles anyway, and values below 0 behvae weirdly in EEVEE. Values greater than 1 can be used for artisitc purposes --- ||-1.0|0.0|1.0|1.111|1.112| |-|-|-|-|-|-|-| |Anisotropic|![Anisotropic - -1.0.jpg](/attachments/1523fad3-6862-4915-9c67-3286955e3203)|![Anisotropic - 0.0.jpg](/attachments/272e9aeb-2ab5-4eb4-823c-cd5b49515346)|![Anisotropic - 1.0.jpg](/attachments/8869ef6d-8407-4515-b456-d07c720d9ed6)|![Anisotropic - 1.111.jpg](/attachments/a3b62cb7-f6bf-40b4-8aa6-de917008f118)|![Anisotropic - 1.112.jpg](/attachments/2376e2fa-7362-4d68-9d87-77945ca06cd4)| My recommendation: Clamp to [0..1]. - Values below 0.0 are not supported. Values above 1.0 should not be supported due to weird behaviour. --- Ansiotropic Rotation - Don't clamp. All values are supported due to `cosf(angle)` and `sinf(angle)` being repeating functions. --- Transmission Weight - It's a mix between closures, clamp to [0..1] --- ||-1.0|0.0|1.0|10.0|100.0| |-|-|-|-|-|-| |Coat Weight|![Coat Weight - -1.0.jpg](/attachments/0b85d386-6101-4767-a051-3ab28a73725c)|![Coat Weight - 0.0.jpg](/attachments/2e39f89e-98ca-47da-a698-377f8386d217)|![Coat Weight - 1.0.jpg](/attachments/3bb46f76-dd36-41fb-8c4d-584300ee6dab)|![Coat Weight - 10.0.jpg](/attachments/1ef1d06c-bd4e-46bd-a8e6-93641018cdec)|![Coat Weight - 100.0.jpg](/attachments/cd8514de-19b8-4790-8366-b0b55e3d247b)| Note: The coat was applied on top of a emission material with a color(1.0) and strength 1.0. This is because some of the issues only appear when applying the coat layer on top of an emission layer. My recommendation: Clamp Coat Weight to [0..1]. - Values below 0.0 don't do anything. Values above 1.0 can cause weird renders. --- Coat Roughness - Clamp to [0..1], values below 0 are not supported on EEVEE, and values above 1.0 are not properly supported with Cycles. --- Coat IOR - Clamp to [1.0..infinity], values below 1.0 are not supported. --- ||-1.0|-0.5|0.0|1.0|2.0|10.0| |-|-|-|-|-|-|-| |Coat Tint|Same as -0.5|![Coat Tint - -0.5.jpg](/attachments/09fb2165-2c52-411d-93fc-0f4041444c0d)|![Coat Tint - 0.0.jpg](/attachments/52f082cb-b7cc-4d57-a97d-273e7beae83f)|![Coat Tint - 1.0.jpg](/attachments/a32b8bde-a6b1-49da-898c-f1ea8fbfba44)|![Coat Tint - 2.0.jpg](/attachments/5b70db65-ebda-43da-b1bb-c4d7145f2222)|![Coat Tint - 10.0.jpg](/attachments/490ed2c7-cbb0-48b3-8ba9-6829ffea74db)| Note: EEVEE behaves differently than Cycles if only some of the channels are below 0.0. My recommendation: Clamp Coat Tint to [0..infinity]. - Values below 0.0 produce weird results. Values above 1.0 can be used for artistic effects. --- ||-1.0|0.0|1.0|10.0| |-|-|-|-|-| |Sheen Weight|![Sheen Weight - -1.0.jpg](/attachments/3b0f2f89-33e5-4565-a276-f4ce01bb0daf)|![Sheen Weight - 0.0.jpg](/attachments/e40ecd3d-c31f-4fd1-9d88-ac6738625587)|![Sheen Weight - 1.0.jpg](/attachments/41bf158f-2f96-40df-b9cf-65c8acd15bcc)|![Sheen Weight - 10.0.jpg](/attachments/0c6bb3c9-c21e-48b6-b91b-a0c7c05a5af4)| Note: The sheen was applied on top of a emission material with a color(1.0) and strength 1.0. This is because some of the issues only appear when applying the sheen layer on top of an emission layer. My recommendation: Clamp Sheen weight to [0..1]. - Values below 0.0 does nothing. Values above 1.0 can produce weird results. --- Sheen roughness - Clamp to [0..1]. Values below 0 are not supported. And values above 1.0 are not supported in Cycles (but are supported in EEVEE) --- Sheen Tint - Behaves similar to Sheen weight and as such, produces weird results above 1.0. My recommendation: Clamp Sheen Tint to [0..1]. - Values below 0.0 does nothing. Values above 1.0 produce weird results. --- Emission color and emission strength - Don't clamp these values. Negative values can be used for artistic effects, and it supports all positive values. --- --- --- >The coat was applied on top of a emission material with a color(1.0) and strength 1.0. This is because some of the issues only appear when applying the coat layer on top of an emission layer. >The sheen was applied on top of a emission material with a color(1.0) and strength 1.0. This is because some of the issues only appear when applying the sheen layer on top of an emission layer. Both of these issues seem to stem from the weight being negative, and the emission layer seems to react to this case differently from every other closure, resulting in the emission layer emitting negative light. This issue can be fixed with a simple change, giving us the ability to "safely" clamp Coat and Sheen weight, and Sheen Tint, to [0..infinity]. Simple change to fix this issue: https://projects.blender.org/Alaska/blender/commit/a546e4779efb56721389c0a922833e6e0c71fa9e (This is in my fork of Blender) Note: The change is only for SVM since OSL already handled this case correctly. I haven't looked into EEVEE.
First-time contributor

Personally, I tweak all sorts of values to create materials and effects that look like they don't quite belong in this universe. Crystals that amplify the light passing through them, fire that emits darkness rather than light, metal with wrong reflections, distortions in spacetime that jumble the light passing through them, etc. I've gotten cool effects using IOR <1, for example. As far as physical accuracy, refractive indices that are less than 1, or even negative, are physically possible in things like plasmas or metamaterials.

Personally, I tweak all sorts of values to create materials and effects that look like they don't quite belong in this universe. Crystals that amplify the light passing through them, fire that emits darkness rather than light, metal with wrong reflections, distortions in spacetime that jumble the light passing through them, etc. I've gotten cool effects using IOR <1, for example. As far as physical accuracy, refractive indices that are less than 1, or even negative, are physically possible in things like plasmas or metamaterials.
Alaska changed title from Shader: Clamp extra inputs in Principled BSDF to WIP: Shader: Clamp extra inputs in Principled BSDF 2023-10-10 04:07:52 +02:00
Alaska force-pushed clamp-tint from b8c556b65c to 58442a420e 2023-10-11 02:23:29 +02:00 Compare
Alaska added 25 commits 2023-10-11 07:16:20 +02:00
Author
Member

I have updated this commit with new clamping. The aim is to clamp only what needs to be clamped for consistency sake, or to avoid errors. Here is a list explaining each clamp:

  • Base colour is clamped to the [0..infinity] range.
    • Values below zero behave differently depending on the render engine and the behaviour is generally weird. Below 0 is not a supported option. Values above 1 can be used for artisitc purposes.
    • If Metallic, Subsurface Scattering, or Tranmission is used, then the base color is clamped to [0..1]. All these materials behave unexpectably above 1.0. A mix is applied to try and achieve a gradual transition from the unclamped materials (Diffuse) to the clamped materials (E.G. Metal).
  • Metallic is clamped to [0..1].
    • It's a mix between two closures. Values outside this range are not supported.
  • All roughness values are clamped to [0..1].
    • Values below 0 are supported in Cycles, but not EEVEE. Values above 1 are sometimes supported in EEVEE and not Cycles.
    • If users want to use negative values, they can connect a "absolute" math node to their roughness. This will give you the exact same behaviour as Blender 3.6 which didn't clamp these values the same way.
  • IOR is clamped to [0..infinity].
    • Values below 0 aren't supported.
  • Alpha is clamped to [0..1].
    • Behaviour outside this range are not well defined and are inconsistent between render engines.

  • Subsurface Weight is clamped to [0..1].
    • It's a mix between two closures. Values outside this range are not supported.

Edit: Subsurface Radius and Subsurface Scale aren't clamped to [0..infinity]. The product of the two are clamped. Allowing for negative radius and negative scale to be used together to create a positive radius. (E.G. -1 * -1 = 1)

  • Subsurface Radius is clamped to [0..infinity].
    • Behaviour with a negative radius is not well defined, is inconsistent between render engines, and doesn't make sense.
  • Subsurface Scale is clamped to [0..infinity].
    • Behaviour with a negative radius is not well defined, is inconsistent between render engines, and doesn't make sense.
  • Subsurface Anisotropy is clamped to [0..0.9].
    • Values outside this range don't render properly.
  • Subsurface IOR is clamped to [1.01..3.8].
    • Values outside this range aren't supported.

  • Specular IOR Level is clamped to [0..infinity].
    • Negative values are not supported.
  • Specular Tint is clamped to [0..infinity].
    • Values below 0 typically doesn't do anything.
    • When Specular Tint to used on the metallic component, it is clamped to [0..1]. Values higher than 1 are not supported in Cycles, but are supported in EEVEE. This clamping is done to ensure consistency between render engines.
  • Anisotropic is clamped to [0..1].
    • Values outside this range don't behave properly.
  • Anisotropic rotation is unclamped.

  • Transmission is clamped to [0..1].
    • It's a mix between two closures. Values outside this range are not supported.

  • Coat weight is clamped to [0..infinity].
    • It behaves differently between EEVEE and Cycles in a major way when above 1.0. Clamping for consistency sake. The issue mentioned here has been fixed.
  • Coat IOR is clamped to [1..infinity].
    • Values below 1 are not supported.
  • Coat tint is clamped to [0..infinity].
    • Kernel asserts are met when using negative values.

  • Sheen weight is clamped to [0..infinity].
    • Due to the code layout, values below 0 are ignored.
  • Sheen tint is clamped to [0..infinity].
    • Values below 0 are not supported in Cycles, but supported in EEVEE. So this clamp is to make sure they match.

  • Emission Color is unclamped.
  • Emission Strength is unclamped.
I have updated this commit with new clamping. The aim is to clamp only what needs to be clamped for consistency sake, or to avoid errors. Here is a list explaining each clamp: - Base colour is clamped to the [0..infinity] range. - Values below zero behave differently depending on the render engine and the behaviour is generally weird. Below 0 is not a supported option. Values above 1 can be used for artisitc purposes. - If Metallic, Subsurface Scattering, or Tranmission is used, then the base color is clamped to [0..1]. All these materials behave unexpectably above 1.0. ~~A mix is applied to try and achieve a gradual transition from the unclamped materials (Diffuse) to the clamped materials (E.G. Metal).~~ - Metallic is clamped to [0..1]. - It's a mix between two closures. Values outside this range are not supported. - All roughness values are clamped to [0..1]. - Values below 0 are supported in Cycles, but not EEVEE. Values above 1 are sometimes supported in EEVEE and not Cycles. - If users want to use negative values, they can connect a "absolute" math node to their roughness. This will give you the exact same behaviour as Blender 3.6 which didn't clamp these values the same way. - IOR is clamped to [0..infinity]. - Values below 0 aren't supported. - Alpha is clamped to [0..1]. - Behaviour outside this range are not well defined and are inconsistent between render engines. --- - Subsurface Weight is clamped to [0..1]. - It's a mix between two closures. Values outside this range are not supported. Edit: Subsurface Radius and Subsurface Scale aren't clamped to [0..infinity]. The product of the two are clamped. Allowing for negative radius and negative scale to be used together to create a positive radius. (E.G. `-1 * -1 = 1`) - Subsurface Radius is clamped to [0..infinity]. - Behaviour with a negative radius is not well defined, is inconsistent between render engines, and doesn't make sense. - Subsurface Scale is clamped to [0..infinity]. - Behaviour with a negative radius is not well defined, is inconsistent between render engines, and doesn't make sense. - Subsurface Anisotropy is clamped to [0..0.9]. - Values outside this range don't render properly. - Subsurface IOR is clamped to [1.01..3.8]. - Values outside this range aren't supported. --- - Specular IOR Level is clamped to [0..infinity]. - Negative values are not supported. - Specular Tint is clamped to [0..infinity]. - Values below 0 typically doesn't do anything. - When Specular Tint to used on the metallic component, it is clamped to [0..1]. Values higher than 1 are not supported in Cycles, but are supported in EEVEE. This clamping is done to ensure consistency between render engines. - Anisotropic is clamped to [0..1]. - Values outside this range don't behave properly. - Anisotropic rotation is unclamped. --- - Transmission is clamped to [0..1]. - It's a mix between two closures. Values outside this range are not supported. --- - Coat weight is clamped to [0..infinity]. - ~~It behaves differently between EEVEE and Cycles in a major way when above 1.0. Clamping for consistency sake.~~ The issue mentioned here has been fixed. - Coat IOR is clamped to [1..infinity]. - Values below 1 are not supported. - Coat tint is clamped to [0..infinity]. - Kernel asserts are met when using negative values. --- - Sheen weight is clamped to [0..infinity]. - Due to the code layout, values below 0 are ignored. - Sheen tint is clamped to [0..infinity]. - Values below 0 are not supported in Cycles, but supported in EEVEE. So this clamp is to make sure they match. --- - Emission Color is unclamped. - Emission Strength is unclamped.
Alaska changed title from WIP: Shader: Clamp extra inputs in Principled BSDF to Shader: Only clamp undefined or unsupported inputs of Principled BSDF 2023-10-11 07:20:42 +02:00
Alaska added 1 commit 2023-10-11 08:16:26 +02:00

@LukasStockner can you review this?

@LukasStockner can you review this?
Author
Member

Coat weight clamping may change based on #113468

Coat weight clamping may change based on #113468
Member

Thanks for the extensive testing (and for implementing the result), this is extremely helpful.

All suggestions sound good to me. Negative values are always questionable (emission being the exception), weights that blend between components only make sense in [0..1], and many of the inputs that are used for lookup tables or numerical fits (e.g. roughness, subsurface anisotropy) just don't work outside of the range that they were designed for.

One thing that I'm not certain about is the logic for blending Base Color between the [0..inf] and [0..1] case - can't we just use the [0..1] version for Subsurface and Metallic while using the [0..inf] version for the others?

Also, you mention in the last comment that Specular Tint > 1 for metallic isn't supported, but in the table above it looks reasonable? What is the issue there?

Both of these issues seem to stem from the weight being negative, and the emission layer seems to react to this case differently from every other closure, resulting in the emission layer emitting negative light. This issue can be fixed with a simple change, giving us the ability to "safely" clamp Coat and Sheen weight, and Sheen Tint, to [0..infinity].
Simple change to fix this issue: a546e4779e (This is in my fork of Blender)
Note: The change is only for SVM since OSL already handled this case correctly. I haven't looked into EEVEE.

Ah, yes, good call. In OSL the layering code makes sure to not accidentally boost the base layer's intensity or make it negative. We should do the same in SVM, I'll do so tomorrow.

Coat weight clamping may change based on #113468

With that and the weight fix above, I think we should be able to go [0..inf] here as well.

Thanks for the extensive testing (and for implementing the result), this is extremely helpful. All suggestions sound good to me. Negative values are always questionable (emission being the exception), weights that blend between components only make sense in [0..1], and many of the inputs that are used for lookup tables or numerical fits (e.g. roughness, subsurface anisotropy) just don't work outside of the range that they were designed for. One thing that I'm not certain about is the logic for blending Base Color between the [0..inf] and [0..1] case - can't we just use the [0..1] version for Subsurface and Metallic while using the [0..inf] version for the others? Also, you mention in the last comment that Specular Tint > 1 for metallic isn't supported, but in the table above it looks reasonable? What is the issue there? > Both of these issues seem to stem from the weight being negative, and the emission layer seems to react to this case differently from every other closure, resulting in the emission layer emitting negative light. This issue can be fixed with a simple change, giving us the ability to "safely" clamp Coat and Sheen weight, and Sheen Tint, to [0..infinity]. Simple change to fix this issue: a546e4779e (This is in my fork of Blender) Note: The change is only for SVM since OSL already handled this case correctly. I haven't looked into EEVEE. Ah, yes, good call. In OSL the layering code makes sure to not accidentally boost the base layer's intensity or make it negative. We should do the same in SVM, I'll do so tomorrow. > Coat weight clamping may change based on #113468 With that and the weight fix above, I think we should be able to go [0..inf] here as well.
Lukas Stockner reviewed 2023-10-19 03:43:01 +02:00
Lukas Stockner left a comment
Member

General comment: I'd prefer to keep clamping explicit in the shader code, even if the closure's internals do it already. That way, we don't e.g. accidentally break compatibility if we change the closure's internals, and it's easier to ensure that things stay consistent. The performance impact should be negligible.

General comment: I'd prefer to keep clamping explicit in the shader code, even if the closure's internals do it already. That way, we don't e.g. accidentally break compatibility if we change the closure's internals, and it's easier to ensure that things stay consistent. The performance impact should be negligible.
@ -104,4 +117,2 @@
TransmissionBSDF = generalized_schlick_bsdf(
Normal, vector(0.0), color(1.0), sqrt(BaseColor), r2, r2, F0, F90, -eta, distribution),
BSDF = mix(BSDF, TransmissionBSDF, clamp(TransmissionWeight, 0.0, 1.0));
Member

Shouldn't this stay [0..1]?
Also, for code consistency, it might be nicer to introduce float transparency at the start of the shader and handle clamping there, like with metallic.

Shouldn't this stay [0..1]? Also, for code consistency, it might be nicer to introduce `float transparency` at the start of the shader and handle clamping there, like with `metallic`.
Author
Member

Part of the clamping comes from if TransmissionWeight > CLOSURE_WEIGHT_CUTOFF. However I have added back the explicit clamp for code clarity.

Part of the clamping comes from `if TransmissionWeight > CLOSURE_WEIGHT_CUTOFF`. However I have added back the explicit clamp for code clarity.
Alaska marked this conversation as resolved
@ -47,3 +61,3 @@
vector T = Tangent;
if (Anisotropic > 0.0) {
float aspect = sqrt(1.0 - clamp(Anisotropic, 0.0, 1.0) * 0.9);
float aspect = sqrt(1.0 - min(Anisotropic, 1.0) * 0.9);
Member

Shouldn't this stay [0..1]?

Shouldn't this stay [0..1]?
Author
Member

It's still technically clamped to [0..1] due to: if Anisotropic > 0.0 in the line above.

However, I have added back the explicit clamp for code clarity.

It's still technically clamped to [0..1] due to: `if Anisotropic > 0.0` in the line above. However, I have added back the explicit clamp for code clarity.
Alaska marked this conversation as resolved
@ -70,1 +81,3 @@
vector radius = SubsurfaceScale * SubsurfaceRadius;
BSDF = base_color * diffuse(Normal);
if (subsurface_weight > CLOSURE_WEIGHT_CUTOFF) {
vector radius = max(SubsurfaceScale, 0.0) * max(SubsurfaceRadius, vector(0.0));
Member

Nitpick, but for some weird corner cases, clamping the product instead of the individual factors might be nicer?

Nitpick, but for some weird corner cases, clamping the product instead of the individual factors might be nicer?
Alaska marked this conversation as resolved
@ -126,3 +139,3 @@
float cosNI = dot(I, CoatNormal);
float cosNT = sqrt(1.0 - coat_neta * coat_neta * (1 - cosNI * cosNI));
BSDF *= pow(CoatTint, CoatWeight / cosNT);
BSDF *= pow(max(CoatTint, color(0.0)), coat_weight / cosNT);
Member

As above - having all the clamping together at the start makes it more maintainable (except for cases where the same parameter gets clamped differently depending on context).

As above - having all the clamping together at the start makes it more maintainable (except for cases where the same parameter gets clamped differently depending on context).
Alaska marked this conversation as resolved
@ -137,3 +150,2 @@
if (SheenWeight > 1e-5) {
normal sheen_normal = normalize(mix(Normal, CoatNormal, clamp(CoatWeight, 0.0, 1.0)));
if (SheenWeight > CLOSURE_WEIGHT_CUTOFF) {
Member

Shouldn't this be [0..inf]?

Shouldn't this be [0..inf]?
Author
Member

Clamping to [0..infinity] is handled by if SheenWeight > CLOSURE_WEIGHT_CUTOFF

However I have added back explicit clamping for code clarity.

Clamping to [0..infinity] is handled by `if SheenWeight > CLOSURE_WEIGHT_CUTOFF` However I have added back explicit clamping for code clarity.
Alaska marked this conversation as resolved
@ -116,3 +116,1 @@
float anisotropic = saturatef(stack_load_float(stack, anisotropic_offset));
float sheen_weight = saturatef(stack_load_float(stack, sheen_weight_offset));
float3 sheen_tint = stack_load_float3(stack, sheen_tint_offset);
/* anisotropic is clamped to > 0 later on resulting in [0..1] clamping. */
Member

Might as well do it explicitly here I guess.

Might as well do it explicitly here I guess.
Alaska marked this conversation as resolved
@ -118,1 +116,3 @@
float3 sheen_tint = stack_load_float3(stack, sheen_tint_offset);
/* anisotropic is clamped to > 0 later on resulting in [0..1] clamping. */
float anisotropic = min(stack_load_float(stack, anisotropic_offset), 1.0f);
/* sheen_weight is clamped to > 0 later on. */
Member

I'm not sure where?

I'm not sure where?
Author
Member

Sheen weight is clamped to [0..infinity] with if (sheen_weight > CLOSURE_WEIGHT_CUTOFF) later on.

However I re-added the full clamping for code clarity.

Sheen weight is clamped to [0..infinity] with `if (sheen_weight > CLOSURE_WEIGHT_CUTOFF)` later on. However I re-added the full clamping for code clarity.
Alaska marked this conversation as resolved
@ -263,3 +273,3 @@
/* Emission (attenuated by sheen and coat) */
if (!is_zero(emission)) {
weight = max(weight, zero_spectrum());
Member

I'd rather make sure the weight doesn't become zero in the first place - I'll update you once I got that fix done.

I'd rather make sure the weight doesn't become zero in the first place - I'll update you once I got that fix done.
Alaska marked this conversation as resolved
Alaska added 2 commits 2023-10-19 06:19:39 +02:00
52b61b37f4 Address review
- Explicitly clamp things to make the code clearer
- Clamp the product of subsurface radius instead of individual components
- Group clamps in OSL script
Author
Member

One thing that I'm not certain about is the logic for blending Base Color between the [0..inf] and [0..1] case - can't we just use the [0..1] version for Subsurface and Metallic while using the [0..inf] version for the others?

My reason for blending between the unclamped and clamped Base Color based on the Subsurface/Metallic input is to avoid "sudden jumps" in some situations.

For example, if a user has a Base Color of 500, and they start increasing the Metallic input. Then Cycles will mix between a super reflective Diffuse material and a normal Metallic material. Due to the extreme reflectivity of the Diffuse material, throughout most of the Metallic Mix input range, there is no noticeable changes to the user, until the user gets very close to a Metallic input of 1, where sudden changes will be visible.

By mixing between the clamped and unclamped Base Color, we can make this transition more gradual. Which is a better user experience, but may not be what artists wants.

Just let me know if you would like the mix removed or not.

Note: Here's a video demonstrating this issue. The Base Color is 500. The sphere on the left is with my Base Color mixing. The sphere on the right is without any Base Color mixing.


Also, you mention in the last comment that Specular Tint > 1 for metallic isn't supported, but in the table above it looks reasonable? What is the issue there?

Edit: Cycles rendering completely "breaks" at high Specular Tint values (E.G. 70). When and how it "breaks" depends on the base colour, tint color, and roughness, and possibly other factors.

Note: The issue mentioned below can be fixed by adjusting some extra code.
Mainly for consistency between Cycles and EEVEE. When the Specular Tint is >1 in Cycles, the tinted area grows but reflections are limited to 100% reflectivity of the incoming light. With EEVEE, the tinted area grows, and reflectivity increases to 200%, 300%, 400%+ of the incoming light.


Coat weight clamping may change based on #113468

With that and the weight fix above, I think we should be able to go [0..inf] here as well.

I'll wait for your change before testing and committing it.

> One thing that I'm not certain about is the logic for blending Base Color between the [0..inf] and [0..1] case - can't we just use the [0..1] version for Subsurface and Metallic while using the [0..inf] version for the others? My reason for blending between the unclamped and clamped Base Color based on the Subsurface/Metallic input is to avoid "sudden jumps" in some situations. For example, if a user has a Base Color of 500, and they start increasing the `Metallic` input. Then Cycles will mix between a super reflective Diffuse material and a normal Metallic material. Due to the extreme reflectivity of the Diffuse material, throughout most of the `Metallic Mix` input range, there is no noticeable changes to the user, until the user gets very close to a Metallic input of 1, where sudden changes will be visible. By mixing between the clamped and unclamped Base Color, we can make this transition more gradual. Which is a better user experience, but may not be what artists wants. Just let me know if you would like the mix removed or not. Note: Here's a video demonstrating this issue. The Base Color is 500. The sphere on the left is with my Base Color mixing. The sphere on the right is without any Base Color mixing. <video src="/attachments/9a0bffa0-a1df-4cf2-a8b2-6e49d47a763b" title="Base color mixing comparison.mp4" controls></video> --- > Also, you mention in the last comment that Specular Tint > 1 for metallic isn't supported, but in the table above it looks reasonable? What is the issue there? Edit: Cycles rendering completely "breaks" at high Specular Tint values (E.G. 70). When and how it "breaks" depends on the base colour, tint color, and roughness, and possibly other factors. Note: The issue mentioned below can be fixed by adjusting some extra code. ~~Mainly for consistency between Cycles and EEVEE. When the Specular Tint is >1 in Cycles, the tinted area grows but reflections are limited to 100% reflectivity of the incoming light. With EEVEE, the tinted area grows, and reflectivity increases to 200%, 300%, 400%+ of the incoming light.~~ --- > > Coat weight clamping may change based on #113468 > > With that and the weight fix above, I think we should be able to go [0..inf] here as well. I'll wait for your change before testing and committing it.
Alaska added 2 commits 2023-10-19 06:40:12 +02:00
Author
Member

The latest revision of this pull request now includes the fix for emission weight from Lukas Stockner, and Coat weight has been adjusted to [0..infinity] clamping due to other changes

I have also applied a copy of the emission weight fix from Lukas Stockner to EEVEE since it had some minor issues in some cases.


There are still an open question:
Should we mix between the clamped and unclamped base color based on the metallic and subsufrace weight inputs? See previous comment.

Also:

  • All roughness values are clamped to [0..1].
    • Values below 0 are supported in Cycles, but not EEVEE. Values above 1 are supported sometimes supported in EEVEE and not Cycles.
    • If users want to use negative values, they can connect a "absolute" math node to their roughness. This will give you the exact same behaviour as Blender 3.6 which didn't clamp these values the same way.

Should versioning code be added to add an aboslute math node?
If a shader used negative roughness in 3.6, then applying versioning will make the 4.0 result in Cycles match the 3.6 result in Cycles. But it will completely change any EEVEE renders that used negative values.

Another option is to revert the roughness clamping change and make EEVEE match Cycles by using abs(roughness) with EEVEE.

The latest revision of this pull request now includes the fix for emission weight from Lukas Stockner, and Coat weight has been adjusted to [0..infinity] clamping due to other changes I have also applied a copy of the emission weight fix from Lukas Stockner to EEVEE since it had some minor issues in some cases. --- There are still an open question: Should we mix between the clamped and unclamped base color based on the metallic and subsufrace weight inputs? See previous comment. Also: > - All roughness values are clamped to [0..1]. > - Values below 0 are supported in Cycles, but not EEVEE. Values above 1 are supported sometimes supported in EEVEE and not Cycles. > - If users want to use negative values, they can connect a "absolute" math node to their roughness. This will give you the exact same behaviour as Blender 3.6 which didn't clamp these values the same way. Should versioning code be added to add an aboslute math node? If a shader used negative roughness in 3.6, then applying versioning will make the 4.0 result in Cycles match the 3.6 result in Cycles. But it will completely change any EEVEE renders that used negative values. Another option is to revert the roughness clamping change and make EEVEE match Cycles by using `abs(roughness)` with EEVEE.
Alaska added 4 commits 2023-10-20 02:30:53 +02:00
Alaska added 1 commit 2023-10-20 02:34:30 +02:00
Member

Should we mix between the clamped and unclamped base color based on the metallic and subsufrace weight inputs? See previous comment.

Honestly, I'd skip it. Setting Base Color over one is already in not-officially-supported territory, so I don't see the value in making the user experience with extremely weird values (e.g. 500) a bit smoother at the expense of more complexity in the code.

Should versioning code be added to add an aboslute math node?

Similar to above - negative roughness is weird enough that I wouldn't bother. The point of this PR is that we shouldn't actively block people from using weird settings if they want to, and that's very reasonable, but still - if you do it, you're on your own, don't expect compromises to make the experience smoother. Adding an extra node to all 3.x shader setups with variable roughness definitely qualifies as an unreasonable compromise IMHO.

> Should we mix between the clamped and unclamped base color based on the metallic and subsufrace weight inputs? See previous comment. Honestly, I'd skip it. Setting Base Color over one is already in not-officially-supported territory, so I don't see the value in making the user experience with extremely weird values (e.g. 500) a bit smoother at the expense of more complexity in the code. > Should versioning code be added to add an aboslute math node? Similar to above - negative roughness is weird enough that I wouldn't bother. The point of this PR is that we shouldn't actively block people from using weird settings if they want to, and that's very reasonable, but still - if you do it, you're on your own, don't expect compromises to make the experience smoother. Adding an extra node to all 3.x shader setups with variable roughness definitely qualifies as an unreasonable compromise IMHO.
Alaska added 4 commits 2023-10-23 14:04:26 +02:00
32888eeb7d Clamp transmission color
Turns out the transmission behaves unpredicatbly when they are rough and have high base colours
buildbot/vexp-code-patch-coordinator Build done. Details
533743bd26
Merge branch 'blender-v4.0-release' into clamp-tint
Author
Member

Should we mix between the clamped and unclamped base color based on the metallic and subsufrace weight inputs? See previous comment.

Honestly, I'd skip it. Setting Base Color over one is already in not-officially-supported territory, so I don't see the value in making the user experience with extremely weird values (e.g. 500) a bit smoother at the expense of more complexity in the code.

This has been removed in the latest commits. However, a modified version had to be kept for EEVEE since the diffuse and subsurface scattering materials are the same material in EEVEE.


Along with that, I also clamped transmission in the Principled BSDF. It renders black when using high base color values, with Multi-GGX, and medium roughness.

> > Should we mix between the clamped and unclamped base color based on the metallic and subsufrace weight inputs? See previous comment. > > Honestly, I'd skip it. Setting Base Color over one is already in not-officially-supported territory, so I don't see the value in making the user experience with extremely weird values (e.g. 500) a bit smoother at the expense of more complexity in the code. This has been removed in the latest commits. However, a modified version had to be kept for EEVEE since the diffuse and subsurface scattering materials are the same material in EEVEE. --- Along with that, I also clamped transmission in the Principled BSDF. It renders black when using high base color values, with Multi-GGX, and medium roughness.
First-time contributor

Along with that, I also clamped transmission in the Principled BSDF. It renders black when using high base color values, with Multi-GGX, and medium roughness.

Clamped to what range? Can we still raise it above 1?

> Along with that, I also clamped transmission in the Principled BSDF. It renders black when using high base color values, with Multi-GGX, and medium roughness. Clamped to what range? Can we still raise it above 1?
Author
Member

Along with that, I also clamped transmission in the Principled BSDF. It renders black when using high base color values, with Multi-GGX, and medium roughness.

Clamped to what range? Can we still raise it above 1?

I have clamped it to [0..1]. I haven't investigated it to see if there is a safe value above 1 that works with all settings combinations.

I would like input from Lukas Stocker and/or Brecht on if we should change the clamp to [0..5] or something like if I find certain values above 1 are "safe".

Also, the issue was only really present in EEVEE and Cycles with Multi-GGX with my limited testing. Should I leave it unclamped for GGX?

Same thing for Subsurface scattering. The issue only impacts Random Walk in Cycles. Should I leave it unclamped in EEVEE and unclamped with Christian Burley in Cycles?

> > Along with that, I also clamped transmission in the Principled BSDF. It renders black when using high base color values, with Multi-GGX, and medium roughness. > > Clamped to what range? Can we still raise it above 1? I have clamped it to [0..1]. I haven't investigated it to see if there is a safe value above 1 that works with all settings combinations. I would like input from Lukas Stocker and/or Brecht on if we should change the clamp to [0..5] or something like if I find certain values above 1 are "safe". Also, the issue was only really present in EEVEE and Cycles with Multi-GGX with my limited testing. Should I leave it unclamped for GGX? Same thing for Subsurface scattering. The issue only impacts Random Walk in Cycles. Should I leave it unclamped in EEVEE and unclamped with Christian Burley in Cycles?
Lukas Stockner approved these changes 2023-10-24 03:05:49 +02:00
Lukas Stockner left a comment
Member

Considering that the 4.0 release is coming closer, I'd say we stick with this for now.

We can always revisit this later and improve handling for extreme cases in 4.1.

Considering that the 4.0 release is coming closer, I'd say we stick with this for now. We can always revisit this later and improve handling for extreme cases in 4.1.
Brecht Van Lommel approved these changes 2023-10-24 12:38:17 +02:00

@blender-bot build

@blender-bot build
Lukas Stockner merged commit a15f9e49ec into blender-v4.0-release 2023-10-31 03:14:14 +01:00
Contributor
  • Clamped Base color to [0..1]
    • Avoid base colors that reflect more light than what's coming in.

: (

This breaks my shader (tuned in 3.6).

I think... If any input not computes to NaN, it should never be clamped. User has ability to clamp it by themselves if they want to be physcially correct.

> - Clamped Base color to [0..1] > - Avoid base colors that reflect more light than what's coming in. : ( This breaks my shader (tuned in 3.6). I think... If any input not computes to NaN, it should never be clamped. User has ability to clamp it by themselves if they want to be physcially correct.
Author
Member
  • Clamped Base color to [0..1]
    • Avoid base colors that reflect more light than what's coming in.

This breaks my shader (tuned in 3.6).

I think... If any input not computes to NaN, it should never be clamped. User has ability to clamp it by themselves if they want to be physcially correct.

Sorry for the late response. The base color was clamped to avoid rendering issues with the glass, metallic, and subsurface scattering components of the Principled BSDF. Rendering issues range from unexpected behaviour (E.G. Subsurface scattering renders darker as the base color increases above 1), to completely incorrect renders (E.G. The metallic component turns black to the camera with bright base colors, while still some times reflecting light on it's surroundings, creating a lot of fireflies).

A decision has to be made. Do we allow these values, that sometimes behave alright, but typically produce extremely incorrect results? Or do we clamp them to reasonable ranges? And the decision made was to clamp them to reasonable ranges, which does remove some artistic freedom/effects from some projects.

Maybe the decision on what to do will change again in the future.

> > - Clamped Base color to [0..1] > > - Avoid base colors that reflect more light than what's coming in. > > This breaks my shader (tuned in 3.6). > > I think... If any input not computes to NaN, it should never be clamped. User has ability to clamp it by themselves if they want to be physcially correct. Sorry for the late response. The base color was clamped to avoid rendering issues with the glass, metallic, and subsurface scattering components of the Principled BSDF. Rendering issues range from unexpected behaviour (E.G. Subsurface scattering renders darker as the base color increases above 1), to completely incorrect renders (E.G. The metallic component turns black to the camera with bright base colors, while still some times reflecting light on it's surroundings, creating a lot of fireflies). A decision has to be made. Do we allow these values, that sometimes behave alright, but typically produce extremely incorrect results? Or do we clamp them to reasonable ranges? And the decision made was to clamp them to reasonable ranges, which does remove some artistic freedom/effects from some projects. Maybe the decision on what to do will change again in the future.
Sign in to join this conversation.
No Label
Interest
Alembic
Interest
Animation & Rigging
Interest
Asset Browser
Interest
Asset Browser Project Overview
Interest
Audio
Interest
Automated Testing
Interest
Blender Asset Bundle
Interest
BlendFile
Interest
Collada
Interest
Compatibility
Interest
Compositing
Interest
Core
Interest
Cycles
Interest
Dependency Graph
Interest
Development Management
Interest
EEVEE
Interest
EEVEE & Viewport
Interest
Freestyle
Interest
Geometry Nodes
Interest
Grease Pencil
Interest
ID Management
Interest
Images & Movies
Interest
Import Export
Interest
Line Art
Interest
Masking
Interest
Metal
Interest
Modeling
Interest
Modifiers
Interest
Motion Tracking
Interest
Nodes & Physics
Interest
OpenGL
Interest
Overlay
Interest
Overrides
Interest
Performance
Interest
Physics
Interest
Pipeline, Assets & IO
Interest
Platforms, Builds & Tests
Interest
Python API
Interest
Render & Cycles
Interest
Render Pipeline
Interest
Sculpt, Paint & Texture
Interest
Text Editor
Interest
Translations
Interest
Triaging
Interest
Undo
Interest
USD
Interest
User Interface
Interest
UV Editing
Interest
VFX & Video
Interest
Video Sequencer
Interest
Virtual Reality
Interest
Vulkan
Interest
Wayland
Interest
Workbench
Interest: X11
Legacy
Blender 2.8 Project
Legacy
Milestone 1: Basic, Local Asset Browser
Legacy
OpenGL Error
Meta
Good First Issue
Meta
Papercut
Meta
Retrospective
Meta
Security
Module
Animation & Rigging
Module
Core
Module
Development Management
Module
EEVEE & Viewport
Module
Grease Pencil
Module
Modeling
Module
Nodes & Physics
Module
Pipeline, Assets & IO
Module
Platforms, Builds & Tests
Module
Python API
Module
Render & Cycles
Module
Sculpt, Paint & Texture
Module
Triaging
Module
User Interface
Module
VFX & Video
Platform
FreeBSD
Platform
Linux
Platform
macOS
Platform
Windows
Priority
High
Priority
Low
Priority
Normal
Priority
Unbreak Now!
Status
Archived
Status
Confirmed
Status
Duplicate
Status
Needs Info from Developers
Status
Needs Information from User
Status
Needs Triage
Status
Resolved
Type
Bug
Type
Design
Type
Known Issue
Type
Patch
Type
Report
Type
To Do
No Milestone
No project
No Assignees
5 Participants
Notifications
Due Date
The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: blender/blender#112895
No description provided.