[Impeller] New impeller round rect class #55946

flar · 2024-10-18T05:05:37Z

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).

jonahwilliams · 2024-10-18T16:24:51Z

impeller/geometry/round_rect.h

@@ -0,0 +1,212 @@
+// Copyright 2013 The Flutter Authors. All rights reserved.


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.

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).

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.

I like this approach!

flutter-dashboard · 2024-10-18T17:14:05Z

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.

Changes reported for pull request #55946 at sha 7cbf4ca

gaaclarke · 2024-10-18T18:14:23Z

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.

jonahwilliams · 2024-10-18T18:39:08Z

impeller/display_list/dl_dispatcher.cc

-    GetCanvas().DrawRRect(skia_conversions::ToRect(rrect.rect()),
-                          skia_conversions::ToSize(rrect.getSimpleRadii()),
-                          paint_);
+  if (rrect.GetRadii().AreAllSame()) {


This needs to use the IsNearlySimpleRRect rrect that has a tolerance for floating point errors

I changed the method to apply a (configurable) tolerance so it should behave as before wherever it is used.

flar · 2024-10-19T00:03:57Z

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.

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)

jonahwilliams

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.

jonahwilliams · 2024-10-19T16:20:08Z

display_list/display_list.h

@@ -115,8 +115,8 @@ namespace flutter {
  V(DrawRect)                       \
  V(DrawOval)                       \
  V(DrawCircle)                     \
-  V(DrawRRect)                      \
-  V(DrawDRRect)                     \
+  V(DrawRoundRect)                  \


jonahwilliams · 2024-10-19T16:25:54Z

impeller/geometry/round_rect.h

@@ -0,0 +1,212 @@
+// Copyright 2013 The Flutter Authors. All rights reserved.


I like this approach!

flar · 2024-10-20T18:32:15Z

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.

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...

)

…157246) flutter/engine@53e187a...68f9225 2024-10-20 [email protected] [Impeller] New impeller round rect class (flutter/engine#55946) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-engine-flutter-autoroll Please CC [email protected],[email protected] on the revert to ensure that a human is aware of the problem. To file a bug in Flutter: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://issues.skia.org/issues/new?component=1389291&template=1850622 Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md

jonahwilliams · 2024-10-21T18:08:26Z

There was some benchmark movement on this PR, I think positive?

Down

https://flutter-engine-perf.skia.org/e/?begin=1729290271&end=1729532343&keys=X5a168f61690cd0d2547ea2a1dd55b365&num_commits=50&request_type=1&xbaroffset=43324

Up

https://flutter-engine-perf.skia.org/e/?begin=1729290271&end=1729532343&keys=X3075c2ba7cb934cce6a4d765b7060982&num_commits=50&request_type=1&xbaroffset=43324

flar · 2024-10-21T18:40:32Z

There was some benchmark movement on this PR, I think positive?

Down

https://flutter-engine-perf.skia.org/e/?begin=1729290271&end=1729532343&keys=X5a168f61690cd0d2547ea2a1dd55b365&num_commits=50&request_type=1&xbaroffset=43324

Up

https://flutter-engine-perf.skia.org/e/?begin=1729290271&end=1729532343&keys=X3075c2ba7cb934cce6a4d765b7060982&num_commits=50&request_type=1&xbaroffset=43324

Those are all (mostly? I didn't check each one) Region benchmarks which should not have been affected by the RoundRect work at all. I saw a similar change in these benchmarks a few weeks back on another completely unrelated PR. It's a mystery.

I zoomed out and scrolled back a bit and you can see where the test was changing performance a few times:

https://flutter-engine-perf.skia.org/e/?begin=1718243397&end=1729532342&keys=X5a168f61690cd0d2547ea2a1dd55b365&requestType=0&selected=commit%3D42649%26name%3D%252Cconfig%253Ddefault%252Ccpu_scaling_enabled%253Dfalse%252Cexecutable%253D_b_s_w_ir_cache_builder_src_out_ci_host_release_benchmarks_display_list_region_benchmarks%252Clibrary_build_type%253Drelease%252Cmhz_per_cpu%253D2200%252Cnum_cpus%253D8%252Csub_result%253Dreal_time%252Ctest%253DBM_SkRegion_FromRects_Tiny%252Cunit%253Dus%252C&xbaroffset=43324

and

https://flutter-engine-perf.skia.org/e/?begin=1705971152&end=1729524674&keys=X5a168f61690cd0d2547ea2a1dd55b365&requestType=0&selected=commit%3D40883%26name%3D%252Cconfig%253Ddefault%252Ccpu_scaling_enabled%253Dfalse%252Cexecutable%253D_b_s_w_ir_cache_builder_src_out_ci_host_release_benchmarks_display_list_region_benchmarks%252Clibrary_build_type%253Drelease%252Cmhz_per_cpu%253D2200%252Cnum_cpus%253D8%252Csub_result%253Dcpu_time%252Ctest%253DBM_SkRegion_FromRects_Small%252Cunit%253Dus%252C&xbaroffset=43324

It looks bimodal, but it locks into one or the other measurements for many runs in a row triggered by seemingly random changes.

github-actions bot added the e: impeller label Oct 18, 2024

jonahwilliams reviewed Oct 18, 2024

View reviewed changes

flutter-dashboard bot added the will affect goldens label Oct 18, 2024

jonahwilliams reviewed Oct 18, 2024

View reviewed changes

flar added 3 commits October 18, 2024 16:51

New impeller round rect class

253da5f

more unit tests

7b57bf6

finish off unit tests

680af3c

flar force-pushed the impeller-round-rect-class branch from 7cbf4ca to 680af3c Compare October 18, 2024 23:59

flar requested a review from jonahwilliams October 19, 2024 00:16

jonahwilliams approved these changes Oct 19, 2024

View reviewed changes

flar added the autosubmit Merge PR when tree becomes green via auto submit App label Oct 20, 2024

auto-submit bot merged commit 68f9225 into flutter:main Oct 20, 2024
32 checks passed

engine-flutter-autoroll mentioned this pull request Oct 20, 2024

Roll Flutter Engine from 53e187a1d4e4 to 68f922560038 (1 revision) flutter/flutter#157246

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Oct 20, 2024

68f922560 [Impeller] New impeller round rect class (flutter/engine#55946

28c44ff

)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Impeller] New impeller round rect class #55946

[Impeller] New impeller round rect class #55946

flar commented Oct 18, 2024

jonahwilliams Oct 18, 2024

flar Oct 18, 2024

flar Oct 19, 2024

jonahwilliams Oct 19, 2024

flutter-dashboard bot commented Oct 18, 2024

gaaclarke commented Oct 18, 2024 •

edited

Loading

jonahwilliams Oct 18, 2024

flar Oct 18, 2024

flar commented Oct 19, 2024 •

edited

Loading

jonahwilliams left a comment

jonahwilliams Oct 19, 2024

jonahwilliams Oct 19, 2024

flar commented Oct 20, 2024

jonahwilliams commented Oct 21, 2024

flar commented Oct 21, 2024

Down

Up

		@@ -0,0 +1,212 @@
		// Copyright 2013 The Flutter Authors. All rights reserved.

[Impeller] New impeller round rect class #55946

[Impeller] New impeller round rect class #55946

Conversation

flar commented Oct 18, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

flutter-dashboard bot commented Oct 18, 2024

gaaclarke commented Oct 18, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

flar commented Oct 19, 2024 • edited Loading

jonahwilliams left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

flar commented Oct 20, 2024

jonahwilliams commented Oct 21, 2024

Down

Up

flar commented Oct 21, 2024

Down

Up

gaaclarke commented Oct 18, 2024 •

edited

Loading

flar commented Oct 19, 2024 •

edited

Loading