mrdoob / three.js

JavaScript 3D Library.
https://threejs.org/
MIT License
102.31k stars 35.35k forks source link

The color of the environment map is displayed brighter than r98 #15285

Closed cx20 closed 4 years ago

cx20 commented 5 years ago

I tried webgl_loader_gltf sample with three.js r98 and r99. As a result of comparison, r99 seems to be slightly brighter than r98. I think that it is necessary to specify THREE.sRGBEncoding in the environment map if it is over r99.

However, applying THREE.sRGBEncoding to the environment map will make the model darker, so I think further actions are necessary. Unfortunately I do not know how to do it.

three.js r98 result: image

three.js r99 result: image

three.js r99 + sRGBEncoding result: image

WestLangley commented 5 years ago

I think that it is necessary to specify THREE.sRGBEncoding in the environment map if it is over r99.

Correct.

We need to implement

renderer.outputEncoding = THREE.sRGBEncoding;

to correctly compensate. As a work-around, you can set

renderer.gammaOutput = true;
renderer.gammaFactor = 2.2;

The model is also darker since the env map encoding has been changed.

cx20 commented 5 years ago

@WestLangley Does renderer.outputEncoding mean https://github.com/mrdoob/three.js/issues/11337 I do not understand gamma space and sRGB so much, so I need to study.

WestLangley commented 5 years ago

Yes.

donmccurdy commented 5 years ago

In summary β€” if the environment map is stored as sRGB, then setting texture.encoding = THREE.sRGBEncoding will mean the model is less brightly lit, but that's correct. We can adjust other lighting in the scene to compensate.

HDR environment maps are probably linear, not sRGB, so don't blindly set the encoding though. :)

cx20 commented 5 years ago

I think it would be nice to have some document that tells what format/encoding the environment map is supposed to have. The following is the result of examining which format/encoding is specified in the sample code.

Texture Name Extension Format Encoding
textures/cube/Bridge2 .jpg RGBFormat ?
textures/cube/MilkyWay .jpg RGBFormat ?
textures/cube/Park2 .jpg RGBFormat ?
textures/cube/Park3Med .jpg ? ?
textures/cube/pisa .png ? GammaEncoding
textures/cube/pisaHDR .hdr ? ?
textures/cube/pisaRGBM16 .png ? RGBM16Encoding
textures/cube/skybox .jpg RGBFormat ?
textures/cube/skyboxsun25deg .jpg ? ?
textures/cube/SwedishRoyalCastle .jpg RGBFormat ?

I think THREE.LinearEncoding will be applied to environment maps that probably do not specify encoding. For environment maps other than HDR, is it necessary to specify THREE.sRGBEncoding explicitly as encoding?

bhouston commented 5 years ago

Cubemaps generally should not be LDR images such as this. Basically this example starts off wrong.

They should be linear HDR images in EXR or HDR or the encoded PNG/RGBM16 format.

Thus the only high quality envmaps are pisaHDR and pisaRGBM16 and the *.exr that Richard Monette added.

cx20 commented 5 years ago

@bhouston I am not a CG professional, so I do not understand the problem much, but what is the reason why LDR should not be used for Cubemaps? Is it a matter of quality?

MEBoo commented 5 years ago

to correctly compensate. As a work-around, you can set

renderer.gammaOutput = true;
renderer.gammaFactor = 2.2;

@WestLangley yesterday I found myself this workaround... the CubeCamera texture was too bright, so I applied THREE.sRGBEncoding and set the other 2 parameters.

A question... I found that also Material.color should be RGB and not sRGB, so I did Three.Color(sRGB color got from RAL).convertSRGBToLinear(). I looked the convertSRGBToLinear code and this is actually using the value "2.4" as it should for a total gamma of 2.2 (as I read from WIKI regarding gamma correction). So my question is: does "gammaFactor = 2.2" equals to the 2.4 standard value for sRGB, so It's internally managed to use the 2.4 constant, or I should write "gammaFactor = 2.4" to adjust my scene?

bhouston commented 5 years ago

renderer.gammaOutput and renderer.gammaFactor should be obsoleted and replaced with renderer.outputColorSpace = sRGB, gamma22, etc. That is the correct solution. We support sRGB -> linear and linear -> sRGB correctly, thus we should just expose that correctly.

WestLangley commented 5 years ago

@MEBoo The sRGB transfer function (gamma curve) consists of a linear segment plus a exponential segment with a exponent of 2.4. To approximate that curve with a simple exponential, use exponent 2.2.

MEBoo commented 5 years ago

@bhouston ok... so now gammaFactor=2.2 acts as an sRGB right conversion

@WestLangley yes I know, I was asking if THREE does an approximation whes using gammaFactor=2.2 or if it does the "true" conversion

Thanks everybody

bhouston commented 5 years ago

yes I know, I was asking if THREE does an approximation whes using gammaFactor=2.2 or if it does the "true" conversion

We have the true code here and we use it currently in certain cases. renderer.gammaInput and renderer.gammaOutput are old methods of doing things that do not do the proper conversion.

Here is the correct conversion code (or at least high quality) and it is used on texture.encoding = sRGB: https://github.com/mrdoob/three.js/blob/dev/src/renderers/shaders/ShaderChunk/encodings_pars_fragment.glsl.js

MEBoo commented 5 years ago

@bhouston ok so for renderer.outputColorSpace we need to wait R99 ? For CubeCamera I set the resulting texture.encoding=sRGB ... has this been changed to automatically set it to sRGB when the renderer output is sRGB?

donmccurdy commented 5 years ago

@MEBoo https://github.com/mrdoob/three.js/issues/11337 is still open β€” the changes we're discussing are not implemented yet.

Note that .convertSRGBToLinear() takes a "gamma" argument, where you can pass 2.2, in the meantime.

MEBoo commented 5 years ago

@MEBoo #11337 is still open β€” the changes we're discussing are not implemented yet.

Note that .convertSRGBToLinear() takes a "gamma" argument, where you can pass 2.2, in the meantime.

Thanks, hope the changes are fast to be implemented, because now threse is a rendering color inconsistency.

But convertSRGBToLinear (from what i read on the source) doen't take "gamma" argument ... instead convertGammaToLinear() does.

For now I leave all as is: gammaFactor = 2.2 gammaOutput= true sRGBEncoding for input textures sRGBEncoding for CubeCamera textures convertSRGBToLinear() for every THREE.Color (hex web color that I get from real contest RAL)

and I'll wait the optimizationw Thanks you all

mrdoob commented 5 years ago

@WestLangley should we set encoding to THREE.sRGBEncoding automatically when loading a jpeg like we do with format already?

WestLangley commented 5 years ago

That would not be appropriate for normal, bump, roughness, metalness maps, etc.

bhouston commented 5 years ago

That would not be appropriate for normal, bump, roughness, metalness maps, etc.

I think it may be correct to have them sRGB if the format is a normal jpeg or a png. If the image was created in photoshop it will be saved as sRGB or the similar Adobe RGB.

Now if the image was made in some other tool, then maybe it happens to be linear. Basically we should check if Substance saves out linear jpg/pngs, etc.

I wouldn't be surprised if the correct solution, decoding sRGB for all jpgs and pngs, is not done by other tools such as UE4 and Unity, just because being correct isn't that different from doing it incorrectly in terms of the final results. Who really notices a slight shift in metalness in the mid range, etc.

donmccurdy commented 5 years ago

Now if the image was made in some other tool, then maybe it happens to be linear. Basically we should check if Substance saves out linear jpg/pngs, etc.

Substance does output linear jpg/png maps. And, for example, glTF requires maps not containing color information to be linear. As discussed in https://github.com/mrdoob/three.js/issues/11337#issuecomment-440795075, I think a robust option would be to change the default value of .encoding to THREE.AutoEncoding1, or even just null. This leaves us free to infer the most likely encoding from the map slot to which the texture is assigned.

1 or .colorSpace and THREE.AutoColorSpace, preferably.

WestLangley commented 5 years ago

This leaves us free to infer the most likely encoding from the map slot to which the texture is assigned.

Personally, I don't think three.js should ever guess.

donmccurdy commented 5 years ago

We're currently guessing that all textures are linear, and it's not a very accurate guess. πŸ˜‰ How would we avoid that? It's possible to read EXIF and XMP metadata in JS, including colorspace, but I'm not sure what the performance implications would be..

As a default, I do agree sRGB might be better than linear, if we're making that change at the same time as other corrections to ease the transition. Having all of the loaders automatically mark their normal/metalness/roughness maps as linear would help. Users manually loading a normal map are probably less common, and more likely to know the right colorspace, than users manually loading a color .map. So having the right default for .map, sRGB, could be nice.

mrdoob commented 5 years ago

I think I agree. sRGB sounds like a better default for me too.

It's likely that people that have not yet studied color spaces are using images in sRGB. For people coming from Substance they have probably heard about color spaces at that point and they'd understand why they'd have to change the texture encoding.

WestLangley commented 5 years ago

This leaves us free to infer the most likely encoding from the map slot to which the texture is assigned.

Personally, I don't think three.js should ever guess.

We're currently guessing that all textures are linear, and it's not a very accurate guess.

A guess is not a default. A guess is inferring something different than the user specified because you think you know better.

three.js should do what the user specifies, as it is doing now.

If you insist on guessing, then you can default encoding or colorSpace to undefined, in which case the renderer can go ahead and guess. If the user specifies a specific encoding, then you should honor that.

donmccurdy commented 5 years ago

If the user specifies a specific encoding, then you should honor that.

Completely agreed. I only meant to suggest guessing when the user had not specified.

Sounds like sRGB as the default, and having loaders set linear on non-color maps as needed, may be a better solution than creating some new β€œauto” mode.

bhouston commented 5 years ago

My two cents:

I would suggest that roughness, metalness, glossiness, normal, bump just be linear always. It sometimes will be slightly wrong but I doubt people will notice.

Only the color channels should allow for decoding, either sRGB, or linear. I would suggest handling sRGB/etc decoding in Javascript and assume within the shaders that all uniforms are in linear color space. The last thing we need is more complexity in the shaders, especially a decode per pixelshader instantiation that is exactly the same each time (and another DEFINE or uniform - there are too many of those already.)

MEBoo commented 5 years ago

It's likely that people that have not yet studied color spaces are using images in sRGB. For people coming from Substance they have probably heard about color spaces at that point and they'd understand why they'd have to change the texture encoding.

Just working today with Substance B2M (3.1) and it has global options to load/export all maps Linear or sRGB. You can't choose to export the diffuse map sRGB and normal map in Linear... everything linear or everything sRGB. Default to sRGB But I don't know if these options (color input gamma / color output gamma) are only for the internal viewer or for the output too

Are the threejs maps using the encoding argument? I have everything manually set to sRGB... hope this option will remain

donmccurdy commented 5 years ago

Only the color channels should allow for decoding, either sRGB, or linear.

Yes, that's true now. I'm OK with this.

Just working today with Substance B2M (3.1) and it has global options to load/export all maps Linear or sRGB. You can't choose to export the diffuse map sRGB and normal map in Linear... everything linear or everything sRGB.

I haven't used B2M, but Substance Painter definitely gives more options than this... there are presets for glTF metal/rough and spec/gloss PBR, for example, where the base color is sRGB and normal map would be linear. Does B2M not have that?

Are the threejs maps using the encoding argument? I have everything manually set to sRGB..

In three.js the color-related maps (like .map and .emissiveMap) do use the encoding argument. Others like .normalMap do not. If you're happy with the result you're getting now, I don't think we're proposing to change that.

MEBoo commented 5 years ago

In three.js the color-related maps (like .map and .emissiveMap) do use the encoding argument. Others like .normalMap do not. If you're happy with the result you're getting now, I don't think we're proposing to change that.

Checked substance B2M export output and the diffuse is sRGB but the other maps are linear. Just found, as you say, that threejs doesn't use encoding for the roughnessMap, and for my use case is not a problem since B2M exports that map as linear.

Please leave the envMap connected to the encoding parameter if you plan changes (actually I'm using it generated from the CubeCamera and needs to be decoded from sRGB).

mrdoob commented 5 years ago

If you insist on guessing, then you can default encoding or colorSpace to undefined, in which case the renderer can go ahead and guess. If the user specifies a specific encoding, then you should honor that.

Would it make sense to set encoding to undefined by default and then in the renderer use sRGBEncoding if gammaOutput is set to true? Otherwise all the current examples using textures get darker.

donmccurdy commented 5 years ago

Would it make sense to set encoding to undefined by default and then in the renderer use sRGBEncoding if gammaOutput is set to true?

That might overload the meaning of gammaOutput in a weird way, and even the proposed replacement name (colorSpaceOutput) wouldn't be quite consistent. Moreover, the user could be doing gamma-correction on the output after post-processing, in which case (1) they still want sRGB inputs, and (2) gammaOutput should be false.

Another option would be simply to change the default values for encoding and gammaOutput at the same time, which reduces the overall change, and treating THREE.Color as sRGB by default would reduce the difference even more. Presumably we don't want all of that controlled by a gammaOutput flag, which is the motivation for my renderer.workflow = 'linear' suggestion β€” it allows us to make multiple related changes to achieve a linear rendering workflow.

I'm working on a PR for A-Frame that will treat all inputs (THREE.Color, THREE.Texture) as sRGB by default, excluding the non-color maps. We'd also have gammaOutput on by default. The net visual effect should be much smaller in that case than just changing texture encoding by itself β€” let me put together some before/after screenshots from our examples.

bhouston commented 5 years ago

I think inputGamma and outputGamma need to be killed. And replaced with outputEncoding and outputColorSpace (or equivalents.)

-ben

On Thu, Nov 29, 2018 at 11:44 AM Don McCurdy notifications@github.com wrote:

Would it make sense to set encoding to undefined by default and then in the renderer use sRGBEncoding if gammaOutput is set to true?

That might overload the meaning of gammaOutput in a weird way, and even the proposed replacement name (colorSpaceOutput) wouldn't be quite consistent. Moreover, the user could be doing gamma-correction on the output after post-processing, in which case (1) they still want sRGB inputs, and (2) gammaOutput should be false.

Another option would be simply to change the default values for encoding and gammaOutput at the same time, which reduces the overall change, and treating THREE.Color as sRGB by default would reduce the difference even more. Presumably we don't want all of that controlled by a gammaOutput flag, which is the motivation for my renderer.workflow = 'linear' suggestion β€” it allows us to make multiple related changes to achieve a linear rendering workflow.

I'm working on a PR for A-Frame that will treat all inputs (THREE.Color, THREE.Texture) as sRGB by default, excluding the non-color maps. We'd also have gammaOutput on by default. The net visual effect should be much smaller in that case than just changing texture encoding by itself β€” let me put together some before/after screenshots from our examples.

β€” You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/mrdoob/three.js/issues/15285#issuecomment-442905107, or mute the thread https://github.com/notifications/unsubscribe-auth/AAj6_euhlBPvD7Mf7lF8jOW67uGQacmaks5u0A7ugaJpZM4YrV3b .

-- Ben Houston, CTO M: +1-613-762-4113 bhouston@threekit.com Ottawa, Canada

http://threekit.com/ ThreeKit Visualization Platform: 3D, 2D, AR, VR http://https//threekit.com/

Mugen87 commented 4 years ago

I think inputGamma and outputGamma need to be killed.

Solved via #18108 and #18127.

tsuchan commented 4 years ago

Hi... I use GLTF files in my project and needed "renderer.gammaOutput = true;" to avoid dark objects. After updating ThreeJS to r112, what's the new method to correct the gamma issue?

WestLangley commented 4 years ago

@tsuchan See https://github.com/mrdoob/three.js/wiki/Migration-Guide

renderer.outputEncoding = THREE.sRGBEncoding; // alternate: THREE.GammaEncoding
tsuchan commented 4 years ago

@tsuchan See https://github.com/mrdoob/three.js/wiki/Migration-Guide

Thanks very much - I will try.

Eketol commented 4 years ago

Hi, when the outline effect is enabled, the scene's background gets darker. This didn't occur with r111. See example: https://jsfiddle.net/Eketol/r7w2vkhx/ (click to toggle the outline effect). Maybe I'm missing something? I found that if I comment out the scene's texture encoding (sceneTexture.encoding = THREE.sRGBEncoding;), the background looks lighter, but then it gets fixed when the outline effect is enabled. So it looks like the gamma correction is being re-applied to the background?

WestLangley commented 4 years ago

@Eketol It would be reasonable to set renderer.outputEncoding = THREE.LinearEncoding and then apply a Composer tone mapping pass at the end. If you need help doing that, see https://discourse.threejs.org.

Eketol commented 4 years ago

Thanks for your quick response @WestLangley. Does that mean it's not a bug, but from now we have to use an extra renderer pass to get it working? Will the pass have a performance issue or it would be equivalent to use renderer.outputEncoding = THREE.sRGBEncoding;? I guess there must be a reason to change the code behind renderer.gammaOutput = true;, but it's a bit frustrating to suddenly lose things that had been working in previous versions.

donmccurdy commented 4 years ago

I guess there must be a reason to change the code behind renderer.gammaOutput = true;, but it's a bit frustrating to suddenly lose things that had been working in previous versions.

The change from previous versions is different syntax for the same thing β€” renderer.outputEncoding now does what renderer.gammaOutput used to do. Either way, if you have post-processing effects, the default (linear) should be used and sRGB correction should be done as a post-processing pass instead.

Another way of thinking about it would be this: all lighting and post-processing effects work best on linear colors, so sRGB encoding should be the last thing that happens before displaying to the screen. If you don't have post-processing you can use .outputEncoding to do that, and if you do have post-processing then the renderer should give linear output to the post-processing stack, and the last post-processing pass should be sRGB encoding.

Eketol commented 4 years ago

Thanks for the explanation! I thought the change also implied a different method and that was causing the dark textures issue. I wonder why it was fine in previous versions? Thanks again, to you both for the prompt replies.

donmccurdy commented 4 years ago

The method changed only very slightly, gammaFactor=2.2 was always an approximation for sRGB. I would be quite surprised if that were the cause. But I'm also surprised that it worked in previous versions, honestly.

Eketol commented 4 years ago

@donmccurdy maybe it could be some caching issue?: Current r112: https://jsfiddle.net/Eketol/r7w2vkhx/ Same code with r111 library and added renderer.gammaOutput = true; : https://jsfiddle.net/Eketol/ow07ukxq/ (click to render with/without postprocessing)

EDIT: Sorry maybe I didn't explain myself well enough, so I have created #18495 to avoid keep using this closed issue and to explain myself better.