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

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

Details

Reviewers

Sergey Sharybin (sergey)
Brecht Van Lommel (brecht)
Pascal Schön (VanCantus)

Commits

rBS197917608804: Cycles: Fix excessive sampling weight of glossy Principled BSDF components
rC9557729e07c7: Fix excessive sampling weight of glossy Principled BSDF components
rB197917608804: Cycles: Fix excessive sampling weight of glossy Principled BSDF components

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

Branch

principled_spec_weight

Build Status

Buildable 646
Build 646: arc lint + arc unit

Event Timeline

Lukas Stockner (lukasstockner97) created this revision.May 19 2017, 2:25 AM

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:

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

mathieu menuet (bliblubli) awarded a token.May 20 2017, 6:07 PM

mathieu menuet (bliblubli) added a subscriber: mathieu menuet (bliblubli).

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.

In D2677#63024, @Brecht Van Lommel (brecht) wrote:

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.

In D2677#63059, @Pascal Schön (VanCantus) wrote:

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.

In D2677#63071, @Brecht Van Lommel (brecht) wrote:

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.

Lukas Stockner (lukasstockner97) mentioned this in T51836: BSDF Weighting Issue with MultiGGX.Jun 18 2017, 7:07 AM

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: