Shader: Only clamp undefined or unsupported inputs of Principled BSDF #112895
No reviewers
Labels
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
No due date set.
Dependencies
No dependencies set.
Reference: blender/blender#112895
Loading…
Reference in New Issue
No description provided.
Delete Branch "Alaska/blender:clamp-tint"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
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.
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?
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.
I should of clarified. Clamping Emission colour with
max(emission_color, 0.0)
so we don't get negative values.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.
Shader: Clamp tint properties in Principled BSDFto Shader: Clamp tint properties in Principled BSDF@blender-bot build linux
47a1e5b3f9
to833409cd06
Shader: Clamp tint properties in Principled BSDFto Shader: Clamp color inputs in Principled BSDFMostly 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));
I think this should be
saturate(base_color)
.Shader: Clamp color inputs in Principled BSDFto Shader: Clamp extra inputs in Principled BSDFI 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?
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.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 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.
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].
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].
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?
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].
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].
Metallic - It's a mix between closures. Clamp to [0..1].
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] insidebsdf_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)
IOR - Clamp to [0..Infinity].
Note: EEVEE doesn't support the negative alpha case.
My recommendation: Clamp Alpha to [0..1].
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.My recommendation: Clamp Specular IOR Level to [0..infinity].
f0
with negative values and theior_from_F0()
function doesn't support negativef0
values).Note: It seems like negative values are not supported. Probably clamped somewhere inside closures.
My recommendation: Clamp Specular Tint to [0..infinity].
Continued in next comment. I reached the upload limit.
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?
Note: EEVEE behaves differently when using negative values. Specifically showing negative reflections.
My recommendation: Clamp to [0..infinity].
My recommendation: Clamp to [0..1].
Ansiotropic Rotation - Don't clamp. All values are supported due to
cosf(angle)
andsinf(angle)
being repeating functions.Transmission Weight - It's a mix between closures, clamp to [0..1]
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].
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.
Note: EEVEE behaves differently than Cycles if only some of the channels are below 0.0.
My recommendation: Clamp Coat Tint to [0..infinity].
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].
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].
Emission color and emission strength - Don't clamp these values. Negative values can be used for artistic effects, and it supports all positive values.
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.
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.
Shader: Clamp extra inputs in Principled BSDFto WIP: Shader: Clamp extra inputs in Principled BSDFb8c556b65c
to58442a420e
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:
A mix is applied to try and achieve a gradual transition from the unclamped materials (Diffuse) to the clamped materials (E.G. Metal).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
)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.WIP: Shader: Clamp extra inputs in Principled BSDFto Shader: Only clamp undefined or unsupported inputs of Principled BSDF@LukasStockner can you review this?
Coat weight clamping may change based on #113468
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?
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.
With that and the weight fix above, I think we should be able to go [0..inf] here as well.
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));
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 withmetallic
.Part of the clamping comes from
if TransmissionWeight > CLOSURE_WEIGHT_CUTOFF
. However I have added back the explicit clamp for code clarity.@ -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);
Shouldn't this stay [0..1]?
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.
@ -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));
Nitpick, but for some weird corner cases, clamping the product instead of the individual factors might be nicer?
@ -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);
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).
@ -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) {
Shouldn't this be [0..inf]?
Clamping to [0..infinity] is handled by
if SheenWeight > CLOSURE_WEIGHT_CUTOFF
However I have added back explicit clamping for code clarity.
@ -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. */
Might as well do it explicitly here I guess.
@ -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. */
I'm not sure where?
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.
@ -263,3 +273,3 @@
/* Emission (attenuated by sheen and coat) */
if (!is_zero(emission)) {
weight = max(weight, zero_spectrum());
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.
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 theMetallic 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.
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.I'll wait for your change before testing and committing it.
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:
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.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.
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.
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.
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?
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.
@blender-bot build
: (
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.