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

Kelvin temperatures under 2000 not handled properly #138

Open
Yavieh opened this issue Oct 22, 2020 · 12 comments
Open

Kelvin temperatures under 2000 not handled properly #138

Yavieh opened this issue Oct 22, 2020 · 12 comments

Comments

@Yavieh
Copy link

Yavieh commented Oct 22, 2020

Hey, i have a problem that i can not fix.
Everytime i set the minTemperature of the kelvin slider unter 2000, for example "1800", the minimun will be set to 1000.
If i set the minTemperature over 2000 its working normal.
I dont find any Issue that has the same problem.
Maybe someone can help me.

@Yavieh
Copy link
Author

Yavieh commented Oct 30, 2020

No one else facing the same problem?

@Aircoookie
Copy link

Hey, I can confirm that I can reproduce this issue as well with minTemperature set below 2000.

Additional observations:
With minTemperature set to 2200, actual minTemperature with slider all the way to the left as reported by picker.color.kelvin is 2194.05.

With minTemperature set to 1999 or anything below, actual min temperature is about 1000. However, the visual rendering of the slider reflects the set range correctly, 1400 makes the left end of the slider appear significantly redder than 1800.

Moving the slider the tiniest bit to the right and it instantly maps to the correct range again. Just far left is the issue.

My guess would be that it is some edge case with the conversation between kelvin and RGB colorspace (which is tricky I believe!), which would also explain the rounding issue. But only @jaames will be able to confirm.

Not an issue for me as I will be using the default min of 2200, but it confirms that the issue is not specific to your usage/setup @JackmoCode . For reference, I'm using the minified 5.2.3 release in the latest Chrome version.

@jaames jaames added the bug label Nov 20, 2020
@jaames
Copy link
Owner

jaames commented Nov 21, 2020

Yeah, looks like there's some weird precision issues when converting between color spaces. iro uses HSV internally, but kelvin temperatures are first converted to RGB before updating the internal HSV value, so I think there's some precision loss along the way that's hard to track down.

This reliance on HSV is for legacy reasons -- maybe now it's a good idea to rewrite the color system to something more precise. I'm not an expert on more exotic colorspaces and my maths is pretty weak though, so I'll need to find someone who can help with that.

In the meantime I'll update the docs to recommend 2000 as the safe minimum for kelvin temperature

@jaames jaames changed the title minTemperature under 2000 will be ignored Kelvin temperatures under 2000 not handled properly Nov 21, 2020
@Myndex
Copy link

Myndex commented Nov 23, 2020

Just to chime in briefly: HSV is not perceptual, and I would not be surprised if it was unable to handle whitepoints that far from D65. CIE Luv LCh is a perceptually based polar-coordinate colorspace that is close to ideal for use with monitors and displays, and I'd think is doable with the right matrix.

Here's the great resource for the math for getting into and out of XYZ spaces of different whitepoints or colortemps using the Bradford transformation: http://www.brucelindbloom.com/ he includes a lot of ready to go matrixes, including I believe one for illuminant A which is 2856K. He also has a JS calculator that calculates matrices as needed...

I'm planning on adding LChuv to the colorpicker for the APCA contrast tool... won't get to it till December or January though, but I mention it as I am contemplating using the iro.js framework to that end. I'm not planning on adding whitepoint adjustment (sRGB is D65) but once the sRGB -> XYZ -> Luv -> LCh -> Luv -> XYZ -> sRGB chain is setup, it should be easy to add other matrix transformations...

The sRGB <-> CIE code is ready to go in the APCA repo and also the SeeLab repo, but it is set to D65 only — but a visit the the Bruce Lindbloom site should be helpful to that end.

Andy
A Guy Too Obsessed with Color.

jaames added a commit that referenced this issue Nov 24, 2020
@jaames jaames mentioned this issue Nov 24, 2020
@jaames
Copy link
Owner

jaames commented Dec 27, 2020

Hi @Myndex, thanks for the link! Unfortunately I still don't really understand the more abstract color spaces very well, and it looks like matrix transformations would require a lot of code. So while it seems helpful, I'm a bit tentative about implementing a large amount of new code that I don't understand well enough to debug effectively...

Is there anything simpler I can use?

@bakman2
Copy link

bakman2 commented Sep 26, 2021

As a sidenote, it would actually be nice if the min/max values could be reversed.
eg. in homeautomation, one wants to control from cold to warm, but the slider is reversed, tried a transform scaleY but doesnt work for the handle.

@Aircoookie
Copy link

Haven't tested this assumption, but just a possible cause might be that blue is set to 0 below temperatures of 2000 (https://github.com/irojs/iro-core/blob/bd46ac36225cc4de5ea2a8453cec4f69b8f40ba6/src/color.ts#L305) while it should only be set to 0 for temperatures of or below 1900 according to https://tannerhelland.com/2012/09/18/convert-temperature-rgb-algorithm-code.html (which the kelvinToRgb function could be based on, although the constants seem to differ).

On a side note, I also use the algorithm by Tanner Helland in WLED (https://github.com/Aircoookie/WLED/blob/master/wled00/colors.cpp#L70), and it appears to work fine for input values down to 1000K as intended

@Myndex
Copy link

Myndex commented Dec 1, 2021

Hi @jaames

Is there anything simpler I can use?

Hmmm... Well, in the words of Lindbloom "There is not a simple formula for computing the correlated color temperature of an XYZ color" I consider him an authority on the subject. He does present A. R. Robertson's solution in C (pasted below).

Converting sRGB to and from XYZ is fairly trivial. Once in XYZ, then the code I pasted below from BruceLindbloom. It's in c but should be a trivial port, there's just a couple arrays, a conversion to uv, and some interpolation.

XYZ to Correlated Color Temperature (Copied from BruceLindbloom.com)

An algorithm is used to inverse interpolate one data table, and then forward interpolate a second table. This method was developed by A. R. Robertson. Below is an ANSI 'C' implementation of Robertson's method.

/*
 *      Name:   XYZtoCorColorTemp.c
 *
 *      Author: Bruce Justin Lindbloom
 *
 *      Copyright (c) 2003 Bruce Justin Lindbloom. All rights reserved.
 *
 *      Input:  xyz = pointer to the input array of X, Y and Z color components (in that order).
 *              temp = pointer to where the computed correlated color temperature should be placed.
 *
 *      Output: *temp = correlated color temperature, if successful.
 *                    = unchanged if unsuccessful.
 *
 *      Return: 0 if successful, else -1.
 *
 *      Description:
 *              This is an implementation of Robertson's method of computing the correlated color
 *              temperature of an XYZ color. It can compute correlated color temperatures in the
 *              range [1666.7K, infinity].
 *
 *      Reference:
 *              "Color Science: Concepts and Methods, Quantitative Data and Formulae", Second Edition,
 *              Gunter Wyszecki and W. S. Stiles, John Wiley & Sons, 1982, pp. 227, 228.
 */

#include <float.h>
#include <math.h>

/* LERP(a,b,c) = linear interpolation macro, is 'a' when c == 0.0 and 'b' when c == 1.0 */
#define LERP(a,b,c)     (((b) - (a)) * (c) + (a))

typedef struct UVT {
        double  u;
        double  v;
        double  t;
} UVT;

double rt[31] = {       /* reciprocal temperature (K) */
         DBL_MIN,  10.0e-6,  20.0e-6,  30.0e-6,  40.0e-6,  50.0e-6,
         60.0e-6,  70.0e-6,  80.0e-6,  90.0e-6, 100.0e-6, 125.0e-6,
        150.0e-6, 175.0e-6, 200.0e-6, 225.0e-6, 250.0e-6, 275.0e-6,
        300.0e-6, 325.0e-6, 350.0e-6, 375.0e-6, 400.0e-6, 425.0e-6,
        450.0e-6, 475.0e-6, 500.0e-6, 525.0e-6, 550.0e-6, 575.0e-6,
        600.0e-6
};

UVT uvt[31] = {
        {0.18006, 0.26352, -0.24341},
        {0.18066, 0.26589, -0.25479},
        {0.18133, 0.26846, -0.26876},
        {0.18208, 0.27119, -0.28539},
        {0.18293, 0.27407, -0.30470},
        {0.18388, 0.27709, -0.32675},
        {0.18494, 0.28021, -0.35156},
        {0.18611, 0.28342, -0.37915},
        {0.18740, 0.28668, -0.40955},
        {0.18880, 0.28997, -0.44278},
        {0.19032, 0.29326, -0.47888},
        {0.19462, 0.30141, -0.58204},
        {0.19962, 0.30921, -0.70471},
        {0.20525, 0.31647, -0.84901},
        {0.21142, 0.32312, -1.0182},
        {0.21807, 0.32909, -1.2168},
        {0.22511, 0.33439, -1.4512},
        {0.23247, 0.33904, -1.7298},
        {0.24010, 0.34308, -2.0637},
        {0.24792, 0.34655, -2.4681},	/* Note: 0.24792 is a corrected value for the error found in W&S as 0.24702 */
        {0.25591, 0.34951, -2.9641},
        {0.26400, 0.35200, -3.5814},
        {0.27218, 0.35407, -4.3633},
        {0.28039, 0.35577, -5.3762},
        {0.28863, 0.35714, -6.7262},
        {0.29685, 0.35823, -8.5955},
        {0.30505, 0.35907, -11.324},
        {0.31320, 0.35968, -15.628},
        {0.32129, 0.36011, -23.325},
        {0.32931, 0.36038, -40.770},
        {0.33724, 0.36051, -116.45}
};


int XYZtoCorColorTemp(double *xyz, double *temp)
{
        double us, vs, p, di, dm;
        int i;


        if ((xyz[0] < 1.0e-20) && (xyz[1] < 1.0e-20) && (xyz[2] < 1.0e-20))
                return(-1);     /* protect against possible divide-by-zero failure */
        us = (4.0 * xyz[0]) / (xyz[0] + 15.0 * xyz[1] + 3.0 * xyz[2]);
        vs = (6.0 * xyz[1]) / (xyz[0] + 15.0 * xyz[1] + 3.0 * xyz[2]);
        dm = 0.0;
        for (i = 0; i < 31; i++) {
                di = (vs - uvt[i].v) - uvt[i].t * (us - uvt[i].u);
                if ((i > 0) && (((di < 0.0) && (dm >= 0.0)) || ((di >= 0.0) && (dm < 0.0))))
                        break;  /* found lines bounding (us, vs) : i-1 and i */
                dm = di;
        }
        if (i == 31)
                return(-1);     /* bad XYZ input, color temp would be less than minimum of 1666.7 degrees, or too far towards blue */
        di = di / sqrt(1.0 + uvt[i    ].t * uvt[i    ].t);
        dm = dm / sqrt(1.0 + uvt[i - 1].t * uvt[i - 1].t);
        p = dm / (dm - di);     /* p = interpolation parameter, 0.0 : i-1, 1.0 : i */
        p = 1.0 / (LERP(rt[i - 1], rt[i], p));
        *temp = p;
        return(0);      /* success */

sRGB to XYZ (JS)

From seelab

    /////  sRGB TO XYZ  /////

function sRGBtoXYZ (sR = 255, sG = 255, sB = 255) {

                     // 8bit sRGB to Linear RGB per IEC Standard:
      function piecewise(chan) {
          chan = chan/255.0;
          return ( chan > 0.04045 ) ?
                  Math.pow((( chan + 0.055 ) / 1.055 ), 2.4 ) :
                  chan / 12.92;
      }

       let Rlin = piecewise( sR ), Glin = piecewise( sG ), Blin = piecewise( sB );

        // sRGB D65 matrix into XYZ
       let X = Rlin * 0.4124564 + Glin * 0.3575761 + Blin * 0.1804375;
       let Y = Rlin * 0.2126729 + Glin * 0.7151522 + Blin * 0.0721750;
       let Z = Rlin * 0.0193339 + Glin * 0.1191920 + Blin * 0.9503041;

       return [X,Y,Z]
}

XYZ to sRGB (JS)

From seelab

    /////  sRGB FROM XYZ  /////

function XYZtosRGB (X,Y,Z) {

                      // Linear RGB to sRGB 0.0-1.0 per IEC Standard:
       function piecewiseEncode(linChan) {
            return ( linChan > 0.0031308 ) ?
                   Math.pow(linChan , 0.416666666666667 ) * 1.055 - 0.055 :
                   linChan * 12.92;
       }

                      // Linear RGB to sRGB per IEC, round to INT and clamp 0-255:
       function sRGB8bit(linChan) {
           return Math.max(Math.min(Math.round(piecewiseEncode(linChan) * 255.0), 255), 0);
       }

              // XYZ matrix to sRGB 8 bit
        let sR = sRGB8bit( X * 3.2404542 + Y * -1.5371385 + Z * -0.4985314 );
        let sG = sRGB8bit( X * -0.9692660 + Y * 1.8760108 + Z * 0.0415560 );
        let sB = sRGB8bit( X * 0.0556434 + Y * -0.2040259 + Z * 1.0572252 );

        return [sR, sG, sB] 
}

Two notes, one is that the matrixes shown are for sRGB. P3 and Rec2020 are different.

Also, once in XYZ it's fairly simple to go to LAB or LUV and their LCh or LSh polar versions, which are much more perceptually uniform than HSV or HSL. For LUV LSh, the controls are perceptual lightness L*, saturation S and hue h.

@Christian-Me
Copy link

Christian-Me commented Jan 31, 2022

Hi ... I just fixed this as it is present in other implementations too. Here you can find a quick explenation why the current code fails below 2000°C (and BTW the docs clearly mention that the lower limit is 2000°K)

gka/chroma.js#288

A simple fix but works for me.

IroColor.rgbToKelvin = function rgbToKelvin(rgb) {
      var r = rgb.r,
          g = rgb.g,
          b = rgb.b;
      var eps = 0.4;
      var minTemp = KELVIN_MIN; // 1000
      var maxTemp = KELVIN_MAX;
      var temp;
      while (maxTemp - minTemp > eps) {
        temp = (maxTemp + minTemp) * 0.5;

        var _rgb = IroColor.kelvinToRgb(temp,true); // results with real values

        if (b>0) { // temperatures over 2000k
          if (_rgb.b / _rgb.r >= b / r) {
            maxTemp = temp;
          } else {
            minTemp = temp;
          }
        } else { // below 2000k where blue is constant  0 and red 255;
          if (_rgb.g >= g) {
            maxTemp = temp;
          } else {
            minTemp = temp;
          }
        }
      }
      return temp;
    };

Happy to provide a PR.

@jaames
Copy link
Owner

jaames commented Feb 1, 2022

@Christian-Me Ah! That looks promising!

If you would like to submit a PR, iro.js' rgbToKelvin method lives here in iro-core: https://github.com/irojs/iro-core/blob/typescript/src/color.ts#L322

I'm happy to handle updating the main iro.js package for these changes :)

@Christian-Me
Copy link

@jaames Happy to do this over the weekend ... 😁

@sblantipodi
Copy link

@jaames / @Christian-Me is that pull request ready to be merged?

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

7 participants