Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add Extended Conversion Trait #106

Merged
merged 5 commits into from Jun 10, 2018
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
176 changes: 173 additions & 3 deletions palette/src/convert.rs
@@ -1,9 +1,12 @@
use num_traits::Float;

use {Component, Hsl, Hsv, Hwb, Lab, Lch, Xyz, Yxy};
use std::error;
use std::fmt::{self, Debug, Display, Formatter};
use {Component, Limited, Hsl, Hsv, Hwb, Lab, Lch, Xyz, Yxy};
use white_point::{D65, WhitePoint};
use rgb::{Rgb, RgbSpace};
use luma::Luma;
use rgb::{Rgb, RgbSpace, RgbStandard};
use luma::{Luma, LumaStandard};
use Alpha;
use encoding::Linear;

/// FromColor provides conversion from the colors.
Expand Down Expand Up @@ -499,6 +502,173 @@ where
}
}

///The error type for a color conversion that converted a color into a color with invalid values.
#[derive(Debug)]
pub struct OutOfBounds<T> {
color: T,
}

impl<T> OutOfBounds<T> {
///Create a new error wrapping a color
fn new(color: T) -> Self {
OutOfBounds { color }
}

///Consume this error and return the wrapped color
pub fn color(self) -> T {
self.color
}
}

impl<T: Debug> error::Error for OutOfBounds<T> {
fn description(&self) -> &str {
"Color conversion is out of bounds"
}
}

impl<T> Display for OutOfBounds<T> {
fn fmt(&self, fmt: &mut Formatter) -> fmt::Result {
write!(fmt, "Color conversion is out of bounds")
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You could reuse the description here: self.description().fmt(fmt).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't implement description at all at first because it is soft deprecated as its docs say, hence I didnt use it there either, but I might as well. No need to duplicate the literal there.

}
}

///An extension for `Result` that offers a method to take out the color of `try_convert` result.
pub trait ResultExt<T> {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we have the same gut feeling regarding this. I have a feeling that it will not be used a lot, since it has to be explicitly imported. Besides, it's a bit redundant with convert_unclamped basically doing the same thing. I vote for skipping it and maybe adding it later if there's ever a need. I'm in a phase where I want to remove things and boil this library down as much as I can. 😄

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes thought the same, why use this when you could just use convert_unclamped in the first place.

///Take the color out of this `Result`.
fn take(self) -> T;
}

impl<T> ResultExt<T> for Result<T, OutOfBounds<T>> {
#[inline]
fn take(self) -> T {
match self {
Ok(color) => color,
Err(color) => color.color(),
}
}
}

///A trait for converting from one color to another.
///
///`convert_unclamped` currently wraps the underlying `Into` implementation.
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess this was meant to say "From".

pub trait Convert<T>: Sized + Into<T> {

///Convert into T with values clamped to the color defined bounds
///
///```
///use palette::Convert;
///use palette::Limited;
///use palette::{Srgb, Lch};
///
///
///let rgb: Srgb = Lch::new(50.0, 100.0, -175.0).convert();
///assert!(rgb.is_valid());
///```
fn convert(self) -> T;

///Convert into T, the resulting color might be invalid in it's color space
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

*its 🙂

///
///```
///use palette::Convert;
///use palette::Limited;
///use palette::{Srgb, Lch};
///
///let rgb: Srgb = Lch::new(50.0, 100.0, -175.0).convert_unclamped();
///assert!(!rgb.is_valid());
///```
#[inline]
fn convert_unclamped(self) -> T {
self.into()
}

///Convert into T, returning ok if the color is inside of its defined range,
///otherwise an `OutOfBounds` error is returned which contains the unclamped color.
///
///```
///use palette::Convert;
///use palette::{Srgb, Hsl};
///
///let rgb: Srgb = match Hsl::new(150.0, 1.0, 1.1).try_convert() {
/// Ok(color) => color,
/// Err(err) => {
/// println!("Color is out of bounds");
/// err.color()
/// },
///};
///```
fn try_convert(self) -> Result<T, OutOfBounds<T>>;
}

macro_rules! expand_impl_convert {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would honestly like to move away from macros if I can. They have been a hassle and I have tried to replace some of them with derives and just regular impls. Do you think all of this would be possible to replace with:

impl<T: Into<U> + Limited, U> Convert<U> for T {
    // ...
}

It has the same pattern as the Into blanket implementation, so I think it should work. It's not a big deal if it's no perfect yet, and I'm sure it will have to change anyway when the rest of the library catches up and the implementation of Into changes. I just want a start that is somewhat in line with my longer term goals (which I maybe should note down in an issue instead of keeping in my head).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Understandable, Macro implementations make code quite difficult to read. I was trying to get rid of the Limited trait-bound but that just wasnt possible with a generic impl. I can revert back to implementing the trait. While we are talking about the similarities to Into, wouldn't it make more sense to make this two traits which are ConvertFrom and ConvertInto then?

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, probably. That would allow conversion in both directions, which is nice. I think it's fine for a blanket implementation to use the Limited trait, since it doesn't become a hard requirement. It will still be possible to add custom implementations, but they are not fully automatic.

(Alpha<$typ:ident<$gp:ident>>) => {
fn convert(self) -> Alpha<$typ<$gp, C>, C> {
let mut this = self.into();
if !this.is_valid() {
this.clamp_self();
}
this
}

fn try_convert(self) -> Result<Alpha<$typ<$gp, C>, C>, OutOfBounds<Alpha<$typ<$gp, C>, C>>> {
let this = self.into();
if this.is_valid() {
Ok(this)
} else {
Err(OutOfBounds::new(this))
}
}
};
($typ:ident<$gp:ident>) => {
fn convert(self) -> $typ<$gp, C> {
let mut this = self.into();
if !this.is_valid() {
this.clamp_self();
}
this
}

fn try_convert(self) -> Result<$typ<$gp, C>, OutOfBounds<$typ<$gp, C>>> {
let this = self.into();
if this.is_valid() {
Ok(this)
} else {
Err(OutOfBounds::new(this))
}
}
};
}

macro_rules! impl_convert {
(impl<$gp:ident: $gt:ident> $typ:ident<$gt2:ident>) => {
impl<$gp: $gt, C: Component, T> Convert<$typ<$gp, C>> for T where T: Into<$typ<$gp, C>> {
expand_impl_convert!($typ<$gp>);
}

impl<$gp: $gt, C: Component, T> Convert<Alpha<$typ<$gp, C>, C>> for T where T: Into<Alpha<$typ<$gp, C>, C>> {
expand_impl_convert!(Alpha<$typ<$gp>>);
}
};
(float_component impl<$gp:ident: $gt:ident> $typ:ident<$gt2:ident>) => {
impl<$gp: $gt, C: Float + Component, T> Convert<$typ<$gp, C>> for T where T: Into<$typ<$gp, C>> {
expand_impl_convert!($typ<$gp>);
}

impl<$gp: $gt, C: Float + Component, T> Convert<Alpha<$typ<$gp, C>, C>> for T where T: Into<Alpha<$typ<$gp, C>, C>> {
expand_impl_convert!(Alpha<$typ<$gp>>);
}
};
}

impl_convert!(impl<S: RgbStandard> Rgb<S>);
impl_convert!(impl<S: LumaStandard> Luma<S>);
impl_convert!(float_component impl<S: RgbSpace> Hsl<S>);
impl_convert!(float_component impl<S: RgbSpace> Hsv<S>);
impl_convert!(float_component impl<S: RgbSpace> Hwb<S>);
impl_convert!(float_component impl<Wp: WhitePoint> Lab<Wp>);
impl_convert!(float_component impl<Wp: WhitePoint> Lch<Wp>);
impl_convert!(float_component impl<Wp: WhitePoint> Xyz<Wp>);
impl_convert!(float_component impl<Wp: WhitePoint> Yxy<Wp>);

macro_rules! impl_into_color {
($self_ty: ident, $from_fn: ident) => {
impl<Wp, T> IntoColor<Wp, T> for $self_ty<Wp, T>
Expand Down
2 changes: 1 addition & 1 deletion palette/src/lib.rs
Expand Up @@ -182,7 +182,7 @@ pub use rgb::{GammaSrgb, GammaSrgba, LinSrgb, LinSrgba, Srgb, Srgba};
pub use xyz::{Xyz, Xyza};
pub use yxy::{Yxy, Yxya};

pub use convert::{FromColor, IntoColor};
pub use convert::{Convert, ResultExt, OutOfBounds, FromColor, IntoColor};
pub use encoding::pixel::Pixel;
pub use hues::{LabHue, RgbHue};
pub use matrix::Mat3;
Expand Down