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

Add Flutter RTree #16923

Merged
merged 23 commits into from
Mar 10, 2020
Merged

Add Flutter RTree #16923

merged 23 commits into from
Mar 10, 2020

Conversation

blasten
Copy link

@blasten blasten commented Mar 4, 2020

This PR adds the Flutter R-tree, which is based on https://chromium.googlesource.com/skia/+/master/src/core/SkRTree.h.

For context about how this is used, refer to http://go/flutter-hybrid-composition and #17049.

@blasten blasten requested review from chinmaygarde and amirh March 4, 2020 07:12
@auto-assign auto-assign bot requested a review from iskakaushik March 4, 2020 07:14
@amirh
Copy link
Contributor

amirh commented Mar 4, 2020

Would be easier to review if the delta from Skia's implementation is visible. Any chance you can split this into 2 commits one is a pure copy and the other is the actual delta?

I'm not sure if we have any precedence of copying over some non-trivial Skia code, as this essentially becomes part of Flutter's codebase now should we copy over Skia's unit tests as well? (@chinmaygarde what's your take?)

Did we check whether Skia are willing to make the RTree public? in that case would we be able to land our changes in a subclass?

@blasten
Copy link
Author

blasten commented Mar 5, 2020

As discussed offline, I moved the rtree to third_party/skia. I pushed two commits: 7800778 is the fork from Skia, dd54c47 the implementation of the searchRect method, and additional tests in 7cc254a and 47731ae.

@amirha raised a concern about changing the code too much, so I copied the .clang-format file from https://github.com/google/skia/blob/master/.clang-format.

In the meanwhile, I have filed flutter/flutter#52000 to keep track of the remaining work.

If this plan sounds ok, please take another look when you get a chance. @chinmaygarde @amirh.

Copy link
Member

@chinmaygarde chinmaygarde left a comment

Choose a reason for hiding this comment

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

My understanding of the work items as discussed offline was:

  • We are going to be sole owners of this component moving forward.
  • Due to the compressed timeline of this deliverable, we are not going to reformat this to follow engine style guides. Instead, we will file a bug to tackle this tech debt at a later time.
  • We are going to thoroughly document the public methods and the expected behavior via Doxygen comments (as we not familiar with the internals of this component but we are now tasked with owning it).

Other construction issues added inline.

@blasten blasten requested review from chinmaygarde and amirh March 5, 2020 22:18
Copy link
Contributor

@amirh amirh left a comment

Choose a reason for hiding this comment

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

Reviewed only the delta from Skia's implementation (blasten/engine@7800778...4440e96#diff-6adb5cf35e31186258beded1d7d56102R229 )

@blasten blasten requested a review from amirh March 6, 2020 00:14
recording_canvas->drawRect(SkRect::MakeLTRB(20, 20, 40, 40), rect_paint);
recorder->finishRecordingAsPicture();

auto hits = r_tree->searchRects(SkRect::MakeLTRB(40, 40, 80, 80));
Copy link
Contributor

Choose a reason for hiding this comment

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

Doesn't this test case means the platform views has a single pixel intersection with the drawing? (pixel 40, 40)

This comment was marked as outdated.

Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure I follow, does it mean that due to this rtree implementation we will end up with pixel that should be drawn on top of a platform view but will instead be drawn below?

Copy link
Author

@blasten blasten Mar 6, 2020

Choose a reason for hiding this comment

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

It looks like the bounds are exclusive: https://fiddle.skia.org/c/7c60511e363665b7588c8bf262220005

Copy link
Author

Choose a reason for hiding this comment

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

To your point, I don't think any pixels will be drawn on the overlay, since the height will be 0 in this case.

Copy link
Author

Choose a reason for hiding this comment

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

In your example, (40, 40, 80, 80) platform view and (20, 20, 40, 40) blue rectangle on top should result in no overlay. The size of this intersection is the equivalent of the rect: (40, 40, 40, 40), which is empty. This is my understanding, but I could be wrong. I can ask the Skia folks to confirm.

Copy link
Contributor

Choose a reason for hiding this comment

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

What should be the color of the pixel at (40,40) in the example above with the red platform view and the blue overlay?

Copy link
Author

Choose a reason for hiding this comment

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

The right and bottom values are exclusive. The pixel at (40,40,41,41) is the platform view itself

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not convinced that a flow platform_view layer that positions a platform view at 0,0, with no transforms will result in the platform view positioned on the screen at (1,1).
Anyway it's a matter of what values we pass in, to my current understanding we need to query for a bigger rect than the actual platform view, but I may very well be missing something. The discussion of what to pass in belongs in the next PR. For this PR lets make sure the comment is explicit about not including the query's edges.

Copy link
Author

Choose a reason for hiding this comment

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

resolved offline. SkRect::MakeLTRB(0,0,1,1) paints 1 pixel.

@blasten blasten requested a review from amirh March 6, 2020 18:58
Copy link
Contributor

@amirh amirh left a comment

Choose a reason for hiding this comment

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

LGTM for the parts I reviewed modulo the last comment nit.

I reviewed the delta from SkRTree. Please wait for Chinmay's approval as well as I'm not familiar with the engine's build/formatting/licensing stuff.

Copy link
Member

@chinmaygarde chinmaygarde left a comment

Choose a reason for hiding this comment

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

I believe the concerns around style and ownership have already been expressed and tracked in linked issues. Once the license is sorted out, I believe this is good to go.

@blasten blasten merged commit d884e27 into flutter:master Mar 10, 2020
@blasten blasten deleted the flutter_rtree branch March 10, 2020 22:47
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Mar 11, 2020
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Mar 11, 2020
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Mar 11, 2020
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Mar 11, 2020
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Mar 11, 2020
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Mar 11, 2020
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Mar 11, 2020
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Mar 11, 2020
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Mar 11, 2020
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Mar 11, 2020
jason-simmons added a commit to jason-simmons/flutter that referenced this pull request Mar 11, 2020
2020-03-11 [email protected] Roll src/third_party/dart 4093d08271f6..37530145ff53 (4 commits) (flutter/engine#17090)
2020-03-11 [email protected] Roll src/third_party/skia bf355123ae3b..0340292972b9 (9 commits) (flutter/engine#17089)
2020-03-11 [email protected] Roll fuchsia/sdk/core/mac-amd64 from r_oCI... to 0Z8VF... (flutter/engine#17087)
2020-03-11 [email protected] Roll fuchsia/sdk/core/linux-amd64 from v32mJ... to X3Xm2... (flutter/engine#17086)
2020-03-11 [email protected] Remove the unused method on iOS surface to make the resource context current. (flutter/engine#17084)
2020-03-11 [email protected] Revert "Add support for the Metal backend on all iOS builds. (flutter#17080)" (flutter/engine#17088)
2020-03-11 [email protected] Roll src/third_party/dart ace1d9b9213a..4093d08271f6 (12 commits) (flutter/engine#17082)
2020-03-11 [email protected] Add support for the Metal backend on all iOS builds. (flutter/engine#17080)
2020-03-11 [email protected] Roll src/third_party/skia d3f67dbf9f36..bf355123ae3b (9 commits) (flutter/engine#17079)
2020-03-11 [email protected] Disable Embedder11yTest::A11yTreeIsConsistent to unblock LUCI. (flutter/engine#17081)
2020-03-10 [email protected] Gather demangled stack traces and report the same to console on crashes. (flutter/engine#16450)
2020-03-10 [email protected] Implement asynchronous texture uploads when using the Metal backend on iOS. (flutter/engine#17046)
2020-03-10 [email protected] Roll src/third_party/dart 97674262bc29..ace1d9b9213a (14 commits) (flutter/engine#17078)
2020-03-10 [email protected] Add RTree to flow (flutter/engine#16923)
2020-03-10 [email protected] Roll src/third_party/skia 78dac6dcb222..d3f67dbf9f36 (6 commits) (flutter/engine#17072)
2020-03-10 [email protected] Revert "Fix bounds of image_filter_layer (flutter#16960)" (flutter/engine#17074)
2020-03-10 [email protected] Use the ELF loader to setup AOT symbols in benchmark runner. (flutter/engine#17051)
2020-03-10 [email protected] Roll src/third_party/skia 23899c64e3db..78dac6dcb222 (19 commits) (flutter/engine#17069)
2020-03-10 [email protected] Roll dart to 97674262bc29447dc59d5c93024b18b27d4bcf98. (flutter/engine#17067)
2020-03-10 [email protected] [web] Fixes for Firefox & Safari double underline decoration bugs. (flutter/engine#16994)
2020-03-10 [email protected] Avoid capturing this unsafely in MultiFrameCodec (flutter/engine#16824)
2020-03-10 [email protected] Revert "Revert "fix shadows and mask filter blurs (flutter#16963)" (flutter#17008)" (flutter/engine#17040)
2020-03-10 [email protected] Add support for firefox mac installer. Update web_ui pubspec for http.wq (flutter/engine#17044)
2020-03-09 [email protected] fix "TREE INCONSISTENT" noise in compositing_test.dart (flutter/engine#16995)
2020-03-09 [email protected] Add more child lifecycle tests (flutter/engine#16689)
2020-03-09 [email protected] Add libfreetype6-dev to desktop Linux dependencies (flutter/engine#17020)
2020-03-09 [email protected] Disable shell benchmarks (flutter/engine#17038)
2020-03-09 [email protected] Fix bounds of image_filter_layer (flutter/engine#16960)
2020-03-09 [email protected] Record fml and shell benchmarks (flutter/engine#16991)
2020-03-09 [email protected] Roll src/third_party/skia c56950442dd1..23899c64e3db (11 commits) (flutter/engine#17033)
2020-03-09 [email protected] use commit date instead of author date (flutter/engine#17032)
jason-simmons added a commit to flutter/flutter that referenced this pull request Mar 11, 2020
2020-03-11 [email protected] Roll src/third_party/dart 4093d08271f6..37530145ff53 (4 commits) (flutter/engine#17090)
2020-03-11 [email protected] Roll src/third_party/skia bf355123ae3b..0340292972b9 (9 commits) (flutter/engine#17089)
2020-03-11 [email protected] Roll fuchsia/sdk/core/mac-amd64 from r_oCI... to 0Z8VF... (flutter/engine#17087)
2020-03-11 [email protected] Roll fuchsia/sdk/core/linux-amd64 from v32mJ... to X3Xm2... (flutter/engine#17086)
2020-03-11 [email protected] Remove the unused method on iOS surface to make the resource context current. (flutter/engine#17084)
2020-03-11 [email protected] Revert "Add support for the Metal backend on all iOS builds. (#17080)" (flutter/engine#17088)
2020-03-11 [email protected] Roll src/third_party/dart ace1d9b9213a..4093d08271f6 (12 commits) (flutter/engine#17082)
2020-03-11 [email protected] Add support for the Metal backend on all iOS builds. (flutter/engine#17080)
2020-03-11 [email protected] Roll src/third_party/skia d3f67dbf9f36..bf355123ae3b (9 commits) (flutter/engine#17079)
2020-03-11 [email protected] Disable Embedder11yTest::A11yTreeIsConsistent to unblock LUCI. (flutter/engine#17081)
2020-03-10 [email protected] Gather demangled stack traces and report the same to console on crashes. (flutter/engine#16450)
2020-03-10 [email protected] Implement asynchronous texture uploads when using the Metal backend on iOS. (flutter/engine#17046)
2020-03-10 [email protected] Roll src/third_party/dart 97674262bc29..ace1d9b9213a (14 commits) (flutter/engine#17078)
2020-03-10 [email protected] Add RTree to flow (flutter/engine#16923)
2020-03-10 [email protected] Roll src/third_party/skia 78dac6dcb222..d3f67dbf9f36 (6 commits) (flutter/engine#17072)
2020-03-10 [email protected] Revert "Fix bounds of image_filter_layer (#16960)" (flutter/engine#17074)
2020-03-10 [email protected] Use the ELF loader to setup AOT symbols in benchmark runner. (flutter/engine#17051)
2020-03-10 [email protected] Roll src/third_party/skia 23899c64e3db..78dac6dcb222 (19 commits) (flutter/engine#17069)
2020-03-10 [email protected] Roll dart to 97674262bc29447dc59d5c93024b18b27d4bcf98. (flutter/engine#17067)
2020-03-10 [email protected] [web] Fixes for Firefox & Safari double underline decoration bugs. (flutter/engine#16994)
2020-03-10 [email protected] Avoid capturing this unsafely in MultiFrameCodec (flutter/engine#16824)
2020-03-10 [email protected] Revert "Revert "fix shadows and mask filter blurs (#16963)" (#17008)" (flutter/engine#17040)
2020-03-10 [email protected] Add support for firefox mac installer. Update web_ui pubspec for http.wq (flutter/engine#17044)
2020-03-09 [email protected] fix "TREE INCONSISTENT" noise in compositing_test.dart (flutter/engine#16995)
2020-03-09 [email protected] Add more child lifecycle tests (flutter/engine#16689)
2020-03-09 [email protected] Add libfreetype6-dev to desktop Linux dependencies (flutter/engine#17020)
2020-03-09 [email protected] Disable shell benchmarks (flutter/engine#17038)
2020-03-09 [email protected] Fix bounds of image_filter_layer (flutter/engine#16960)
2020-03-09 [email protected] Record fml and shell benchmarks (flutter/engine#16991)
2020-03-09 [email protected] Roll src/third_party/skia c56950442dd1..23899c64e3db (11 commits) (flutter/engine#17033)
2020-03-09 [email protected] use commit date instead of author date (flutter/engine#17032)
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