Skip to content

Commit

Permalink
[canvaskit] Clip before applying ColorFilter so it doesn't filter bey…
Browse files Browse the repository at this point in the history
…ond child bounds (flutter#52704)

When a ColorFilter affects transparent black, it will expand its bounds
to the entire screen, even if the `saveLayer` call is bounded. This
applies a clip before applying the ColorFilter so the filter is bounded
to just the child drawings.

Also fixes bug with ColorFilter being used as an ImageFilter.

Before:

![canvaskit_colorfilter_bounds_before](https://github.com/flutter/engine/assets/1961493/25394b40-c40d-44fb-9c78-9638a40d3329)

After:

![canvaskit_colorfilter_bounds_after](https://github.com/flutter/engine/assets/1961493/b25e4084-ccae-4e41-b6e6-37e8cbbd9d54)

Fixes flutter/flutter#88866
Fixes flutter/flutter#144015

## Pre-launch Checklist

- [x] I read the [Contributor Guide] and followed the process outlined
there for submitting PRs.
- [x] I read the [Tree Hygiene] wiki page, which explains my
responsibilities.
- [x] I read and followed the [Flutter Style Guide] and the [C++,
Objective-C, Java style guides].
- [x] I listed at least one issue that this PR fixes in the description
above.
- [x] 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.
- [x] I updated/added relevant documentation (doc comments with `///`).
- [x] I signed the [CLA].
- [x] All existing and new tests are passing.

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

<!-- Links -->
[Contributor Guide]:
https://github.com/flutter/flutter/wiki/Tree-hygiene#overview
[Tree Hygiene]: https://github.com/flutter/flutter/wiki/Tree-hygiene
[test-exempt]:
https://github.com/flutter/flutter/wiki/Tree-hygiene#tests
[Flutter Style Guide]:
https://github.com/flutter/flutter/wiki/Style-guide-for-Flutter-repo
[C++, Objective-C, Java style guides]:
https://github.com/flutter/engine/blob/main/CONTRIBUTING.md#style
[testing the engine]:
https://github.com/flutter/flutter/wiki/Testing-the-engine
[CLA]: https://cla.developers.google.com/
[flutter/tests]: https://github.com/flutter/tests
[breaking change policy]:
https://github.com/flutter/flutter/wiki/Tree-hygiene#handling-breaking-changes
[Discord]: https://github.com/flutter/flutter/wiki/Chat
  • Loading branch information
harryterkelsen authored May 10, 2024
1 parent f553caa commit ba8e0d3
Show file tree
Hide file tree
Showing 5 changed files with 119 additions and 9 deletions.
2 changes: 1 addition & 1 deletion lib/web_ui/lib/src/engine/canvaskit/canvaskit_api.dart
Original file line number Diff line number Diff line change
Expand Up @@ -1352,7 +1352,7 @@ extension SkPaintExtension on SkPaint {

@JS('setColorInt')
external JSVoid _setColorInt(JSNumber color);
void setColorInt(double color) => _setColorInt(color.toJS);
void setColorInt(int color) => _setColorInt(color.toJS);

external JSVoid setShader(SkShader? shader);
external JSVoid setMaskFilter(SkMaskFilter? maskFilter);
Expand Down
36 changes: 31 additions & 5 deletions lib/web_ui/lib/src/engine/canvaskit/layer.dart
Original file line number Diff line number Diff line change
Expand Up @@ -4,9 +4,11 @@

import 'package:ui/ui.dart' as ui;

import '../color_filter.dart';
import '../vector_math.dart';
import 'canvas.dart';
import 'canvaskit_api.dart';
import 'color_filter.dart';
import 'embedded_views.dart';
import 'image_filter.dart';
import 'n_way_canvas.dart';
Expand Down Expand Up @@ -409,12 +411,23 @@ class ImageFilterEngineLayer extends ContainerLayer
childMatrix.translate(_offset.dx, _offset.dy);
prerollContext.mutatorsStack
.pushTransform(Matrix4.translationValues(_offset.dx, _offset.dy, 0.0));
final CkManagedSkImageFilterConvertible convertible;
if (_filter is ui.ColorFilter) {
convertible = createCkColorFilter(_filter as EngineColorFilter)!;
} else {
convertible = _filter as CkManagedSkImageFilterConvertible;
}
final ui.Rect childPaintBounds =
prerollChildren(prerollContext, childMatrix);
(_filter as CkManagedSkImageFilterConvertible)
.imageFilter((SkImageFilter filter) {
paintBounds =
rectFromSkIRect(filter.getOutputBounds(toSkRect(childPaintBounds)));
convertible.imageFilter((SkImageFilter filter) {
// If the filter is a ColorFilter, the extended paint bounds will be the
// entire screen, which is not what we want.
if (_filter is ui.ColorFilter) {
paintBounds = childPaintBounds;
} else {
paintBounds =
rectFromSkIRect(filter.getOutputBounds(toSkRect(childPaintBounds)));
}
});
prerollContext.mutatorsStack.pop();
}
Expand All @@ -424,6 +437,8 @@ class ImageFilterEngineLayer extends ContainerLayer
assert(needsPainting);
paintContext.internalNodesCanvas.save();
paintContext.internalNodesCanvas.translate(_offset.dx, _offset.dy);
paintContext.internalNodesCanvas
.clipRect(paintBounds, ui.ClipOp.intersect, false);
final CkPaint paint = CkPaint();
paint.imageFilter = _filter;
paintContext.internalNodesCanvas.saveLayer(paintBounds, paint);
Expand Down Expand Up @@ -518,10 +533,21 @@ class ColorFilterEngineLayer extends ContainerLayer
final CkPaint paint = CkPaint();
paint.colorFilter = filter;

// We need to clip because if the ColorFilter affects transparent black,
// then it will fill the entire `cullRect` of the picture, ignoring the
// `paintBounds` passed to `saveLayer`. See:
// https://github.com/flutter/flutter/issues/88866
paintContext.internalNodesCanvas.save();

// TODO(hterkelsen): Only clip if the ColorFilter affects transparent black.
paintContext.internalNodesCanvas
.clipRect(paintBounds, ui.ClipOp.intersect, false);

paintContext.internalNodesCanvas.saveLayer(paintBounds, paint);
paint.dispose();
paintChildren(paintContext);
paintContext.internalNodesCanvas.restore();
paintContext.internalNodesCanvas.restore();
paint.dispose();
}
}

Expand Down
4 changes: 2 additions & 2 deletions lib/web_ui/lib/src/engine/canvaskit/painting.dart
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ import 'shader.dart';
class CkPaint implements ui.Paint {
CkPaint() : skiaObject = SkPaint() {
skiaObject.setAntiAlias(_isAntiAlias);
skiaObject.setColorInt(_defaultPaintColor.toDouble());
skiaObject.setColorInt(_defaultPaintColor);
_ref = UniqueRef<SkPaint>(this, skiaObject, 'Paint');
}

Expand Down Expand Up @@ -127,7 +127,7 @@ class CkPaint implements ui.Paint {
return;
}
_color = value.value;
skiaObject.setColorInt(value.value.toDouble());
skiaObject.setColorInt(value.value);
}

int _color = _defaultPaintColor;
Expand Down
2 changes: 1 addition & 1 deletion lib/web_ui/lib/src/engine/canvaskit/text.dart
Original file line number Diff line number Diff line change
Expand Up @@ -1187,7 +1187,7 @@ class CkParagraphBuilder implements ui.ParagraphBuilder {
SkPaint? foreground = skStyle.foreground?.skiaObject;
if (foreground == null) {
_defaultTextForeground.setColorInt(
(skStyle.color?.value ?? 0xFF000000).toDouble(),
skStyle.color?.value ?? 0xFF000000,
);
foreground = _defaultTextForeground;
}
Expand Down
84 changes: 84 additions & 0 deletions lib/web_ui/test/canvaskit/color_filter_golden_test.dart
Original file line number Diff line number Diff line change
Expand Up @@ -195,6 +195,90 @@ void testMain() {
await matchSceneGolden('canvaskit_dst_colorfilter.png', builder.build(),
region: region);
});

test('ColorFilter only applies to child bounds', () async {
final LayerSceneBuilder builder = LayerSceneBuilder();

builder.pushOffset(0, 0);

// Draw a red circle and add it to the scene.
final CkPictureRecorder recorder = CkPictureRecorder();
final CkCanvas canvas = recorder.beginRecording(region);

canvas.drawCircle(
const ui.Offset(75, 125),
50,
CkPaint()..color = const ui.Color.fromARGB(255, 255, 0, 0),
);
final CkPicture redCircle = recorder.endRecording();

builder.addPicture(ui.Offset.zero, redCircle);

// Apply a green color filter.
builder.pushColorFilter(
const ui.ColorFilter.mode(ui.Color(0xff00ff00), ui.BlendMode.color));
// Draw another red circle and apply it to the scene.
// This one should be green since we have the color filter.
final CkPictureRecorder recorder2 = CkPictureRecorder();
final CkCanvas canvas2 = recorder2.beginRecording(region);

canvas2.drawCircle(
const ui.Offset(425, 125),
50,
CkPaint()..color = const ui.Color.fromARGB(255, 255, 0, 0),
);
final CkPicture greenCircle = recorder2.endRecording();

builder.addPicture(ui.Offset.zero, greenCircle);

await matchSceneGolden(
'canvaskit_colorfilter_bounds.png',
builder.build(),
region: region,
);
});

test('ColorFilter works as an ImageFilter', () async {
final LayerSceneBuilder builder = LayerSceneBuilder();

builder.pushOffset(0, 0);

// Draw a red circle and add it to the scene.
final CkPictureRecorder recorder = CkPictureRecorder();
final CkCanvas canvas = recorder.beginRecording(region);

canvas.drawCircle(
const ui.Offset(75, 125),
50,
CkPaint()..color = const ui.Color.fromARGB(255, 255, 0, 0),
);
final CkPicture redCircle = recorder.endRecording();

builder.addPicture(ui.Offset.zero, redCircle);

// Apply a green color filter as an ImageFilter.
builder.pushImageFilter(
const ui.ColorFilter.mode(ui.Color(0xff00ff00), ui.BlendMode.color));
// Draw another red circle and apply it to the scene.
// This one should be green since we have the color filter.
final CkPictureRecorder recorder2 = CkPictureRecorder();
final CkCanvas canvas2 = recorder2.beginRecording(region);

canvas2.drawCircle(
const ui.Offset(425, 125),
50,
CkPaint()..color = const ui.Color.fromARGB(255, 255, 0, 0),
);
final CkPicture greenCircle = recorder2.endRecording();

builder.addPicture(ui.Offset.zero, greenCircle);

await matchSceneGolden(
'canvaskit_colorfilter_as_imagefilter.png',
builder.build(),
region: region,
);
});
// TODO(hterkelsen): https://github.com/flutter/flutter/issues/71520
}, skip: isSafari || isFirefox);
}

0 comments on commit ba8e0d3

Please sign in to comment.