-
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 CIE Luv support #221
Add CIE Luv support #221
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! First step! I have only nit picks, really, except the dbg
lines that got into the commit. They are also why the tests are failing. I also noticed the indentation was a bit mixed, so please run this through rustfmt
as well. 🙂 Well done!
palette/src/lch.rs
Outdated
@@ -8,7 +8,7 @@ use rand::distributions::{Distribution, Standard}; | |||
#[cfg(feature = "random")] | |||
use rand::Rng; | |||
|
|||
use crate::color_difference::ColorDifference; | |||
use crate::{color_difference::ColorDifference}; |
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.
You can revert this change while fixing other parts.
palette/src/luv.rs
Outdated
/// u* goes from -100 to 100. | ||
pub u: T, | ||
|
||
/// v* goes from -100 to 100. |
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.
It's probably good to escape the *
for good measure, to avoid accidental emphasis formatting.
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.
The comment itself (about the [-100, 100] range) also isn't fully accurate, so I'll address that as well.
palette/src/luv.rs
Outdated
} | ||
|
||
fn clamp_self(&mut self) { | ||
self.l = clamp(self.l, T::zero(), from_f64(100.0)); |
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.
You could use {min,max}_l()
here too.
#[derive(Clone, Debug)] | ||
struct HsluvExample { | ||
name: String, | ||
lch: Lch<D65, f64>, |
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.
Is this the correct LCH? 😬
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 believe this is the appropriate LCH, but I'll remove it until I actually implement LCH in the next round so it's actually tested.
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.
Silly me. That's definitely not the right Lch
. I started working on Lchuv
in a new branch on top of this one, and mentally I've already changed this. But of course it's not right in this branch yet 🤦🏾
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.
Parallel universes can be confusing like that. 😁
palette/src/luv.rs
Outdated
dbg!(u_ref_prime.to_f64().unwrap()); | ||
|
||
let v_prime: T = from_f64(9.0) * color.y * prime_denom_recip; | ||
let v_ref_prime = from_f64(9.0) * w.y * prime_ref_denom_recip; | ||
dbg!(v_ref_prime.to_f64().unwrap()); |
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.
These dbg
lines should be removed.
Remove debugging statements Fix use statement formatting Use `Luv::min_l`, `Luv::max_l` consistently in clamp functions Comment formatting and accuracy for `Luv::u`, `Luv::v` fields
There are a few other files that |
No need to include the additional files. I think this is ready for a merge. Thanks! bors r+ |
Build succeeded: |
Implements the CIELUV color space, translating directly to and from
Xyz
.First step towards
Hsluv
implementation.