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

[question] Are brdfLUT sampling coordinates correct? #89

Closed
lhog opened this issue Oct 12, 2018 · 5 comments
Closed

[question] Are brdfLUT sampling coordinates correct? #89

lhog opened this issue Oct 12, 2018 · 5 comments

Comments

@lhog
Copy link

lhog commented Oct 12, 2018

Hi!

I was going through several PBR implementation and noticed that this implementation samples the BRDF LUT texture using the (1.0 - roughness) as a second texture coordinate:
https://github.com/KhronosGroup/glTF-WebGL-PBR/blob/master/shaders/pbr-frag.glsl#L151
texture2D(u_brdfLUT, vec2(pbrInputs.NdotV, 1.0 - pbrInputs.perceptualRoughness))

while all other implementations use just roughness (no "1.0 - roughness" flip):

  1. Google's Filament: https://github.com/google/filament/blob/master/shaders/src/light_indirect.fs#L58
  2. LearnOpenGL: https://github.com/JoeyDeVries/LearnOpenGL/blob/master/src/6.pbr/2.2.2.ibl_specular_textured/2.2.2.pbr.fs#L162
  3. McNopper's OpenGL: https://github.com/McNopper/OpenGL/blob/master/Example33/shader/brdf.frag.glsl#L46
  4. SaschaWillems' Vulkan-glTF: https://github.com/SaschaWillems/Vulkan-glTF-PBR/blob/master/data/shaders/pbr.frag#L175
  5. SaschaWillems' Vulkan: https://github.com/SaschaWillems/Vulkan/blob/master/data/shaders/pbribl/pbribl.frag#L136

LUT files across the different implementation look similar (see below). This fact makes me wonder who has it right?
Our LUT:

Filament's LUT:

LearnOpenGL's LUT:

@lhog lhog changed the title [question] Are brdfLut sampling coordinates correct [question] Are brdfLUT sampling coordinates correct? Oct 12, 2018
@javagl
Copy link
Contributor

javagl commented Oct 13, 2018

A (very) wild guess: This might be related to the infamous "y-flipping" of OpenGL, which has already been discussed here: #16 . Iff this guess is correct, then the coordinates are flipped (intentionally) because the implementation sets UNPACK_FLIP_Y_WEBGL to false.

Side notes:

  • If the coordinates were really wrong, I think the rendered results would look vastly different.
  • Except for the size, there is no (noticable) difference between the LearnOpenGL LUT and our LUT (but I didn't compare the pixel values)
  • The reason for the different LUT in Filament remains to be explained....

@emackey
Copy link
Member

emackey commented Oct 15, 2018

I think so too. If the Y-flip is wrong on this LUT, smooth areas will appear rough and vice versa. So if an implementation gets it wrong, the sample models will make that obvious (for example Damaged Helmet would not be shiny at all).

Also, there are lingering questions about the correct colorspace of this LUT in #64. A new reference viewer is under development now, so this may change soon.

@lhog
Copy link
Author

lhog commented Oct 15, 2018

Thanks for the input, looks like Y-flip is indeed the reason.

I was questioning sRGB to linear space conversion myself as well. Usually it's only required for textures made by artists. I tried to go with or without colorspace conversion of this LUT and came to the conclusion that I like better the one with sRGB transformation (as it's in the code now).
@emackey you mentioned other viewer is under development. Is it on github? Can I have a link?

@javagl
Copy link
Contributor

javagl commented Oct 15, 2018

As far as I understood, the reference viewer is about to be developed in this fork/branch: https://github.com/ux3d/glTF-WebGL-PBR/tree/reference-viewer , but it only just started. Correct me if I'm wrong.

@lhog
Copy link
Author

lhog commented Oct 16, 2018

Guess this one is sorted out.
Thanks a lot for your inputs and closing it for now.

@lhog lhog closed this as completed Oct 16, 2018
emackey pushed a commit that referenced this issue Mar 3, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants