Fix error in Delta E algorithm

[facelessuser]

Fixes #84

[LeaVerou]

Thank you for this! Could you explain what issue it fixes?

[LeaVerou]

Is there a reason not to use === here? (question to both you and @svgeesus )

[facelessuser]

No, I didn't use === because I write in many languages and accidentally didn't use JS ===.

[facelessuser]

Thank you for this! Could you explain what issue it fixes?

I believe I explain it in issue #84. The algorithm isn't correct and will yield different results than intended.

c1 * c2 === 0 is very different than c1 === 0 && c2 === 0. I guess if c1 === 0 || c2 === 0 was used then it would have been roughly the same as the product will be zero if either c1 or c2 is zero.

The second case may not be a problem at all. If h1 and h2 are never negative by the time they are evaluated in the second case, then it won't make a difference. I noticed the c1 * c2 issue and went over the algorithm again to catch anything that had been missed. While the second "fix" is more true to how the algorithm describes it, the result may be equivalent. I didn't double check if either h1 or h2 could be negative.

[facelessuser]

I double checked. Since atan2 returns values in the range of -pi and pi and there is a correction of

	if (h1 < 0) {
		h1 += 2 * π;
	}
	if (h2 < 0) {
		h2 += 2 * π;
	}

Then I think h1 and h2 are always positive, and since they are, the result will be the same regardless of the order.

I reverted those changes but kept the c1 * c2 fix.

[facelessuser]

I've been able to confirm that changing from c1 == 0 && c2 == 0 to c1 * c2 === 0 does change the course of the code down different paths, but I'll also admit that I haven't been able to observe a different result because of the change.

So, in general, maybe there is some clever optimization here which is why c1 == 0 && c2 == 0 was chosen? But if a case is discovered in the future that is not correct, it may be difficult to debug as while the result currently seems the same (for cases I chose to force the issue), the algorithm is following different logic.

It is possible that c1 * c2 was simply a shortcut to save some calculation, and the end product is the same either way. I have not explored this further.

[facelessuser]

I ran a much more extensive test, and still see no difference in the final output. As a matter of fact, I even disabled the condition under the c1 == 0 && c2 == 0 so it would always pick one of the other cases, and got no difference. I can only conclude that this must be a shortcut and not impact the algorithm in any way.

C1 * C2 === 0 is more correct (according to the spec), but even though C1 === 0 && C2 === 0 only targets a subset of cases that C1 * C2 === 0 does, I cannot really argue that it affects the output in any way.

IMHO, I think being true to the spec, in this case, is less confusing, but I also can't argue that my change actually improves anything except being more true to the spec 🙃.

So, feel free to close if you find no value in this.

[svgeesus]

C1 * C2 === 0 is correct, so merging.

Many thanks for testing and yes as @danburzo said

when either Chroma is zero, a whole lot of the color difference formula (eqn. 22) is nullified via delta H prime being zero.

but still, as so many other deltaE2000 implementations are incorrect in small ways for some color combinations, I was going for verifiable correctness.

Sadly the Sharma testsuite does not include a test for Eqn(10 being correctly implemented.