Convert all colors to be generic over floats, f32 and f64 #18

sidred · 2016-01-22T05:20:06Z

Convert all colors to be generic over floats

Change all colors (Color, Rgb, Hsl, Hsv, Lab, Xyz) and traits (Mix and Saturation etc) to be generic over floats and closes #13.

This is a breaking change. Type f32 or f64 must be explicitly annotated.

// OLD
let new_color: Rgb = lch_color.shift_hue(180.0.into()).into();

// NEW
let new_color: Rgb<f32> = lch_color.shift_hue(180.0.into()).into();

Since Rust defaults to f64, most of the color types will default to f64. For example Rgb::linear_rgb(1.0, 0.0, 0.0) will be inferred as Rgb. To use f32, constants must be explicitly set like rgb::linear_rgb(1.0_f32, 0.0, 0.0) or the type explicitly annotated.

T::from( constant ).unwrap() etc is used in a number of places to convert constants to num::Float trait ( via the num::Numcast trait).

For Hues, the Into trait is not generic over floats due to unwrap() and possibility of runtime panic and is separately implemented for f32 and f64 for Hue<32> and Hue<64>. Also the Partial Eq trait could not be implemented for Hues as float and was removed.

The RgbPixel could not be made generic for [T;3] and (T,T,T) and is separately implemented for f32, f64 and u8.

Using associated typed for Mix and Saturation is not ergonomic and the trait was made generic over float.

sidred · 2016-01-22T05:35:46Z

Formatting changes have been removed. There might be an odd one here and there :)
All commented code blocks have been removed.
Associated types for Mix and Saturation
Doesn't seem to be ergonomic.
```
min_color.mix(max_color, factor)
```
errors with
```
expected `<C as Mix>::T`,
found `T`
```
Floats unwrap():
Piston Developers float crate has a cast for f32 <-> f64, with no unwraps required. But that trait is not compatible with the num Float trait.
The other issue is with the f32 -> u8 conversion for RgbPixel. We are using unwrap there without checking if x*255 > 255.
Need to look into a macro for the num casts. So instead of T::from(255.0).unwrap(), we can use flt!(255.0). Should make the code easier to read.

Ogeon · 2016-01-22T15:22:57Z

examples/readme_examples.rs


-        display_colors("examples/readme_color_spaces.png", &[Rgb::srgb(0.8, 0.2, 0.1).to_srgb(), new_color.to_srgb()]);
+        display_colors("examples/readme_color_spaces.png",
+                       &[Rgb::srgb(0.8, 0.2, 0.1).to_srgb(), new_color.to_srgb()]);


This and following calls to the same function are still reformatted.

Ogeon · 2016-01-22T15:54:36Z

One of the failing tests can be fixed with approx. The other may need a redesign. Comparing colors is a pain.

sidred · 2016-01-22T16:23:01Z

The reason I didn't change those was because they are documentation tests. Didn't want to have anything too complicated in the docs.

sidred · 2016-01-22T16:28:08Z

Cleaned up all the formatting issues. Also made into for hues separate for f32 and f64

Ogeon · 2016-01-22T16:34:15Z

Didn't want to have anything too complicated in the docs.

Good point. Those examples could definitely be improved, but let's try to work around it for now. It looks like the GetHue example defaulted to f64 for some reason, and it may pass if it can be changed to use f32. The other example is a bit harder, but it could maybe be tweaked to use numbers that doesn't cause rounding errors.

Ogeon · 2016-01-22T16:36:24Z

src/rgb.rs

+            clamp(to_srgb(self.red), T::zero(), T::one()),
+            clamp(to_srgb(self.green), T::zero(), T::one()),
+            clamp(to_srgb(self.blue), T::zero(), T::one()),
+            clamp(self.alpha, T::zero(), T::one()))


The closing parenthesis is still on the wrong line...

sidred · 2016-01-22T16:41:30Z

If I remove the GetHue test for green, blue is failing with

(left: `Some(RgbHue(-119.99999996517673))`, right: `Some(RgbHue(240))`)', <anon>:11
', ../src/librustdoc/test.rs:282

sidred · 2016-01-22T16:44:56Z

to_gamma test passes with the change to f32

Ogeon · 2016-01-22T16:56:06Z

If I remove the GetHue test for green, blue is failing with

Yet another precision error...

to_gamma test passes with the change to f32

Great! 😄

sidred · 2016-01-22T17:00:12Z

Blue is not only a precision error, its a negative sign ie -180-180 range. Its comparing -120 to 240. Are we showing hue output in -180-180 or [0-360] for now

Ogeon · 2016-01-22T17:02:23Z

Debug prints are shown as the are stored, since that may help debugging. The sign isn't really the problem here, though, since 240 - 360 = -120 so they should be regarded as equal.

sidred · 2016-01-22T17:09:35Z

I got this example to pass

use palette::{Rgb, GetHue};

fn float_compare(val1: f32, val2: f32) {
    assert!( val1 - val2 <= std::f32::EPSILON)
}


let red = Rgb::linear_rgb(1.0, 0.0, 0.0);
let green = Rgb::linear_rgb(0.0, 1.0, 0.0);
let blue = Rgb::linear_rgb(0.0, 0.0, 0.9);
let gray = Rgb::linear_rgb(0.5, 0.5, 0.5);

float_compare(red.get_hue().unwrap().into(), 0.0);
float_compare(green.get_hue().unwrap().into(), 120.0);
float_compare(blue.get_hue().unwrap().into(), 240.0);
assert_eq!(gray.get_hue(), None);

Ogeon · 2016-01-22T17:15:52Z

That defeats parts of the purpose and we could just as well use approx... I don't like floats anymore. Does the original test really fail if you force it to use f32?

sidred · 2016-01-22T17:19:27Z

Good catch. It passes.

///use palette::{Rgb, GetHue};
///
///let red = Rgb::<f32>::linear_rgb(1.0, 0.0, 0.0);
///let green = Rgb::<f32>::linear_rgb(0.0, 1.0, 0.0);
///let blue = Rgb::<f32>::linear_rgb(0.0, 0.0, 1.0);
///let gray = Rgb::<f32>::linear_rgb(0.5, 0.5, 0.5);
///
///assert_eq!(red.get_hue(), Some(0.0.into()));
///assert_eq!(green.get_hue(), Some(120.0.into()));
///assert_eq!(blue.get_hue(), Some(240.0.into()));
///assert_eq!(gray.get_hue(), None);

Ogeon · 2016-01-22T17:20:49Z

Cool! What about doing something like this instead: Rgb::linear_rgb(1.0f32, 0.0, 0.0); and let the compiler infer as much as possible?

Ogeon · 2016-01-22T17:23:54Z

I filed issue #21 as an effect of this madness.

sidred · 2016-01-22T17:30:36Z

Readme change is done. All the formatting changes listed above are done. Also changed the example to use f32 impl.

Ogeon · 2016-01-22T17:30:39Z

I saw that f64 implements From<f32> now. That could possibly be leveraged to get rid of some unwrap()s, but that can go into an other PR. Let's get this one going first.

Ogeon · 2016-01-22T17:39:11Z

src/hues.rs

+                normalize_angle(self.0) * T::from(PI).unwrap() / T::from(180.0).unwrap()
+            }
+
+            ///Returns the hue value as a f32 or f64


This description should be more general.

Ogeon · 2016-01-22T17:44:32Z

I think this is more or less done. It was just that description I commented on now, but the rest is good enough. I'm going to pretend that I didn't see those last few formatting changes, because I'm tired of hunting them down and the show must go on. They are quite small, anyway.

Now, on to the final act. I think it may be a good idea to squash everything into one commit again. I would also appreciate it if you could summarize the changes in the top comment (somewhat like in the first PR) and add a note that this is a breaking change. Then I think it's ready to be merged.

Change all colors (Color, Rgb, Hsl, Hsv, Lab, Xyz) and traits (Mix and Saturation etc) to be generic over floats. This is a breaking change. Since Rust defaults to f64, most of the color types will default to f64. For example Rgb::linear_rgb(1.0, 0.0, 0.0) will be inferred as Rgb<f64>. To use f32, constants must be explicitly set like Rgb::linear_rgb(1.0_f32, 0.0, 0.0) or the type explicitly annotated. T::from( constant ).unwrap() etc was used in a number of places to convert constants to num::Float trait ( via the num::Numcast trait). For Hues, the Into<T> trait was not generic over floats due to unwrap() and possibility of runtime panic and was separately implemented for f32 and f64 for Hue<32> and Hue<64>. Also the Partial Eq trait could not be implemented for Hues as float and was removed. The RgbPixel could not be made generic for [T;3] and (T,T,T) and was separately implemented for f32, f64 and u8. Using associated typed for Mix and Saturation was not ergonomic and the trait was made generic over float. s

Ogeon · 2016-01-22T20:45:32Z

I don't know if you understood it correctly. That summary you wrote in the commit message should go into the PR description (a.k.a. your first comment in this thread). See CONTRIBUTING.md.

sidred · 2016-01-23T03:58:51Z

Yeah, forgot that this was not a new PR. Thanks for being so patient.

Ogeon · 2016-01-23T11:48:41Z

Don't worry about it. You forgot to mention that it closes #13, but I can squeeze it in there, myself.

Ogeon · 2016-01-23T12:39:21Z

I have looked through it again to see if I missed anything important and it looks alright. Thank you for all of this and for putting up with me being somewhat pedantic.

@homu r+

homu · 2016-01-23T12:39:23Z

📌 Commit 5ae93ac has been approved by Ogeon

homu · 2016-01-23T12:39:32Z

⚡ Test exempted - status

Convert all colors to be generic over floats, f32 and f64 **Convert all colors to be generic over floats** Change all colors (Color, Rgb, Hsl, Hsv, Lab, Xyz) and traits (Mix and Saturation etc) to be generic over floats and closes #13. **_This is a breaking change. Type f32 or f64 must be explicitly annotated._** ``` // OLD let new_color: Rgb = lch_color.shift_hue(180.0.into()).into(); // NEW let new_color: Rgb<f32> = lch_color.shift_hue(180.0.into()).into(); ``` Since Rust defaults to f64, most of the color types will default to f64. For example Rgb::linear_rgb(1.0, 0.0, 0.0) will be inferred as Rgb\<f64\>. To use f32, constants must be explicitly set like rgb::linear_rgb(1.0_f32, 0.0, 0.0) or the type explicitly annotated. T::from( constant ).unwrap() etc is used in a number of places to convert constants to num::Float trait ( via the num::Numcast trait). For Hues, the Into<T> trait is not generic over floats due to unwrap() and possibility of runtime panic and is separately implemented for f32 and f64 for Hue\<32\> and Hue\<64\>. Also the Partial Eq trait could not be implemented for Hues as float and was removed. The RgbPixel could not be made generic for [T;3] and (T,T,T) and is separately implemented for f32, f64 and u8. Using associated typed for Mix and Saturation is not ergonomic and the trait was made generic over float.

Separate sRGB and gamma encoded RGB from the Rgb type This closes #7 by adding separate types for sRGB and gamma encoded RGB. `Rgb` becomes a purely linear variant of sRGB and the types for encoded sRGB and gamma RGB can be found in a newly added `pixel` module. The `RgbPixel` trait has also been moved to the `pixel` module. The `linear_*` prefix has also been dropped from the `Rgb` constructors and some bugs from #18 were fixed as a bonus. This is a breaking change, since it moves, removes and renames a lot of functions, and the `RgbPixel` trait is no longer available in the crate root.

sidred mentioned this pull request Jan 22, 2016

Make all colors generic over f32 and f64 #17

Closed

Ogeon reviewed Jan 22, 2016
View reviewed changes

sidred force-pushed the generic_float branch from 7c501b4 to 5ae93ac Compare January 22, 2016 19:05

homu merged commit 5ae93ac into Ogeon:master Jan 23, 2016

Ogeon mentioned this pull request Jan 24, 2016

Separate sRGB and gamma encoded RGB from the Rgb type #24

Merged

sidred deleted the generic_float branch January 27, 2016 04:27

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Convert all colors to be generic over floats, f32 and f64 #18

Convert all colors to be generic over floats, f32 and f64 #18

sidred commented Jan 22, 2016

sidred commented Jan 22, 2016

Ogeon Jan 22, 2016

Ogeon commented Jan 22, 2016

sidred commented Jan 22, 2016

sidred commented Jan 22, 2016

Ogeon commented Jan 22, 2016

Ogeon Jan 22, 2016

sidred commented Jan 22, 2016

sidred commented Jan 22, 2016

Ogeon commented Jan 22, 2016

sidred commented Jan 22, 2016

Ogeon commented Jan 22, 2016

sidred commented Jan 22, 2016

Ogeon commented Jan 22, 2016

sidred commented Jan 22, 2016

Ogeon commented Jan 22, 2016

Ogeon commented Jan 22, 2016

sidred commented Jan 22, 2016

Ogeon commented Jan 22, 2016

Ogeon Jan 22, 2016

Ogeon commented Jan 22, 2016

Ogeon commented Jan 22, 2016

sidred commented Jan 23, 2016

Ogeon commented Jan 23, 2016

Ogeon commented Jan 23, 2016

homu commented Jan 23, 2016

homu commented Jan 23, 2016

Convert all colors to be generic over floats, f32 and f64 #18

Convert all colors to be generic over floats, f32 and f64 #18

Conversation

sidred commented Jan 22, 2016

sidred commented Jan 22, 2016

Ogeon Jan 22, 2016

Choose a reason for hiding this comment

Ogeon commented Jan 22, 2016

sidred commented Jan 22, 2016

sidred commented Jan 22, 2016

Ogeon commented Jan 22, 2016

Ogeon Jan 22, 2016

Choose a reason for hiding this comment

sidred commented Jan 22, 2016

sidred commented Jan 22, 2016

Ogeon commented Jan 22, 2016

sidred commented Jan 22, 2016

Ogeon commented Jan 22, 2016

sidred commented Jan 22, 2016

Ogeon commented Jan 22, 2016

sidred commented Jan 22, 2016

Ogeon commented Jan 22, 2016

Ogeon commented Jan 22, 2016

sidred commented Jan 22, 2016

Ogeon commented Jan 22, 2016

Ogeon Jan 22, 2016

Choose a reason for hiding this comment

Ogeon commented Jan 22, 2016

Ogeon commented Jan 22, 2016

sidred commented Jan 23, 2016

Ogeon commented Jan 23, 2016

Ogeon commented Jan 23, 2016

homu commented Jan 23, 2016

homu commented Jan 23, 2016