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

[css-color-4] Sample color conversion code leads to NaN values #9477

Closed
nex3 opened this issue Oct 17, 2023 · 5 comments
Closed

[css-color-4] Sample color conversion code leads to NaN values #9477

nex3 opened this issue Oct 17, 2023 · 5 comments
Labels
Closed as Question Answered Used when the issue is more of a question than a problem, and it's been answered. css-color-4 Current Work

Comments

@nex3
Copy link
Contributor

nex3 commented Oct 17, 2023

The sample code for converting from XYZ to OKLab involves taking the cube root of the result of multiplying the XYZ color by the XYZtoLMS matrix. This is undefined for negative numbers, and returns NaN, despite the fact that the XYZ color space is notionally unbounded and thus could very easily produce negative numbers.

This comes up even with fairly reasonable in-bounds colors. For example, lab(0.01% 35 1) is equivalent to xyz(0.008679770007260038 0.000011070564598794538 -0.0005206593067627204), which converts to LMS as [0.006751691618866409, 0.000022646971037596888, -0.00003832537822546193].

Should the LMS logic clamp channels so that they're 0 at minimum? Should it take the cube root of the absolute value and then match the sign afterwards?

@svgeesus
Copy link
Contributor

svgeesus commented Oct 17, 2023

XYZ was designed (by the use of super-saturated, not physically realizable primaries) to use positive numbers only.

However, negative numbers do crop up as a result of chromatic adaptation and this is the case here: Lab uses D50 and Oklab uses D65.

Should the LMS logic clamp channels so that they're 0 at minimum?

In general we want to avoid clamping intermediate results.

Should it take the cube root of the absolute value and then match the sign afterwards?

That seems a reasonable way to handle it (and is what is done for the extended transfer functions for RGB color spaces).

@nex3
Copy link
Contributor Author

nex3 commented Oct 17, 2023

I've verified that sign-matched cube root round-trips correctly, at least in this case.

Edit: Actually, I just realized that JavaScript's Math.cbrt() actually does do a sign-matched cube root. I was using a different implementation which doesn't, but I think the sample code is actually correct here.

@svgeesus
Copy link
Contributor

Good that JavaScript does a sign-matched cube root in practice, although the ECMAScript standard is curiously silent on the matter and has some weasel words about implementation approximated.

However, it is at least worth a comment, to aid those implementing in other languages.

@Loirooriol
Copy link
Contributor

This is undefined for negative numbers

It's not. (-1)³ = -1, thus ∛(-1) = -1 and ∛(-x) = ∛(-1 * x) = ∛(-1) * ∛(x) = - ∛(x).
Sure, for complex numbers there are 3 possible cubic roots of -1, and the principal one is not real, but for real numbers it's properly defined.

JavaScript's Math.cbrt() actually does do a sign-matched cube root

Sure, because that's what's expected for an odd root. Even roots have no real solution for negative numbers, so Math.sqrt(-1) is NaN.

See https://en.wikipedia.org/wiki/Nth_root#Definition_and_notation

Every positive real number x has a single positive nth root [...]
For even values of n, positive numbers also have a negative nth root, while negative numbers do not have a real nth root. For odd values of n, every negative number x has a real negative nth root.

I was using a different implementation which doesn't

I guess you were using something like Math.pow(x, 1/3), which is not a cubic root. Floating-point numbers can't represent 1 / 3 exactly.

nex3 added a commit to sass/dart-sass that referenced this issue Oct 18, 2023
@svgeesus
Copy link
Contributor

I added a porting hint in the sample code

@svgeesus svgeesus added Closed as Question Answered Used when the issue is more of a question than a problem, and it's been answered. and removed Needs Edits labels Oct 26, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Closed as Question Answered Used when the issue is more of a question than a problem, and it's been answered. css-color-4 Current Work
Projects
None yet
Development

No branches or pull requests

3 participants