Add Extended Conversion Trait #106

Veykril · 2018-06-03T15:40:36Z

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.

Ogeon · 2018-06-03T16:22:57Z

Thanks for putting in the effort! It's a clever implementation and should work really well with today's behavior. I think TryFrom (which apparently got un-stabilized) may make sense if it returns an OutOfBounds<T>(T) value to signal that the value is invalid, but still make it accessible. What I think people in #41 wanted was a default that always returns valid values. I can imagine the current behavior being quite surprising for someone who doesn't know about gamuts and it makes sense to think it's somehow broken.

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?

Ogeon · 2018-06-03T16:28:36Z

palette/src/convert.rs

+{
+    ///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]))


This could be as_raw and convert_with could take &self. Could also use into_raw, but that becomes more complicated.

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?

I have no better name suggestion at the moment.

Veykril · 2018-06-03T16:52:31Z

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 out of bounds colors should probably be done explicitly. Panicking is out of the question for the default behavior I'd say, especially since the standard library also says that the From trait should never fail. So I guess I am leaning towards default clamping as well.

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.

Ogeon · 2018-06-03T17:11:59Z

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. FromColor and IntoColor will need a new design, since they are what people will reach for when From and Into isn't enough. It may be best to rename them and add clamping conversion traits with the current names. An alternative is to add more methods. The derives will also need to change a bit. I'm trying to come up with a way to make it ergonomic.

Ogeon · 2018-06-06T13:57:36Z

Since we can't rely on TryFrom being a thing in the near future, I'm thinking that we should go with these traits, but maybe with a few modifications. My suggestion is to make it a first step towards making clamping the default, so let's swap the semantics of convert and convert_clamped to be convert (clamped) and convert_unclamped (better name is welcome). You will probably have to go without a default implementation for now, to avoid requiring Limited.

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.

Veykril · 2018-06-06T23:26:56Z

Okay, I'll get to this in the next few days. So should I include the try_convert function and the as_raw function as well? Also the convert function would require Limited on the implementing type since it clamps according to the trait implementation. Unless you meant something different with that sentence.
Edit: Nevermind you meant as in implementing the convert trait individually for every color right?

Ogeon · 2018-06-07T17:06:33Z

Nevermind you meant as in implementing the convert trait individually for every color right?

Yes, exactly, otherwise we can't convert to every color. Only the ones that makes sense to clamp. And yes, try_convert should stay.

I have been thinking a bit about convert_with (I guess that's what you mean with as_raw) and I'm starting to wonder if has much of a use case anymore, or if that issue is a bit obsolete now when it's so easy to convert to [T] or [T; N]. You just have to use AsRef to convert it before calling the convert function, so convert_to_something(my_color.as_ref()) instead of my_color.convert_with(convert_to_something).

Veykril · 2018-06-07T20:09:57Z

I just tried to make Convert not require Limited anywhere, but if I do not make Limited a bound on the output type then I wont be able to do any clamping ofc. Unless I am missing something crucial in my head right now. Just noticed what I was forgetting/doing wrong. Also I just noticed but doesn't every color implement Limited? So wouldn't that trait bound be no problem?

And I agree the raw conversion method is probably not really needed.

Ogeon · 2018-06-07T20:27:48Z

Oh, you are right. Then I guess it would either have to be a ConvertFrom/ConvertInto pair, where ConvertFrom controls the implementation and ConvertInto just refers to it, or Limited have to be assumed. I was thinking on a pretty abstract level when writing that it can't be assumed, because then it doesn't have to be a hard requirement, but maybe it's a good requirement. It makes sense semantically, but a theoretically unlimited system doesn't have to be clamped. Maybe it shouldn't be called Limited at all. But... I guess most, if not all, color spaces have some kind of range or limit. Might as well require it until it becomes a problem.

Veykril · 2018-06-07T23:20:15Z

I didn't notice that you replied so quickly unfortunately but I actually did manage to remove the Limited trait bounds by implementing the trait in a way that the output color is defined explicitly. With that we have now access to the Limited functions should the output color have access too.

I also added some examples in the docs and changed the return type of try_convert into a Result with a custom error.

I'm not so sure if I like the take function that I added for Result<T, OutOfBounds<T>> though, not too sure why I thought of this to be honest.

Ogeon

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.

Ogeon · 2018-06-09T10:09:53Z

palette/src/convert.rs

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


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

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.

Ogeon · 2018-06-09T10:11:31Z

palette/src/convert.rs

+    ///```
+    fn convert(self) -> T;
+
+    ///Convert into T, the resulting color might be invalid in it's color space


Ogeon · 2018-06-09T10:48:54Z

palette/src/convert.rs

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


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. 😄

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

Ogeon · 2018-06-09T10:58:21Z

palette/src/convert.rs

+    fn try_convert(self) -> Result<T, OutOfBounds<T>>;
+}
+
+macro_rules! expand_impl_convert {


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).

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?

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.

Ogeon · 2018-06-10T12:00:35Z

palette/src/convert.rs

+
+///A trait for converting one color from another.
+///
+///`convert_unclamped` currently wraps the underlying `Into` implementation.


I guess this was meant to say "From".

Ogeon · 2018-06-10T12:01:37Z

palette/src/convert.rs

+    }
+}
+
+/*


Also remove these. 🙂

Ogeon · 2018-06-10T12:08:51Z

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 ConvertInto wrapping ConvertFrom to align it with From and Into. Just for consistency. After that and the small fixes, I think it's done from my point of view. 👍

Veykril · 2018-06-10T14:07:48Z

Alright, made ConvertInto wrap ConvertFrom in the generic implementation since it wouldnt be possible to do it on the trait level due to ConvertInto not requiring ConvertFrom.

Ogeon · 2018-06-10T14:13:06Z

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.

Veykril · 2018-06-10T14:33:43Z

I'd say it's good to go.

Ogeon · 2018-06-10T14:38:58Z

I think you typoed the issue number. #4 is not that relevant 🤔

Veykril · 2018-06-10T14:48:06Z

That was supposed to be a 41, my bad. 😅

Ogeon · 2018-06-10T15:40:53Z

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>

bors · 2018-06-10T15:57:28Z

Build succeeded

continuous-integration/travis-ci/push

Add conversion trait

f8f364c

Ogeon reviewed Jun 3, 2018

View reviewed changes

removed Limited trait bounds

9eeced6

Add missing error description

277263d

Ogeon requested changes Jun 9, 2018

View reviewed changes

Ogeon mentioned this pull request Jun 9, 2018

Converting from a wider gamut to a smaller one should return an option #41

Closed

Changed convert to ConvertFrom/ConvertInto pair

1a6e53f

Ogeon reviewed Jun 10, 2018

View reviewed changes

palette/src/convert.rs Outdated

}

}

/*

Copy link

Owner

Ogeon Jun 10, 2018

Choose a reason for hiding this comment

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

Also remove these. 🙂

Make ConvertInto wrap ConvertFrom for convert_unclamped

79c848c

Ogeon approved these changes Jun 10, 2018

View reviewed changes

Veykril changed the title ~~[Wip] Add Extended Conversion Trait~~ Add Extended Conversion Trait Jun 10, 2018

bors bot merged commit 79c848c into Ogeon:master Jun 10, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Extended Conversion Trait #106

Add Extended Conversion Trait #106

Veykril commented Jun 3, 2018 •

edited

Ogeon commented Jun 3, 2018

Ogeon Jun 3, 2018 •

edited

Veykril Jun 3, 2018

Ogeon Jun 3, 2018

Veykril commented Jun 3, 2018

Ogeon commented Jun 3, 2018

Ogeon commented Jun 6, 2018

Veykril commented Jun 6, 2018 •

edited

Ogeon commented Jun 7, 2018

Veykril commented Jun 7, 2018 •

edited

Ogeon commented Jun 7, 2018

Veykril commented Jun 7, 2018 •

edited

Ogeon left a comment •

edited

Ogeon Jun 9, 2018

Veykril Jun 9, 2018

Ogeon Jun 9, 2018

Ogeon Jun 9, 2018

Veykril Jun 9, 2018

Ogeon Jun 9, 2018

Veykril Jun 9, 2018

Ogeon Jun 9, 2018

Ogeon Jun 10, 2018

Ogeon Jun 10, 2018

Ogeon commented Jun 10, 2018

Veykril commented Jun 10, 2018

Ogeon commented Jun 10, 2018

Veykril commented Jun 10, 2018

Ogeon commented Jun 10, 2018

Veykril commented Jun 10, 2018

Ogeon commented Jun 10, 2018 •

edited

bors bot commented Jun 10, 2018

+                  }
+              }
+              /*

Add Extended Conversion Trait #106

Add Extended Conversion Trait #106

Conversation

Veykril commented Jun 3, 2018 • edited

Ogeon commented Jun 3, 2018

Ogeon Jun 3, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Veykril commented Jun 3, 2018

Ogeon commented Jun 3, 2018

Ogeon commented Jun 6, 2018

Veykril commented Jun 6, 2018 • edited

Ogeon commented Jun 7, 2018

Veykril commented Jun 7, 2018 • edited

Ogeon commented Jun 7, 2018

Veykril commented Jun 7, 2018 • edited

Ogeon left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ogeon commented Jun 10, 2018

Veykril commented Jun 10, 2018

Ogeon commented Jun 10, 2018

Veykril commented Jun 10, 2018

Ogeon commented Jun 10, 2018

Veykril commented Jun 10, 2018

Ogeon commented Jun 10, 2018 • edited

bors bot commented Jun 10, 2018

Build succeeded

Veykril commented Jun 3, 2018 •

edited

Ogeon Jun 3, 2018 •

edited

Veykril commented Jun 6, 2018 •

edited

Veykril commented Jun 7, 2018 •

edited

Veykril commented Jun 7, 2018 •

edited

Ogeon left a comment •

edited

Ogeon commented Jun 10, 2018 •

edited