Skip to content

Retry "Implements the Android native stretch effect as a fragment shader (Impeller-only)." #173885

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Open
wants to merge 40 commits into
base: master
Choose a base branch
from
Open

Retry "Implements the Android native stretch effect as a fragment shader (Impeller-only)." #173885

wants to merge 40 commits into from
+1,250 −710

Conversation

MTtankkeo
Copy link
Contributor

@MTtankkeo MTtankkeo commented Aug 15, 2025 •
edited
Loading

This PR retries the previously reverted PR #169293.

Note

This feature was originally intended to be Impeller-only, and it was agreed that no additional engine changes were necessary. However, during testing on the Metal backend, a shader loading issue occurred. In my view, this seems to be a simple problem caused by the shader not being included in the engine’s runtime_stages.

Pre-launch Checklist

  • I read the [Contributor Guide] and followed the process outlined there for submitting PRs.
  • 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 documentation (doc comments with ///).
  • 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.

MTtankkeo and others added 30 commits May 23, 2025 02:19
@MTtankkeo
Add stretch overscroll effect identical to Android native
c2a6ef1
@MTtankkeo
Updated StretchOverscrollEffect docs and renamed related fields
79bf0e3
@MTtankkeo
Update .frag file about impeller emulator env
90a2177
@MTtankkeo @dkwingsmt
Update packages/flutter/lib/src/material/stretch_overscroll_effect.dart
de7d34c 
Co-authored-by: Tong Mu <dkwingsmt@users.noreply.github.com>
@MTtankkeo @dkwingsmt
Update packages/flutter/lib/src/material/stretch_overscroll_effect.dart
e33cbcc 
Co-authored-by: Tong Mu <dkwingsmt@users.noreply.github.com>
@MTtankkeo @dkwingsmt
Update packages/flutter/lib/src/widgets/overscroll_indicator.dart
067d9cd 
Co-authored-by: Tong Mu <dkwingsmt@users.noreply.github.com>
@MTtankkeo @Piinks
Update packages/flutter/lib/src/widgets/stretch_overscroll_effect.dart
1d0d293 
Co-authored-by: Kate Lovett <katelovett@google.com>
@MTtankkeo @Piinks
Update packages/flutter/lib/src/widgets/stretch_overscroll_effect.dart
f13235a 
Co-authored-by: Kate Lovett <katelovett@google.com>
@MTtankkeo @Piinks
Update packages/flutter/lib/src/widgets/image_filter.dart
f907071 
Co-authored-by: Kate Lovett <katelovett@google.com>
@MTtankkeo
Refactor stretch shader to consider only one overscroll axis
2a88814
@MTtankkeo @Piinks
Update packages/flutter/lib/src/widgets/stretch_overscroll_effect.dart
1d92ed6 
Co-authored-by: Kate Lovett <katelovett@google.com>
@MTtankkeo
Merge remote-tracking branch 'upstream/master' into stretch
a11a6c8
@MTtankkeo
Reverted to avoid using a separate golden image name
7297766
@MTtankkeo @dkwingsmt
Update packages/flutter/lib/src/material/shaders/stretch_effect.frag
1275943 
Co-authored-by: Tong Mu <dkwingsmt@users.noreply.github.com>
@MTtankkeo
Commit for review and feedback
1f46a17
@MTtankkeo
Merge branch 'master' into stretch
45f2150
@MTtankkeo
Merge branch 'master' into stretch
73907db
@MTtankkeo
Merge branch 'master' into stretch
ee85d62
@MTtankkeo @dkwingsmt
Update packages/flutter/test/widgets/overscroll_stretch_indicator_tes…
457106d 
…t.dart

Co-authored-by: Tong Mu <dkwingsmt@users.noreply.github.com>
@MTtankkeo
Commit for review and feedback
82ac0c9
@MTtankkeo
Merge remote-tracking branch 'upstream/master' into stretch
f8c5ea0
@MTtankkeo
Fixed about analyze
32d0fda
@MTtankkeo
Merge branch 'stretch' of https://github.com/MTtankkeo/flutter into s…
be32b8e 
…tretch
@MTtankkeo
Merge branch 'master' into stretch
922db22
@MTtankkeo
commit for reverse scrolling
e57c895
@MTtankkeo
Merge branch 'master' into stretch
22c017b
@MTtankkeo
Merge branch 'master' into stretch
f7f1404
@MTtankkeo
Fixed issues reported by flutter analyze
aea54bc
@MTtankkeo
Merge remote-tracking branch 'upstream/master' into stretch
89b474b
@MTtankkeo
Make unstable widget private and add public wrapper
375bdbe
@github-actions github-actions bot added tool Affects the "flutter" command-line tool. See also t: labels. framework flutter/packages/flutter repository. See also f: labels. engine flutter/engine repository. See also e: labels. f: scrolling Viewports, list views, slivers, etc. e: impeller Impeller rendering backend issues and features requests labels Aug 15, 2025
@MTtankkeo MTtankkeo mentioned this pull request Aug 15, 2025
Merged
9 tasks
gemini-code-assist[bot]
gemini-code-assist bot reviewed Aug 15, 2025
View reviewed changes
Copy link
Contributor

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

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 appears to implement the Android native stretch effect using a fragment shader for Impeller, despite the "Revert" in the title. I have reviewed it as an implementation. The changes introduce a new StretchEffect widget that uses a GLSL shader to create a more visually appealing overscroll effect, with a fallback to a Transform-based effect on platforms where shaders are not supported. The implementation looks solid, but I have identified a potential null-pointer exception, a bug in the shader loading logic, and an opportunity to improve code maintainability by replacing magic numbers with named constants. Addressing these points will improve the robustness and clarity of the new feature.

@MTtankkeo
Copy link
Contributor Author

MTtankkeo commented Aug 15, 2025 •
edited
Loading

The stretch_effect.frag shader is required for the runtime_stage Impeller tests. I don't have the ability to format the shader file locally. Could someone please add this shader to flutter/impeller/fixtures?

@MTtankkeo MTtankkeo changed the title Revert "Implements the Android native stretch effect as a fragment shader (Impeller-only)." Retry "Implements the Android native stretch effect as a fragment shader (Impeller-only)." Aug 15, 2025
@reidbaker reidbaker requested a review from justinmc August 18, 2025 14:31
@gmackall gmackall mentioned this pull request Aug 18, 2025
Open
@MTtankkeo
Copy link
Contributor Author

I’ve formatted the shader file and added it to the engine.

@justinmc
Copy link
Contributor

We're working on a fix on the Google test side. It seems like you won't need to add the shader to the Impeller fixtures (be889ea).

@justinmc
Copy link
Contributor

The fix on our side has landed, now running the Google tests.

@MTtankkeo
Copy link
Contributor Author

MTtankkeo commented Aug 21, 2025 •
edited
Loading

@justinmc Should we revert the recent changes? Is this just a Google test issue rather than a configuration problem?

@justinmc
Copy link
Contributor

@MTtankkeo Yes, can you revert be889ea? Then assuming it still passes the Google tests I think we'll be good to reland.

@github-actions github-actions bot removed engine flutter/engine repository. See also e: labels. e: impeller Impeller rendering backend issues and features requests labels Aug 23, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@justinmc justinmc Awaiting requested review from justinmc

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

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
f: scrolling Viewports, list views, slivers, etc. framework flutter/packages/flutter repository. See also f: labels. tool Affects the "flutter" command-line tool. See also t: labels.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@MTtankkeo @justinmc @dkwingsmt