Add conversion trait

[sidred]

This commit adds a FromColor and IntoColor conversion trait. These traits require from_xyz and into_xyz respectively to be implemented and auto derive the conversions for other colors.

To implement FromColor and IntoColor for a type, a few things must by kept in mind.

• Derived colors like Hsl must override the default from_rgb and use this for from_xyz for efficiency.
• The FromColor for the same color must override the default. For example, from_rgb for Rgb will convert via Xyz which needs to be overridden with self to avoid the unnecessary conversion.
• If a direct transform exists between the colors, then it should be overridden in both the colors. For example, Luma has direct conversion to Rgb. So from_rgb for Luma and from_luma for Rgb override the defaults.

[Ogeon]

This depends on an unstable feature, so it won't work in stable and beta. It's an interesting idea and I would like to entertain it a bit more, but I'm not willing to drop stable and beta support for it.

That being said, any improvement that can minimize the amount of impl From spamming is most welcome. The good thing about the way From and Into works is that they are generic, so you don't have to have from_* and into_* for every single type. It's also trivial to integrate custom types and make them work like the other. The more specific conversion functions can still be added for extra convenience.

[sidred]

I am not sure I understand what you are trying say regarding from_, into_ and custom types.

I have removed the negative impl trait (unstable feature) and used a macro instead. Do you think this is any better?

[sidred]

One of the reasons I was looking at this was to avoid rewriting macros, for some of the upcoming changes regarding WhitePoint and RgbVariants. I guess we can't really avoid it.

I think there is an advantage in having a conversion trait and I am happy to make these changes if you see a benefit.

[Ogeon]

Oh, wait, now I get it! I must have been too distracted... This is actually a very nice approach, and making ColorConvert public would probably be even nicer. I thought it was a replacement for From and Into, but I see that it's not.

[sidred]

I have implemented the color trait and changed the default behaviour of xyz -> rgb to clamp. All color enum / alpha From traits implemented via macro

[Ogeon]

I only skimmed through it, so far, but this looks very nice. One thing, though; wouldn't it be a non-breaking change if we didn't clamp? Also, why only clamp RGB? An other alternative for now, provided that it's not breaking, is to add generic clamped_from<C> and clamped_to<C> functions. That would keep status quo for now, and we can merge it under 0.2.1 instead of 0.3.0. The clamping behavior can be changed retroactively when a concrete decision has been made in #41. What do you think about that?

[sidred]

Sounds good. I revert the clamping for Rgb.

Why only clamp rgb?
The way I see it is that Xyz, Yxy, Luv and Lab conversions do not need clamping.
The Rgb , Hsl and Hsv do not need clamping for conversion between them and to the Cie colors.
Only cie colors to Rgb/Hsl/Hsv need clamping and all of these go through rgb.
Luma -> Rgb also will not need clamping.

[sidred]

I think there was an error in the code as well. into_rgb clamped, but from_xyz for Rgb did not clamp. Need to think a bit more about it.

[Ogeon]

You can still end up with strange values if you are working with strange values from the start, but that's for a later discussion. I'll look through it in a moment.

[Ogeon]

///Color Conver

This looks like a mistake. I would also suggest swapping this line with the line below, to keep that as one paragraph, while removing this sentence from the trait list. I don't know if you have checked how things looks after running cargo doc...

[Ogeon]

Great, that was all I could find 😄 The only thing, apart from those, that's bothering me a bit is that I can't decide which one of &self and self is the most appropriate. &self is obviously nice because it doesn't take ownership of the input, but it's limited because you can't deconstruct more complex types. self gives more freedom to the conversion implementation, but it takes ownership. All the standard operations and the From and Into traits takes ownership, but it's always possible to implement them for &T. Maybe we should change this to take self, after all. I'm leaning towards doing the same with the other trait, as well. What do you think?

[sidred]

Regarding the ColorConvert trait, the code currently seems duplicated. For example into_yxy and from_yxy for Luma is implemented directly and I have to make sure that from_luma and into_luma should also be changed to use these conversions. Same for hsl <-> hsv conversion.

I was also thinking about FromColor and IntoColor as separate traits, I think I got tripped up while trying to implement the IntoColor automatically for the FromColor. Separating the traits might be better though.

Another reason for From trait not to take &self is because this is a no-op

impl<T> From<T> for T {
    fn from(t: T) -> T { t }
}

Doing this might be expensive

impl<T> From<T> for T {
    fn from(t: &T) -> T { t.to_owned() }
}

But we have no such restriction. The color type is copy. So this should be pretty fast

    fn from_yxy(yxy: &Yxy<T>) -> Self {
        *yxy
    }

I think I now prefer &self. No need to take ownership when not required.

I don't understand this part "but it's limited because you can't deconstruct more complex types"

[Ogeon]

I don't understand this part "but it's limited because you can't deconstruct more complex types"

I was thinking about types where Copy isn't implemented. They could contain a Box or whatever, and &self would require them to be cloned behind the scenes. It doesn't matter for the built-in types, but it may become more complicated for third party types.

Separating the traits might be better though.

That would be a good idea, I think. Some types may be possible to convert one way, but not the other. It could be destructive, semantically wrong, or whatever else.

[sidred]

Let me take another crack at separating the from and into color traits.

[Ogeon]

Go for it 👍 Just ask if you need an extra pair of eyes.

[sidred]

Given FromColor and IntoColor

pub trait FromColor<T>: Sized
    where T: Float,
{...}


pub trait IntoColor<T>: Sized
    where T: Float,
{...}

How would you auto impl IntoColor for From

impl<T:Float, U, V> IntoColor<U> for V where U: FromColor<V> {

[Ogeon]

It would rather be something like

impl<T:Float, C: FromColor<T>> IntoColor<T> for C {

but that's not right either, because something is missing. From<T> and Into<T> works because T is the type we are converting to and from. FromColor and IntoColor isn't generic over the "main type" but only its components, so I don't think it's possible to derive one from the other.

[sidred]

I've used a macro to implement IntoColor trait. Not sure how sure how optimal this is. For example self conversion for into does this

  impl< T: Float > IntoColor<T> for Xyz<T> {

            fn into_xyz(&self) -> Xyz<T> {
                Xyz::from_xyz(self)
   }

instead of

  impl< T: Float > IntoColor<T> for Xyz<T> {

            fn into_xyz(&self) -> Xyz<T> {
               *self
   }

May be the compiler will optimize this away.

Is this any better than the ColorConvert trait?

[Ogeon]

It's much better because it's divided into two parts and I think the compiler will inline those calls. It would have been possible to implement them in the same way as before, though, but just divided into two impls.

I think we can just as well take everything by value, by the way. The implementor can still choose to limit the implementation to &SomeColor, so it won't ruin it for anyone. It will also make the trait names consistent with the Rust conventions, where "into" implies that it takes ownership of something.

[Ogeon]

...It will also leave any copying to the user, instead of always forcing it if it's necessary.

[Ogeon]

Would you mind separating the first sentence into its own paragraph? It will look nicer in the docs.

[sidred]

Not sure if anything else needs to be done here. I've made the recommended changes.

[Ogeon]

No, not unless you want to add anything. Possibly those tests, but they can be added separately, as long as it's done before the release. The [WIP] tag is still there, so I thought you had something else in store, and then I got distracted by other things I had to do... The description is outdated, though.

[sidred]

Let's close this and I'll add the tests separately. The description is correct.

[Ogeon]

Alright :) Just edit the title and description, first.

[sidred]

I didn't realize that you could edit the title :). Description is ok

[Ogeon]

It mentioned a "FromConvert" trait, but I changed that part just now.

@homu r+

[homu]

📌 Commit 50997dc has been approved by Ogeon

[homu]

⚡ Test exempted - status