sebh / UnrealEngineSkyAtmosphere

Unreal Engine Sky Atmosphere Rendering Technique
https://www.unrealengine.com
MIT License
714 stars 69 forks source link

Throughput is not updated with uniform sampling #11

Closed c52e closed 2 years ago

c52e commented 2 years ago

Hello, Thank you for sharing this code. It helped me a lot in understanding volume rendering algorithms. I'm reading the path tracing implemention. I‘m a little perplexed by the following code. With uniform sampling, phaseValue / phasePdf is not constant 1. Why throughput is not updated? https://github.com/sebh/UnrealEngineSkyAtmosphere/blob/f675c23ee16ef84bcbd3b314d4280c8f24c953b7/Resources/RenderSkyPathTracing.hlsl#L836-L843 With MieScattScale larger than 1.0 (need to change SliderFloat v_max) and sun behind camera, sky will be brighter when MIE_PHASE_IMPORTANCE_SAMPLING is disabled than when it is enabled.

sebh commented 2 years ago

Hi, Happy if it helps you! MIE_PHASE_IMPORTANCE_SAMPLING: I think this code path I never really tested and that is why it is off (it seems like there could be a bug then). I should have added a comment there. When it comes to uniform sampling: isotropic phase function value is 1/(4PI). When selecting a direction uniformly the PDF is also 1/(4PI). So value/pdf = 1 ==> there is no need to update the throughput. Assuming I am not forgetting anything else (I think that is another comment I should have added)

c52e commented 2 years ago

When both uniform sampling and isotropic phase function is used, value/pdf == 1 is true, but it seems that path tracing shader is using anisotropic phase function value even if MIE_PHASE_IMPORTANCE_SAMPLING is disabled (default case).

sebh commented 2 years ago

I have checked the code. yes it seems that HG phase function is the one always used. And, although problematic for spicky phase, this is still correct with uniform sampling. MIE_PHASE_IMPORTANCE_SAMPLING 1 is probably faulty (changed phase function to another one and importance sample is maybe buggy). Something for someone to investigate and fix maybe. I'll add a comment that this is an unfinished path.

sebh commented 2 years ago

Done in https://github.com/sebh/UnrealEngineSkyAtmosphere/commit/f446f242ebd4bde512446a1a3212a5e333a0adb5 Thanks for reporting!