[skwasm] Scene builder optimizations for platform view placement #54949

eyebrowsoffire · 2024-09-03T21:49:48Z

This PR refactors the scene builder's logic in order to more aggressively merge flutter content and platform view content together. This essentially covers the case discussed in this flutter issue: flutter/flutter#149863

This optimization ensures that each picture or platform view is applied to the lowest possible slice in the scene, which avoids the proliferation of redundant slices and overlays in the scene.

flutter-dashboard · 2024-09-06T20:31:30Z

Golden file changes have been found for this pull request. Click here to view and triage (e.g. because this is an intentional change).

If you are still iterating on this change and are not ready to resolve the images on the Flutter Gold dashboard, consider marking this PR as a draft pull request above. You will still be able to view image results on the dashboard, commenting will be silenced, and the check will not try to resolve itself until marked ready for review.

Changes reported for pull request #54949 at sha d1e53d3

harryterkelsen

Approach mostly LGTM but it looks like there are a couple bugs to squash. More comments would be very helpful

harryterkelsen · 2024-09-11T20:27:16Z

lib/web_ui/lib/src/engine/layers.dart

@@ -477,6 +597,16 @@ class PlatformViewPosition {

  bool get isZero => (offset == null) && (transform == null);

+  ui.Rect mapLocalToGlobal(ui.Rect rect) {
+    if (offset != null) {
+      return rect.shift(offset!);


do we want to return here? can there be both an offset and a transform?

I suppose I should add a comment that says this exactly, but either one is set or the other, never both. Or maybe I should just refactor this to have subclasses and stuff.

harryterkelsen · 2024-09-11T20:27:53Z

lib/web_ui/lib/src/engine/layers.dart

+      return rect.shift(offset!);
+    }
+    if (transform != null) {
+      transform!.transformRect(rect);


I think you want rect = transform!.transformRect(rect) here. Otherwise, this is a no-op.

Good catch, thank you.

lib/web_ui/lib/src/engine/layers.dart

harryterkelsen · 2024-09-11T20:54:01Z

lib/web_ui/lib/src/engine/scene_builder.dart

    );
  }

+  int _placePicture(ui.Offset offset, ScenePicture picture) {


more documentation would be very helpful to understand what is going on with LayerOperation and LayerSlice and LayerSliceBuilder

something small like "Returns the slice index for [picture] at [offset]. The picture is added to the last slice where it either intersects with a picture in the slice or it intersects with a platform view in the preceding slice. If the picture intersects with a platform view in the last slice, a new slice is added at the end and the picture goes in there." That's very clunky wording which could be improved a lot but basically a comment explaining what this method is trying to do would be helpful in understanding the purpose of the slices and how we are trying to optimize.

Good call, I'll add some comments.

harryterkelsen · 2024-09-11T20:54:47Z

lib/web_ui/lib/src/engine/scene_builder.dart

+      }
+      sliceIndex--;
+    }
+    if (sliceIndex == sceneSlices.length) {


impossible to hit this condition

You're right, I should remove this.

lib/web_ui/lib/src/engine/scene_builder.dart

harryterkelsen

LGTM

…ement (flutter/engine#54949)

auto-submit · 2024-09-13T22:15:20Z

A reason for requesting a revert of flutter/engine/54949 could
not be found or the reason was not properly formatted. Begin a comment with 'Reason for revert:' to tell the bot why
this issue is being reverted.

eyebrowsoffire · 2024-09-13T22:15:34Z

Reason for revert: Incorrect golden diffs on engine roll, see flutter/flutter#155181

…ent (#54949)" This reverts commit 950f92f.

…ment (#54949)" (#55193) Reverts: #54949 Initiated by: eyebrowsoffire Reason for reverting: Incorrect golden diffs on engine roll, see flutter/flutter#155181 Original PR Author: eyebrowsoffire Reviewed By: {harryterkelsen} This change reverts the following previous change: This PR refactors the scene builder's logic in order to more aggressively merge flutter content and platform view content together. This essentially covers the case discussed in this flutter issue: flutter/flutter#149863 This optimization ensures that each picture or platform view is applied to the lowest possible slice in the scene, which avoids the proliferation of redundant slices and overlays in the scene.

…ement (flutter/engine#54949)

…155194) flutter/engine@ab9daaa...4d8d851 2024-09-14 [email protected] Synthesize remove events on `PointerChange.ACTION_UP` and `PointerChange.ACTION_POINTER_UP` (flutter/engine#55157) 2024-09-13 [email protected] Roll Skia from bdc5e73cb6c9 to 2b8e33aa4824 (1 revision) (flutter/engine#55192) 2024-09-13 [email protected] Roll Dart SDK from 302b6472b849 to c0f7e399ff4a (1 revision) (flutter/engine#55191) 2024-09-13 98614782+auto-submit[bot]@users.noreply.github.com Reverts "[skwasm] Scene builder optimizations for platform view placement (#54949)" (flutter/engine#55193) 2024-09-13 [email protected] Delete VolatilePathTracker in favor of Dispatch tracking (flutter/engine#55125) 2024-09-13 [email protected] Roll Fuchsia Linux SDK from 3YH1DEYJ-s93fHBw5... to -kKh_AYzPh_iEmTxK... (flutter/engine#55190) 2024-09-13 [email protected] Roll Skia from 9877f459399a to bdc5e73cb6c9 (1 revision) (flutter/engine#55189) 2024-09-13 [email protected] [impeller] add Android flag for disabling surface control for debugging. (flutter/engine#55185) 2024-09-13 [email protected] Roll Skia from a5a6d12b3642 to 9877f459399a (2 revisions) (flutter/engine#55187) 2024-09-13 [email protected] Roll Dart SDK from eb664303c5ff to 302b6472b849 (2 revisions) (flutter/engine#55182) 2024-09-13 [email protected] [skwasm] Scene builder optimizations for platform view placement (flutter/engine#54949) 2024-09-13 [email protected] add back test itSendsTextShowPasswordToFrameworkOnAttach with new mock for display metrics (flutter/engine#55110) Also rolling transitive DEPS: fuchsia/sdk/core/linux-amd64 from 3YH1DEYJ-s93 to -kKh_AYzPh_i 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 [email protected],[email protected],[email protected] 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

eyebrowsoffire added 2 commits August 30, 2024 16:41

WIP of skwasm occlusion logic stuff.

b401e16

WIP of skwasm occlusion optimizations.

3a7d9c7

github-actions bot added the platform-web Code specifically for the web engine label Sep 3, 2024

eyebrowsoffire added 7 commits September 4, 2024 09:55

Some fixes.

5df0b5b

Merge branch 'main' into skwasm_occlusion

513b283

Add grid view test.

e00c698

Optimized to avoid redraw.

9e5c703

Use a real bounding box hierarchy.

150c87a

Fix math imports.

57f59b1

Added some comments.

d1e53d3

eyebrowsoffire marked this pull request as ready for review September 6, 2024 19:51

eyebrowsoffire changed the title ~~Skwasm occlusion~~ [skwasm] Scene builder optimizations for platform view placement Sep 6, 2024

flutter-dashboard bot added the will affect goldens label Sep 6, 2024

Recursively insert to build a more balanced occlusion tree.

3f563da

eyebrowsoffire requested a review from harryterkelsen September 9, 2024 20:09

harryterkelsen requested changes Sep 11, 2024

View reviewed changes

Addressed Harry's comments.

87287a0

harryterkelsen approved these changes Sep 13, 2024

View reviewed changes

eyebrowsoffire added the autosubmit Merge PR when tree becomes green via auto submit App label Sep 13, 2024

auto-submit bot merged commit 950f92f into flutter:main Sep 13, 2024
30 checks passed

engine-flutter-autoroll mentioned this pull request Sep 13, 2024

Roll Flutter Engine from ab9daaa0bcc7 to 715ede5967c8 (3 revisions) flutter/flutter#155175

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Sep 13, 2024

950f92f66 [skwasm] Scene builder optimizations for platform view plac…

8fe7538

…ement (flutter/engine#54949)

engine-flutter-autoroll mentioned this pull request Sep 13, 2024

Roll Flutter Engine from ab9daaa0bcc7 to 94f56a2c6b85 (4 revisions) flutter/flutter#155181

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Sep 13, 2024

950f92f66 [skwasm] Scene builder optimizations for platform view plac…

6f7d72b

…ement (flutter/engine#54949)

eyebrowsoffire added the revert Label used to revert changes in a closed and merged pull request. label Sep 13, 2024

auto-submit bot removed the revert Label used to revert changes in a closed and merged pull request. label Sep 13, 2024

eyebrowsoffire added the revert Label used to revert changes in a closed and merged pull request. label Sep 13, 2024

auto-submit bot pushed a commit that referenced this pull request Sep 13, 2024

Revert "[skwasm] Scene builder optimizations for platform view placem…

eccda26

…ent (#54949)" This reverts commit 950f92f.

auto-submit bot mentioned this pull request Sep 13, 2024

Reverts "[skwasm] Scene builder optimizations for platform view placement (#54949)" #55193

Merged

auto-submit bot removed the revert Label used to revert changes in a closed and merged pull request. label Sep 13, 2024

engine-flutter-autoroll mentioned this pull request Sep 14, 2024

Roll Flutter Engine from ab9daaa0bcc7 to 4d8d8517a0ff (12 revisions) flutter/flutter#155194

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Sep 14, 2024

950f92f66 [skwasm] Scene builder optimizations for platform view plac…

1f97d1a

…ement (flutter/engine#54949)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[skwasm] Scene builder optimizations for platform view placement #54949

[skwasm] Scene builder optimizations for platform view placement #54949

eyebrowsoffire commented Sep 3, 2024 •

edited

Loading

flutter-dashboard bot commented Sep 6, 2024

harryterkelsen left a comment

harryterkelsen Sep 11, 2024

eyebrowsoffire Sep 11, 2024

harryterkelsen Sep 11, 2024

eyebrowsoffire Sep 11, 2024

harryterkelsen Sep 11, 2024

eyebrowsoffire Sep 11, 2024

harryterkelsen Sep 11, 2024

eyebrowsoffire Sep 11, 2024

harryterkelsen left a comment

auto-submit bot commented Sep 13, 2024

eyebrowsoffire commented Sep 13, 2024

[skwasm] Scene builder optimizations for platform view placement #54949

[skwasm] Scene builder optimizations for platform view placement #54949

Conversation

eyebrowsoffire commented Sep 3, 2024 • edited Loading

flutter-dashboard bot commented Sep 6, 2024

harryterkelsen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

harryterkelsen left a comment

Choose a reason for hiding this comment

auto-submit bot commented Sep 13, 2024

eyebrowsoffire commented Sep 13, 2024

eyebrowsoffire commented Sep 3, 2024 •

edited

Loading