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

THREE.Color: added sRGB conversion methods #14332

Merged
merged 3 commits into from
Jun 20, 2018

Conversation

WestLangley
Copy link
Collaborator

I expect these methods will, at some point, replace convertGammaToLinear(), etc.

@donmccurdy
Copy link
Collaborator

+1 for the clearer naming of convert* methods.

Just to confirm here, the power of 2.4 is chosen to approximate a gamma factor of 2.2, yes? That assumption might benefit from a comment in the code and/or docs.

It seems a little weird to diverge from our existing renderer.gammaFactor default of 2.0, but I agree this value makes much more sense... probably we should change that default as well.

@WestLangley
Copy link
Collaborator Author

No, you have a misunderstanding. Have a look at the Wikipedia sRGB article, and read about the sRGB transfer function.

gammaInput and gammaOutput (and the gamma nomenclature) will be deprecated. We will instead refer to the sRGB color space.

@donmccurdy
Copy link
Collaborator

donmccurdy commented Jun 20, 2018

It's this bit...

It is often casually stated that the decoding gamma for sRGB data is 2.2, yet the above transform shows an exponent of 2.4 ... it goes from gamma = 1 at zero to a gamma of 2.4 at maximum intensity with a median value being close to 2.2. The transformation was designed to approximate a gamma of about 2.2.

...that I find non-obvious. But assuming a change to from gamma nomenclature to renderer.outputColorspace=THREE.sRGBColorSpace would also mean deprecating renderer.gammaFactor, I think this all makes sense.

@@ -329,6 +329,54 @@ Object.assign( Color.prototype, {

},

copySRGBToLinear: function ( color ) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it should be

copySRGBToLinear: function () {

    function SRGBToLinear( c ) {

        return ( c < 0.04045 ) ? c * 0.0773993808 : Math.pow( c * 0.9478672986 + 0.0521327014, 2.4 );

    }

    return function( color ) {
       this.r = SRGBToLinear( color.r );
       this.g = SRGBToLinear( color.g );
       this.b = SRGBToLinear( color.b );

       return this;
   };
}()

so that it will not create function in every call.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oops. Thanks!

@mrdoob mrdoob added this to the r95 milestone Jun 20, 2018
@WestLangley
Copy link
Collaborator Author

r95 milestone? How about right now?

@mrdoob
Copy link
Owner

mrdoob commented Jun 20, 2018

Okay 😛

@mrdoob mrdoob modified the milestones: r95, r94 Jun 20, 2018
@mrdoob mrdoob merged commit ebc162a into mrdoob:dev Jun 20, 2018
@mrdoob
Copy link
Owner

mrdoob commented Jun 20, 2018

Thanks!

@WestLangley WestLangley deleted the dev-sRGB_color branch June 20, 2018 05:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants