Shader: Clamp inputs of various nodes #120390

Merged

Brecht Van Lommel merged 1 commits from Alaska/blender:clamp-other-shaders into main 2024-04-26 17:39:52 +02:00

Conversation 11 Commits 1 Files Changed 7 +34 -26

Alaska commented 2024-04-08 09:05:01 +02:00

Member

Copy Link

Clamp some of the inputs of the Glossy BSDF, Glass BSDF, Sheen BSDF,
and Subsurface Scattering nodes to improve consistency
between render engines and to avoid unexpected results.
Some parameters are left unclamped for artistic uses,
but there is a chance they will break with abnormal values.

Glossy BSDF:

• Clamped roughness to 0..1
  • Negative values don’t make sense (and weren’t supported in EEVEE).
    Values above 1 are already handled gracefully, but I thought
    I would clamp it anyway.

Glass BSDF:

• Clamp roughness to 0..1
  • Values outside this range don’t make much sense.

Sheen BSDF:

• Clamped roughness to 0..1
  • Neither of these do anything as it was already handled
    somewhere else. This was just done for code consistency
    (E.G. Some of these existed in EEVEE and OSL, but not SVM).

Subsurface Scattering:

• Clamped radius to 0..inf
  • Negative radius doesn’t make sense.

And clamp the color input to 0..inf on all four.

Clamp some of the inputs of the Glossy BSDF, Glass BSDF, Sheen BSDF, and Subsurface Scattering nodes to improve consistency between render engines and to avoid unexpected results. Some parameters are left unclamped for artistic uses, but there is a chance they will break with abnormal values. Glossy BSDF: - Clamped roughness to 0..1 - Negative values don’t make sense (and weren’t supported in EEVEE). Values above 1 are already handled gracefully, but I thought I would clamp it anyway. Glass BSDF: - Clamp roughness to 0..1 - Values outside this range don’t make much sense. Sheen BSDF: - Clamped roughness to 0..1 - Neither of these do anything as it was already handled somewhere else. This was just done for code consistency (E.G. Some of these existed in EEVEE and OSL, but not SVM). Subsurface Scattering: - Clamped radius to 0..inf - Negative radius doesn’t make sense. And clamp the color input to 0..inf on all four.

🎉 2

Alaska added the

Module

Render & Cycles

label 2024-04-08 09:05:01 +02:00

Alaska added 4 commits 2024-04-08 09:05:10 +02:00

7516f0c66e Clamp Glossy BSDF ...

Clamp the color to 0..1
Clamp the roughness to 0..1

da119fadc0 Clamp inputs on Glass BSDF ...

- Clamp Color to 0..1
- Clamp roughness to 0..1

8612f45ac9 Clamp sheen BSDF ...

- Clamp color to 0..inf
- Clamp roughness to 0..1

0351d4558a Clamp subsurface scattering node ...

- Clamp color to 0..1
- Clamp radius to 0..inf

Alaska added this to the Render & Cycles project 2024-04-08 09:05:15 +02:00

Alaska commented 2024-04-08 09:11:07 +02:00

Author

Member

Copy Link

The clamped used here aligns with the clamping used in a15f9e49ec

I investigated this due to a report about the rendering issue with high brightness glossy BSDFs made here: #120349

---

There are concerns from some people that we are clamping some inputs unnecessarily, and removing the ability for some users to experiment with non-realistic values for artistic effects. I'm just bringing this point up for consideration.

I will note that some values don't need clamping some of the time. For example with the Glossy BSDF, Glass BSDF, and Subsurface Scattering node, the issues with colours brighter than 1 only occur in specific modes (Multiscatter GGX and Random Walk), while others are fine (E.G. GGX and Christen-Burley can handle these values just fine). So we can clamp the color input when using the modes that behave weirdly, but leave it unclamped on the modes that behave alright.

This approach wasn't taken with the Principled BSDF just for simplicity and consistency sake.

The clamped used here aligns with the clamping used in a15f9e49eca088605de756729ddb0e1424c617bc I investigated this due to a report about the rendering issue with high brightness glossy BSDFs made here: #120349 --- There are concerns from some people that we are clamping some inputs unnecessarily, and removing the ability for some users to experiment with non-realistic values for artistic effects. I'm just bringing this point up for consideration. I will note that some values don't need clamping some of the time. For example with the Glossy BSDF, Glass BSDF, and Subsurface Scattering node, the issues with colours brighter than 1 only occur in specific modes (Multiscatter GGX and Random Walk), while others are fine (E.G. GGX and Christen-Burley can handle these values just fine). So we can clamp the color input when using the modes that behave weirdly, but leave it unclamped on the modes that behave alright. This approach wasn't taken with the Principled BSDF just for simplicity and consistency sake.

Alaska requested review from Brecht Van Lommel 2024-04-08 09:11:18 +02:00

Alaska requested review from Lukas Stockner 2024-04-08 09:11:25 +02:00

Alaska requested review from Sergey Sharybin 2024-04-08 09:11:34 +02:00

fwfeet commented 2024-04-08 10:02:48 +02:00

First-time contributor

Copy Link

Shouldn't it affect NPR users?

Shouldn't it affect NPR users?

Alaska commented 2024-04-08 15:01:03 +02:00

Author

Member

Copy Link

Shouldn't it affect NPR users?

Sorry, it's not clear what your question is. So I'll provide information that hopefully helps you.

Some people will use unrealistic values (E.G. A base color with a brightness greater than 1) as inputs for certain nodes for artistic effects. This pull request will remove the ability to do that on some nodes. This is being done to avoid situations where the shader breaks or provides unexpected results with the weird input values.

A discussion needs to be had about "Should we remove the ability to create these artistic effects, just to avoid broken shaders in specific situations?" Or "Should we be more fine grained with the inputs we clamp? For example with the Glossy BSDF, it breaks with high brightness color inputs, only when using Multiscatter GGX. So should we clamp the input for Multiscatter GGX and leave GGX unclamped? So if users want to create unrealistic effects, they can with the right settings."

> Shouldn't it affect NPR users? Sorry, it's not clear what your question is. So I'll provide information that hopefully helps you. Some people will use unrealistic values (E.G. A base color with a brightness greater than 1) as inputs for certain nodes for artistic effects. This pull request will remove the ability to do that on some nodes. This is being done to avoid situations where the shader breaks or provides unexpected results with the weird input values. A discussion needs to be had about "Should we remove the ability to create these artistic effects, just to avoid broken shaders in specific situations?" Or "Should we be more fine grained with the inputs we clamp? For example with the Glossy BSDF, it breaks with high brightness color inputs, only when using Multiscatter GGX. So should we clamp the input for Multiscatter GGX and leave GGX unclamped? So if users want to create unrealistic effects, they can with the right settings."

❤️ 1

AgentA1cr commented 2024-04-08 19:32:25 +02:00

First-time contributor

Copy Link

As a user, I would ask that you keep clamping to a minimum. I would rather have more freedom, even if that means that I have the freedom to break things.

As a user, I would ask that you keep clamping to a minimum. I would rather have more freedom, even if that means that I have the freedom to break things.

Sergey Sharybin commented 2024-04-16 14:38:45 +02:00

Owner

Copy Link

@Alaska Are there some issues which those values can cause if unclamped, like nan pixels?
The thing is, with changes like this someone's render will start looking different, it will be perceived as a regression, and in own experience it creates more frustration than it brings solutions. So I'd be quite careful about introducing such changes.

@Alaska Are there some issues which those values can cause if unclamped, like `nan` pixels? The thing is, with changes like this someone's render will start looking different, it will be perceived as a regression, and in own experience it creates more frustration than it brings solutions. So I'd be quite careful about introducing such changes.

Alaska commented 2024-04-16 15:53:52 +02:00

Author

Member

Copy Link

@Alaska Are there some issues which those values can cause if unclamped, like nan pixels?

I don't believe any of them cause NaN pixels. But some do produce unexpected results.

Here are some examples:

---

Glossy BSDF - Color input:

Input Color	1.0	5.0	10.0

When large values are input into the Glossy BSDF color input, it produces extremely incorrect results (The black pixels are negative values). How large the value needs to be to cause this issue is dependent on the roughness. This same issue plagues the Glass BSDF.
This issue only occurs with Multiscatter GGX, so if we wanted too, we could only clamp this value when using Multiscatter GGX.

---

Subsurface scattering node - Color input:

Input Color	1.0	5.0	10.0

When large values are input into the Subsurface scattering node color input, it turns dark, while reflecting more indirect light (it falls back to diffuse for indirect light). This is an unexpected result and isn't useful for anyone. So just clamp it to avoid bug reports about it. This issue only plagues random walk and random walk skin, so maybe we could only do clamping on random walk?

This same effect can be achieved with a simple light path node setup.

---

The clamping of roughness to 0..1 is mainly to align with EEVEE (Cycles supports negative roughness due to sqr(roughness), while EEVEE didn't). These materials did not support roughnesses >1 anyway so clamping that is just to be "safe". If a user would like negative roughness back, they can add a absolute math node.

Clamping negative SSS scale is just to be safe. Negative scale doesn't appear to do anything.

And clamping the negative color Sheen was just to be safe. Negative color Sheen doesn't do anything.

---

Just a quick recap of changes and their impact to make things clear:

Glossy BSDF:

• Clamp color to 0..1
  • Fixes issues with Multiscatter GGX at high brightness (E.G. 10) with roughness.
  • Removes support for super reflective Glossy BSDF with GGX and other methods (Bad change, it is a regression)
  • Clamping the negative value does nothing in my testing and is just to be "safe".
• Clamped roughness to 0..1
  • Removes negative roughness support from Cycles, this aligns with EEVEE. The same effect can be reproduced with a single absolute math node.

Glass BSDF:

• Clamped color to 0..1
  • Mostly the same points as Glossy BSDF
  • Some negative colors could produce some interesting results in Cycles (not supported in EEVEE). We are losing that.
• Clamp roughness to 0..1
  • Same point as Glossy BSDF

Sheen BSDF:

• Clamped color to 0..inf
  • This is just to be safe. There is no change in renders.
• Clamped roughness to 0..1
  • This is just to be safe. There is no change in renders.

Subsurface Scattering:

• Clamped color to 0..1
  • Fixes "unexpected results" at values greater than 1 with Random walk.
  • Removes support for super bright scattering in Christain Burley. (Bad change, it is a regression)
• Clamped radius to 0..inf
  • This is just to be safe. There is no change in renders.

When I say there are no changes in scenes, I mean I couldn't find any in my testing.
Many of the "this change does nothing and is just to be safe" changes were made to align with the Principled BSDF component. The Principled BSDF needed some of these changes due to issues with the layering system around the middle of Blender 4.0 development.

> @Alaska Are there some issues which those values can cause if unclamped, like `nan` pixels? I don't believe any of them cause NaN pixels. But some do produce unexpected results. Here are some examples: --- Glossy BSDF - Color input: |Input Color|1.0|5.0|10.0| |-|-|-|-| ||||| When large values are input into the Glossy BSDF color input, it produces extremely incorrect results (The black pixels are negative values). How large the value needs to be to cause this issue is dependent on the roughness. This same issue plagues the Glass BSDF. This issue only occurs with Multiscatter GGX, so if we wanted too, we could only clamp this value when using Multiscatter GGX. --- Subsurface scattering node - Color input: |Input Color|1.0|5.0|10.0| |-|-|-|-| ||||| When large values are input into the Subsurface scattering node color input, it turns dark, while reflecting more indirect light (it falls back to diffuse for indirect light). This is an unexpected result and isn't useful for anyone. So just clamp it to avoid bug reports about it. This issue only plagues random walk and random walk skin, so maybe we could only do clamping on random walk? This same effect can be achieved with a simple light path node setup. --- The clamping of roughness to 0..1 is mainly to align with EEVEE (Cycles supports negative roughness due to `sqr(roughness)`, while EEVEE didn't). These materials did not support roughnesses >1 anyway so clamping that is just to be "safe". If a user would like negative roughness back, they can add a absolute math node. Clamping negative SSS scale is just to be safe. Negative scale doesn't appear to do anything. And clamping the negative color Sheen was just to be safe. Negative color Sheen doesn't do anything. --- Just a quick recap of changes and their impact to make things clear: Glossy BSDF: - Clamp color to 0..1 - Fixes issues with Multiscatter GGX at high brightness (E.G. 10) with roughness. - Removes support for super reflective Glossy BSDF with GGX and other methods (Bad change, it is a regression) - Clamping the negative value does nothing in my testing and is just to be "safe". - Clamped roughness to 0..1 - Removes negative roughness support from Cycles, this aligns with EEVEE. The same effect can be reproduced with a single absolute math node. Glass BSDF: - Clamped color to 0..1 - Mostly the same points as Glossy BSDF - Some negative colors could produce some interesting results in Cycles (not supported in EEVEE). We are losing that. - Clamp roughness to 0..1 - Same point as Glossy BSDF Sheen BSDF: - Clamped color to 0..inf - This is just to be safe. There is no change in renders. - Clamped roughness to 0..1 - This is just to be safe. There is no change in renders. Subsurface Scattering: - Clamped color to 0..1 - Fixes "unexpected results" at values greater than 1 with Random walk. - Removes support for super bright scattering in Christain Burley. (Bad change, it is a regression) - Clamped radius to 0..inf - This is just to be safe. There is no change in renders. When I say there are no changes in scenes, I mean I couldn't find any in my testing. Many of the "this change does nothing and is just to be safe" changes were made to align with the Principled BSDF component. The Principled BSDF needed some of these changes due to issues with the layering system around the middle of Blender 4.0 development.

Glossy 5.jpg

19 KiB

Glossy 1.jpg

18 KiB

SSS 1.jpg

21 KiB

SSS 10.jpg

35 KiB

SSS 5.jpg

29 KiB

Glossy 10.jpg

37 KiB

Brecht Van Lommel requested changes 2024-04-24 20:34:19 +02:00

Dismissed

Brecht Van Lommel left a comment

Owner

Copy Link

It would have been better to clamp everything from the beginning, but I'd rather not break compatibility with existing shaders much or making things inconvenient for NPR, even if they rely on undefined behavior.

I think clamping the roughness and radius is ok, but would rather leave colors completely unchanged.

The individual BSDF nodes are also a bit more low level than Principled BSDF and we don't need to be so strict I think.

It would have been better to clamp everything from the beginning, but I'd rather not break compatibility with existing shaders much or making things inconvenient for NPR, even if they rely on undefined behavior. I think clamping the roughness and radius is ok, but would rather leave colors completely unchanged. The individual BSDF nodes are also a bit more low level than Principled BSDF and we don't need to be so strict I think.

👍 1

Alaska added 6 commits 2024-04-25 03:30:44 +02:00

2fbefeca34 Merge branch 'main' into clamp-other-shaders

1d0add43dd Revert glossy colour clamp

a857afddc8 Revert glass colour clamp

60d519543f Revert sheen colour clamp

fdabf594ba Revert SSS colour clamp

e21310ebf9 Make format

Alaska added 1 commit 2024-04-25 03:33:30 +02:00

b7315a14a2 Fix missing changes

Alaska added 1 commit 2024-04-25 03:34:44 +02:00

c104f44ceb Fix missing changes

Sergey Sharybin commented 2024-04-26 10:59:43 +02:00

Owner

Copy Link

@blender-bot build

@blender-bot build

Brecht Van Lommel approved these changes 2024-04-26 15:27:00 +02:00

Brecht Van Lommel commented 2024-04-26 15:28:16 +02:00

Owner

Copy Link

Thanks for the changes.

It looks good, but there is a merge conflict to be resolved.

Thanks for the changes. It looks good, but there is a merge conflict to be resolved.

Alaska commented 2024-04-26 16:50:20 +02:00

Author

Member

Copy Link

I have rebased against main.
But I've also clamped Glossy BSDF, Glass BSDF, Sheen BSDF, Subsurface Scattering colour input to [0..inf].

They were already clamped like this in EEVEE, and partially clamped like this in SVM. So I'm just adding extra clamps to make everything consistent.

However as a side effect of this change, we lose negative coloured glass in Cycles (EEVEE didn't support it). Just say if you'd like this reverted.

I have rebased against main. But I've also clamped `Glossy BSDF`, `Glass BSDF`, `Sheen BSDF`, `Subsurface Scattering` colour input to [0..inf]. They were already clamped like this in EEVEE, and partially clamped like this in SVM. So I'm just adding extra clamps to make everything consistent. However as a side effect of this change, we lose negative coloured glass in Cycles (EEVEE didn't support it). Just say if you'd like this reverted.

Alaska requested review from Brecht Van Lommel 2024-04-26 16:50:29 +02:00

Alaska force-pushed clamp-other-shaders from c104f44ceb to a0cc68a220 2024-04-26 16:50:37 +02:00 Compare

Brecht Van Lommel commented 2024-04-26 17:12:14 +02:00

Owner

Copy Link

@blender-bot build

@blender-bot build

Brecht Van Lommel commented 2024-04-26 17:39:14 +02:00

Owner

Copy Link

Ok, let's do it like this.

Ok, let's do it like this.

Brecht Van Lommel merged commit afa66fc628 into main 2024-04-26 17:39:52 +02:00

Brecht Van Lommel referenced this issue from a commit 2024-04-26 17:39:53 +02:00

Shader: Clamp invalid inputs of various BSDF nodes

Brecht Van Lommel deleted branch clamp-other-shaders 2024-04-26 17:39:55 +02:00

Sign in to join this conversation.

Reviewers

No reviewers

Lukas Stockner

Sergey Sharybin

Brecht Van Lommel

Labels

Clear labels   

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

This issue affects/is about backward or forward 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

Issues relating to Wayland windowing support.

  

Interest

Workbench

  

Interest: X11

Issues relating to Xorg/X11 support.

  

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

Issues relating to security: https://wiki.blender.org/wiki/Process/Vulnerability_Reports

  

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 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

Milestone

Clear milestone

No items

No Milestone

Projects

Clear projects

No project

Render & Cycles

Assignees

Clear assignees

No Assignees

5 Participants

Notifications

Subscribe

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#120390

No description provided.