[Reland] Add rects to accumulator rather than bounds (#37435) #37451

iskakaushik · 2022-11-08T23:10:54Z

When the accumulator is an RTreeBoundsAccumulator rather than a RectBoundsAccumulator just accumulating the bounds results in incorrect results as the rtree would need to be aware of the constituent non-overlapping rectangles. This would work fine for RectBoundsAccumulator as it would just adjust its bounds based on the passed rects.

Fixes: flutter/flutter#113251

flutter-dashboard · 2022-11-08T23:10:58Z

It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact Hixie on the #hackers channel in Chat (don't just cc him here, he won't see it! He's on Discord!).

If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix?

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

flar

LGTM, thanks for filing the follow-on issue!

flar · 2022-11-09T00:01:42Z

display_list/display_list_utils.cc

+          display_list->rtree()->searchNonOverlappingDrawnRects(bounds);
+      for (const SkRect& rect : rects) {
+        // TODO (https://github.com/flutter/flutter/issues/114919): Attributes
+        // are not neccessarily `kDrawDisplayListFlags`.


Typo on necessarily.

flar · 2022-11-09T00:13:24Z

Also, would it be easy to add a unit test to display_list_unittests.cc that constructs nested DLs and gets their RTrees and see if the sparse nesting is preserved?

iskakaushik · 2022-11-09T00:25:25Z

Also, would it be easy to add a unit test to display_list_unittests.cc that constructs nested DLs and gets their RTrees and see if the sparse nesting is preserved?

Done! Made sure that the test fails without the change as well 😀

auto-submit · 2022-11-09T01:16:58Z

auto label is removed for flutter/engine, pr: 37451, due to - The status or check suite Mac mac_host_engine has failed. Please fix the issues identified (or deflake) before re-applying this label.

flar

LGTM

When the accumulator is an `RTreeBoundsAccumulator` rather than a `RectBoundsAccumulator` just accumulating the bounds results in incorrect results as the `rtree` would need to be aware of the constituent non-overlapping rectangles. This would work fine for `RectBoundsAccumulator` as it would just adjust its bounds based on the passed rects. Fixes: flutter/flutter#113251

…er#37435) (flutter/engine#37451)

…#37435) (flutter#37451)" This reverts commit f8048be. pre-emptively reverting flutter/flutter#114997 (comment) to unblock the roll.

…er#37435) (flutter/engine#37451)

…lutter#37451) * [Reland] Add rects to accumulator rather than bounds (flutter#37435) When the accumulator is an `RTreeBoundsAccumulator` rather than a `RectBoundsAccumulator` just accumulating the bounds results in incorrect results as the `rtree` would need to be aware of the constituent non-overlapping rectangles. This would work fine for `RectBoundsAccumulator` as it would just adjust its bounds based on the passed rects. Fixes: flutter/flutter#113251 * Add a test for nested display list rtree

…er#37435) (flutter/engine#37451)

…115025) * 6daa75ad4 Roll Skia from 0c8127b3dd7d to 6a056a26b3c5 (16 revisions) (flutter/engine#37438) * 6517ee8ee Roll Skia from 6a056a26b3c5 to 3cecb22c5827 (1 revision) (flutter/engine#37439) * c1200c44f Sdk roll failure - add facet to allowlist test_manager and other packages. (flutter/engine#37395) * 179154ec1 Revert "Sdk roll failure - add facet to allowlist test_manager and other packages. (#37395)" (flutter/engine#37449) * c171f27c4 Revert "Add rects to accumulator rather than bounds (#37435)" (flutter/engine#37444) * 583c71ce4 Clang-tidy: Fixed math on shard-id validator. (flutter/engine#37433) * d224648fc Roll Skia from 3cecb22c5827 to 94450cd1df4e (4 revisions) (flutter/engine#37445) * 5df42a87b Oops, accidentally compiled the non-null-safe platform file as null safe. (flutter/engine#37453) * 05455ac0a Roll Skia from 94450cd1df4e to 22526b55be02 (6 revisions) (flutter/engine#37455) * 9628070ee Roll Skia from 22526b55be02 to 511aa59461f3 (1 revision) (flutter/engine#37458) * fc7293441 [Impeller] Don't double-convert include path encodings in ImpellerC (flutter/engine#37408) * d82a2df8a Roll Skia from 511aa59461f3 to 790aedd91cd8 (2 revisions) (flutter/engine#37463) * be54e3ea0 Roll Skia from 790aedd91cd8 to fe143a0dc368 (3 revisions) (flutter/engine#37467) * 410f270e6 Roll Dart SDK from c15cdb978761 to 874a662f7507 (3 revisions) (flutter/engine#37466) * e778ca4e0 Roll Fuchsia Mac SDK from d4l6A1aPw6Z0YjxmA... to vEw8iuSZYmlfk1JiE... (flutter/engine#37468) * f8048be39 [Reland] Add rects to accumulator rather than bounds (#37435) (flutter/engine#37451) * 214b84137 Roll Dart SDK from 874a662f7507 to 4fcefaf03496 (1 revision) (flutter/engine#37470) * 8ecb1c913 [macOS] Remove OpenGL rendering backend (flutter/engine#37448) * 1a8888b51 Roll Skia from fe143a0dc368 to 0854badd5783 (1 revision) (flutter/engine#37471) * ca303bfd4 Roll Dart SDK from 4fcefaf03496 to fa6586382e5d (1 revision) (flutter/engine#37473) * 5d0df764b Use traingle strip for difference clip (flutter/engine#37469) * eeba9b3c8 Support HLSL ingestion in ImpellerC (flutter/engine#37461) * 5f88ee42b Use IndexError.check instead of RangeError.checkValidIndex in buffers. (flutter/engine#37450) * b65ac4ac4 Roll expat to 2.5.0 (flutter/engine#37383) * ddde8300e [sdk_roll_failure] add facet to allowlist test_manager and other packages. (flutter/engine#37479) * 62b581ee0 Roll CanvasKit to 0.37.1 (flutter/engine#37454) * aa4b3ea2f Turned on performance-unnecessary-value-param everywhere. (flutter/engine#37447)

…lutter#37451) * [Reland] Add rects to accumulator rather than bounds (flutter#37435) When the accumulator is an `RTreeBoundsAccumulator` rather than a `RectBoundsAccumulator` just accumulating the bounds results in incorrect results as the `rtree` would need to be aware of the constituent non-overlapping rectangles. This would work fine for `RectBoundsAccumulator` as it would just adjust its bounds based on the passed rects. Fixes: flutter/flutter#113251 * Add a test for nested display list rtree

…lutter#115025) * 6daa75ad4 Roll Skia from 0c8127b3dd7d to 6a056a26b3c5 (16 revisions) (flutter/engine#37438) * 6517ee8ee Roll Skia from 6a056a26b3c5 to 3cecb22c5827 (1 revision) (flutter/engine#37439) * c1200c44f Sdk roll failure - add facet to allowlist test_manager and other packages. (flutter/engine#37395) * 179154ec1 Revert "Sdk roll failure - add facet to allowlist test_manager and other packages. (flutter#37395)" (flutter/engine#37449) * c171f27c4 Revert "Add rects to accumulator rather than bounds (flutter#37435)" (flutter/engine#37444) * 583c71ce4 Clang-tidy: Fixed math on shard-id validator. (flutter/engine#37433) * d224648fc Roll Skia from 3cecb22c5827 to 94450cd1df4e (4 revisions) (flutter/engine#37445) * 5df42a87b Oops, accidentally compiled the non-null-safe platform file as null safe. (flutter/engine#37453) * 05455ac0a Roll Skia from 94450cd1df4e to 22526b55be02 (6 revisions) (flutter/engine#37455) * 9628070ee Roll Skia from 22526b55be02 to 511aa59461f3 (1 revision) (flutter/engine#37458) * fc7293441 [Impeller] Don't double-convert include path encodings in ImpellerC (flutter/engine#37408) * d82a2df8a Roll Skia from 511aa59461f3 to 790aedd91cd8 (2 revisions) (flutter/engine#37463) * be54e3ea0 Roll Skia from 790aedd91cd8 to fe143a0dc368 (3 revisions) (flutter/engine#37467) * 410f270e6 Roll Dart SDK from c15cdb978761 to 874a662f7507 (3 revisions) (flutter/engine#37466) * e778ca4e0 Roll Fuchsia Mac SDK from d4l6A1aPw6Z0YjxmA... to vEw8iuSZYmlfk1JiE... (flutter/engine#37468) * f8048be39 [Reland] Add rects to accumulator rather than bounds (flutter#37435) (flutter/engine#37451) * 214b84137 Roll Dart SDK from 874a662f7507 to 4fcefaf03496 (1 revision) (flutter/engine#37470) * 8ecb1c913 [macOS] Remove OpenGL rendering backend (flutter/engine#37448) * 1a8888b51 Roll Skia from fe143a0dc368 to 0854badd5783 (1 revision) (flutter/engine#37471) * ca303bfd4 Roll Dart SDK from 4fcefaf03496 to fa6586382e5d (1 revision) (flutter/engine#37473) * 5d0df764b Use traingle strip for difference clip (flutter/engine#37469) * eeba9b3c8 Support HLSL ingestion in ImpellerC (flutter/engine#37461) * 5f88ee42b Use IndexError.check instead of RangeError.checkValidIndex in buffers. (flutter/engine#37450) * b65ac4ac4 Roll expat to 2.5.0 (flutter/engine#37383) * ddde8300e [sdk_roll_failure] add facet to allowlist test_manager and other packages. (flutter/engine#37479) * 62b581ee0 Roll CanvasKit to 0.37.1 (flutter/engine#37454) * aa4b3ea2f Turned on performance-unnecessary-value-param everywhere. (flutter/engine#37447)

iskakaushik requested review from jmagman and flar November 8, 2022 23:10

flutter-dashboard bot added the needs tests label Nov 8, 2022

This was referenced Nov 8, 2022

Use proper attribute types when delegating to accumulator in drawDisplayList flutter/flutter#114919

Open

Revert "Add rects to accumulator rather than bounds (#37435)" #37444

Merged

flar approved these changes Nov 9, 2022

View reviewed changes

iskakaushik added the autosubmit Merge PR when tree becomes green via auto submit App label Nov 9, 2022

auto-submit bot removed the autosubmit Merge PR when tree becomes green via auto submit App label Nov 9, 2022

flar approved these changes Nov 9, 2022

View reviewed changes

iskakaushik added 2 commits November 9, 2022 05:14

Add a test for nested display list rtree

fe260ce

iskakaushik force-pushed the reland-pv-fix branch from e38447e to fe260ce Compare November 9, 2022 10:14

iskakaushik added the autosubmit Merge PR when tree becomes green via auto submit App label Nov 9, 2022

auto-submit bot merged commit f8048be into flutter:main Nov 9, 2022

engine-flutter-autoroll mentioned this pull request Nov 9, 2022

Roll Flutter Engine from e7d7edab98ad to f8048be39a7c (30 revisions) flutter/flutter#114969

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Nov 9, 2022

f8048be39 [Reland] Add rects to accumulator rather than bounds (flutt…

9a4c4d3

…er#37435) (flutter/engine#37451)

engine-flutter-autoroll mentioned this pull request Nov 9, 2022

Roll Flutter Engine from e7d7edab98ad to 214b84137bbe (31 revisions) flutter/flutter#114972

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Nov 9, 2022

f8048be39 [Reland] Add rects to accumulator rather than bounds (flutt…

5377c1d

…er#37435) (flutter/engine#37451)

engine-flutter-autoroll mentioned this pull request Nov 9, 2022

Roll Flutter Engine from e7d7edab98ad to 214b84137bbe (31 revisions) flutter/flutter#114976

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Nov 9, 2022

f8048be39 [Reland] Add rects to accumulator rather than bounds (flutt…

f370242

…er#37435) (flutter/engine#37451)

engine-flutter-autoroll mentioned this pull request Nov 9, 2022

Roll Flutter Engine from e7d7edab98ad to 8ecb1c913ba5 (32 revisions) flutter/flutter#114977

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Nov 9, 2022

f8048be39 [Reland] Add rects to accumulator rather than bounds (flutt…

c5fcdbd

…er#37435) (flutter/engine#37451)

engine-flutter-autoroll mentioned this pull request Nov 9, 2022

Roll Flutter Engine from e7d7edab98ad to 1a8888b5170b (33 revisions) flutter/flutter#114997

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Nov 9, 2022

f8048be39 [Reland] Add rects to accumulator rather than bounds (flutt…

a07fbe4

…er#37435) (flutter/engine#37451)

iskakaushik mentioned this pull request Nov 9, 2022

Revert "[Reland] Add rects to accumulator rather than bounds (#37435)… #37476

Closed

engine-flutter-autoroll mentioned this pull request Nov 9, 2022

Roll Flutter Engine from e7d7edab98ad to ca303bfd4eed (34 revisions) flutter/flutter#115001

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Nov 9, 2022

f8048be39 [Reland] Add rects to accumulator rather than bounds (flutt…

1c9b817

…er#37435) (flutter/engine#37451)

engine-flutter-autoroll mentioned this pull request Nov 9, 2022

Roll Flutter Engine from e7d7edab98ad to 5d0df764b03a (35 revisions) flutter/flutter#115003

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Nov 9, 2022

f8048be39 [Reland] Add rects to accumulator rather than bounds (flutt…

1e603ed

…er#37435) (flutter/engine#37451)

engine-flutter-autoroll mentioned this pull request Nov 9, 2022

Roll Flutter Engine from e7d7edab98ad to 5f88ee42b1c3 (37 revisions) flutter/flutter#115011

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Nov 9, 2022

f8048be39 [Reland] Add rects to accumulator rather than bounds (flutt…

0c363a6

…er#37435) (flutter/engine#37451)

engine-flutter-autoroll mentioned this pull request Nov 9, 2022

Roll Flutter Engine from c76035429c36 to b65ac4ac4776 (24 revisions) flutter/flutter#115016

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Nov 9, 2022

f8048be39 [Reland] Add rects to accumulator rather than bounds (flutt…

a214b2e

…er#37435) (flutter/engine#37451)

engine-flutter-autoroll mentioned this pull request Nov 9, 2022

Roll Flutter Engine from c76035429c36 to aa4b3ea2f733 (27 revisions) flutter/flutter#115022

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Nov 9, 2022

f8048be39 [Reland] Add rects to accumulator rather than bounds (flutt…

b154045

…er#37435) (flutter/engine#37451)

engine-flutter-autoroll mentioned this pull request Nov 9, 2022

Roll Flutter Engine from c76035429c36 to aa4b3ea2f733 (27 revisions) flutter/flutter#115025

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Nov 9, 2022

f8048be39 [Reland] Add rects to accumulator rather than bounds (flutt…

277c4bd

…er#37435) (flutter/engine#37451)

ColdPaleLight mentioned this pull request Dec 8, 2022

Collapse bounds calculations into DisplayListBuilder #34365

Merged

8 tasks

flar mentioned this pull request Aug 28, 2023

Add benchmark for scrolling very long pictures flutter/flutter#133434

Merged

flar mentioned this pull request Nov 30, 2023

Drawing RTree DisplayList into another RTree DisplayList ferries all of the rectangles flutter/flutter#139323

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Reland] Add rects to accumulator rather than bounds (#37435) #37451

[Reland] Add rects to accumulator rather than bounds (#37435) #37451

iskakaushik commented Nov 8, 2022

flutter-dashboard bot commented Nov 8, 2022

flar left a comment

flar Nov 9, 2022 •

edited

Loading

flar commented Nov 9, 2022

iskakaushik commented Nov 9, 2022 •

edited

Loading

auto-submit bot commented Nov 9, 2022

flar left a comment

[Reland] Add rects to accumulator rather than bounds (#37435) #37451

[Reland] Add rects to accumulator rather than bounds (#37435) #37451

Conversation

iskakaushik commented Nov 8, 2022

flutter-dashboard bot commented Nov 8, 2022

flar left a comment

Choose a reason for hiding this comment

flar Nov 9, 2022 • edited Loading

Choose a reason for hiding this comment

flar commented Nov 9, 2022

iskakaushik commented Nov 9, 2022 • edited Loading

auto-submit bot commented Nov 9, 2022

flar left a comment

Choose a reason for hiding this comment

flar Nov 9, 2022 •

edited

Loading

iskakaushik commented Nov 9, 2022 •

edited

Loading