Skip to content
This repository was archived by the owner on Feb 25, 2025. It is now read-only.

Make ui.Canvas.getDestinationClipBounds works with matrix #34835

Merged
auto-submit merged 8 commits into from
Jul 28, 2022
Merged

Make ui.Canvas.getDestinationClipBounds works with matrix #34835

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
31d44f5
Make ui.Canvas.getDestinationClipBounds works with matrix
ColdPaleLight Jul 22, 2022
739bdfb
Add test to dart
ColdPaleLight Jul 22, 2022
56f9eb1
Add logic about clip path with inverse fill type
ColdPaleLight Jul 27, 2022
230f9d6
Clean code
ColdPaleLight Jul 27, 2022
fb1a872
Merge branch 'flutter:main' into clip_with_matrix
ColdPaleLight Jul 27, 2022
e0b1a0a
Clean code
ColdPaleLight Jul 27, 2022
1869882
Merge branch 'clip_with_matrix' of https://github.com/ColdPaleLight/e…
ColdPaleLight Jul 27, 2022
cab83b1
Add comment
ColdPaleLight Jul 28, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 9 additions & 9 deletions display_list/display_list_builder.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -602,9 +602,7 @@ void DisplayListBuilder::clipRect(const SkRect& rect,
switch (clip_op) {
case SkClipOp::kIntersect:
Push<ClipIntersectRectOp>(0, 1, rect, is_aa);
if (!current_layer_->clip_bounds.intersect(rect)) {
current_layer_->clip_bounds.setEmpty();
}
intersect(rect);
break;
case SkClipOp::kDifference:
Push<ClipDifferenceRectOp>(0, 1, rect, is_aa);
Expand All @@ -620,9 +618,7 @@ void DisplayListBuilder::clipRRect(const SkRRect& rrect,
switch (clip_op) {
case SkClipOp::kIntersect:
Push<ClipIntersectRRectOp>(0, 1, rrect, is_aa);
if (!current_layer_->clip_bounds.intersect(rrect.getBounds())) {
current_layer_->clip_bounds.setEmpty();
}
intersect(rrect.getBounds());
break;
case SkClipOp::kDifference:
Push<ClipDifferenceRRectOp>(0, 1, rrect, is_aa);
Expand Down Expand Up @@ -653,15 +649,19 @@ void DisplayListBuilder::clipPath(const SkPath& path,
switch (clip_op) {
case SkClipOp::kIntersect:
Push<ClipIntersectPathOp>(0, 1, path, is_aa);
if (!current_layer_->clip_bounds.intersect(path.getBounds())) {
current_layer_->clip_bounds.setEmpty();
}
intersect(path.getBounds());
Copy link
Contributor

@flar flar Jul 26, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should check "inverse fill type" here for completeness, but I don't think Flutter can create those so it's only theoretical.

Copy link
Member Author

@ColdPaleLight ColdPaleLight Jul 27, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done, I also added the logic for inverse fill type with SkClipOp::kDifference. (Although Flutter can't access it yet)

break;
case SkClipOp::kDifference:
Push<ClipDifferencePathOp>(0, 1, path, is_aa);
break;
}
}
void DisplayListBuilder::intersect(const SkRect& rect) {
SkRect devClipBounds = getTransform().mapRect(rect);
if (!current_layer_->clip_bounds.intersect(devClipBounds)) {
current_layer_->clip_bounds.setEmpty();
}
}
SkRect DisplayListBuilder::getLocalClipBounds() {
SkM44 inverse;
if (current_layer_->matrix.invert(&inverse)) {
Expand Down
2 changes: 1 addition & 1 deletion display_list/display_list_builder.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -211,7 +211,7 @@ class DisplayListBuilder final : public virtual Dispatcher,
void clipRect(const SkRect& rect, SkClipOp clip_op, bool is_aa) override;
void clipRRect(const SkRRect& rrect, SkClipOp clip_op, bool is_aa) override;
void clipPath(const SkPath& path, SkClipOp clip_op, bool is_aa) override;

void intersect(const SkRect& rect);
/// Conservative estimate of the bounds of all outstanding clip operations
/// measured in the coordinate space within which this DisplayList will
/// be rendered.
Expand Down
62 changes: 62 additions & 0 deletions display_list/display_list_unittests.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2116,6 +2116,25 @@ TEST(DisplayList, ClipRectAffectsClipBounds) {
ASSERT_EQ(builder.getDestinationClipBounds(), initialDestinationBounds);
}

TEST(DisplayList, ClipRectAffectsClipBoundsWithMatrix) {
DisplayListBuilder builder;
SkRect clipBounds1 = SkRect::MakeLTRB(0, 0, 10, 10);
SkRect clipBounds2 = SkRect::MakeLTRB(10, 10, 20, 20);
builder.save();
builder.clipRect(clipBounds1, SkClipOp::kIntersect, false);
builder.translate(10, 0);
builder.clipRect(clipBounds1, SkClipOp::kIntersect, false);
ASSERT_EQ(builder.getDestinationClipBounds(), SkRect::MakeEmpty());
Copy link
Contributor

@flar flar Jul 27, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps we should relax these to "get...Bounds().isEmpty()" rather than force a specific empty return value? Are they documented to return exactly that rectangle for an empty condition? I suppose we could just modify the test if we ever return a different value, but an implementation might decide to just max/min the 8 values and let their natural "emptiness" be a return value. We only get a hard-coded 0,0,0,0 rect because we use Skia for the intersections and they decided to return a false boolean with no changes to the rectangle for an empty intersection and so we have to manually force the answer to an empty answer (using MakeEmpty() for convenience).

Copy link
Member Author

@ColdPaleLight ColdPaleLight Jul 27, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

builder.restore();

builder.save();
builder.clipRect(clipBounds1, SkClipOp::kIntersect, false);
builder.translate(-10, -10);
builder.clipRect(clipBounds2, SkClipOp::kIntersect, false);
ASSERT_EQ(builder.getDestinationClipBounds(), clipBounds1);
builder.restore();
}

TEST(DisplayList, ClipRRectAffectsClipBounds) {
DisplayListBuilder builder;
SkRect clipBounds = SkRect::MakeLTRB(10.2, 11.3, 20.4, 25.7);
Expand Down Expand Up @@ -2156,6 +2175,28 @@ TEST(DisplayList, ClipRRectAffectsClipBounds) {
ASSERT_EQ(builder.getDestinationClipBounds(), initialDestinationBounds);
}

TEST(DisplayList, ClipRRectAffectsClipBoundsWithMatrix) {
DisplayListBuilder builder;
SkRect clipBounds1 = SkRect::MakeLTRB(0, 0, 10, 10);
SkRect clipBounds2 = SkRect::MakeLTRB(10, 10, 20, 20);
SkRRect clip1 = SkRRect::MakeRectXY(clipBounds1, 3, 2);
SkRRect clip2 = SkRRect::MakeRectXY(clipBounds2, 3, 2);

builder.save();
builder.clipRRect(clip1, SkClipOp::kIntersect, false);
builder.translate(10, 0);
builder.clipRRect(clip1, SkClipOp::kIntersect, false);
ASSERT_EQ(builder.getDestinationClipBounds(), SkRect::MakeEmpty());
builder.restore();

builder.save();
builder.clipRRect(clip1, SkClipOp::kIntersect, false);
builder.translate(-10, -10);
builder.clipRRect(clip2, SkClipOp::kIntersect, false);
ASSERT_EQ(builder.getDestinationClipBounds(), clipBounds1);
builder.restore();
}

TEST(DisplayList, ClipPathAffectsClipBounds) {
DisplayListBuilder builder;
SkPath clip = SkPath().addCircle(10.2, 11.3, 2).addCircle(20.4, 25.7, 2);
Expand Down Expand Up @@ -2196,6 +2237,27 @@ TEST(DisplayList, ClipPathAffectsClipBounds) {
ASSERT_EQ(builder.getDestinationClipBounds(), initialDestinationBounds);
}

TEST(DisplayList, ClipPathAffectsClipBoundsWithMatrix) {
DisplayListBuilder builder;
SkRect clipBounds = SkRect::MakeLTRB(0, 0, 10, 10);
SkPath clip1 = SkPath().addCircle(2.5, 2.5, 2.5).addCircle(7.5, 7.5, 2.5);
SkPath clip2 = SkPath().addCircle(12.5, 12.5, 2.5).addCircle(17.5, 17.5, 2.5);

builder.save();
builder.clipPath(clip1, SkClipOp::kIntersect, false);
builder.translate(10, 0);
builder.clipPath(clip1, SkClipOp::kIntersect, false);
ASSERT_EQ(builder.getDestinationClipBounds(), SkRect::MakeEmpty());
builder.restore();

builder.save();
builder.clipPath(clip1, SkClipOp::kIntersect, false);
builder.translate(-10, -10);
builder.clipPath(clip2, SkClipOp::kIntersect, false);
ASSERT_EQ(builder.getDestinationClipBounds(), clipBounds);
builder.restore();
}

TEST(DisplayList, DiffClipRectDoesNotAffectClipBounds) {
DisplayListBuilder builder;
SkRect diff_clip = SkRect::MakeLTRB(0, 0, 15, 15);
Expand Down
67 changes: 67 additions & 0 deletions testing/dart/canvas_test.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -731,6 +731,27 @@ void main() {
expect(canvas.getDestinationClipBounds(), initialDestinationBounds);
});

test('Canvas.clipRect with matrix affects canvas.getClipBounds', () async {
final PictureRecorder recorder = PictureRecorder();
final Canvas canvas = Canvas(recorder);
const Rect clipBounds1 = Rect.fromLTRB(0.0, 0.0, 10.0, 10.0);
const Rect clipBounds2 = Rect.fromLTRB(10.0, 10.0, 20.0, 20.0);

canvas.save();
canvas.clipRect(clipBounds1);
canvas.translate(0, 10.0);
canvas.clipRect(clipBounds1);
expect(canvas.getDestinationClipBounds(), Rect.fromLTRB(0.0, 0.0, 0.0, 0.0));
Copy link
Contributor

@flar flar Jul 27, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same comment about measuring expectations against the "isEmpty" property rather than requiring a specific answer as I made above for the native tests.

Copy link
Member Author

@ColdPaleLight ColdPaleLight Jul 27, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

canvas.restore();

canvas.save();
canvas.clipRect(clipBounds1);
canvas.translate(-10.0, -10.0);
canvas.clipRect(clipBounds2);
expect(canvas.getDestinationClipBounds(), clipBounds1);
canvas.restore();
});

test('Canvas.clipRRect affects canvas.getClipBounds', () async {
final PictureRecorder recorder = PictureRecorder();
final Canvas canvas = Canvas(recorder);
Expand Down Expand Up @@ -772,6 +793,29 @@ void main() {
expect(canvas.getDestinationClipBounds(), initialDestinationBounds);
});

test('Canvas.clipRRect with matrix affects canvas.getClipBounds', () async {
final PictureRecorder recorder = PictureRecorder();
final Canvas canvas = Canvas(recorder);
const Rect clipBounds1 = Rect.fromLTRB(0.0, 0.0, 10.0, 10.0);
const Rect clipBounds2 = Rect.fromLTRB(10.0, 10.0, 20.0, 20.0);
final RRect clip1 = RRect.fromRectAndRadius(clipBounds1, const Radius.circular(3));
final RRect clip2 = RRect.fromRectAndRadius(clipBounds2, const Radius.circular(3));

canvas.save();
canvas.clipRRect(clip1);
canvas.translate(0, 10.0);
canvas.clipRRect(clip1);
expect(canvas.getDestinationClipBounds(), Rect.fromLTRB(0.0, 0.0, 0.0, 0.0));
canvas.restore();

canvas.save();
canvas.clipRRect(clip1);
canvas.translate(-10.0, -10.0);
canvas.clipRRect(clip2);
expect(canvas.getDestinationClipBounds(), clipBounds1);
canvas.restore();
});

test('Canvas.clipPath affects canvas.getClipBounds', () async {
final PictureRecorder recorder = PictureRecorder();
final Canvas canvas = Canvas(recorder);
Expand Down Expand Up @@ -813,6 +857,29 @@ void main() {
expect(canvas.getDestinationClipBounds(), initialDestinationBounds);
});

test('Canvas.clipPath with matrix affects canvas.getClipBounds', () async {
final PictureRecorder recorder = PictureRecorder();
final Canvas canvas = Canvas(recorder);
const Rect clipBounds1 = Rect.fromLTRB(0.0, 0.0, 10.0, 10.0);
const Rect clipBounds2 = Rect.fromLTRB(10.0, 10.0, 20.0, 20.0);
final Path clip1 = Path()..addRect(clipBounds1)..addOval(clipBounds1);
final Path clip2 = Path()..addRect(clipBounds2)..addOval(clipBounds2);

canvas.save();
canvas.clipPath(clip1);
canvas.translate(0, 10.0);
canvas.clipPath(clip1);
expect(canvas.getDestinationClipBounds(), Rect.fromLTRB(0.0, 0.0, 0.0, 0.0));
canvas.restore();

canvas.save();
canvas.clipPath(clip1);
canvas.translate(-10.0, -10.0);
canvas.clipPath(clip2);
expect(canvas.getDestinationClipBounds(), clipBounds1);
canvas.restore();
});

test('Canvas.clipRect(diff) does not affect canvas.getClipBounds', () async {
final PictureRecorder recorder = PictureRecorder();
final Canvas canvas = Canvas(recorder);
Expand Down