Color ergonomics improvements

[raphlinus]

This is split off from discussion in #70. The current Color::rgb24(0xrr_gg_bb) is deemed not very rust-like. Let the bikeshedding begin!

I think the most appealing proposal on the table is to let Color::rgb(r: u8, g: u8, b: u8) be the primary constructor. We can rename the u32 constructors from_rgba32 and the like. The existing Color::rgb that takes three float values would have to be renamed. What to? Perhaps we keep that as-is and use Color::rgb8 for the new constructor?

Dealing with the alpha value is tricky. A Color::rgba that takes an additional a: u8 feels consistent but that does mismatch CSS, which uses a float for alpha.

There are other proposals in the thread.

Another consideration is the use of Into<Trait>. This prevents the constructor from being const, so I'd prefer the primary constructors not to use it, but to favor concrete types.

Lastly, we might want to clarify our thinking on whether the RGB values are defined to be interpreted as sRGB. That's more of a documentation issue than one involving constructors though.

[SimonSapin]

I would say don’t worry too much about alpha having different syntax in CSS. In CSS Color Module Level 4 you can have rgb(…) functions with percentages (which are sort of closer to 0.0 .. 1.0 floats than 0 .. 255 integers), and #RGBA / #RRGGBBAA with four-digits or height-digits hex values.

So pick a naming convention and have four combinations:

• r: u8, g: u8, b: u8
• r: u8, g: u8, b: u8, a: u8
• r: f32, g: f32, b: f32
• r: f32, g: f32, b: f32, a: f32

… and clarify in docs what the type actually stores. If it’s 32 bits, the constructors taking f32 are doing some rounding.

[cmyr]

So I think the only thing that needs clarity here are the names.

I think that doing rgb and rgb8 is okay, but does feel just ever so slightly bad. The alternative is something like this, which would allow you to write rgb(0x33, 0x44, 0xFF) or rgb(0.5, 1.0, 0.3), and which has (I think) very clear semantics in both cases, which could be clearly documented:

trait RgbaScalar {
    fn to_u8(self) -> u8;
}

// by design, all arguments must be the same concrete type; no `rgb(0xFF, 0.5, 0x23)`.
fn rgb<T: RgbaScalar>(r: T, g: T, b: T) -> Color {
    Color::rgb24((r.to_u8() << 24) | (g.to_u8() << 16) | (b.to_u8() << 8))
}

impl RgbaScalar for u8 {
    fn to_u8(self) -> u8 { self }
}

// maybe also separately for f64 
impl RgbaScalar for f32 {
    fn to_u8(self) -> u8 { 
        (self.max(0.0).min(1.0) * 255.0).round() as u8
    }
}

I'm not sure that this design is worth the hassle, but I do think it is an entirely reasonable design. The main downside, to me, is that it is that the code is slightly more confusing to read; the win is (hopefully) that the API is slightly easier to use.

[raphlinus]

I don't have very strong feelings on this, so will defer to any consensus. I see the upside of this proposal (RgbaScalar), but there are two paper cuts. First, having a trait means the function can't be const fn, while using a concrete type can. Second, you have the situation where rgba(1, 1, 1, 1) is a nearly transparent very dark gray, as opposed to rgba(1., 1., 1., 1.) which is white.

To me, the extra noise of calling it rgb8 and rgba8 slightly outweighs the convenience factor of not naming the type explicitly. But as I say this feels like a judgment call.

ETA: I had no idea GitHub formatting would inline the color values! Handy, and it happens to match the proposed syntax :)

[raphlinus]

Also on this thread, in Zulip discussion we've considered the idea that the ergonomic win for Color::hlc outweighs the explicitness of cielab_hlc. We won't have other HLC color spaces in piet, and we definitely refer to the docs to explain details such as the color science basis for the method.

[cmyr]

I think that's the right decision, documentation feels like the correct place to describe implementation details.

[raphlinus]

I assume that adding a Debug impl on Color would count as an ergonomics improvement also?

[cmyr]

I think all public types should have Debug, and most should have Clone (the exceptions being where an API very intentionally disallow cloning)