feat: color: add HSV methods to slint #4819
Conversation
|
Ref issue #911 |
|
I had some extra time to play more with this and tried out making it member-field access instead of function - I don't think there's any kind of easy way for me to do that without a new I also didn't want to add more fields on the Color type as that would affect the struct size. Unless y'all think that adding a new slint type is a good thing to do? If so, I'd best leave it to the pros, otherwise I will complete this PR soon. |
flukejones
changed the title
There was a problem hiding this comment.
Thank you so much for your work.
I'm a bit busy this week, so I won't comment much on the implementation at this point. But I just want to get the API right.
As is, this PR adds:
- Functions on the Slint color and brush type:
hue()->float(return value between 0.0 and 360.0)saturation()->float(return value between 0.0 and 255.0)brightness()->float(return value between 0.0 and 255.0)
- Functions in the Colors namespace:
hsvandhsvawhich are alias to eachother. It seems that the hue is a float between 0 and 360, when saturation, value, and alpha are flaot between 0.0 and 1.0
- Rust API on the Color struct
/// Converts this color to a tuple of `(hue, saturation, value)` pub fn to_hsv(&mut self) -> (f32, f32, f32) /// Converts this color to a tuple of `(hue, saturation, value, alpha)` pub fn to_hsva(&mut self) -> (f32, f32, f32, f32) /// Construct a color from the hue, saturation, and value HSV color parameters. The alpha /// channel will have the value 1.0. pub fn from_hsv(hue: f32, saturation: f32, value: f32) -> Self /// Construct a color from the hue, saturation, and value HSV color parameters. pub fn from_hsva(hue: f32, saturation: f32, value: f32, alpha: f32) -> Self /// Returns the hue channel of the color as f32 in degrees 0..PI. pub fn hue(mut self) -> f32 /// Returns the saturation of the color as u8 in the range 0..255. pub fn saturation(mut self) -> f32 /// Returns the brightness of the color as u8 in the range 0..255. pub fn brightness(mut self) -> f32
- Not included in this patch, but something will be needed in the C++ API as well
One thing to note is that the compiler can do transformations so that the Rust API and the slint API don't have to be the same.
For example, wether or not hue is a field or a function in Slint is just a matter of having an Expression::Call or not in the lookup.rs
(for example, red is a field in slint and is calling BuiltinFunction::ColorRgbaStruct in the generated code before accessing a field)
Also you don't need to add a Type::ColorHsva, you can just use anonymous types such as {a: float, h: angle, s: float, v: float} which
is represented in Rust as a tuple (f32, f32, f32, f32) and in C++ std::tuple<float, float, float, float>. This can be an internal detail
in rust.rs and cpp.rs which is not part of the C++ and Rust API.
Given that, we have much freedom to do anything.
So my comments/questions on the API
- for the hue in Slint: should we use the
angletype or thefloattype or theinttype? I think i'd go with float since one probably want to do some math on it and not dobackround.hue/1degall the time. - for the brightness and saturation, it is sometimes between 0 and 1 and sometimes between 0 and 255, this is not consistent. I'd use the 0 to 1 range consistently. Although we must also consider what the rgb or alpha values are.
- I have commented on the Slint API in this comment Add properties to the color type to get HSV component #911 (comment) , I think it should either be a field:
background.hueor a function that return an anonymous type:background.to-hsv().hthe later would allow to dinstinguish the saturation between the HSV and HSL color space. - Do we need both Colors.hsv and Colors.hsva? rgb and rgba both exist for consistency with CSS, but there is no hsv in CSS. (there is hsl() and a bunch of other) I'd go with only the former.
- For the Rust API, we probably don't need both to_hsv and the getter on each component. I wonder if the best would not be to make the
HsvaColortype public and have conversion between these. Otherwise i think a tuple is not that bad. But the range needs to be well documented. (and if we go for the 0..255, it should probably be u8) - Same for the C++ API
@tronical: opinions on the API?
I agree with this. The reason I need hsv myself is because I want to do math on it.
Documentation mistake by me. Corrected now.
One of my concerns was the addition of extra colourspaces. So I agree with later option.
No alpha?
I would prefer making |
|
I'm afraid I will have to leave this as is now. I have too many tasks at work. I converted to struct member access with the goal of getting to If a clear example of how to achieve @tronical or @ogoffart if one of you could please take over? |
|
Please also note I have not touched the C++ bindings - this is not one of my strong suits |
|
Updated the pr to add a |
TODO:
vcomponent