-
Notifications
You must be signed in to change notification settings - Fork 60
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
Conversation
Thanks for putting in the effort! It's a clever implementation and should work really well with today's behavior. I think I think our options for default behavior are the current one, clamping or panicking. A of these have some downsides and I don't know yet which one of them is more worth it. I need to think bit more before making up my mind, but I'm leaning away from the current behavior because its usecase is admittedly quite slim. I don't know how many conversions will produce sane values while overflowing. What do you think? |
palette/src/convert.rs
Outdated
{ | ||
///Convert `self` to `T` by applying a function over its components | ||
fn convert_with<F, Out>(self, f: F) -> Out where F: FnOnce(&[T]) -> Out, Out: Sized { | ||
f(Pixel::into_raw_slice(&[self])) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This could be as_raw
and convert_with
could take &self
. Could also use into_raw
, but that becomes more complicated.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah yes, I had problems with coming up for a name for this function actually. as_raw
should work here I suppose, and I didn't take self by reference because I thought of it being a conversion into something else so consuming self would've fit there. It's probably better to take it be ref nevertheless. Any Ideas on the trait name for that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have no better name suggestion at the moment.
Yes, making from/the default conversion clamp to the output's valid range is probably the better approach. I believe the more common use case is only working with valid colors instead. Working with That leaves the question if we even need a extra unclamped conversion, since as you mentioned we could also use the Err return value from the TryFrom implementation. One downside I see with this though is that it will be somewhat cumbersome to extract the values when you do not care about valid colors, since unwrap wont just work like that for you then. So we would probably need to implement a function + trait for Result<T, OurError> that will unwrap to T in oth Ok and Err cases. |
I agree about the panicking. It would just result in unexpected crashes instead of unexpected values. Changing the default behavior to clamping will require a bit more work on top of this trait. |
Since we can't rely on I would also appreciate some examples in the documentation. That's always nice. The rest will need some more time and design to get the relationships between the traits right. I don't want to put all of that into this PR, to avoid turning it into some kind of monster task. It's better to split it and get this in while it's fresh. |
Okay, I'll get to this in the next few days. So should I include the |
Yes, exactly, otherwise we can't convert to every color. Only the ones that makes sense to clamp. And yes, I have been thinking a bit about |
And I agree the raw conversion method is probably not really needed. |
Oh, you are right. Then I guess it would either have to be a |
I didn't notice that you replied so quickly unfortunately but I actually did manage to remove the I also added some examples in the docs and changed the return type of I'm not so sure if I like the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is basically the ConvertInto
trait in the ConvertFrom
/ConvertInto
pair. We'll see if I end up going all the way with that later or not, but I think this is a fine concept for now. Just a few things I think can be improved.
|
||
impl<T> Display for OutOfBounds<T> { | ||
fn fmt(&self, fmt: &mut Formatter) -> fmt::Result { | ||
write!(fmt, "Color conversion is out of bounds") |
There was a problem hiding this comment.
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)
.
There was a problem hiding this comment.
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.
palette/src/convert.rs
Outdated
///``` | ||
fn convert(self) -> T; | ||
|
||
///Convert into T, the resulting color might be invalid in it's color space |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
*its 🙂
palette/src/convert.rs
Outdated
} | ||
|
||
///An extension for `Result` that offers a method to take out the color of `try_convert` result. | ||
pub trait ResultExt<T> { |
There was a problem hiding this comment.
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. 😄
There was a problem hiding this comment.
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.
palette/src/convert.rs
Outdated
fn try_convert(self) -> Result<T, OutOfBounds<T>>; | ||
} | ||
|
||
macro_rules! expand_impl_convert { |
There was a problem hiding this comment.
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 impl
s. 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).
There was a problem hiding this comment.
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 impl
ementing 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?
There was a problem hiding this comment.
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.
palette/src/convert.rs
Outdated
|
||
///A trait for converting one color from another. | ||
/// | ||
///`convert_unclamped` currently wraps the underlying `Into` implementation. |
There was a problem hiding this comment.
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".
palette/src/convert.rs
Outdated
} | ||
} | ||
|
||
/* |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also remove these. 🙂
Very nice! I think this will be the way to go. I'm just thinking that it may be wise to change the relationship to |
Alright, made |
That's exactly what I expected. Looks good! The PR title and description are going to be included in the merge commit message, so feel free to edit it a bit (at least remove "[wip]") and let me know when/if you feel like it's done. |
I'd say it's good to go. |
I think you typoed the issue number. #4 is not that relevant 🤔 |
That was supposed to be a 41, my bad. 😅 |
Alright! bors: r+ |
106: Add Extended Conversion Trait r=Ogeon a=Veykril This commit tries to implement a conversion trait similar to the one mentioned in #41 mimicking the `From`/`Into` traits of the `std` library. `convert_*` clamps the resulting color to its color space limitations. `convert_unclamped_*` simply converts the color. `try_convert_*` converts the color and returns it in a `Result` depending on it being valid or not. Co-authored-by: Lukas Wirth <lukastw97@gmail.com>
Build succeeded |
This commit tries to implement a conversion trait similar to the one mentioned in #41 mimicking the
From
/Into
traits of thestd
library.convert_*
clamps the resulting color to its color space limitations.convert_unclamped_*
simply converts the color.try_convert_*
converts the color and returns it in aResult
depending on it being valid or not.