Repository navigation
Use named uniforms instead of setFloat in StretchEffect - #195372
Conversation
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
|
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 |
|
This pull request contains changes to Material or Cupertino, which are currently frozen in this repository. Changes should be made in Please refer to #188444 for instructions. |
There was a problem hiding this comment.
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.
…able in StretchEffect
There was a problem hiding this comment.
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.
| 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); | ||
| } |
There was a problem hiding this comment.
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
- Suggest simplification and refactoring to enhance readability and maintainability. (link)
| if (widget.axis == Axis.vertical) { | ||
| _fragmentShader!.setFloat(3, 0.0); | ||
| _fragmentShader!.setFloat(4, widget.stretchStrength); | ||
| shader.getUniformFloat('u_overscroll_x').set(0.0); |
There was a problem hiding this comment.
I would prefer that we stored the shader.getUniformFloat result an ivar. That way during animations we aren't constantly resolving the slot.
There was a problem hiding this comment.
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?
|
test exempt: no logical change, moving away from deprecated api |
|
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.
|
|
Thanks for the quick review |
| _fragmentShader = _StretchEffectShader._program!.fragmentShader(); | ||
| _fragmentShader!.setFloat(2, maxStretchIntensity); | ||
| final _StretchEffectUniforms uniforms = _uniforms ??= _StretchEffectUniforms( | ||
| _StretchEffectShader._program!.fragmentShader(), |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
left a comment
There was a problem hiding this comment.
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) |
There was a problem hiding this comment.
there is no need for this additional private constructor, the class itself is private already
|
@gaaclarke can you add CICD label? |
|
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.
|
|
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. |
…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
|
@gaaclarke ci done u can add the autosubmit |
|
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
left a comment
There was a problem hiding this comment.
LGTM 👍 . The code freeze check appears stuck for now, but it's unrelated to this PR.
|
And right when I wrote that it passed. Autosubmitting. |
|
@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 |
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]>
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 ...
IMoves
StretchEffectfrom the index-basedFragmentShader.setFloattogetUniformFloat, as part of deprecatingsetFloat(#195357).StretchEffectcreates 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_sizeandu_textureare still set by the engine forImageFilter.shader. Every uniform gets the same value as before, so there is no visual change.The
InkSparklepart 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 instretch_effect_test.dart, which runs in the Impeller shard, and a misspelled uniform name would throw when that test runs.Pre-launch Checklist
///).