-
Notifications
You must be signed in to change notification settings - Fork 603
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
Support dithering gradients in the skia renderer #5482
Support dithering gradients in the skia renderer #5482
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.
Thanks a lot for the patch!
The logic looks fine to me as far as i can see. Just made some style comments
@@ -117,6 +128,7 @@ impl<'a> SkiaItemRenderer<'a> { | |||
} | |||
_ => None, | |||
} | |||
.and_then(|shader| Some(func(paint, Some(shader)))) |
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.
If i'm not mistaken, this could be written
.and_then(|shader| Some(func(paint, Some(shader)))) | |
.map(|shader| func(paint, Some(shader))) |
brush: Brush, | ||
width: PhysicalLength, | ||
height: PhysicalLength, | ||
) -> Option<skia_safe::shader::Shader> { | ||
func: impl FnOnce(skia_safe::Paint, Option<skia_safe::Shader>) -> T, |
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.
This is a bit of a strangle pattern.
Why not having a function with this signature: brush_to_shader(...) -> Option(Paint, Shader)
instead of taking a callback?
Then it can be used like so:
let (mut paint, shader) = Self::brush_to_shader(...)?;
Anyway it seems that the Shader cannot be None when the function is called, so no need to take an Option
as argument.
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.
Just personal preference, I felt like colorize_image
was 2% clearer with the closure argument over a tuple return value, but I've been living mostly outside of rust lately and that was probably my being overly accustomed to foreign idioms.
Adjusted to match your feedback.
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.
Edit: [disregard. you were right, this is better. fixup commit added]
Re: taking an Option
as an argument, brush_to_paint()
is more frequently referenced/called in the source and operates on an Option<shader>
, so the cruft in colorize_ image()
, to me, was simply picking favorites between the two callers (even though colorize suffers more than to_paint benefits). Happy to knock a ?
from one and add a Some
to the other if you find that preferable.
dd50a43
to
c7ddb4d
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.
Thanks!
Notes
This overcomplicates
brush_to_shader()
a bit more than necessary to avoid duplicating two lines of code, but it made sense to me to pair shader-specific paint settings with shader-instantiation. One consideration is thatbrush_to_paint()
already exists and has several callers in the renderer, but produces aPaint
object that isn't compatible withcolorize_image()
.brush_to_shader()
only has these two callers(one beingbrush_to_paint()
, the other beingcolorize_image()
so the complexity is mostly tucked away from the rest of the module.Also, when trying this out with the energy monitor example, it seems to have traded banding in the background radial gradient for jitter in the background radial gradient. This doesn't make sense to me because dithering in skia is deterministic, but still seems like an improvement to me, and doesn't seem to affect linear gradients. Unsure if that's something idiosyncratic to the energy monitor, or to radial gradients.