Skip to content

[flutter_tools] write SkSL file to local file#53859

Merged
jonahwilliams merged 5 commits intoflutter:masterfrom
jonahwilliams:add_sksl_protocol_dump
Apr 7, 2020
Merged

[flutter_tools] write SkSL file to local file#53859
jonahwilliams merged 5 commits intoflutter:masterfrom
jonahwilliams:add_sksl_protocol_dump

Conversation

@jonahwilliams
Copy link
Contributor

@jonahwilliams jonahwilliams commented Apr 2, 2020

Description

Update the tool to support writing SkSL to a unique file, along with the current engine hash and target platform. This allows users to create multiple sksl files for a given build (different mains, platforms, flavors) and will simplify the problem of providing as a build input by keeping it bundled as a single file.

Fixes #53116

Split off from #53278

@fluttergithubbot fluttergithubbot added tool Affects the "flutter" command-line tool. See also t: labels. work in progress; do not review labels Apr 2, 2020
@jonahwilliams jonahwilliams changed the title [flutter_tools] write SkSL file to local project [flutter_tools] write SkSL file to local file Apr 2, 2020
@jonahwilliams jonahwilliams marked this pull request as ready for review April 3, 2020 01:02
@jonahwilliams
Copy link
Contributor Author

This one is ready for review, I split off the simpler changes from the original PR

Copy link
Contributor

@liyuqian liyuqian left a comment

Choose a reason for hiding this comment

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

Mostly looks good! See some minor comments below.

);
return;
}
final File outputFile = globals.fsUtils.getUniqueFile(
Copy link
Contributor

Choose a reason for hiding this comment

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

Just to confirm: if I run this 10 times with 10 devices, I'll get 10 different output files, each of which will be a plain text file with all the Base64 encoded SkSLs of that device, right?

Copy link
Contributor

Choose a reason for hiding this comment

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

Another question: if I run this twice on the same device, does it generate two files with the exact same content? (If so, that looks a little duplicative.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, both are correct. Are you concerned about management of these files? I am too, I'm not quite sure what a reasonable workflow for these files will be

Copy link
Contributor

Choose a reason for hiding this comment

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

What if we group the SkSLs with the device id, and use the newer SkSL to rewrite the older SkSL if both the SkSL key and device id matches?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Tracking and managing these files is a bit too high level for the command line API. once they are written a user can (and should) be allowed to move and rename them

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm curious what would happen for the next PR that package the SkSLs into assets. If each run generates a different json map of SkSLs, how are you going to merge those maps and put them into asset? Do you have to deduplicate by then since you can only have one SkSL file per key/filename?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think to start with, it would be fair to say that the tool can support including a single one of these files in the build, something like:

flutter build apk --release --include=sksl=path/tp/flutter2.sksl

That allows for different shaders for iOS and Android, but requires them to be the same across Android versions.

If we wanted a workflow beyond that, I think it definitely needs more design work. I think that will be easier to do once the entire thing is wired up end to end.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For example, I don't know of a way to do assets per device in an appbundle, only per ABI

Copy link
Contributor

Choose a reason for hiding this comment

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

Sounds good. LGTM!

);
final Map<String, Object> manifest = <String, Object>{
'device': getNameForTargetPlatform(await flutterDevices.first.device.targetPlatform),
'version': globals.flutterVersion.engineRevision,
Copy link
Contributor

Choose a reason for hiding this comment

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

This version should be the git hash for the engine repo, right? It might be nice to rename version to engine_revision to make that more clear.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

'sksl',
);
final Map<String, Object> manifest = <String, Object>{
'device': getNameForTargetPlatform(await flutterDevices.first.device.targetPlatform),
Copy link
Contributor

Choose a reason for hiding this comment

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

Other than the targetPlatform, it might be nice to also record the exact device type, such as whether it's a Moto G4, or a Pixel 4. I'm not sure if our tools already have that information. If not, maybe just write down the device name, i.e., the first column of the following flutter devices output:

GM1910                 • 370d7342                                 • android-arm64  • Android 10 (API 29)
Moto G 4               • ZY2245H6KS                               • android-arm    • Android 7.0 (API 24)
SixDegreesOfSeparation • 2b398993d86f42b05511b330a89ae65289832852 • ios            • iOS 12.4.1
Chrome                 • chrome                                   • web-javascript • Google Chrome 80.0.3987.149
Web Server             • web-server                               • web-javascript • Flutter Tools

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

Copy link
Member

@zanderso zanderso left a comment

Choose a reason for hiding this comment

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

lgtm after addressing @liyuqian's comments.

CommandHelpOption _M;
CommandHelpOption get M => _M ??= _makeOption(
'M',
'Write SkSL shaders to a file.',
Copy link
Member

Choose a reason for hiding this comment

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

Consider noting where the output will go.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@jonahwilliams jonahwilliams requested a review from liyuqian April 6, 2020 17:45
@jonahwilliams jonahwilliams merged commit 08fe78f into flutter:master Apr 7, 2020
@jonahwilliams jonahwilliams deleted the add_sksl_protocol_dump branch April 7, 2020 19:17
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jul 31, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

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.

Connect flutter tools to VM service callback to pull sksl files from device

5 participants