[flutter_tools] write SkSL file to local file#53859
[flutter_tools] write SkSL file to local file#53859jonahwilliams merged 5 commits intoflutter:masterfrom
Conversation
|
This one is ready for review, I split off the simpler changes from the original PR |
liyuqian
left a comment
There was a problem hiding this comment.
Mostly looks good! See some minor comments below.
| ); | ||
| return; | ||
| } | ||
| final File outputFile = globals.fsUtils.getUniqueFile( |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.)
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
For example, I don't know of a way to do assets per device in an appbundle, only per ABI
| ); | ||
| final Map<String, Object> manifest = <String, Object>{ | ||
| 'device': getNameForTargetPlatform(await flutterDevices.first.device.targetPlatform), | ||
| 'version': globals.flutterVersion.engineRevision, |
There was a problem hiding this comment.
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.
| 'sksl', | ||
| ); | ||
| final Map<String, Object> manifest = <String, Object>{ | ||
| 'device': getNameForTargetPlatform(await flutterDevices.first.device.targetPlatform), |
There was a problem hiding this comment.
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
| CommandHelpOption _M; | ||
| CommandHelpOption get M => _M ??= _makeOption( | ||
| 'M', | ||
| 'Write SkSL shaders to a file.', |
There was a problem hiding this comment.
Consider noting where the output will go.
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