-
-
Notifications
You must be signed in to change notification settings - Fork 3.9k
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
Set THREE.ColorManagement.legacyMode to false when using color management #5210
Conversation
ef06aaf
to
da21f35
Compare
tests/__init.test.js
Outdated
@@ -77,6 +77,8 @@ teardown(function (done) { | |||
this.sinon.restore(); | |||
delete AFRAME.components.test; | |||
delete AFRAME.systems.test; | |||
// Reset global legacyMode flag in THREE | |||
AFRAME.THREE.ColorManagement.legacyMode = true; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this needed for unit tests?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note that legacyMode = true
is the (current) default value in THREE.js. (Re)setting it here ensures all unit tests start with the expected default value. Otherwise as soon as one unit test changes it to false
(i.e. uses colorManagement), subsequent unit tests will run with the non-legacy behaviour of THREE.Color
.
For most tests it doesn't actually matter, but for any test that works with and inspects the values of THREE.Color
instances directly, it will cause differences. Setting it for all tests is the most effective.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FYI .legacyMode=false renamed to .enabled=true in r150
https://github.com/mrdoob/three.js/blob/7b04b508e32275b18f5def78c40ca38f426b7b2a/src/math/ColorManagement.js#L80-L86
In r152 enabled=true is the default and some other renaming was done, see https://twitter.com/donrmccurdy/status/1648412730691297280
I guess this PR will be merged for aframe 1.5.0 with three r152 or later because this will break some third parties components. It will surely break my code, I have some THREE.Color(cssColor).convertSRGBToLinear()
in some of my components that I'll need to remove.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It makes sense to merge it for 1.5.0 and to also switch the default to being color managed, matching Three.js. There will indeed be slight differences in behaviour, which I'm afraid is unavoidable. In a good chunk of cases this will likely not be an issue (color management can be disabled matching old defaults) and in some cases will actually fix broken behaviour ("old" components not converting to linear). I hadn't considered the manual usage of .convertSRGBToLinear()
as I tend to use rendererSystem.applyColorCorrection(color)
, but that would indeed break.
To summarize:
Pre 1.5.0 (colorManagement: false) | Pre 1.5.0 (colorManagement: true) | 1.5.0 (colorManagement: false) | 1.5.0 (colorManagement: true) | |
---|---|---|---|---|
new THREE.Color(...) |
✅ | ❌ | ✅ | ✅ |
.applyColorCorrection(color) |
✅ | ✅ | ✅ | ✅ |
color.convertSRGBToLinear() |
✅/❌* | ✅ | ✅/❌* | ❌ |
*) depending on if the convertSRGBToLinear()
is conditionally called or not.
Of course, this assumes that the (input) colour is intended to be sRGB, but components might very well be constructing Colors using hex notation, but expecting it to be linear or, like aframe-environment-component does, blend sRGB colours. 🫤
So it can't be entirely seamless, but we should document the changes properly. Not sure what the common approach for backwards/forwards compatibility and breaking changes is for A-Frame, but it would be nice for it to be possible to make components compatible with both 1.4.x and 1.5.0 in terms of colours.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh I see applyColorCorrection
has a condition on the conversion, I'll try to modify my components to use that instead of color.convertSRGBToLinear()
myself to be compatible with both 1.4.0 and 1.5.0. Thanks.
da21f35
to
ab47c72
Compare
src/systems/renderer.js
Outdated
@@ -38,6 +38,7 @@ module.exports.System = registerSystem('renderer', { | |||
|
|||
if (data.colorManagement || data.gammaOutput) { | |||
renderer.outputEncoding = THREE.sRGBEncoding; | |||
THREE.ColorManagement.legacyMode = false; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For r152 where THREE.ColorManagement.enabled=true
by default (equivalent of .legacyMode=false
). We will need here out of the condition if I'm not mistaken:
THREE.ColorManagement.enabled = data.colorManagement || data.gammaOutput
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And setting colorManagement
to true by default to match the default in three like you suggested @mrxz
@dmarcos after you cut super-three r151 or r152 and included it in master, tell us here so @mrxz can do the changes to the PR and you can merge it to release 1.5.0. Also you may want to release a new environment component at the same time with the two PRs supermedium/aframe-environment-component#82 and supermedium/aframe-environment-component#84 so we have all the color changes at the same time. |
FYI in r150 changelog https://github.com/mrdoob/three.js/releases/tag/r150 |
@mrxz master is on r152 now. Can you please rebase and make the discussed changes? |
@vincentfretin Updated the PR to address the relevant changes. I also took the liberty to remove the |
Thanks so much for sticking with it. It's awesome! |
Description:
Currently A-Frame handles the conversion from sRGB to Linear-sRGB colour space by using
.applyColorCorrection(colorOrTexture)
. Big drawback of this approach is that you have to remember to use it, and most non-core components don't actually do this, preventing them from being used in combination withcolorManagement: true
. Since r139 three.js has an option to handle this conversion (for colours) under the hood (mrdoob/three.js#23392), I think it's worth considering using that in A-Frame, hence this PR.It simplifies the code a bit, but most importantly, relieves anyone writing components from having to remember to call this every time a color is updated or changed.
Changes proposed:
THREE.ColorManagement.legacyMode
to false when usingcolorManagement: true
applyColorCorrection
to only operate on texturesapplyColorCorrection
onTHREE.Color
objects as they are now handled by three.js