Refactor gradient renderer to produce image. Implement gradient based Shadermask #24029
Conversation
|
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. |
| /// 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. |
There was a problem hiding this comment.
The dartdoc seems wrong. The function returns an image URL in all circumstances.
| @@ -195,6 +198,31 @@ class _WebGlRenderer implements _GlRenderer { | |||
| /// 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, ) { | |||
There was a problem hiding this comment.
Do you need the trailing comma?
| /// 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, ) { |
There was a problem hiding this comment.
Do you need the trailing comma?
| this.shader, | ||
| this.maskRect, | ||
| this.blendMode, | ||
| ) : super(oldLayer); |
There was a problem hiding this comment.
Something's off in the indent here
There was a problem hiding this comment.
done. ran dartfmt on file.
| @@ -190,7 +180,7 @@ class GradientLinear extends EngineGradient { | |||
| _GlContext gl = _OffScreenCanvas.supported | |||
| ? _GlContext.fromOffscreenCanvas(offScreenCanvas._canvas!) | |||
| : _GlContext.fromCanvas(offScreenCanvas._glCanvas!, | |||
| webGLVersion == WebGLVersion.webgl1); | |||
| webGLVersion == WebGLVersion.webgl1); | |||
|
|
||
| /// Returns image data in data url format. | ||
| String toImageUrl() { | ||
| html.CanvasElement canvas = html.CanvasElement(width: _widthInPixels, height: _heightInPixels); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
I'm not sure what's done :)
There was a problem hiding this comment.
Oh, nvm, you do set the dimensions to 0
| final ui.BlendMode blendMode; | ||
| html.Element? _shaderElement; | ||
| html.Element? _imageElement; | ||
| bool containerVisible = true; |
| final ui.Rect maskRect; | ||
| final ui.BlendMode blendMode; | ||
| html.Element? _shaderElement; | ||
| html.Element? _imageElement; |
There was a problem hiding this comment.
I only see this element being removed and set to null? Is it used for anything?
There was a problem hiding this comment.
Was for future ImageShader. removed for now.
| @override | ||
| html.Element createElement() { | ||
| html.Element element = defaultCreateElement('flt-shader-mask'); | ||
| html.Element container = html.Element.tag('flt-mask-interior'); |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Yes otherwise we can't host svg resource.
| (swapLayers | ||
| ? '<feBlend in="SourceGraphic" in2="image" mode="$feBlend"/>' | ||
| : '<feBlend in="image" in2="SourceGraphic" mode="$feBlend"/>'), | ||
| ); |
There was a problem hiding this comment.
Are all blend modes covered by tests?
| @@ -0,0 +1,99 @@ | |||
| // Copyright 2013 The Flutter Authors. All rights reserved. | |||
There was a problem hiding this comment.
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)
…nt based Shadermask (flutter/engine#24029)
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
writing and running engine tests.
///).If you need help, consider asking for advice on the #hackers-new channel on Discord.