-
Notifications
You must be signed in to change notification settings - Fork 61
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 Hsluv support #225
Add Hsluv support #225
Conversation
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.
Nice! Good call with the macros. I think they could be used for the other spaces as well. As for the random sampling, I'm pretty sure you can simply use the same method as "regular" HSL uses, which is to sample a bi-cone, where the saturation becomes slimmer towards min and max lightness. There's already code for it in the HSL module. It's going to be its own space in a way, since it's warped from Luv's perspective, but I think that's fine. It's meant to be a more balanced alternative to the naive sampling (which anyone can do on their own), but not necessarily perfect in every way.
palette/src/arithmetic.rs
Outdated
/// Implement `Add` and `AddAssign` traits for a color space. | ||
/// | ||
/// Both scalars and color arithmetic are implemented. | ||
#[macro_export] |
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 always forget the details, but I'm pretty sure #[macro_export]
is unnecessary for internal macros, but the order the modules are declared in the lib.rs
file matters. Macros and macro modules have to come first. I would suggest putting this module inside the macros
module that's already in here.
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, I bet the ordering was tripping me up on macros. I'll take your suggestion and just move them to macros.rs
, though. I'll also do a bi-cone style implementation for Hsluv
.
Nice, thank you! It looks to me like this is ready to be merged. I just need to make the |
It should hopefully be fixed now in the master branch 🤞, so you need to merge it in here or rebase your branch. Whatever you are more comfortable with. 🙂 |
0747291
to
11954d1
Compare
I re-based the branch on top of master. |
Nice and the tests passed this time, so it seem to be all good! Thank you for all of this! I'll just take the liberty to add a "closing" line to the description, so that goes into the machinery. |
bors r+ |
Build succeeded: |
Round three!
This finally implements the HSLuv color space as
Hsluv
. A few notes.Hsluv
because I wasn't convinced that the naive version would be anything close to correct. I think you could do something like rejection sample inLchuv
(based on if, after converting that to Xyz, it were in-gamut) and then convert the result toHsluv
, but that's a very different approach than anything I see currently implemented.core::ops::Add
and related traits, since they're pretty repetitive. I also added tests to confirm that all of the arithmetic operations are implemented (though there is no correctness checking).l
as the field name, rather thanlightness
, since it is the same as theLchuv
l
.Closes #112