Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

GGX_Smith implementation error? #68

Closed
Catddly opened this issue Oct 26, 2022 · 3 comments
Closed

GGX_Smith implementation error? #68

Catddly opened this issue Oct 26, 2022 · 3 comments
Labels
bug Something isn't working

Comments

@Catddly
Copy link
Contributor

Catddly commented Oct 26, 2022

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!

@Catddly Catddly added the bug Something isn't working label Oct 26, 2022
@h3r2tic
Copy link
Collaborator

h3r2tic commented Oct 26, 2022

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 :)

@h3r2tic h3r2tic closed this as completed Oct 26, 2022
@Catddly
Copy link
Contributor Author

Catddly commented Oct 26, 2022

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

@h3r2tic
Copy link
Collaborator

h3r2tic commented Oct 26, 2022

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants