Refactor gradient renderer to produce image. Implement gradient based Shadermask by ferhatb · Pull Request #24029 · flutter/engine

ferhatb · 2021-01-28T21:08:41Z

Implements Shadermask for html and canvaskit rendering backends.

Issue: flutter/flutter#44152
flutter/flutter#75495

Will unblock work for flutter/flutter#52967

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 Hixie said 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.
The reviewer has submitted any presubmit flakes in this PR using the engine presubmit flakes form before re-triggering the failure.

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

… ShaderMask

flutter-dashboard · 2021-01-28T21:08:44Z

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.

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.

yjbanov · 2021-02-10T22:54:17Z

lib/web_ui/lib/src/engine/html/render_vertices.dart

+  /// Renders a rectangle using given program into an image resource.
+  ///
+  /// Browsers that support OffscreenCanvas and the transferToImageBitmap api
+  /// will return ImageBitmap, otherwise will return CanvasElement.


The dartdoc seems wrong. The function returns an image URL in all circumstances.

yjbanov · 2021-02-10T22:55:34Z

lib/web_ui/lib/src/engine/html/render_vertices.dart

  /// Browsers that support OffscreenCanvas and the transferToImageBitmap api
  /// will return ImageBitmap, otherwise will return CanvasElement.
  Object? drawRect(ui.Rect targetRect, _GlContext gl, _GlProgram glProgram,
+      NormalizedGradient gradient, int widthInPixels, int heightInPixels, ) {


Do you need the trailing comma?

yjbanov · 2021-02-10T22:55:42Z

lib/web_ui/lib/src/engine/html/render_vertices.dart

+  /// Browsers that support OffscreenCanvas and the transferToImageBitmap api
+  /// will return ImageBitmap, otherwise will return CanvasElement.
+  String drawRectToImageUrl(ui.Rect targetRect, _GlContext gl, _GlProgram glProgram,
+      NormalizedGradient gradient, int widthInPixels, int heightInPixels, ) {


Do you need the trailing comma?

yjbanov · 2021-02-10T22:57:46Z

lib/web_ui/lib/src/engine/html/shader_mask.dart

+      this.shader,
+      this.maskRect,
+      this.blendMode,
+      ) : super(oldLayer);


Something's off in the indent here

done. ran dartfmt on file.

yjbanov · 2021-02-10T23:26:35Z

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

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


yjbanov · 2021-02-10T23:46:43Z

lib/web_ui/lib/src/engine/html/render_vertices.dart

+
+  /// Returns image data in data url format.
+  String toImageUrl() {
+    html.CanvasElement canvas = html.CanvasElement(width: _widthInPixels, height: _heightInPixels);


How often is this called? If a lot, we should either cache and reuse the canvas element, or set the width/height to 0 so that we don't run iOS Safari out of memory.

I'm not sure what's done :)

Oh, nvm, you do set the dimensions to 0

yjbanov · 2021-02-10T23:47:24Z

lib/web_ui/lib/src/engine/html/shader_mask.dart

+  final ui.BlendMode blendMode;
+  html.Element? _shaderElement;
+  html.Element? _imageElement;
+  bool containerVisible = true;


Is this field used?

yjbanov · 2021-02-10T23:48:31Z

lib/web_ui/lib/src/engine/html/shader_mask.dart

+  final ui.Rect maskRect;
+  final ui.BlendMode blendMode;
+  html.Element? _shaderElement;
+  html.Element? _imageElement;


I only see this element being removed and set to null? Is it used for anything?

Was for future ImageShader. removed for now.

yjbanov · 2021-02-10T23:50:11Z

lib/web_ui/lib/src/engine/html/shader_mask.dart

+  @override
+  html.Element createElement() {
+    html.Element element = defaultCreateElement('flt-shader-mask');
+    html.Element container = html.Element.tag('flt-mask-interior');


Is the interior element necessary? We typically use it in clipping layers when the clipping element changes the coordinate system. I'm not sure this layer has this problem?

Yes otherwise we can't host svg resource.

yjbanov · 2021-02-10T23:54:05Z

lib/web_ui/lib/src/engine/html/shader_mask.dart

+        (swapLayers
+            ? '<feBlend in="SourceGraphic" in2="image" mode="$feBlend"/>'
+            : '<feBlend in="image" in2="SourceGraphic" mode="$feBlend"/>'),
+  );


Are all blend modes covered by tests?

yjbanov

yjbanov · 2021-02-11T17:42:55Z

lib/web_ui/test/canvaskit/shader_mask_golden_test.dart

@@ -0,0 +1,99 @@
+// Copyright 2013 The Flutter Authors. All rights reserved.


nit: this file is called shader_mask_golden_test.dart, but the HTML equivalent is called shadermask_golden_test.dart. Let's keep them consistent? (probably shader_mask is better because the class is ShaderMask as opposed to Shadermask)

yjbanov

…nt based Shadermask (flutter/engine#24029)

flutter/engine#24029 landed

… Shadermask (flutter#24029)

Refactor gradient renderer to produce image. Implement gradient based…

f3a0963

… ShaderMask

googlebot added the cla: yes label Jan 28, 2021

ferhatb mentioned this pull request Jan 28, 2021

Partial ShaderMask for Web DOM #21756

Closed

14 tasks

update license

1ebd1e8

chinmaygarde added the platform-web Code specifically for the web engine label Feb 4, 2021

ferhatb added 2 commits February 10, 2021 10:29

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

e8a4dda

Add shadermask golden

fb59a28

ferhatb changed the title ~~WIP : Refactor gradient renderer to produce image. Implement gradient based Shadermask~~ Refactor gradient renderer to produce image. Implement gradient based Shadermask Feb 10, 2021

ferhatb requested a review from yjbanov February 10, 2021 22:13

ferhatb mentioned this pull request Feb 10, 2021

Add shadermask golden flutter/goldens#163

Merged

Implement shadermask for Canvaskit

780f4a9

yjbanov reviewed Feb 10, 2021

View reviewed changes

ferhatb added 5 commits February 11, 2021 09:19

address reviewer comments

ec5662c

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

8e5c0af

update golden lock

0b1d2e9

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

498fdd9

Update golden locks to regen commit

ac2f830

yjbanov approved these changes Feb 11, 2021

View reviewed changes

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

af2583e

yjbanov mentioned this pull request Feb 11, 2021

[web] UnimplementedError when using SliderTheme flutter/flutter#75495

Closed

ferhatb added 9 commits February 11, 2021 14:52

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

4ff1194

fix nullability analysis warnings

551d934

update golden locks for ios

4bde534

rename file for consistency with canvaskit

218bf8b

initWebGl when creating shader

f67e3ef

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

ced2f6e

Prevent crash due to empty mask rect

be5b55c

Fix svgelement fill

dff7a53

revert width/height on filter element

429f77c

ferhatb added 2 commits February 12, 2021 11:38

Fix maskRect

23bf058

Update test with correct maskRect

7d4014a

mzdm mentioned this pull request Feb 12, 2021

Not supported for web. hnvn/flutter_shimmer#25

Closed

ferhatb added 11 commits February 12, 2021 15:09

Fix svg filter wrapper

33be612

Use shared svg resource header constant

e7e8635

skip xor webkit test

000554b

skip plus filter for webkit

e4d4d9e

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

e07c56e

merge

5adcf08

merge

64a547a

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

e1dc686

Workaround webkit svg filter bug, fix BlendMode.src add more tests

d53839e

update golden lock

14bede3

merge

84999ed

yjbanov approved these changes Feb 19, 2021

View reviewed changes

ferhatb merged commit 57c5777 into flutter:master Feb 20, 2021

engine-flutter-autoroll mentioned this pull request Feb 22, 2021

Roll Engine from b793775d2a01 to 07afcf7d0f12 (12 revisions) flutter/flutter#76536

Merged

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

57c5777 Refactor gradient renderer to produce image. Implement gradie…

a21d129

…nt based Shadermask (flutter/engine#24029)

mzdm added a commit to mzdm/virtual_traveller_flutter that referenced this pull request Feb 23, 2021

feat: web loading glowing effect

6b2ad8a

flutter/engine#24029 landed

hjfreyer pushed a commit to hjfreyer/engine that referenced this pull request Mar 22, 2021

Refactor gradient renderer to produce image. Implement gradient based…

02772ac

… Shadermask (flutter#24029)

chriscraws pushed a commit to chriscraws/engine that referenced this pull request Mar 23, 2021

Refactor gradient renderer to produce image. Implement gradient based…

05f8cf9

… Shadermask (flutter#24029)

		@@ -0,0 +1,99 @@
		// Copyright 2013 The Flutter Authors. All rights reserved.

Conversation

ferhatb commented Jan 28, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pre-launch Checklist

Uh oh!

flutter-dashboard bot commented Jan 28, 2021

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!

ferhatb Feb 11, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

yjbanov left a comment

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!

yjbanov left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

ferhatb commented Jan 28, 2021 •

edited

Loading

ferhatb Feb 11, 2021 •

edited

Loading