Allow HSV, HSL and HWB to represent nonlinear RGB #188
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Benchmark for 3f73fe7Click to view benchmark
|
|
This is a really nice change. 👍 Should a |
|
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. |
|
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. |
|
True. It should be quick to add anyway. |
|
Wow, how do I even git? Anyway, I added some benchmarks. Not all of them, for the reasons above. |
Benchmark for 513828cClick to view benchmark
|
Benchmark for 4eff68eClick to view benchmark
|
Benchmark for ea060ddClick to view benchmark
|
okaneco
reviewed
May 4, 2020
palette/src/hsl.rs
Outdated
| @@ -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 | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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!
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
palette/src/hsv.rs
Outdated
| @@ -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 | |||
There was a problem hiding this comment.
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 |
palette/src/hwb.rs
Outdated
| @@ -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 | |||
There was a problem hiding this comment.
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 |
Benchmark for 7371a64Click to view benchmark
|
|
I think that's it for this one. Thanks for reviewing! bors r+ |
|
Build succeeded: |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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) toRgb<Srgb>is still a single step process, while converting toRgb<Linear<Srgb>>is a two step process. There are still shortcuts in place to only change the RGB standard, meaning it's possible to writeHsl::<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