Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Impeller] fixed Rect::Contains #49294

Merged
merged 1 commit into from
Dec 20, 2023
Merged

Conversation

gaaclarke
Copy link
Member

fixes flutter/flutter#140464

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

If you need help, consider asking for advice on the #hackers-new channel on Discord.

@gaaclarke gaaclarke requested review from flar and bdero December 20, 2023 19:18
@@ -130,7 +130,8 @@ struct TRect {
}

[[nodiscard]] constexpr bool Contains(const TRect& o) const {
Copy link
Member Author

Choose a reason for hiding this comment

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

We want a.Contains(a) == true, right?

Copy link
Member

Choose a reason for hiding this comment

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

Yes indeed, we assume this inclusivity in our current usages

Copy link
Member Author

Choose a reason for hiding this comment

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

I only ask because the Contains(Point) uses < operators for the right and bottom axises.

Copy link
Contributor

Choose a reason for hiding this comment

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

Another issue with these methods is that empty can contain another rectangle and vice versa. I'm planning to address all of these in a rewrite now that the fields are hidden, but I'm just starting on that now and if this PR fixes an existing practical issue, then we should proceed with it.

Copy link
Contributor

Choose a reason for hiding this comment

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

The left and top edges are considered "inside", the right and bottom edges are considered "outside".

For points you want >= left/top and < right/bottom to match that rule.

For rects, both have the same insideness and so matching edges always have the same containment as each other. In the case of right and bottom, those edges are the non-inclusive limit of what each rectangle considers "inside" and so if they are equal then all of the points inside one of them are also inside the other (wrt that one edge).

Copy link
Member

@bdero bdero left a comment

Choose a reason for hiding this comment

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

LGTM. This looks safe because the Get[Direction] methods return sorted directions.

@@ -130,7 +130,8 @@ struct TRect {
}

[[nodiscard]] constexpr bool Contains(const TRect& o) const {
Copy link
Member

Choose a reason for hiding this comment

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

Yes indeed, we assume this inclusivity in our current usages

@gaaclarke gaaclarke added the autosubmit Merge PR when tree becomes green via auto submit App label Dec 20, 2023
@flar
Copy link
Contributor

flar commented Dec 20, 2023

LGTM. This looks safe because the Get[Direction] methods return sorted directions.

Actually, the opposite. Because of the sorted data an empty rectangle can "contain" a non-empty rectangle. The auto-sorting of rectangles is one of the first things I'm going to fix in the rework of Rect.

@flar
Copy link
Contributor

flar commented Dec 20, 2023

The eventual implementation of these methods will be based upon the versions that were created for the DL-specific geometry classes, such as:

https://github.com/flar/engine/blob/f391c31b882278603f42b725b3587fc94dddc0ab/display_list/geometry/dl_rect.h#L553

Note that we decided to not proceed with the DL-geometry package and instead have it share a geometry package with Impeller by updating Impeller's classes.

@auto-submit auto-submit bot merged commit 23d2fcd into flutter:main Dec 20, 2023
27 checks passed
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Dec 21, 2023
auto-submit bot pushed a commit to flutter/flutter that referenced this pull request Dec 21, 2023
…140519)

Roll Flutter Engine from c70f0a495ace to 1b1b2a12a597 (32 revisions)

flutter/engine@c70f0a4...1b1b2a1

2023-12-21 skia-flutter-autoroll@skia.org Roll Dart SDK from 4a10533a4dc8 to bcf68d22f0fa (1 revision) (flutter/engine#49329)
2023-12-21 skia-flutter-autoroll@skia.org Roll Dart SDK from 1d0c3ecd1349 to 4a10533a4dc8 (1 revision) (flutter/engine#49326)
2023-12-21 flar@google.com Ensure sorted rects in ui.Canvas for legacy compatibility (flutter/engine#49309)
2023-12-21 skia-flutter-autoroll@skia.org Roll Dart SDK from 63a452b45026 to 1d0c3ecd1349 (1 revision) (flutter/engine#49318)
2023-12-21 dnfield@google.com [Impeller] Make IPLR files multi-platform (flutter/engine#49253)
2023-12-21 ditman@gmail.com [web] Defer injection of platform views until needed. (flutter/engine#48960)
2023-12-21 skia-flutter-autoroll@skia.org Roll Dart SDK from 1aef027ec953 to 63a452b45026 (2 revisions) (flutter/engine#49311)
2023-12-21 skia-flutter-autoroll@skia.org Roll Skia from 29917d8c97ca to 4b16117e94b2 (4 revisions) (flutter/engine#49310)
2023-12-21 737941+loic-sharma@users.noreply.github.com Reland "[Windows] Move to FlutterCompositor for rendering" (flutter/engine#49262)
2023-12-21 30870216+gaaclarke@users.noreply.github.com Reland `[Impeller] new blur: refactored math and fixed expanded padding size` (flutter/engine#49302)
2023-12-20 dkwingsmt@users.noreply.github.com Multiview pipeline Pt. 1: Skip illegal render calls (flutter/engine#49266)
2023-12-20 barpac02@gmail.com SemanticsUpdateBuilder: make all args non-null (flutter/engine#49148)
2023-12-20 30870216+gaaclarke@users.noreply.github.com [Impeller] fixed Rect::Contains (flutter/engine#49294)
2023-12-20 skia-flutter-autoroll@skia.org Roll Dart SDK from 97c3b7e1885a to 1aef027ec953 (1 revision) (flutter/engine#49295)
2023-12-20 30870216+gaaclarke@users.noreply.github.com Revert "[Impeller] new blur: refactored math and fixed expanded padding size" (flutter/engine#49298)
2023-12-20 30870216+gaaclarke@users.noreply.github.com [Impeller] new blur: refactored math and fixed expanded padding size (flutter/engine#49206)
2023-12-20 dkwingsmt@users.noreply.github.com Multi-view pointer event (flutter/engine#46213)
2023-12-20 1961493+harryterkelsen@users.noreply.github.com [web:multiview] Only call `Renderer.clearFragmentProgramCache` on hot restart (flutter/engine#48758)
2023-12-20 skia-flutter-autoroll@skia.org Roll Skia from 9cb1bb1164ea to 29917d8c97ca (1 revision) (flutter/engine#49289)
2023-12-20 bdero@google.com [Impeller] Add interactive Blur+Clip AiksTest. (flutter/engine#49283)
2023-12-20 sergiy.dubovik@supercell.com [macos] FlutterKeyboardManager memory leak fix (flutter/engine#48824)
2023-12-20 zanderso@users.noreply.github.com Don't guard Windows arm64 Dart SDK download on the release candidate flag (flutter/engine#49244)
2023-12-20 15619084+vashworth@users.noreply.github.com Fix testAppExtensionLaunching for Xcode 15/iOS 17 (flutter/engine#49242)
2023-12-20 skia-flutter-autoroll@skia.org Roll Skia from 8060d6b36066 to 9cb1bb1164ea (2 revisions) (flutter/engine#49288)
2023-12-20 skia-flutter-autoroll@skia.org Roll Dart SDK from ed415d966d8a to 97c3b7e1885a (1 revision) (flutter/engine#49287)
2023-12-20 skia-flutter-autoroll@skia.org Roll Skia from d0f09ad481f7 to 8060d6b36066 (1 revision) (flutter/engine#49285)
2023-12-20 kevinjchisholm@gmail.com [release] Update release config (flutter/engine#49254)
2023-12-20 skia-flutter-autoroll@skia.org Roll Dart SDK from 1732c4c92ccd to ed415d966d8a (1 revision) (flutter/engine#49274)
2023-12-20 skia-flutter-autoroll@skia.org Roll Dart SDK from 4c59838945d9 to 1732c4c92ccd (1 revision) (flutter/engine#49269)
2023-12-20 goderbauer@google.com Sync lints with flutter/flutter (flutter/engine#49192)
2023-12-19 1961493+harryterkelsen@users.noreply.github.com [web] Enforce onDrawFrame/onBeginFrame render rule (flutter/engine#49214)
2023-12-19 barpac02@gmail.com [Docs] Add more info about running tests on iOS (flutter/engine#48859)

If this roll has caused a breakage, revert this CL and stop the roller
using the controls here:
https://autoroll.skia.org/r/flutter-engine-flutter-autoroll
Please CC jimgraham@google.com,rmistry@google.com,zra@google.com on the revert to ensure that a human
is aware of the problem.

To file a bug in Flutter: https://github.com/flutter/flutter/issues/new/choose

To report a problem with the AutoRoller itself, please file a bug:
https://issues.skia.org/issues/new?component=1389291&template=1850622

Documentation for the AutoRoller is here:
https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
...
CoderDake pushed a commit to CoderDake/flutter that referenced this pull request Dec 28, 2023
…lutter#140519)

Roll Flutter Engine from c70f0a495ace to 1b1b2a12a597 (32 revisions)

flutter/engine@c70f0a4...1b1b2a1

2023-12-21 skia-flutter-autoroll@skia.org Roll Dart SDK from 4a10533a4dc8 to bcf68d22f0fa (1 revision) (flutter/engine#49329)
2023-12-21 skia-flutter-autoroll@skia.org Roll Dart SDK from 1d0c3ecd1349 to 4a10533a4dc8 (1 revision) (flutter/engine#49326)
2023-12-21 flar@google.com Ensure sorted rects in ui.Canvas for legacy compatibility (flutter/engine#49309)
2023-12-21 skia-flutter-autoroll@skia.org Roll Dart SDK from 63a452b45026 to 1d0c3ecd1349 (1 revision) (flutter/engine#49318)
2023-12-21 dnfield@google.com [Impeller] Make IPLR files multi-platform (flutter/engine#49253)
2023-12-21 ditman@gmail.com [web] Defer injection of platform views until needed. (flutter/engine#48960)
2023-12-21 skia-flutter-autoroll@skia.org Roll Dart SDK from 1aef027ec953 to 63a452b45026 (2 revisions) (flutter/engine#49311)
2023-12-21 skia-flutter-autoroll@skia.org Roll Skia from 29917d8c97ca to 4b16117e94b2 (4 revisions) (flutter/engine#49310)
2023-12-21 737941+loic-sharma@users.noreply.github.com Reland "[Windows] Move to FlutterCompositor for rendering" (flutter/engine#49262)
2023-12-21 30870216+gaaclarke@users.noreply.github.com Reland `[Impeller] new blur: refactored math and fixed expanded padding size` (flutter/engine#49302)
2023-12-20 dkwingsmt@users.noreply.github.com Multiview pipeline Pt. 1: Skip illegal render calls (flutter/engine#49266)
2023-12-20 barpac02@gmail.com SemanticsUpdateBuilder: make all args non-null (flutter/engine#49148)
2023-12-20 30870216+gaaclarke@users.noreply.github.com [Impeller] fixed Rect::Contains (flutter/engine#49294)
2023-12-20 skia-flutter-autoroll@skia.org Roll Dart SDK from 97c3b7e1885a to 1aef027ec953 (1 revision) (flutter/engine#49295)
2023-12-20 30870216+gaaclarke@users.noreply.github.com Revert "[Impeller] new blur: refactored math and fixed expanded padding size" (flutter/engine#49298)
2023-12-20 30870216+gaaclarke@users.noreply.github.com [Impeller] new blur: refactored math and fixed expanded padding size (flutter/engine#49206)
2023-12-20 dkwingsmt@users.noreply.github.com Multi-view pointer event (flutter/engine#46213)
2023-12-20 1961493+harryterkelsen@users.noreply.github.com [web:multiview] Only call `Renderer.clearFragmentProgramCache` on hot restart (flutter/engine#48758)
2023-12-20 skia-flutter-autoroll@skia.org Roll Skia from 9cb1bb1164ea to 29917d8c97ca (1 revision) (flutter/engine#49289)
2023-12-20 bdero@google.com [Impeller] Add interactive Blur+Clip AiksTest. (flutter/engine#49283)
2023-12-20 sergiy.dubovik@supercell.com [macos] FlutterKeyboardManager memory leak fix (flutter/engine#48824)
2023-12-20 zanderso@users.noreply.github.com Don't guard Windows arm64 Dart SDK download on the release candidate flag (flutter/engine#49244)
2023-12-20 15619084+vashworth@users.noreply.github.com Fix testAppExtensionLaunching for Xcode 15/iOS 17 (flutter/engine#49242)
2023-12-20 skia-flutter-autoroll@skia.org Roll Skia from 8060d6b36066 to 9cb1bb1164ea (2 revisions) (flutter/engine#49288)
2023-12-20 skia-flutter-autoroll@skia.org Roll Dart SDK from ed415d966d8a to 97c3b7e1885a (1 revision) (flutter/engine#49287)
2023-12-20 skia-flutter-autoroll@skia.org Roll Skia from d0f09ad481f7 to 8060d6b36066 (1 revision) (flutter/engine#49285)
2023-12-20 kevinjchisholm@gmail.com [release] Update release config (flutter/engine#49254)
2023-12-20 skia-flutter-autoroll@skia.org Roll Dart SDK from 1732c4c92ccd to ed415d966d8a (1 revision) (flutter/engine#49274)
2023-12-20 skia-flutter-autoroll@skia.org Roll Dart SDK from 4c59838945d9 to 1732c4c92ccd (1 revision) (flutter/engine#49269)
2023-12-20 goderbauer@google.com Sync lints with flutter/flutter (flutter/engine#49192)
2023-12-19 1961493+harryterkelsen@users.noreply.github.com [web] Enforce onDrawFrame/onBeginFrame render rule (flutter/engine#49214)
2023-12-19 barpac02@gmail.com [Docs] Add more info about running tests on iOS (flutter/engine#48859)

If this roll has caused a breakage, revert this CL and stop the roller
using the controls here:
https://autoroll.skia.org/r/flutter-engine-flutter-autoroll
Please CC jimgraham@google.com,rmistry@google.com,zra@google.com on the revert to ensure that a human
is aware of the problem.

To file a bug in Flutter: https://github.com/flutter/flutter/issues/new/choose

To report a problem with the AutoRoller itself, please file a bug:
https://issues.skia.org/issues/new?component=1389291&template=1850622

Documentation for the AutoRoller is here:
https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
autosubmit Merge PR when tree becomes green via auto submit App e: impeller
Projects
No open projects
Archived in project
Development

Successfully merging this pull request may close these issues.

[Impeller] Rect::Contains has floating point errors
3 participants