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

RFC: workflow canvas prop #2299

Open
Tracked by #2338
gsimone opened this issue Jun 1, 2022 · 10 comments
Open
Tracked by #2338

RFC: workflow canvas prop #2299

gsimone opened this issue Jun 1, 2022 · 10 comments
Labels
request for comments come & have your say v10

Comments

@gsimone
Copy link
Member

gsimone commented Jun 1, 2022

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:

  • sRGB / THREE.sRGBEncoding: sRGB transfer function, primaries, and white point
  • Linear-sRGB / THREE.LinearEncoding: sRGB primaries and white point, but Linear transfer function
    For that reason flag names like linear={true} and flat={true} are probably adding some confusion... I love short property names as much as the next JavaScript programmer, but color seems to be an area where precise wording pays off. Alternative to the full technical terms would be to have an API in terms of 'what the user wants to do', like workflow={ 'lit' | 'unlit' | 'legacy' | ... } and internally set opinionated defaults for each workflow.
    [/pedantic stuff]

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

@joshuaellis joshuaellis added the request for comments come & have your say label Jun 1, 2022
@joshuaellis joshuaellis pinned this issue Jun 1, 2022
@CodyJasonBennett
Copy link
Member

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
Copy link
Member

donmccurdy commented Jun 1, 2022

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

  • lit: color inputs assumed sRGB, converted to Linear-sRGB, scene rendering in Linear-sRGB, output to sRGB with a (probably filmic) tone map. examples: most PBR or lit non-photorealistic e.g. pixar scenes.
    • ColorManagement.legacyMode = false
    • texture.encoding = sRGBEncoding (for color textures only)
    • renderer.outputEncoding = sRGBEncoding
    • renderer.toneMapping = FilmicToneMapping
  • unlit: color inputs assumed sRGB, no lighting so no need to convert anything to Linear-sRGB, skip output encoding and tone map. examples: 2D art with sprites, text, illustrated styles.
    • Some debate about semantically 'best' way to do this moving forward. The defaults below give the correct result, but for implicit and confusing reasons.
    • ColorManagement.legacyMode = true
    • texture.encoding = LinearEncoding (always)
    • renderer.outputEncoding = LinearEncoding
    • renderer.toneMapping = NoToneMapping
  • legacy: if we need to keep current behavior available for backward compatibility
    • ...

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
Copy link
Member

krispya commented Jun 1, 2022

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
Copy link
Member

donmccurdy commented Jun 1, 2022

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
Copy link
Member

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
Copy link
Member

krispya commented Jun 3, 2022

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
Copy link
Member

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

@CodyJasonBennett CodyJasonBennett closed this as not planned Won't fix, can't repro, duplicate, stale Jun 12, 2023
@CodyJasonBennett CodyJasonBennett unpinned this issue Jun 12, 2023
@donmccurdy
Copy link
Member

donmccurdy commented Oct 25, 2023

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 } />
  • ⚠️ LinearSRGBColorSpace: Equivalent to current linear option, use sparingly
  • ⚠️ LinearDisplayP3ColorSpace: Like current linear but wider gamut, use sparingly
  • ✅ SRGBColorSpace: Current default
  • ✅ DisplayP3ColorSpace: Wide-gamut display
  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
Copy link
Member

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 mrdoob/three.js#27042 recently which somewhat worked around this.

@krispya
Copy link
Member

krispya commented Oct 30, 2023

I support this change as well.

@krispya krispya added the v10 label Dec 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
request for comments come & have your say v10
Projects
None yet
Development

No branches or pull requests

5 participants