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

Srgb -> Okhsl conversion results in NaN saturation #368

Closed
ecton opened this issue Jan 10, 2024 · 5 comments · Fixed by #369
Closed

Srgb -> Okhsl conversion results in NaN saturation #368

ecton opened this issue Jan 10, 2024 · 5 comments · Fixed by #369
Labels
defect Something that isn't as or doesn't work as intended

Comments

@ecton
Copy link

ecton commented Jan 10, 2024

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

  • Cargo.toml entry:

    Both of these reproduce the issue:

    palette = { git = "https://github.com/Ogeon/palette.git" }
    palette = "0.7.3"
  • Rust version(s): rustc 1.75.0 (82e1608df 2023-12-21)

  • Target platform: Arch Linux / x86_64-unknown-linux-gnu

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.

@ecton ecton added the defect Something that isn't as or doesn't work as intended label Jan 10, 2024
@Ogeon
Copy link
Owner

Ogeon commented Jan 10, 2024

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
Copy link
Author

ecton commented Jan 10, 2024

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 😆

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

@Ogeon
Copy link
Owner

Ogeon commented Jan 10, 2024

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

Ogeon commented Jan 14, 2024

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
Copy link
Author

ecton commented Jan 14, 2024

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
defect Something that isn't as or doesn't work as intended
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants