search

NVIDIAGameWorks / RTXGI-DDGI

RTX Global Illumination (RTXGI)
https://developer.nvidia.com/rtxgi
Other
689 stars 55 forks source link

Possible Error when Blending Irradiance #60

Closed dasmysh closed 2 years ago

dasmysh commented 2 years ago

I think I found an error in the blending shader for the irradiance. From how I understand it you do not account for the cosine term in the radiance integral correctly.

Here you correctly calculate that term, but by using it as a weight for the blending you actually cancel it out again (when dividing by the sum of weights). Therefore I think when using the weight you should actually do this: result += float4(probeRayRadiance * weight, 1.0); The depth branch is correct though.

This will also remove the need for the division by 2 here which you canceled out for the depth values in in one of your latest commits anyways.

I hope I did not overlook something.

tiont commented 2 years ago

Thank you for reporting this issue @dasmysh. We appreciate the feedback. Our teams will consider your suggestion for future update.

acmarrs-nvidia commented 2 years ago

Hi @dasmysh, thanks for looking deeply at the math in the blending shader.

Check out the Irradiance Integral Estimator section of the Math Guide in the documentation.

Your suggested changes are technically correct, but they compute the same result a slightly different way (and consequently have different characteristics). Your changes accumulate the number of radiance samples, which are then used in the divisor on line 560. You are correct that when dividing the sum of radiance by the number of samples the division by two we perform is not necessary.

For irradiance, we purposely divide by the sum of the cosine weights (instead of the number of samples) to decrease the variance in the estimate. Because of this, we need to include the division by 2 on line 560 to normalize the result properly. Hopefully the Math Guide explains this a bit better than I am here.

Note that distance values are not cosine weighted and are not divided by the sum of cosine weights. This makes the division by two on line 560 unnecessary for distance, which is why I multiply by two when sampling the distance values. I chose to multiply by two after sampling distance to avoid branching (albeit static) in the blending shader. I'll put a note in the comments for normalization (line 560) to make this more clear.

Hope that helps!

dasmysh commented 2 years ago

That did help. Thanks. But you may also want to adjust that same comment where it says https://github.com/NVIDIAGameWorks/RTXGI/blob/0c6ad946353640e0e1b59baa1935ddf9985b2d39/rtxgi-sdk/shaders/ddgi/ProbeBlendingCS.hlsl#L558 which is still confusing, especially because it is not what you are actually doing and not 'the sum of the weights' (I am refering to the N * part).

acmarrs-nvidia commented 2 years ago

Good catch. I've corrected the comment. Thank you for the feedback!