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
merged 8 commits into from
Jul 28, 2022
Merged

Make ui.Canvas.getDestinationClipBounds works with matrix #34835

merged 8 commits into from
Jul 28, 2022

Conversation

@ColdPaleLight
Copy link
Member

@ColdPaleLight ColdPaleLight commented Jul 22, 2022 •
edited
Loading

fix flutter/flutter#108154

cc @flar

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.

@ColdPaleLight ColdPaleLight requested a review from flar July 22, 2022 07:35
This was referenced Jul 22, 2022
Merged
Merged
flar
flar reviewed Jul 26, 2022
View reviewed changes
Copy link
Contributor

@flar flar left a comment

Choose a reason for hiding this comment

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

Clicking "submit" on the 2 changes I've identified so I don't forget, but I'm still looking at the unit tests when I can find the chance.

display_list/display_list_builder.cc Outdated
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)

flar
flar suggested changes Jul 27, 2022
View reviewed changes
flar
flar reviewed Jul 27, 2022
View reviewed changes
display_list/display_list_unittests.cc Outdated
clip.setFillType(SkPathFillType::kInverseWinding);
builder.clipPath(clip, SkClipOp::kIntersect, false);

SkRect initial_local_bounds = builder.getLocalClipBounds();
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.

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

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.

flar
flar reviewed Jul 27, 2022
View reviewed changes
display_list/display_list_unittests.cc Outdated
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.

flar
flar reviewed Jul 27, 2022
View reviewed changes
testing/dart/canvas_test.dart Outdated
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.

@ColdPaleLight ColdPaleLight requested a review from flar July 27, 2022 06:23
flar
flar approved these changes Jul 27, 2022
View reviewed changes
Copy link
Contributor

@flar flar left a comment

Choose a reason for hiding this comment

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

Just a request to add a comment, but LGTM.

display_list/display_list_builder.cc
break;
case SkClipOp::kDifference:
Push<ClipDifferencePathOp>(0, 1, path, is_aa);
if (path.isInverseFillType()) {
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.

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

Copy link
Member Author

@ColdPaleLight ColdPaleLight Jul 28, 2022

Choose a reason for hiding this comment

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

Done.

@ColdPaleLight ColdPaleLight added the autosubmit Merge PR when tree becomes green via auto submit App label Jul 28, 2022
@auto-submit auto-submit bot merged commit 68e9e35 into flutter:main Jul 28, 2022
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Jul 28, 2022
Merged
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jul 28, 2022
@engine-flutter-autoroll
68e9e35 Make ui.Canvas.getDestinationClipBounds works with matrix (fl…
9104d2c 
…utter/engine#34835)
betrevisan pushed a commit to betrevisan/engine that referenced this pull request Jul 29, 2022
@ColdPaleLight @betrevisan
Make ui.Canvas.getDestinationClipBounds works with matrix (flutter#34835
bdf757f 
)
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

1 more reviewer

@flar flar flar approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

autosubmit Merge PR when tree becomes green via auto submit App

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

The result returned by the "ui.canvas.getDestinationClipBounds()" method is incorrect when with matrix

2 participants

@ColdPaleLight @flar