Add Lchuv support #223
Add Lchuv support #223
Conversation
| pub fn run_luv_to_lchuv_tests() { | ||
| for (_, v) in TEST_DATA.iter() { | ||
| let mut to_lchuv: Lchuv<D65, f64> = v.luv.into_color_unclamped(); | ||
| if to_lchuv.chroma < 1e-8 { |
There was a problem hiding this comment.
This is a worth a comment.
In the implementation that generates the examples, chroma values below a certain threshold are a treated as 0.0, so the hue is set as 0 rather than using whatever comes out of atan (and probably isn't all that meaningful). My implementation matches the existing Lab -> Luv implementation and only does this if u* and v* are strictly 0. I can change my implementation to use a threshold on chroma instead, or we can leave this tweak in place in the test.
There was a problem hiding this comment.
There's going to be a point where the hue is almost just noise from the floating point operations. This was mentioned just yesterday in #107. I'm ok with a cut-off point like this.
There was a problem hiding this comment.
To be clear, do you want the cut-off point in the code itself for hue calculation, or just the tests?
There was a problem hiding this comment.
Ah, yes, sorry. Let's have it only in the test for now.
palette/src/hues.rs
Outdated
| @@ -20,7 +20,7 @@ macro_rules! make_hues { | |||
| /// number (like `f32`). This makes many calculations easier, but may | |||
| /// also have some surprising effects if it's expected to act as a | |||
| /// linear number. | |||
| #[derive(Clone, Copy, Debug, Default)] | |||
| #[derive(Clone, Copy, Debug, Default)] | |||
There was a problem hiding this comment.
cargo fmt does weird indenting here.
There was a problem hiding this comment.
Indeed, what is this? Maybe some bug. You can try to shuffle the attributes around, but if this is what it want to do, then so be it. 🤷♂️
There was a problem hiding this comment.
I saw some other odd thing further down, so maybe it's not a big fan of macros at the moment.
There was a problem hiding this comment.
Playing around with it more, it seems to just be not opinionated inside the macro. If I manually correct the indentation, rustfmt doesn't "fix" it.
There was a problem hiding this comment.
I would expect it to not want to do too much with macro code, considering it's much more free form. 🤔 Just do your best to make it look nice.
|
The benchmark is failing for some pretty inscrutable reason, even though the CI build passed. |
|
You can ignore the benchmarks they are not working for PRs from forks at the moment. |
rustfmt is unopionated inside macros for now, so this commit manually adjusts some indentation to make it look better.
|
Hah, simple but effective. And the implementation looks great! Thank you! bors r+ |
|
Build succeeded: |
Round two! This branch implements the CIELCHuv color space.
I also cleaned up the
Uniform*Hueimplementation inhues.rsto use a macro to save on some repetitive typing.