This patch implements the Disney BSDF directly in the Cycles renderer. The code is based on the 2015 version of the Disney shader.
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.
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.
|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.
|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.
This would be nicer if there was just one if(bsdf->extra && bsdf->extra->use_fresnel)
Picky: This empty line is diff noise.
Same as above, one if would be nicer.
Again just an empty line.
There seems to be some intendation noise here, I'm not sure which one is the correct one.
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.
This function seems unused?
|51 ↗||(On Diff #7699)|
If you use the accurate function above, you should also use those components here.
More descriptive variable names would be nice here, clearcoat_normal tells a lot more than CN.
Brackets around single statements are usually preferred in Cycles code to avoid potential merge conflicts in the future.
As above, a single if might be nicer.
You should probably use linear_rgb_to_gray here.
Same as above.
Automatically switching between MultiGGX and GGX is nice from a usability point of view, but might give visible borders when using textued roughness.
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.
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.
|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.
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?
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.
You mean the calculation of refl_color? This is used as weight for the bsdf ;-)
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.
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).
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?
- 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:
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.
Here is a fix which should be applied to comply strict compiler flags:
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
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.
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.
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) 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
I was reading this thread regarding how the Subsurface slider should work and wanted to give my two cents. Currently, you can't create a pure white material with mixed 0,1,1 diffuse and 1,0,0 subsurface like in fig1, because with the slider at 0.5, the shader converges to a total reflectivity of 0.5,0.5,0.5 . Would Renderman give that same result?
The two methods artists use in most software is to either control each internal color and let the shader converge to the final color indirectly by simple addition (fig 2), or to directly control the final converged color and the perceived sss color and let the shader do the math under the hood (fig 3) The first method it's harder to predict and can give values above 1 if not normalized. The second method is more common and easier for artists, as they don't have to guess what the final color will be. However, if you have a shader that can do the first method, you can replicate the second one with just some extra nodes at the front. (I know you can replicate the second method by moving the subsurface slider to 1.0, using the subsurface color as the final color and adjusting the individual radius values. But that is only a single scatter)
It would be nice if Principled BSDF did a simple normalized sum of diffuse and sss when the slider is at 0.5, instead of a mix, that way it would have the intuitive blend from base color to sss color at 0 and 1 respectively, like it currently does, while at the same time allow for the two methods above and for a white mixed material. It basically allows for every case above (Thanks for T51589 and D2685 btw)
@Wo!262 (wo262), normalizing the resulting color means the albedo will be 1 for at least one of the color channels, which is not realistic. It should be possible to link a typical albedo texture to both colors without having to compensate for that somehow.
I think the Subsurface Radius provides the control that is needed here, when that goes to 0 it becomes equivalent to diffuse. Mixing diffuse and subsurface with additive color channels seems like a roundabout way of doing that.
I understand, but when linking image textures to those colors you would often end up with reflectance 100%, which is almost never correct. Regardless, we are aiming for compatibility with other renderers and I don't think any of them are adding these colors.