mrdoob / three.js

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

Reflector (WebGLRenderTarget): Fog Color problem in sRGB outputEncoding #24362

Open takahirox opened 2 years ago

takahirox commented 2 years ago

From https://github.com/mozilla/hubs/issues/5572

Describe the bug

Fog color in Reflector (WebGLRenderTarget) can mismatch the one in the main scene.

I made an online example to show the problem.

https://raw.githack.com/takahirox/three.js/MirrorFogExample/examples/webgl_mirror_fog.html

Screenshots

sRGB output encoding + fog. Fog color in the reflector looks brighter than the one in the main scene. (See the green wall.)

image

sRGB output encoding + no fog. Color in the reflector looks the same as the one in the main scene.

image

Linear output encoding + fog. Fog color in the reflector looks the same as the one in the main scene.

image

Live example

https://raw.githack.com/takahirox/three.js/MirrorFogExample/examples/webgl_mirror_fog.html

Expected behavior

Fog color in the reflector looks same as the one in the main scene even in sRGB outputEncoding.

Platform:

takahirox commented 2 years ago

Probably the root issue is color space conversion. In short, the fog color in the main scene doesn't seem to be applied color space conversion when rendering to the screen.

In the example, first rendering the scene including the fog to the reflector render target, and then rendering the scene including the reflector render target texture to the screen.

Three.js does color space conversion in shader (<encodings_fragment>) when rendering to the screen.

https://github.com/mrdoob/three.js/blob/b90f614fd59e1983d59dc24c1aa05c9c2c3886cd/src/renderers/shaders/ShaderChunk/encodings_fragment.glsl.js

When rendering to the screen the fog color in the reflector render target texture set to material.map is converted in shader. But please see the shader codes in ShaderLib directory. It first applys <encodings_fragment> and then <fog_fragment>. So, the fog color in the main scene is not converted.

https://github.com/mrdoob/three.js/blob/12f550e0f44f68e5b6c946dd242ee08e42d209e4/src/renderers/shaders/ShaderLib/meshphysical.glsl.js#L210-L211

Therefore, the fog colors in the reflector and in the main scene can mismatch.

An easy solution may be swap the order of <encodings_fragment> and <fog_fragment>. Is there any reasons why applying fog after the color space conversion?

This is the screenshot of sRGB output encoding + fog + swap the order. The fog color in the reflecor looks the same as the one in the main scene.

image

WestLangley commented 2 years ago

Is there any reasons why applying fog after the color space conversion?

I believe this explains the history, although at the moment, I am unable to locate the original discussion on the topic.

LeviPesin commented 2 years ago

Related: https://github.com/mrdoob/three.js/pull/23937

takahirox commented 2 years ago

Thanks @WestLangley @LeviPesin for pointing out the related threads.

donmccurdy commented 2 years ago

I expect there will be similar challenges with tone mapping and fog, too.

LeviPesin commented 1 year ago

Is this still an issue after https://github.com/mrdoob/three.js/pull/23937?

takahirox commented 1 year ago

Unfortunately the problem doesn't seem to be resolved yet

donmccurdy commented 1 year ago

Note that https://github.com/mrdoob/three.js/pull/23937 will require setting ColorManagement.legacyMode = false to have any effect. But I think there are other things going on here, this is tricky. 😕

takahirox commented 1 year ago

ColorManagement.legacyMode = false

image

https://raw.githack.com/takahirox/three.js/MirrorFogExample/examples/webgl_mirror_fog.html

takahirox commented 1 year ago

As I commented https://github.com/mrdoob/three.js/issues/24362#issuecomment-1188411765 , swapping the order of <encodings_fragment> and <fog_fragment> (and no fog color space conversion in CPU) seems to resolve the problem.

image

https://raw.githack.com/takahirox/three.js/MirrorFogExample2/examples/webgl_mirror_fog.html

https://github.com/takahirox/three.js/commit/4788ed5e5b1dc33a4c045e1ebfc7114bd77b2933

If I understand correctly, the reason why the current order <encodings_fragment> and then <fog_fragment> was to match fog color and clear color. But with #23937 we do clear color space conversion so it may be ok to swap the order now?

WestLangley commented 1 year ago

Fog should be applied in scene-referred linear color space.

Since tone mapping converts from scene-referred color space to display-referred color space, the fog chunk should precede tone mapping.

#include <fog_fragment>
#include <tonemapping_fragment>
#include <encodings_fragment>

Ideally, we should adhere to that convention -- and make whatever changes are required to accommodate it.

/ping @donmccurdy (color management) /ping @bhouston (legacy conventions)

donmccurdy commented 1 year ago

Fog should be applied in scene-referred linear color space.

...

#include <fog_fragment>
#include <tonemapping_fragment>
#include <encodings_fragment>

Yes, agreed. This would be simpler and easier to reason about than the current ordering. For implementation purposes it would be clear improvement, and makes bugs like this easier to address.

The troublesome part is breaking convention, and explaining that to users. Consider a user writing this today:

// (A) ColorManagement.legacyMode = true
renderer.outputEncoding = sRGBEncoding;
scene.fog.color.setRGB( 40, 40, 40 );              // sRGB → Linear-sRGB 🚨
scene.background.setRGB( 40, 40, 40 );             // sRGB
renderer.setClearColor( new Color( 40, 40, 40 ) ); // sRGB

material.color.setRGB( 40, 40, 40 );               // Linear-sRGB
light.color.setRGB( 40, 40, 40 );                  // Linear-sRGB

Or, this:

// (B) ColorManagement.legacyMode = false
renderer.outputEncoding = sRGBEncoding;
scene.fog.color.setRGB( 40, 40, 40 );              // Linear-sRGB
scene.background.setRGB( 40, 40, 40 );             // Linear-sRGB
renderer.setClearColor( new Color( 40, 40, 40 ) ); // Linear-sRGB

material.color.setRGB( 40, 40, 40 );               // Linear-sRGB
light.color.setRGB( 40, 40, 40 );                  // Linear-sRGB

Under legacyMode = true (A) it's awkward. Fog is related to background and clear color — none are affected by lighting, and you probably want them all to match — but now they're specified in different color spaces.

Under legacyMode = false (B) this should not be a breaking change. We still assume all RGB components are Linear-sRGB, and convert them internally as needed, when setting background or clear color. Only the internal conversion would change for fog.


Most users (and examples) are currently in case (A). I wish there were a gentler way to make the change under that scenario, but I'm not sure there is.

donmccurdy commented 1 year ago

^My concerns above are addressed in r152, as color management is now enabled by default.

Discussion continued in: