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 and Substract - clamp after operations? #19

Closed
sidred opened this issue Jan 22, 2016 · 5 comments · Fixed by #25
Closed

Add and Substract - clamp after operations? #19

sidred opened this issue Jan 22, 2016 · 5 comments · Fixed by #25
Milestone

Comments

@sidred
Copy link
Contributor

sidred commented Jan 22, 2016

For colors with known ranges like rgb, hsl etc., should we clamp the values? A possible issue is with the f32 -> u8 conversion panicking if not clamped.

@Ogeon
Copy link
Owner

Ogeon commented Jan 22, 2016

That may mess with the math. I think it's better to clamp just before converting. Isn't that already implemented?

@sidred
Copy link
Contributor Author

sidred commented Jan 22, 2016

Currently not implemented even in the current master.

impl<T: Float> RgbPixel<T> for (u8, u8, u8, u8) {
    fn from_rgba(red: T, green: T, blue: T, alpha: T) -> (u8, u8, u8, u8) {
        ((red * T::from(255.0).unwrap()).to_u8().unwrap(),
         (green * T::from(255.0).unwrap()).to_u8().unwrap(),
         (blue * T::from(255.0).unwrap()).to_u8().unwrap(),
         (alpha * T::from(255.0).unwrap()).to_u8().unwrap())
    }

    fn to_rgba(&self) -> (T, T, T, T) {
        let (r, g, b, a) = *self;
        (T::from(r).unwrap() / T::from(255.0).unwrap(),
         T::from(g).unwrap() / T::from(255.0).unwrap(),
         T::from(b).unwrap() / T::from(255.0).unwrap(),
         T::from(a).unwrap() / T::from(255.0).unwrap())
    }
}

@Ogeon
Copy link
Owner

Ogeon commented Jan 22, 2016

Yes, it is in each conversion function.

@sidred
Copy link
Contributor Author

sidred commented Jan 22, 2016

I've at this looked at this function link

I am not sure I understand it correctly. Lets say I have 2 Rgb and I add them and convert to [u8;3]. These is no clamping in this process, is there?

@Ogeon
Copy link
Owner

Ogeon commented Jan 22, 2016

No, it happens in the functions I linked to, just before those are called. It may still be a good idea to clamp after multiplying with 255, because of how floating point math works.

@Ogeon Ogeon added this to the 0.2.0 milestone Jan 25, 2016
@homu homu closed this as completed in #25 Jan 28, 2016
homu added a commit that referenced this issue Jan 28, 2016
Fix or relax some color ranges and clamping

The `ColorSpace` trait has been repurposed as a trait for checking limits and clamping, and is now called `Limited`. The limits of `Xyz` has been changed to use the white point as its maximum (closing #10), and the maximum of `Lch.chroma` has been removed, since it's not very well defined. The ranges of `Lch` stays as they are, for the sake of consistency.

Extra clamping is also added when converting RGB colors to `u8` based pixel representation, to prevent float shenanigans when multiplying values with 255.0. It's impossible to know what a type `T` will do when it's asked to convert `255.12312312` or whatever to `u8`. This closes #19.

This is a breaking change, since it changes the ranges of some color spaces and renames the `ColorSpace` trait to `Limited`.
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 a pull request may close this issue.

2 participants