Add {into,from}_u32 methods for RGB/A, Packed struct for u32 representations of RGBA

[okaneco]

Add into_u32 and from_u32 methods for Rgb and Rgba.
Add Packed struct which stores Rgb/a colors as a packed u32 with support for multiple channel orders: ABGR, ARGB, BGRA, and RGBA.
Add RgbChannels trait for splitting and combining of Rgba components in the previously mentioned channel orderings.
Add corresponding From impls for Packed, Rgb/a, u32

Closes #144

[okaneco]

There are a few different ways to do this and I wasn't sure what was best. I've seen all different orderings in libraries I've used, so I figured it's best to include them all.

I wasn't sure if I should implement any further derives on the structs like serialization or #[repr(C)] since it only has one field. Another way of attacking this was with a trait but I chose a struct with only one field so that the types couldn't be mixed together unintentionally.

---

I ended up going back and figuring out the trait bounds for hex methods inside of Rgb/Rgba itself. It didn't save much in the packed structs but they're nice convenience methods to have that save typing all the commas or periods when prototyping.

[Ogeon]

This is an interesting way of tackling it. I would probably just have made a simple conversion method like from_hex(ordering: ChannelOrdering, value: u32) -> Rgb<S, u8> (where ChannelOrdering is an enum) and nothing more, but I see the value of making it an actual type if it's part of a larger workflow.

Thinking a bit about this, I think making to/from_hex be parametric on the value type would be to over engineer it for no good reason if there are separate packed types that can have their own converters. The bare u32 is the odd one, since it doesn't have a defined channel ordering.

Just to exchange ideas, would something like this make sense? See it as a sketch, but the idea is to twist the concept a bit to turn the channel ordering into a type parameter (allowing operations on any packed RGB) and unify that with the {from,into}_hex methods. The downside is, of course, that it's complex in a different way and it forces the user to be aware of the channel ordering.

/// Splits and combines RGB(A) types with some channel ordering.
pub trait RgbChannels {
    fn split_rgb<S>(rgb: Rgba<S, u8>) -> (u8, u8, u8, u8);
    fn combine_rgb<S>(channels: (u8, u8, u8, u8)) -> Rgba<S, u8>;
}


struct Abgr;
impl SplitChannels for Abgr {
    //...
}

//...

// Adding Pixel here makes it easy to convert to and from slices of u32.
#[derive(..., Pixel)]
#[repr(transparent)]
struct PackedRgb<C: RgbChannels> {
    color: u32,
    channel_order: PhantomData<C>, // This is not a nice aspect of this, but implementing `From<u32>` should make it less painful to construct.
}

impl<S, C: RgbChannels> Into<Rgb<S, u8>> for PackedRgb<C> {
    fn into(self) -> Rgb<S, u8> {
        //...
        C::combine_rgb((c1, c2, c3, c4)).color
    }
}

// rgb.rs:
impl<S> Rgb<S, u8> {
    fn from_hex<C: RgbChannels>(color: u32) -> Self {
        PackedRgb::<C>::from(color).into()
    }

    fn into_hex<C: RgbChannels>(self) -> Packed<C> {
        PackedRgb::from(self).color
    }
}

Whichever way it's done, I think it's best to start with a conservative approach and only allow it for RGB(A)8, to begin with. That's the most straightforward one and anyone who want to convert to any other component format can easily do it anyway.

[okaneco]

I liked those ideas a lot and thought they would clean up some parts nicely. 👍

It was very helpful and chopped down about 100 lines of repetitive code. Luckily I had the test data all there waiting for the implementation to finish which gave me confidence in refactoring, and the tests barely needed changing.

During my first go, I did try some type of channel ordering marker data but that and other complexity got in the way of a rough initial implementation. The bounds trip me up with the error messages not being that helpful sometimes and any time I write what I think should work I run into features that are currently unstable, but I think I'm starting to understand a bit more.

---

In no particular order, there are a few implementation details which came up in the refactor that I'm not sure about.

RgbaPacked

The four impls of RgbChannels are on RgbaPacked, Argb, Bgra, or Abgr. It's a horrible name but I didn't want to collide with the type Rgba (I was confused coming across Srgb the type and struct when first using the library). I initially named it RgbaDefault since the rest of the crate is RGBA order and set it as the default for the trait RgbChannels. RgbaPacked feels verbose and confusing since the containing struct is called PackedRgb.

pub struct PackedRgb<C: RgbChannels = Argb> {
    /// The sRGB color packed into a `u32`.
    pub color: u32,

    /// The channel ordering for red, green, blue, and alpha components in the
    /// packed integer; can be `Argb`, `Abgr`, `Bgra`, or `RgbaPacked`.
    #[palette_unsafe_zero_sized]
    pub channel_order: PhantomData<C>,
}

Defaulting to Argb and writing Hex values

This continues from the previous point. An observation made during the first implementation was that I normally think in RGBA like the library, but when I write hex RGB values I'm really thinking ARGB (0x00rrggbb). The rest of the crate is RGBA, why should this be ARGB?

Because the least surprising thing to me for Rgb::from_hex(0x112233) would be getting rgb(0x11, 0x22, 0x33) and not rgb(0x00, 0x11, 0x22). For this reason I decided to make ARGB the default channel ordering for PackedRgb<C>.

Rgb packing, 0x00 or 0xFF for dummy alpha?

When creating a PackedRgb from Rgb type, it needs an alpha value for one of the bytes. I can see arguments for either 0 or 255. I chose 0 for now because with Argb, Rgb::into_hex would return 0x112233 while RgbaPacked would be 0x11223300. The first result is what I would expect however it might not be obvious to users that Argb is the default since there's a chance they haven't had to think about this. That's why I do think it's good that channel order is maintained as a parameter for users to keep in mind.

Type annotations

I had some slight frustrations with how it still needs the type annotation despite a default being specified, is there something more I could do so that's not needed? I don't really understand why the Argb is needed here, and gets slightly long to type with needing to specify the u8 as well (I know that's being worked on). I can see users expecting Rgba to go there and being surprised with results, I've added some more comments mentioning ARGB to the functions in rgb.rs.

let unpacked = Srgb::<u8>::from_hex::<Argb>(0xFF8000);

[Ogeon]

As a concept, it seems to work out. I think it's possible to iron out a couple of those remaining wrinkles with good shortcuts.

I addressed some of your point as inline comments too, but overall, I'm open for alternate names (and module structures) to everything, wherever it helps. We are essentially implementing the same concepts (RGB encodings) in two different formats, so name collisions are unfortunately inevitable to some extent.

Looking around online, it seems like it's mostly just RGBA when the alpha channel is mentioned, and [?]RGB when it's not there (an emergent side effect of the bit shifting, I suppose). This makes it hard to really give a universal default, so ARGB is as good as any if a default is necessary. This is more of a "soft" property of the library that's hard to say if it's an improvement or not without studying how it's used. It may even be that it's possible for the language to infer it from containers or return types where it ends up being used.

As for the default for alpha, I wonder if it would be any bad consequences if we pick 0xFF. I'm thinking it may even have the opposite effect of being helpful when it's interpreted on the receiving end.

I'm not sure why it doesn't infer u8 components as the only alternative, but type inference and defaults can be weird sometimes. I believe the issue may be minimized if we implement and maybe somehow promote From<u32> as a shortcut. Then it would just be Srgb::<u8>::from(...) at worst.

[okaneco]

As for the default for alpha, I wonder if it would be any bad consequences if we pick 0xFF. I'm thinking it may even have the opposite effect of being helpful when it's interpreted on the receiving end.

I changed to 0xFF again because that's probably more in line and expected during use to have full alpha for an RGB color. If you want 0x00 for the alpha use an Rgba with the same hex number instead of Rgb so we have the best of both worlds.

I believe the issue may be minimized if we implement and maybe somehow promote From as a shortcut. Then it would just be Srgb::::from(...) at worst.

What would this look like roughly? If I try to implement for Packed<C> and again for Packed<Argb> I get an error for conflicting implementations of a trait. Stabilization isn't stable and the closest thing I could find was https://github.com/dtolnay/case-studies/blob/master/autoref-specialization/README.md but it doesn't seem like it'd work with the trait bounds in this case.

[okaneco]

I was able to simplify the From<Rgb> but not Into<Rgb>.

This is the current implementation. I tried to piggyback on Rgba when I could it didn't seem possible to use anything more.

fn into(self) -> Rgb<S, u8> {
    let bytes: [u8; 4] = self.color.to_be_bytes();
    let color: Alpha<Rgb<S, u8>, u8> = C::combine_rgb((bytes[0], bytes[1], bytes[2], bytes[3]));
    Rgb::from_components((color.red, color.green, color.blue))
}

Is there an easier way to convert from Rgba to Rgb in this case?

error[E0277]: the trait bound `u8: FromF64` is not satisfied
   --> palette/src/rgb/packed.rs:158:19
    |
158 |         Rgb::from(color)
    |                   ^^^^^ the trait `FromF64` is not implemented for `u8`
    |
    = note: required because of the requirements on the impl of `component::FloatComponent` for `u8`
    = note: required because of the requirements on the impl of `std::convert::From<alpha::Alpha<rgb::rgb::Rgb<S, u8>, u8>>` for `rgb::rgb::Rgb<_, u8>`
    = note: required by `std::convert::From::from`

[Ogeon]

What would this look like roughly?

We are probably in name confusion land. I had this in mind:

impl<S: RgbStandard> From<u32> for Rgb<S, u8> {
    fn from(color: u32) -> Self {
        Self::from_u32::<Argb>(color)
    }
}

impl<S: RgbStandard> From<u32> for Alpha<Rgb<S, u8>, u8> {
    fn from(color: u32) -> Self {
        // Notice the ordering here, going for the seemingly popular one.
        Self::from_u32::<Rgba>(color)
    }
}

That way one can get quick and dirty default conversions. I suppose From<Rgb<...>> for u32 is also possible now.

[okaneco]

Oh I see what you mean now. That works nicely and gives us a somewhat sane "universal" behavior. I assume that should live in packed.rs and not rgb.rs.

I changed the other Into for Rgba to a From. I implemented From<Rgb<...>> for u32 since I could. The other cleanups removed a lot of clutter. Thanks for explaining the Alpha, I didn't fully grasp it until then.

[Ogeon]

I would probably have placed it in rgb.rs, since it doesn't mention anything from packed.rs publicly, but both are valid option! (No need to change)

Yes, the Alpha type is a bit magical. Deref is not really meant to be used that way, since it's not really a reference, but I figured being a wrapper would be close enough and that the benefits would outweigh the costs. Like with Packed and Rgb and all other types, it reduces them from combinatorial monsters to something more manageable.

Anyway, I do really like the way it's implemented now! Very nice! Thanks! The only improvements I can think of now is just a tiny bit of documentation polish:

• Mentioning/showing u32::from(Rgb) and Rgb::from(u32) in the documentation for from_u32 and into_u32, just to make people more aware of them.
• Maybe demoing converting a &[u32] to &[Packed], via the Pixel trait. It has a few examples of its own that can be used as inspiration. It's also a free unit test.

Essentially a couple of "did you know..." examples, that I think may make someone's life a bit easier. Other than those details, I'm ready to merge when you are!

[Ogeon]

Nice! I think that's all for this one! Feel free to squash it.

[okaneco]

Those examples were good ideas, and I'm happy with how this turned out. 👍

Massive Github outage so wasn't able to post comments or push. I'll squash this now.

I've not used the Pixel trait before so I wasn't sure what the example should show.

I was trying to think of ways to improve discovery or surface these features. I noticed there's no Rgb or Srgb structs or types visible on the doc page and wasn't sure why that is, is that due to re-exports? There are plenty of doctest examples that show how to use them but you have to know to click the rgb module to see the structs/types.
https://docs.rs/palette/0.5.0/palette/#modules

[Ogeon]

I think the fact that it refers to the Pixel trait, combined with the small and straight forward example, shows what it may be good for.

You are right in that it's not super easy to find examples from the documentation root. Part of why I haven't put the Rgb type there is that I don't want to encourage people to use it over the Srgb and LinSrgb aliases. It was something I noticed in an earlier version. People tend to reach for the familiar name, but the Rgb type is not that ergonomic on its own. It's often nicer to make an alias for it, especially because one will often need two versions (linear and non-linear) of it.

The examples and documentation is in need of a bit of an overhaul, I think, but that's much larger than this PR. Hence why I'm not requiring anything more fancy than this. I would like to document various common patterns too, a bit like making a cookbook, for people to look up how to do things like read and write to images and/or pixel buffers, how to interoperate with other graphics crates, how to set up a basic linear pipeline, and all sorts of other things one may use this for.

[Ogeon]

But the point of the Pixel trait is to be glue code for converting arrays and slices to Palette types. If you have some data that can be represented as a slice or array, you can turn it into a Palette color without copying anything. It's pretty neat to be able to take a massive &mut [u8] from somewhere, turn it into &mut [Srgb<u8>] with almost no cost, and get all the type system benefits.

[okaneco]

That makes sense, I can imagine some applications with a slice of u32.

I think that wraps this up. I accidentally re-inserted a gap between the documentation and the Packed struct when I added the Pixel example; with the gap, there's no context for the documentation to appear when I hover over the struct in my IDE. I also removed a type annotation in the assert that was leftover from copying the value.

[Ogeon]

Alright! Let's get it merged.

bors r+

[bors]

Build succeeded

• continuous-integration/travis-ci/push