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

Roadmap for a color-managed workflow in three.js #23614

Closed
17 of 20 tasks
donmccurdy opened this issue Feb 28, 2022 · 70 comments
Closed
17 of 20 tasks

Roadmap for a color-managed workflow in three.js #23614

donmccurdy opened this issue Feb 28, 2022 · 70 comments

Comments

@donmccurdy
Copy link
Collaborator

donmccurdy commented Feb 28, 2022

Overview

Summary issue to organize and track progress toward offering a color-managed workflow in three.js. Loosely based on Maya's documentation, I'll define color management as follows:

Color management involves applying the appropriate transforms to convert between color spaces as needed. These transforms are applied at specific points along the initialization, rendering, and display processes. For example:

  • On input, a transform is applied to convert colors and textures from the color space in which they were saved into the working color space.
  • As you work, colors in the working color space may be converted to/from other color spaces in order to accommodate bit depth and precision considerations.
  • On output to an image or display, an output transform is applied to convert colors appropriately for the image format or browser and device capabilities.

Roadmap

  1. (1.1) Add documentation of color management APIs (@donmccurdy)
  2. (1.2) Loaders identify and convert color spaces correctly (@gkjohnson)
  3. (1.3) Rename "encoding" properties more precisely as "colorSpace" (@donmccurdy + @Mugen87)
  4. (1.4) Change output color space default from Linear-sRGB to sRGB (@donmccurdy)
  5. (1.5) Convert some inputs automatically from sRGB to Linear-sRGB (@donmccurdy)
  6. (1.6) Change texture.colorSpace default to THREE.NoColorSpace (e.g. normal maps)
  7. (1.7) Ensure intermediate frame buffers have sufficient bit depth for their color space. See outputEncoding not handled correctly when using sRGB render target in WebGL2 #23251 (comment). No changes required.
  8. (1.8) Let CubeTextureLoader return sRGB cube textures by default.
  9. (1.9) Change renderer.useLegacyLights default to false

Timing considerations: If we are doing (1.3), (1.4), and (1.5), it would cause less disruption to existing code if we can make these changes within the same release. Fewer changes, if any, will be required for end-user code.

Future

These changes are more speculative, and may be considered explorative steps toward support for wide-gamut color spaces in WebGPU and SVG renderers.

  1. (2.1) Support changing ColorManagement.workingColorSpace
  2. (2.2) Support color input/output in Display P3 color space
  3. (3.3) Support vertex color workflow changes if necessary (ColorBufferAttribute?)

Continued in #26479.

@donmccurdy
Copy link
Collaborator Author

/cc @bhouston @gkjohnson @WestLangley I'm opening this is a higher-level tracking issue, since (a) any particular PR may be closed and lose that discussion context, and (b) more changes are needed than just #23392.

@gkjohnson
Copy link
Collaborator

(1.2) Loaders identify and convert color spaces correctly (@gkjohnson)

From my perspective this is nearly done. Aside from FBX the most common format loaders and exporters are converted to handle the current three.js color system correctly. The rest I think are less commonly used and / or I don't know a lot about. It would be nice if someone could help with the FBXLoader and example conversion.

@donmccurdy
Copy link
Collaborator Author

#23392 (1.5) has been merged, and I'm hoping #23430 (1.1) will also make it into r139.

At this point we may want to pause for a release or two, and see if there's feedback from users adopting the new ColorManagement.legacyMode = false option. I'm inclined to batch the 'breaking' changes together as much as possible. Those include (1.3) and (1.4) above, and — perhaps depending on feedback? — enabling .legacyMode = false by default.

@donmccurdy
Copy link
Collaborator Author

donmccurdy commented Mar 16, 2022

Not going to push too hard if others feel this is out of scope, but I wonder if we should consider enabling renderer.physicallyCorrectLights = true by default at some point as well.

@bhouston
Copy link
Contributor

we should consider enabling renderer.physicallyCorrectLights = true by default at some point as well.

That would be amazing! It was always a hack to make it false, but it was required initially until we settled on it being the right solution: #8260

@donmccurdy
Copy link
Collaborator Author

donmccurdy commented Apr 8, 2022

Feedback from the R3F folks (pmndrs/react-three-fiber#2127) has been that switching old demos to physicallyCorrectLights is hard, e.g. every light needs .intensity *= Math.PI and a considerable reduction in .decay to preserve the original look.

What is WebGPURenderer doing in terms of physicallyCorrectLights? I can't tell from the code but I think we should at least omit the property there and stick with physically-correct lighting. Users can always reduce light.decay if they need more artistic control.

@drcmda
Copy link
Contributor

drcmda commented Apr 8, 2022

when we switched to physicallyCorrectLights it seemed a change that is too big to just put out there. it didn't seem straight forward to restore projects and demos, everything turned pitch black. two properties (decay and intensity) didn't make it easier to comprehend how to fix it. when three makes it the default i think it should first educate people how to use it properly, similar to colormanagement.legacy=false and the new article explaining everything.

as for color management, react+three + eco system uses it by default now. i have already updated most sandboxes and everything works great. i love being able to omit the occasional convertSRGBToLinear and everything behaves much more consistent.

@mrdoob
Copy link
Owner

mrdoob commented Apr 8, 2022

What we could do is to make WebGPURenderer physically correct and be the only mode.

If a developer uses WebGPURenderer and wants to use WebGLRenderer as fallback, they'd have to make sure they're setting physicallyCorrectLights = true in WebGLRenderer.

However, I do not know how to change the lights decay to a physically correct value by default without breaking things.

/cc @sunag @Mugen87

@donmccurdy
Copy link
Collaborator Author

donmccurdy commented Apr 8, 2022

I don't feel strongly on whether the decay default really needs to change. It's true that it's only physically correct with a value of decay=2, but given all the other constraints of lighting in WebGL that's not a hill I'd choose to die on. And certainly there's no "one size fits all" upgrade path to get people there.

Perhaps we could change the decay default only under physicallyCorrectLights somehow; I don't think most users enable that setting so a breaking change in that mode might not be so bad, and e.g. glTF files containing lights will already set decay=2.

@Mugen87
Copy link
Collaborator

Mugen87 commented Apr 9, 2022

I'm glad this discussion came up^^. I have not added physicallyCorrectLights on purpose when filing the initial version of WebGPURenderer. #23614 (comment) sounds good to me.

@sunag
Copy link
Collaborator

sunag commented Apr 9, 2022

What we could do is to make WebGPURenderer physically correct and be the only mode.

@mrdoob @Mugen87 I agree so much that I was doing this ^^ #23872

@mrdoob
Copy link
Owner

mrdoob commented Apr 14, 2022

Perhaps we could change the decay default only under physicallyCorrectLights somehow; I don't think most users enable that setting so a breaking change in that mode might not be so bad, and e.g. glTF files containing lights will already set decay=2.

Hmmm, I think I'm willing to try changing the default to 2 (this month maybe) and see what happens...

@donmccurdy
Copy link
Collaborator Author

One more for the list – GammaCorrectionShader probably should be renamed to match up with the successor to renderer.outputEncoding, currently proposed to be renderer.outputColorSpace. Generally we should avoid references to 'gamma' in color management APIs.

For example:

composer.addPass( new ShaderPass( ColorSpaceTransformShader ) );

Other ideas:

  • OutputTransformShader
  • OutputColorSpaceShader
  • DisplayTransformShader

I'm not sure how to provide configuration options on a Shader object, but in current usage this would be Linear-sRGB to sRGB (the same as GammaCorrectionShader).

@Mugen87
Copy link
Collaborator

Mugen87 commented Apr 23, 2022

OutputColorSpaceShader probably fits best to WebGLRenderer.outputColorSpace.

@Mugen87
Copy link
Collaborator

Mugen87 commented Apr 23, 2022

BTW: I wonder if we could avoid the output* prefix. It comes from a time when gammaInput and gammaOutput were in place. Any objections against the simplified/shortened WebGLRenderer.colorSpace?

@donmccurdy
Copy link
Collaborator Author

donmccurdy commented Apr 23, 2022

I'm happy with OutputColorSpaceShader if @WestLangley approves. 👍

WebGLRenderer.colorSpace might be confusing. The working color space, Linear-sRGB, is at least as relevant to WebGLRenderer as the output color space, sRGB, even if the former is not customizable.

It's also helpful to look at how desktop software like Blender and Maya (both using OpenColorIO) refer to these concepts:

The term "display" is usually involved in describing our equivalent of output color space, whereas "view" and "look" transforms refer to tone mapping. Not a perfect analogy though – desktop software can render more directly to the display, but we're constrained to writing to a Canvas (implicitly sRGB), regardless of what the final display might be. Or the result might be a file rather than a display of course. Perhaps "output" is better than "display" in our context, for these reasons.

@WestLangley
Copy link
Collaborator

Maybe remove GammaCorrectionShader and add ColorSpaceShader, while retaining the nomenclature renderer.outputColorSpace.

//

The thing that is confusing, IMO, is something I mentioned previously. The terminology has changed, but the issue is the same: when rendering to a render target, do you honor the renderer's settings -- or the render target's texture settings?

@LeviPesin
Copy link
Contributor

when rendering to a render target, do you honor the renderer's settings -- or the render target's texture settings?

Maybe .outputEncoding should be understood as just the encoding of the "default" render target - the display?

@donmccurdy
Copy link
Collaborator Author

donmccurdy commented Apr 23, 2022

Maybe remove GammaCorrectionShader and add ColorSpaceShader, while retaining the nomenclature renderer.outputColorSpace.

This is also fine with me. Is there a way for Shader objects to take configuration parameters? If not, we just need to be aware that the generically-named ColorSpaceShader will only do Linear-sRGB to sRGB conversions for the moment.

when rendering to a render target, do you honor the renderer's settings -- or the render target's texture settings?

Maybe .outputEncoding should be understood as just the encoding of the "default" render target - the display?

So the color space of a render target used throughout a post-processing chain should be decoupled from the output color space written to a Canvas. I can't quite tell how this works today – it looks like neither the renderer's nor the render target's encoding is determining the output-to-canvas color space (when post-processing uses Linear-sRGB throughout the internal chain), we instead specify an explicit GammaCorrectionShader and ignore encoding properties?

@LeviPesin
Copy link
Contributor

LeviPesin commented Apr 23, 2022

I mean that in terms of encoding, no render target (i.e. null) would be equivalent to a render target with encoding renderer.outputEncoding. I think that with post-processing it should look like this:

renderer renders scene in linear color space - it applies post-processing effects that are working in linear color space - renderer converts the output to the color space of the render target - it applies post-processing effects that are working in output color space (linear or srgb)

@donmccurdy
Copy link
Collaborator Author

donmccurdy commented Apr 23, 2022

Can we avoid (or even deprecate) GammaCorrectionShader in that case? So output to the correct color space is not a separate pass in post-processing. I could — for example — make my last pass a DitherPass applied to linear colors, then write 8-bit sRGB output in the same pass.

@LeviPesin
Copy link
Contributor

With what I proposed the renderer (and not the passes) will authomatically convert the output to the correct space and then yes, GammaCorrectionShader is not needed at all.

@donmccurdy
Copy link
Collaborator Author

donmccurdy commented Apr 17, 2023

Remaining work related to r152:

  1. Write migration guide
  2. Enable THREE.ColorManagement on remaining 118 71 examples, where appropriate
  3. Enable renderer.outputColorSpace = SRGBColorSpace on remaining 56 20 examples, where appropriate

I believe (1) is necessary for r152, where (2) and (3) can be completed on a best-effort basis before or after the release.

I'll work on (1) next.

@Mugen87
Copy link
Collaborator

Mugen87 commented Apr 21, 2023

r152 will be released next week and publish the many important color management related changes. At this point thanks to @donmccurdy for pushing this topic so far!

Since this release potentially needs more explanation, I wonder if it's best to write a topic at the forum similar when we removed THREE.Geometry and examples/js.

https://discourse.threejs.org/t/three-geometry-will-be-removed-from-core-with-r125/22401
https://discourse.threejs.org/t/the-examples-js-directory-will-be-removed-with-r148/45349

My impression is the community met both topics with positive response since they allowed users to ask related questions without filing new topics or issues. The structure of the topics (consequences of the change, motivation, migration tasks and a roadmap) also provide a compact overview since most users do not follow the entire discussion at GitHub.

I think it's best to write a similar announcement for next week as an addition to the migration guide.

@donmccurdy Since you are working on (1), do you think you can use the contents of your migration guide to build such a topic?

@donmccurdy
Copy link
Collaborator Author

Thanks @Mugen87, that's a great idea. I'll plan to post something on the forum early next week, and perhaps we link there from the migration guide.

@donmccurdy
Copy link
Collaborator Author

Announcement —

https://discourse.threejs.org/t/updates-to-color-management-in-three-js-r152/50791

@Mugen87
Copy link
Collaborator

Mugen87 commented Apr 24, 2023

Awesome topic! I've pinned it globally for a month so it should be clearly visible for all forum users.

@mrdoob
Copy link
Owner

mrdoob commented Apr 24, 2023

Amazing @donmccurdy! 🙏🙏🙏

Nit:

I'm still using renderer.outputEncoding = LinearEncoding (default)

How about (previous default) instead?

@donmccurdy
Copy link
Collaborator Author

Done! ✅

@mad-wizard
Copy link

@donmccurdy Is there any ETA on P3 color support? I've had to abandon three.js as a solution because color accuracy for our application is mission-critical and we could not get accurate colors.

Would love to re-integrate three but this is holding us back.

Would be willing to assist if required.

Thank you.

@donmccurdy
Copy link
Collaborator Author

donmccurdy commented Jul 21, 2023

Hi @mad-wizard!

While I could be misunderstanding the issues you've encountered, I expect that a Display P3 workflow can solve "color accuracy" problems by itself if — and only if — you're using a fully unlit (no lighting) scene. For example, photogrammetry or baked lights. Is that the case here?

In a lit scene, it's more likely that image formation (tone mapping, exposure, lighting, etc.) are the relevant concerns. If you aren't using tone mapping yet, then it's much too early in the image formation process to be worried about Display P3. 🙂

I believe it would also be fair to say that our current tone mapping options could be improved... if this sounds like it would be of interest, would you mind starting a new issue on the color accuracy problems you're facing? I'd also be interested in whether you've found alternatives to three.js that solve the problem better, or whether the problem remains completely unsolved for now.

P.S. It's a deeper dive, but @elalish's writing on glTF Color Accuracy may provide helpful background.

@mad-wizard
Copy link

mad-wizard commented Jul 21, 2023

Hi @mad-wizard!

While I could be misunderstanding the issues you've encountered, I expect that a Display P3 workflow can solve "color accuracy" problems by itself if — and only if — you're using a fully unlit (no lighting) scene. For example, photogrammetry or baked lights. Is that the case here?

In a lit scene, it's more likely that image formation (tone mapping, exposure, lighting, etc.) are the relevant concerns. If you aren't using tone mapping yet, then it's much too early in the image formation process to be worried about Display P3. 🙂

I believe it would also be fair to say that our current tone mapping options could be improved... if this sounds like it would be of interest, would you mind starting a new issue on the color accuracy problems you're facing? I'd also be interested in whether you've found alternatives to three.js that solve the problem better, or whether the problem remains completely unsolved for now.

P.S. It's a deeper dive, but @elalish's writing on glTF Color Accuracy may provide helpful background.

Thanks for the response!

Yes I am using a texture rendered on a plane with an unlit material (MeshBasic).

I have tried many encoding and color space changes and have isolated the problem; images taken with modern iPhones use the P3 color profile which has a wider gamut than sRGB. When these images are rendered in three.js they appear washed out.

Here is an example of a P3 image. https://goldeneye.nyc3.cdn.digitaloceanspaces.com/woman.png

I tried downloading the repo and replacing the texture of the crate in one of the examples with the above image of the woman, that should be a way for you to easily reproduce the issue.

Since we are making a photo editing app, color quality is mission critical so we were on the verge of abandoning three.js, then this morning I was able to find a work-around where we convert the p3 profile to srgb manually and then use the processed texture in three.js this is not an ideal workaround.

We want to use three.js to implement 3D effects and more advanced features than other photo editing apps.

On an unrelated side-note, do you happen to have any idea on how we could implement photo filters using three.js for this type of app?

Best

Callan

@gkjohnson
Copy link
Collaborator

I have tried many encoding and color space changes and have isolated the problem; images taken with modern iPhones use the P3 color profile which has a wider gamut than sRGB.
...
Here is an example of a P3 image. https://goldeneye.nyc3.cdn.digitaloceanspaces.com/woman.png

Independent of supporting the Color Space we need a way of determining what color space is used by a loaded texture. Currently there's no way to determine the color space of an image even if it's correctly embedded (see #21336). And even then when I use image magick to inspect the embedded color space of the image provided it says it's sRGB. I'm not sure what the expected approach is for working with new color spaces in WebGL is. As Don mentioned I think this requires making a different issue to discuss.

On an unrelated side-note, do you happen to have any idea on how we could implement photo filters using three.js for this type of app?

In the interest of keeping issues on topic - you can ask questions at the forum: https://discourse.threejs.org/

@donmccurdy
Copy link
Collaborator Author

And even then when I use image magick to inspect the embedded color space of the image provided it says it's sRGB...

Color space information embedded in PNG or JPEG images is very unreliable, unfortunately. The ICC Profile appears to have the correct information.

I'm viewing this as a web platform issue – as things stand, we can't do it automatically. Perhaps it should be escalated to a W3C working group. In the meantime, and for the foreseeable future, users must identify the color space of their textures with texture.colorSpace.

Support for rendering lit scenes in wide color gamuts will be difficult. Support for rendering unlit scenes, much less so. But I'm unsure if we want to attempt one without the other.

Perhaps this issue can now be closed, and another opened to discuss support for wide-gamut color spaces in three.js? This thread has probably covered enough territory already. 😅

@mad-wizard
Copy link

mad-wizard commented Jul 22, 2023

I can share some ruby code I am using to convert the image from P3 to SRGB. It uses mini-magick:

def convert_to_srgb!
  unique_filename = "#{original.filename}-#{SecureRandom.uuid}"
  download_path = Rails.root.join('tmp', unique_filename)

  File.open(download_path, 'wb') { |file| file.write(original.download) }

  image = MiniMagick::Image.open(download_path) # open with imagemagick
  color_profile = image["%[profile:icc]"]  # get color profile

  unless color_profile.include?("Display P3")  # if image is not p3, return
    File.delete(download_path) if File.exist?(download_path)
    return
  end

  image.combine_options do |b|  # convert p3 color profile to sRGB
    b.profile("#{Rails.root}/colors/P3.icc")
    b.profile("#{Rails.root}/colors/sRGB.icc")
  end

  original.attach(io: File.open(image.path), filename: original.filename.to_s, content_type: original.content_type)
  File.delete(download_path) if File.exist?(download_path)
end 

@donmccurdy
Copy link
Collaborator Author

To be continued in #26479.

@gkjohnson
Copy link
Collaborator

The ICC Profile appears to have the correct information.

Ha how nice that they keep two out of sync fields with color space information 😅

magick identify -verbose woman.png

    ...
    Colorspace: sRGB
    ...
    icc:copyright: Copyright Apple Inc., 2022
    icc:description: Display P3
    ...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests