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

Allow HSV, HSL and HWB to represent nonlinear RGB #188

Merged
merged 3 commits into from
May 10, 2020
Merged

Conversation

Ogeon
Copy link
Owner

@Ogeon Ogeon commented May 2, 2020

This removes the old restriction that meant HSV, HSL and HWB can only represent linear RGB. They can now be converted to and from RGB (and each other) as long as the RGB standard is the same. This restriction is there to still allow type inference and reduce implementation complexity. So converting Hsl<Srgb> (not linear) to Rgb<Srgb> is still a single step process, while converting to Rgb<Linear<Srgb>> is a two step process. There are still shortcuts in place to only change the RGB standard, meaning it's possible to write Hsl::<Linear<Srgb>>::from_color(Hsl::<Srgb>::new(...)), similar to how it worked before.

This will mean that existing code may have a different output, adding more breaking changes to the pile.

Fixes #160, fixes #187

@Ogeon Ogeon mentioned this pull request May 2, 2020
@github-actions
Copy link

github-actions bot commented May 2, 2020

Benchmark for 3f73fe7

Click to view benchmark
Test PR Benchmark Master Benchmark %
Cie family/lab to lch 2.8±0.21µs 2.7±0.16µs 103%
Cie family/lab to xyz 779.1±79.13ns 788.2±72.82ns 99%
Cie family/lch to lab 2.4±0.18µs 2.3±0.14µs 103%
Cie family/linsrgb to xyz 3.2±0.40µs 3.2±0.24µs 99%
Cie family/xyz to lab 8.5±0.66µs 8.8±0.91µs 97%
Cie family/xyz to yxy 490.1±35.74ns 510.5±49.10ns 96%
Cie family/yxy to xyz 474.6±34.81ns 490.9±40.20ns 97%
Matrix functions/matrix_inverse 12.8±0.98ns 12.3±0.99ns 100%
Matrix functions/multiply_3x3 9.2±0.67ns 8.5±0.76ns 109.00000000000001%
Matrix functions/multiply_rgb_to_xyz 4.2±0.36ns 4.0±0.29ns 104%
Matrix functions/multiply_xyz 4.1±0.32ns 4.0±0.26ns 100%
Matrix functions/multiply_xyz_to_rgb 4.0±0.25ns 4.2±0.31ns 96%
Matrix functions/rgb_to_xyz_matrix 20.2±1.70ns 17.5±1.06ns 114.99999999999999%
Rgb family/hsl to hsv 564.0±34.93ns 517.9±38.98ns 109.00000000000001%
Rgb family/hsl to rgb 2.1±0.20µs 6.2±0.65µs -89.99999999999999%
Rgb family/hsv to hsl 973.5±41.58ns 893.3±73.27ns 109.00000000000001%
Rgb family/hsv to hwb 241.2±21.01ns 231.4±21.51ns 104%
Rgb family/hsv to rgb 2.1±0.17µs 6.0±0.54µs -94%
Rgb family/hwb to hsv 479.6±67.65ns 461.4±34.20ns 104%
Rgb family/linsrgb to rgb 4.1±0.33µs 4.1±0.32µs 101%
Rgb family/linsrgb_f32 to rgb_u8 5.9±0.38µs 5.4±0.39µs 109.00000000000001%
Rgb family/rgb to hsl 565.3±79.65ns 5.4±0.50µs -760%
Rgb family/rgb to hsv 415.1±30.32ns 4.7±0.33µs -933%
Rgb family/rgb to linsrgb 4.2±0.29µs 4.2±0.38µs 99%
Rgb family/rgb_u8 to linsrgb_f32 4.9±0.32µs 4.4±0.31µs 112.00000000000001%
Rgb family/xyz to linsrgb 5.6±0.42µs 5.5±0.41µs 101%

@okaneco
Copy link
Contributor

okaneco commented May 2, 2020

This is a really nice change. 👍

Should a TODO note be added to the Rgb family benchmarks for implementing Hsl/Hsv/Hwb <-> Linear Hsl/Hsv/Hwb, and Linear Hsl/Hsv/Hwb <->LinSrgb? They're not completely necessary right now but it'd be good to let people know they're not covered currently.

@Ogeon
Copy link
Owner Author

Ogeon commented May 2, 2020

I don't know how interesting they are. Linear Hsl/Hsv/Hwb <->LinSrgb is the exact same as Hsl/Hsv/Hwb <-> Srgb, so that's already implicitly covered, and Hsl/Hsv/Hwb <-> Linear Hsl/Hsv/Hwb would just measure Srgb <-> LinSrgb plus the roundtrip that's hopefully optimized away. But I can add Hsl/Hsv/Hwb <-> Linear Hsl/Hsv/Hwb tomorrow if you think it would be interesting to keep an eye on.

@okaneco
Copy link
Contributor

okaneco commented May 2, 2020

I completely agree that they're not that interesting and are probably unnecessary. I meant it more for the sake of thoroughness. It can be hard to keep track of what the internal conversions are if you're looking in from the outside.

The LinSrgb probably isn't necessary for the reasons you stated. It was more of a double check, although we don't really have much to compare to.

@Ogeon
Copy link
Owner Author

Ogeon commented May 2, 2020

True. It should be quick to add anyway.

@Ogeon
Copy link
Owner Author

Ogeon commented May 3, 2020

Wow, how do I even git?

Anyway, I added some benchmarks. Not all of them, for the reasons above.

@github-actions
Copy link

github-actions bot commented May 3, 2020

Benchmark for 513828c

Click to view benchmark
Test PR Benchmark Master Benchmark %
Cie family/lab to lch 3.3±0.11µs 3.3±0.15µs 103%
Cie family/lab to xyz 969.3±48.26ns 901.6±24.70ns 108%
Cie family/lch to lab 2.8±0.11µs 2.8±0.12µs 100%
Cie family/linsrgb to xyz 4.0±0.16µs 3.8±0.13µs 103%
Cie family/xyz to lab 10.4±0.37µs 10.2±0.24µs 102%
Cie family/xyz to yxy 589.8±20.70ns 589.5±29.87ns 100%
Cie family/yxy to xyz 586.5±13.81ns 576.6±21.91ns 102%
Matrix functions/matrix_inverse 14.6±0.58ns 14.6±0.61ns 100%
Matrix functions/multiply_3x3 10.1±0.56ns 9.9±0.43ns 102%
Matrix functions/multiply_rgb_to_xyz 4.9±0.19ns 4.9±0.31ns 100%
Matrix functions/multiply_xyz 5.0±0.22ns 4.9±0.15ns 100%
Matrix functions/multiply_xyz_to_rgb 5.0±0.24ns 4.9±0.23ns 100%
Matrix functions/rgb_to_xyz_matrix 21.1±0.65ns 20.8±0.81ns 102%
Rgb family/hsl to hsv 643.8±36.34ns 619.1±26.41ns 104%
Rgb family/hsl to linear hsl 9.1±0.47µs undefined 100%
Rgb family/hsl to rgb 2.5±0.09µs 7.2±0.25µs -92%
Rgb family/hsv to hsl 1113.2±97.78ns 1068.3±48.41ns 104%
Rgb family/hsv to hwb 286.9±24.60ns 275.0±8.90ns 104%
Rgb family/hsv to linear hsv 8.8±0.32µs undefined 100%
Rgb family/hsv to rgb 2.4±0.09µs 7.4±0.38µs -104%
Rgb family/hwb to hsv 537.2±15.67ns 543.2±22.93ns 99%
Rgb family/hwb to linear hwb 9.6±0.43µs undefined 100%
Rgb family/linear hsl to hsl 9.5±0.40µs undefined 100%
Rgb family/linear hsv to hsv 9.3±0.49µs undefined 100%
Rgb family/linear hwb to hwb 9.9±0.39µs undefined 100%
Rgb family/linsrgb to rgb 4.8±0.20µs 4.8±0.15µs 99%
Rgb family/linsrgb_f32 to rgb_u8 6.6±0.23µs 6.7±0.25µs 98%
Rgb family/rgb to hsl 617.2±18.90ns 6.2±0.18µs -802%
Rgb family/rgb to hsv 455.5±17.67ns 5.6±0.22µs -1029%
Rgb family/rgb to linsrgb 5.0±0.17µs 5.0±0.18µs 100%
Rgb family/rgb_u8 to linsrgb_f32 5.3±0.17µs 5.3±0.16µs 101%
Rgb family/xyz to linsrgb 7.2±0.31µs 6.8±0.17µs 106%

@github-actions
Copy link

github-actions bot commented May 3, 2020

Benchmark for 4eff68e

Click to view benchmark
Test PR Benchmark Master Benchmark %
Cie family/lab to lch 3.2±0.18µs 3.0±0.09µs 104%
Cie family/lab to xyz 922.4±73.04ns 847.0±32.97ns 109.00000000000001%
Cie family/lch to lab 2.7±0.16µs 2.6±0.09µs 103%
Cie family/linsrgb to xyz 3.6±0.17µs 3.6±0.13µs 101%
Cie family/xyz to lab 9.9±0.69µs 9.6±0.53µs 103%
Cie family/xyz to yxy 564.8±23.95ns 533.1±18.70ns 106%
Cie family/yxy to xyz 548.5±21.36ns 547.6±26.87ns 100%
Matrix functions/matrix_inverse 14.0±0.44ns 14.0±0.51ns 100%
Matrix functions/multiply_3x3 9.7±0.40ns 9.6±0.69ns 100%
Matrix functions/multiply_rgb_to_xyz 4.6±0.27ns 4.6±0.16ns 100%
Matrix functions/multiply_xyz 4.8±0.16ns 4.5±0.19ns 100%
Matrix functions/multiply_xyz_to_rgb 4.8±0.18ns 4.5±0.15ns 100%
Matrix functions/rgb_to_xyz_matrix 22.7±0.95ns 20.5±1.12ns 110.00000000000001%
Rgb family/hsl to hsv 606.1±66.75ns 591.1±21.96ns 103%
Rgb family/hsl to linear hsl 8.5±0.47µs undefined 100%
Rgb family/hsl to rgb 2.3±0.09µs 7.2±0.27µs -108%
Rgb family/hsv to hsl 1078.6±39.68ns 977.1±46.67ns 110.00000000000001%
Rgb family/hsv to hwb 259.4±11.59ns 272.8±22.19ns 95%
Rgb family/hsv to linear hsv 8.0±0.34µs undefined 100%
Rgb family/hsv to rgb 2.2±0.09µs 7.3±0.38µs -125%
Rgb family/hwb to hsv 526.6±15.88ns 536.4±22.60ns 98%
Rgb family/hwb to linear hwb 8.8±0.29µs undefined 100%
Rgb family/linear hsl to hsl 8.8±0.35µs undefined 100%
Rgb family/linear hsv to hsv 8.3±0.33µs undefined 100%
Rgb family/linear hwb to hwb 9.2±0.31µs undefined 100%
Rgb family/linsrgb to rgb 4.5±0.18µs 4.9±0.77µs 92%
Rgb family/linsrgb_f32 to rgb_u8 6.2±0.26µs 6.4±0.21µs 96%
Rgb family/rgb to hsl 597.3±22.16ns 6.1±0.22µs -823%
Rgb family/rgb to hsv 438.2±18.92ns 5.5±0.19µs -1060%
Rgb family/rgb to linsrgb 4.9±0.16µs 4.9±0.21µs 99%
Rgb family/rgb_u8 to linsrgb_f32 5.0±0.19µs 5.2±0.20µs 94%
Rgb family/xyz to linsrgb 6.7±0.26µs 6.6±0.22µs 102%

@github-actions
Copy link

github-actions bot commented May 3, 2020

Benchmark for ea060dd

Click to view benchmark
Test PR Benchmark Master Benchmark %
Cie family/lab to lch 2.2±0.15µs 2.4±0.24µs 94%
Cie family/lab to xyz 690.2±44.43ns 677.0±31.39ns 102%
Cie family/lch to lab 2.7±0.17µs 2.7±0.20µs 101%
Cie family/linsrgb to xyz 3.0±0.49µs 3.0±0.24µs 101%
Cie family/xyz to lab 7.2±0.55µs 7.2±0.57µs 100%
Cie family/xyz to yxy 479.8±55.93ns 481.8±40.26ns 100%
Cie family/yxy to xyz 548.7±37.34ns 554.2±35.43ns 99%
Matrix functions/matrix_inverse 11.2±1.31ns 11.7±1.57ns 95%
Matrix functions/multiply_3x3 8.5±0.92ns 8.4±0.49ns 100%
Matrix functions/multiply_rgb_to_xyz 3.8±0.41ns 3.6±0.22ns 100%
Matrix functions/multiply_xyz 3.7±0.26ns 3.6±0.19ns 100%
Matrix functions/multiply_xyz_to_rgb 3.8±0.37ns 3.6±0.22ns 100%
Matrix functions/rgb_to_xyz_matrix 16.8±0.92ns 16.8±0.88ns 100%
Rgb family/hsl to hsv 509.9±38.98ns 541.5±27.62ns 94%
Rgb family/hsl to linear hsl 7.0±0.41µs undefined 100%
Rgb family/hsl to rgb 1830.0±152.31ns 5.7±0.51µs -110.00000000000001%
Rgb family/hsv to hsl 859.8±158.11ns 1018.6±65.91ns 82%
Rgb family/hsv to hwb 184.5±13.96ns 189.4±15.92ns 97%
Rgb family/hsv to linear hsv 6.6±0.37µs undefined 100%
Rgb family/hsv to rgb 1737.6±90.23ns 5.6±0.66µs -124.00000000000003%
Rgb family/hwb to hsv 424.4±25.75ns 464.0±29.03ns 90.99999999999999%
Rgb family/hwb to linear hwb 7.3±0.52µs undefined 100%
Rgb family/linear hsl to hsl 7.3±0.56µs undefined 100%
Rgb family/linear hsv to hsv 6.9±0.59µs undefined 100%
Rgb family/linear hwb to hwb 8.2±0.82µs undefined 100%
Rgb family/linsrgb to rgb 3.8±0.20µs 3.9±0.24µs 97%
Rgb family/linsrgb_f32 to rgb_u8 5.2±0.56µs 5.1±0.34µs 102%
Rgb family/rgb to hsl 492.2±37.91ns 4.8±0.36µs -778.9999999999999%
Rgb family/rgb to hsv 363.7±30.63ns 4.5±0.52µs -1047%
Rgb family/rgb to linsrgb 3.9±0.24µs 4.0±0.32µs 98%
Rgb family/rgb_u8 to linsrgb_f32 4.2±0.27µs 4.2±0.31µs 100%
Rgb family/xyz to linsrgb 5.1±0.37µs 4.9±0.37µs 105%

@@ -24,7 +24,7 @@ use crate::{
/// `Alpha`](struct.Alpha.html#Hsla).
pub type Hsla<S = Srgb, T = f32> = Alpha<Hsl<S, T>, T>;

/// Linear HSL color space.
/// HSL color space.
///
/// The HSL color space can be seen as a cylindrical version of
/// [RGB](rgb/struct.LinRgb.html), where the `hue` is the angle around the color
Copy link
Contributor

@okaneco okaneco May 4, 2020

Choose a reason for hiding this comment

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

Suggested change
/// [RGB](rgb/struct.LinRgb.html), where the `hue` is the angle around the color
/// [RGB](rgb/struct.Rgb.html), where the `hue` is the angle around the color

These doc links are currently broken in the Hsl, Hsv, and Hwb files. Not related directly to this PR, but found out recently while looking at the docs.

Copy link
Owner Author

Choose a reason for hiding this comment

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

Ah, but they should point to just Rgb after these changes, anyway, since none of them are necessarily based on linear RGB anymore. Good catch!

Copy link
Contributor

Choose a reason for hiding this comment

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

Exactly 👍

What I also meant was that the current release's docs don't point anywhere
https://docs.rs/palette/0.5.0/palette/rgb/struct.LinRgb.html

Copy link
Owner Author

Choose a reason for hiding this comment

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

Yep, it will be two birds with one stone. Would be nice with a lint for it, but I guess we will just have to wait for intra-rustdoc links first.

@@ -24,7 +24,7 @@ use crate::{
/// `Alpha`](struct.Alpha.html#Hsva).
pub type Hsva<S = Srgb, T = f32> = Alpha<Hsv<S, T>, T>;

/// Linear HSV color space.
/// HSV color space.
///
/// HSV is a cylindrical version of [RGB](rgb/struct.LinRgb.html) and it's very
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
/// HSV is a cylindrical version of [RGB](rgb/struct.LinRgb.html) and it's very
/// HSV is a cylindrical version of [RGB](rgb/struct.Rgb.html) and it's very

@@ -24,10 +24,10 @@ use crate::{
/// `Alpha`](struct.Alpha.html#Hwba).
pub type Hwba<S = Srgb, T = f32> = Alpha<Hwb<S, T>, T>;

/// Linear HWB color space.
/// HWB color space.
///
/// HWB is a cylindrical version of [RGB](rgb/struct.LinRgb.html) and it's very
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
/// HWB is a cylindrical version of [RGB](rgb/struct.LinRgb.html) and it's very
/// HWB is a cylindrical version of [RGB](rgb/struct.Rgb.html) and it's very

@github-actions
Copy link

github-actions bot commented May 9, 2020

Benchmark for 7371a64

Click to view benchmark
Test PR Benchmark Master Benchmark %
Cie family/lab to lch 3.0±0.18µs 2.9±0.14µs 103%
Cie family/lab to xyz 856.6±61.77ns 859.6±38.81ns 100%
Cie family/lch to lab 2.6±0.15µs 2.6±0.13µs 101%
Cie family/linsrgb to xyz 3.7±0.22µs 3.4±0.19µs 108%
Cie family/xyz to lab 9.3±0.40µs 9.5±0.42µs 98%
Cie family/xyz to yxy 561.5±28.87ns 537.4±27.28ns 104%
Cie family/yxy to xyz 530.4±30.58ns 511.3±29.91ns 104%
Matrix functions/matrix_inverse 13.5±0.76ns 12.9±0.71ns 105%
Matrix functions/multiply_3x3 9.1±0.43ns 9.0±0.51ns 101%
Matrix functions/multiply_rgb_to_xyz 4.4±0.21ns 4.4±0.26ns 100%
Matrix functions/multiply_xyz 4.6±0.20ns 4.4±0.24ns 100%
Matrix functions/multiply_xyz_to_rgb 4.6±0.22ns 4.4±0.27ns 100%
Matrix functions/rgb_to_xyz_matrix 19.7±1.61ns 19.6±1.85ns 100%
Rgb family/hsl to hsv 579.7±30.01ns 579.8±29.56ns 100%
Rgb family/hsl to linear hsl 8.4±0.33µs undefined 100%
Rgb family/hsl to rgb 2.3±0.09µs 6.8±0.34µs -89.99999999999999%
Rgb family/hsv to hsl 1055.3±54.30ns 946.0±53.54ns 112.00000000000001%
Rgb family/hsv to hwb 264.8±14.57ns 260.3±14.34ns 102%
Rgb family/hsv to linear hsv 8.0±0.40µs undefined 100%
Rgb family/hsv to rgb 2.3±0.11µs 6.8±0.55µs -100%
Rgb family/hwb to hsv 507.6±37.57ns 507.4±26.42ns 100%
Rgb family/hwb to linear hwb 9.1±0.33µs undefined 100%
Rgb family/linear hsl to hsl 9.0±0.34µs undefined 100%
Rgb family/linear hsv to hsv 8.4±0.37µs undefined 100%
Rgb family/linear hwb to hwb 9.3±0.44µs undefined 100%
Rgb family/linsrgb to rgb 4.6±0.41µs 4.4±0.28µs 105%
Rgb family/linsrgb_f32 to rgb_u8 6.2±0.21µs 6.0±0.39µs 104%
Rgb family/rgb to hsl 580.8±39.78ns 6.2±0.35µs -872.0000000000001%
Rgb family/rgb to hsv 419.6±21.68ns 5.6±0.29µs -1135%
Rgb family/rgb to linsrgb 4.6±0.25µs 5.0±0.24µs 90.99999999999999%
Rgb family/rgb_u8 to linsrgb_f32 5.2±0.53µs 5.4±0.30µs 96%
Rgb family/xyz to linsrgb 6.7±0.30µs 6.3±0.34µs 106%

@Ogeon
Copy link
Owner Author

Ogeon commented May 10, 2020

I think that's it for this one. Thanks for reviewing!

bors r+

@bors
Copy link
Contributor

bors bot commented May 10, 2020

Build succeeded:

@bors bors bot merged commit 48453b6 into master May 10, 2020
@bors bors bot deleted the rgb_standard_aware branch May 10, 2020 12:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Strange conversion results Understanding when to use Srgba vs LinSrgba
2 participants