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

Add CIE Luv support #221

Merged
merged 8 commits into from May 10, 2021
Merged

Add CIE Luv support #221

merged 8 commits into from May 10, 2021

Conversation

masonium
Copy link
Contributor

@masonium masonium commented May 9, 2021

Implements the CIELUV color space, translating directly to and from Xyz.

First step towards Hsluv implementation.

Copy link
Owner

@Ogeon Ogeon left a 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!

@@ -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};
Copy link
Owner

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.

Comment on lines 51 to 54
/// u* goes from -100 to 100.
pub u: T,

/// v* goes from -100 to 100.
Copy link
Owner

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.

Copy link
Contributor Author

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.

}

fn clamp_self(&mut self) {
self.l = clamp(self.l, T::zero(), from_f64(100.0));
Copy link
Owner

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>,
Copy link
Owner

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? 😬

Copy link
Contributor Author

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.

Copy link
Contributor Author

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 🤦🏾

Copy link
Owner

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. 😁

Comment on lines 241 to 245
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());
Copy link
Owner

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
@masonium
Copy link
Contributor Author

masonium commented May 9, 2021

There are a few other files that cargo fmt modifies when running it over the whole repo (palette/build/named.rs, palette/examples/color_scheme.rs, palette/src/named.rs). I didn't include those with my formatting commit because they are technically unrelated, but I can just add them if you like.

@Ogeon
Copy link
Owner

Ogeon commented May 10, 2021

No need to include the additional files. I think this is ready for a merge. Thanks!

bors r+

@bors
Copy link
Contributor

bors bot commented May 10, 2021

Build succeeded:

@bors bors bot merged commit b1bdbc1 into Ogeon:master May 10, 2021
@Ogeon Ogeon changed the title Feature/cieluv Add CIE Luv support Jul 4, 2021
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.

None yet

2 participants