search

kornelski / rust-rgb

struct RGB for sharing pixels between crates
https://lib.rs/rgb
MIT License
97 stars 19 forks source link

De-Genericify From conversions #104

Closed ripytide closed 1 month ago

ripytide commented 2 months ago

The issue with the current conversions is that they are not consistent due to us not being able to provide From<Rgb<T>> for Rgb<U> where U: From<T> due to a trait collision with the blanket impl From<T> for T impl provided in the standard library.

Therefore I think it is better to only provide same component-type conversion between all pixel types for consistency and to prevent user confusion such as: Rgb<u8> -> Bgr<u32> is allowed but Rgb<u8> -> Rgb<u32> is not?!? :smile:

If more flexible conversion are required that can convert both pixel types and all component types consistently we could add our own trait for it, such as FromPixel. I don't think that's too important to me at the moment but adding it would be backwards compatible so I think we can leave it for later down the road. Especially if we combine that conversion with color spaces which may make such a non-colorspace-aware conversion redundant anyway.

ripytide commented 2 months ago

Closing temporarily in favor of #106 since I'd rather rebase this change onto that PR than the other way round as that ones huge lol.

kornelski commented 2 months ago

Ehh, I've forgotten why the code has this ad-hoc conversion list, and hoped it could be changed to a generic RGB<T> -> RGB<U> ;(

I'd keep the rest of conversions generic where possible. That gives a lot of flexibility for very little code. Limitations of the same-pixel-type conversions shouldn't hold others back.

For the same pixel case there's px.map(From::from) fallback when someone runs into this limitation.

ripytide commented 2 months ago

Although more flexible I think it's not the right choice since it doesn't fit with standard library types like Option:

fn main() {
    let x: Option<u8> = Some(0);
    let y: Option<u32> = x.into();
}

This also doesn't compile for the same reason, I think we should stick to the standard libraries approach to From implementations here, for the principle of least surprise.

If rust ever gets specialization we might be able to pull this off though.

And as you say if people want to do component and pixel type conversion I think it's also more readable if they do those operations in two steps:

pixel.map(U::from).into()
kornelski commented 1 month ago

Okay, the Option analogy is convincing.