Cycles Disney BRDF
ClosedPublic

Authored by Pascal Schön (VanCantus) on Oct 20 2016, 11:22 AM.
Tags
Tokens
"Love" token, awarded by cgvirus."Love" token, awarded by robocyte."Love" token, awarded by poor."Love" token, awarded by plasmasolutions."Love" token, awarded by dfelinto."Love" token, awarded by danthedeckie.

Details

Summary

This patch implements the Disney BSDF directly in the Cycles renderer. The code is based on the 2015 version of the Disney shader.

Diff Detail

Repository
rB Blender
There are a very large number of changes, so older changes are hidden. Show Older Changes
intern/cycles/kernel/closure/bsdf_microfacet_multi_impl.h
86–96

How should I handle it then? Should I try to merge the D2003 code parts into my code or is it better to keep it mostly and wait until it's committed?

Pascal Schön (VanCantus) marked 12 inline comments as done.Oct 26 2016, 9:02 AM
Pascal Schön (VanCantus) removed rB Blender as the repository for this revision.
  • Rearranged the inputs of the shader node
  • Added "Subsurface Radius" parameter to control the per color channel radius of the subsurface scattering
  • Fixed the issue with artifacts when using anisotropy without linking the tangent input to a tangent node
  • Removed the color parameters from the diffuse and sheen shader and use them as closure weights instead
  • Switched from uniform to cosine hemisphere sampling for the diffuse and sheen part
  • Removed default values in setup functions and added extra functions for GGX with fresnel instead
  • Fixed a bug when using glossy reflections/refractions with low roughness values
  • Cleanup of the code

Thanks for the updates. I don't have time to review it this weekend, but from a quick look it seems to be getting close to ready.

intern/cycles/kernel/closure/bsdf_microfacet_multi_impl.h
86–96

The fresnel code is actually there already, but it only starts getting used in D2003. So it should be possible to merge this somehow already without conflicts later on.

Mainly code style comments here, so it's not really critical to implement all of them right away.

On a general note, I don't really like the variable names cspec0 and csheen0. In small code parts, "technical" variable names that are references to some mathematical operation (like NdotL or val_sqr) are reasonable, but when they're used all over the place a more descriptive name would be nice.

intern/cycles/kernel/closure/bsdf_disney_diffuse.h
51 ↗(On Diff #7699)

Is there a reason to not just use M_1_PI_F * NdotL * Fd here?

76 ↗(On Diff #7699)

I'd move those to one line as well and get rid of cos_pi.

114 ↗(On Diff #7699)

Picky, but 0.0f would be nicer for float.
Same in other places in the patch actually, but it doesn't matter that much.

intern/cycles/kernel/closure/bsdf_disney_sheen.h
49 ↗(On Diff #7699)

As above, maybe just do float value = schlick_fresnel(LdotH) * NdotL?

72 ↗(On Diff #7699)

Same as above, cos_pi isn't really needed.

intern/cycles/kernel/closure/bsdf_microfacet.h
247

This would be nicer if there was just one if(bsdf->extra && bsdf->extra->use_fresnel)

531

Picky: This empty line is diff noise.

583

Same as above, one if would be nicer.

607

Also here.

711

Again just an empty line.

intern/cycles/kernel/closure/bsdf_microfacet_multi_impl.h
32

There seems to be some intendation noise here, I'm not sure which one is the correct one.

86

I didn't check every single detail, but so far it looks like you could just fold the extra terms into eval instead of having both eval and eval_fresnel, since you only return one of them anyways? That'd also allow to get rid of some duplicated computations (mainly mf_G1) further down in the code.
Same for throughput and throughput_fresnel.

intern/cycles/kernel/closure/bsdf_util.h
134

This function seems unused?

intern/cycles/kernel/shaders/node_disney_bsdf.osl
51 ↗(On Diff #7699)

If you use the accurate function above, you should also use those components here.

intern/cycles/kernel/svm/svm_closure.h
131

More descriptive variable names would be nice here, clearcoat_normal tells a lot more than CN.

150

Brackets around single statements are usually preferred in Cycles code to avoid potential merge conflicts in the future.

155

As above, a single if might be nicer.

242

You should probably use linear_rgb_to_gray here.

282

Same as above.

308

Automatically switching between MultiGGX and GGX is nice from a usability point of view, but might give visible borders when using textued roughness.
I don't really mind, but I think it's worth noting.

327

In theory, you could move all the calculations above into that if since they're useless if the if isn't executed. Not that it really matters in practise.

source/blender/makesrna/intern/rna_nodetree.c
2998

Not strictly related to this patch, but at some point we might consider having a general update function here since rna_ShaderNodeSubsurface_update is essentially the same.

intern/cycles/kernel/closure/bsdf_disney_sheen.h
49 ↗(On Diff #7699)

I thought it would be nice to have the same variables like in the code of the BRDF Explorer of Disney, but I guess, this isn't really necessary anymore, so I will flatten this.

intern/cycles/kernel/closure/bsdf_microfacet_multi_impl.h
86–96

I'm getting a bit of a problem to integrate the code here, because the fresnel_conductor that is used in D2003 can only be applied for metals. But in Disney one can have an interpolation between metal and non-metal materials and thus I would need to have both the fresnel_conductor and fresnel_dielectric_cos part and interpolate between those two.

Besides this I don't know how to intuitively map the IOR/specular to the complex representation. Would I have to simply set n to the IOR and k to 0?

intern/cycles/kernel/svm/svm_closure.h
308

I checked it and didn't get any strange stuff, but probably there could be a case, where artifacts occur. Currently I would keep it as is and if strange behavior is noticed, I can fix this.

327

You mean the calculation of refl_color? This is used as weight for the bsdf ;-)

On a general note, I don't really like the variable names cspec0 and csheen0. In small code parts, "technical" variable names that are references to some mathematical operation (like NdotL or val_sqr) are reasonable, but when they're used all over the place a more descriptive name would be nice.

I replaced csheen0 with sheen_color, but the cspec0 is a bit harder to rename. As it is the specular color at normal incidence, a name like specular_col_normal_incidence would be good but too long. A shorter version would be specular_F0_color, if this is sufficient, to describe what this variable is for.

When I download the raw diff I see quite a bit of inconsistent indentation - spaces vs tabs. Could that be fixed too, please?

Pascal Schön (VanCantus) updated this revision to Diff 7754.EditedNov 7 2016, 12:19 PM

Again some updates on the code based on the comments:

  • Consolidated code in the bsdf_microfacet_multi_impl.h file to reduce some computational expense.
  • Use reflection BSDF for glossy reflections when roughness is 0.0 to reduce computational expense.
  • Improved the optics of the clearcoat part by using GTR1 instead of GTR2 for the D term.
  • Code style cleanup and removed some unneeded code.
  • Resolved inconsistencies in using tabs and spaces.

I have a problem for clearcoat. When I'm using default light sources (like directional lights), the clearcoat highlight gets too bright (approx. 10 times), when comparing with the BRDF Explorer implementation. For image based lighting it seems to be working correctly. Also the default specular highlight has the correct brightness... Does anyone have an idea why this happens? I thought of adding a factor of 0.1 to the eval_reflect function, but that would be a magic number without any context.

About the name, I suggest to name it principled BSDF in the source code at least. That's somewhat unique and easy to search for, and matches the naming from the paper.

Then in the UI I'm fine with either "Principled BSDF" (matching e.g. Houdini) or "Physically Based BSDF" (matches terminology used in games more, but more difficult to search for in Blender documentation, stackexchange, forums).

I have a problem for clearcoat. When I'm using default light sources (like directional lights), the clearcoat highlight gets too bright (approx. 10 times), when comparing with the BRDF Explorer implementation. For image based lighting it seems to be working correctly. Also the default specular highlight has the correct brightness... Does anyone have an idea why this happens? I thought of adding a factor of 0.1 to the eval_reflect function, but that would be a magic number without any context.

This must be either a bug in the BSDF, or a bug in the integrator. First I would call bsdf_*_eval_reflect from bsdf_*_sample with the sampled direction, and check that the eval and pdf values match. If they don't then you'd have to track down why they are different.

If they match then you can still have the problem that the pdf does not match the probability of sampling the direction, but I don't think you changed that logic? So either is already broken (which you could verify with an existing Glossy BSDF) or it's a different issue.

You can also try disabling multiple importance sampling on the lamp or world, and verifying if it converges to the same result (using a BSDF with non-zero roughness and a lamp with non-zero radius or world texture that is not too high frequency).

This must be either a bug in the BSDF, or a bug in the integrator. First I would call bsdf_*_eval_reflect from bsdf_*_sample with the sampled direction, and check that the eval and pdf values match. If they don't then you'd have to track down why they are different.

If they match then you can still have the problem that the pdf does not match the probability of sampling the direction, but I don't think you changed that logic? So either is already broken (which you could verify with an existing Glossy BSDF) or it's a different issue.

You can also try disabling multiple importance sampling on the lamp or world, and verifying if it converges to the same result (using a BSDF with non-zero roughness and a lamp with non-zero radius or world texture that is not too high frequency).

I found the bug... the error was in the GTR1 calculation function where I mistakenly wrote a M_1_PI_F instead of M_PI_F. As this is only one line of code I will provide this correction together with other fixes after the next revision.

What is the status here?

I suggest we name this Principled BSDF, verify that this doesn't break GPU rendering or cause major performance regressions, and then we can go ahead and commit this?

I was waiting for further reviews. ;-)

Ok, so then I will rename it to Principled BSDF. Maybe I have some extra time to spend on performance.

And how about the thin surface approximation?

Pascal Schön (VanCantus) set the repository for this revision to rC Cycles.

Changes:

  • Renamed the Disney BSDF to Principled BSDF
  • Fixed artifacts in specular reflections for low roughness values
  • Distinction between GGX with or without Fresnel is now handled via the type of the closure (e.g. CLOSURE_BSDF_MICROFACET_GGX_FRESNEL_ID)
  • Fixed the error in the clearcoat, where the highlight appeared too bright for default light sources (like directional lights)

The updates look good to me, I didn't see anything new to comment on, so basically LGTM. I've been testing various parameter and didn't succeed in breaking it so far.

There's some plots for performance of this patch here:
https://brechtvl.github.io/cycles-ci/

CPU timings are too noisy to say much, but seem basically neutral. AMD OpenCL is a little faster for some reason, while with CUDA are some 2-3% slowdowns. This is the typical extra register pressure issue, where it might help to inline or uninline some functions, or move code like the SVM closure allocation. Perhaps moving the principled BSDF setup in svm_closure.h to a non-inlined functions might help, but it might also make things work.

I can do some experiments to try to fix the minor CUDA performance regression somewhere in the next few weeks.

If other Cycles devs agree, I'd suggest to commit this to master as a feature for 2.79. I'm sure this will be very popular and that we can get some good feedback and bug reports. We'll probably want to make this the default node when creating a new Cycles material even. This might also help for 2.8, as a reference for implementing an OpenGL version of this shader and for importers/exporters to use.

This revision is now accepted and ready to land.Feb 1 2017, 4:45 AM
Sergey Sharybin (sergey) requested changes to this revision.Feb 1 2017, 11:23 AM

Here is a fix which should be applied to comply strict compiler flags:

1diff --git a/intern/cycles/kernel/osl/osl_closures.h b/intern/cycles/kernel/osl/osl_closures.h
2index 462990bc40d..c9740f81c8a 100644
3--- a/intern/cycles/kernel/osl/osl_closures.h
4+++ b/intern/cycles/kernel/osl/osl_closures.h
5@@ -61,6 +61,7 @@ OSL::ClosureParam *closure_bsdf_microfacet_ggx_aniso_fresnel_params();
6​ OSL::ClosureParam *closure_bsdf_microfacet_multi_ggx_fresnel_params();
7​ OSL::ClosureParam *closure_bsdf_microfacet_multi_ggx_glass_fresnel_params();
8​ OSL::ClosureParam *closure_bsdf_microfacet_multi_ggx_aniso_fresnel_params();
9+OSL::ClosureParam *closure_bsdf_principled_clearcoat_params();
10
11​ void closure_emission_prepare(OSL::RendererServices *, int id, void *data);
12​ void closure_background_prepare(OSL::RendererServices *, int id, void *data);
13@@ -81,6 +82,7 @@ void closure_bsdf_microfacet_ggx_aniso_fresnel_prepare(OSL::RendererServices *,
14​ void closure_bsdf_microfacet_multi_ggx_fresnel_prepare(OSL::RendererServices *, int id, void *data);
15​ void closure_bsdf_microfacet_multi_ggx_glass_fresnel_prepare(OSL::RendererServices *, int id, void *data);
16​ void closure_bsdf_microfacet_multi_ggx_aniso_fresnel_prepare(OSL::RendererServices *, int id, void *data);
17+void closure_bsdf_principled_clearcoat_prepare(OSL::RendererServices *, int id, void *data);
18
19​ #define CCLOSURE_PREPARE(name, classname) \
20​ void name(RendererServices *, int id, void *data) \

There is also non-working GLSL shading mode which should be fixed. Here is what i've got in the console:

GPUShader: compile error:
0(3682) : error C7011: implicit cast from "float" to "vec3"
0(3687) : error C7623: implicit narrowing of type from "vec3" to "float"

The full shader source is: https://hastebin.com/irebedemap.cs

This revision now requires changes to proceed.Feb 1 2017, 11:23 AM
  • Fix to comply strict compiler flags
  • Fix for non-working GLSL shading
  • Implemented the real-time view port for the Principled BSDF (except for transparency and SSS)

Thanks for your review. The current update implements the GLSL version of the Principled BSDF. I also tried to implement the anisotropy correctly but failed due to the missing tangent. I then computed a default tangent as a workaround. Maybe one of you (who has more knowledge in GLSL shaders) can have a look into it and give a better way of handling this problem.

The other two mentioned problems have also been resolved.

Brecht Van Lommel (brecht) accepted this revision.EditedFeb 18 2017, 11:02 PM

I didn't expect you'd actually implement the BSDF in GLSL, but that's great. For the other nodes we have a quite poor approximation, this is better than that. The GLSL shader seems to be working ok on macOS.

For the tangent, we'd need to add support for that in the GLSL system indeed. Maybe refractor the code to move that computation out of the renderers as much as possible and make it a new customdata layer for Blender itself.

I wouldn't worry about that at this point though, Cycles GLSL is still a very simple approximation and there's more important things to improve than anisotropic shading.

Occasionally testing this branch (build 26e906d) it appears to have a major bug rendering SSS with Branched Path Tracing, as shown in pictures here: https://blenderartists.org/forum/showthread.php?403342-Cycles-Disney-Brdf&p=3162218&viewfull=1#post3162218. Basically branched takes care of base color only, as if it is ignoring the SSS color input.

Pascal Schön (VanCantus) planned changes to this revision.Tue, Mar 28, 8:04 AM

Regarding the SSS color topic: Currently I'm "multiplying" the surface and subsurface color together. Other renderers like the one in Modo and Renderman are simply blending those colors together using the subsurface amount. But there's also another approach which is described here. At the moment I think it would make sense to handle it like Renderman does because this is the "ground truth" for the Disney shader, but it can be discussed before I change it.

Multiplying doesn't seem all that useful to me, for typical 0..1 textures it will just make the subsurface part darker than the diffuse part, for which there is no good justification I think. Controlling the result of two textures multiplied together also isn't very intuitive. A case could be made for having just a base color and no subsurface color at all, but having the separate subsurface color there is useful too. Since we're trying to match an existing standard we might as well follow it here and use blending.

Is there any reason not to merge this for Blender 2.79 still? @Sergey Sharybin (sergey), any objections from your side assuming @Pascal Schön (VanCantus) has time to handle bug reports for this shader? It has already received user testing as well.

@Brecht Van Lommel (brecht), looked through the code and didn't notice anything major. Would only suggest using proper copytight line in the new files, the year is wrong there.

The patch needs update against latest master.

Was it tested on CUDA and OpenCL?

The last patch worked with OpenCL and CUDA for me. I didn't try updating it to latest master, but I don't foresee a problem there.

Switched from multiplication of base and subsurface color to blending between them using the subsurface parameter. I also updated the copyright for the new files.

Looks good to commit.

It's working here with CPU and OpenCL rendering.

@Pascal Schön (VanCantus), do you have time to commit this to master and handle possible bug reports?

Not sure if you're busy or if I was unclear about this being ok to merge, just checking :)

@Brecht Van Lommel (brecht) I was on vacation for a few days. ;-) I'll do the merge and also handle appearing bugs. But if some bigger stuff appears that I can't handle, I will ask for help.

As this is my first patch I'm going to apply, I don't want to mess things up. Should I do a diff and apply this as described here? The only thing is that I haven't rebased my branch all the way through, but merged master directly into it every once in a while.

I think it would be good to rebase/squash everything together into one commit. No need to go through patches for that though, the easiest way to do that is to merge the branch into master like this:

git checkout master
git merge --squash cycles_disney_brdf
git commit

At the end of the commit log you can have this exact line so it references this review:

Differential Revision: https://developer.blender.org/D2313
Closed by commit rB8825a8e951c1: Squashed commit of the following: (authored by Pascal Schoen <pascal.schoen@adidas-group.com>). · Explain WhyTue, Apr 18, 11:44 AM
This revision was automatically updated to reflect the committed changes.