Skip to content

Uber compactify - #196392

Merged
auto-submit[bot] merged 4 commits into
flutter:masterfrom
walley892:uber-compactify
Nov 12, 2026
Merged

Uber compactify#196392
auto-submit[bot] merged 4 commits into
flutter:masterfrom
walley892:uber-compactify

Conversation

@walley892

@walley892 walley892 commented Nov 11, 2026

Copy link
Copy Markdown
Contributor

Refactors a few things in uber_sdf to save on uniform registers.

  • makes aa_pixels a constant in the fragment shader rather than being set by uniform
  • reworks the superellipse corner calculation to remove atan and mod
  • Folds the superellipse angle_span into unused radii fields
  • Moves center and gradient_origin to vertex inputs

Prework for #195703

Pre-launch Checklist

  • I read the [Contributor Guide] and followed the process outlined there for submitting PRs.
  • I read the [AI contribution guidelines] and understand my responsibilities, or I am not using AI tools.
  • 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 in-code documentation (doc comments with ///).
  • If this PR introduces a new feature or capability, I created and linked a website documentation issue or PR in [flutter/website] (or verified none is needed).
  • 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.

@walley892
walley892 requested a review from gaaclarke November 11, 2026 18:30
@flutter-dashboard

flutter-dashboard Bot commented Nov 11, 2026

Copy link
Copy Markdown

It looks like this pull request may not have tests. Please make sure to add tests or get an explicit test exemption before merging.

If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix?

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing. If both you and the PR author believe this PR qualifies for a test exemption, escalate to the tech lead for the changed area to review and apply the test-exempt label. All reviewers should feel empowered to ask for tests before escalating.

@github-actions github-actions Bot added engine flutter/engine related. See also e: labels. e: impeller Impeller rendering backend issues and features requests labels Nov 11, 2026
gemini-code-assist[bot]
gemini-code-assist Bot reviewed Nov 11, 2026
View reviewed changes

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request introduces a new vertex shader, uber_sdf.vert, which offloads per-draw offsets from the fragment shader to reduce uniform register usage. It also optimizes rounded superellipse calculations in rse_sdf.glsl by utilizing the cosine of the angular span to avoid expensive operations, packing these values into the unused radii.zw uniform components. Feedback on these changes highlights a potential rendering regression in uber_sdf_common.glsl where directly casting float uniforms to integers via int() can lead to truncation errors on certain GPU architectures; using round() before casting is recommended to ensure robust conversion.

Comment thread engine/src/flutter/impeller/entity/shaders/uber_sdf_common.glsl Resolved
Comment thread engine/src/flutter/impeller/entity/shaders/uber_sdf_common.glsl Resolved
@walley892 walley892 mentioned this pull request Nov 11, 2026
Open
11 tasks done
@walley892 walley892 added CICD Run CI/CD and removed CICD Run CI/CD labels Nov 11, 2026
gaaclarke
gaaclarke requested changes Nov 11, 2026
View reviewed changes

@gaaclarke gaaclarke left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a few cleanup requests and one question about ints, otherwise liking the approach.

Comment thread engine/src/flutter/impeller/entity/contents/uber_sdf_contents.cc Resolved
Comment thread engine/src/flutter/impeller/entity/shaders/rse_sdf.glsl Outdated
Comment on lines +33 to +38
// Equivalent to `useCircularDistance`, but takes the cosine of the span.
//
// `p` is within the span of the corner circle's arc when the angle between `p`
// and the 45 degree midline is less than the span, i.e. when
// cos(angle) > cos(span). This avoids atan() and mod(), which need more
// constants and uniform registers.

@gaaclarke gaaclarke Nov 11, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you format this like doxygen comments? Also have it describe what it does and do a see also for useCircularDistance

@walley892 walley892 Nov 11, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yessir

Comment thread engine/src/flutter/impeller/entity/shaders/uber_sdf_common.glsl Outdated
float pixel_size;
if (frag_info.type < 0.5) { // Circle
// frag_info.type holds an exact small integer.
int type = int(frag_info.type);

@gaaclarke gaaclarke Nov 11, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we have int support for all our targeted devices? I thought some opengles 2 stuff might not have it?

@walley892 walley892 Nov 11, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We have int support within the shader

flutter/engine/src/flutter/impeller/entity/shaders/blending/blend_select.glsl

Line 25 in b718471

f16vec3 AdvancedBlend(f16vec3 dst, f16vec3 src, int blend_type) {
.

Int uniforms are converted into floats, maybe you're thinking of that?

@walley892
walley892 requested a review from gaaclarke November 11, 2026 23:06
gaaclarke
gaaclarke approved these changes Nov 11, 2026
View reviewed changes

@gaaclarke gaaclarke left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, I have one nit that I would ask for a follow up when you get a chance please

Comment thread engine/src/flutter/impeller/entity/shaders/rse_sdf.glsl
Comment on lines +33 to +46
/// Returns whether the closest point on a rounded superellipse octant to `p`
/// is on the octant's corner circle, in which case the distance to the
/// superellipse is the distance to that circle.
///
/// `p` is relative to the corner circle's center. The closest point is on the
/// circle when `p` is within the circle's arc, i.e. when the angle between `p`
/// and the 45 degree midline is less than the arc's angular span. This is
/// tested as cos(angle) > `cos_span`, where `cos_span` is the cosine of the
/// angular span.
///
/// This gives the same result as `useCircularDistance`, which takes the span
/// itself, but avoids atan() and mod(), which need more constants and uniform
/// registers.
///

@gaaclarke gaaclarke Nov 11, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, this isn't quite doxygen format. I'd like to see parameter comments. This is important because those parameters are not easily understood by looking at the signature.

/// brief
///
/// details
/// @param p blah blah
/// @ param cos_span blah blah
/// @returns true when yadda yadda yadda

@gaaclarke

gaaclarke commented Nov 12, 2026

Copy link
Copy Markdown
Member

test-exempt: no logic change, just an optimization

@gaaclarke gaaclarke added the test-exempt Exempt from tests. Overrides `missing-tests` label label Nov 12, 2026
@walley892 walley892 added the autosubmit Merge PR when tree becomes green via auto submit App label Nov 12, 2026
@auto-submit auto-submit Bot removed the autosubmit Merge PR when tree becomes green via auto submit App label Nov 12, 2026
@auto-submit

auto-submit Bot commented Nov 12, 2026

Copy link
Copy Markdown
Contributor

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

@flutter-dashboard

flutter-dashboard Bot commented Nov 12, 2026

Copy link
Copy Markdown

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 #196392 at sha e7a7d12

@flutter-dashboard flutter-dashboard Bot added the will affect goldens Changes to golden files label Nov 12, 2026
@walley892 walley892 added the autosubmit Merge PR when tree becomes green via auto submit App label Nov 12, 2026
@auto-submit
auto-submit Bot added this pull request to the merge queue Nov 12, 2026
Merged via the queue into flutter:master with commit c08f4b8 Nov 12, 2026
32 checks passed
@flutter-dashboard flutter-dashboard Bot removed the autosubmit Merge PR when tree becomes green via auto submit App label Nov 12, 2026
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Nov 12, 2026
Merged
auto-submit Bot pushed a commit to flutter/packages that referenced this pull request Nov 12, 2026
@engine-flutter-autoroll
Roll Flutter from b98afb524077 to 9c427b5d7b1c (29 revisions) (#13636)
11da3ba 
flutter/flutter@b98afb5...9c427b5

2026-11-12 [email protected] Roll Skia from d67d0f22e3ec to 9aa2513a4ac9 (2 revisions) (flutter/flutter#196429)
2026-11-12 [email protected] [flutter_tools] Move CpuArch to flutter_tools_core and define HostPlatform by osName and cpuArch (flutter/flutter#195841)
2026-11-12 [email protected] Roll Skia from 23e7e63789b8 to d67d0f22e3ec (2 revisions) (flutter/flutter#196424)
2026-11-12 [email protected] Uber compactify (flutter/flutter#196392)
2026-11-12 [email protected] Roll Skia from 3d7e335da165 to 23e7e63789b8 (12 revisions) (flutter/flutter#196422)
2026-11-12 [email protected] Roll Dart SDK from 13af329ea4f2 to e5add7a25b32 (1 revision) (flutter/flutter#196413)
2026-11-12 [email protected] Don't terminate the shared EGL display when an engine is disposed (flutter/flutter#194520)
2026-11-11 [email protected] Adds manifest flag to turn on SDFs for Android. (flutter/flutter#196056)
2026-11-11 [email protected] Remove bringup from Linux_pixel_7pro integration_ui_keyboard_inset_jump (flutter/flutter#196334)
2026-11-11 [email protected] Uses system default locale for text rendering when preferred locale isn't supported (flutter/flutter#195997)
2026-11-11 [email protected] [android]: prewarms shaders when the context is created (flutter/flutter#196069)
2026-11-11 [email protected] iOS: Recomposite platform views only if their parameters change (flutter/flutter#196287)
2026-11-11 [email protected] [flutter_tools] Forward encoding in ErrorHandlingFile.readAsStringSync (flutter/flutter#196136)
2026-11-11 [email protected] Roll Skia from b89c24bbab8c to 3d7e335da165 (8 revisions) (flutter/flutter#196390)
2026-11-11 [email protected] Add Flutter Gardener instructions on how to disable a test (flutter/flutter#196312)
2026-11-11 [email protected] [Impeller] Enable malioc analysis for framebuffer_blend shaders (flutter/flutter#196343)
2026-11-11 [email protected] [flutter_tools] Migrate general.shard Android, asset, and build tests off globals (flutter/flutter#196125)
2026-11-11 [email protected] Roll Dart SDK from 8f15c4ca2d41 to 13af329ea4f2 (1 revision) (flutter/flutter#196386)
2026-11-11 [email protected] [Impeller] Do not clamp the local coverage limit to the root render target (flutter/flutter#195860)
2026-11-11 [email protected] [Impeller] Use separate uniform buffers on VMware SVGA3D (flutter/flutter#196022)
2026-11-11 [email protected] [flutter_tools] Introduce AndroidSdkProvider for lazy native_assets SDK lookup (flutter/flutter#196122)
2026-11-11 [email protected] Moves linux to ThinLTO. (flutter/flutter#196314)
2026-11-11 [email protected] [flutter_tools] Migrate bin/fuchsia_tester.dart off globals and runInContext (flutter/flutter#196074)
2026-11-11 [email protected] Changes the tolerances for frame_rate_main_test.dart. (flutter/flutter#196304)
2026-11-11 [email protected] Roll Packages from 08faf64 to 653628e (16 revisions) (flutter/flutter#196381)
2026-11-11 [email protected] Allow SliverCrossAxisGroup children to lay out with zero cross-axis extent instead of asserting (flutter/flutter#191287)
2026-11-11 [email protected] Roll Skia from cc4c54f752ba to b89c24bbab8c (4 revisions) (flutter/flutter#196375)
2026-11-11 [email protected] [Linux] Fix layer offset when compositing with glBlitFramebuffer (flutter/flutter#196189)
2026-11-11 [email protected] [flutter_tools] Inject CrashReportSender into SwiftPM migration (flutter/flutter#196315)

If this roll has caused a breakage, revert this CL and set the roller
to dry run mode 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@gaaclarke gaaclarke gaaclarke approved these changes
+1 more reviewer
@gemini-code-assist gemini-code-assist[bot] gemini-code-assist[bot] left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

CICD Run CI/CD e: impeller Impeller rendering backend issues and features requests engine flutter/engine related. See also e: labels. missing-tests test-exempt Exempt from tests. Overrides `missing-tests` label will affect goldens Changes to golden files

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@walley892 @gaaclarke