Cycles: Rework Principled BSDF Emission #111155
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
4 Participants
Notifications
Due Date
No due date set.
Dependencies
No dependencies set.
Reference: blender/blender#111155
Loading…
Reference in New Issue
No description provided.
Delete Branch "LukasStockner/blender:emission-principled"
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?
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.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.
Otherwise looks fine to me.
The discrepancy in the principled emission alpha test is because there is a
IF_KERNEL_NODES_FEATURE(BSDF)
check insvm_node_closure_bsdf()
, therefore the Principled BSDF is skipped forsurface_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.
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:
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
Great, thanks!
Yeah, it's not expected to be pixel-perfect.
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 #110993Not sure how layering works.
I figured that when emission does not render,
closure_stack[0]
isOSL_CLOSURE_GenericEmissive_ID
, thenclosure_stack[1]
is assigned bylayer->base
which isnullptr
, the nextclosure
isOSL_CLOSURE_GeneralizedSchlickBSDF_ID
, after evaluating that it reads thenullptr
and returns.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
inosl.h
is "increased" to accommodate the diffuse component, but the diffuse closure isn't added to the stack, instead a NULL closure is, resulting inlayer(generalized_schlick_bsdf, NULL) + emission
and as a side effect of this, theflatten_closure_tree()
function inosl.h
exits after computing thegeneralized_schlick_bsdf
layer but before computing theemission
layer. Specificallyflatten_closure_tree()
loops over the layerswhile (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 ofosl.h
where it's setting uplayer(generalized_schlick_bsdf, BaseColor * diffuse)
. You can see that in theBaseColor(1.0, 1.0, 1.0)
case the base layer is a diffuse closure. In theBaseColor(0.0, 0.0, 0.0)
case the base layer is NULL.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.Yes, that sounds VERY plausible. Thanks for checking!
I guess the proper fix would be to restructure the loop here into (pseudocode):
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.
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.WIP: Cycles: Rework Principled BSDF Emissionto Cycles: Rework Principled BSDF EmissionOkay, I've submitted the fix as #112213. With that applied, the issue is solved from what I can tell.
Waiting for #112213.
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.