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

BRDF LUT uses the sRGB format, by design? #64

Closed
ziriax opened this issue Feb 1, 2018 · 10 comments
Closed

BRDF LUT uses the sRGB format, by design? #64

ziriax opened this issue Feb 1, 2018 · 10 comments
Labels

Comments

@ziriax
Copy link
Contributor

ziriax commented Feb 1, 2018

The textures seem to be loaded using the following formats:

textures/brdfLUT.png  ->sRGB
models/DamagedHelmet/glTF/Default_emissive.jpg  ->sRGB
models/DamagedHelmet/glTF/Default_normal.jpg  ->RGBA
models/DamagedHelmet/glTF/Default_AO.jpg  ->RGBA
models/DamagedHelmet/glTF/Default_albedo.jpg  ->sRGB
models/DamagedHelmet/glTF/Default_metalRoughness.jpg  ->RGBA

the BRDF lookup table seems to be using the sRGB color space. Is this by design?

@emackey
Copy link
Member

emackey commented Apr 24, 2018

I've been wondering this too. @snagy Would you be willing to comment? In particular:

https://github.com/KhronosGroup/glTF-WebGL-PBR/blob/0d1a32003253067ab98e6aaca9e8028028a4ef65/shaders/pbr-frag.glsl#L150

Is this conversion simply because the PNG behind this is stored in sRGB?

In Cesium I have a branch open where I've applied the Linear/sRGB conversions. But I didn't apply this conversion on the BRDF LUT, because in Cesium it is computed in a shader. I think this makes my dev branch have much more "rim lighting" than master, and of course adding the conversion puts it back the way it was. I'm trying to figure out if the conversion is supposed to be there, even for textures that have been computed as opposed to read in.

@snagy
Copy link
Contributor

snagy commented Apr 24, 2018

@emackey In your case where you're computing it in a shader and putting it into a texture, you shouldn't be converting from sRGB unless you're calculating the values in sRGB space or converting before you store them in a texture (you might have reason to do this if you want to take advantage of sRGB's adjusted precision). Glancing at your code, I don't think you're doing either of those.

IIRC, the BRDF LUT from IBLBaker is in sRGB. I don't remember how I determined this, so I could definitely be wrong here.

@ziriax
Copy link
Contributor Author

ziriax commented Sep 25, 2018

I've been doing some more testing, comparing GLTF with Substance Painter, and the results are much closer when interpreting the GLTF BRDFLUT as linear.

Especially for high roughness values, the GLTF rendering is much darker. Also at the edges, SP is a lot brighter for any roughness.

As already mentioned by @snagy , for testing, I replaced the BRDF with a 128-bits linear-space DDS (generated with IBLBaker) in Maya2glTF, and got nearly identical result when using it, giving strong hints that indeed the bundled 24-bit BRDFLUT.PNG is linear.

If this is indeed the case, then the 24-bit RGB BRDFLUT.PNG should be converted to sRGB, to avoid banding?

@lhog
Copy link

lhog commented Oct 29, 2018

I think applying sRGB --> RGB transform is certainly wrong.
In my pet implementation I've integrated BRDF LUT calculation right before PBR is activated.
BRDF calculated online (before the actual PBR renderpasses) and loaded from the LUT texture yield same result only if sRGB to linear transform is not used. Color space transformation (as it's done currently) gives completely different visual representation.

sRGB-->RGB transformation is off:
screen00026
sRGB-->RGB transformation is on (current default):
screen00027

That said as you might notice the "correct" LUT application produces rather unpleasing visuals. Glancing angles seem to produce too much IBL reflectivity. LUT sRGB transformation kinda hides these artifacts, but it doesn't make it use correct in the first place.

To make visuals look better we might want to change the way BRDF LUT is calculated. Here bellow I put together some of the options I found in different implementations as far as generation of BRDF LUT is concerned. I understand choosing one or another is mostly a matter of taste as there's no exact science behind these formulas:

#define G_OPTION 1

// Geometric Shadowing function
float G_SchlicksmithGGX(float dotNL, float dotNV, float roughness)
{
	#if (G_OPTION == 1)
		float k = (roughness * roughness) / 2.0;
	#elif (G_OPTION == 2)
		float k = (roughness + 1.0);
		k = k * k;	
	#elif (G_OPTION == 3)
		float k = (roughness + 1.0);
		k = k * k / 8.0;		
	#elif (G_OPTION == 4)
		float k = (0.5 + 0.5 * roughness);
		k = k * k;
	#endif
	float GL = dotNL / (dotNL * (1.0 - k) + k);
	float GV = dotNV / (dotNV * (1.0 - k) + k);
	return GL * GV;	
}

Please find the in-game screenshots of 4 options above:
screen00032
screen00033
screen00034
screen00035

Note in all 4 cases no sRGB-->RGB transformation is applied to BRDF LUT

Also find the resulting LUT textures in the respecting order:
brdf_32983
brdf_33423
brdf_33784
brdf_34169

For your reference, here's how the current BRDF LUT looks factually if it's applied with sRGB-->RGB transform:
brdf_42793

@lhog
Copy link

lhog commented Oct 29, 2018

More screenshots of 4 options above:

Sphere with roughness of 0.0:
screen00036
screen00037
screen00038
screen00039

Sphere with roughness of 0.5:
screen00040
screen00041
screen00042
screen00043

As far as my person taste, I can conclude that options number 3 or 4 give most realistic picture:

		float k = (roughness + 1.0);
		k = k * k / 8.0;		
	#elif (G_OPTION == 4)
		float k = (0.5 + 0.5 * roughness);
		k = k * k;```

@ziriax
Copy link
Contributor Author

ziriax commented Oct 31, 2018

@MiiBond I was reading your nice 'thin transparency' extension, and noticed:

Roughness of 0, 0.5 and 1.0 using renderer in Adobe Dimension. Note that an issue in Dimension at the time of writing is causing rough surfaces to appear too dark.

Might this be related to this issue? When using the GLTF PBR code of this repo, I noticed that rough surfaces are a lot darker than say Substance Painter...

@lhog
Copy link

lhog commented Oct 31, 2018

@ziriax, I'd kindly advise to trust your senses and experience rather than some other implementation. It can be a big name or a vendor, but still it doesn't guarantee that implementation in some particular product is done right. Remember: PBR is an approach, not a prescribed recipe.

Please look at the screenshots above and tell me what picture looks right to you.

@ziriax
Copy link
Contributor Author

ziriax commented Oct 31, 2018

@ihog Thanks for the advice, in our case it doesn't matter, our artists use Substance Painter, and we try to match that as much as possible. It is difficult to judge from your scenes what would be the "correct" one (in the sense that "correct" would mean closest to a ground truth that we don't have here), but I certainly follow your reasoning.

@emackey
Copy link
Member

emackey commented Oct 31, 2018

Remember: PBR is an approach, not a prescribed recipe.

That said, there's interest in glTF providing a prescribed recipe for its models to look the same in all implementations. There's a new reference viewer effort underway in the UX3D reference-viewer branch that will eventually start producing reference images for other glTF implementations to compare.

@ziriax
Copy link
Contributor Author

ziriax commented Oct 31, 2018

This issue can be settled when the person that created the BRDFLUT checks how the original BRDF was converted to PNG (was sRGB conversion done or not?)

If he/she started with the linear DDS from IBLBaker, then he/she did not convert it to sRGB, since rendering with this linear DDS gives different results from using the PNG with sRGB conversion.

Leaving out the sRGB conversion gives identical results.

Of course if IBLBaker was indeed used to generate the BRDF, nobody says that tool is correct...

emackey pushed a commit that referenced this issue Mar 3, 2021
added option to "deselect" material variant (GSVN-148)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

5 participants