EmbarkStudios / kajiya

💡 Experimental real-time global illumination renderer 🦀
Apache License 2.0
4.86k stars 181 forks source link

GGX_Smith implementation error? #68

Closed Catddly closed 2 years ago

Catddly commented 2 years ago

I am a student who is learning pbr right now, and i have an question about ggx smith implementation.

I am confused that maybe i misunderstand somthing or it is just your impletation is wrong.

In your shader brdf.hlsl, line 114, function g_smith_ggx1().

I try to change the form of this equation into the same form in https://ubm-twvideo01.s3.amazonaws.com/o1/vault/gdc2017/Presentations/Hammon_Earl_PBR_Diffuse_Lighting.pdf (On page 80). The G1(v) formular. But I realized that your implementation is (2 ndotv) / (ndotv + sqrt(ndotv^2 + α^2 (1 - ndotv^2))), and the fomular in GDC is (2 ndotv) / (ndotv + sqrt(α^2 + ndotv^2 (1 - α^2))).

It seems that term α^2 and ndotv is reversed in finding square root of some value.

Your: sqrt(ndotv^2 + α^2 (1 - ndotv^2)). GDC: sqrt(α^2 + ndotv^2 (1 - α^2)).

Maybe your implementation is different with the formular in GDC? But i can't understand. Thanks in advance for your help.

Your project is a really good learning material!

h3r2tic commented 2 years ago

Hey! Happy to hear you're enjoying digging through kajiya!

Let's write those out:

a) sqrt(ndotv² + α² * (1 - ndotv²))
b) sqrt(α² + ndotv² * (1 - α²))

Expanding the terms:

a) sqrt(ndotv² + α² - α² * ndotv²)
b) sqrt(α² + ndotv² - ndotv² * α²)

Rearranging:

a) sqrt(α² + ndotv² - α² * ndotv²)
b) sqrt(α² + ndotv² - ndotv² * α²)

They're the same thing :)

Catddly commented 2 years ago

Oh! I am so silly!!!!!!! Thank you! Really appreciated your works!

h3r2tic commented 2 years ago

No worries, thank you for caring to let me know about the potential issue ❤️