-
Notifications
You must be signed in to change notification settings - Fork 768
Add support for CSS conic-gradient 'from <angle>' syntax #9830
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
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the motivation behind this.
It looks like this makes Slint different from what the CSS spec says.
|
Thanks for your review.
In https://snapshots.slint.dev/master/editor/index.html?load_url=https://raw.githubusercontent.com/slint-ui/slint/master/examples/speedometer/demo.slint, CarButton tries to rotate angles in @conic-gradients when it is pressed. To solve this, I thought it would be good to support any degrees.
Right. should we introduce the |
|
I see. I can achieve the desired effect with
I added stop4 to stop6 back at the beginning with a I would prefer keeping close to the CSS spec for gradients. |
1a156d1 to
578faaf
Compare
Implement rotation support for conic gradients by adding the 'from <angle>' syntax, which rotates the entire gradient by the specified angle. - Add `from_angle` field to ConicGradient expression - Parse 'from <angle>' syntax in compiler (defaults to 0deg when omitted) - Normalize angles to 0-1 range (0.0 = 0°, 1.0 = 360°) - Add `ConicGradientBrush::rotated_stops()` method that: * Applies rotation by adding from_angle to each stop position * Adds boundary stops at 0.0 and 1.0 with interpolated colors * Handles stops outside [0, 1] range for boundary interpolation - Update all renderers (Skia, FemtoVG, Qt, Software) to use rotated_stops() The rotation is applied at render time by the rotated_stops() method, which ensures all renderers consistently handle the gradient rotation.
5a02287 to
820ae2b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice! thank you very much.
This looks quite good to me.
I'm just wondering what you think about the idea to normalize the gradient brush at construction time instead of every time it is used.
internal/core/graphics/brush.rs
Outdated
| /// | ||
| /// This is useful when you need to work with the actual visual positions of the stops | ||
| /// after the gradient has been rotated. | ||
| pub fn rotated_stops(&self) -> alloc::vec::Vec<GradientStop> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't we be storing them like this instead of re-allocating a new vector every time we want to draw?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When from is not set, we can use the vector prepared in new(). In case of with from, we can pass the vector to backends as it is. However, (all?) backend that doesn't support angles < 0 and > 1 needs to do the conversion.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But why don't we compute that in new already since new already knows from?
| } | ||
|
|
||
| /// Helper: Linearly interpolate between two colors | ||
| fn interpolate_color(c1: Color, c2: Color, t: f32) -> Color { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this different from Color::mix ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The CSS gradient spec uses simple linear interpolation between color stops. The gradient line’s color is interpolated between the colors of the two color stops, with the interpolation taking place in premultiplied RGBA space. premultiplied is not implemented in the interpolate_color yet.
https://www.w3.org/TR/css-images-3/#coloring-gradient-line
Color::mix implements the Sass color mixing algorithm, which applies alpha-weighted blending.
https://github.com/sass/sass/blob/47d30713765b975c86fa32ec359ed16e83ad1ecc/spec/built-in-modules/color.md#mix
Without alpha, no difference. with alpha, there is difference. Do you think Color should support two mix methods?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The code of Color::mix is maybe a bit hard to read. But I think the reason is that when we go from, say #ffff to #f001, this should go from white to slightly red, but it shouldn't show the red too much.
This example illustrate the difference: slintpad
Now I must say that i haven't compared the functions implementation much, but your implementation here is not in premultiplied space.
But if you say that Color::mix doesn't work for this, so be it.
Wraps the rotated conic gradient example in CodeSnippetMD to automatically generate and display a visual screenshot of the gradient rotation effect. This makes it easier for users to understand how the 'from' parameter rotates the gradient.
The from_angle and stops fields don't need to be pub since: - Rust code in the same module can access them without pub - C++ FFI access works through cbindgen-generated struct (C++ struct members are public by default)
- Changed return type from Vec to SharedVector - When from_angle is zero, returns a clone of internal SharedVector (only increments reference count instead of allocating new Vec) - Removed break from duplicate position separation loop to handle all duplicate pairs, not just the first one - Updated documentation to match actual implementation
- CSS conic-gradient does not automatically sort color stops - Stops are processed in the order specified by the user - Changed boundary stop interpolation logic to use max_by/min_by instead of relying on sorted order - This allows CSS-style hard transitions when stops are out of order
2a5b848 to
daabee4
Compare
| /// The `from_angle` parameter is in normalized form (0.0 = 0°, 1.0 = 360°), corresponding | ||
| /// to CSS's `from <angle>` syntax. It rotates the entire gradient clockwise. | ||
| pub fn new(from_angle: f32, stops: impl IntoIterator<Item = GradientStop>) -> Self { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For consistency with LinearGradientBrush::new, i think the angle should be specified in degrees.
In Slint, angles are always degrees.
| } | ||
| Brush::ConicGradient(g) => { | ||
| Brush::ConicGradient(ConicGradientBrush::new(g.stops().map(|s| GradientStop { | ||
| Brush::ConicGradient(g) => Brush::ConicGradient(ConicGradientBrush::new( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since this touches only the colors, and not the stops, this could just map the colors without having to re-normalize thing.
This doesn't have to be done in this PR, but we could imagine a (internal) Brush::visit_colors(&mut self, map: impl Fn(&mut Color)) and so one could implement brighter, darker, with_alpha and transparentize using it. But anyway, this can be done independently.
| } | ||
|
|
||
| /// Helper: Linearly interpolate between two colors | ||
| fn interpolate_color(c1: Color, c2: Color, t: f32) -> Color { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The code of Color::mix is maybe a bit hard to read. But I think the reason is that when we go from, say #ffff to #f001, this should go from white to slightly red, but it shouldn't show the red too much.
This example illustrate the difference: slintpad
Now I must say that i haven't compared the functions implementation much, but your implementation here is not in premultiplied space.
But if you say that Color::mix doesn't work for this, so be it.
internal/core/graphics/brush.rs
Outdated
| /// | ||
| /// This is useful when you need to work with the actual visual positions of the stops | ||
| /// after the gradient has been rotated. | ||
| pub fn rotated_stops(&self) -> alloc::vec::Vec<GradientStop> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But why don't we compute that in new already since new already knows from?
Summary
Implements rotation support for conic gradients by adding the CSS-standard
from <angle>syntax, which rotates the entire gradient by the specified angle.Syntax