ThatOpen / web-ifc-three

The official IFC Loader for Three.js.
https://ifcjs.github.io/info/
MIT License
506 stars 132 forks source link

Specify colors as sRGB-Rec709-D65 for color management #148

Closed donmccurdy closed 1 year ago

donmccurdy commented 1 year ago

Hi! The three.js project is working toward improved color management workflows, which are necessary for better lighting, PBR shading, and upcoming WebGL / WebGPU features like wide-gamut color spaces. As part of that, we've added a color management guide. One thing to note there is a new THREE.ColorManagement API:

THREE.ColorManagement.enabled = true; // r150+

It's called legacyMode = false in the current release but should be renamed as shown above soon:

Under this mode, RGB components are assumed to be in Linear-sRGB colorspace by default, and that conflicts with current usage here. I don't know what color management information exists for IFC models, whether configurable or not. But (without this PR) the new mode changes display of colors in IFC models, which you probably don't want!

Based on the current example in the repository, I'm assuming that material colors for IFC models are given in sRGB color space (sRBG transfer function, Rec 709 primaries, D65 white point). This PR specifies colorspace when configuring the material, so that:

  1. If THREE.ColorManagement APIs are not enabled, nothing changes.
  2. If THREE.ColorManagement APIs are enabled, the color is automatically converted to Linear-sRGB as required for a linear rendering workflow.
donmccurdy commented 1 year ago

It would also be an option to do this:

const col = new Color(color.x, color.y, color.z).convertSRGBToLinear();

On a strictly technical level that's better. This way it fixes both paths, with or without THREE.ColorManagement. three.js users should be setting renderer.outputEncoding = sRGBEncoding to have a linear lighting workflow, and that requires this conversion. Whereas THREE.ColorManagement.enabled is a new API and not widely used yet.

But that will be a breaking change for any of your current users, so the code given in the PR is safer as an upgrade. Up to you!

aka-blackboots commented 1 year ago

Hey @donmccurdy, I tested this for web-ifc-three. I have added the screenshot, the colors are more whitish with sRGBEncoding. test

donmccurdy commented 1 year ago

Hi @aka-blackboots — you'll have to tell me what I'm looking at here, sorry, or how I can reproduce it. :) I only have the single example found in the three.js codebase. renderer.outputEncoding = sRGBEncoding causes the conversion from Linear-sRGB to sRGB, before writing to the canvas. If input colors are not originally specified in Linear-sRGB, they must be converted before the rendering pipeline, which is a goal of THREE.ColorManagement and this PR.

Input (???) → Rendering (Linear-sRGB) → Canvas (sRGB)

If input colors are specified in the correct color space, then outputEncoding = sRGBEncoding will not affect overall brightness and hue negatively, as in your screenshot.

makc commented 1 year ago

@donmccurdy if only the line in screenshot was added, @aka-blackboots never called convertSRGBToLinear() on the colors

donmccurdy commented 1 year ago

This fix is contingent on THREE.ColorManagement being enabled — without it the change does nothing and .outputEncoding = sRGBEncoding will look incorrect.

Maybe I'm overcomplicating this in the name of backward-compatibility though. Other three.js loaders like GLTFLoader and FBXLoader simply call .convertSRGBToLinear() where necessary. This means users always need to set .outputEncoding = sRGBEncoding to get correct output, and results will be the same with or without THREE.ColorManagement enabled. If it's OK, I'll change the PR to that approach?


.outputEncoding = sRGBEncoding will begin to be set by default in a future three.js release. THREE.ColorManagement may be enabled by default later too, but that is tentative.

agviegas commented 1 year ago

Sorry for the delay, I'll check this out tomorrow!

agviegas commented 1 year ago

image I tried it and it looks good to me. I'm merging this. If there are reports of strange color displays, we'll handle them. Thanks a lot for this contribution! 🙏