Remove LineContents experimental AA line shader that is no longer used#189619
Conversation
github-actions
Bot
added
platform-android
|
/gemini review |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request removes the experimental antialiased lines feature from the Impeller rendering engine, which includes deleting the associated shaders, line contents source files, unit tests, and platform-specific configuration flags. Feedback on the changes points out a potential regression in platform_view_android.cc, where the impeller_use_sdfs flag is no longer propagated to settings.impeller_flags after the removal of the antialiased lines setting.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request removes the experimental antialiased lines feature from Impeller. The changes include deleting the line shaders (line.frag, line.vert), the LineContents class, and associated unit and golden tests. Additionally, all related command-line switches, settings, and platform-specific flags across Android, iOS, and the embedder have been removed. There are no review comments to evaluate, and I have no additional feedback to provide.
b-luk
changed the title
LineContents experimental AA line shader that is no longer used
|
Warning Gemini encountered an error creating the review. You can try again by commenting |
flar
left a comment
flar
left a comment
There was a problem hiding this comment.
I pose some questions about whether we should be transferring new flags on Android that had not yet been transferred explicitly (unlike other platforms where the transfer was explicit). Perhaps if we decide to go this route we should make it a separate PR that introduces a new transfer of capabilities on Android?
| .antialiased_lines = | ||
| settings.impeller_flags.antialiased_lines, | ||
| }, | ||
| .impeller_flags = settings.impeller_flags, |
There was a problem hiding this comment.
This is an odd translation.
Before, the flags were set to all defaults, but copy over the AA lines flag.
Now all of the remaining flags are copied, but they didn't use to be copied.
This line of code predates a refactoring that happened a year ago so it's not clear why individual flags were targeted for copying, but it would be worth looking at how these flags get used and whether or not extraneous flags (like the SDF flag) might get in the way of Android contexts.
There was a problem hiding this comment.
Done. For this, and also for the other case you commented on, I changed it to not forward the remaining impeller_flags.
I think forwarding these flags is something we may actually want to do. But for now, the only existing flag is use_sdfs, and that is not enabled for Android. So there's no current reason why it needs to be forwarded here.
I removed this forwarding to better match the existing behavior. Even though I think it should probably be forwarded, I think you are right that making this change in this PR is odd. This PR should purely be about removing the LineContents implementation. So I think it's better to not sneak in other small changes like this, even if it is likely a behavioral no-op with the current flags.
There was a problem hiding this comment.
In general, I agree with your assessment, but didn't think it was for this specific PR to change behavior on.
| settings.enable_surface_control = p_settings.enable_surface_control; | ||
| settings.impeller_flags.antialiased_lines = | ||
| p_settings.impeller_antialiased_lines; | ||
| settings.impeller_flags.use_sdfs = p_settings.impeller_use_sdfs; |
There was a problem hiding this comment.
This would awaken all use of SDFs on Android whereas before only the AA lines were transferred. Are there potential ramifications of this? Would it be better to just delete this flag transfer and decide independently when and why to transfer sdf to the android platform?
There was a problem hiding this comment.
Done. I don't think there are potential ramifications because use_sdfs isn't enabled for Android. But I think it's better to remove this to keep the PR focused. Also see my other comment.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request removes the experimental antialiased lines feature from Impeller. The changes include deleting the LineContents class, its vertex and fragment shaders, related unit tests, command-line switches, platform-specific configuration flags across Android and iOS, and Malioc shader analysis entries. There are no review comments, so I have no feedback to provide.
|
autosubmit label was removed for flutter/flutter/189619, 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
removed
the
autosubmit
|
autosubmit label was removed for flutter/flutter/189619, 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
removed
the
autosubmit
flutter-dashboard
Bot
removed
the
autosubmit
Roll Flutter from fc1ad955f164 to 8005793c3562 (40 revisions) flutter/flutter@fc1ad95...8005793 2026-07-18 [email protected] Remove `LineContents ` experimental AA line shader that is no longer used (flutter/flutter#189619) 2026-07-18 [email protected] Roll Skia from 9468e96cc40f to ba90f98535de (8 revisions) (flutter/flutter#189680) 2026-07-17 [email protected] Roll Dart SDK from 33e4b71e984b to 666e1e2133b7 (2 revisions) (flutter/flutter#189673) 2026-07-17 [email protected] [iOS] Fix potential use-after-free in a11y bridge channel handler (flutter/flutter#189637) 2026-07-17 [email protected] Migrate dev/tools skill validation to new dart_skills_lint API (flutter/flutter#189626) 2026-07-17 [email protected] Add validation for required fields during xcodebuild (flutter/flutter#187772) 2026-07-17 [email protected] Upgrade android_hardware_smoke_test CI to run instrumented tests (flutter/flutter#189390) 2026-07-17 [email protected] Roll Dart SDK from 0867cb1897b5 to 33e4b71e984b (1 revision) (flutter/flutter#189661) 2026-07-17 [email protected] Roll Skia from 702c3e790232 to 9468e96cc40f (1 revision) (flutter/flutter#189660) 2026-07-17 [email protected] Roll Packages from 9f95026 to 4fdc766 (11 revisions) (flutter/flutter#189659) 2026-07-17 [email protected] Roll Skia from 2e4a3ae035cd to 702c3e790232 (1 revision) (flutter/flutter#189655) 2026-07-17 [email protected] [macOS] Add FlutterPluginRegistrar.valuePublished(byPlugin:) just like iOS (flutter/flutter#189614) 2026-07-17 [email protected] Roll Dart SDK from c69d138c9646 to 0867cb1897b5 (2 revisions) (flutter/flutter#189649) 2026-07-17 [email protected] Roll Skia from 4f5aca109c87 to 2e4a3ae035cd (3 revisions) (flutter/flutter#189646) 2026-07-17 [email protected] [iOS] Fix missing nil checks and improve SemanticsObject bridge API (flutter/flutter#189630) 2026-07-17 [email protected] Roll Skia from 37c5e6b26aee to 4f5aca109c87 (19 revisions) (flutter/flutter#189638) 2026-07-17 [email protected] Roll Fuchsia Linux SDK from lLFbh5kFWbUGgC9Ek... to NL8xtzr8cxr5E8r8E... (flutter/flutter#189632) 2026-07-17 [email protected] Add blendMode parameter to RawImage and RenderImage (flutter/flutter#185938) 2026-07-17 [email protected] [web] Cache WASM network requests in flutter test (flutter/flutter#189623) 2026-07-17 [email protected] android_hardware_smoke_tests: Apply semantic line breaks to readme (flutter/flutter#189613) 2026-07-16 [email protected] ci(github-actions): resolve zizmor github-env findings in composite flutter actions (flutter/flutter#189602) 2026-07-16 [email protected] Remove obsolete packages analysis flag (flutter/flutter#189525) 2026-07-16 [email protected] Roll Dart SDK from e24870ff15bc to c69d138c9646 (2 revisions) (flutter/flutter#189597) 2026-07-16 [email protected] [fuchsia][iwyu] Remove transitive include of fuchsia.input.report. (flutter/flutter#188891) 2026-07-16 [email protected] Remove unnecessary value key hack in `key.dart` (flutter/flutter#189291) 2026-07-16 [email protected] Roll Dart SDK from 81306a2ed317 to e24870ff15bc (1 revision) (flutter/flutter#189569) 2026-07-16 [email protected] [Impeller] Fix atlas growth test (flutter/flutter#189533) 2026-07-16 [email protected] Roll Dart SDK from d402ff7c9c84 to 81306a2ed317 (4 revisions) (flutter/flutter#189558) 2026-07-16 [email protected] Move shared darwin plugin tests into own builder (flutter/flutter#189411) 2026-07-16 [email protected] Roll Skia from ab2410bc857c to 37c5e6b26aee (19 revisions) (flutter/flutter#189549) 2026-07-16 [email protected] [ios] Fix //flutter:unittests build for physical devices (flutter/flutter#189543) 2026-07-16 [email protected] Fix lower DragTarget not being recognized in overlapping targets (flutter/flutter#188979) 2026-07-16 [email protected] Relocate cupertino samples that were under widgets/ (flutter/flutter#188876) 2026-07-15 [email protected] [iOS] Migrate DisplayLinkManager to a shared instance (flutter/flutter#189492) 2026-07-15 [email protected] [Impeller] Call glfwTerminate during global test environment teardown if a playground test called glfwInit (flutter/flutter#189523) 2026-07-15 [email protected] Roll pub packages (flutter/flutter#189515) 2026-07-15 [email protected] Roll Packages from ad2eab1 to 9f95026 (8 revisions) (flutter/flutter#189509) 2026-07-15 [email protected] add `@nonVirtual` to `RenderObject.attached`, fix `WidgetTester.hasRunningAnimations` (flutter/flutter#186832) 2026-07-15 [email protected] Roll vulkan-deps to 0582f446e54a (flutter/flutter#188524) 2026-07-15 [email protected] Roll Dart SDK from 0c408ff6dce9 to d402ff7c9c84 (1 revision) (flutter/flutter#189497) If this roll has caused a breakage, revert this CL and stop the roller 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. ...
2 participants
After #188514,
LineContentscan no longer be enabled. This deletes its code and its associatedantialiased_linesflag.See #188514 (comment) for some more context.
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.