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

Error in colorspace conversion from RGBA to HSLA #4382

Closed
fgiordana opened this issue Mar 31, 2022 · 0 comments
Closed

Error in colorspace conversion from RGBA to HSLA #4382

fgiordana opened this issue Mar 31, 2022 · 0 comments
Labels
A-Rendering Drawing game state to the screen C-Bug An unexpected or incorrect behavior

Comments

@fgiordana
Copy link
Contributor

fgiordana commented Mar 31, 2022

Bevy version

commit 5e6400a

Operating system & version

macOS 12.2.1 on MBP 2021, Apple M1 Max

What you did

#[cfg(test)]
mod test {
    use bevy::prelude::*;

    const EPSILON: f32 = 0.0001;
    fn are_equal(a: &[f32; 4], b: &[f32; 4]) -> bool {
        for i in 0..4 {
            if (a[i] - b[i]).abs() > EPSILON {
                return false;
            }
        }
        true
    }

    #[test]
    fn test_hsl() {
        for r in 0..255 {
            for g in 0..255 {
                for b in 0..255 {
                    let c_rgba = Color::rgba(r as f32 / 255.0, g as f32 / 255.0, b as f32 / 255.0, 1.0);
                    let c_hsla = c_rgba.as_hsla();
                    if !are_equal(&(c_rgba.as_rgba_f32()), &(c_hsla.as_rgba_f32())) {
                        panic!("colors are different: {:?}, {:?}\n{:?}, {:?}", c_rgba, c_hsla, c_rgba.as_rgba_f32(), c_hsla.as_rgba_f32());
                    }
                }
            }
        }
    }
}

What you expected to happen

The test should be successful. There is an error in the conversion formula when the R and G components are the same

What actually happened

thread 'scene::test::test_hsl' panicked at 'colors are different: Rgba { red: 0.003921569, green: 0.003921569, blue: 0.0, alpha: 1.0 }, Hsla { hue: 240.0, saturation: 1.0, lightness: 0.0019607844, alpha: 1.0 }
[0.003921569, 0.003921569, 0.0, 1.0], [2.3283064e-10, 2.3283064e-10, 0.0039215684, 1.0]'

Additional information

There is an error in the conversion formula, it should actually be:

/// converts a color in sRGB space to HLS space
#[inline]
pub fn nonlinear_srgb_to_hsl([red, green, blue]: [f32; 3]) -> (f32, f32, f32) {
    // https://en.wikipedia.org/wiki/HSL_and_HSV#From_RGB
    let x_max = red.max(green.max(blue));
    let x_min = red.min(green.min(blue));
    let chroma = x_max - x_min;
    let lightness = (x_max + x_min) / 2.0;
    let hue = if chroma == 0.0 {
        0.0
    } else if red == x_max {
        60.0 * (green - blue) / chroma
    } else if green == x_max {
        60.0 * (2.0 + (blue - red) / chroma)
    } else {
        60.0 * (4.0 + (red - green) / chroma)
    };
    let hue = if hue < 0.0 { 360.0 + hue } else { hue };
    let saturation = if lightness <= 0.0 || lightness >= 1.0 {
        0.0
    } else {
        (x_max - lightness) / lightness.min(1.0 - lightness)
    };

    (hue, saturation, lightness)
}
@fgiordana fgiordana added C-Bug An unexpected or incorrect behavior S-Needs-Triage This issue needs to be labelled labels Mar 31, 2022
@alice-i-cecile alice-i-cecile added A-Rendering Drawing game state to the screen and removed S-Needs-Triage This issue needs to be labelled labels Mar 31, 2022
@bors bors bot closed this as completed in dbd5e7a Apr 4, 2022
aevyrie pushed a commit to aevyrie/bevy that referenced this issue Jun 7, 2022
…ne#4383)

https://en.wikipedia.org/wiki/HSL_and_HSV#From_RGB

# Objective
Fixes bevyengine#4382

## Solution

- Describe the solution used to achieve the objective above.
Fixed conversion formula to account for red and green component being max and equal
---

## Changelog
Fixed RGB -> HSL colorspace conversion

## Migration Guide


Co-authored-by: Francesco Giordana <[email protected]>
ItsDoot pushed a commit to ItsDoot/bevy that referenced this issue Feb 1, 2023
…ne#4383)

https://en.wikipedia.org/wiki/HSL_and_HSV#From_RGB

# Objective
Fixes bevyengine#4382

## Solution

- Describe the solution used to achieve the objective above.
Fixed conversion formula to account for red and green component being max and equal
---

## Changelog
Fixed RGB -> HSL colorspace conversion

## Migration Guide


Co-authored-by: Francesco Giordana <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Rendering Drawing game state to the screen C-Bug An unexpected or incorrect behavior
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants