Add color clipping #6
Conversation
color/src/colorspace.rs
Outdated
| @@ -206,6 +245,10 @@ impl Colorspace for XyzD65 { | |||
| ]; | |||
| matmul(&LINEAR_SRGB_TO_XYZ, src) | |||
| } | |||
|
|
|||
| fn clip(src: [f32; 3]) -> [f32; 3] { | |||
| src | |||
There was a problem hiding this comment.
Should we explain why XYZ does not clip? In any case, I think an argument should be made to disallow negative values, as they have no possible physical manifestation.
There was a problem hiding this comment.
I'm not sure about clamping XYZ, e.g., changing the reference illumination by going from XYZ D65 to D50 negative values often roll out of the maths (but only, I believe, for quite extreme colors). I don't know whether those extreme colors prior to changing the reference are physically possible.
If my understanding is correct, those colors at least are impossible under the new reference illumination, but given that XYZ is probably mostly used exactly for its color representation under a reference, maybe negative values should be allowed?
I followed CSS Color 4 in not clamping XYZ, but I don't have strong feelings about this, and even less strong confidence in my knowledge on the topic.
There was a problem hiding this comment.
I've merged the PR – a follow-up could be done either commenting clip for XYZ, or, more generally, what the components represent for each color space (and, in particular, what their natural bounds are).
Clipping colors to colorspaces' bounds is useful for e.g. final display or as part of gamut mapping. A related method would be `Colorspace::is_in_bounds(src: [f32; 3])`, but I'm undecided whether that's useful enough to include. It could have a default implementation (`src == Self::clip(src)`). Roughly the same considerations hold for a const `Colorspace::IS_(UN)BOUNDED`.
| pub fn clip(self) -> Self { | ||
| let (opaque, alpha) = split_alpha(self.components); | ||
| let components = self.cs.clip(opaque); | ||
| let alpha = alpha.clamp(0., 1.); |
There was a problem hiding this comment.
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.
Maybe; would clip leave alpha unchanged in that case?
There was a problem hiding this comment.
I don't think so? The other thing to think about is clipping the alpha for premultiplied
There was a problem hiding this comment.
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).
| Self::Oklch => Oklch::clip(src), | ||
| Self::DisplayP3 => DisplayP3::clip(src), | ||
| Self::XyzD65 => XyzD65::clip(src), | ||
| _ => todo!(), |
There was a problem hiding this comment.
This isn't ideal, but I think this is fine. The expectation is that all three todo!s in this file would be fixed as a block
There was a problem hiding this comment.
Some tags are not yet implemented. To move the concerns (and code duplication) to a single place, perhaps a private match-like macro that takes a ColorSpace method invocation makes sense for the ergonomics. ColorSpace is not currently object-safe so we can't go through a &dyn ColorSpace.
There was a problem hiding this comment.
(Alternatively, we deal with this when more color spaces are implemented and accept the code duplication.)
There was a problem hiding this comment.
We will probably end up with a macro, but keep in mind some methods do really want custom logic; convert is the big one.
There was a problem hiding this comment.
I suspect for the amount of things, a macro is probably not necessary. But we can discuss that when the last implementations land.
|
I'll put this comment here. I'm pretty sure negative XYZ values don't occur in the normal course of events. Of course negative RGB values will occur in any RGB-based color space, for values in the chromaticity horseshoe but outside the RGB triangle. But I think it's basically an academic question, as clipping in XYZ is also seldom useful. |
Clipping colors to colorspaces' bounds is useful for e.g. final display or as part of gamut mapping.
A related method would be
Colorspace::is_in_bounds(src: [f32; 3]), but I'm undecided whether that's useful enough to include. It could have a default implementation (src == Self::clip(src)). Roughly the same considerations hold for a constColorspace::IS_(UN)BOUNDED.