Skip to content

Use named uniforms instead of setFloat in StretchEffect - #195372

Merged
auto-submit[bot] merged 5 commits into
flutter:masterfrom
mohamedalsamrai:named-shader-uniforms
Nov 3, 2026
Merged

auto-submit[bot] merged 5 commits into
flutter:masterfrom
mohamedalsamrai:named-shader-uniforms

Conversation

@mohamedalsamrai

@mohamedalsamrai mohamedalsamrai commented Oct 26, 2026 •

Copy link
Copy Markdown
Contributor

IMoves StretchEffect from the index-based FragmentShader.setFloat to getUniformFloat, as part of deprecating setFloat (#195357).

StretchEffect creates a new shader on every build, so it looks up its four uniforms by name right after creating it, using a local variable instead of repeating _fragmentShader!. u_size and u_texture are still set by the engine for ImageFilter.shader. Every uniform gets the same value as before, so there is no visual change.

The InkSparkle part of #195358 is in flutter/packages#13445, since Material is frozen in this repo (#188444).

Part of #195358

This is a refactor with no semantic change, so I think it qualifies for test-exempt. It's covered by the existing golden test in stretch_effect_test.dart, which runs in the Impeller shard, and a misspelled uniform name would throw when that test runs.

Pre-launch Checklist

InkSparkle and StretchEffect now set their shader uniforms by name with getUniformFloat, getUniformVec2 and getUniformVec4 instead of the index-based setFloat, which is being deprecated in flutter#195357. Every uniform gets the same value as before, so rendering does not change.

Fixes flutter#195358
@flutter-dashboard

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

Copy link
Copy Markdown

This pull request contains changes to Material or Cupertino, which are currently frozen in this repository.

Changes should be made in material_ui and/or cupertino_ui in the flutter/packages repository.

Please refer to #188444 for instructions.

@github-actions github-actions Bot added framework flutter/packages/flutter repository. See also f: labels. p: material_ui material_ui package in flutter/packages labels Oct 26, 2026

@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 refactors shader uniform assignments in both InkSparkle and StretchOverscrollEffect to use named uniform lookups instead of positional index-based setFloat calls. For InkSparkle, a helper class _InkSparkleUniforms is introduced to cache uniform slots. Feedback suggests introducing a local non-nullable variable for the fragment shader in StretchOverscrollEffect to avoid repeated null-assertions and improve readability.

@mohamedalsamrai
mohamedalsamrai marked this pull request as draft October 26, 2026 22:38
@github-actions github-actions Bot removed the p: material_ui material_ui package in flutter/packages label Oct 26, 2026
@mohamedalsamrai mohamedalsamrai changed the title Use named uniforms instead of setFloat in InkSparkle and StretchEffect Use named uniforms instead of setFloat in StretchEffect Oct 26, 2026
@mohamedalsamrai
mohamedalsamrai marked this pull request as ready for review October 26, 2026 23:16

@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 updates the stretch effect widget to use named uniform lookups on the fragment shader instead of positional float setters. Feedback suggests refactoring the uniform lookups for 'u_overscroll_x' and 'u_overscroll_y' outside of the conditional block to avoid duplicate lookups and improve readability.

Comment on lines 190 to 196
if (widget.axis == Axis.vertical) {
_fragmentShader!.setFloat(3, 0.0);
_fragmentShader!.setFloat(4, widget.stretchStrength);
shader.getUniformFloat('u_overscroll_x').set(0.0);
shader.getUniformFloat('u_overscroll_y').set(widget.stretchStrength);
} else {
_fragmentShader!.setFloat(3, widget.stretchStrength);
_fragmentShader!.setFloat(4, 0.0);
shader.getUniformFloat('u_overscroll_x').set(widget.stretchStrength);
shader.getUniformFloat('u_overscroll_y').set(0.0);
}

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.

medium

To avoid duplicating the string literals 'u_overscroll_x' and 'u_overscroll_y' and to reduce redundant lookup calls, consider retrieving these uniforms once outside the conditional block.

      final overscrollX = shader.getUniformFloat('u_overscroll_x');
      final overscrollY = shader.getUniformFloat('u_overscroll_y');
      if (widget.axis == Axis.vertical) {
        overscrollX.set(0.0);
        overscrollY.set(widget.stretchStrength);
      } else {
        overscrollX.set(widget.stretchStrength);
        overscrollY.set(0.0);
      }
References
  1. Suggest simplification and refactoring to enhance readability and maintainability. (link)

@justinmc
justinmc requested review from chunhtai and justinmc October 27, 2026 22:47
if (widget.axis == Axis.vertical) {
_fragmentShader!.setFloat(3, 0.0);
_fragmentShader!.setFloat(4, widget.stretchStrength);
shader.getUniformFloat('u_overscroll_x').set(0.0);

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.

I would prefer that we stored the shader.getUniformFloat result an ivar. That way during animations we aren't constantly resolving the slot.

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.

Done. The state now creates the shader once and keeps it with its four slots (_StretchEffectUniforms), so each build only sets the values and wraps the shader in a new ImageFilter.shader. Before, it created a new shader on every build.

This relies on the native filter copying the uniform values when it's created (ReusableFragmentShader::as_image_filter), so filters from different builds still compare unequal when the values change. Is reusing the shader like this the intended usage?

gaaclarke
gaaclarke previously approved these changes Oct 28, 2026

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

@gaaclarke gaaclarke added the test-exempt Exempt from tests. Overrides `missing-tests` label label Oct 28, 2026
@gaaclarke

Copy link
Copy Markdown
Member

test exempt: no logical change, moving away from deprecated api

@gaaclarke gaaclarke added the autosubmit Merge PR when tree becomes green via auto submit App label Oct 28, 2026
@auto-submit auto-submit Bot removed the autosubmit Merge PR when tree becomes green via auto submit App label Oct 28, 2026
@auto-submit

auto-submit Bot commented Oct 28, 2026

Copy link
Copy Markdown
Contributor

autosubmit label was removed for flutter/flutter/195372, because This PR has not met approval requirements for merging. The PR author is not a member of flutter-hackers and needs 1 more review(s) in order to merge this PR.

  • Merge guidelines: A PR needs at least one approved review if the author is already part of flutter-hackers or two member reviews if the author is not a member of flutter-hackers before re-applying the autosubmit label. Reviewers: If you left a comment approving, please use the "approve" review action instead.

@mohamedalsamrai

Copy link
Copy Markdown
Contributor Author

Thanks for the quick review

_fragmentShader = _StretchEffectShader._program!.fragmentShader();
_fragmentShader!.setFloat(2, maxStretchIntensity);
final _StretchEffectUniforms uniforms = _uniforms ??= _StretchEffectUniforms(
_StretchEffectShader._program!.fragmentShader(),

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.

this is a weird ownership model. who owns the shader object?

if _StretchEffectUniforms owns it, either call _StretchEffectShader._program!.fragmentShader() directly in the constructor of _StretchEffectUniforms, or pass a callback to create the fragmentShader and call that in the constructor.

if _StretchOverscrollEffectState owns it, it should keep a direct reference to the shader object and call dispose on it in the dispose

I lean toward the _StretchEffectUniforms owns it.

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.

also it looks like the original code will attempt to dispose old shader before creating a new one, but now it will keep using the old shader. Is this an intentional change?

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.

Good points. _StretchEffectUniforms now owns the shader: it creates it from the FragmentProgram and has a dispose() method, which the state calls from its own dispose().

And yes, reusing the shader is intentional. The slots are tied to a specific FragmentShader, so caching them (as gaaclarke suggested) means reusing the shader across builds instead of creating a new one each time, which the fragment shader docs also recommend. It's safe because ImageFilter.shader copies the uniform values, so each build's filter has that build's values and compares unequal when they change. I added a comment about this in the code.

@chunhtai chunhtai 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.

LGTM, just a nitpick for private constructor

/// Creates a shader from [program] and looks up its uniforms.
_StretchEffectUniforms(ui.FragmentProgram program) : this._(program.fragmentShader());

_StretchEffectUniforms._(this.shader)

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.

there is no need for this additional private constructor, the class itself is private already

@gaaclarke gaaclarke added the autosubmit Merge PR when tree becomes green via auto submit App label Oct 30, 2026
@mohamedalsamrai

Copy link
Copy Markdown
Contributor Author

@gaaclarke can you add CICD label?

@gaaclarke gaaclarke added the CICD Run CI/CD label Nov 2, 2026
@auto-submit auto-submit Bot removed the autosubmit Merge PR when tree becomes green via auto submit App label Nov 2, 2026
@auto-submit

auto-submit Bot commented Nov 2, 2026

Copy link
Copy Markdown
Contributor

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

  • The status or check suite Dashboard Checks has failed. Please fix the issues identified (or deflake) before re-applying this label.

@flutter-dashboard flutter-dashboard Bot removed the CICD Run CI/CD label Nov 2, 2026
@gaaclarke gaaclarke added CICD Run CI/CD autosubmit Merge PR when tree becomes green via auto submit App labels Nov 2, 2026
@auto-submit auto-submit Bot removed the autosubmit Merge PR when tree becomes green via auto submit App label Nov 2, 2026
@auto-submit

auto-submit Bot commented Nov 2, 2026

Copy link
Copy Markdown
Contributor

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

auto-submit Bot pushed a commit to flutter/packages that referenced this pull request Nov 2, 2026
…13445)

Moves `InkSparkle` from the index-based `FragmentShader.setFloat` to the named uniform API (`getUniformFloat`, `getUniformVec2`, `getUniformVec4`), as part of deprecating `setFloat` (flutter/flutter#195357).

`InkSparkle` looks up its 13 uniforms once, when it creates its shader, and sets them through those slots on each paint. Every uniform gets the same value as before, so there is no visual change. The named API is available in Flutter 3.47.0, the package's minimum, so the SDK constraint doesn't change.

flutter/flutter#195358 covers both `InkSparkle` and `StretchEffect`. Since Material is frozen in flutter/flutter (flutter/flutter#188444), the work is split: `StretchEffect` is in flutter/flutter#195372, and `InkSparkle` is here.

Part of flutter/flutter#195358

Version and CHANGELOG: adds a `pending_changelogs` entry with `version: patch`.

Tests: this is a refactor with no semantic change, so I think it qualifies for a test exemption. It's covered by the existing golden tests in `test/ink_sparkle_test.dart`, and a misspelled uniform name would throw when those tests paint.

## Pre-Review Checklist
@mohamedalsamrai

Copy link
Copy Markdown
Contributor Author

@gaaclarke ci done u can add the autosubmit

@gaaclarke gaaclarke added the autosubmit Merge PR when tree becomes green via auto submit App label Nov 2, 2026
@auto-submit auto-submit Bot removed the autosubmit Merge PR when tree becomes green via auto submit App label Nov 2, 2026
@auto-submit

auto-submit Bot commented Nov 2, 2026

Copy link
Copy Markdown
Contributor

auto label is removed for flutter/flutter/195372, Failed to enqueue flutter/flutter/195372 with HTTP 400: Pull request Required status check "Check Code Freeze" is queued..

@justinmc justinmc 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.

LGTM 👍 . The code freeze check appears stuck for now, but it's unrelated to this PR.

@justinmc justinmc added the autosubmit Merge PR when tree becomes green via auto submit App label Nov 2, 2026
@justinmc

justinmc commented Nov 2, 2026

Copy link
Copy Markdown
Contributor

And right when I wrote that it passed. Autosubmitting.

@gaaclarke

Copy link
Copy Markdown
Member

@justinmc yea I'm seeing this on other prs today. I reported a delay in the revert bot and jackson said there is a known delay is assigning bots today for some reason

@auto-submit
auto-submit Bot added this pull request to the merge queue Nov 2, 2026
Merged via the queue into flutter:master with commit 852534a Nov 3, 2026
41 of 42 checks passed
@flutter-dashboard flutter-dashboard Bot removed the autosubmit Merge PR when tree becomes green via auto submit App label Nov 3, 2026
gmackall pushed a commit to gmackall/flutter that referenced this pull request Nov 3, 2026
IMoves `StretchEffect` from the index-based `FragmentShader.setFloat` to
`getUniformFloat`, as part of deprecating `setFloat` (flutter#195357).

`StretchEffect` creates a new shader on every build, so it looks up its
four uniforms by name right after creating it, using a local variable
instead of repeating `_fragmentShader!`. `u_size` and `u_texture` are
still set by the engine for `ImageFilter.shader`. Every uniform gets the
same value as before, so there is no visual change.

The `InkSparkle` part of flutter#195358 is in flutter/packages#13445, since
Material is frozen in this repo (flutter#188444).

Part of flutter#195358

This is a refactor with no semantic change, so I think it qualifies for
`test-exempt`. It's covered by the existing golden test in
`stretch_effect_test.dart`, which runs in the Impeller shard, and a
misspelled uniform name would throw when that test runs.
## 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].
- [x] I listed at least one issue that this PR fixes in the description
above.
- [x] I updated/added relevant in-code documentation (doc comments with
`///`).
- [x] 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).
- [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.
- [x] All existing and new tests are passing.


<!-- Links -->
[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/CONTRIBUTING.ai.md
[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
[flutter/website]: https://github.com/flutter/website
[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

---------

Co-authored-by: gaaclarke <[email protected]>
auto-submit Bot pushed a commit to flutter/packages that referenced this pull request Nov 3, 2026
Roll Flutter from fcfb4f203eae to 934720261be0 (38 revisions)

flutter/flutter@fcfb4f2...9347202

2026-11-03 [email protected] Roll Skia from 5bceaa033999 to c733ad3e37cb (5 revisions) (flutter/flutter#195872)
2026-11-03 [email protected] Roll Skia from 34d2bf479385 to 5bceaa033999 (9 revisions) (flutter/flutter#195864)
2026-11-03 [email protected] fix: prefer pointer events over semantics tap for mouse clicks on web (flutter/flutter#191529)
2026-11-03 [email protected] [VPAT] Expose a semantics text field value while it is not being edited (flutter/flutter#195445)
2026-11-03 [email protected] [Impeller] Make a GL context current before constructing an AiksContext without a surface (flutter/flutter#195629)
2026-11-03 [email protected] [flutter_tools] Eliminate globals.dart from project.dart (flutter/flutter#194066)
2026-11-03 [email protected] [tool] Complete context runner elimination and migrate AppContext usages to ToolContext (flutter/flutter#190797)
2026-11-03 [email protected] [web] Unify font collections between CanvasKit and Skwasm (flutter/flutter#195539)
2026-11-03 [email protected] [Android] Replay predictive back state for retained engines (flutter/flutter#192286)
2026-11-03 [email protected] [vulkan]: Adds depth/stencil synchronization for subpasses (flutter/flutter#195689)
2026-11-03 [email protected] [web] Honor requested upscaling in instantiateImageCodecWithSize and ImageDescriptor.instantiateCodec (flutter/flutter#195836)
2026-11-03 [email protected] Roll Skia from 866b61ca92c9 to 34d2bf479385 (9 revisions) (flutter/flutter#195848)
2026-11-03 [email protected] Roll Fuchsia Test Scripts from xl1LWBuppzu-BnOzV... to B6zarykZp6PKWRueg... (flutter/flutter#195847)
2026-11-03 [email protected] Remove unused ffi (flutter/flutter#195710)
2026-11-02 [email protected] Revert: [Android] Let the raster thread use every core of a chip without little cores (flutter/flutter#195838)
2026-11-02 [email protected] Run 10 impeller iOS benchmarks as one devicelab task (flutter/flutter#195661)
2026-11-02 [email protected] Use named uniforms instead of setFloat in StretchEffect (flutter/flutter#195372)
2026-11-02 [email protected] [flutter_tools] Report when the browser disconnects during a web test run (flutter/flutter#192198)
2026-11-02 [email protected] fix: Resolve memory leak in image_list example (flutter/flutter#189526)
2026-11-02 [email protected] Only run the common local engine configurations in presubmit (flutter/flutter#195688)
2026-11-02 [email protected] Update gcloud_version to version [email protected] (flutter/flutter#195769)
2026-11-02 [email protected] [Impeller] Order the Vulkan copy-texture-to-texture layout transition after the copy (flutter/flutter#195814)
2026-11-02 [email protected] [Impeller] Order the Vulkan ResizeTexture layout transition after its blit (flutter/flutter#195721)
2026-11-02 [email protected] [web] Fix CanvasKit fragment shader uniforms being freed or mutated after recording (flutter/flutter#195718)
2026-11-02 [email protected] Roll Fuchsia Linux SDK from MqypI6RNyVZ_FLUE-... to T_-6-6X1lDMghhAvX... (flutter/flutter#195809)
2026-11-02 [email protected] [Android] Log why Impeller falls back to OpenGLES (flutter/flutter#194764)
2026-11-02 [email protected] Roll Skia from 3f35fb6e591b to 866b61ca92c9 (11 revisions) (flutter/flutter#195802)
2026-11-02 [email protected] Roll Skia from 3f35fb6e591b to 866b61ca92c9 (11 revisions) (flutter/flutter#195802)
2026-11-02 [email protected] Only run the common local engine configurations in presubmit (flutter/flutter#195688)
2026-11-02 [email protected] Balance tool_integration_tests subshards by expected duration (flutter/flutter#195628)
2026-11-02 [email protected] [web_paragraph] Map code units without a cluster to a cluster (flutter/flutter#195384)
2026-11-02 [email protected] [web] Hide synthetic autofill inputs from assistive technology (flutter/flutter#195618)
2026-11-02 [email protected] Roll Packages from 3742a4e to 3f34a6f (8 revisions) (flutter/flutter#195795)
2026-11-02 [email protected] [web] Unskip web tests that pass on CanvasKit and Skwasm: platform_view_test, route_notification_messages_test (flutter/flutter#195605)
2026-11-02 [email protected] Roll Skia from e5ad3868b8db to 3f35fb6e591b (1 revision) (flutter/flutter#195789)
2026-11-02 [email protected] Roll Skia from f5215e74aa16 to e5ad3868b8db (3 revisions) (flutter/flutter#195779)
2026-11-02 [email protected] Roll Skia from e43478a2f9f8 to f5215e74aa16 (1 revision) (flutter/flutter#195776)
2026-11-01 [email protected] Roll Fuchsia Linux SDK from kW0A0xxNkqf38ljm4... to MqypI6RNyVZ_FLUE-... (flutter/flutter#195768)

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
...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CICD Run CI/CD framework flutter/packages/flutter repository. See also f: labels. missing-tests test-exempt Exempt from tests. Overrides `missing-tests` label

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants