Branched Path Tracing converges to different result than plain Path Tracing #46760

New Issue

Stefan Werner · 2015-11-13T13:50:25+01:00

Stefan Werner commented

2015-11-13 13:50:25 +01:00

System Information
OS X 10.11, Nvidia GT 750M

Blender Version
Broken: 2.76b, hash f337fea

Short description of error
In shaders that combine sharp specular with diffuse, when lit with an area light, the diffuse result in branched path tracing differs from the result with path tracing

**Exact steps for others to report
pt_vs_branchedpt.blend
Open the attached file.
Make two renders with Cycles, one set to path tracing and one set to branched path tracing. Save the diffuse pass of each rendered image - the blend file should be set up to save combined, diffuse and glossy passes. Open the diffuse in an image editor and compare.

Expected result: With the exception of noise (variance), path tracing and branched path tracing should converge to the same result.

Actual result: In the diffuse pass of the branched path tracing render, there is a dark spot on Suzanne's forehead. It is in the location of the highlight in the glossy pass. This reproduces in both CUDA and CPU renders, as well as on Windows 10. Mesh lights or large spherical/point lights give similar results.

Regression: This happens only with multiple importance sampling. When multiple importance sampling is turned off on the shaders, branched path tracing converges to the same result as path tracing.

I believe there must be something wrong in how the branched path tracer uses MIS to combine samples. I have tried figuring out in the code, where exactly this happens, but so far I have not succeeded.

**System Information** OS X 10.11, Nvidia GT 750M **Blender Version** Broken: 2.76b, hash f337fea **Short description of error** In shaders that combine sharp specular with diffuse, when lit with an area light, the diffuse result in branched path tracing differs from the result with path tracing **Exact steps for others to report [pt_vs_branchedpt.blend](https://archive.blender.org/developer/F254277/pt_vs_branchedpt.blend) Open the attached file. Make two renders with Cycles, one set to path tracing and one set to branched path tracing. Save the diffuse pass of each rendered image - the blend file should be set up to save combined, diffuse and glossy passes. Open the diffuse in an image editor and compare. Expected result: With the exception of noise (variance), path tracing and branched path tracing should converge to the same result. Actual result: In the diffuse pass of the branched path tracing render, there is a dark spot on Suzanne's forehead. It is in the location of the highlight in the glossy pass. This reproduces in both CUDA and CPU renders, as well as on Windows 10. Mesh lights or large spherical/point lights give similar results. Regression: This happens only with multiple importance sampling. When multiple importance sampling is turned off on the shaders, branched path tracing converges to the same result as path tracing. I believe there must be something wrong in how the branched path tracer uses MIS to combine samples. I have tried figuring out in the code, where exactly this happens, but so far I have not succeeded.

Stefan Werner commented

2015-11-13 13:50:25 +01:00

Changed status to: 'Open'

Stefan Werner commented

2015-11-13 13:50:25 +01:00

Added subscriber: @Stefan_Werner

Aaron Carlisle commented

2015-11-13 14:23:27 +01:00

Added subscriber: @Blendify

Sergey Sharybin was assigned by Bastien Montagne

2015-11-13 15:51:47 +01:00

Bastien Montagne commented

2015-11-13 15:51:47 +01:00

Added subscriber: @mont29

Thomas Dinges commented

2015-11-13 18:00:21 +01:00

Added subscriber: @ThomasDinges

Thomas Dinges commented

2015-11-13 18:00:21 +01:00

Confirmed, the difference only happens with MIS ensabled for the Mesh Light.

Edit: Seems it's not related to Sample All Lights as far as I can tell.

Confirmed, the difference only happens with MIS ensabled for the Mesh Light. Edit: Seems it's not related to Sample All Lights as far as I can tell.

Thomas Dinges commented

2015-11-13 18:09:22 +01:00

Added subscriber: @brecht

Brecht Van Lommel commented

2015-11-14 15:11:29 +01:00

When sampling a BSDF for indirect rays in path tracing, then we all take into account how much other BSDFs contribute. That's in _shader_bsdf_multi_eval. The ray will be marked as having the ray type of the BSDF, but the throughput can contain contributions from other types of BSDFs too. Indirect light passes are affected by this, but also lamp MIS since that is computed as part of indirect lighting.

A solution would be to pass along separate throughputs for the different ray types. This adds quite some code complexity and has an impact on performance. Another would be to not use the contribution from other ray types, but that will increase noise in some cases. It might not be so bad in practice though, perhaps it's worth testing what happens when you modify _shader_bsdf_multi_eval to use only BSDFs with the same ray type.

I never made fixing this a priority because I don't think it's that bad in practice. With more advanced BSDF models the distinction between diffuse and glossy is difficult anyway, and the best you can get is diffuse-like and glossy-like results in these passes, which I think the current code satisfies.

When sampling a BSDF for indirect rays in path tracing, then we all take into account how much other BSDFs contribute. That's in `_shader_bsdf_multi_eval`. The ray will be marked as having the ray type of the BSDF, but the `throughput` can contain contributions from other types of BSDFs too. Indirect light passes are affected by this, but also lamp MIS since that is computed as part of indirect lighting. A solution would be to pass along separate throughputs for the different ray types. This adds quite some code complexity and has an impact on performance. Another would be to not use the contribution from other ray types, but that will increase noise in some cases. It might not be so bad in practice though, perhaps it's worth testing what happens when you modify `_shader_bsdf_multi_eval` to use only BSDFs with the same ray type. I never made fixing this a priority because I don't think it's that bad in practice. With more advanced BSDF models the distinction between diffuse and glossy is difficult anyway, and the best you can get is diffuse-like and glossy-like results in these passes, which I think the current code satisfies.

Brecht Van Lommel commented

2015-11-14 15:14:47 +01:00

Skipping some BSDF evaluations in _shader_bsdf_multi_eval will also be faster, if you sample e.g. a diffuse BSDF you then skip evaluating glossy BSDFs which can be quite costly. So perhaps it ends up being a net benefit, depends how much it increases noise.

Skipping some BSDF evaluations in `_shader_bsdf_multi_eval` will also be faster, if you sample e.g. a diffuse BSDF you then skip evaluating glossy BSDFs which can be quite costly. So perhaps it ends up being a net benefit, depends how much it increases noise.

Stefan Werner commented

2015-11-17 18:28:50 +01:00

Brecht, I'm not sure I'm following you. In my opinion, the path tracing integrator gives the correct result, where diffuse is uniform, the branched path tracer is incorrect and darkens diffuse in the presence of specular.

PT calls _shader_bsdf_multi_eval() twice, once when it takes a light sample and once when it takes a BSDF sample. The shading pdf used in both cases is derived from all BSDFs on a surface.

In the BPT case, _shader_bsdf_multi_eval() is called for light samples, but not for BSDF samples. I'm not sure if MIS gives us the correct result when we work with different pdf derivations for light sampling and BSDF sampling?

Brecht, I'm not sure I'm following you. In my opinion, the path tracing integrator gives the correct result, where diffuse is uniform, the branched path tracer is incorrect and darkens diffuse in the presence of specular. PT calls _shader_bsdf_multi_eval() twice, once when it takes a light sample and once when it takes a BSDF sample. The shading pdf used in both cases is derived from all BSDFs on a surface. In the BPT case, _shader_bsdf_multi_eval() is called for light samples, but not for BSDF samples. I'm not sure if MIS gives us the correct result when we work with different pdf derivations for light sampling and BSDF sampling?

Brecht Van Lommel commented

2015-11-17 21:17:27 +01:00

Ah, I assumed too quickly that it was this related to this, if it's the branched path tracing that's wrong then it must be something else.

You may be right that using _shader_bsdf_multi_eval() for light samples but not BSDF samples could be an issue, I haven't worked out the math. If the MIS weight to balance the BSDF and light samples was computed for each BSDF individually then I think it could work, but with all those different BSDF pdfs are accumulated into a single pdf which is the used to compute the MIS weight it's not so clear.

Ah, I assumed too quickly that it was this related to this, if it's the branched path tracing that's wrong then it must be something else. You may be right that using `_shader_bsdf_multi_eval()` for light samples but not BSDF samples could be an issue, I haven't worked out the math. If the MIS weight to balance the BSDF and light samples was computed for each BSDF individually then I think it could work, but with all those different BSDF pdfs are accumulated into a single pdf which is the used to compute the MIS weight it's not so clear.

Sergey Sharybin commented

2015-11-18 08:56:50 +01:00

@brecht, samping only BSDFs of the ray type in _shader_bsdf_multi_eval() didn't give any measurable difference actually. Also, if you re-combine light passes back into combined then you'll have exact the same result with both regular and branched path tracing. So overall light contribution is preserved, just separation doesn't match between two integrators.

Now, what do we consider a ground-truth here? It's kinda both approaches has own logic here..

@brecht, samping only BSDFs of the ray type in `_shader_bsdf_multi_eval()` didn't give any measurable difference actually. Also, if you re-combine light passes back into combined then you'll have exact the same result with both regular and branched path tracing. So overall light contribution is preserved, just separation doesn't match between two integrators. Now, what do we consider a ground-truth here? It's kinda both approaches has own logic here..

Stefan Werner commented

2015-11-18 10:52:12 +01:00

The ground truth would be the path traced result without MIS, which matches PT with MIS and BPT without MIS.

I think found a fix for the BPT/MIS case, it appears to work correctly when you don't use the one-sample model for light samples, but instead call power_heuristic individually per closure. That way, both light samples and BSDF samples use the same method for calculating MIS.

I'll do some more testing to make sure, then I can hopefully submit a patch.

The ground truth would be the path traced result without MIS, which matches PT with MIS and BPT without MIS. I think found a fix for the BPT/MIS case, it appears to work correctly when you don't use the one-sample model for light samples, but instead call power_heuristic individually per closure. That way, both light samples and BSDF samples use the same method for calculating MIS. I'll do some more testing to make sure, then I can hopefully submit a patch.

Stefan Werner commented

2015-11-20 17:45:24 +01:00

Here we go. This would be my proposed fix. I hope my code style isn't too far from your standards.
0001-fix-for-#46760.patch

Here we go. This would be my proposed fix. I hope my code style isn't too far from your standards. [0001-fix-for-#46760.patch](https://archive.blender.org/developer/F256412/0001-fix-for-#46760.patch)

Sergey Sharybin commented

2015-11-25 10:35:37 +01:00

@Stefan_Werner, Code would need some cleanup (some whitespace and naming) but that'd be easier to do from our side.

@brecht, does it seems a correct thing to do for you as well?

@Stefan_Werner, Code would need some cleanup (some whitespace and naming) but that'd be easier to do from our side. @brecht, does it seems a correct thing to do for you as well?

Brecht Van Lommel commented

2015-11-25 21:26:09 +01:00

It seems correct.