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

Conversation

@cyanglaz
Copy link
Contributor

@cyanglaz cyanglaz commented Jun 19, 2020

Description

Adds a method to calculate the final bounding rect of the platform view.
Clippings are currently ignored

Related Issues

flutter/flutter#59821

Tests

embedded_view_params_unittests.cc

Checklist

Before you create this PR confirm that it meets all requirements listed below by checking the relevant checkboxes ([x]). This will ensure a smooth and quick review process.

  • I read the contributor guide and followed the process outlined there for submitting PRs.
  • I signed the CLA.
  • I read and followed the C++, Objective-C, Java style guides for the engine.
  • I read the tree hygiene wiki page, which explains my responsibilities.
  • I updated/added relevant documentation.
  • All existing and new tests are passing.
  • I am willing to follow-up on review comments in a timely manner.

Breaking Change

Did any tests fail when you ran them? Please read handling breaking changes.

@cyanglaz cyanglaz requested a review from blasten June 19, 2020 20:51
// https://github.com/flutter/flutter/issues/59821
SkRect GetBoundingRectAfterMutations() const {
SkPath path;
SkRect starting_rect = SkRect::MakeSize(sizePoints);
Copy link

Choose a reason for hiding this comment

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

does it still need the offsetPixels?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think mutator stack should contain all the offset informations already, By querying through the stack, we have the final offset

@cyanglaz
Copy link
Contributor Author

@blasten updated with our offline discussion

Comment on lines 212 to 215
const SkPoint& offsetPixels() const { return offset_pixels_; };
const SkSize& sizePoints() const { return size_points_; };
const MutatorsStack& mutatorsStack() const { return mutators_stack_; };
const SkRect& finalBoundingRect() const { return final_bounding_rect_; }
Copy link

Choose a reason for hiding this comment

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

nit: could we document these methods?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

params.offsetPixels.y(), //
params.sizePoints.width() * device_pixel_ratio_, //
params.sizePoints.height() * device_pixel_ratio_ //
SkRect::MakeXYWH(params.offsetPixels().x(), //
Copy link

Choose a reason for hiding this comment

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

is this just finalBoundingRect() now?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think the size is not the same as the bounding rect. The size points are the original size before applying mutation.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@chinmaygarde I thought params.sizePoints did not consider scaling and rotation at all? Does embedder free from scaling and rotation layers?

Copy link
Member

Choose a reason for hiding this comment

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

I believe the embedder is supposed to apply the scaling and rotation themselves from the data present in the mutator stack to get the final width and height. I believe there is a test for this in the embedder unit-tests harness and this should be documented in the header.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I meant this rect returned from this method does not include rotation and scaling. Want to make sure if the original size used here is intended.

Copy link
Member

Choose a reason for hiding this comment

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

Yes. It is.

Copy link

@blasten blasten left a comment

Choose a reason for hiding this comment

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

LGTM + confirmation from @chinmaygarde

@cyanglaz cyanglaz merged commit f5c315f into flutter:master Jun 22, 2020
@cyanglaz cyanglaz deleted the mutator_stack_get_rect branch June 22, 2020 04:01
brianosman added a commit that referenced this pull request Jun 22, 2020
…calculate the final bounding rect for platform view (#19170)"

This reverts commit f5c315f.
brianosman added a commit that referenced this pull request Jun 22, 2020
…calculate the final bounding rect for platform view (#19170)" (#19204)

This reverts commit f5c315f.
cyanglaz pushed a commit to cyanglaz/engine that referenced this pull request Jun 22, 2020
…rams to calculate the final bounding rect for platform view (flutter#19170)" (flutter#19204)"

This reverts commit 9cecc5f.
cyanglaz pushed a commit that referenced this pull request Jun 22, 2020
…calculate the final bounding rect for platform view #19170" (#19212)
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jun 23, 2020
…calculate the final bounding rect for platform view (flutter/engine#19170)
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jun 23, 2020
…calculate the final bounding rect for platform view (flutter/engine#19170)
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jun 23, 2020
…calculate the final bounding rect for platform view (flutter/engine#19170)
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jun 23, 2020
…calculate the final bounding rect for platform view (flutter/engine#19170)
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jun 23, 2020
…calculate the final bounding rect for platform view (flutter/engine#19170)
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jun 23, 2020
…calculate the final bounding rect for platform view (flutter/engine#19170)
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jun 23, 2020
…calculate the final bounding rect for platform view (flutter/engine#19170)
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jun 23, 2020
…calculate the final bounding rect for platform view (flutter/engine#19170)
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jun 23, 2020
…calculate the final bounding rect for platform view (flutter/engine#19170)
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jun 23, 2020
…calculate the final bounding rect for platform view (flutter/engine#19170)
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jun 23, 2020
…calculate the final bounding rect for platform view (flutter/engine#19170)
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jun 23, 2020
…calculate the final bounding rect for platform view (flutter/engine#19170)
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jun 23, 2020
…calculate the final bounding rect for platform view (flutter/engine#19170)
zhongwuzw pushed a commit to zhongwuzw/engine that referenced this pull request Jun 24, 2020
…calculate the final bounding rect for platform view (flutter#19170)" (flutter#19204)

This reverts commit f5c315f.
zhongwuzw pushed a commit to zhongwuzw/engine that referenced this pull request Jun 24, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants