[Impeller] Make rounded superellipse a style of `RoundRect` #162349

dkwingsmt · 2025-01-28T23:17:43Z

This PR make RoundRect support two styles, kCircular for the traditional RRect and kContinuous for the Apple squircle.

I've experimented with two ways, a) to add RSE as a new drawing op (prototype PR), and b) to add RSE as a style of RRect (this PR). Unsurprisingly the latter option requires much, much less code (for more functionality). It also makes sense since the two kinds of shapes look quite similar, and specifying them with styles is also how the SwiftUI API handles them (which inspired the names of the two styles.)

The major obstacle of this change is to implement RoundRect::Contains(). Current code takes a lot of computation to expand the input parameters (bounds, radii) to the detailed variables (e.g. detailed parameters for each arc segments). To reuse the algorithm, the code to expand the parameters has been extracted to a new class called RoundSuperellipseParam, which is used by both the containment check and the drawing.

Pre-launch Checklist

I read the Contributor Guide and followed the process outlined there for submitting PRs.
I read the Tree Hygiene wiki page, which explains my responsibilities.
I read and followed the Flutter Style Guide, including Features we expect every widget to implement.
I signed the CLA.
I listed at least one issue that this PR fixes in the description above.
I updated/added relevant documentation (doc comments with ///).
I added new tests to check the change I am making, or this PR is test-exempt.
I followed the breaking change policy and added Data Driven Fixes where supported.
All existing and new tests are passing.

If you need help, consider asking for advice on the #hackers-new channel on Discord.

jonahwilliams

I think make this an additional configuration of the RRect makes sense to me, but I'd also like @flar to take a look to see if there are any gotchas I'm over looking.

jonahwilliams · 2025-01-30T02:56:38Z

engine/src/flutter/impeller/display_list/canvas.cc

  auto& radii = round_rect.GetRadii();
+  if (round_rect.GetStyle() == RoundRect::Style::kContinuous) {
+    // The continuous style only supports filling.
+    if (paint.style != Paint::Style::kFill) {


We generally ignore paint properties that aren't supported. So rather than early return perhaps just ignore the paint.style?

I'd argue that the DrawRoundRect operation should be consistent with what it does with the attributes. If we can't stroke a continuous superellipse then we should have it be its own operation even if it carries much the same data.

Also, on the subject of "should we use the same structure?" - I'd have to dig more into the math of the super-ellipse. I had thought it was basically an ellipse with a different power function, but how does that map onto the width and height of the rounded corners of the round rect? Do we need 8 parameters to define a super-ellipse?

I'd argue that the DrawRoundRect operation should be consistent with what it does with the attributes.

I agree and I'm researching for a good way to implement stroking with enough accuracy. May we leave that to a future PR? I can open an issue to track that.

I had thought it was basically an ellipse with a different power function, but how does that map onto the width and height of the rounded corners of the round rect?

You're totally right about superellipses, but this one is a rounded superellipse - replacing the "corners" or superellipses with circular arcs, so that it has a constant curvature at the corner that smoothly transits to a straight line going out. Strange might it sound, we found that this is the exact shape of Apple's squircles, and you can find more details about the researching process and maths in this design doc.

Do we need 8 parameters to define a super-ellipse?

A superellipse is only defined by its semi-axes and degree (n). A rounded superellipse, at its primitive definition, is also only defined by its bounds and corner_radius, a scalar. However Apple also supports "asymmetrical radii" - different radii for X and Y axes - as well as "uneven radii" - different radii for the four corners. Collectively they're exactly Flutter's RoundingRadii (which is why combining the two kinds of shapes is possible in the first place). How are they drawn? Well, by concatenating arcs of different radii and scaling the shape. And that's why the final class uses 8 radii. You can watch how we evolved to the current state by watching the videos in the following two PRs:

This PR added support for the primitive rounded superellipses, i.e. 1 radius.

This PR added support for full RoundingRadii, i.e. 8 radii.

You may also notice that the shapes in the videos are not superellipses at all, and if you watch closely, they are also slightly different from classical RRects by having a smoother curvature transition.

I'd argue that the DrawRoundRect operation should be consistent with what it does with the attributes.

I agree and I'm researching for a good way to implement stroking with enough accuracy. May we leave that to a future PR? I can open an issue to track that.

Only if you are 100% sure that you can solve the stroking issue.

I had thought it was basically an ellipse with a different power function, but how does that map onto the width and height of the rounded corners of the round rect?

You're totally right about superellipses, but this one is a rounded superellipse - replacing the "corners" or superellipses with circular arcs, so that it has a constant curvature at the corner that smoothly transits to a straight line going out. Strange might it sound, we found that this is the exact shape of Apple's squircles, and you can find more details about the researching process and maths in this design doc.

I scanned much of it, but the important distinction is that this is just a new way to do the math with the same data.

Another point to consider is that there are places in the code where we do "insideness" computations on these and so that code would have to be updated.

How about redirecting to DrawPath for the stroke case?

How about redirecting to DrawPath for the stroke case?

Yeah stroking is definitely going to be done with Bezier paths.

I scanned much of it, but the important distinction is that this is just a new way to do the math with the same data.
Another point to consider is that there are places in the code where we do "insideness" computations on these and so that code would have to be updated.

I don't understand neither of the sentences. Can you explain more?

Also, I'd be very happy to introduce the maths and other details over a video call if it helps with understanding and reviewing!

How about redirecting to DrawPath for the stroke case?

Yeah stroking is definitely going to be done with Bezier paths.

So why is it not implemented here? I thought the conversion to Bezier's was already established technology that we just need to implement (basically copy/paste into a helper function) here.

I scanned much of it, but the important distinction is that this is just a new way to do the math with the same data.
Another point to consider is that there are places in the code where we do "insideness" computations on these and so that code would have to be updated.

I don't understand neither of the sentences. Can you explain more?

First sentence just acknowledges that the document you provided was a good description of what kind of data this new variant of rounded rectangles needed and that it was using the same inputs as the existing definition, even if I didn't really study the math involved.

Second sentence is a reminder that the objects are not just involved in rendering, but that there is also code in the engine that does things like take their bounds (not an issue because the bounds considerations are the same between the variants) and compute insideness of points within the round rect (as used by the clip/transform classes that you had already spotted and modified).

Also, I'd be very happy to introduce the maths and other details over a video call if it helps with understanding and reviewing!

I don't need to do a deep dive into exactly how the math works, I'm working out whether or not using the same input data for both elliptical and continuous rounded rects makes sense. And that document helped make that point whether or not I fully understand the math.

So why is it not implemented here? I thought the conversion to Bezier's was already established technology that we just need to implement (basically copy/paste into a helper function) here.

Although approximating circular arcs with Bezier curves is established technology, approximating superellipsoidal arcs is not (as far as I know).

Anyway I've just computed these params numerically and I'll implement them in this PR and let you know.

jonahwilliams · 2025-01-31T18:14:13Z

Lets leave the stroke part of the implementation to a future change.

Another thought on combining them: On the Dart Side you likely cannot combine these objects. Because Path can accept an RRect (https://api.flutter.dev/flutter/dart-ui/Path/addRRect.html) but we will not be able to implement that API, as it requires that the shape is expressable via bezier curves.

dkwingsmt · 2025-01-31T19:04:06Z

Ok, yesterday I have derived the algorithm to express rounded rectangles as Bezier paths. I'll implement it in a prototype PR for verification.

jonahwilliams · 2025-01-31T19:07:28Z

@dkwingsmt if you have an algorithm to express via bezier, then we don't need any engine changes. That could be entirely done in the framework.

dkwingsmt · 2025-01-31T19:22:10Z

I doubt it because previously the Apple squircle has been implemented using Bezier's curves using Figma's algorithm, but the performance turns out too poor to be acceptable https://github.com/rydmike/squircle_study

jonahwilliams · 2025-01-31T19:24:47Z

ahh kk. For now I would suggest making Rrect a different type. The Dart:ui path is still using the Skia path and converting to Impeller afterwards, so we can't express new shapes in it yet.

dkwingsmt · 2025-01-31T19:31:35Z

to confirm you're talking about the next step, right?

Also, if we make it this way, how will platforms detect if rounded super ellipses are supported? My idea would be to make it fall back to typical rounded rectangles in the engine. This way, this fallback is implementation details and a code that wants to use the new style can at least have something rendered, a classic rounded rectangle that they have been rendering anyway.

jonahwilliams · 2025-01-31T19:43:13Z

to confirm you're talking about the next step, right?

For the dart:ui object yes. It doesn't really matter if the backend object is separate or the same.

how will platforms detect if rounded super ellipses are supported?

Why would you need to? If you add a bezier approximation in the skia dispatcher it should just work (tm) right?

dkwingsmt · 2025-01-31T20:52:26Z

Ah I see you're right. Thank you!

dkwingsmt · 2025-01-31T20:53:51Z

Let me see if @flar has more comments.

flar · 2025-01-31T23:20:27Z

engine/src/flutter/display_list/dl_op_records.h

 // DlRect is 16 more bytes, which packs efficiently into 24 bytes total
-// DlRoundRect is 48 more bytes, which rounds up to 48 bytes
-//         which packs into 56 bytes total
+// DlRoundRect is 52 more bytes, which packs into 60 bytes total


These structs round up to a multiple of 8 bytes I believe which means that change will cause more padding to occur. Look around for a size comment that talks about "rounding up" and borrow that wording.

I anticipated 8-byte alignment too but I got these numbers by printf the sizeof (on macOS). Could this be platform specific?

The struct is not 8-byte padded, but they are installed in the buffer on 8-byte alignments (see the templated Push method at the top of the file).

Also, there are 2 different ops that embed this object - Clip and Draw? And DrawDDRect maybe? Scan the file for other uses.

As a query tool - instead of printf, I sometimes use "static_assert(sizeof(foo) == N)" and then adjust N until my IDE tells me it isn't failing. Then I know the size. I also use this to plan how to lay out the fields so it takes fewer byes if it can. I don't think there is any leeway here to achieve that

And yes, sometimes it can be different on different platforms, but I've only seen that happen so far with vectors and shared_ptrs which have different layouts on Windows.

And there is a practical reason for predicting the sizes of the objects - there is a test that makes a bunch of different DisplayLists and verifies that they concatenate to the expected size (i.e. testing alignment and such). Look in dl_test_snippets.cc for a bunch of "Invoker"s in a giant array that list the expected DL buffer sizes for each item it defines. You'll likely just have to add the delta to anything that uses RoundRect (and twice for the DRRect version).

The easiest way to debug this is out/<engine variant>/display_list_unittests --gtest_filter="DisplayListTest.SingleOpSizes" which will test each entry individually for appropriate sizes and print which entry in the giant array of test data is off. Once that passes, all of the combinatorial tests should also pass.

flar · 2025-01-31T23:21:47Z

engine/src/flutter/display_list/utils/dl_matrix_clip_tracker.cc

-  for (auto corner : corners) {
-    if (!outer.Contains(corner)) {
-      return false;
+  if (content.GetStyle() == impeller::RoundRect::Style::kContinuous) {


I'm glad you spotted this and addressed it. I'll review the math later, but I was worried this would get missed.

flar · 2025-01-31T23:25:44Z

I thought I saw a comment in passing about adding these to paths. Currently the ui.Path is implemented on top of SkPath, but eventually we will switch that to impeller::Path[Builder] at which point we could actually support implementing a "super-continuous-corner" path element when a RoundRect is added to maintain precision down to the rasterization level.

jonahwilliams · 2025-01-31T23:44:45Z

at which point we could actually support implementing a "super-continuous-corner" path element when a RoundRect is added to maintain precision down to the rasterization level

I do not want us to do that, because I'm concerned it will increase the complexity of the already fiddly+slow path bits of the engine.

flar · 2025-01-31T23:54:06Z

at which point we could actually support implementing a "super-continuous-corner" path element when a RoundRect is added to maintain precision down to the rasterization level

I do not want us to do that, because I'm concerned it will increase the complexity of the already fiddly+slow path bits of the engine.

The 2 issues that I'm concerned with is that it might mean that drawing a path would look different than drawing the primitive and that the rendering of the bezier decomposition is already considered slow anyway...?

We could, ahem, "Fiddle Around and Find Out". But that's a future task anyway.

jonahwilliams · 2025-01-31T23:57:35Z

The 2 issues that I'm concerned with is that it might mean that drawing a path would look different than drawing the primitive

Right, I agree and I don't want us to have this problem - which is why on the dart:ui side of things I'd like us to keep the rrect types separate so that there is no Dart Path::AddRRect API we need to worry about. But I think we might have that problem on this side of the API too right? We have a PathBuilder::AddRRect that we'd need to deal with.

I think that implies we need to fully separate out the regular rrect from the squircle rect.

flar · 2025-02-01T00:02:07Z

I'm happy with keeping them separate.

One thing I'm less sure of is that there are Widgets that might expect something to be expressed as a Path. Am I misremembering that?

flar · 2025-02-01T00:37:51Z

Is there a reason for adding this if we don't even have a way to call it from Dart yet?

dkwingsmt · 2025-02-01T00:40:18Z

Is there a reason for adding this if we don't even have a way to call it from Dart yet?

Just to separate the changes into smaller chunks for easier reviews and development. I've had the Dart changes ready (might need better docs or tests), all the way to applying to this shape to the desired widgets. I'm opening these PRs right after.

engine/src/flutter/display_list/dl_op_records.h

outdated

jonahwilliams · 2025-02-05T18:36:00Z

To restate our discussion from yesterday:

RRect and "Apple Squircle" should be different primitives on both engine and dart side.
Dart and native engine changes can be separate PRs
Stroking / Skia support can be separate PR
Stroking / Skia support can be done with Bezier approximation.

@dkwingsmt Please update this PR to separate the Rect and "Apple Squircle" classes

dkwingsmt · 2025-02-05T18:39:39Z

@jonahwilliams Separating the engine side would mean something like this PR #160883, is it right?

jonahwilliams · 2025-02-05T18:43:02Z

Yes, that is the way I'd do it.

dkwingsmt · 2025-02-05T18:46:14Z

I'm closing this PR and moving on to finalize #160883.

#162826) This PR: * Adds a `RoundSuperellipse` class, which mirrors the current `RoundRect` class. * Implements `RoundSuperellipse::Contains`, which checks if a point is contained by the RSE. * Adds `Path::AddSuperellipse`, which draws an RSE with Bezier approximation. * Adds a `RoundSuperellipseParam` class, which is the common computation shared by geometry drawing, stroking, and containment check. https://github.com/user-attachments/assets/883c7762-7b35-432e-9b31-d204db3bd6e1 This PR also updates the RSE algorithm according to my recent research, which uses one fewer precomputed variable (no more `d`), shares the same gap factor with RRect, and allows much better precision. The result shape is almost unchanged (~0.2% slimmer). > For reviewers: The `RoundSuperellipseParam` and `RoundSuperellipse::Contains` parts are repurposed from the abandoned #162349. This PR is a preparation for #160883. ## Pre-launch Checklist - [ ] I read the [Contributor Guide] and followed the process outlined there for submitting PRs. - [ ] I read the [Tree Hygiene] wiki page, which explains my responsibilities. - [ ] I read and followed the [Flutter Style Guide], including [Features we expect every widget to implement]. - [ ] I signed the [CLA]. - [ ] I listed at least one issue that this PR fixes in the description above. - [ ] I updated/added relevant documentation (doc comments with `///`). - [ ] I added new tests to check the change I am making, or this PR is [test-exempt]. - [ ] I followed the [breaking change policy] and added [Data Driven Fixes] where supported. - [ ] All existing and new tests are passing. If you need help, consider asking for advice on the #hackers-new channel on [Discord].  [Contributor Guide]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#overview [Tree Hygiene]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md [test-exempt]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#tests [Flutter Style Guide]: https://github.com/flutter/flutter/blob/main/docs/contributing/Style-guide-for-Flutter-repo.md [Features we expect every widget to implement]: https://github.com/flutter/flutter/blob/main/docs/contributing/Style-guide-for-Flutter-repo.md#features-we-expect-every-widget-to-implement [CLA]: https://cla.developers.google.com/ [flutter/tests]: https://github.com/flutter/tests [breaking change policy]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#handling-breaking-changes [Discord]: https://github.com/flutter/flutter/blob/main/docs/contributing/Chat.md [Data Driven Fixes]: https://github.com/flutter/flutter/blob/main/docs/contributing/Data-driven-Fixes.md

dkwingsmt added 12 commits January 18, 2025 18:31

extract param

0ee123e

Works

f74dff3

Merge remote-tracking branch 'origin/main' into rse-api2

15402c6

Some doc

5ee1661

Half size, and better norm algo

61b50b2

MakeSizeRadiusForTopRight

4a58fc4

.all_corners_same

1266458

Format

469f4db

Contains

ff35bc6

Merge remote-tracking branch 'origin/main' into rse-api2

ab3cb1e

Add style and contain check

018364a

Tests

e66d9cc

github-actions bot added engine flutter/engine related. See also e: labels. e: impeller Impeller rendering backend issues and features requests labels Jan 28, 2025

dkwingsmt added 4 commits January 29, 2025 17:16

C APIs

f46637e

Format

3f40875

Fix

e822c7d

Fix

2241bde

dkwingsmt marked this pull request as ready for review January 30, 2025 01:42

Merge remote-tracking branch 'origin/main' into rse-api2

617070c

dkwingsmt requested a review from jonahwilliams January 30, 2025 01:43

jonahwilliams previously approved these changes Jan 30, 2025

View reviewed changes

jonahwilliams requested a review from flar January 30, 2025 02:59

flar reviewed Jan 31, 2025

View reviewed changes

dkwingsmt added 2 commits February 4, 2025 14:37

Fix op size

c0ba9b9

Merge remote-tracking branch 'origin/main' into rse-api2

fdac2c1

jonahwilliams self-requested a review February 4, 2025 22:40

flar reviewed Feb 5, 2025

View reviewed changes

engine/src/flutter/display_list/dl_op_records.h Outdated Show resolved Hide resolved

flar reviewed Feb 5, 2025

View reviewed changes

engine/src/flutter/display_list/dl_op_records.h Outdated Show resolved Hide resolved

flar reviewed Feb 5, 2025

View reviewed changes

engine/src/flutter/display_list/dl_op_records.h Outdated Show resolved Hide resolved

dkwingsmt and others added 4 commits February 4, 2025 17:54

Remove static_assert and fix license

35ced0d

Fix doc

5db7bb6

Fix doc

a1302e1

Merge branch 'master' into rse-api2

4c07c79

dkwingsmt closed this Feb 5, 2025

dkwingsmt mentioned this pull request Feb 11, 2025

[Impeller] Add RoundSuperellipse class, containment check and stroking #162826

Merged

9 tasks

[Impeller] Make rounded superellipse a style of RoundRect #162349

[Impeller] Make rounded superellipse a style of RoundRect #162349

Uh oh!

Conversation

dkwingsmt commented Jan 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pre-launch Checklist

Uh oh!

jonahwilliams left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dkwingsmt Jan 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dkwingsmt Feb 1, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

flar Feb 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dkwingsmt Feb 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jonahwilliams commented Jan 31, 2025

Uh oh!

dkwingsmt commented Jan 31, 2025

Uh oh!

jonahwilliams commented Jan 31, 2025

Uh oh!

dkwingsmt commented Jan 31, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jonahwilliams commented Jan 31, 2025

Uh oh!

dkwingsmt commented Jan 31, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jonahwilliams commented Jan 31, 2025

Uh oh!

dkwingsmt commented Jan 31, 2025

Uh oh!

dkwingsmt commented Jan 31, 2025

Uh oh!

flar Jan 31, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

flar commented Jan 31, 2025

Uh oh!

jonahwilliams commented Jan 31, 2025

Uh oh!

flar commented Jan 31, 2025

Uh oh!

jonahwilliams commented Jan 31, 2025

Uh oh!

flar commented Feb 1, 2025

Uh oh!

flar commented Feb 1, 2025

Uh oh!

dkwingsmt commented Feb 1, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

[Impeller] Make rounded superellipse a style of `RoundRect` #162349

[Impeller] Make rounded superellipse a style of `RoundRect` #162349

dkwingsmt commented Jan 28, 2025 •

edited

Loading

dkwingsmt Jan 30, 2025 •

edited

Loading

dkwingsmt Feb 1, 2025 •

edited

Loading

flar Feb 3, 2025 •

edited

Loading

dkwingsmt Feb 3, 2025 •

edited

Loading

dkwingsmt commented Jan 31, 2025 •

edited

Loading

dkwingsmt commented Jan 31, 2025 •

edited

Loading

flar Jan 31, 2025 •

edited

Loading

dkwingsmt commented Feb 1, 2025 •

edited

Loading

dkwingsmt commented Feb 5, 2025 •

edited

Loading