Add Color::hlc constructor #70
Conversation
Provide a method to construct colors based on the CIE HLC color space, which is both colorimetrically sound and useful for communication; it is the space advocated by freiefarbe.de. Currently the implementation just creates an 8-bit per channel sRGB value, but as we adopt high-gamut spaces we can make a better color from the same inputs, so hopefully clients won't have to change.
|
Discussion question: is |
|
Coming from CSS |
| /// Change just the alpha value of a color. | ||
| /// | ||
| /// The `a` value represents alpha in the range 0.0 to 1.0. | ||
| pub fn with_alpha(self, a: impl Into<f64>) -> Color { |
There was a problem hiding this comment.
I think this is a useful function, but it's weird in the context of the other color constructors, which take u32.
Thinking about this a bit more, I think that having those other constructors accept u32 is a mistake, especially in light of the new hlc constructor. It would make more sense if rgb just took three u8s, and rgba took four.
This also makes me wonder if we couldn't have a ColorComponent and/or AlphaComponent trait, that could be implemented for f64 and for u8. It would also be nice if we had bounded numeric types (not sure if there's a better term here) so we could have f64<0..=1> and f64<0..=100> or something similar to verify inputs.
There was a problem hiding this comment.
So this is all deep waters. Color is one of those fantastically deep and complex topics, and I consciously want to avoid that - most actual 2D rendering API's represent it with 32 bits of state, and I want to respect that.
Adding extra traits gives me a YAGNI feeling. My feeling here is that we need to start with the requirements of what it actually does, (to which there's virtually no bottom), then design the type to support that. What it does today is a 32 bit RGBA value.
I hope I've addressed some of your uneasiness by having the low level constructors named with the number of bits. The ones that don't have specific bit naming take higher level, less representation-specific components.
This is tricky territory, and I don't mean to fight. I'm very open to improvements that don't bring in needless complexity or performance issues and move us towards a more principled approach to color. But I think it's not obvious now what that is, and I'd like to defer the deep stuff until we actually need it.
There was a problem hiding this comment.
Would having an additional with_alpha8 feel more orthogonal? Then most of the basic constructors have both 8 bit per sample and float variants.
There was a problem hiding this comment.
So I'm happy to represent this with 32 bits internally, but it isn't clear that there's any advantage to taking a u32 instead of 1, 3, or 4 u8s. In particular, I find that needing to include type information in the method name is unidiomatic, when we could easily have that information contained in the signature; i.e. with rgb(r: u8, b: u8, g: u8) -> Color, and rgba(r: u8, b: u8, g: u8, a: u8) -> Color.
When I think about adding a new trait, my main thinking is that it would be nice to say with_alpha(0xAA) or with_alpha(0.66); a minor point of convenience.
There was a problem hiding this comment.
Hmm, I think I see the point. I think a sticking point for me is how 1 should be interpreted. Currently it's Into<f64> so becomes 1.0. Having 1 and 1.0 behave (very) differently seems maybe confusing.
Btw, with the ranged types, for wide gamut values outside the normal range are potentially useful.
To me the advantage of u32 is that you can pass around in a single scalar, rather than doing byte packing and unpacking.
I get what you're saying about the type info in the method name being unidiomatic, but it feels okay to me because it's a very specific representation. To me, the goal of the color type is not to create an abstraction, but to make the existing low level representation a bit more principled.
There was a problem hiding this comment.
I think the particular argument for splitting up the input argument for rgb is that it lets us have all of our constructor signatures match.
Also, in use when I type Color::rgb24(0x_FF_00_00) I'm already basically typing three different arguments, I'm just separating them with _ instead of ,; and for legibility I'm generally going to need to include all the fields, so I'm never going to be writing like Color::rgb24(1337).
There was a problem hiding this comment.
so yea, let's limit this discussion to whether or not it is better, in constructor functions, to take arguments as a single u32 or multiple u8s. I am happy to store a single u32 internally, and I'm happy to shelve any talk about fancy traits, at least for the time being.
There was a problem hiding this comment.
I take your ergonomics point about the value of having (r: u8, g: u8, b: u8) as one possible constructor signature, and would be open to renaming the existing rgba32 method from_rgba32 to emphasize that it's more of a representation conversion than a constructor. But then what happens to the float constructor? I'm looking at CSS and not feeling a lot of guidance, their rgb function takes three u8 as you're requesting. It feels maybe a little crufty?
What would you say to having rgb8(u8, u8, u8) (and similarly 4xu8 for rgba8) and otherwise keeping it as it is (or maybe prefixing from_ to the big scalar constructors)?
There was a problem hiding this comment.
yes, sorry to clarify my previous point I'm happy to shelve any talk of float constructors for rgb, although I reserve the right to propose something down the road.
In any case, I think that this should probably be a separate PR; this PR mostly just highlighted the discrepancy.
I think I would just call the (r: u8, g: u8, b: u8) constructor rgb, and the (r: u8, g: u8, b: u8, a: u8) one rgba, e.g. not include the 8; I think they're unambiguous.
|
Looking at https://www.w3.org/TR/css-color-4/#specifying-lab-lch I have another concern. CSS specifies the Bradford Transform for D65 to D50 white point adaptation, and I just do XYZ scaling. (See this page on Chromatic Adaptation for more discussion). I think I might want to have another look at that, or get the input of a real color expert. |
|
@jrus What do you think? I've been experimenting with Bradford Transformation. The difference is subtle, a little more reddish in the H300 - H310 region. It feels like the hue constancy failure is slightly better, but it it's hard to be certain. So I'm inclined to do this matching the CSS math, but would like an expert opinion. |
|
@danburzo also. |
|
I started working on the Lab / LCh color models in culori based on the CSS Color Level 4 spec. In the process I found a small inconsistency (which has been addressed in the spec since then). It seems that the D50 illuminant is predominant across Lab implementations, and the spec aims for best compatibility. I've also made a pull request in |
The Bradford Transformation gives better results for chromatic adaptation than just piecewise scaling of CIE XYZ values. The exact white point values and matrices are taken from the W3C sRGB spec.
|
Thanks @danburzo. I did a little more digging, and this is what I've found (it might be useful for other projects). The w3 srgb spec contains what I would consider a pretty authoritative answer. One of the basic validation tests for this transform is that L=100 C=0 transforms to (1, 1, 1). I've found so far 4 different values for the D50 white point (see colab notebook), and different choices generally give errors after the 3rd decimal place. But if you rely on a D50 of (0.9642, 1, 0.8249) (the official ICC spec value, though considered to have too low a Z value) and matrices strictly from the w3 srgb spec, then you get a white value accurate to 6+ decimal places. I think it would be possible to redo the matrices with a "corrected" D50 (say Z = 0.8251), but from what I can see the difference would be extraordinarily tiny. The srgb spec document gives a recommended D65 to D50 matrix but doesn't explicitly state that it's based on the Bradford Transformation. However, it matches the matrix from Lindbloom to 3 decimal places, so pretty sure it's the same underlying math, just slightly different values for the white point references. So now I'm pretty confident this is the right way to proceed: do the conversions using the ICC standard D50 reference, and matrices taken directly from the srgb spec. This will get us Bradford Chromatic Adaptation, and a chroma error in white less than 1 per million. I'll push a revised PR shortly. One last note: getting numbers and matrices from Wikipedia has led me astray. |
|
I've also updated my HLC colorspace explorer with the same math as the updated PR. |
Thanks @jrus for the suggestion; the old name wasn't specific.
I think you want to have matrices for the forward/reverse transforms which are exact inverses. The spec rounds to some number of decimal places in both ways. It’s not that useful to precisely match the spec beyond 3–4 digits, but having your own code do a round trip without roundoff errors can be helpful. e.g. you can see I used some matrices in https://observablehq.com/@jrus/srgb which are not quite the same as the spec (off by up to 0.5% or something, because I didn’t round everything aggressively to 3ish digits of precision). Most of the digits of the numbers there are practically meaningless, but it’s nice to have the round trip be nearly exact. You could e.g. use the spec’s matrix for one of the directions and then calculate the inverse of that for the other direction. Edit: if you are just outputting 8-bit integers, probably disregard the above for now; that’s mostly relevant for float <-> float. I think matching CSS / ICC profiles / etc. with CIELAB relative to D50 white point with a Bradford CAT to/from other white points seems fine. |
It’s not inherently better. It’s just a good idea to have a reference white point if you are dealing with e.g. both print and screen output. For a greenfield project with an all-screen render target it would be fine to just use D65. If you want compatibility with existing software (e.g. ICC profiles) which was mostly interested in compatibility with CMYK output workflows, then it makes sense to use D50. |
|
@raphlinus there’s not that big a difference in performance between the Bradford CAT and e.g. CAT02 or CAT16 or .... |
|
Merging after discussion on Zulip. Also overriding the CI failure, which is just because rustfmt nightly failed to build - still want to refine CI so it's not sensitive to that. |
Provide a method to construct colors based on the CIE HLC color space,
which is both colorimetrically sound and useful for communication; it
is the space advocated by freiefarbe.de.
Currently the implementation just creates an 8-bit per channel sRGB
value, but as we adopt high-gamut spaces we can make a better color
from the same inputs, so hopefully clients won't have to change.