Implement PartialEq/Eq for all colorspaces, Alpha, PreAlpha, and LabHue/RgbHue

[okaneco]

Implement PartialEq/Eq for:

• Alpha, PreAlpha, Luma, Rgb, Hsl, Hsv, LabHue/RgbHue, Hwb, Lab, Lch, Xyz, Yxy

closes #206

[okaneco]

I'm not sure if I missed any wrappers.

I don't think we can easily implement Eq on the other spaces because they require T to be some form of FloatComponent, Float, or FromF64. I first realized this when I tried to impl it for Hsl which led me to not being able to impl it in the make_hues! macro.

[Ogeon]

Thanks for picking this up as well! I think it's hitting some legacy parts of the library, so there are some things I would have done different today with all the hindsight I have accumulated. Particularly different trait requirements and to not derive traits that apply themselves to all type parameters. 🤔

My intention with implementing Eq everywhere was for situations like if someone uses it with a float wrapper (like https://crates.io/crates/noisy_float) that allows Eq, or I guess something like fixed point numbers that happen to fulfill the trait criteria. The rounding issues with floats would still be a thing, but it would at least be possible to check if color_a is a copy of color_b. I'm thinking it would be like this for the hues impl<T: Float + FromF64 + Eq> Eq for $name<T> {}. Even if it doesn't match that many, or any types at the moment.

Also, in hindsight once again, I'm not sure if there's a good reason to require hues to implement Float. I prefer more granular traits these days and wouldn't mind reducing dependencies on Float. But that's more of a separate discussion where I can dream about how I would wish it to work. 🤷‍♂️

I think the only other wrapper is PreAlpha for pre-multiplied alpha in blending. Easy to forget.

[okaneco]

I added PreAlpha, it's hidden pretty well.

I didn't even try to derive Eq at first but I believe that's what you meant. Is this what you meant by We don't really need to require this meaning the handwritten impls?

And yes, the traits are a bit broad it seems. I agree that hues probably shouldn't need to be Float.

[Ogeon]

Sorry, I wasn't clear regarding the deriving. I meant the opposite, that deriving adds unnecessary requirements in some cases, like requiring white points, standards and spaces to also implement the traits. I would prefer a hand written impl that only requires the component type to implement the same trait.

[okaneco]

This seems like it might be a bigger change than I anticipated. If I understand correctly, the struct definitions would all need to have their trait bounds removed? Currently, it seems that because the struct definitions contain trait bounds, any impl of Eq/PartialEq would need the bounds on the white points/spaces/standards even if handwritten.

Lab would change as such:

- pub struct Lab<Wp = D65, T = f32>
- where
-     T: FloatComponent,
-     Wp: WhitePoint,
- {
+ pub struct Lab<Wp = D65, T = f32> {

Then handwrite the impls like this:

impl<Wp, T: PartialEq> PartialEq for Lab<Wp, T> {
    fn eq(&self, other: &Self) -> bool {
        self.l == other.l && self.a == other.a && self.b == other.b
    }
}

impl<Wp, T: Eq> Eq for Lab<Wp, T> {}

Does this seem like the correct approach?

[Ogeon]

No, I'm just talking about the traits in the derive. But let's limit it to the *Eq traits here. Anything else is for future changes. It would have to be like this, if I'm not missing something:

impl<Wp, T> PartialEq for Lab<Wp, T>
where
     T: FloatComponent + PartialEq,
     Wp: WhitePoint,
{
    fn eq(&self, other: &Self) -> bool {
        self.l == other.l && self.a == other.a && self.b == other.b
    }
}

impl<Wp, T> Eq for Lab<Wp, T> 
where
     T: FloatComponent + Eq,
     Wp: WhitePoint,
{}

So, yeah, not a tiny change, but a good quality of life improvement.

[okaneco]

Ah, that works. Thanks for the guidance.

I had to add a a Component trait bound to Alpha's AbsDiffEq, RelativeEq, and UlpsEq impls for T. I'm not sure if that's required. PreAlpha's impls of those traits has the corresponding bound of Float.

---

For adding trait bounds to struct types, it seems to be discouraged except in special cases. Since the bounds exist on the impl blocks, they don't have to be on the generic parameters in the struct definition in most cases. That's not related to this topic though.
https://rust-lang.github.io/api-guidelines/future-proofing.html#data-structures-do-not-duplicate-derived-trait-bounds-c-struct-bounds

[Ogeon]

I bet the Component here is what makes it require them for the other comparison traits. It shouldn't be necessary here and for Eq. 🙂

[Ogeon]

Very nice! You are right about the trait bounds on structs. They can probably be removed in most cases. That may even open up some use of const fn, since the trait bounds are among the blockers there! But yes, separate topic. There's a bunch of these "small" things that can be improved.

[Ogeon]

Alright, but let's merge. Thanks!

bors r+

[bors]

Build succeeded:

• continuous-integration/travis-ci/push