Move more RSE rendering cases to complex_rse.frag by walley892 · Pull Request #191235 · flutter/flutter

walley892 · 2026-08-18T00:26:40Z

Move all non-squarelike non-symmetric RoundSuperellipse rendering to complex_rse.frag. See discussion in #190874

walley892 · 2026-08-18T00:26:55Z

/gemini review

gemini-code-assist

Code Review

This pull request simplifies the rendering of symmetric rounded superellipses in Impeller. It refactors the shader logic by introducing a shared rse_sdf.glsl file, simplifying the UberSDFParameters structure, and updating the coordinate mapping in RoundSuperellipseParam. Feedback was provided regarding a potential issue in uber_sdf.frag where normalized and local coordinates are mixed in the call to distanceFromRoundedSuperellipse, which could lead to rendering artifacts.

gemini-code-assist

Code Review

This pull request refactors and simplifies the rounded superellipse rendering pipeline in Impeller. It introduces a shared shader header rse_sdf.glsl to deduplicate distance calculation logic between complex_rse.frag and uber_sdf.frag. Additionally, it simplifies UberSDFParameters and the corresponding shader uniforms by utilizing symmetric corner parameters for square-like superellipses, and updates RoundSuperellipseParam to populate all four quadrants. Feedback suggests explicitly including sdf_utils.glsl in the new rse_sdf.glsl shader file to avoid compilation errors from missing constant definitions.

b-luk · 2026-08-18T23:12:59Z

+    const Rect& bounds = round_superellipse.GetBounds();
+    const RoundingRadii& radii = round_superellipse.GetRadii();
+
+    bool is_uniform_circular = radii.AreAllCornersSame() &&


This used to check round_superellipse_params.all_corners_same and now it checks radii.AreAllCornersSame(). There's a corner case where these are not the same: when the radii are empty

flutter/engine/src/flutter/impeller/geometry/round_superellipse_param.cc

Lines 579 to 591 in d8dd11d

if (radii.AreAllCornersSame() && !radii.top_left.IsEmpty()) {

// Having four empty corners indicate a rectangle, which needs special

// treatment on border containment and therefore is not `all_corners_same`.

return RoundSuperellipseParam{

.top_right = ComputeQuadrant(bounds.GetCenter(), bounds.GetRightTop(),

radii.top_right, {-1, 1}),

.all_corners_same = true,

.top_split = bounds.GetCenter().x,

.bottom_split = bounds.GetCenter().x,

.left_split = bounds.GetCenter().y,

.right_split = bounds.GetCenter().y,

};

}

Does this corner case difference matter here?

I don't think the radii can be empty here coming from RoundSuperellipse. The RoundSuperellipseParam has empty octant information if all the octants have the same parameters (in which case, only the top right is populated).

I don't mind keeping all_corners_same, though - it seemed more natural to do two checks on radii than one on radii and one on rse_param. Updated

I don't think the radii can be empty here coming from RoundSuperellipse

Wouldn't it be empty if DrawRoundSuperellipse is called with all radii 0 (with a rectangle shape)?

So before, when DrawRoundSuperellipse is called with a rectangle, the RoundSuperellipseParam constructor I linked above checks (radii.AreAllCornersSame() && !radii.top_left.IsEmpty()) and gets false (all corners are the same, but the top left radii is empty). The created Params gets .all_corners_same = false, and it sets all 4 corners. The check in canvas.cc is if (round_superellipse_params.all_corners_same), so for a rectangular RSE this is false, and we fall back to ComplexRSE.

With the PR at the time I commented, the canvas.cc check is if (radii.AreAllCornersSame() && radii.top_left.width == radii.top_left.height. So when called with a rectangle, this check returns true, and canvas.cc sends it to UberSDF instead of ComplexRSE.

it seemed more natural to do two checks on radii than one on radii and one on rse_param

It's definitely more natural to do two checks on radii than one on radii and one on rse_param. So if we can check only the RoundSuperellipse radii and not check rse_param, I'm all for it. I just wanted to make sure this difference in behavior is thought through and is handled correctly.

Oh, I see.

The display list builder prevents this from happening by routing DrawRoundSuperellipse calls directly to DrawRect if the radii are zero. Both the rendering in UberSDF and ComplexRSE are capable of supporting zero radii though.

Makes sense that dl_builder routes to DrawRect when it gets a rectangular RSE. It seems fine to check either radii or rse_params then.

I do think it's weird that we need this special case logic in RSEParams for the rectangle case. Removing that would simplify some of the confusing parts of RSEParams. The comment says it has to do with border containment (I assume the Contains method, which has different logic depending on all_corners_same). But I feel like this can be done in a more intuitive way than this weird thing where all_corners_same means all the corners are the same with the exception of when all the corners are 0. Maybe we just always detect rectangular RSEs for whatever code path uses this, and treat it as a rectangle. Simplifying this is not within the scope of this PR, so no need to do anything about it.

Yeah, I don't really want to touch that file

b-luk · 2026-08-18T23:22:12Z

Should we add other DCHECKs for the new stricter UberSDF RSE requirements? Similar to here.

It's not possible to check that the corner radii are equal with just a RoundSuperelllipseParam. In the case of a rectangular RSE with "circular corners", the corner radii are actually transformed within RoundSuperelllipseParam.

We could refactor this and the similar ComplexRSE function to take in a RoundSuperellipse, check using that, and construct the RSEParam in here. Thoughts?

Checks would only be for the UberSDF one here and not the ComplexRSE function, because ComplexRSE accepts all RSEs.

I do like the idea of making this function take a RoundSuperellipse, then doing checks and constructing the RSEParam in this function. Maybe the function should return an optional and return nullopt if the RSE is not UberSDF compatible. That way canvas.cc doesn't need to do any checks. It just calls this, and if this returns nullopt then canvas.cc falls back to ComplexRSE.

Changed the factory to return Optional and take in a RoundSuperellipse.

I thought it was awkward to have canvas construct a rse_param for only ComplexRSEContents, so I modified both factories to take in a RoundSuperellipse.

auto-submit · 2026-08-21T02:45:17Z

autosubmit label was removed for flutter/flutter/191235, because - The status or check suite Dashboard Checks has failed. Please fix the issues identified (or deflake) before re-applying this label.

flutter/flutter@65c9a8d...9a82789 2026-08-24 [email protected] Roll Packages from 252bb33 to df2ba94 (5 revisions) (flutter/flutter#191578) 2026-08-24 [email protected] Roll Dart SDK from cb056da580b6 to 78bbc37b6ff6 (13 revisions) (flutter/flutter#191574) 2026-08-24 [email protected] Roll Skia from 50452b13f05b to 57968d087d18 (1 revision) (flutter/flutter#191572) 2026-08-24 [email protected] Move more RSE rendering cases to complex_rse.frag (flutter/flutter#191235) 2026-08-24 [email protected] Roll Skia from 925e13559532 to 50452b13f05b (2 revisions) (flutter/flutter#191570) 2026-08-24 [email protected] iOS: Add stylus pointer tests (flutter/flutter#191564) 2026-08-24 [email protected] iOS: Improve FlutterEngineGroup tests (flutter/flutter#191563) 2026-08-24 [email protected] Roll Skia from c78782c7c1c2 to 925e13559532 (5 revisions) (flutter/flutter#191568) 2026-08-24 [email protected] Roll Skia from 886302c7c68e to c78782c7c1c2 (1 revision) (flutter/flutter#191566) 2026-08-24 [email protected] Revert "Improve initial size and placement logic for popups and toolt…ips on Linux (#189994) (flutter/flutter#191498) 2026-08-23 [email protected] Roll Fuchsia Linux SDK from Cqs-NyeELd60hqv1e... to 8Xu4ujBJniC0nQGx3... (flutter/flutter#191561) 2026-08-23 [email protected] Fixes MSAA on the linux embedder. (flutter/flutter#191379) 2026-08-23 [email protected] Remove Material/Cupertino references in error messages (flutter/flutter#191305) 2026-08-23 [email protected] Roll Skia from a1825062f832 to 886302c7c68e (1 revision) (flutter/flutter#191559) 2026-08-23 [email protected] Roll Skia from ad2106c0bb64 to a1825062f832 (1 revision) (flutter/flutter#191555) 2026-08-22 [email protected] Roll Fuchsia Linux SDK from ic6GjOSn-KN508XyK... to Cqs-NyeELd60hqv1e... (flutter/flutter#191547) 2026-08-22 [email protected] [WebParagraph] Fixing minor problem with getBoxesForRange (flutter/flutter#191419) 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 [email protected],[email protected] 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

… uniforms (flutter#191597) Followup to flutter#191235 (Move more RSE rendering cases to complex_rse.frag). In that PR, there was some discussion about whether removing the `octant_offset_c` uniform would have an affect on the registers used reported by malioc. We decided that we didn't need to remove `octant_offset_c`. But from testing out the ubesdf gradient changes (flutter#190874) after merging with flutter#191235, the Mali-G78 registers used on Vulkan (flutter/impeller/entity/uber_sdf.frag.vkspv) is still 66. Removing `octant_offset_c` brings the registers used down by 2, as seen in the malioc changes in this PR. So this is needed to bring down the registers used to 64 when the gradient changes are included. This PR also incorporates the reordering and adding of comments to uber_sdf's uniforms. This way the upcoming gradient change is scoped to only adding the new gradient functionality for easier review. ## Pre-launch Checklist - [x] I read the [Contributor Guide] and followed the process outlined there for submitting PRs. - [x] I read the [AI contribution guidelines] and understand my responsibilities, or I am not using AI tools. - [x] I read the [Tree Hygiene] wiki page, which explains my responsibilities. - [x] I read and followed the [Flutter Style Guide], including [Features we expect every widget to implement]. - [x] I signed the [CLA]. - [ ] I listed at least one issue that this PR fixes in the description above. - [x] I updated/added relevant documentation (doc comments with `///`). - [x] I added new tests to check the change I am making, or this PR is [test-exempt]. - [x] 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]. If this change needs to override an active code freeze, provide a comment explaining why. The code freeze workflow can be overridden by code reviewers. See pinned issues for any active code freezes with guidance. **Note**: The Flutter team is currently trialing the use of [Gemini Code Assist for GitHub](https://developers.google.com/gemini-code-assist/docs/review-github-code). 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.  [Contributor Guide]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#overview [AI contribution guidelines]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#ai-contribution-guidelines [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

…r#12578) flutter/flutter@65c9a8d...9a82789 2026-08-24 [email protected] Roll Packages from 252bb33 to df2ba94 (5 revisions) (flutter/flutter#191578) 2026-08-24 [email protected] Roll Dart SDK from cb056da580b6 to 78bbc37b6ff6 (13 revisions) (flutter/flutter#191574) 2026-08-24 [email protected] Roll Skia from 50452b13f05b to 57968d087d18 (1 revision) (flutter/flutter#191572) 2026-08-24 [email protected] Move more RSE rendering cases to complex_rse.frag (flutter/flutter#191235) 2026-08-24 [email protected] Roll Skia from 925e13559532 to 50452b13f05b (2 revisions) (flutter/flutter#191570) 2026-08-24 [email protected] iOS: Add stylus pointer tests (flutter/flutter#191564) 2026-08-24 [email protected] iOS: Improve FlutterEngineGroup tests (flutter/flutter#191563) 2026-08-24 [email protected] Roll Skia from c78782c7c1c2 to 925e13559532 (5 revisions) (flutter/flutter#191568) 2026-08-24 [email protected] Roll Skia from 886302c7c68e to c78782c7c1c2 (1 revision) (flutter/flutter#191566) 2026-08-24 [email protected] Revert "Improve initial size and placement logic for popups and toolt…ips on Linux (#189994) (flutter/flutter#191498) 2026-08-23 [email protected] Roll Fuchsia Linux SDK from Cqs-NyeELd60hqv1e... to 8Xu4ujBJniC0nQGx3... (flutter/flutter#191561) 2026-08-23 [email protected] Fixes MSAA on the linux embedder. (flutter/flutter#191379) 2026-08-23 [email protected] Remove Material/Cupertino references in error messages (flutter/flutter#191305) 2026-08-23 [email protected] Roll Skia from a1825062f832 to 886302c7c68e (1 revision) (flutter/flutter#191559) 2026-08-23 [email protected] Roll Skia from ad2106c0bb64 to a1825062f832 (1 revision) (flutter/flutter#191555) 2026-08-22 [email protected] Roll Fuchsia Linux SDK from ic6GjOSn-KN508XyK... to Cqs-NyeELd60hqv1e... (flutter/flutter#191547) 2026-08-22 [email protected] [WebParagraph] Fixing minor problem with getBoxesForRange (flutter/flutter#191419) 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 [email protected],[email protected] 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

move more RSE rendering cases to complex_rse.frag

17615ce

walley892 added the CICD Run CI/CD label Aug 18, 2026

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

gemini-code-assist Bot reviewed Aug 18, 2026

View reviewed changes

Comment thread engine/src/flutter/impeller/entity/shaders/uber_sdf.frag Resolved

malioc

0be3152

walley892 mentioned this pull request Aug 18, 2026

Handle linear and radial gradients in UberSDF #190874

Merged

10 tasks done

walley892 marked this pull request as ready for review August 18, 2026 19:53

walley892 requested review from b-luk and gaaclarke August 18, 2026 19:53

gemini-code-assist Bot reviewed Aug 18, 2026

View reviewed changes

Comment thread engine/src/flutter/impeller/entity/shaders/rse_sdf.glsl Resolved

walley892 added 4 commits August 18, 2026 15:52

Move fewer cases to complexRSE

8c49259

remove semi_axis param from ubersdf

f8c6489

malioc

2ca9252

gemini comments

d4c10a2

walley892 added CICD Run CI/CD and removed CICD Run CI/CD labels Aug 18, 2026

b-luk requested changes Aug 18, 2026

View reviewed changes

walley892 added 4 commits August 19, 2026 13:59

comments round 1

3efe01b

Move missing quadrant info logic into rse_contents

c2d8dc3

more comments

23a9940

malioc round2

ccac7bd

walley892 requested a review from b-luk August 19, 2026 23:24

b-luk requested changes Aug 20, 2026

View reviewed changes

Move compatability checking into UberSDFParams

86c580d

walley892 added CICD Run CI/CD and removed CICD Run CI/CD labels Aug 20, 2026

b-luk previously approved these changes Aug 20, 2026

View reviewed changes

Merge remote-tracking branch 'upstream/master' into rse-refactor

02a080f

walley892 added CICD Run CI/CD autosubmit Merge PR when tree becomes green via auto submit App and removed CICD Run CI/CD labels Aug 21, 2026

auto-submit Bot removed the autosubmit Merge PR when tree becomes green via auto submit App label Aug 21, 2026

update unit test value assertion

5c719d0

walley892 dismissed b-luk’s stale review via 5c719d0 August 21, 2026 02:51

walley892 added CICD Run CI/CD and removed CICD Run CI/CD labels Aug 21, 2026

silly

5ed0e30

walley892 added CICD Run CI/CD and removed CICD Run CI/CD labels Aug 21, 2026

walley892 requested a review from b-luk August 21, 2026 16:48

b-luk approved these changes Aug 21, 2026

View reviewed changes

walley892 added the autosubmit Merge PR when tree becomes green via auto submit App label Aug 24, 2026

auto-submit Bot added this pull request to the merge queue Aug 24, 2026

Merged via the queue into flutter:master with commit ed21324 Aug 24, 2026
29 checks passed

flutter-dashboard Bot removed the autosubmit Merge PR when tree becomes green via auto submit App label Aug 24, 2026

This was referenced Aug 24, 2026

Roll Flutter from 65c9a8dc60bc to a48daa092b0b (16 revisions) flutter/packages#12573

Closed

Roll Flutter from 65c9a8dc60bc to 9a8278982588 (17 revisions) flutter/packages#12578

Merged

b-luk mentioned this pull request Aug 24, 2026

Remove octant_offset_c uniform from ubersdf, reorder/document ubersdf uniforms #191597

Merged

8 of 10 tasks

	if (radii.AreAllCornersSame() && !radii.top_left.IsEmpty()) {
	// Having four empty corners indicate a rectangle, which needs special
	// treatment on border containment and therefore is not `all_corners_same`.
	return RoundSuperellipseParam{
	.top_right = ComputeQuadrant(bounds.GetCenter(), bounds.GetRightTop(),
	radii.top_right, {-1, 1}),
	.all_corners_same = true,
	.top_split = bounds.GetCenter().x,
	.bottom_split = bounds.GetCenter().x,
	.left_split = bounds.GetCenter().y,
	.right_split = bounds.GetCenter().y,
	};
	}

Conversation

walley892 commented Aug 18, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

walley892 commented Aug 18, 2026

Uh oh!

gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

Uh oh!

gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

Code Review

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!

Choose a reason for hiding this comment

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!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

auto-submit Bot commented Aug 21, 2026

Uh oh!

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

walley892 commented Aug 18, 2026 •

edited

Loading