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

Add OkLrab and OkLrCh #511

Merged
merged 3 commits into from
Aug 26, 2024
Merged

Conversation

facelessuser
Copy link
Collaborator

Resolves #505

Copy link

netlify bot commented May 2, 2024

Deploy Preview for colorjs ready!

Name Link
🔨 Latest commit 3da03e1
🔍 Latest deploy log https://app.netlify.com/sites/colorjs/deploys/66ccd2a7513a0900087d9d05
😎 Deploy Preview https://deploy-preview-511--colorjs.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@facelessuser
Copy link
Collaborator Author

facelessuser commented May 2, 2024

Only lint issues related to this change were addressed.

@facelessuser
Copy link
Collaborator Author

This will clearly need a rework now that the null changes are in.

@facelessuser
Copy link
Collaborator Author

Rebase completed and is ready for review.

@svgeesus svgeesus requested a review from LeaVerou August 26, 2024 15:59
@@ -430,22 +430,78 @@ const tests = {
expect: [1.0, 0.0, null],
},
{
name: "sRGB red (D65) to OKlab",
Copy link
Member

Choose a reason for hiding this comment

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

good catch, yes

Copy link
Member

@svgeesus svgeesus left a comment

Choose a reason for hiding this comment

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

Approved, one question but not a blocker

{
name: "sRGB white (D65) to OKlrab",
args: "white",
expect: [ 1.0000000000000002, -4.996003610813204e-16, 0 ],
Copy link
Member

Choose a reason for hiding this comment

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

Should this actually be zero and this is cumulative error?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah, cumulative floating point error.

Oklab in Color.js currently yields that for white, so OkLrab will also yield that for white as OkLrab just applies a toe function to the lightness, and at white, there is no change.

> new Color('white').to('oklab').coords
[ 1.0000000000000002, -4.996003610813204e-16, 0 ]

Floating point errors can vary slightly depending on how the original matrix was generated, the exact steps and in what order. The invert matrix result can be slightly different (floating point error-wise) depending on exactly how the inversion is performed (did it use LU decomposition, some other way, what order were operations done in, etc).

This isn't too different for other Lab-like spaces, especially those that employ more complicated calculations:

> new Color('white').to('ictcp').coords
[ 0.5806888810416109, 1.1102230246251565e-16, 2.914335439641036e-16 ]

I think my library has slightly different floating point errors. It yields an exact 1 for lightness, but non-zero results for both a and b.

>>> Color('white').convert('oklab').coords()
[1.0, -4.996003610813204e-16, 1.1102230246251565e-16]

But the same things happen for various other Lab-ish spaces (Y is the lightness in XYB):

>>> Color('white').convert('hunter-lab').coords()
[100.0, 1.912989643463523e-14, -7.46260389005987e-15]
>>> Color('white').convert('xyb').coords()
[0.0, 0.8453085619621621, 2.220446049250313e-16]

@svgeesus
Copy link
Member

Someone more familiar with ts should probably look at the lint errors

@facelessuser
Copy link
Collaborator Author

Someone more familiar with ts should probably look at the lint errors

As far as this review is concerned, none of the lint errors apply to the code changes made in this PR. I consider these all existing, separate issues. I imagine it would be useful for someone to open a separate review to address these once and for all, but I do not consider any the lint errors to be blocking this review as they are unrelated and were not introduced by these changes.

@jgerigmeyer
Copy link
Member

I do not consider any the lint errors to be blocking this review as they are unrelated and were not introduced by these changes

Agreed. If main is merged into this branch, CI may pass even with the lint errors.

@jgerigmeyer jgerigmeyer removed their request for review August 26, 2024 18:36
@facelessuser
Copy link
Collaborator Author

Rebased and fixed imports due to recent changes on main. The lint action now passes. I think this can be considered done.

@facelessuser facelessuser merged commit 40e7a05 into color-js:main Aug 26, 2024
5 checks passed
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.

Add interpolation support for OkLrCH
3 participants