HSL algorithm can divide by zero

[facelessuser]

Is there a reason the HSL algorithm does not implement the part relating to when L == 0 or L == 1 found on the wiki: https://en.wikipedia.org/wiki/HSL_and_HSV#From_RGB?

The L == 1 is the part that is of particular concern as it causes a divide by zero:

let color = new Color("srgb", [2, 0, -2]).to('hsl');

Will yield:

hsl(30 Infinity% 0%)

I'm not sure if this is an oversight or purposeful, and a clean methodology for handling the case has not been outlined yet.

EDIT: Correction, both these cases can cause division by zero.

[facelessuser]

I've found that simply checking for whether l === 0 or l === 1 is not enough. Consider the following:

Based on the algorithm of 2(V - L) / (1 - |2 * L - 1|) which this library currently uses, during conversion, you may get a very small number, and with this equation, you can still get something that resolves to zero.

> (1.0 - Math.abs(2.0 * 5.204170427930421e-18 - 1))
0

As 2(V - L) / (1 - |2 * L - 1|) is equivalent to (V - L) / min(L, 1 - L) (according to the wiki), I decided to use this as I figured it would be more stable, and it is:

> Math.min(5.204170427930421e-18, 1 - 5.204170427930421e-18)
5.204170427930421e-18

Coupled with the saturation check (l === 0 or l === 1), this seems like a better way to go. At least that is what I found. I don't mind getting together a pull if you'd like to go this route, but I figured I'd leave my findings at least.

[svgeesus]

Also from wikipedia

so it doesn't surprise me that values outside [0 .. 1] give problems.

That said, avoiding blowing up on a divide by zero is clearly beneficial.
The question is, should the inputs be coerced into [0 .. 1] first.

[svgeesus]

As 2(V - L) / (1 - |2 * L - 1|) is equivalent to (V - L) / min(L, 1 - L) (according to the wiki), I decided to use this as I figured it would be more stable, and it is:

Yes, that does seem like a nice refactoring.

Please feel free to make a PR!

This change will also impact HWB, right?

[facelessuser]

The question is, should the inputs be coerced into [0 .. 1] first.

I'm not sure it makes much of a difference, but I personally do in my library.

This change will also impact HWB, right?

In my library it did as I specifically, when going from HWB to HSL, go HWB -> sRGB -> HSL. A matter of fact, basically all my HWB conversions go through sRGB when going to/from HSL or HWB. But Colorjs.io goes from HWB -> HSV -> HSL and that path doesn't have a problem with divide by zero as far as I can tell.

With that said, Colorjs.io does take a path mine does not which looks like it might have issues, particularly this line let s = 100 - (100 * w) / (100 - b);. There doesn't seem to be a check to protect against a divide by zero here.

		hsv (hwb) {
			let [h, w, b] = hwb;

			// Now convert percentages to [0..1]
			w /= 100;
			b /= 100;

			// Achromatic check (white plus black >= 1)
			let sum = w + b;
			if (sum >= 1) {
				 let gray = w / sum;
				 return [h, 0, gray];
			}

			let v = 1 - b;
			let s = 100 - (100 * w) / (100 - b);
			return [h, s, v * 100];
		},

To pull off the divide by zero, the case is pretty contrived, but possible:

> new Color("hwb(0 -10000% 10000%)").to('hsv').coords
[0,Infinity,-9900]

Because I pass through HWB -> sRGB -> HSV, I don't see this issue, but with that said, after you asked this, I ran a quick test, and I don't know that the HWB to HSV conversion is correct in Colorjs.io. Ignoring hue, you can see that value is quite different. I suspect the path through sRGB is fine and that the issue is with the direct HWB -> HSV conversion, but one of them is probably wrong.

> new Color("hwb(37 10% 100%)").to('hsv').coords;
[37,0,0.091]
> new Color("hwb(37 10% 100%)").to('srgb').to('hsv').coords
[300,0,9.091]

[facelessuser]

Yeah, I think this confirms HWB to HSV is not right. Roundtrip is broken.

> new Color("hwb(37 20% 80%)").to('hsv').to('hwb').coords
[37,0.2,99.8]

[facelessuser]

I think I know what the issue is with HWB as well. I can probably fix it and address its divide by zero case as well. I'll submit some pulls tomorrow.

[facelessuser]

All known issues have been fixed in #79. Changes can be reviewed there. I made sure, as discussed, that R, G, B are in the range [0, 1] as well.