[Impeller] New impeller round rect class #55946
Conversation
| @@ -0,0 +1,212 @@ | |||
| // Copyright 2013 The Flutter Authors. All rights reserved. | |||
There was a problem hiding this comment.
I'm not sure what method we'll use for detection of SimpleRRects, but just a note that we should tolerate floating point errors on it.
There was a problem hiding this comment.
The rendering code already has its own NearlySimple method that I thought should be fine as that is a rendering issue. I originally had a method here that was similar, but then I decided to leave that to the caller since it wasn't clear what level of epsilon to use (and eventually we should be taking the matrix into account for any epsilon and that would complicate the methods even more).
There was a problem hiding this comment.
As mentioned down below, rather than have the rendering code do its own thing, the RoundRect method now applies a tolerance which can be supplied by the caller. We should supply a transform-based tolerance for rendering code and eventually we have to figure out what to do about all of these transform-dependent tolerances in the DL code that doesn't have access to the final transform.
|
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. |
|
I tried this out with the interactive test from #55929 to see if the dimples show up with different parameters, I don't see them. There is one bug with this thought that when the width is 0 nothing is drawn, it winks out of existence. I expect it to look almost exactly like width 0.01 edit: Before this change a rect was drawn, which isn't correct either but is better than nothing. |
| GetCanvas().DrawRRect(skia_conversions::ToRect(rrect.rect()), | ||
| skia_conversions::ToSize(rrect.getSimpleRadii()), | ||
| paint_); | ||
| if (rrect.GetRadii().AreAllSame()) { |
There was a problem hiding this comment.
This needs to use the IsNearlySimpleRRect rrect that has a tolerance for floating point errors
There was a problem hiding this comment.
I changed the method to apply a (configurable) tolerance so it should behave as before wherever it is used.
flar
force-pushed
the
impeller-round-rect-class
branch
from
7cbf4ca
to
680af3c
Compare
I'll take a look at this, but one of the changes I made in the last commit was that I had been replacing the rect with a default rect if it was empty. That would account for what you saw in that the rect might have had a width or height and it would lose both of the other was empty. Now I just transfer the rect along, but set the radii to all 0s which is what would happen when they were normalized anyway. (Confirmed, it now goes to a rectangle due to the loss of corner radii) |
There was a problem hiding this comment.
LGTM!
Slightly related:
One problem with stroked or non-simple round rects is that we will re-convert them to paths on every single dispatch, whereas if they had been recorded in the display list as a path we'd only do that conversion once. Similar for the difference rrect, where we do the conversion on each frame.
I don't think it would be harmful to the skia backend if we eagerly did the path conversion during recording, but its also probably out of scope for this chage.
| @@ -115,8 +115,8 @@ namespace flutter { | |||
| V(DrawRect) \ | |||
| V(DrawOval) \ | |||
| V(DrawCircle) \ | |||
| V(DrawRRect) \ | |||
| V(DrawDRRect) \ | |||
| V(DrawRoundRect) \ | |||
| @@ -0,0 +1,212 @@ | |||
| // Copyright 2013 The Flutter Authors. All rights reserved. | |||
It caused 11k scuba diffs and the diffs were visible to the naked eye. Their path rendering does not match their primitive rendering. See #55376 and #55410 which were closed until we stop testing with Skia... |
Impeller geometry class analogous to SkRRect - soon to be the standard round rectangle class throughout most of the engine that doesn't talk directly to Skia.
More unit tests are being written, but this PR will enable developers to look at the API of the new class and comment as well as test its effect on any goldens (there should be none).