[web] Implement tilemode for gradient shaders. by ferhatb · Pull Request #22597 · flutter/engine

ferhatb · 2020-11-18T22:34:40Z

Description

Implements tilemode for gradient shaders.

Related Issues

Tests

I added the following tests:
linear_gradient_golden_test.dart
radial_gradient_golden_test.dart

Checklist

Before you create this PR confirm that it meets all requirements listed below by checking the relevant checkboxes ([x]). This will ensure a smooth and quick review process.

I read the [contributor guide] and followed the process outlined there for submitting PRs.
I signed the [CLA].
I read and followed the [C++, Objective-C, Java style guides] for the engine.
I read the [tree hygiene] wiki page, which explains my responsibilities.
I updated/added relevant documentation.
All existing and new tests are passing.
I am willing to follow-up on review comments in a timely manner.

Reviewer Checklist

I have submitted any presubmit flakes in this PR using the [engine presubmit flakes form] before re-triggering the failure.

Breaking Change

Did any tests fail when you ran them? Please read [handling breaking changes].

No, no existing tests failed, so this is not a breaking change.
Yes, this is a breaking change. If not, delete the remainder of this section.
- I wrote a design doc: https://flutter.dev/go/template Replace this with a link to your design doc's short link
- I got input from the developer relations team, specifically from: Replace with the names of who gave advice
- I wrote a migration guide: https://flutter.dev/go/breaking-changes-template Replace this with a link to a pull request that adds the migration guide to https://flutter.dev/docs/release/breaking-changes

yjbanov · 2020-11-18T23:32:36Z

lib/web_ui/lib/src/engine/html/shaders/shader.dart

+        assert(colors != null),
+  // ignore: unnecessary_null_comparison
+        assert(tileMode != null),
+  // ignore: unnecessary_null_comparison


Do these // ignore still apply correctly after formatting?

yjbanov · 2020-11-18T23:39:54Z

lib/web_ui/lib/src/engine/html/shaders/shader.dart

+
+    // Render gradient into a bitmap and create a canvas pattern.
+    _OffScreenCanvas offScreenCanvas =
+    _OffScreenCanvas(widthInPixels, heightInPixels);


nit: this line needs an extra indent

yjbanov · 2020-11-18T23:40:07Z

lib/web_ui/lib/src/engine/html/shaders/shader.dart

+    _GlContext gl = _OffScreenCanvas.supported
+        ? _GlContext.fromOffscreenCanvas(offScreenCanvas._canvas!)
+        : _GlContext.fromCanvas(offScreenCanvas._glCanvas!,
+        webGLVersion == WebGLVersion.webgl1);


nit: this line needs an extra indent

yjbanov · 2020-11-18T23:49:52Z

lib/web_ui/lib/src/engine/html/shaders/shader.dart

-// For now only GradientRotation is supported in flutter which is implemented
-// for linear gradient.
-// See https://github.com/flutter/flutter/issues/32819
+/// Returns name of gradient treshold variable to use to compute color.


nit: the doc comment makes it sound like this function only returns the name and nothing else.

yjbanov · 2020-11-18T23:59:45Z

lib/web_ui/lib/src/engine/html/shaders/shader.dart

+    _GlContext gl = _OffScreenCanvas.supported
+        ? _GlContext.fromOffscreenCanvas(offScreenCanvas._canvas!)
+        : _GlContext.fromCanvas(offScreenCanvas._glCanvas!,
+        webGLVersion == WebGLVersion.webgl1);


nit: this line needs an extra indent

yjbanov · 2020-11-19T00:01:38Z

lib/web_ui/lib/src/engine/html/shaders/shader.dart

-      return gradient;
+    _GlProgram glProgram = gl.useAndCacheProgram(
+        _WebGlRenderer.writeBaseVertexShader(),
+        _createLinearFragmentShader(normalizedGradient, tileMode))!;


I think we can make useAndCacheProgram return a non-null value instead of null-asserting at call sites.

yjbanov · 2020-11-19T00:01:51Z

lib/web_ui/lib/src/engine/html/shaders/shader.dart

+
+    _GlProgram glProgram = gl.useAndCacheProgram(
+        _WebGlRenderer.writeBaseVertexShader(),
+        _createRadialFragmentShader(normalizedGradient, tileMode))!;


ditto (non-null useAndCacheProgram)

yjbanov · 2020-11-19T00:04:44Z

lib/web_ui/lib/src/ui/painting.dart

    ? engine.CkGradientLinear(from, to, colors, colorStops, tileMode, matrix4)
-    : engine.GradientLinear(from, to, colors, colorStops, tileMode, matrix4);
+    : engine.GradientLinear(from, to, colors, colorStops, tileMode,
+      matrix4 == null ? null : engine.toMatrix32(matrix4));


nit: this line needs an extra indent

yjbanov · 2020-11-19T00:06:02Z

lib/web_ui/lib/src/engine/html/shaders/shader.dart

+    final String probeName =
+      _writeSharedGradientShader(builder, method, gradient, tileMode);
+    method.addStatement('${fragColor.name} = ${probeName} * scale + bias;');
+    String shader = builder.build();


nit: shader variable unnecessary, can return builder.build();

You may want to add more shader code in the future after you calculate fragColor.

yjbanov · 2020-11-19T00:08:20Z

lib/web_ui/lib/src/engine/html/shaders/shader.dart

+    normalizedGradient.setupUniforms(gl, glProgram);
+    Object gradientMatrix = gl.getUniformLocation(
+        glProgram.program, 'm_gradient');
+    gl.setUniformMatrix4fv(gradientMatrix, false, matrix4 == null ? Matrix4.identity().storage : matrix4!);


Is identity or null matrix a common case? If so, perhaps we should reuse a static identity matrix instead of instantiating a new one every time. JS-engines are still pretty bad at allocating large typed arrays.

Noticed it too. I'm working on a separate CL to go through a lot of other identity() calls we have and optimize identity itself. Matrix4.zero().setIdentity() is not efficient either since Float32List does get already initialized with 0s.

…e#22597)

* 53fc019 Split AOT Android Embedder and shell (flutter/engine#22179) * fc55814 Implement Scene.toImage() in CanvasKit mode. (flutter/engine#22085) * c45e02a Roll Dart SDK from 12fded61a2bc to a06d469024fd (1 revision) (flutter/engine#22623) * 550c750 Remove opt outs for dart:ui (flutter/engine#22603) * f2803ac [fuchsia] shader warmup fixes (flutter/engine#22439) * ce94c4e Roll Dart SDK from a06d469024fd to b8fea79a2549 (1 revision) (flutter/engine#22630) * 76b6acb Roll Fuchsia Linux SDK from aAb3NJv_h... to X1ue-JZsc... (flutter/engine#22631) * 976e887 Roll Skia from ed289e777cfa to 9dce4d081f8a (3 revisions) (flutter/engine#22632) * 885bd65 Roll Fuchsia Mac SDK from DQpWjEN59... to wGZWtwuY4... (flutter/engine#22633) * 8971b82 Roll Dart SDK from b8fea79a2549 to 861ebcb175b6 (1 revision) (flutter/engine#22634) * a09cdfd Roll Skia from 9dce4d081f8a to 8c5889937172 (1 revision) (flutter/engine#22635) * a9f332c Roll Dart SDK from 861ebcb175b6 to 1adf3d5fa9d0 (1 revision) (flutter/engine#22636) * 1bf5c8b [web] Implement tilemode for gradient shaders. (flutter/engine#22597) * 97cacfb Add more runtime intrinsic symbols to the export checker script (flutter/engine#22641)

ferhatb added 3 commits November 17, 2020 10:14

Add linear tile shader

1216d86

Merge remote-tracking branch 'upstream/master' into tilemode2

c681ca5

Add linear/radial gradient tilemode support

ff2cefa

ferhatb requested a review from yjbanov November 18, 2020 22:34

googlebot added the cla: yes label Nov 18, 2020

yjbanov approved these changes Nov 19, 2020

View reviewed changes

ferhatb added 4 commits November 19, 2020 11:30

update golden locks

0dcf254

Merge remote-tracking branch 'upstream/master' into tilemode2

6c5aff6

Address reviewer comments

79bfe34

Fix gradient radial screenshot bounds

c809ab7

chinmaygarde added the platform-web Code specifically for the web engine label Nov 19, 2020

ferhatb added 3 commits November 19, 2020 16:18

Update maxDiffRate for iOS

4da8535

Remove write:true for screenshot test

e2a5b84

Update diffrate for iOS

73147bf

ferhatb merged commit 1bf5c8b into flutter:master Nov 20, 2020

engine-flutter-autoroll mentioned this pull request Nov 20, 2020

Roll Engine from 23a8e027dbbe to 97cacfbfec89 (14 revisions) flutter/flutter#70958

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Nov 20, 2020

1bf5c8b [web] Implement tilemode for gradient shaders. (flutter/engin…

6ad8378

…e#22597)

chaselatta pushed a commit to chaselatta/engine that referenced this pull request Nov 30, 2020

[web] Implement tilemode for gradient shaders. (flutter#22597)

844bb15

ferhatb mentioned this pull request Dec 2, 2020

TileMode.mirror/repeated not implementing correctly on web flutter/flutter#45505

Closed

ferhatb deleted the tilemode2 branch January 14, 2021 18:33

ferhatb mentioned this pull request Jan 19, 2021

RenderFlex overflow indicator is styled differently than mobile flutter/flutter#32223

Closed

Comments

Conversation

ferhatb commented Nov 18, 2020

Description

Related Issues

Tests

Checklist

Reviewer Checklist

Breaking Change

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants