Skip to content

Conversation

@zanderso
Copy link
Member

@zanderso zanderso commented Mar 19, 2023

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
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 🙃

Copy link
Member Author

@zanderso zanderso Mar 20, 2023

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
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
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?

Copy link
Contributor

@jonahwilliams jonahwilliams Mar 20, 2023

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
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
Contributor

Choose a reason for hiding this comment

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

Copy link
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
Copy link
Contributor

@jonahwilliams jonahwilliams left a comment

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