Move strech fragment shader to widget layer#187960
Conversation
There was a problem hiding this comment.
Code Review
This pull request refactors how framework shaders are bundled by separating material shaders (ink_sparkle.frag) and widgets shaders (stretch_effect.frag) into their respective directories instead of treating them all as material shaders. The corresponding asset bundle and asset tests are updated to mock and assert these new paths. The review feedback suggests refactoring the updated tests to use loops over the shader definitions to reduce boilerplate and improve readability.
| result.add( | ||
| _Asset( | ||
| baseDir: materialShaderPath, | ||
| relativeUri: Uri(path: 'ink_sparkle.frag'), |
There was a problem hiding this comment.
This needs to be removed when the material in flutter/flutter is removed.
The material_ui need to attach this fragment shader as part of its pubspec so that the InkSparkle effect will use the version in the material_ui
There was a problem hiding this comment.
Can you leave a TODO here with a tracking issue?
justinmc
left a comment
There was a problem hiding this comment.
LGTM 👍 . I agree that the shader should be in Widgets. Besides the fact that it's used there, it's really system UI that Android devs will want even if they aren't using Material.
But the description says fixes #187807, does this PR still need to add IMPELLER_OPENGLES_UNFLIPPED_DEPRECATED to the shader?
We only need this if material_ui provides this file. If we move it to widget layer, it will by in sync with impeller breaking change, so we don't need to worry about backward and forward compatibility |
|
Google tests are failing, but it looks like it's just a couple of tests that are expecting the shaders/stretch_effect.frag file to exist among some assets. Probably we need a g3fix that just removes the filename from the test. Or should it still exist? |
|
I think we should deprecate the public classes in the file that is being moved in material_ui when it is ready to accept changes, otherwise it will be breaking to just remove it. I've marked this PR as not ready to port to flutter/packages yet. The changes in the widgets library will remain here after. |
|
We can keep the file in material just for the code freeze and create another issue to remove it from both flutter/flutter and flutter/package later |
|
removing the do not port label as this is non breaking and the removal of the file will be a follow up |
There was a problem hiding this comment.
We can keep the file in material just for the code freeze and create another issue to remove it from both flutter/flutter and flutter/package later
That makes sense to me 👍
I reran a failing test that seemed like it might have been a flake, not sure though.
This fragment is used in widget layer. With the decoupling, this fragment should stay in flutter/flutter
In this pr, I moved the shader from material/shaders to widgets/shaders, also adjust the flutter tooling to use the updated file path
fixes #187807
Pre-launch Checklist
///).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. Comments from the
gemini-code-assistbot 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.