[Impeller] Direct tessellation of simple filled round rects #48919
Conversation
|
Golden file changes have been found for this pull request. Click here to view and triage (e.g. because this is an intentional change). If you are still iterating on this change and are not ready to resolve the images on the Flutter Gold dashboard, consider marking this PR as a draft pull request above. You will still be able to view image results on the dashboard, commenting will be silenced, and the check will not try to resolve itself until marked ready for review. |
jonahwilliams
left a comment
jonahwilliams
left a comment
There was a problem hiding this comment.
LGTM with minor nit.
Have you benchmarked this compared to the current approach for RRects + Skia?
| } else { | ||
| return FilledEllipse(view_transform, bounds); | ||
| } |
There was a problem hiding this comment.
uber-nit
| } else { | |
| return FilledEllipse(view_transform, bounds); | |
| } | |
| } | |
| return FilledEllipse(view_transform, bounds); |
There was a problem hiding this comment.
I'm curious what is the reasoning for the nit.
One advantage of using the else is that it makes sure that the "previous implementation code" did, in fact, return - otherwise the method would lack a return statement.
I just ran into a case of that within this PR where I forgot to return after "doing one version of the work" of a method and so ended up doing "both versions of the work" instead.
There was a problem hiding this comment.
It's basically a case of "do this OR do that".
Also, if it is "do 1 line of code or do 30 lines of code", readability might be improved by
if (condition) {
1 line;
return
};
30 lines;
but in this case both blocks are small.
There was a problem hiding this comment.
I think that is a fair point. Generally I would say that I prefer that we use as little nesting as possible. So if you think of this as an early return, then it makes sense to remove the nesting. As you point out, you could also think about this as being, return one of two options, so it makes sense to leave the else.
Some thoughts on the matter can be seen at https://engdoc.corp.google.com/eng/doc/tott/episodes/651.md?cl=head and https://engdoc.corp.google.com/eng/doc/tott/episodes/680.md?cl=head
I'd say taking either approach is fine, I tend to (overly simplistically probably) push for unnesting.
|
I didn't bother to graph them, but on a quick run of my primitive basher I saw: Skia - 5ms/frame |
| const Matrix& view_transform, | ||
| const Rect& bounds, | ||
| const Size& radii) { | ||
| if (radii.width * 2 < bounds.GetSize().width && |
There was a problem hiding this comment.
This should be || not &&. Reverting.
There was a problem hiding this comment.
Revert wasn't clean so I fixed it instead: #49021
3 participants
Simple round rects with the same width and height at each corner are tessellated directly for fill operations, but not yet for stroke operations.