fix: consider array size on canvaskit shader data #49754

renancaraujo · 2024-01-12T11:55:45Z

This PR changes the ShaderData construction on canvaskit to consider array uniforms.

flutter/flutter#141296

flutter/flutter#141838

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.
I read and followed the Flutter Style Guide and the C++, Objective-C, Java style guides.
I listed at least one issue that this PR fixes in the description above.
I added new tests to check the change I am making or feature I am adding, or the PR is test-exempt. See testing the engine for instructions on writing and running engine tests.
I updated/added relevant documentation (doc comments with ///).
I signed the CLA.
All existing and new tests are passing.

If you need help, consider asking for advice on the #hackers-new channel on Discord.

renancaraujo · 2024-01-12T12:07:41Z

Still missing some tests, thats why its draft.

jonahwilliams · 2024-01-12T17:53:30Z

This looks like the right change! In terms of testing, we have a test file: https://github.com/flutter/engine/blob/main/lib/web_ui/test/canvaskit/fragment_program_test.dart

You could copy down the JSON format for a shader with array uniform and assert that it had the correct structure?

renancaraujo · 2024-02-05T19:22:47Z

Hello mr @jonahwilliams , updated the PR with tests and fixed flutter/flutter#141838

jonahwilliams · 2024-02-06T17:07:09Z

Thanks, i will try and take a look this week - but I'm at a conference today and tomorrow so there will be some delay.

renancaraujo · 2024-02-06T18:01:16Z

lib/web_ui/lib/src/engine/shader_data.dart

      }
-      uniforms[location] = UniformData(
+      uniforms[i] = UniformData(


To reviewer: Attention to this change, in the current impl we get an out of bounds error as the location value also considers the array size.

* Use the original kJsonIPLR shader source and metadata to match the test expectations * Make kJsonArrayIPLR a Dart raw string

jonahwilliams

LGTM

This PR changes the ShaderData construction on canvaskit to consider array uniforms. flutter/flutter#141296 flutter/flutter#141838 [C++, Objective-C, Java style guides]: https://github.com/flutter/engine/blob/main/CONTRIBUTING.md#style

…engine#49754)

2024-02-15 30870216+gaaclarke@users.noreply.github.com Added tool to easily check golden diffs locally. (flutter/engine#50654) 2024-02-15 skia-flutter-autoroll@skia.org Roll Skia from 4bbf2060b008 to 12d0b7fac4c3 (2 revisions) (flutter/engine#50689) 2024-02-15 johnoneil@users.noreply.github.com Provide a matrix inverse shim for GLES 2.0. (flutter/engine#50545) 2024-02-15 jonahwilliams@google.com [iOS] Ensure FlutterMetalLayer has correct backpressure. (flutter/engine#50486) 2024-02-15 skia-flutter-autoroll@skia.org Roll Skia from 682f0e1e7e77 to 4bbf2060b008 (3 revisions) (flutter/engine#50686) 2024-02-15 103135467+sealesj@users.noreply.github.com Pin OSV-Scanner reusable workflow (flutter/engine#50649) 2024-02-15 whesse@google.com Add support for dart_src GN variable to flutter_frontend_server build (flutter/engine#50685) 2024-02-15 6718144+renancaraujo@users.noreply.github.com fix: consider array size on canvaskit shader data (flutter/engine#49754) 2024-02-15 skia-flutter-autoroll@skia.org Roll Skia from 1277910beec9 to 682f0e1e7e77 (1 revision) (flutter/engine#50683) 2024-02-15 skia-flutter-autoroll@skia.org Roll Skia from 85ab600a9519 to 1277910beec9 (2 revisions) (flutter/engine#50682)

This PR changes the ShaderData construction on canvaskit to consider array uniforms. flutter/flutter#141296 flutter/flutter#141838 [C++, Objective-C, Java style guides]: https://github.com/flutter/engine/blob/main/CONTRIBUTING.md#style

rlealfacephi · 2024-05-17T07:12:23Z

Hi @renancaraujo. Im just just fighting with same problem on flutter web with some custom shaders that works perfect on native platforms. Seams that all shaders with issues in my side has array uniforms. I just try today all flutter channels (master included), but any of the channels fix the problem. Your fix is working for you?

Appreciate your comments!!! :)

renancaraujo · 2024-05-18T11:25:05Z

Hey @rlealfacephi now for me i was not able to reproduce the problem described in the issue. Would you open another issues decribing what is the output for you? Thanks.

github-actions bot added the platform-web Code specifically for the web engine label Jan 12, 2024

renancaraujo mentioned this pull request Jan 12, 2024

Array uniforms wont work on Flutter web flutter/flutter#141296

Closed

This comment was marked as outdated.

Sign in to view

renancaraujo force-pushed the renan/canvaskit branch 2 times, most recently from a431ed9 to 81576f8 Compare February 5, 2024 19:19

renancaraujo marked this pull request as ready for review February 5, 2024 19:20

renancaraujo mentioned this pull request Feb 5, 2024

Update Web to parse the new ImpellerC JSON shader output format #50182

Closed

jonahwilliams self-requested a review February 6, 2024 17:06

renancaraujo commented Feb 6, 2024

View reviewed changes

jason-simmons approved these changes Feb 7, 2024

View reviewed changes

add test

818858d

jason-simmons force-pushed the renan/canvaskit branch from 2be3ad4 to 818858d Compare February 13, 2024 17:49

jason-simmons added 2 commits February 14, 2024 00:59

Some fixes to the unit tests

a77f90a

* Use the original kJsonIPLR shader source and metadata to match the test expectations * Make kJsonArrayIPLR a Dart raw string

More test updates copied from #50182

fcc14b3

jonahwilliams approved these changes Feb 15, 2024

View reviewed changes

jason-simmons added the autosubmit Merge PR when tree becomes green via auto submit App label Feb 15, 2024

auto-submit bot merged commit c720046 into flutter:main Feb 15, 2024
26 checks passed

engine-flutter-autoroll mentioned this pull request Feb 16, 2024

Roll Flutter Engine from 15a358bbaf71 to 7de84271eb65 (56 revisions) flutter/flutter#143552

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Feb 16, 2024

c72004602 fix: consider array size on canvaskit shader data (flutter/…

c50c30e

…engine#49754)

jason-simmons mentioned this pull request Feb 16, 2024

Manual roll Flutter Engine from d3c71d78f8ef to b7103bc8b3743 flutter/flutter#143563

Merged

This was referenced Feb 28, 2024

Flutter beta channel broken for fragment shaders (windows 11) flutter/flutter#141838

Closed

Reland Activate InkSparkle on CanvasKit flutter/flutter#144314

Closed

renancaraujo mentioned this pull request Mar 2, 2024

[CP] Hotfix fragment shaders fix into stable flutter/flutter#144503

Closed

renancaraujo mentioned this pull request Apr 25, 2024

[CP][canvaskit] fix: consider array size on canvaskit shader data (#49754) #52400

Closed

8 tasks

bleroux mentioned this pull request May 3, 2024

InkSparkle.constantTurbulenceSeedSplashFactory causes a runtime error in web flutter/flutter#147771

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: consider array size on canvaskit shader data #49754

fix: consider array size on canvaskit shader data #49754

renancaraujo commented Jan 12, 2024 •

edited

renancaraujo commented Jan 12, 2024 •

edited

jonahwilliams commented Jan 12, 2024

This comment was marked as outdated.

renancaraujo commented Feb 5, 2024 •

edited

jonahwilliams commented Feb 6, 2024

renancaraujo Feb 6, 2024

jonahwilliams left a comment

rlealfacephi commented May 17, 2024

renancaraujo commented May 18, 2024

fix: consider array size on canvaskit shader data #49754

fix: consider array size on canvaskit shader data #49754

Conversation

renancaraujo commented Jan 12, 2024 • edited

Pre-launch Checklist

renancaraujo commented Jan 12, 2024 • edited

jonahwilliams commented Jan 12, 2024

This comment was marked as outdated.

renancaraujo commented Feb 5, 2024 • edited

jonahwilliams commented Feb 6, 2024

renancaraujo Feb 6, 2024

Choose a reason for hiding this comment

jonahwilliams left a comment

Choose a reason for hiding this comment

rlealfacephi commented May 17, 2024

renancaraujo commented May 18, 2024

renancaraujo commented Jan 12, 2024 •

edited

renancaraujo commented Jan 12, 2024 •

edited

renancaraujo commented Feb 5, 2024 •

edited