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 4 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
21 changes: 13 additions & 8 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,24 @@ 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();
if (!path.isInverseFillType()) {
intersect(path.getBounds());
}
break;
case SkClipOp::kDifference:
Push<ClipDifferencePathOp>(0, 1, path, is_aa);
if (path.isInverseFillType()) {
Comment thread
flar marked this conversation as resolved.

@flar flar Jul 27, 2022

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Add a reminder comment here about how this works. Nothing too detailed, just reminding the reader (i.e. me?) that inverted difference is like intersection...

@ColdPaleLight ColdPaleLight Jul 28, 2022

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done.

intersect(path.getBounds());
}
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
1 change: 1 addition & 0 deletions display_list/display_list_builder.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -361,6 +361,7 @@ class DisplayListBuilder final : public virtual Dispatcher,

void setAttributesFromDlPaint(const DlPaint& paint,
const DisplayListAttributeFlags flags);
void intersect(const SkRect& rect);

// kInvalidSigma is used to indicate that no MaskBlur is currently set.
static constexpr SkScalar kInvalidSigma = 0.0;
Expand Down
90 changes: 90 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());

@flar flar Jul 27, 2022

Copy link
Copy Markdown
Contributor

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).

@ColdPaleLight ColdPaleLight Jul 27, 2022

Copy link
Copy Markdown
Contributor Author

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 Expand Up @@ -2252,6 +2314,34 @@ TEST(DisplayList, DiffClipPathDoesNotAffectClipBounds) {
ASSERT_EQ(builder.getDestinationClipBounds(), initialDestinationBounds);
}

TEST(DisplayList, ClipPathWithInvertFillTypeDoesNotAffectClipBounds) {
SkRect cull_rect = SkRect::MakeLTRB(0, 0, 100.0, 100.0);
DisplayListBuilder builder(cull_rect);
SkPath clip = SkPath().addCircle(10.2, 11.3, 2).addCircle(20.4, 25.7, 2);
clip.setFillType(SkPathFillType::kInverseWinding);
builder.clipPath(clip, SkClipOp::kIntersect, false);

SkRect initial_local_bounds = builder.getLocalClipBounds();

@flar flar Jul 27, 2022

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

"initial_"? That modifier would make sense if you were going to take measurements later on under a different name. It might be reasonable to just inline the getBounds calls into the ASSERT macros here.

@ColdPaleLight ColdPaleLight Jul 27, 2022

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done.

SkRect initial_destination_bounds = builder.getDestinationClipBounds();
ASSERT_EQ(initial_local_bounds, cull_rect);
ASSERT_EQ(initial_destination_bounds, cull_rect);
}

TEST(DisplayList, DiffClipPathWithInvertFillTypeAffectsClipBounds) {
SkRect cull_rect = SkRect::MakeLTRB(0, 0, 100.0, 100.0);
DisplayListBuilder builder(cull_rect);
SkPath clip = SkPath().addCircle(10.2, 11.3, 2).addCircle(20.4, 25.7, 2);
clip.setFillType(SkPathFillType::kInverseWinding);
SkRect clip_bounds = SkRect::MakeLTRB(8.2, 9.3, 22.4, 27.7);
SkRect clip_expanded_bounds = SkRect::MakeLTRB(8, 9, 23, 28);
builder.clipPath(clip, SkClipOp::kDifference, false);

SkRect initial_local_bounds = builder.getLocalClipBounds();
SkRect initial_destination_bounds = builder.getDestinationClipBounds();
ASSERT_EQ(initial_local_bounds, clip_expanded_bounds);
ASSERT_EQ(initial_destination_bounds, clip_bounds);
}

TEST(DisplayList, FlatDrawPointsProducesBounds) {
SkPoint horizontal_points[2] = {{10, 10}, {20, 10}};
SkPoint vertical_points[2] = {{10, 10}, {10, 20}};
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));

@flar flar Jul 27, 2022

Copy link
Copy Markdown
Contributor

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.

@ColdPaleLight ColdPaleLight Jul 27, 2022

Copy link
Copy Markdown
Contributor Author

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