mrdoob / three.js

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

Washed out colors w/ color-encoding despite conversion #19169

Closed drcmda closed 2 years ago

drcmda commented 4 years ago
Description of the problem

606daf2938efc1f282be99d4b4bc8e833b437064

left: threejs, right: figma - same rgb colors

gl.toneMapping = THREE.ACESFilmicToneMapping
gl.outputEncoding = THREE.sRGBEncoding

causes washed out colors despite converting them to linear color space

 new THREE.Color(color).convertSRGBToLinear()

And here is a reduced testcase: codesandbox

The outer ring should have the same color as the background (#fff9be), but it appears in a muddy gray. It works without filmic tonemapping, but now gamma is off and highlights blow out.

I've first posted on discourse (https://discourse.threejs.org/t/washed-out-colors-with-acesfilmic-despite-convertsrgbtolinear-and-srgbencoding/14080) to make sure that it's not my fault, but it seems like a bug.

Three.js version
sciecode commented 4 years ago

I'm not sure if this is actually a bug, AFAIK the inverse ACESFilmicToneMapping function should also be considered when picking the target color.

drcmda commented 4 years ago

there's a color picker in the sandbox, it just seems as if you cant have any vibrant color at all with a tone mapping that prevents blown out highlights. i wonder what's the common method that ppl use, but i always see these two together: sRGB and Filmic.

donmccurdy commented 4 years ago

Yeah sorry, my reply on Discourse was probably incorrect. You can see the same effect in Blender:

Filmic: Screen Shot 2020-04-19 at 11 48 23 AM

Standard: Screen Shot 2020-04-19 at 11 48 31 AM

The sRGB→linear→sRGB conversion exists because the renderer operates on linear values; it is not intended to change a scene's look, but to get the lighting math objectively correct for the specified colors. Tone mapping, on the other hand, is often used to change a scenes "look" and "mood", and inherently does change colors.

I'm not sure what would be considered best practice, here, or if there is something we should change. If your workflow relies on Blender, where the default tonemapping is already Filmic, then you'll get consistent results with Filmic in three.js. Presumably that's the basic process for a fullscreen game. For webdev, where you need WebGL content to blend with the rest of the experience, you may want to alter three.js' input colors so you get better perceived dynamic range while matching colors to the CSS content. That sounds reasonable to me but I'm not confident saying it's best practice: in particular I wonder if it would exacerbate banding, and make dither more necessary, e.g. when applied to textures.

donmccurdy commented 4 years ago

This is probably not a bug to be fixed, then, but here are some suggestions:

  1. There is a per-material material.toneMapped = false option. For specific objects where tonemapping is unnecessary and matching CSS content is important, that should be a good option. In particular, MeshBasicMaterial+.toneMapped=false would be a good choice for UI overlays.
  2. It might be useful to have additional functions like ColorUtils.convertFromToneMapping(color, ACESFilmicToneMapping) and convertToToneMapping. That would enable the workflow described above. Whether it's a good workflow, I don't know. 🙂
sciecode commented 4 years ago

there's a color picker in the sandbox, it just seems as if you cant have any vibrant color at all with a tone mapping that prevents blown out highlights. i wonder what's the common method that ppl use, but i always see these two together: sRGB and Filmic.

I don't think there's broad use of tonemapping to be honest, but when people use it, I doubt they have a target color in mind. The workflow for your use-case doesn't exist yet, but that's not to say your use-case is not valid.

There is a per-material material.toneMapped = false option. For specific objects where tonemapping is unnecessary and matching CSS content is important, that should be a good option. In particular, MeshBasicMaterial+.toneMapped=false

Yes, this would work well when objects have no need for toneMapping.

It might be useful to have additional functions like ColorUtils.convertFromToneMapping(color, ACESFilmicToneMapping) and convertToToneMapping. That would enable the workflow described above. Whether it's a good workflow, I don't know.

Perhaps a ColorUtils function would make sense, but it would be little more complicated. As we also would need to provide the target Exposure and White Point (when applicable). I'm not sure as well if it makes for a good workflow, but it is the required solution for working with tone mappped materials.

drcmda commented 4 years ago

i wasn't aware of material.toneMapped = false but in my case writing <meshBasicMaterial toneMapped={false} /> is an easy fix, and it worked perfectly. and yes, this was more or less intended for basic one-color overlay shapes that match figmas rgb, so at least my usecase is solved, thank you very much!

should i leave this open? the discussion about conversion tools should be interesting as well ...

WestLangley commented 4 years ago

Tone mapping, on the other hand, is often used to change a scenes "look" and "mood", and inherently does change colors.

I have never seen that. You are likely confusing tone mapping with color grading or color correction.

It is true that ACES Filmic tone mapping causes a hue shift.

WestLangley commented 4 years ago

@drcmda

In three.js, scene background color (set either via renderer.setClearColor() or scene.background = color) does not respond to tone mapping or encoding settings; textured backgrounds do, however.

Also, you should be applying exposure control prior to tone mapping:

renderer.toneMappingExposure = 0.5;

Tone mapping alone will not "fix" an over-bright scene.

looeee commented 4 years ago

AFAIK the inverse ACESFilmicToneMapping function should also be considered when picking the target color.

It might be useful to have additional functions like ColorUtils.convertFromToneMapping(color, ACESFilmicToneMapping) and convertToToneMapping.

Most of the tone mapping functions use a saturate (clamp) operation which means the functions are lossy and hence not invertible. That makes sense since the functions map high-precision HDR color to low-precision LDR color.

An inverse tone map should work for CSS colors though since they are LDR (I guess you would just drop the saturate and then invert the function).

However, I'm not sure that tone mapping makes sense at all if you're already working with LDR CSS colors. The goal of tone mapping is a perceptually plausible mapping from HDR to LDR. There's no need for that when already working with LDR colors.

Instead, it makes more sense to convert the CSS color to linear space, render, then convert back to sRGB.

I've set up a pen for testing CSS color matching with the scene background color in case it's useful to anyone.

drcmda commented 4 years ago

the challenge was that i wanted some of the geometry appear flat while still having the occasional object that bites through. the toneMapped trick works really well for that imo, i just never saw it before.

looeee commented 4 years ago

I'm coming to the opinion that Linear tonemapping is basically the worst possible default. It's not really a tone mapping function at all since the values remain unbounded. In other words, it doesn't map colors from HDR to LDR, it maps colors from HDR to HDR.

From the substance painter docs:

Linear: The output color is not clamped to 0 to 1 for this type only. This is optimal for when implementing some effect in the HDR space on the application side after the applying of the effects. We do not recommend this unless you have some specific reason for using it, because the high luminance components will be completely lost and blown out highlights will occur if linear mapping is used as the final screen output as is.

In other words, the only reason for using linear tone mapping is when you need to output a HDR image for use in post (or maybe to output for a HDR display).

Instead, ACESFilmic should be the default. See @bhouston's comment in support of this position: https://github.com/mrdoob/three.js/pull/15277#issuecomment-440343241.

Then you can disable tone mapping on a per-material basis when you need to match CSS colors.

drcmda commented 4 years ago

@donmccurdy mentioned a new renderer arg called colorManagement, this could switch encoding to sRGB, tone maping to filmic, and if possible, convert colors and textures. Then you'd avoid breaking peoples projects as @bhouston feared

looeee commented 4 years ago

Then you'd avoid breaking peoples projects as @bhouston feared

Some people would need to set renderer.toneMapping = LinearToneMapping when upgrading to maintain backward compatibility. That's a pretty small change, and we frequently require changes like this between versions. I think it would be worth it.

Auto color management would be even better, but improved defaults would still be good enough.

Once we reach a consensus on how to proceed with this - either provide better defaults or introduce auto color management - my vote would be to make all the changes in a single release.

If we change the defaults rather than auto management, then these settings should change:

Post-processing needs color management too:

OR

Possibly the EffectComposer should default to a half float buffer as well.

Of course, this is all a lot of work. I'm not claiming to know the best solution here, the changes I've suggested are just what my research has turned up so far and may not all be correct.

Perhaps the best approach would be to set up e2e testing of color management. Once we identify the workflows we need to support, we can set up tests and then work to ensure they are correct. For example:

Each of these tests should have two variants - WebGLRenderer and EffectComposer.

donmccurdy commented 2 years ago

I've just opened a high-level roadmap issue for color management tasks in https://github.com/mrdoob/three.js/issues/23614. @looeee I think the checklist there covers many of your points above, please feel free to make suggestions as well. However — I'm not convinced that tone mapping should be enabled by default, and in any case it appears that was decided against in https://github.com/mrdoob/three.js/pull/19197.

Compared to the the Substance Painter documentation excerpt above, Blender and Maya use different (and IMHO clearer) terminology. When these tools refer to a "view transform" they're describing a process that includes both color space conversion and tone mapping. Maya does not enable a tone map by default. Blender does ("Filmic"), but the option to disable it ("Standard") is not discouraged at all and is described as follows:

Standard: Does no extra conversion besides the conversion for the display device. Often used for non-photorealistic results or video editing where a specific look is already baked into the input video.

three.js is not Substance Painter, and I suspect that applications where three.js content sits alongside into other web content (and needs to visually match) are more common for our users than the HDR workflows where tone-mapping becomes necessary. All that to say, I think I agree with the comments from @mrdoob and @WestLangley in the thread below:

However — I do like @bhouston's suggestion of a "View" concept with tone mapping and additional state, and that could be a good direction to explore.


I'm closing this issue because it has drifted from the original topic and isn't clearly actionable, but further discussion in #23614 or a new issue is welcome.