kornelski / rust-rgb

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

Creation of a `PixelExt` trait for all non-fundamental trait methods. #69

Closed ripytide closed 3 months ago

ripytide commented 3 months ago

Following on from #66 I would like to suggest that we put all methods into two traits, Pixel and PixelExt.

The idea being that Pixel would contain the six fundamental operations:

pub trait Pixel<S, const N: usize> {
    fn as_subpixels(self) -> [S; N];
    fn as_subpixels_ref(&self) -> &[S; N];
    fn as_subpixels_mut(&mut self) -> &mut [S; N];

    fn from_subpixels(subpixels: [S; N]) -> Self;
    fn from_subpixels_ref(subpixels: &[S; N]) -> &Self;
    fn from_subpixels_mut(subpixels: &mut [S; N]) -> &mut Self;
}

And then the PixelExt trait would contain everything else:

pub trait PixelExt<S, const N: usize> {
    fn map<F, T, Q>(&self, f: F) -> Q
      where
        F: FnMut(S) -> T,
        Q: Pixel<T>;
    ...
}

Finally, a default implementation would be provided:

impl<P, S> PixelExt<S> for P where P: Pixel<S> {
    ...
}

Alternatively, all the PixelExt methods could be implemented as default implementations on the Pixel trait, however, the disadvantage (in my opinion) with this is that is would allow the implementer to override that default implementation and cause unexpected non-trivial behavior, hence making the Pixel trait untrustworthy for newtypes.

kornelski commented 3 months ago

Currently this crate has several traits, and I think they're unnecessarily detailed, and it's a hassle to import them for basically one method. So grouping multiple things under a Pixel trait would be great.

It may also be a good idea to move many inherent methods from struct RGB to a trait, to make the crate more future-proof. Inherent methods can't be changed without breaking compatibility, but it's always possible to deprecate an old trait, and add a new one.

Addition of PixelExt seems unnecessary? Unless there's some technical reason like object safety or orphan rules, why not just put everything on Pixel?

Another angle to consider, would be to make it easy to write generic functions that operate on any color space:

fn blur(image: Vec<impl Pixel>)

For this, you may want to have trait PixelAlpha: Pixel, because alpha is not just yet another channel, but needs special handing. For example you can't blur RGBA images by blending all four channels the same way — RGB needs to be multiplied and divided by A. Same with Porter-Duff operations — A is special. And with addition of RGBW, it's not always just a fourth channel.

ripytide commented 3 months ago

Another angle to consider, would be to make it easy to write generic functions that operate on any color space: fn blur(image: Vec) For this, you may want to have trait PixelAlpha: Pixel, because alpha is not just yet another channel, but needs special handing. For example you can't blur RGBA images by blending all four channels the same way — RGB needs to be multiplied and divided by A. Same with Porter-Duff operations — A is special. And with addition of RGBW, it's not always just a fourth channel.

I'd consider that out of scope of this crate, all of that can be achieved by another crate or by the end-user using extension traits.

Addition of PixelExt seems unnecessary? Unless there's some technical reason like object safety or orphan rules, why not just put everything on Pixel?

So the only reason I suggested doing it this way is that it would remove the possibility of someone accidentally or maliciously changing the implementation of one of the helper methods. These helper methods are meant to be simple time-savers and not encode specialized color-space logic, hence using a separate trait with a blanket implementation prevents anyone from changing the implementation of these methods on any Pixel type.

It's not a huge benefit as I would hope no-one would try and change the default implementations if they were given on Pixel but it leaves the door open which we could avoid.

ripytide commented 3 months ago

Closing since with the new HomogeneousPixel and HeterogeneousPixel traits this isn't possible which makes the choice easier.