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.

Sign up for GitHub

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

Jump to bottom

Color difference between 1.4.2 and 1.5.0 #5383

Open
dmarcos opened this issue Nov 14, 2023 · 10 comments
Open

Color difference between 1.4.2 and 1.5.0 #5383

dmarcos opened this issue Nov 14, 2023 · 10 comments

Comments

@dmarcos
Copy link
Member

dmarcos commented Nov 14, 2023

Just soft launched 1.5.0 and noticed a color difference on the hello-world with respect to 1.4.2. Screenshots below

We changed color management related stuff in #5210. It might be related

@mrxz @vincentfretin what do you think? thanks

1.4.2
image

1.5.0
image
@vincentfretin
Copy link
Contributor

That's excepted because colorManagement is true by default. If you set colorManagement:false it should be the same, right?

@mrxz
Copy link
Contributor

mrxz commented Nov 14, 2023

That does indeed look like an (expected) difference due to colorManagement being enabled by default now. While colours should pretty much match up as they were before, the lighting calculations now take place in linear colour space, which will result in (slight) differences. Especially the brighter/specular parts will appear different. See for example the comparison screenshots in this Three.js thread: https://discourse.threejs.org/t/updates-to-color-management-in-three-js-r152/50791#motivation-3

However, there have been various changes in Three.js since, so to rule out any regressions/oversights, could you test with colorManagement: false like @vincentfretin suggests? That should give identical results to 1.4.2

@dmarcos
Copy link
Member Author

dmarcos commented Nov 14, 2023

With colorManagement: false it looks like 1.4.2. Was definitively prettier before. Not sure what to do here.

@dmarcos dmarcos mentioned this issue Nov 14, 2023
Merged
@arpu
Copy link
Contributor

arpu commented Nov 14, 2023

a possible solution could be to update the injected lights to higher value

@vincentfretin
Copy link
Contributor

vincentfretin commented Nov 15, 2023 •
edited
Loading

Changing the intensity of the default injected lights can make the colors brighter but the colors will still differ from what it used to be. The color changes here are just related to the linear-sRGB conversion.
With colorManagement:true (the default now in 1.5.0), if you want exactly the same colors as 1.4.2, you need to convert all used colors, you can find the new value with
new THREE.Color().set("#4CC3D9").convertLinearToSRGB().getHexString()
and fix the injected ambient light color from #BBB to #dedede

diff --git a/examples/boilerplate/hello-world/index.html b/examples/boilerplate/hello-world/index.html
index 4d06c95d..6f834511 100644
--- a/examples/boilerplate/hello-world/index.html
+++ b/examples/boilerplate/hello-world/index.html
@@ -7,11 +7,11 @@
     <script src="../../../dist/aframe-master.js"></script>
   </head>
   <body>
-    <a-scene background="color: #ECECEC">
-      <a-box position="-1 0.5 -3" rotation="0 45 0" color="#4CC3D9" shadow></a-box>
-      <a-sphere position="0 1.25 -5" radius="1.25" color="#EF2D5E" shadow></a-sphere>
-      <a-cylinder position="1 0.75 -3" radius="0.5" height="1.5" color="#FFC65D" shadow></a-cylinder>
-      <a-plane position="0 0 -4" rotation="-90 0 0" width="4" height="4" color="#7BC8A4" shadow></a-plane>
+    <a-scene background="color: #f6f6f6">
+      <a-box position="-1 0.5 -3" rotation="0 45 0" color="#94e3ee" shadow></a-box>
+      <a-sphere position="0 1.25 -5" radius="1.25" color="#f875a3" shadow></a-sphere>
+      <a-cylinder position="1 0.75 -3" radius="0.5" height="1.5" color="#ffe4a3" shadow></a-cylinder>
+      <a-plane position="0 0 -4" rotation="-90 0 0" width="4" height="4" color="#b9e5d2" shadow></a-plane>
     </a-scene>
   </body>
 </html>
diff --git a/src/systems/light.js b/src/systems/light.js
index bad27c48..1f54da23 100755
--- a/src/systems/light.js
+++ b/src/systems/light.js
@@ -67,7 +67,7 @@ module.exports.System = registerSystem('light', {
     }
 
     ambientLight = document.createElement('a-entity');
-    ambientLight.setAttribute('light', {color: '#BBB', type: 'ambient'});
+    ambientLight.setAttribute('light', {color: '#dedede', type: 'ambient'});
     ambientLight.setAttribute(DEFAULT_LIGHT_ATTR, '');
     ambientLight.setAttribute(constants.AFRAME_INJECTED, '');
     sceneEl.appendChild(ambientLight);

With these new colors, we have exactly the same render.
You may not want to change the default ambient color, or change it to #dedede only if colorManagement is true? But existing scene will still be different anyway from 1.4.2 to 1.5.0 if the user defined colors in their scene. You may just add a note in the release notes to specify renderer="colorManagement: false" if you don't want rendering changes.

@mrxz
Copy link
Contributor

mrxz commented Nov 15, 2023

There should be no need to adjust colours in your A-Frame scenes1, as the colour space conversions are correctly taken care of. In other words the colours themselves should still be identical between 1.4.2 and 1.5.0. The difference lies in the effect of the lighting calculations. An easy way to verify this is to use a single full-bright ambient light, which effectively avoids any shading, showing the entities base diffuse colour:

1.4.2 1.5.0
image image 
<a-light type="ambient" intensity="1"></a-light>

As for the lights, there's no real way to adjust them with ColorManagement enabled to get identical results as before. In a way this makes sense, otherwise there wouldn't have been a need for this change. Though it should be possible to tweak them to get the desired look and feel. Like @arpu suggests, boosting the intensity of the default lights would likely bring the result for the hello-world scene closer to how it was in 1.4.2. Though it likely won't be a one-size-fits-all change.

I think the key point is that most scenes will have been authored with the old approach, and as a result will inevitably look slightly 'off' due to some differences. Most noticeable will be the high-specular/bright areas (plasticky look) and the very dark areas. For new scenes this shouldn't be an issue, and when aiming for realistic PBR the new default should behave better, which for MR experiences can aid making the virtual entities appear more realistic/grounded.

1) This is only referring to using colours in HTML, or when setting colour properties through setAttribute and the like. Changes might be needed if a user manually creates THREE.Color instances in their code and does certain operations with those colours. ↩

@vincentfretin
Copy link
Contributor

I agree, there is no need to do any color space conversions yourself anymore in 1.5.0, this is taken care of for you.
I'll try to clarify my previous comment, hopefully I don't give wrong information here.
Previously in 1.4.2 with colorManagement:false, the specified hex color was treated in linear color space and didn't match the color you saw in css (that uses sRGB). With colorManagement:true, the hex color is assumed to be in sRGB (like css) and converted to linear automatically, with convertSRGBToLinear() that was done in aframe applyColorCorrection in 1.4.2, and now done by threejs in 1.5.0.
If you choose those specific hex colors for the way is rendered in your scene with colorManagement:false, you may want to do the conversion yourself to find the sRGB css hex color you really wanted.

@mrxz
Copy link
Contributor

mrxz commented Nov 15, 2023

Previously in 1.4.2 with colorManagement:false, the specified hex color was treated in linear color space and didn't match the color you saw in css (that uses sRGB).

While the colours were indeed treated as being in linear colour space, the (linear) output was presented on the screen as-is. These "two wrongs" effectively negated each other. The colours did in fact match up with what you'd see when using them in CSS, barring any shading/lighting. You can verify this yourself by using shader="flat" as this isn't influenced by lights.

Enabling colorManagement actually introduces two conversions, one when ingesting colours (sRGB to linear) and one when outputting the fragments colour (linear to sRGB). These two conversions are inverses of each-other and as a result behave the same, barring any shading/lighting.

Input Output
sRGB CSS colour (assumed linear) shading/lighting (output as-is) presented as sRGB
sRGB CSS colour converted to linear shading/lighting converted to sRGB presented as sRGB

With colorManagement:true, the hex color is assumed to be in sRGB (like css) and converted to linear automatically, with convertSRGBToLinear() that was done in aframe applyColorCorrection in 1.4.2, and now done by threejs in 1.5.0.

Indeed, in 1.5.0 it's three.js that handles (most) of the colour management burden. Only for textures does A-Frame still need to explicitly state the colour space. One thing that might not be immediately obvious is that Three.js does this inside of the THREE.Color class. So caution when using that class directly, especially when comparing or porting older code, for example:

Version Code Value
1.4.2 new THREE.Color().set("#4CC3D9").r 0.2980392156862745
1.5.0 new THREE.Color().set("#4CC3D9").r 0.0722718506743852
1.5.0 new THREE.Color().setStyle("#4CC3D9", THREE.SRGBColorSpace).r 0.0722718506743852
1.5.0 new THREE.Color().setStyle("#4CC3D9", THREE.LinearSRGBColorSpace).r 0.2980392156862745
1.5.0 new THREE.Color().setStyle("#4CC3D9", THREE.NoColorSpace).r 0.2980392156862745

As you can see the conversion happens immediately. Many of the setter methods now also accept a colour space argument which you can use to indicate which colour space the provided value is in.

Most user shouldn't face any issues, but when inspecting colour channel values or performing arithmetic with colours, there'll likely be differences when toggling colorManagement on and off. It might be worth always being explicit about the colour space in those cases to avoid any oversights.

If you choose those specific hex colors for the way is rendered in your scene with colorManagement:false, you may want to do the conversion yourself to find the sRGB css hex color you really wanted.

I'm afraid this would actually lead to incorrect colours. It's really the lights and possibly material properties you should tweak to (re)gain the desired visuals.

That being said, a lot of people might actually pick colours iteratively by judging how they look in the scene (= under the lighting setup in said scene). In those cases the colours don't actually reflect the actual diffuse of the material. They might need fixing up manually as it's not possible to automatically convert these in a meaningful way. When faced with many of those, it's probably easier to disable colorManagement.

tl;dr When migrating from 1.4.2, leave colours untouched. My advice: adjust lights first, followed by possibly tweaking material properties. If that isn't enough, consider manually changing the colours, though probably easier to disable colorManagement in this case.

@vincentfretin
Copy link
Contributor

The colours did in fact match up with what you'd see when using them in CSS

Oh you're right sorry, my personal experience where it didn't match was with colorManagement:true, aframe 1.4.2 and THREE.Color() where I wrongly didn't call applyColorCorrection that called convertSRGBToLinear. In 1.5.0 there won't be this issue anymore, it's done automatically.

Sorry folks for the noise, listen to @mrxz he knows this stuff better than me obviously. Thanks for doing again a summary on this topic, much appreciated.

@dmarcos
Copy link
Member Author

dmarcos commented Nov 15, 2023

Thanks. Yeah the hello world is just the canary in the mine. Worried about surprising people. All of a sudden scenes look more muted / boring :)

@diarmidmackenzie diarmidmackenzie mentioned this issue Dec 17, 2023
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
4 participants
@arpu @dmarcos @vincentfretin @mrxz