[Impeller] add support for superellipse. by jonahwilliams · Pull Request #54562 · flutter/engine

jonahwilliams · 2024-08-14T22:32:10Z

This is an entity testing only implementation of a superellipse. rectellipse is a special case where degree = 4.

chinmaygarde · 2024-08-15T17:48:59Z

impeller/entity/geometry/superellipse_geometry.h

+
+  Point center_;
+  // 4 is a rectellipse
+  int degree_;


This needs to be a scalar. Thats how you'd get stuff like astroids in https://en.wikipedia.org/wiki/Superellipse

chinmaygarde

Awesome. We can work with this. Have some nits and the todos already mentioned. But we can land and keep tinkering on this.

flar · 2024-08-15T18:22:25Z

impeller/entity/geometry/superellipse_geometry.cc

+  // quadrants.
+  std::vector<Point> points;
+  points.reserve(41);
+  for (int i = 0; i < 41; i++) {


Nit: I prefer <= 40 in a case like this where 40 is an inclusive target as opposed to for loops < count which are an exclusive target.

flar · 2024-08-15T18:25:11Z

impeller/entity/geometry/superellipse_geometry.cc

+  Scalar n = degree_;
+
+  // TODO(jonahwilliams): determine parameter values based on scaling factor.
+  Scalar step = kPi / 80;


Putting this part in the Tessellator would allow it to leverage the "GetTrigsForDivisions" caching.

I'm going to leave this out for now so its stays separate before we decide how to ship it, but I agree it should eventually live in the tessellator.

Good call if it isolates it until we figure out its future. It will matter a little for benchmarks, but not for testing its usefulness.

chinmaygarde · 2024-08-15T18:33:00Z

For folks trying to run the playground:

Build engine.

et build -c host_debug_unopt_arm64

Run test.

./out/host_debug_unopt_arm64/impeller_unittests --gtest_filter="*DrawSuperEllipse/Metal" --timeout=0 --enable_playground

Superellipse.mov

flar · 2024-08-15T20:11:37Z

impeller/entity/geometry/superellipse_geometry.cc

+    Scalar x = a * pow(abs(cos(t)), 2 / n);
+    Scalar y = b * pow(abs(sin(t)), 2 / n);
+    points.emplace_back(x, y);
+  }


Nit: final points are always computed from points[i] * radius, so just bake radius into the point here.

flar · 2024-08-15T20:12:28Z

impeller/entity/geometry/superellipse_geometry.cc

+  geometry.reserve(1 + 4 * points.size());
+  geometry.push_back(center_);
+  for (auto i = 0u; i < points.size(); i++) {
+    geometry.push_back(center_ + ((reflection[0] * points[i]) * radius_));


Does OOO matter for these equations? ((ref * pt) * rad) vs (ref * (pt * rad)) vs just (ref * pt * rad)?

(Relates to my comment above about just multiplying by radius in the points array...)

No, removed

flar · 2024-08-15T21:34:58Z

If this is just for testing, can the new Entity/Geometry classes be just inlined in the test file for now? They're pretty small.

jonahwilliams · 2024-08-15T21:36:29Z

I think its OK as it is ... and I really don't want to update the license script again 😢

…dd_rectiple

flutter/engine@65fd6ca...68938ab 2024-08-15 jonahwilliams@google.com [Impeller] add support for superellipse. (flutter/engine#54562) 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 rmistry@google.com,zra@google.com 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

) flutter/engine@65fd6ca...68938ab 2024-08-15 jonahwilliams@google.com [Impeller] add support for superellipse. (flutter/engine#54562) 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 rmistry@google.com,zra@google.com 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

[Impeller] add support for rectellipse.

9ac0765

jonahwilliams requested a review from chinmaygarde August 14, 2024 22:32

github-actions bot added the e: impeller label Aug 14, 2024

full configuration.

ffddf05

jonahwilliams changed the title ~~[Impeller] add support for rectellipse.~~ [Impeller] add support for superellipse. Aug 15, 2024

jonahwilliams added 4 commits August 14, 2024 17:55

linting.

190dfc8

quick fix

ded4ae8

move to entity tests.

0d161ad

add doc comment.

e369328

chinmaygarde reviewed Aug 15, 2024

View reviewed changes

chinmaygarde approved these changes Aug 15, 2024

View reviewed changes

jonahwilliams requested a review from flar August 15, 2024 18:17

Jonah Williams added 2 commits August 15, 2024 11:18

Update superellipse_geometry.h

2e9d74b

Update superellipse_geometry.cc

4cc92e0

flar reviewed Aug 15, 2024

View reviewed changes

Update superellipse_geometry.cc

a037a7d

chinmaygarde mentioned this pull request Aug 15, 2024

[Impeller] Proposal: Support rendering analytically clean "squircles" and "rectellipses". flutter/flutter#139321

Closed

jonahwilliams added the autosubmit Merge PR when tree becomes green via auto submit App label Aug 15, 2024

flar reviewed Aug 15, 2024

View reviewed changes

jonahwilliams removed the autosubmit Merge PR when tree becomes green via auto submit App label Aug 15, 2024

inline radius.

e014ce9

Merge branch 'add_rectiple' of github.com:jonahwilliams/engine into a…

d684295

…dd_rectiple

jonahwilliams added the autosubmit Merge PR when tree becomes green via auto submit App label Aug 15, 2024

auto-submit bot merged commit 68938ab into flutter:main Aug 15, 2024

engine-flutter-autoroll mentioned this pull request Aug 16, 2024

Roll Flutter Engine from 65fd6ca194c1 to 68938abd03b9 (1 revision) flutter/flutter#153533

Merged

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

68938ab [Impeller] add support for superellipse. (flutter/engine#54562)

fb9423e

dkwingsmt mentioned this pull request Nov 26, 2024

[Impeller] Add rounded superellipse #56726

Merged

8 tasks

Comments

Conversation

jonahwilliams commented Aug 14, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chinmaygarde 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!

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!

chinmaygarde commented Aug 15, 2024

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 Aug 15, 2024

Uh oh!

jonahwilliams commented Aug 15, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

jonahwilliams commented Aug 14, 2024 •

edited

Loading