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

Suggestion: add a bufferAttribute.convertGammaToLinear method #15678

Closed
looeee opened this issue Jan 31, 2019 · 10 comments
Closed

Suggestion: add a bufferAttribute.convertGammaToLinear method #15678

looeee opened this issue Jan 31, 2019 · 10 comments

Comments

@looeee
Copy link
Collaborator

looeee commented Jan 31, 2019

Vertex colors are usually specified in gamma space, at least on the models that I've come across.

This makes it hard to use a properly linear workflow, since setting renderer.gammaOutput = true will mess up the vertex colors on these models. The Parrot.glb model is one example of this.

It's fairly simple method:

function convertGammaToLinear( colorAttribute, gammaFactor ) {

       if ( gammaFactor === undefined ) gammaFactor = 2.0;

      for ( let i = 0; i < colorAttribute.array.length; i++ ) {

            colorAttribute.array[ i ] **= gammaFactor;

      }

      colorAttribute.needsUpdate = true;

}

What do people think about adding bufferAttribute.convertGammaToLinear ?

@WestLangley
Copy link
Collaborator

From https://threejs.org/docs/#examples/loaders/GLTFLoader

Textures containing color information (.map, .emissiveMap, and .specularMap) always use sRGB colorspace in glTF, while vertex colors and material properties (.color, .emissive, .specular) use linear colorspace.

@looeee
Copy link
Collaborator Author

looeee commented Jan 31, 2019

Yes, the models are not spec compliant. But it seems like quite a few models are not spec compliant, which is why I'm suggesting this.

Also, as far as I'm aware other model formats don't specify the color space for vertex colors.

@donmccurdy
Copy link
Collaborator

I'm not aware of any formats other than glTF that specify what the vertex colors should use. 😞 But yeah, it's not uncommon to find models with vertex colors using sRGB.

I don't have a preference whether we add the method or not, but:

(1) if so, perhaps convertSRGBToLinear() instead
(2) we should fix the glTF models in our own repo to use linear colorspace if they don't

I can help with the second, that's my mistake on the Parrot model.

@looeee
Copy link
Collaborator Author

looeee commented Jan 31, 2019

we should fix the glTF models in our own repo to use linear colorspace if they don't

Agreed. I've been working on converting the Ro.me models.

It seems like the blender 2.80 exporter correctly converts the vertex colors to linear, but unfortunately the file sizes are much bigger. Parrot.glb goes from 95k to 326kb, even with uvs and normals disabled. If you have any ideas on how to reduce that, let me know 😃

I don't have a preference whether we add the method or not

All of this linear workflow stuff is confusing for users, and it's not helped by the fact that the transition has been slow and is incomplete across the industry, meaning that many apps are stuck half way between a linear and gamma workflow, such as three.js with its backwards compatible gammaFactor = 2 and gammaOut = false.

Older models like this generally have colors or vertex colors specified in sRGB, probably always if they are any other format than glTF. Older formats and apps often don't even give any consideration to color space.

Personally, I've been studying how three.js handles gamma for the last couple of days and I think I understand it well now, but it is tricky, and adding this method might make things easier for people.

if so, perhaps convertSRGBToLinear() instead

Perhaps we would should add both to match the methods on Color.

@looeee
Copy link
Collaborator Author

looeee commented Jan 31, 2019

if so, perhaps convertSRGBToLinear() instead

I've done a bit more testing, and at least for the Ro.me models it should be convertGammaToLinear.

That's what works for materials:

renderer.gammaFactor = 2.2;
renderer.gammaOutput = true;

const material = new THREE.MeshBasicMaterial( {color: 0x800080} );

// testing the output of the material, this gives #800080
material.color.convertGammaToLinear( 2.2 );

// however, this outputs a material with color #880088
material.color.convertSRGBToLinear( 2.2 );

Likewise, when I apply the convertGammaToLinear( colorAttribute, 2.2 ) method to the Parrot.glb and set the material to a MeshBasicMaterial then I get a 1-1 match with the vertex colors as they display in Blender. convertSRGBToLinear( colorAttribute, 2.2 ) would not give a correct match in this case.

@donmccurdy
Copy link
Collaborator

It seems like the blender 2.80 exporter correctly converts the vertex colors to linear, but unfortunately the file sizes are much bigger. Parrot.glb goes from 95k to 326kb, even with uvs and normals disabled.

It's probably resampling the animations too aggressively. 🙃 I can fix the colors without bringing them through Blender I think; I have some scripts lying around heh.

when I apply [convertGammaToLinear] I get a 1-1 match with the vertex colors as they display in Blender. [convertSRGBToLinear] would not give a correct match in this case

I think that's a result of three.js having a gammaOutput=true property but no equivalent of sRGBOutput=true. Really it should all be sRGB instead of plain gamma... from What every coder should know about gamma,

The sRGB specification defines what gamma to use for encoding and decoding sRGB images (among other things such as colour gamut, but these are not relevant to our current discussion). sRGB gamma is very close to a standard gamma of 2.2, but it has a short linear segment in the very dark range to avoid a slope of infinity at zero (this is more convenient in numeric calculations)... You don’t actually need to understand all these finer details; the important thing to know is that in 99% of the cases you’ll want to use sRGB instead of plain gamma.

Also note that convertSRGBToLinear doesn't need a 2.2 parameter, that value is implicit in sRGB.

@looeee
Copy link
Collaborator Author

looeee commented Jan 31, 2019

I can fix the colors without bringing them through Blender I think

Ok, that would be cool. At least Parrot, Stork, Flamingo and Horse need this fixed. Not sure if other models in the repo have vertex colors.

I think that's a result of three.js having gammaOutput=true property but no equivalent of sRGBOutput=true

I guess we'll have to wait for #11337 to be resolved for that.

Yeah, this makes sense. So, that means that when I do material.color.convertGammaToLinear( 2.2 ), I'm actually performing an incorrect calculation since CSS colors are in sRGB.

However, because renderer.gammaOutput=true is also not correct then the end result is correct - that is, two wrongs DO make a right (in this case). Man, no wonder this is taking me a while to understand 😅

That also means we should use convertSRGBToLinear for the vertex colors (presumably they're in sRGB) and we'll just have to live with the renderer outputting slightly wrong colors pending the addition of renderer.colorSpace. It will be a very subtle difference anyway.

@WestLangley
Copy link
Collaborator

We have known we need to handle color space correctly for years. #6383.

Changes to the library in this regard have been slow because there has been a lack of standardization or convention in the industry. I have also been waiting for sufficient demand in this project. In the past, only a few contributors have shown interest in this topic.

Regarding gamma, stick to sRGB.

@looeee
Copy link
Collaborator Author

looeee commented Feb 1, 2019

In the past, only a few contributors have shown interest in this topic

I think that because, in general, if you have a nearly correct workflow (such as converting to Gamma instead of sRGB) then errors will be minor and people don't notice there's a problem, as with my purple color example above that ends up outputting #880088 instead of #800080. Factor in lights and it's going to be basically impossible to notice something is wrong.

However, I'm coming to believe that this is one of those things that subtly effects the overall quality of the scene and lighting in a very important way - that is, it's one of the things that makes people go from "hmm... another 3D scene" to "wow, that looks amazing! 😁".

So count me interested, although unfortunately very short on time to contribute over the next while.

@Mugen87
Copy link
Collaborator

Mugen87 commented Mar 16, 2021

I do not vote to introduce convertGammaToLinear() or convertSRGBToLinear() to BufferAttribute. This class should have a generic interface and not provide methods which depend on the actual data. Existing methods like copyColorsArray() were only introduced as a convenience method for the Geometry to BufferGeometry conversion process. When Geometry is removed at some point from examples, these methods should be deprecated, too.

I think it's okay to ask the app/user to iterate over the color buffer attribute and convert it if required.

I suggest to discuss other color management related topics in #19169.

@looeee looeee closed this as completed Mar 17, 2021
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

4 participants