Cycles: Rework Principled BSDF Emission #111155

Merged

Lukas Stockner merged 7 commits from LukasStockner/blender:emission-principled into main

2023-09-13 03:05:35 +02:00

Lukas Stockner commented

2023-08-16 01:59:05 +02:00

Member

Changes defaults from Emission Color 0.0, Emission Strength 1.0 to be the other way around (Color 1.0, Strength 0.0), suggested by @brecht
Makes emission component occluded by sheen and coat (to simulate e.g. dust-covered light sources)
Moves transparency into the Principled SVM/OSL node, to allow for future support for e.g. transparent shadows in thin sheet mode.

Note that there are optimization opportunities here (mostly skipping the non-transparent components for transparent shadow evaluation, and skipping the parts that don't affect emission for light evaluation), but I have a separate point for those in the Principled V2 planning since there's some other optimization topics as well.

The reason why this is still WIP is that there's still a discrepancy in the principled emission alpha test that I haven't had the time to debug yet.

- Changes defaults from Emission Color 0.0, Emission Strength 1.0 to be the other way around (Color 1.0, Strength 0.0), suggested by @brecht - Makes emission component occluded by sheen and coat (to simulate e.g. dust-covered light sources) - Moves transparency into the Principled SVM/OSL node, to allow for future support for e.g. transparent shadows in thin sheet mode. Note that there are optimization opportunities here (mostly skipping the non-transparent components for transparent shadow evaluation, and skipping the parts that don't affect emission for light evaluation), but I have a separate point for those in the Principled V2 planning since there's some other optimization topics as well. The reason why this is still WIP is that there's still a discrepancy in the `principled emission alpha` test that I haven't had the time to debug yet.

Lukas Stockner added the

Render & Cycles

label 2023-08-16 01:59:05 +02:00

Lukas Stockner added 3 commits 2023-08-16 01:59:17 +02:00

8bc33c2e62 Cycles: Move Emission in Principled BSDF below Sheen and Coat

This way, sheen (e.g. a dust layer) and coat (e.g. a layer of paint) partially
obscure the emission and reduce its intensity.

In terms of implementation, this requires moving Emission, which previously
was expanded into a separate node, into the Principled node in SVM and OSL
so that we can compute the layering properly.

fd4ad794e2 Cycles: Also move transparency into Principled node internally

For now, the transparency (which was also expanded out) could have remained
separate, but in the future when we e.g. implement the thin sheet mode, it'll
be helpful to handle transparency from within the node.

Note that there's optimization potential here - for fully transparent surfaces
and when evaluating transparent shadows, we should skip evaluating the other
components. Same goes for evaluating emission during light sampling.

c54e867262 Versioning for rare corner case

Lukas Stockner requested review from Weizhen Huang 2023-08-16 01:59:25 +02:00

Lukas Stockner added this to the Render & Cycles project 2023-08-16 01:59:30 +02:00

Lukas Stockner referenced this pull request

2023-08-16 02:00:11 +02:00

Principled v2 BSDF #99447

Brecht Van Lommel commented

2023-08-16 10:32:47 +02:00

Owner

Note that there are optimization opportunities here (mostly skipping the non-transparent components for transparent shadow evaluation, and skipping the parts that don't affect emission for light evaluation), but I have a separate point for those in the Principled V2 planning since there's some other optimization topics as well.

I'd rather have this alpha change committed when there is an optimization for it, since we don't need it yet. I would like to avoid a performance regression here, and the solution for this to me seems non-trivial so could take a while to do.

> Note that there are optimization opportunities here (mostly skipping the non-transparent components for transparent shadow evaluation, and skipping the parts that don't affect emission for light evaluation), but I have a separate point for those in the Principled V2 planning since there's some other optimization topics as well. I'd rather have this alpha change committed when there is an optimization for it, since we don't need it yet. I would like to avoid a performance regression here, and the solution for this to me seems non-trivial so could take a while to do.

Lukas Stockner commented

2023-08-16 11:39:02 +02:00

Author

Member

I'd rather have this alpha change committed when there is an optimization for it, since we don't need it yet. I would like to avoid a performance regression here, and the solution for this to me seems non-trivial so could take a while to do.

Makes sense, I wasn't sure either so I've made it a separate commit. We can just skip it for now.

> I'd rather have this alpha change committed when there is an optimization for it, since we don't need it yet. I would like to avoid a performance regression here, and the solution for this to me seems non-trivial so could take a while to do. Makes sense, I wasn't sure either so I've made it a separate commit. We can just skip it for now.

Brecht Van Lommel reviewed 2023-08-16 12:13:18 +02:00

Brecht Van Lommel left a comment

Owner

Otherwise looks fine to me.

Otherwise looks fine to me.

Weizhen Huang added 1 commit 2023-08-16 16:34:17 +02:00

8813bb0d4a Fix discrepancy in principled emission alpha test

surface BSDF was skipped when sampling light

Weizhen Huang commented

2023-08-16 16:34:39 +02:00

Member

The discrepancy in the principled emission alpha test is because there is a IF_KERNEL_NODES_FEATURE(BSDF) check in svm_node_closure_bsdf(), therefore the Principled BSDF is skipped for surface_shader_eval<KERNEL_FEATURE_NODE_MASK_SURFACE_LIGHT>. I committed a fix.
The test now passes, there is still small discrepancy, I assume this is expected because there is clearcoat on top of it? It already looks a bit different now in main.
There is another problem I don't know how to fix: if the base color is set to black, the surface appears completely black in OSL even when there is emission.

The discrepancy in the principled emission alpha test is because there is a `IF_KERNEL_NODES_FEATURE(BSDF)` check in `svm_node_closure_bsdf()`, therefore the Principled BSDF is skipped for `surface_shader_eval<KERNEL_FEATURE_NODE_MASK_SURFACE_LIGHT>`. I committed a fix. The test now passes, there is still small discrepancy, I assume this is expected because there is clearcoat on top of it? It already looks a bit different now in main. There is another problem I don't know how to fix: if the base color is set to black, the surface appears completely black in OSL even when there is emission.

Alaska commented

2023-08-16 17:47:06 +02:00

Member

if the base color is set to black, the surface appears completely black in OSL even when there is emission.

I just thought I would expand on this. This issue seems to be related to specular layer on top of the diffuse component. (Line 77 - 80 of node_principled_bsdf.osl)

This part:

	    BSDF = layer(
        generalized_schlick_bsdf(
            Normal, T, color(1.0), color(0.0), alpha_x, alpha_y, f0, f90, -IOR, distribution),
        BSDF);

If the IOR of the Principled BSDF is a small value (E.G. 0.05) or a large value (7.0) while Specular is at 1.0, then the emission will render.
Or if the layer is removed (or skipped by setting Metallic or Transmission[1] to 1.0), Then the emission will render.

This suggests something goes wrong when the specular layer with certain properties is applied on top of the diffuse component, and then the emission component is added.

I didn't do much other investigating. So I don't know if it's an issue with layer() or if it's an issue with something interacting with the layer incorrectly.

[1] Transmission does not render properly with OSL. This has been fixed in the main branch of Blender 71587663ac

> if the base color is set to black, the surface appears completely black in OSL even when there is emission. I just thought I would expand on this. This issue seems to be related to specular layer on top of the diffuse component. (Line 77 - 80 of `node_principled_bsdf.osl`) This part: ``` BSDF = layer( generalized_schlick_bsdf( Normal, T, color(1.0), color(0.0), alpha_x, alpha_y, f0, f90, -IOR, distribution), BSDF); ``` If the IOR of the Principled BSDF is a small value (E.G. 0.05) or a large value (7.0) while Specular is at 1.0, then the emission will render. Or if the layer is removed (or skipped by setting Metallic or Transmission[1] to 1.0), Then the emission will render. This suggests something goes wrong when the specular layer with certain properties is applied on top of the diffuse component, and then the emission component is added. I didn't do much other investigating. So I don't know if it's an issue with layer() or if it's an issue with something interacting with the layer incorrectly. --- [1] Transmission does not render properly with OSL. This has been fixed in the main branch of Blender 71587663ac0b3811f5000226f7c470f4342befec

Lukas Stockner commented

2023-08-16 17:53:19 +02:00

Author

Member

I committed a fix.

Great, thanks!

The test now passes, there is still small discrepancy, I assume this is expected because there is clearcoat on top of it? It already looks a bit different now in main.

Yeah, it's not expected to be pixel-perfect.

There is another problem I don't know how to fix: if the base color is set to black, the surface appears completely black in OSL even when there is emission.

Based on the info from @Alaska that it's layering-related, it might be the same problem as the "blue=0" problem in the Coat PR. Maybe try cherry-picking the safe_divide_color fix from there into this branch?

> I committed a fix. Great, thanks! > The test now passes, there is still small discrepancy, I assume this is expected because there is clearcoat on top of it? It already looks a bit different now in main. Yeah, it's not expected to be pixel-perfect. > There is another problem I don't know how to fix: if the base color is set to black, the surface appears completely black in OSL even when there is emission. Based on the info from @Alaska that it's layering-related, it might be the same problem as the "blue=0" problem in the Coat PR. Maybe try cherry-picking the `safe_divide_color` fix from there into this branch?

Alaska commented

2023-08-16 18:12:12 +02:00

Member

There is another problem I don't know how to fix: if the base color is set to black, the surface appears completely black in OSL even when there is emission.

Based on the info from @Alaska that it's layering-related, it might be the same problem as the "blue=0" problem in the Coat PR. Maybe try cherry-picking the safe_divide_color fix from there into this branch?

Cherry picking that commit [1] didn't fix the issue for me. Unless I did something wrong, this will probably need further investigation.

[1] c5219c9cd5 from #110993

> > There is another problem I don't know how to fix: if the base color is set to black, the surface appears completely black in OSL even when there is emission. > > Based on the info from @Alaska that it's layering-related, it might be the same problem as the "blue=0" problem in the Coat PR. Maybe try cherry-picking the `safe_divide_color` fix from there into this branch? Cherry picking that commit [1] didn't fix the issue for me. Unless I did something wrong, this will probably need further investigation. [1] https://projects.blender.org/blender/blender/commit/c5219c9cd54c4dd13c8746701c4fe4395629facd from #110993

Weizhen Huang commented

2023-08-16 18:34:58 +02:00

Member

Not sure how layering works.
I figured that when emission does not render, closure_stack[0] is OSL_CLOSURE_GenericEmissive_ID, then closure_stack[1] is assigned by layer->base which is nullptr, the next closure is OSL_CLOSURE_GeneralizedSchlickBSDF_ID, after evaluating that it reads the nullptr and returns.

Not sure how layering works. I figured that when emission does not render, `closure_stack[0]` is `OSL_CLOSURE_GenericEmissive_ID`, then `closure_stack[1]` is assigned by `layer->base` which is `nullptr`, the next `closure` is `OSL_CLOSURE_GeneralizedSchlickBSDF_ID`, after evaluating that it reads the `nullptr` and returns.

Alaska commented

2023-08-17 01:59:14 +02:00

Member

Edit: Turns out this is basically exactly what Weizhen Huang said. Sorry for repeating.

Before I start, I just wanted to explain that these findings are from debugging the flatten_closure_tree() function in /intern/cycles/kernel/osl/osl.h. And so everything I talk about (increased closure_stack, the closure isn't added, etc) all happens in that function.

I might of debugged this wrong, but this appears to be what happens, or at least it's a part of what's happening.

The issue occurs when this is run in OSL and the base colour is RGB(0, 0, 0) layer(generalized_schlick_bsdf, BaseColor * diffuse) + emission
When the base colour is 0, 0, 0, the closure_stack in osl.h is "increased" to accommodate the diffuse component, but the diffuse closure isn't added to the stack, instead a NULL closure is, resulting in layer(generalized_schlick_bsdf, NULL) + emission and as a side effect of this, the flatten_closure_tree() function in osl.h exits after computing the generalized_schlick_bsdf layer but before computing the emission layer. Specifically flatten_closure_tree() loops over the layers while (closure) and if the closure is NULL, it exits.

The reason why the emission works again when the IOR is changes to low or high values is because the NULL diffuse part is skipped since it is "fully occluded" by the specular layer meaning we don't process the NULL diffuse closure and thus don't exit early.

I don't know the best way to fix this issue, but I hope the added information helps someone else in fixing the issue.

Here's some screenshots from debugging in Xcode from within /intern/cycles/kernel/osl/osl.h. Note: The break point is at line 113 of osl.h where it's setting up layer(generalized_schlick_bsdf, BaseColor * diffuse). You can see that in the BaseColor(1.0, 1.0, 1.0) case the base layer is a diffuse closure. In the BaseColor(0.0, 0.0, 0.0) case the base layer is NULL.

BaseColor(1.0, 1.0, 1.0) - Working	BaseColor(0.0, 0.0, 0.0) - Broken

Edit: This is a quick hack to avoid putting NULL closures into the stack in the specific case that we're experiencing issues with, and it fixes this specific issue (may not fix issues in some custom OSL scripts). It's probably better to go with the approach Lukas Stockner described below. But I just wanted to verify that osl.h was the only place where issues occurred, so I hacked this in to test it.

diff --git a/intern/cycles/kernel/osl/osl.h b/intern/cycles/kernel/osl/osl.h
index 945cbce8fbe..2f3ad60e91e 100644
--- a/intern/cycles/kernel/osl/osl.h
+++ b/intern/cycles/kernel/osl/osl.h
@@ -104,11 +104,13 @@ ccl_device void flatten_closure_tree(KernelGlobals kg,
         kernel_assert(layer_stack_level == -1);
 
         /* Push base layer onto the stack, will be handled after the top layers */
-        weight_stack[stack_size] = weight;
-        closure_stack[stack_size] = layer->base;
-        /* Start accumulating albedo of the top layers */
-        layer_stack_level = stack_size++;
-        layer_albedo = zero_float3();
+          if (layer->base != nullptr) {
+              weight_stack[stack_size] = weight;
+              closure_stack[stack_size] = layer->base;
+              /* Start accumulating albedo of the top layers */
+              layer_stack_level = stack_size++;
+              layer_albedo = zero_float3();
+          }
         /* Continue with the top layers */
         closure = layer->top;
         continue;

Edit: Turns out this is basically exactly what Weizhen Huang said. Sorry for repeating. Before I start, I just wanted to explain that these findings are from debugging the `flatten_closure_tree()` function in `/intern/cycles/kernel/osl/osl.h`. And so everything I talk about (increased closure_stack, the closure isn't added, etc) all happens in that function. I might of debugged this wrong, but this appears to be what happens, or at least it's a part of what's happening. The issue occurs when this is run in OSL and the base colour is RGB(0, 0, 0) `layer(generalized_schlick_bsdf, BaseColor * diffuse) + emission` When the base colour is 0, 0, 0, the `closure_stack` in `osl.h` is "increased" to accommodate the diffuse component, but the diffuse closure isn't added to the stack, instead a NULL closure is, resulting in `layer(generalized_schlick_bsdf, NULL) + emission` and as a side effect of this, the `flatten_closure_tree()` function in `osl.h` exits after computing the `generalized_schlick_bsdf` layer but before computing the `emission` layer. Specifically `flatten_closure_tree()` loops over the layers `while (closure)` and if the closure is NULL, it exits. The reason why the emission works again when the IOR is changes to low or high values is because the NULL diffuse part is skipped since it is "fully occluded" by the specular layer meaning we don't process the NULL diffuse closure and thus don't exit early. I don't know the best way to fix this issue, but I hope the added information helps someone else in fixing the issue. Here's some screenshots from debugging in Xcode from within `/intern/cycles/kernel/osl/osl.h`. Note: The break point is at line 113 of `osl.h` where it's setting up `layer(generalized_schlick_bsdf, BaseColor * diffuse)`. You can see that in the `BaseColor(1.0, 1.0, 1.0)` case the base layer is a diffuse closure. In the `BaseColor(0.0, 0.0, 0.0)` case the base layer is NULL. | BaseColor(1.0, 1.0, 1.0) - Working | BaseColor(0.0, 0.0, 0.0) - Broken | | - | - | | ![BaseColor(1.0, 1.0, 1.0).png](/attachments/404bc9c3-049e-4f55-8b5c-f5fc717c78b1) | ![BaseColor(0.0, 0.0, 0.0).png](/attachments/505186d0-f0dd-4576-8b35-fa56a111ff93) | --- Edit: This is a quick hack to avoid putting NULL closures into the stack in the specific case that we're experiencing issues with, and it fixes this specific issue (may not fix issues in some custom OSL scripts). It's probably better to go with the approach Lukas Stockner described below. But I just wanted to verify that `osl.h` was the only place where issues occurred, so I hacked this in to test it. ``` diff --git a/intern/cycles/kernel/osl/osl.h b/intern/cycles/kernel/osl/osl.h index 945cbce8fbe..2f3ad60e91e 100644 --- a/intern/cycles/kernel/osl/osl.h +++ b/intern/cycles/kernel/osl/osl.h @@ -104,11 +104,13 @@ ccl_device void flatten_closure_tree(KernelGlobals kg, kernel_assert(layer_stack_level == -1); /* Push base layer onto the stack, will be handled after the top layers */ - weight_stack[stack_size] = weight; - closure_stack[stack_size] = layer->base; - /* Start accumulating albedo of the top layers */ - layer_stack_level = stack_size++; - layer_albedo = zero_float3(); + if (layer->base != nullptr) { + weight_stack[stack_size] = weight; + closure_stack[stack_size] = layer->base; + /* Start accumulating albedo of the top layers */ + layer_stack_level = stack_size++; + layer_albedo = zero_float3(); + } /* Continue with the top layers */ closure = layer->top; continue; ```

BaseColor(0.0, 0.0, 0.0).png

447 KiB

BaseColor(1.0, 1.0, 1.0).png

469 KiB

Lukas Stockner commented

2023-08-17 02:12:35 +02:00

Author

Member

Yes, that sounds VERY plausible. Thanks for checking!

I guess the proper fix would be to restructure the loop here into (pseudocode):

while True:
  handle closure
  do:
    if stack is empty: return
    pop closure and weight from stack
    if layering: adjust weight
  while closure == nullptr or weight == 0

This is both cleaner than the current code (avoids the extra nested pop in the layer case) and should avoid the issue as far as I can tell.

Alternatively, change the cods to only push non-NULL closures onto the stack.

Yes, that sounds VERY plausible. Thanks for checking! I guess the proper fix would be to restructure the loop here into (pseudocode): ``` while True: handle closure do: if stack is empty: return pop closure and weight from stack if layering: adjust weight while closure == nullptr or weight == 0 ``` This is both cleaner than the current code (avoids the extra nested pop in the layer case) and should avoid the issue as far as I can tell. Alternatively, change the cods to only push non-NULL closures onto the stack.

Weizhen Huang approved these changes 2023-08-17 11:25:10 +02:00

Weizhen Huang left a comment

Member

Since the aforementioned bug is not introduced by this patch, the current state looks good to me.
Side note: currently the maximum number of closures for Principled BSDF specified in shader_graph.cpp is 8, after all these changes we should probably recalculate the number.

Since the aforementioned bug is not introduced by this patch, the current state looks good to me. Side note: currently the maximum number of closures for Principled BSDF specified in `shader_graph.cpp` is 8, after all these changes we should probably recalculate the number.

Weizhen Huang referenced this pull request

2023-08-31 22:59:42 +02:00

EEVEE: change Principled BSDF to match Cycles #111754

Lukas Stockner added 1 commit 2023-09-11 04:27:04 +02:00

07cc2a12d0 Merge branch 'main' into emission-principled

Lukas Stockner changed title from ~~WIP: Cycles: Rework Principled BSDF Emission~~ to Cycles: Rework Principled BSDF Emission

2023-09-11 04:29:25 +02:00

Lukas Stockner referenced this pull request

2023-09-11 04:32:40 +02:00

Cycles: Fix OSL potentially skipping closures when using layering #112213

Lukas Stockner commented

2023-09-11 04:33:39 +02:00

Author

Member

Okay, I've submitted the fix as #112213. With that applied, the issue is solved from what I can tell.

Okay, I've submitted the fix as #112213. With that applied, the issue is solved from what I can tell.

Brecht Van Lommel approved these changes 2023-09-11 18:08:27 +02:00

Lukas Stockner added 1 commit 2023-09-13 01:54:13 +02:00

4edf97ffd1 Merge remote-tracking branch 'origin/main' into emission-principled

Lukas Stockner commented

2023-09-13 02:03:49 +02:00

Author

Member

Waiting for #112213.

Waiting for #112213.

Lukas Stockner added 1 commit 2023-09-13 03:03:27 +02:00

dcb7b4e1f1 Merge branch 'main' into emission-principled

Lukas Stockner commented

2023-09-13 03:04:18 +02:00

Author

Member

Actually, as @weizhen said, the bug is not caused by this PR so I don't think it's a blocker. The fix is fairly simple anyways, so it won't be open for long.

Actually, as @weizhen said, the bug is not caused by this PR so I don't think it's a blocker. The fix is fairly simple anyways, so it won't be open for long.

Lukas Stockner merged commit 4c229070a9 into main

2023-09-13 03:05:35 +02:00

Lukas Stockner deleted branch emission-principled

2023-09-13 03:05:36 +02:00

Lukas Stockner referenced this issue from a commit

2023-09-13 03:05:36 +02:00

Cycles: Rework Principled BSDF Emission

Brecht Van Lommel referenced this issue from a commit

2023-12-05 01:37:13 +01:00

Rework principled bsdf emission

Sign in to join this conversation.

No reviewers

No Label

Animation & Rigging

Asset Browser Project Overview

Automated Testing

Blender Asset Bundle

Dependency Graph

Development Management

EEVEE & Viewport

Images & Movies

Motion Tracking

Nodes & Physics

Pipeline, Assets & IO

Platforms, Builds & Tests

Render & Cycles

Render Pipeline

Sculpt, Paint & Texture

Video Sequencer

Virtual Reality

Blender 2.8 Project

Milestone 1: Basic, Local Asset Browser

Good First Issue

Animation & Rigging

Development Management

EEVEE & Viewport

Nodes & Physics

Pipeline, Assets & IO

Platforms, Builds & Tests

Render & Cycles

Sculpt, Paint & Texture

Needs Info from Developers

Needs Information from User

No Milestone

No project

No Assignees

4 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#111155

No description provided.