-
-
Notifications
You must be signed in to change notification settings - Fork 85
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
Ensure Okhsl and Okhsv return undefined hues for achromatic colors #517
Ensure Okhsl and Okhsv return undefined hues for achromatic colors #517
Conversation
✅ Deploy Preview for colorjs ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
9829553
to
af37eb5
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks!
Will need updating due to the switch from |
Yeah, I was hoping to get in before all that. I'll rebase and update it probably tomorrow if I have time. |
@facelessuser let me know if I can help! |
2e15d98
to
ce85731
Compare
I've rebased and updated to use null. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changes LGTM, thanks!
As a side comment, these spaces appear to be duplicating a lot of logic that they should ideally import from other spaces, but that shouldn't hold this PR up.
@LeaVerou Okhsl and Okhsv is duplicating logic? Which part? Did you mean to comment in the Oklrab PR? I could see the claim that that PR was duplicating code. |
Perhaps it's not exactly duplication, but the conversion of these spaces to/from base is very weird, it seems to be going through sRGB Linear somehow? If so, why not just define sRGB linear as the base? |
It goes through linear sRGB as that is the gamut the Okhsl and Okhsv space are converting to. This is required to find the cusp of the hue slice of Oklab in relation to linear sRGB. It is specifically part of the conversion algorithm, but the algorithm also requires Oklab to calculate the initial chroma and lightness. That's just how the algorithm works. I need to convert to both Oklab and linear sRGB, I can't just pick one. If I require the base to be sRGB linear, then I'll also have to convert it to Oklab right away. |
To be honest, I'm pretty indifferent as to whether the base is Oklab or linear sRGB. If you want the base to be different, I can do it, and instead of having the Oklab to sRGB linear conversion, I'll add the linear sRGB to Oklab conversion. It's all the same to me. It all amounts to pretty much the same thing. |
For context, the original algorithm did conversion back and forth between llnear sRGB and Oklab, but I've made the initial base Oklab and only require a conversion to sRGB linear, not from, simplifying the code. |
If it needs both color spaces, then I guess up to you. |
I'll leave it as is unless a specific need arises requiring such a change. |
Fixes #516