Skip to content

Replaces the InkSparkle shader SPIR-V program from in-memory (List<int> as bytes) to a binary asset (ink_sparkle.spv) - #101699

Closed
clocksmith wants to merge 3 commits into
flutter:masterfrom
clocksmith:ink-shader-file
Closed

Replaces the InkSparkle shader SPIR-V program from in-memory (List<int> as bytes) to a binary asset (ink_sparkle.spv)#101699
clocksmith wants to merge 3 commits into
flutter:masterfrom
clocksmith:ink-shader-file

Conversation

@clocksmith

@clocksmith clocksmith commented Apr 11, 2022

Copy link
Copy Markdown
Contributor

Replaces the InkSparkle shader SPIR-V program from in-memory (List<int> as bytes) to a binary asset (ink_sparkle.spv)

With this change, the asset is added to runtime assets and loaded asynchronously, right before the compile step, which is already asynchronous.

TODO(reviewers): For WIP, copied asset to cache subdirectory, where material_fonts are read, but where should it be stored instead?

closes: #99783

There are no tests because this is an implementation change, but the behavior is the same, and there are existing tests.

Pre-launch Checklist

  • I read the [Contributor Guide] and followed the process outlined there for submitting PRs.
  • I read the [Tree Hygiene] wiki page, which explains my responsibilities.
  • [x]I read and followed the [Flutter Style Guide], including [Features we expect every widget to implement].
  • I signed the [CLA].
  • I listed at least one issue that this PR fixes in the description above.
  • I updated/added relevant documentation (doc comments with ///).
  • I added new tests to check the change I am making, or this PR is [test-exempt].
  • All existing and new tests are passing.

@clocksmith clocksmith added framework flutter/packages/flutter repository. See also f: labels. p: material_ui material_ui package in flutter/packages a: assets Packaging, accessing, or using assets work in progress; do not review e: impeller Impeller rendering backend issues and features requests labels Apr 11, 2022
@clocksmith
clocksmith requested a review from zanderso April 11, 2022 13:38
@flutter-dashboard flutter-dashboard Bot added the tool Affects the "flutter" command-line tool. See also t: labels. label Apr 11, 2022
@flutter-dashboard

flutter-dashboard Bot commented Apr 11, 2022

Copy link
Copy Markdown

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.

@clocksmith clocksmith mentioned this pull request Apr 11, 2022
Closed
@zanderso

zanderso commented Apr 11, 2022

Copy link
Copy Markdown
Member

It looks like the parts of the change to use the shader asset in the framework were removed? Maybe github is just not loading the change properly for me for some reason, though.

@zanderso

zanderso commented Apr 11, 2022

Copy link
Copy Markdown
Member

Ah, I misread the PR. The presubmits are failing because of the binary artifact. It looks like we'll need to get the shader compiler pulled in first. I will look into that ASAP.

@clocksmith

clocksmith commented Apr 19, 2022

Copy link
Copy Markdown
Contributor Author

@zanderso any updates needed on my end?

@dnfield

dnfield commented Apr 20, 2022

Copy link
Copy Markdown
Contributor

I think Zach got blocked on #102165, and now will be OOO for a bit. That bug is probably resolved by a PR I'm about to land, but getting impellerc pulled in will probably have to wait for a bit now.

@clocksmith

clocksmith commented Apr 26, 2022 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

@dnfield Does that mean this can't land until impellerc lands?

@zanderso Does the fact that this shader lives as in memory SPIR-V make it a blocker for anything?

@zanderso

zanderso commented Apr 26, 2022

Copy link
Copy Markdown
Member

@clocksmith Yeah, this is blocked waiting for impellerc to be available, but as of yesterday, impellerc is already being vended by the engine build. The next step is to consume it in the tool. Using the in-memory SPIR-V is a blocker for Impeller to support InkSparkle.

@clocksmith

clocksmith commented Apr 26, 2022 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Thanks @zanderso

The next step is to consume it in the tool.

To consume this sparkle effect shader? Is there a tracking bug to know when that is unblocked, so this one can be pushed forward?

@zanderso

zanderso commented May 6, 2022

Copy link
Copy Markdown
Member

This is obsolete.

@zanderso zanderso closed this May 6, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@zanderso zanderso Awaiting requested review from zanderso

Assignees

No one assigned

Labels

a: assets Packaging, accessing, or using assets e: impeller Impeller rendering backend issues and features requests framework flutter/packages/flutter repository. See also f: labels. p: material_ui material_ui package in flutter/packages 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.

Change InkSparkle SPIR-V byte code from static const to a file to prepare for Impeller

3 participants

@clocksmith @zanderso @dnfield