Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Add Color::hlc constructor #70

Merged
merged 4 commits into from Jul 28, 2019
Merged

Add Color::hlc constructor #70

Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
e45d8d0
Add Color::hlc constructor
raphlinus Jul 25, 2019
74b867c
Improve docs, mostly argument ranges.
raphlinus Jul 25, 2019
d03cba2
Use Bradford Transformation in Color::hlc
raphlinus Jul 26, 2019
41bcc03
Change name to cielab_hlc
raphlinus Jul 27, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
56 changes: 56 additions & 0 deletions piet/src/color.rs
View file
Open in desktop
Expand Up @@ -44,6 +44,62 @@ impl Color {
Color::rgba32((r << 24) | (g << 16) | (b << 8) | 0xff)
}

/// Create a color from an HLC (aka CIEHLC) specification.
///
/// Currently this is just converted into sRGB, but in the future as we
/// support high-gamut colorspaces, it can be used to specify more colors
/// or existing colors with a higher accuracy.
#[allow(non_snake_case)]
pub fn hlc<F: Into<f64>>(h: F, l: F, c: F) -> Color {
raphlinus marked this conversation as resolved.
Show resolved Hide resolved
// The reverse transformation from Lab to XYZ, see
// https://en.wikipedia.org/wiki/CIELAB_color_space
fn f_inv(t: f64) -> f64 {
let d = 6. / 29.;
if t > d {
t.powi(3)
} else {
3. * d * d * (t - 4. / 29.)
}
}
let th = h.into() * (std::f64::consts::PI / 180.);
let c = c.into();
let a = c * th.cos();
let b = c * th.sin();
let L = l.into();
let ll = (L + 16.) * (1. / 116.);
// Produce XYZ values scaled to D65 white reference
let X = 0.9505 * f_inv(ll + a * (1. / 500.));
let Y = f_inv(ll);
let Z = 1.0890 * f_inv(ll - b * (1. / 200.));
// See https://en.wikipedia.org/wiki/SRGB
let r_lin = 3.2406 * X - 1.5372 * Y - 0.4986 * Z;
let g_lin = -0.9689 * X + 1.8758 * Y + 0.0415 * Z;
let b_lin = 0.0557 * X - 0.2040 * Y + 1.0570 * Z;
fn gamma(u: f64) -> f64 {
if u <= 0.0031308 {
12.92 * u
} else {
1.055 * u.powf(1. / 2.4) - 0.055
}
}
Color::rgb(gamma(r_lin), gamma(g_lin), gamma(b_lin))
}

/// Create a color from an HLC specification and alpha.
///
/// The `a` value represents alpha in the range 0.0 to 1.0.
pub fn hlca<F: Into<f64>>(h: F, l: F, c: F, a: impl Into<f64>) -> Color {
Color::hlc(h, c, l).with_alpha(a)
}

/// 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 {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@raphlinus raphlinus Jul 25, 2019 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@raphlinus raphlinus Jul 25, 2019 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would having an additional with_alpha8 feel more orthogonal? Then most of the basic constructors have both 8 bit per sample and float variants.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@raphlinus raphlinus Jul 25, 2019 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

let a = (a.into().max(0.0).min(1.0) * 255.0).round() as u32;
Color::rgba32((self.as_rgba32() & !0xff) | a)
}

/// Convert a color value to a 32-bit rgba value.
pub fn as_rgba32(&self) -> u32 {
match *self {
Expand Down