pmndrs / react-three-fiber

πŸ‡¨πŸ‡­ A React renderer for Three.js
https://docs.pmnd.rs/react-three-fiber
MIT License
26.76k stars 1.52k forks source link

RFC: `workflow` canvas prop #2299

Open gsimone opened 2 years ago

gsimone commented 2 years ago

from discord

Don: [pedantic stuff] three.js always renders with sRGB primaries and white point (i.e. color gamut) but lighting calculations explicitly do not use the sRGB transfer function (sometimes called a gamma curve). Usually what people mean when referring to named color spaces are:

Cody: I'm not sure if people think beyond which encoding/tonemapping they're using.

CodyJasonBennett commented 2 years ago

My general concern with short prop names is how they describe themselves -- linear, flat, legacy, etc. don't inform me that I'm working with color. I think this makes for a confusing API, despite the above semantical concerns. I like how Don's proposal clarifies their relation to each other in more familiar terms, although I think it's important for options to be documented as to how they would be implemented in three. For existing projects, the only reference point users have would be which encoding, tonemapping, or colorspace they're using.

donmccurdy commented 2 years ago

if we want the workflow-oriented approach, these could be some opinionated defaults:

Users will reasonably want to change tone mapping and change texture encoding in certain cases. The other settings could perhaps be hidden behind the 'workflow' flag or something like it.

Probably it's also worth spelling out what more explicit versions of the API might look like, for example:

<Canvas
  inputColorSpace={'srgb'}
  workingColorSpace={'srgb-linear'}
  outputColorSpace={'srgb'}
  toneMapping={'filmic'} />

Predefined color space names above borrowed from CSS Color Module Level 4.

krispya commented 2 years ago

Maybe having the props split up like this for advanced users:

<Canvas
  colorspace={{ input: 'srgb', working: 'srgb-linear', output: 'srgb' }}
  tonemapping={'filmic'} />

Then if I am doing a postprocessing workflow, assuming 'lit' is the default, I can just do:

<Canvas tonemapping={false} />

I'm wondering how useful wide reaching presets are here. Seems to me most users use the 'lit' preset for most of their content and then some run into texture encoding issues, and then rarely others are trying to import an entire prelit scene. In those cases I'm thinking it would be better to have a short guide for how to use the colorspace and tonemapping options to get the result they want.

@donmccurdy The part I'm curious about in your presets is texture.encoding. That seems to me the biggest magic setting, touching all my textures. How does three handle it?

donmccurdy commented 2 years ago

three.js cannot guess the right texture.encoding settings β€” it's a property on the Texture class and no usage context is really available there. Suggested settings are provided in the color management docs, but even here there's some variation β€” .map is usually an albedo / diffuse texture and would be [0,1] sRGB, but using [0,∞] Linear-sRGB OpenEXR data for .map with a MeshBasicMaterial is a totally fair (though far more advanced) thing to do. In either case three.js must specify the encoding when uploading the texture to the GPU, and WebGL handles the conversion to Linear-sRGB if required.

I'm thinking it would be better to have a short guide for how to use the colorspace and tonemapping options to get the result they want...

I'm certainly not opposed to exposing more granular options if they're clearly and precisely named. I would caution that these "short guides" can get quickly get more complicated as we try to cover lit vs. unlit workflows, and the various post-processing implementations. That complexity cannot totally be avoided β€” we do need more of these guides β€” but (maybe) presets are a way to help some users avoid the pain.

donmccurdy commented 2 years ago

Side note β€” I'm not totally confident on the best 'unlit' settings. Maybe those users are totally fine going with the 'lit' workflow. The actual performance overhead for the (possibly unnecessary) sRGB β†’ Linear-sRGB β†’ sRGB round trip is probably not much. And effects like blending and color grading do still apply to 2D and unlit stuff, but I have pretty limited understanding of the tradeoffs of doing those operations in sRGB or Linear-sRGB space.

krispya commented 2 years ago

I'm certainly not opposed to exposing more granular options if they're clearly and precisely named. I would caution that these "short guides" can get quickly get more complicated as we try to cover lit vs. unlit workflows, and the various post-processing implementations.

My intuition is since these workflows are so complicated trying to cover them with presets might also cause more pain compared to having users read some information and ask questions on the Discord. On the other hand, it is nice to tell a user when doing some diagnosis to try putting flat or linear on their Canvas and see if it just solves their issue. But also this doesn't help them learn what the actual problem is if they run into an issue that can't be solved with those presets.

CodyJasonBennett commented 1 year ago

I believe this has become redundant since the introduction of THREE.ColorManagement which has made this much more stable in R3F.

donmccurdy commented 9 months ago

three.js r157 added (early, experimental) support for wide gamut color. We have a lot more to do on that side, but I think I feel comfortable suggesting that R3F will β€” eventually β€” need some changes to take full advantage of that work:

  1. <Canvas linear /> may be confusing in this workflow. I'd suggest, instead:
<Canvas colorSpace={ LinearSRGBColorSpace | SRGBColorSpace | DisplayP3ColorSpace } />
  1. I anticipate more tone-mapping options coming to three.js, and would suggest <Canvas toneMapping={ X } /> to handle that.

Threlte ships both (1) and (2) now.

Wide-gamut color will also require setting THREE.ColorManagement.workingColorSpace = THREE.LinearDisplayP3ColorSpace, but that's probably outside of what R3F needs to think about.

Finally, textures need to be tagged with their color space correctly, e.g. texture.colorSpace = SRGBColorSpace or texture.colorSpace = DisplayP3ColorSpace. I've been unable to find a safe way to do that automatically in three.js, unfortunately.

CodyJasonBennett commented 9 months ago

Definitely a fan of such configuration, and find linear and flat more confusing than not for those who are familiar with these workflows.

WRT assigning texture color space, I saw https://github.com/mrdoob/three.js/pull/27042 recently which somewhat worked around this.

krispya commented 8 months ago

I support this change as well.