Ogeon / palette

A Rust library for linear color calculations and conversion
Apache License 2.0
755 stars 60 forks source link

Srgb -> Okhsl conversion results in NaN saturation #368

Closed ecton closed 10 months ago

ecton commented 10 months ago

How To Reproduce

When converting from an Srgb value with 1.0 in all components into an Okhsl, the resulting saturation value is NaN. This test demonstrates the issue:

#[test]
fn okhsl_nan() {
    use palette::IntoColor;
    let hsl: palette::Okhsl = palette::Srgb::new(1.0, 1.0, 1.0).into_color();
    assert!(!hsl.saturation.is_nan());
}

Expected Outcome

I would expect anything except not a number. I think I'm expecting 1.0, but I'm not a color expert so I wouldn't be surprised if I was wrong.

Actual Outcome

The saturation component is not a number.

Additional Details

Thank you for the wonderful library! This is my first time ever having any sort of problem with it. It's always my go-to when I need to work with colors.

Ogeon commented 10 months ago

Hi, thanks for pointing this out! The saturation should be 0 in that case (due to it being grayscale), but definitely not NaN. This should not be an unusual input, so I'm a bit surprised it wasn't discovered sooner.

Also, thanks for the kind words and I'm happy to see that it has been working so well so far! 🎉

ecton commented 10 months ago

Honestly, I was just as surprised as you that it was so simple to reproduce. You're absolutely right that 0.0 is the correct saturation. I was still having my morning coffee and hadn't figured that out yet :laughing:

I have a simple workaround in place for now, so this isn't a high priority for me to have resolved. Thanks again!

Ogeon commented 10 months ago

Ok, good to know that you aren't stuck on this. I will probably save it for the weekend, when I have time to have a proper look at where it comes from. It's usually a missing check for 0 or something similar, but finding it can be a bit tricky.

Ogeon commented 10 months ago

I found the issue and merged a fix. There was an edge case with saturated white or black input, as explained in #369. I'll just address a couple of requests from the Debian rust team before making a release, so I may end up postponing it a bit so they have a chance to respond.

ecton commented 10 months ago

Thank you! Definitely no rush needed on a release for me.