Test for invalid elevations #27137

dnfield · 2019-01-26T23:34:49Z

Updates the paint method on _RenderPhysicalModelBase to ask the pipeline owner to check the elevation in the area of the screen it wants to paint in. If there is such a problem, a FlutterError is thrown and the screen is painted with a path outlining the two problematic elevations. This is only done in debug mode (or at least only when asserts are enabled).

The algorithm used has a worst-case performance of 1 + 2 + 3 + 4 + .. + n, where n is the number of _RenderPhysicalModelBase objects in the render tree (which should be a small subset of total render objects in the tree for a given frame). I've added a parameter to control how many iterations we acutally check for (so at runtime it can be set lower if needed). The default is completely arbitrary right now, I'll try to test it on an older phone soon. The default for tests is arbitrarily high to be more aggressive about catching them in testing scenarios - but even for a specific test they can be turned off.

dnfield · 2019-01-26T23:53:39Z

A couple screen shot of this in action. In both, the top shapes are a stack with two overlapping materials with elevations out of order; the bottom has two materials also out of order elevations; in the first screen shot they're overlapping, in the second they're not.

dnfield · 2019-01-27T00:22:32Z

I have to fix the tests. I tried this at one point and it was passing, but probably because the implementation was off.

dnfield · 2019-01-28T03:38:06Z

Marking this as WIP until I can figure out a way to determine whether a widget should be excluded from this test or not. There seems to be a whole class of objects that ask to paint but don't actually affect the rendering.

dnfield · 2019-01-28T19:11:56Z

There's still a potentially open question about this around whether we really should scope to overlays (or, RenderOffstages here).

I'd like to leave it as is for now, as it at least gives a check for a lot of common cases. We can figure out how to handle this for transitions once we get some more input from the folks who want this.

packages/flutter/lib/src/rendering/object.dart

goderbauer · 2019-01-30T18:46:21Z

packages/flutter/lib/src/rendering/object.dart

+    final Matrix4 matrix = Matrix4.identity()..translate(offset.dx, offset.dy);
+    return p.transform(matrix.storage);
+  }
+  bool _printedExceededmaxElevationObjectsToCheckWarning = false;


Should Max be capitalized in here?

goderbauer · 2019-01-30T18:47:19Z

packages/flutter/lib/src/rendering/object.dart

+  /// [RenderPhysicalModel] or [RenderPhysicalShape]. The [area] parameter
+  /// specifies the [Rect] on the screen in which the physical model will be
+  /// drawn, and the [path] parameter specifies the actual [Path] to be drawn.
+  /// The path is expected to be smaller than the area.


Can both be the same?

Yes - clarifying this...

goderbauer · 2019-01-30T18:47:32Z

packages/flutter/lib/src/rendering/object.dart

+  /// drawn, and the [path] parameter specifies the actual [Path] to be drawn.
+  /// The path is expected to be smaller than the area.
+  ///
+  /// If the durrent paint cycle results in drawing a shape at an elevation


durrent > current

goderbauer · 2019-01-30T18:48:51Z

packages/flutter/lib/src/rendering/object.dart

+    double elevation,
+    Rect area,
+    Path path,
+    RenderObject object,


Can you document these two parameters as well?

packages/flutter/lib/src/rendering/object.dart

goderbauer · 2019-01-30T19:18:56Z

packages/flutter/lib/src/rendering/object.dart

+
+  Map<RenderObject, List<_ElevationData>> _elevations;
+  Path _translatePath(Path p, Offset offset) {
+    final Matrix4 matrix = Matrix4.identity()..translate(offset.dx, offset.dy);


his probably also need to take other paint transformations into consideration? (applyPaintTransform maybe)

Done.

I could use some feedback on whether I'm right to ignore RenderView. That one ends up applying th edevicepixelratio to the transform, and it messes things up. But I'm not sure how else it might get used.

goderbauer · 2019-01-30T19:22:23Z

packages/flutter/lib/src/rendering/object.dart

+      );
+      // Check if the union created a path with a single segment. If so, the
+      // paths overlap.
+      // This will not work if we're dealing with input paths that are already


I wonder how common this is going to be - and how expensive it actually would be to account for it.

I've updated the logic to support this - if it's looking good I'll do some perf tests and we can figure out if it needs to go or maybe get guarded somehow.

packages/flutter/lib/src/rendering/object.dart

dnfield · 2019-02-02T00:13:16Z

@goderbauer This is ready for another review pass. I've added some tests to acocunt for a Transform and added a bunch of comments.

dnfield · 2019-02-05T06:34:43Z

@amirh I'm hoping you have some context on the failing test here (notched app bar with no margin).

The algorithm I'm using marks it as a problem, because we're trying to draw a FAB with elevation 6 after an app bar with elevation 8, and the edges are touching. I'm not entirely sure what that would do in Fuchsia, but it seems off (in fact, the whole shadow rendering on iOS and Android for this seems pretty wrong to me).

goderbauer · 2019-02-05T19:03:03Z

packages/flutter/lib/src/rendering/object.dart

+  ///
+  /// See also: [debugCheckElevationData].
+  int get maxElevationObjectsToCheck => _maxElevationObjectsToCheck;
+  int _maxElevationObjectsToCheck = 10;


I wonder what the default should be here. Setting this to 10 may break some existing apps? Should we have a way to opt-in to this?

goderbauer · 2019-02-05T19:04:43Z

packages/flutter/lib/src/rendering/object.dart

+  /// an `assert` from within the framework. All of the parameters must not be
+  /// null.
+  ///
+  /// The [elevation] refers to the elevation specified on the


Document if this value is relative to the parent or in terms of absolute elevation?

goderbauer · 2019-02-05T19:05:22Z

packages/flutter/lib/src/rendering/object.dart

+  /// [RenderPhysicalModel] or [RenderPhysicalShape]. The [area] parameter
+  /// specifies the [Rect] on the screen in which the physical model will be
+  /// drawn, and the [path] parameter specifies the actual [Path] to be drawn,
+  /// with the corret paint translations already applied. The path may be


What coordinate system are these in?

goderbauer · 2019-02-05T19:10:44Z

packages/flutter/lib/src/rendering/proxy_box.dart

+      final Matrix4 transform = Matrix4.identity()..translate(offset.dx, offset.dy);
+      while (ancestor != null) {
+        if (ancestor is _RenderPhysicalModelBase) {
+          // We're the direct child of another physical model. We need to add


Given the while loop, it may not be a direct child, but a grandchild or grand-grand-...-child etc?

goderbauer · 2019-02-05T19:25:29Z

packages/flutter/lib/src/rendering/proxy_box.dart

+        }
+        if (ancestor is RenderObject) {
+          // We don't want the devicePixelRatio included in this transform.
+          if (ancestor is! RenderView) {


This seems odd. Is the problem maybe that it introduces another transform and the algo can't handle two transformation?

It seems odd to me that the path drawn on the canvas below is not transformed back into the coordinate system of that canvas...

goderbauer · 2019-02-05T20:44:06Z

packages/flutter/lib/src/rendering/object.dart

+                 // doing the more expensive path comparison
+                 area.overlaps(elevationData.area) &&
+                 // ignore PhysicalModels that are direct descendants of this
+                 // one, in case a parent is getting checked after its children


This one is odd. Why would that happen?

goderbauer · 2019-02-05T20:45:34Z

I wonder if the problem with that failing test is that we mark shapes that are just touching as overlapping.

Hixie · 2019-02-05T23:21:48Z

Rather than breaking out of the transforms, I would recommend moving the logic for reporting this to somewhere higher-level, e.g. a widget child of WidgetsApp, the same way we do the DEBUG banner.

packages/flutter/lib/src/rendering/object.dart

goderbauer · 2019-02-13T22:48:53Z

I am closing this one for now since @dnfield is working on a new approach, which is going to be different from what's proposed here.

Test for invalid elevations

e58302a

dnfield requested review from Hixie and goderbauer January 26, 2019 23:34

googlebot added the cla: yes label Jan 26, 2019

dnfield added a: tests "flutter test", flutter_test, or one of our tests framework flutter/packages/flutter repository. See also f: labels. a: fidelity Matching the OEM platforms better customer: fuchsia customer: dream (g3) labels Jan 26, 2019

analysis fixes

3398ea2

dnfield mentioned this pull request Jan 27, 2019

Add docs and sample for takeException #27140

Merged

dnfield added 7 commits January 27, 2019 09:49

Avoid .length

160f3ee

Merge remote-tracking branch 'upstream/master' into elevations

1d1a2bf

avoid throwing during paint

f0d0983

Ignore offstage widgets

f66acd8

use renderoffstage

90ef29f

move offstage check, move tests

b89abe0

just check for object, not offstage prop

f940922

dnfield changed the title ~~Test for invalid elevations~~ [WIP] Test for invalid elevations Jan 28, 2019

dnfield added 2 commits January 28, 2019 00:26

scope to Overlay(Offstage), fix a couple tests

a845447

Move test, restrict checks to parent-most material

5fe8068

dnfield changed the title ~~[WIP] Test for invalid elevations~~ Test for invalid elevations Jan 28, 2019

dnfield and others added 4 commits January 28, 2019 23:38

Merge branch 'master' into elevations

ee7f535

revert unnecessary changes to test

00f2084

Merge branch 'elevations' of github.com:dnfield/flutter into elevations

9259a68

Merge remote-tracking branch 'upstream/master' into elevations

afbf88f

dnfield added 3 commits January 29, 2019 22:22

remove stale import

c7a80f1

Merge remote-tracking branch 'upstream/master' into elevations

764b794

revert file

33630a7

goderbauer reviewed Jan 30, 2019

View reviewed changes

dnfield added 2 commits February 1, 2019 15:41

Merge remote-tracking branch 'upstream/master' into elevations

74355f5

Support transforms, multi-contour widgets, add comments

aaf3372

dnfield added 5 commits February 1, 2019 21:15

fix import

8fb88f9

Test for total elevation

755da6f

Merge remote-tracking branch 'upstream/master' into elevations

c014d65

Merge remote-tracking branch 'upstream/master' into elevations

926a8b7

fix case where child is checked against parent.

0cd3640

goderbauer reviewed Feb 5, 2019

View reviewed changes

dnfield mentioned this pull request Feb 5, 2019

expose setMatrix for Canvas flutter/engine#7704

Closed

use intersect, fix translation

951ab85

Hixie reviewed Feb 5, 2019

View reviewed changes

packages/flutter/lib/src/rendering/object.dart Outdated Show resolved Hide resolved

dnfield added 3 commits February 5, 2019 16:14

remove setmatrix, remove unnecessary import

d7b0ec0

Merge remote-tracking branch 'upstream/master' into elevations

0e9a882

remove stale override

bce9497

goderbauer closed this Feb 13, 2019

dnfield mentioned this pull request Mar 29, 2019

Check for invalid elevations #30215

Merged

9 tasks

dnfield deleted the elevations branch January 7, 2020 03:57

github-actions bot locked as resolved and limited conversation to collaborators Aug 2, 2021

Test for invalid elevations #27137

Test for invalid elevations #27137

Uh oh!

Conversation

dnfield commented Jan 26, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dnfield commented Jan 26, 2019

Uh oh!

dnfield commented Jan 27, 2019

Uh oh!

dnfield commented Jan 28, 2019

Uh oh!

dnfield commented Jan 28, 2019

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

dnfield commented Feb 2, 2019

Uh oh!

dnfield commented Feb 5, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

goderbauer commented Feb 5, 2019

Uh oh!

Hixie commented Feb 5, 2019

Uh oh!

Uh oh!

goderbauer commented Feb 13, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

dnfield commented Jan 26, 2019 •

edited

Loading