-
Notifications
You must be signed in to change notification settings - Fork 2
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
Clamp RGB to sRGB gamut #10
base: master
Are you sure you want to change the base?
Conversation
// only clamp if necessary | ||
if (okhsv.s > 1 || okhsv.v > 1) { | ||
const { r, g, b } = oklab_to_rgb(okhsv_to_oklab(okhsv)) |
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.
This seems a bit roundabout—converting to RGB just to clamp—but the problem isn’t really fixed otherwise. And I made it as performant as I could be by not clamping unless it’s actually needed (i.e. you can call clamp_color()
for “free” on in-range colors)
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.
Alternate approach
Also, edited the PR notes, but if we wanted to do this the “correct” way, it’d just involve using Culori’s clampChroma(…, 'oklch')
method here instead.
clampChroma()
is the same method used on oklch.com for the “fallback” color which matches Björn’s implementation from my extensive testing.
So the tradeoff here is basically just is 3.8 kB
of weight worth it to fix the problem? I could go either way, honestly
@@ -0,0 +1,90 @@ | |||
<script lang="ts"> |
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.
Added a new /oklab
page to test this out. Just a copy of the original RGB demo, just with the obvious colorspace change
Changes
Fixes #9. Clamps colors to the RGB range.
Worth noting that this is the “bad” way to clamp; Björn Ottosen outlines keeping hue consistent and clamping lightness and chroma, which culori has implemented in their
clampChroma(… 'oklch')
method. I instead took the lazier approach of just clamping to(0, 1)
as a first pass.Culori’s
clampRGB()
function also takes the same lazy approach, but loads a bunch of converter methods for a total of3.6 kB
for what ends up being the same end result.Alternative method
The alternate way of doing it the “right” way has a couple caveats:
3.8 kB
of client weight. Which may not be much all things considered. Also worth noting that even thoughclampRGB()
is a bit bloated,clampChroma()
is not—it’s actually a complex calculation and does involve shifting a color through a couple color spaces to get the accuracy right.But if the alternate method is preferred, it’s an easy swap
Reviewing