-
Notifications
You must be signed in to change notification settings - Fork 600
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
Path rendering on software renderer #6032
base: master
Are you sure you want to change the base?
Conversation
Thank you very much for the PR! One thing is that this is only for std, while the software renderer on desktop may be used also to preview how it'll look on no_std MCU. We'll have to think of a way to disable that. But regardless, I think it make sense to merge something like this. |
Yes. I couldn't find a good crate for this without std. I thought of converting it to svg and using that to render, as this is already supported, but it seems like this si rasterized at compile time so wont work. But I think it's better to have this supported only on std instead of no support on the software renderer, just configuring it to be enabled and imported only on std envs (I have no idea how to do it but i guess it's possible) |
@ogoffart Hey! I was looking at the tiny-skia cargo file and noticed that it has support fot no_std environments by enabling the no-std-float feat. So this should be able to work on this environment, right? To be clear, I'm pretty new to rust and specially on embedded environments, so am I missing something too? |
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.
Thanks a lot for the PR.
This looks good.
I've added a few comments but they don't need all to be addressed before merging.
I think it is ok to merge it even if it is not fully complete.
All is required is that it passes CI. Ideally it can be disabled, which would be the case if the feature was not enabled by default.
@@ -38,6 +38,8 @@ software-renderer = ["bytemuck"] | |||
image-decoders = ["dep:image", "dep:clru"] | |||
svg = ["dep:resvg", "shared-fontdb"] | |||
|
|||
path = ["dep:zeno"] |
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.
perhaps should be renamed "software-renderer-path" since that doesn't impact other renderer.
@@ -24,7 +24,7 @@ libm = ["num-traits/libm", "euclid/libm"] | |||
# Allow the viewer to query at runtime information about item types | |||
rtti = [] | |||
# Use the standard library | |||
std = ["euclid/std", "once_cell/std", "scoped-tls-hkt", "lyon_path", "lyon_algorithms", "lyon_geom", "lyon_extra", "dep:web-time", "image-decoders", "svg", "raw-window-handle-06?/std", "chrono/std", "chrono/wasmbind", "chrono/clock"] | |||
std = ["euclid/std", "once_cell/std", "scoped-tls-hkt", "lyon_path", "lyon_algorithms", "lyon_geom", "lyon_extra", "dep:web-time", "image-decoders", "svg", "raw-window-handle-06?/std", "chrono/std", "chrono/wasmbind", "chrono/clock", "tiny-skia/std", "path"] |
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 tiny-skia still required?
@@ -2659,6 +2797,71 @@ impl<'a, T: ProcessScene> crate::item_rendering::ItemRenderer for SceneBuilder<' | |||
} | |||
} | |||
|
|||
// impl From<Color> for tiny_skia::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.
some leftover
stroke_brush: path_props.stroke(), | ||
fill_mask, | ||
fill_brush: path_props.fill(), |
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.
Note: the "global" alpha needs to be applied to that color. (That's why other functions do self.alpha_color
let stroke_mask = cmd.stroke_mask.as_ref(); | ||
|
||
let fill_color = cmd.fill_brush.color(); | ||
let fill_color_alpha = fill_color.alpha() as f32 / 255.0; |
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.
We should ideally try to avoid these kind of computation in floating point for every lines. That's why the other commands store the PremultipliedRgbaColor.
let mut zeno_pb: Vec<zeno::Command> = Vec::new(); | ||
let (logical_offset, path_events2) = path.fitted_path_events(item).unwrap(); | ||
|
||
for event in path_events2.iter() { |
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.
I wonder if we can implement zeno::PathData for our own path.
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.
agreed! one thing that is blocking to render paths on no_std is the dependency of lyon_path on other parts.
|
||
let fill_mask = if !path_props.fill().is_transparent() { | ||
let mut mask = Vec::new(); | ||
mask.resize((geometry.size.width * geometry.size.height) as usize, 0u8); |
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.
That seems big.
Internally, zeno renders row by row as well, I wonder if there is a way to do the actual path rendering in draw_zeno_path_line instead. (assuming that the storage needed in the rasterizer is less)
But anyway, there doesn't seem to be API in zeno to do that.
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.
yes, it doesn't seem to be able to render it line by line. at least the command saves two u8 buffers, which is less if it was a rgba buffer. On some occasions, there may be big blank areas, like when full sizing on flex layouts. Zeno provides a way to make the buffer on the needed size, but it would need to limit this somehow (prevent huge buffers with overflow lines) and have a reference point to translate the buffer to the correct location
@@ -0,0 +1,40 @@ | |||
export component MultiplePathTest inherits Window { |
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.
These files will need a copyright header like all other files
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.
Great work!
@@ -1398,6 +1409,16 @@ struct GradientCommand { | |||
bottom_clip: PhysicalLength, | |||
} | |||
|
|||
#[derive(Debug)] | |||
struct ZenoPathCommand { |
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.
(in fact, this is a simplified version of SharedBufferCommand, for an alphamap)
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.
yes. saving as an alpha map reduces the memory size, but makes it heavier on the line drawing function, as gradients need to be calculated there. This shouldn't be a problem as gradient fill on rectangles already does that
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.
I meant that the ZenoPathCommand is basically the same as two SharedBufferCommand { buffer: SharedBufferData::AlphaMap { ... }, ... }
In fact, the draw_texture_line can be re-used, i think (if you create a SceneTexture
)
Thank you for the reviews! I will take some time to make some changes. |
This PR aims to implement path rendering for the software renderer, which closes #4178.
This is still a draft PR starting as a proof-of-concept to validate the work, but I will try to contribute on this feature further along.
Important notes
tiny_skia
to implement the rendering. I've seen the recommendation comment to usezeno
, but I found that this crate does rasterization to a mask, which is not suitable for paths with stroke and fill (at least not easily)TODO