UberSDFContent refactoring and handle stroke miter limit for stroked rects by b-luk · Pull Request #184603 · flutter/flutter

b-luk · 2026-04-04T00:43:00Z

Creates a new UberSDFParameters struct, which is encapsulates all the state needed for a UberSDF FragInfo. It has shape-specific constructors to populate this state for different shapes.

Creates a new UberSDFGeometry class to be used as the Geometry for UberSDFContents. It contains all the AA padding logic for UberSDF, so the AA padding can be removed from canvas and from FillRectGeometry. UberSDFGeometry's GetPositionBuffer leverages a FillRectGeometry to return a quad that properly accounts for AA padding.

UberSDFContents is updated to be constructed with UberSDFParameters rather than having shape-specific constructors. It becomes agnostic to the specific shape being drawn, and now has no shape- or geometry-aware logic. It simply pipes through UberSDFParameters values to the UberSDF shader's FragInfo.

This is mostly a no-op refactoring. The exception is for UberSDF stroke rects with a miter limit. UberSDFParameters properly handles miter limit for these, so now stroked rects with low miter limits properly become beveled.

Part of #184402
Fixes #184404
Part of #184352

Pre-launch Checklist

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

Note: The Flutter team is currently trialing the use of Gemini Code Assist for GitHub. Comments from the gemini-code-assist bot should not be taken as authoritative feedback from the Flutter team. If you find its comments useful you can update your code accordingly, but if you are unsure or disagree with the feedback, please feel free to wait for a Flutter team member's review for guidance on which automated comments should be addressed.

engine/src/flutter/impeller/entity/contents/circle_contents.cc

…dback

flar

Minor changes needed and a couple of nits.

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

engine/src/flutter/impeller/entity/contents/uber_sdf_contents.cc

flar · 2026-04-07T22:36:20Z

engine/src/flutter/impeller/entity/contents/uber_sdf_contents.cc

-    case Join::kRound:
-      frag_info.stroke_join = 2.0f;
-      break;
+  FS::FragInfo frag_info = {};


Do we need the = {}?

It was needed to zero out the struct because the logic below would only optionally populate certain values (e.g. frag_info.stroke_width is not populated if params_.stroke is null). I changed the logic to just always populate every value, with 0s for default values.

flar · 2026-04-07T22:38:40Z

engine/src/flutter/impeller/entity/contents/uber_sdf_contents.cc

+  frag_info.stroked = params_.stroke ? 1.0f : 0.0f;
+  if (params_.stroke) {
+    frag_info.stroke_width = params_.stroke->width;
+    switch (params_.stroke->join) {


Maybe have a static or anonymous static ToShaderJoin method?
(And the same for .type even though it only has 2 values right now.
And if the type conversion function uses a switch statement on an enum class then we can be sure that we update it when we add new types.

Can the shader define constants for the joins and types that appear in the generated C++ header file along side the FragInfo so we can ensure consistency?

I added anonymous static methods as you suggested.

I don't know of a way to define these constants in the shader's generated C++.

engine/src/flutter/impeller/entity/geometry/uber_sdf_geometry.cc

flar · 2026-04-07T22:52:49Z

engine/src/flutter/impeller/entity/entity_unittests.cc

-  FillRectGeometry geometry(rect.Expand(1.0f));
-  auto contents = UberSDFContents::MakeRect(Color::Red(), 0.0f, Join::kMiter,
-                                            false, &geometry);
+  auto params = UberSDFParameters::MakeRect(Color::Red(), rect, std::nullopt);


Do we have one that tests a circle as well?

Added more tests here, for stroked rects and filled/stroked circles

gemini-code-assist

Code Review

This pull request refactors the Impeller engine's SDF rendering by introducing a dedicated UberSDFParameters struct and UberSDFGeometry class to centralize parameter management and geometry calculations. The changes simplify the UberSDFContents interface and improve code maintainability. I have identified a potential division-by-zero issue in UberSDFGeometry::GetPositionBuffer that should be addressed, and I recommend adding documentation to public members to comply with the project's style guide.

engine/src/flutter/impeller/entity/geometry/uber_sdf_geometry.cc

engine/src/flutter/impeller/entity/contents/uber_sdf_parameters.h

engine/src/flutter/impeller/entity/geometry/uber_sdf_geometry.h

engine/src/flutter/impeller/entity/contents/uber_sdf_contents.h

walley892

Great change, glad we came up with a good foundation to build on! A couple of comments

engine/src/flutter/impeller/entity/contents/uber_sdf_parameters.h

engine/src/flutter/impeller/entity/geometry/uber_sdf_geometry.cc

engine/src/flutter/impeller/entity/geometry/uber_sdf_geometry.h

walley892 · 2026-04-08T22:15:15Z

engine/src/flutter/impeller/entity/contents/uber_sdf_contents.h

-                  Scalar aa_padding);
+  static std::unique_ptr<UberSDFContents> Make(
+      const UberSDFParameters& params,
+      std::unique_ptr<Geometry> geometry);


I don't know enough about the surrounding code or the lifetime of this object relative to the surrounding objects where it's used , but it looks like right after this object is created in Canvas a raw pointer to it is grabbed. Is this safe to be a unique_ptr managed by the Contents?

The contents doesn't manage the unique_ptr. Whoever calls this to create a contents manages the unique_ptr.

I believe UberSDFContents was based on CircleContents. So this constructor returns a unique_ptr in the same way as CircleContents

flutter/engine/src/flutter/impeller/entity/contents/circle_contents.h

Line 18 in 129628a

Make(std::unique_ptr<CircleGeometry> geometry, Color color, bool stroked);

and a couple other similar Contents classes.

Oh wait, I think I misinterpreted your comment. You were referring to the geometry unique_ptr, not the constructor's returned unique_ptr.

I think this should be correct with how it's currently used. UberSDFContents is only ever used in canvas.cc, and in both of the use cases it seems correct to me to hand over ownership of the geometry to the Contents.

Edit: And looking at CircleContents, that also takes a unique_ptr for its geometry. So my first comment is still perfectly applicable, accidentally.

Sorry, I wasn't clear. The pointer to the geometry is owned by the Contents, then a raw pointer to the geometry is retrieved in the Canvas.

Maybe the Contents owning the geometry is conceptually correct. The existing code in canvas.cc that uses CircleContents that uses this ownership model doesn't then grab a raw pointer to the geometry, because it doesn't use AddRenderSDFEntityToCurrentPass.

I'm not sure if there will be any issues with giving ownership of the geometry to this Contents class, then later passing around a raw pointer to the geometry - I don't know enough about the surrounding code.

Code pointers (no pun intended) to clarify:

This implementation (which grabs a raw pointer to the geometry owned by the Contents): https://github.com/flutter/flutter/pull/184603/changes#diff-66371f190607b925575505f2a8250f7738c9c444328cb6f453575652c4bde57dL1053

CircleContents (which doesn't grab a raw pointer right away): https://github.com/flutter/flutter/blob/master/engine/src/flutter/impeller/display_list/canvas.cc#L525

All of this is behind a flag so users won't be affected if there are any resulting memory access issues. Can you merge ci.yaml from HEAD so we can test this in CICD?

Merged with HEAD.

I see your point about canvas.cc grabbing and using the raw pointer. Following the code, I do think this is being used in a safe way. But it's non obvious and kind of fragile. I think there's a lot of unnecessary passing around of geometry pointers happening which could be cleaned up. I'm going to make a separate PR to do this so it doesn't clutter this PR.

We have quite a history of using stack allocated objects with raw pointers in Canvas for things like this. The eventual call to AddFooToCurrentPass consumes these objects and is finished with them when it returns.

But unique pointers are safe. We can revisit whether we think stack allocation is important later.

engine/src/flutter/impeller/entity/geometry/uber_sdf_geometry.cc

flar

Submitting a few responses, but not done reviewing everything.

flar · 2026-04-08T22:35:38Z

engine/src/flutter/impeller/entity/geometry/uber_sdf_geometry.cc

+UberSDFGeometry::~UberSDFGeometry() = default;
+
+Rect UberSDFGeometry::GetBaseBounds() const {
+  Rect bounds = Rect::MakeOriginSize(params_.center - params_.size,


thought - I'm questioning whether MakeOriginSize buys us any readability here. Perhaps if Rect had a MakeCenterRadii(Point, Size) factory, but at this point is it more readable to just do the LTRB with center.xy +/- size.xy?

We might consider having a MakeDiagonal that takes 2 points and does a more optimized init than MakePointBounds. Then this could be MakeDiagonal(c-s, c+s)

Done. I changed to to LTRB, which I think is a little more straightforward.

engine/src/flutter/impeller/entity/geometry/uber_sdf_geometry.cc

engine/src/flutter/impeller/entity/geometry/uber_sdf_geometry.h

engine/src/flutter/impeller/entity/contents/uber_sdf_parameters_unittests.cc

walley892

CI is passing, so I guess the weird memory handling is good for now. LGTM

flutter-dashboard · 2026-04-09T03:46:30Z

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.

For more guidance, visit Writing a golden file test for package:flutter.

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

Changes reported for pull request #184603 at sha bf0d218

flar

LGTM, some comments about some potential future work mainly.

flar · 2026-04-09T08:33:04Z

engine/src/flutter/impeller/entity/contents/uber_sdf_contents.h

-                  Scalar aa_padding);
+  static std::unique_ptr<UberSDFContents> Make(
+      const UberSDFParameters& params,
+      std::unique_ptr<Geometry> geometry);


We have quite a history of using stack allocated objects with raw pointers in Canvas for things like this. The eventual call to AddFooToCurrentPass consumes these objects and is finished with them when it returns.

flar · 2026-04-09T08:34:06Z

engine/src/flutter/impeller/entity/contents/uber_sdf_contents.h

-                  Scalar aa_padding);
+  static std::unique_ptr<UberSDFContents> Make(
+      const UberSDFParameters& params,
+      std::unique_ptr<Geometry> geometry);


But unique pointers are safe. We can revisit whether we think stack allocation is important later.

engine/src/flutter/impeller/entity/contents/uber_sdf_parameters.h

engine/src/flutter/impeller/entity/geometry/uber_sdf_geometry.cc

b-luk · 2026-04-09T15:51:57Z

There is one golden change which is significantly different than expected:

And a few others which are slightly weird. Maybe related to blending. I will investigate.

b-luk · 2026-04-09T16:07:09Z

As recommended by walley892, I'm just going to get this submitted to unblock other work that needs to go on top of this. I filed #184828 to investigate the golden issues.

…r stroked rects (flutter/flutter#184603)

flutter/flutter@81c87ea...bf18e39 2026-04-10 25263018+trizin@users.noreply.github.com [flutter_tools] Fix arm64e incorrectly matching arm64 in regex check (flutter/flutter#184057) 2026-04-10 jacksongardner@google.com Specify GitHub Repo in GH CLI calls for revert workflow. (flutter/flutter#184878) 2026-04-10 jacksongardner@google.com Don't use `git add -N` in the sync engine workflow. (flutter/flutter#184882) 2026-04-10 engine-flutter-autoroll@skia.org Roll Skia from af67d5555e35 to 25b01e5f4ea0 (14 revisions) (flutter/flutter#184865) 2026-04-10 737941+loic-sharma@users.noreply.github.com [Dot shorthands] Finish examples/api migration (flutter/flutter#183967) 2026-04-10 engine-flutter-autoroll@skia.org Roll Dart SDK from 98a143f8873e to e715805ddbd3 (1 revision) (flutter/flutter#184864) 2026-04-10 98614782+auto-submit[bot]@users.noreply.github.com Reverts "Disable async mode with LLDB (#184768)" (flutter/flutter#184868) 2026-04-09 katelovett@google.com Skip freeze check in the merge queue (flutter/flutter#184854) 2026-04-09 116356835+AbdeMohlbi@users.noreply.github.com Remove unused variable in `ProcessTextPlugin.java` (flutter/flutter#184161) 2026-04-09 engine-flutter-autoroll@skia.org Roll Fuchsia Linux SDK from pDXMXRIjEHTw7B0sk... to lZcRfPoCLnDttrf9P... (flutter/flutter#184842) 2026-04-09 engine-flutter-autoroll@skia.org Roll Dart SDK from bd6280c3e8e9 to 98a143f8873e (5 revisions) (flutter/flutter#184824) 2026-04-09 34871572+gmackall@users.noreply.github.com Remove `linux_android_emu_unstable android_engine_vulkan_tests` (flutter/flutter#184787) 2026-04-09 engine-flutter-autoroll@skia.org Roll Packages from 0e0a032 to 1aa892c (9 revisions) (flutter/flutter#184829) 2026-04-09 dacoharkes@google.com [record_use] Add experimental flag and test project (flutter/flutter#184719) 2026-04-09 15619084+vashworth@users.noreply.github.com Disable async mode with LLDB (flutter/flutter#184768) 2026-04-09 1063596+reidbaker@users.noreply.github.com Update link for rolling forward to aligned Dart hash (flutter/flutter#184780) 2026-04-09 engine-flutter-autoroll@skia.org Roll Skia from 4d0f5389e131 to af67d5555e35 (3 revisions) (flutter/flutter#184825) 2026-04-09 97480502+b-luk@users.noreply.github.com UberSDFContent refactoring and handle stroke miter limit for stroked rects (flutter/flutter#184603) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-packages Please CC bmparr@google.com,stuartmorgan@google.com on the revert to ensure that a human is aware of the problem. To file a bug in Packages: 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

commit

ccc4941

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

b-luk mentioned this pull request Apr 4, 2026

Improvements and refactoring of UberSDF and Circle SDF classes #184540

Closed

10 tasks

flar reviewed Apr 6, 2026

View reviewed changes

engine/src/flutter/impeller/entity/contents/circle_contents.cc Outdated Show resolved Hide resolved

Revert Circle SDF changes

45c8c25

b-luk force-pushed the ubersdfalwaysfilledrectgeometry branch from f9b2e5a to 45c8c25 Compare April 7, 2026 19:08

b-luk added 3 commits April 7, 2026 13:01

UberSDFParameters

1a3521b

Factor out UberSF Geometry creation out to an UberSDFGeometry class

3da9517

Change UberSDFParams to a struct. Change UberSDFGeometry based on fee…

2b463e7

…dback

flar requested changes Apr 7, 2026

View reviewed changes

flar and others added 4 commits April 7, 2026 19:26

refactor UberSDFGeometry to stand (mostly) on its own

dde2195

store circle data to be compatible with ovals and rect bounds

8399b68

fix device/local bounds in UberSDFGeometry

6795e55

Fixes, cleanups, unittests

39be53e

b-luk changed the title ~~Quick UberSDFContent and CircleContent refactoring prototype~~ UberSDFContent refactoring Apr 8, 2026

b-luk marked this pull request as ready for review April 8, 2026 21:26

b-luk requested review from flar and walley892 April 8, 2026 21:27

gemini-code-assist bot reviewed Apr 8, 2026

View reviewed changes

b-luk changed the title ~~UberSDFContent refactoring~~ UberSDFContent refactoring and adding support for stroked rects Apr 8, 2026

b-luk changed the title ~~UberSDFContent refactoring and adding support for stroked rects~~ UberSDFContent refactoring Apr 8, 2026

Gemini comments

d6cbeaf

b-luk added the CICD Run CI/CD label Apr 8, 2026

b-luk changed the title ~~UberSDFContent refactoring~~ UberSDFContent refactoring and handle stroke miter limit for stroked rects Apr 8, 2026

walley892 requested changes Apr 8, 2026

View reviewed changes

flar reviewed Apr 8, 2026

View reviewed changes

Address comments

6e32e96

github-actions bot removed the CICD Run CI/CD label Apr 8, 2026

b-luk requested a review from flar April 8, 2026 23:23

Merge branch 'master' into ubersdfalwaysfilledrectgeometry

bf0d218

github-actions bot removed the CICD Run CI/CD label Apr 9, 2026

walley892 added the CICD Run CI/CD label Apr 9, 2026

walley892 approved these changes Apr 9, 2026

View reviewed changes

flutter-dashboard bot added the will affect goldens Changes to golden files label Apr 9, 2026

flar approved these changes Apr 9, 2026

View reviewed changes

b-luk added the autosubmit Merge PR when tree becomes green via auto submit App label Apr 9, 2026

b-luk mentioned this pull request Apr 9, 2026

Investigate golden issues that appeared switching UberSDF to use quad geometry #184828

Open

auto-submit bot added this pull request to the merge queue Apr 9, 2026

Merged via the queue into flutter:master with commit 47b5032 Apr 9, 2026
201 checks passed

flutter-dashboard bot removed the autosubmit Merge PR when tree becomes green via auto submit App label Apr 9, 2026

b-luk deleted the ubersdfalwaysfilledrectgeometry branch April 9, 2026 22:58

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Apr 10, 2026

47b50322f UberSDFContent refactoring and handle stroke miter limit fo…

13b2ba5

…r stroked rects (flutter/flutter#184603)

engine-flutter-autoroll mentioned this pull request Apr 10, 2026

Roll Flutter from 81c87ea165df to c6f719d7a523 (11 revisions) flutter/packages#11481

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Apr 10, 2026

47b50322f UberSDFContent refactoring and handle stroke miter limit fo…

04238bb

…r stroked rects (flutter/flutter#184603)

engine-flutter-autoroll mentioned this pull request Apr 10, 2026

Roll Flutter from 81c87ea165df to 49233d08009d (12 revisions) flutter/packages#11483

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Apr 10, 2026

47b50322f UberSDFContent refactoring and handle stroke miter limit fo…

998a0e4

…r stroked rects (flutter/flutter#184603)

engine-flutter-autoroll mentioned this pull request Apr 10, 2026

Roll Flutter from 81c87ea165df to 0fe62a57bbac (13 revisions) flutter/packages#11485

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Apr 10, 2026

47b50322f UberSDFContent refactoring and handle stroke miter limit fo…

4dc0184

…r stroked rects (flutter/flutter#184603)

engine-flutter-autoroll mentioned this pull request Apr 10, 2026

Roll Flutter from 81c87ea165df to f417bcba3c36 (15 revisions) flutter/packages#11486

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Apr 10, 2026

47b50322f UberSDFContent refactoring and handle stroke miter limit fo…

6014d22

…r stroked rects (flutter/flutter#184603)

engine-flutter-autoroll mentioned this pull request Apr 10, 2026

Roll Flutter from 81c87ea165df to b7165406fbc3 (17 revisions) flutter/packages#11487

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Apr 10, 2026

47b50322f UberSDFContent refactoring and handle stroke miter limit fo…

9eea4be

…r stroked rects (flutter/flutter#184603)

engine-flutter-autoroll mentioned this pull request Apr 10, 2026

Roll Flutter from 81c87ea165df to bf18e3958a05 (18 revisions) flutter/packages#11488

Merged

Piinks mentioned this pull request Apr 10, 2026

Flutter Notable Commits - 2026 #181433

Open

Conversation

b-luk commented Apr 4, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pre-launch Checklist

Uh oh!

Uh oh!

flar left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

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!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

walley892 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

b-luk Apr 8, 2026 • 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!

walley892 Apr 8, 2026 • 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!

Uh oh!

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

Uh oh!

Uh oh!

Uh oh!

Uh oh!

b-luk commented Apr 4, 2026 •

edited

Loading

b-luk Apr 8, 2026 •

edited

Loading

walley892 Apr 8, 2026 •

edited

Loading