Skip to content

[flutter_tool] Support disabling Impeller - #122960

Merged
zanderso merged 1 commit into
flutter:masterfrom
zanderso:disable-impeller-flag
Mar 21, 2023
Merged

zanderso merged 1 commit into
flutter:masterfrom
zanderso:disable-impeller-flag

Conversation

@zanderso

@zanderso zanderso commented Mar 19, 2023 •

Copy link
Copy Markdown
Member

Since Impeller will be default-on on some platforms but not others, this PR allows explicitly enabling, disabling, or using the platform default when --enable-impeller, --no-enable-impeller, or no flag is passed, respectively.

@flutter-dashboard flutter-dashboard Bot added the tool Affects the "flutter" command-line tool. See also t: labels. label Mar 19, 2023
@zanderso
zanderso force-pushed the disable-impeller-flag branch 2 times, most recently from 2fa410d to 67da160 Compare March 19, 2023 02:59
@zanderso
zanderso requested a review from jonahwilliams March 19, 2023 18:38

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.

don't we still need to pass --enable-impeller sometimes? I'm having a bit of a hard time wrapping my head around two independent flags 🙃

@zanderso zanderso Mar 20, 2023 •

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This function creates the flags for iOS and iOS simulator. The --enable-impeller(=true) flag is always a no-op there after the engine flag flip. It can't override the Info.plist setting. When there is no Info.plist setting either way, the engine flag --enable-impeller=false will disable Impeller on iOS and iOS sim.

After this change, when --enable-impeller is passed on Android, --enable-impeller will still be passed to the engine.

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.

But we're not changing the info.plist value, just the default if not specified right? Can we update --no-enable-impeller to pass --enable-impeller=false?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What I was having trouble figuring out was how to handle the case where the --enable-impeller flag is absent on the command line. The default value that we'd like the flag to have differs depending on the target device, so DebuggingOptions would need to know both the value of the flag, and whether the flag was present or absent on the command line. Do you have thoughts on whether that's preferable to the --disable-impeller flag?

@jonahwilliams jonahwilliams Mar 20, 2023 •

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.

I don't think debugging options actually needs to know, the only difference for the tooling is what shaders are generated and we've so far worked around that by just generating both variants

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.

We could instead update this logic to always generate both variants for iOS and call it good?

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.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Switched to an enum as discussed offline. PTAL.

@zanderso
zanderso force-pushed the disable-impeller-flag branch from 67da160 to a4ebb6d Compare March 20, 2023 21:57
@zanderso zanderso changed the title [flutter_tool] Adds a flag to disable Impeller [flutter_tool] Support disabling Impeller Mar 20, 2023

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

@zanderso
zanderso force-pushed the disable-impeller-flag branch 2 times, most recently from 97185a2 to 89cade1 Compare March 21, 2023 03:02
@zanderso
zanderso force-pushed the disable-impeller-flag branch from 89cade1 to 99f49b2 Compare March 21, 2023 04:06
@zanderso
zanderso merged commit 7e88acf into flutter:master Mar 21, 2023
@zanderso
zanderso deleted the disable-impeller-flag branch March 21, 2023 05:23
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Mar 21, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request May 10, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request May 10, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

tool Affects the "flutter" command-line tool. See also t: labels.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants