mrdoob / three.js

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

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

Closed donmccurdy closed 1 year ago

donmccurdy commented 2 years ago

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. [x] (1.1) Add documentation of color management APIs (@donmccurdy)
  2. [x] (1.2) Loaders identify and convert color spaces correctly (@gkjohnson)
  3. [x] (1.3) Rename "encoding" properties more precisely as "colorSpace" (@donmccurdy + @Mugen87)
  4. [x] (1.4) Change output color space default from Linear-sRGB to sRGB (@donmccurdy)
  5. [x] (1.5) Convert some inputs automatically from sRGB to Linear-sRGB (@donmccurdy)
  6. [x] (1.6) Change texture.colorSpace default to THREE.NoColorSpace (e.g. normal maps)
  7. [x] (1.7) Ensure intermediate frame buffers have sufficient bit depth for their color space. See https://github.com/mrdoob/three.js/issues/23251#issuecomment-1014606909. No changes required.
  8. [x] (1.8) Let CubeTextureLoader return sRGB cube textures by default.
    • [x] #26162
      1. [x] (1.9) Change renderer.useLegacyLights default to false
    • [x] #26392

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 https://github.com/mrdoob/three.js/issues/26479.

donmccurdy commented 1 year ago

That would be my preference personally — THREE.ColorManagement.enabled = true is enough to handle many of the migrations in https://github.com/mrdoob/three.js/pull/25783. I believe it's a good way to achieve what @mrdoob describes in https://twitter.com/mrdoob/status/1273743531551551488, creating a workflow consistent with CSS and HTML.

But I don't want to step on toes here: if others are not comfortable with it at this point, that is OK. 😇

Mugen87 commented 1 year ago

I'm fine with enabling THREE.ColorManagement.enabled, too. At this point, it makes sense to integrate this change into r152.

@donmccurdy Would you prefer to include this change in https://github.com/mrdoob/three.js/pull/25783 or in a different PR?

donmccurdy commented 1 year ago

OK, let's update THREE.ColorManagement.enabled in an additional PR. I'll be out for a few days, and back on April 14. The remaining changes for r152 will be:

LeviPesin commented 1 year ago

I think after 2 we wouldn't need to have ColorManagement explicitly enabled like with outputColorSpace -- because ColorManagement feels like an internal thing. So just nothing with ColorManagement in already updated examples and explicit opt-out in others (but outputColorSpace is specified in every example).

Mugen87 commented 1 year ago

Update remaining examples to set outputColorSpace explicitly

In general, I'm not a fan of explicitly setting default values since this patterns ends up in verbose code. I vote to only define renderer.outputColorSpace = LinearSRGBColorSpace (and later THREE.ColorManagement.enabled = false) where it is actually required.

donmccurdy commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

Announcement —

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

Mugen87 commented 1 year ago

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

mrdoob commented 1 year ago

Amazing @donmccurdy! 🙏🙏🙏

Nit:

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

How about (previous default) instead?

donmccurdy commented 1 year ago

Done! ✅

mad-wizard commented 1 year ago

@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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

To be continued in https://github.com/mrdoob/three.js/issues/26479.

gkjohnson commented 1 year ago

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
    ...