Cycles: Improve sample weighting of the Principled BSDF specular component
ClosedPublic

Authored by Lukas Stockner (lukasstockner97) on May 19 2017, 2:25 AM.

Details

Summary

Currently the specular/glossy component always uses a weight of 1 even if it
just contributes a tiny amount to the result.

This patch adds an estimate for the weight that makes the sampling weight
more reasonable, which can drastically improve noise in some cases.

Also, this is important for denoising feature pass generation, since it uses
sampling weights of closures in its specular/diffuse heuristic. With the
current weighting, even setting Specular to 0.001 will cause Cycles to
wait for the next bounce and therefore ignore the texture while denoising.

Diff Detail

Repository
rB Blender

Example scene (pointlamp over principled BSDF with specular=roughness=0.1):

rB49bd3d3a1a7:

This patch:

@Lukas Stockner (lukasstockner97) The render settings of your scene are one sample per pixel, aren't they?

What I found out is that with this fix the diffuse gets brightened for high specular and roughness values. For me it looks ok how it behaves, although I will do some further tests with RenderMan and your patched version.

Here is the comparison with the different brightnesses at specular=roughness=1.0:

49bd3d3a1a73:

This patch:

Brecht Van Lommel (brecht) requested changes to this revision.EditedMay 20 2017, 5:05 PM

I don't think the converged render should change because of this patch? That seems like a bug.

Weighting the closure sample weights by the fresnel is absolutely what we should be doing for better importance sampling, ideally not just for the microfacet BSDF but also diffuse, transmission, BSSRDF, etc.

Can we move this modification of the weight / sample_weight into bsdf_microfacet_ggx_fresnel_setup and similar setup functions? That will make it work also for OSL, and is a little more clear and reusable I think.

This revision now requires changes to proceed.May 20 2017, 5:05 PM

Also, this is using the Fresnel along the normal instead of Fresnel integrated over the microfacet normals. I guess that is a decent approximation, though I'm not sure how big the difference gets, if there is some case where it is very far off and noise is increased.

I don't think the converged render should change because of this patch? That seems like a bug.

I think this is the difference to a correct fresnel, where the microstructure is considered, that every node tree based Uber shader has. The higher the roughness, the more the fresnel wrt the surface normal is off to the integration. That's why I decided to put the weighting into the shader call. I've also seen the problem with importance sampling but didn't want to implement the approximation in the first run as it was more important for me to be as close to Disneys implementation as possible.

I think this is the difference to a correct fresnel, where the microstructure is considered, that every node tree based Uber shader has. The higher the roughness, the more the fresnel wrt the surface normal is off to the integration. That's why I decided to put the weighting into the shader call. I've also seen the problem with importance sampling but didn't want to implement the approximation in the first run as it was more important for me to be as close to Disneys implementation as possible.

The way I understood this patch is that is that it is multiplying the weight with the average normal fresnel , and then later dividing it out again and multiplying with the more correct per-microfacet fresnel, so that sampling is affected but the integrated BSDF actually remains the same.

The simpler way to achieve that would be to adjust only the closure sample_weight.

The way I understood this patch is that is that it is multiplying the weight with the average normal fresnel , and then later dividing it out again and multiplying with the more correct per-microfacet fresnel, so that sampling is affected but the integrated BSDF actually remains the same.

The simpler way to achieve that would be to adjust only the closure sample_weight.

Yeah, that's my understanding as well, but I think that it's kind of a similar approach that others were doing when creating a node-based version, although it's not completely equal. At least when it comes to weighting the closure call and with this the sampling.

Here's a simplified version of the patch that still gives the same improvement.

Also included is the proposed fix for T51836.

I really think that this should still go into 2.79 since the noise difference can be huge and there's no risk of a regression (since the Principled BSDF is new anyways).

Note that better sampling weights for the diffuse/SSS components aren't included yet, but those tend to be fairly high-weight anyways, so they're not as important for now.

And here's a quick test render, credit for the scene goes to Rhys on BlenderArtists.

Master:


Patch:

Looks good to me. This is similar to how we handled the situation with the "Physical Root Node" in Poser.

For me this looks good as well. Brightness seems to be more plausible especially when using the Multiscatter GGX.

Both the sample weight tweak and GGX pdf fixes look good to me, seems safe.

This revision is now accepted and ready to land.Jun 19 2017, 8:52 PM
This revision was automatically updated to reflect the committed changes.