-
Notifications
You must be signed in to change notification settings - Fork 7
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 color clipping #6
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -178,6 +178,21 @@ impl ColorSpaceTag { | |
} | ||
} | ||
} | ||
|
||
/// Clip the color's components to fit within the natural gamut of the color space. | ||
/// | ||
/// See [`ColorSpace::clip`] for more details. | ||
pub fn clip(self, src: [f32; 3]) -> [f32; 3] { | ||
match self { | ||
Self::Srgb => Srgb::clip(src), | ||
Self::LinearSrgb => LinearSrgb::clip(src), | ||
Self::Oklab => Oklab::clip(src), | ||
Self::Oklch => Oklch::clip(src), | ||
Self::DisplayP3 => DisplayP3::clip(src), | ||
Self::XyzD65 => XyzD65::clip(src), | ||
_ => todo!(), | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This isn't ideal, but I think this is fine. The expectation is that all three There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Some tags are not yet implemented. To move the concerns (and code duplication) to a single place, perhaps a private There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. (Alternatively, we deal with this when more color spaces are implemented and accept the code duplication.) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We will probably end up with a macro, but keep in mind some methods do really want custom logic; There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I suspect for the amount of things, a macro is probably not necessary. But we can discuss that when the last implementations land. |
||
} | ||
} | ||
} | ||
|
||
impl TaggedColor { | ||
|
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.
Would it be meaningful to have a separate
clip_alpha
method? Semantically, a less than zero alpha is definitely equivalent to zero, and likely similarly for greater than 1?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.
Maybe; would
clip
leave alpha unchanged in that case?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.
I don't think so? The other thing to think about is clipping the alpha for premultiplied
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.
I think that makes a good case to add it as a convenience method. Clipping unconditionally to premultiply would be a performance hit (plus premul math still works out when out-of-bounds, if
alpha
is interpreted as an arbitrary weighting).