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

add currentTransform/Clip methods to DL and ui.Canvas #33308

Merged
merged 11 commits into from
Jun 15, 2022

Conversation

flar
Copy link
Contributor

@flar flar commented May 13, 2022

@flar
Copy link
Contributor Author

flar commented May 13, 2022

Some of the errors cannot be fixed until this framework PR is merged: flutter/flutter#103737

@flar
Copy link
Contributor Author

flar commented May 17, 2022

I've run into trouble in implementing the web version of this. I've only looked at the canvaskit path so far, but it looks like canvaskit does not provide the methods that return the clip bounds. Further investigation is required to proceed.

@flar flar added the Work in progress (WIP) Not ready (yet) for review! label May 17, 2022
@flar flar force-pushed the DL-current-transform-clip-methods branch from 594e2b9 to e63f578 Compare May 20, 2022 21:51
@flar
Copy link
Contributor Author

flar commented May 20, 2022

@yjbanov I've added the new methods to the web_ui canvas.dart, but they aren't implemented anywhere (waiting to see what havoc that makes with the tests).

Right now canvasKit is missing the implementation of some of these methods so we can't implement them in the canvaskit back end. I think the HTML back end might keep a stack of this information so that it could probably implement them now, but I wasn't sure if implementing things like this on HTML but not CK would be appropriate.

Skia has been notified that we need the new methods and I've filed https://bugs.chromium.org/p/chromium/issues/detail?id=1327776

@flar flar removed the Work in progress (WIP) Not ready (yet) for review! label May 21, 2022
@flar
Copy link
Contributor Author

flar commented May 23, 2022

Letting this sit until https://bugs.chromium.org/p/skia/issues/detail?id=13347 is implemented.

@chinmaygarde chinmaygarde added the Work in progress (WIP) Not ready (yet) for review! label May 26, 2022
@flar flar force-pushed the DL-current-transform-clip-methods branch from 1b78aec to 9e6d51a Compare June 10, 2022 03:35
@flar flar requested a review from yjbanov June 10, 2022 03:36
@flar flar removed the Work in progress (WIP) Not ready (yet) for review! label Jun 10, 2022
@flar flar force-pushed the DL-current-transform-clip-methods branch from 1da5b83 to 06efd18 Compare June 13, 2022 21:58
@flar flar requested review from yjbanov and dnfield and removed request for yjbanov and dnfield June 14, 2022 21:10
clip_op == SkClipOp::kIntersect //
? Push<ClipIntersectRRectOp>(0, 1, rrect, is_aa)
: Push<ClipDifferenceRRectOp>(0, 1, rrect, is_aa);
if (clip_op == SkClipOp::kIntersect) {
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: avoid == with enums.

Copy link
Contributor

Choose a reason for hiding this comment

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

(and elsewhere)

SkM44 inverse;
if (current_layer_->matrix.invert(&inverse)) {
SkRect devBounds;
current_layer_->clip_bounds.roundOut(&devBounds);
Copy link
Contributor

Choose a reason for hiding this comment

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

Why roundOut?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The clip_bounds are sub-pixel accurate, but the actual pixels touched will be rounded out to a whole pixel which means any rendering op that touches any of these pixels might leak through into the resulting rendering since we don't actually do geometric clipping, so they must be accounted for in the returned bounds for culling analysis and they must be accounted for before the inverse transform is applied in the next line.

Copy link
Contributor

@dnfield dnfield left a comment

Choose a reason for hiding this comment

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

LGTM w/ nit

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jun 16, 2022
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jun 16, 2022
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jun 16, 2022
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jun 16, 2022
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jun 16, 2022
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jun 16, 2022
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jun 16, 2022
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jun 16, 2022
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jun 16, 2022
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jun 16, 2022
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jun 16, 2022
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jun 17, 2022
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jun 17, 2022
iskakaushik added a commit to iskakaushik/engine that referenced this pull request Jun 17, 2022
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jun 17, 2022
houhuayong pushed a commit to houhuayong/engine that referenced this pull request Jun 21, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
waiting for tree to go green This PR is approved and tested, but waiting for the tree to be green to land.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Provide ui.Canvas.getCurrentMatrix and getCurrentClip
4 participants