Skip to content

[flutter_tool] Build shaders as .iplr and use FragmentProgram.fromAsset for ink_sparkle#108071

Merged
zanderso merged 1 commit intoflutter:masterfrom
zanderso:fp-from-asset
Jul 22, 2022
Merged

[flutter_tool] Build shaders as .iplr and use FragmentProgram.fromAsset for ink_sparkle#108071
zanderso merged 1 commit intoflutter:masterfrom
zanderso:fp-from-asset

Conversation

@zanderso
Copy link
Member

@zanderso zanderso commented Jul 21, 2022 •
edited
Loading

This PR causes the tool to bundle shaders as SkSL in a .iplr container, and to use FragmentProgram.fromAsset to load the ink_sparkle shader.

This will require fixes to the internal build, which are here: cl/462521386 cc @chingjun

After this lands we can start deleting the transpiler and old API.

@flutter-dashboard flutter-dashboard bot added the f: material design flutter/packages/flutter/material repository. label Jul 21, 2022
@flutter-dashboard
Copy link

flutter-dashboard bot commented Jul 21, 2022

It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact Hixie on the #hackers channel in Chat (don't just cc him here, he won't see it! He's on Discord!).

If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix?

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

@flutter-dashboard flutter-dashboard bot added framework flutter/packages/flutter repository. See also f: labels. tool Affects the "flutter" command-line tool. See also t: labels. labels Jul 21, 2022
@zanderso zanderso marked this pull request as draft July 21, 2022 03:49
@zanderso zanderso mentioned this pull request Jul 21, 2022
Merged
@zanderso zanderso force-pushed the fp-from-asset branch 2 times, most recently from be726b9 to 9a3754a Compare July 21, 2022 16:51
@zanderso zanderso marked this pull request as ready for review July 21, 2022 16:51
@zanderso zanderso requested a review from jonahwilliams July 21, 2022 16:51
@zanderso
Copy link
Member Author

zanderso commented Jul 21, 2022

@jonahwilliams Switching over to FragmentProgram.fromAsset for ink_sparkle is going to break the internal build, so I'd suggest we land a simple change like this before the one you have going to hopefully make the internal fix simpler/easier.

@jonahwilliams
Copy link
Contributor

jonahwilliams commented Jul 21, 2022

That SGTM

jonahwilliams
jonahwilliams reviewed Jul 21, 2022
View reviewed changes
packages/flutter_tools/lib/src/build_system/targets/shader_compiler.dart Outdated
Copy link
Contributor

@jonahwilliams jonahwilliams Jul 21, 2022

Choose a reason for hiding this comment

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

I think we have a safe delete helper? Though its not exactly harmless, they end up bundled into the app

Copy link
Member Author

@zanderso zanderso Jul 21, 2022

Choose a reason for hiding this comment

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

Replaced with ErrorHandlingFileSystem.deleteIfExists, which will throwToolExit if the delete fails.

jonahwilliams
jonahwilliams approved these changes Jul 21, 2022
View reviewed changes
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 withnit

@zanderso
Copy link
Member Author

zanderso commented Jul 21, 2022

This is going to be easier to roll internally after we use a flag rather than the output file extension to indicating wrapping in the .iplr format, so I am switching this back to draft until that's done.

@zanderso zanderso marked this pull request as draft July 21, 2022 19:18
@flutter-dashboard
Copy link

flutter-dashboard bot commented Jul 21, 2022

This pull request has been changed to a draft. The currently pending flutter-gold status will not be able to resolve until a new commit is pushed or the change is marked ready for review again.

For more guidance, visit Writing a golden file test for package:flutter.

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

@zanderso zanderso mentioned this pull request Jul 21, 2022
Merged
@zanderso zanderso marked this pull request as ready for review July 22, 2022 03:46
@zanderso zanderso merged commit dd20919 into flutter:master Jul 22, 2022
@zanderso zanderso deleted the fp-from-asset branch July 22, 2022 03:50
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Jul 22, 2022
Closed
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jul 22, 2022
@engine-flutter-autoroll
dd20919 [flutter_tool] Build shaders as .iplr and use FragmentProgram…
0153d65 
….fromAsset for ink_sparkle (flutter/flutter#108071)
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Jul 22, 2022
Closed
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jul 22, 2022
@engine-flutter-autoroll
dd20919 [flutter_tool] Build shaders as .iplr and use FragmentProgram…
fde25b1 
….fromAsset for ink_sparkle (flutter/flutter#108071)
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Jul 22, 2022
Closed
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jul 24, 2022
@engine-flutter-autoroll
dd20919 [flutter_tool] Build shaders as .iplr and use FragmentProgram…
2bab40b 
….fromAsset for ink_sparkle (flutter/flutter#108071)
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Jul 24, 2022
Closed
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jul 24, 2022
@engine-flutter-autoroll
dd20919 [flutter_tool] Build shaders as .iplr and use FragmentProgram…
30dc69b 
….fromAsset for ink_sparkle (flutter/flutter#108071)
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Jul 25, 2022
Closed
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jul 25, 2022
@engine-flutter-autoroll
dd20919 [flutter_tool] Build shaders as .iplr and use FragmentProgram…
ccb586c 
….fromAsset for ink_sparkle (flutter/flutter#108071)
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Jul 25, 2022
Closed
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jul 25, 2022
@engine-flutter-autoroll
dd20919 [flutter_tool] Build shaders as .iplr and use FragmentProgram…
072f153 
….fromAsset for ink_sparkle (flutter/flutter#108071)
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Jul 25, 2022
Closed
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jul 25, 2022
@engine-flutter-autoroll
dd20919 [flutter_tool] Build shaders as .iplr and use FragmentProgram…
bddc94a 
….fromAsset for ink_sparkle (flutter/flutter#108071)
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Jul 25, 2022
Closed
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jul 25, 2022
@engine-flutter-autoroll
dd20919 [flutter_tool] Build shaders as .iplr and use FragmentProgram…
c2af370 
….fromAsset for ink_sparkle (flutter/flutter#108071)
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Jul 25, 2022
Closed
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jul 25, 2022
@engine-flutter-autoroll
dd20919 [flutter_tool] Build shaders as .iplr and use FragmentProgram…
3493c55 
….fromAsset for ink_sparkle (flutter/flutter#108071)
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Jul 25, 2022
Closed
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jul 25, 2022
@engine-flutter-autoroll
dd20919 [flutter_tool] Build shaders as .iplr and use FragmentProgram…
4bf5e12 
….fromAsset for ink_sparkle (flutter/flutter#108071)
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Jul 25, 2022
Closed
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jul 25, 2022
@engine-flutter-autoroll
dd20919 [flutter_tool] Build shaders as .iplr and use FragmentProgram…
7fd2a93 
….fromAsset for ink_sparkle (flutter/flutter#108071)
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jul 25, 2022
@engine-flutter-autoroll
dd20919 [flutter_tool] Build shaders as .iplr and use FragmentProgram…
9bd23aa 
….fromAsset for ink_sparkle (flutter/flutter#108071)
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jul 25, 2022
@engine-flutter-autoroll
dd20919 [flutter_tool] Build shaders as .iplr and use FragmentProgram…
ed6691c 
….fromAsset for ink_sparkle (flutter/flutter#108071)
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jul 25, 2022
@engine-flutter-autoroll
dd20919 [flutter_tool] Build shaders as .iplr and use FragmentProgram…
2a03b8a 
….fromAsset for ink_sparkle (flutter/flutter#108071)
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jul 25, 2022
@engine-flutter-autoroll
dd20919 [flutter_tool] Build shaders as .iplr and use FragmentProgram…
5c655c7 
….fromAsset for ink_sparkle (flutter/flutter#108071)
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jul 25, 2022
@engine-flutter-autoroll
dd20919 [flutter_tool] Build shaders as .iplr and use FragmentProgram…
1d19712 
….fromAsset for ink_sparkle (flutter/flutter#108071)
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jul 26, 2022
@engine-flutter-autoroll
dd20919 [flutter_tool] Build shaders as .iplr and use FragmentProgram…
038cda9 
….fromAsset for ink_sparkle (flutter/flutter#108071)
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jul 26, 2022
@engine-flutter-autoroll
dd20919 [flutter_tool] Build shaders as .iplr and use FragmentProgram…
b678493 
….fromAsset for ink_sparkle (flutter/flutter#108071)
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jul 26, 2022
@engine-flutter-autoroll
dd20919 [flutter_tool] Build shaders as .iplr and use FragmentProgram…
f106ca0 
….fromAsset for ink_sparkle (flutter/flutter#108071)
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jul 26, 2022
@engine-flutter-autoroll
dd20919 [flutter_tool] Build shaders as .iplr and use FragmentProgram…
088cc29 
….fromAsset for ink_sparkle (flutter/flutter#108071)
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jul 26, 2022
@engine-flutter-autoroll
dd20919 [flutter_tool] Build shaders as .iplr and use FragmentProgram…
fc50f9e 
….fromAsset for ink_sparkle (flutter/flutter#108071)
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jul 26, 2022
@engine-flutter-autoroll
dd20919 [flutter_tool] Build shaders as .iplr and use FragmentProgram…
98e8813 
….fromAsset for ink_sparkle (flutter/flutter#108071)
camsim99 pushed a commit to camsim99/flutter that referenced this pull request Aug 10, 2022
@zanderso @camsim99
[flutter_tool] Build shaders as .iplr and use FragmentProgram.fromAss…
3064792 
…et for ink_sparkle (flutter#108071)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@jonahwilliams jonahwilliams jonahwilliams approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

f: material design flutter/packages/flutter/material repository. framework flutter/packages/flutter repository. See also f: labels. tool Affects the "flutter" command-line tool. See also t: labels.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@zanderso @jonahwilliams