Remove cullRect calculation on TransformLayers with a perspective transform. #5693

jonahwilliams · 2018-07-08T04:52:57Z

do not produce a cullRect from a TransformLayer with perspective.
Fix type error in Xcode-beta.

Fixes flutter/flutter#19062

The problem

Creating a transform layer with a perspective transform and a child layer that needs compositing causes said child to always be culled in the DefaultLayerBuilder. This was not a bug that was introduced recently, and I've confirmed this occurs at least two months back in master, (and I believe it has existed since basically forever). It can be reproduced trivially with the following snippet, The result of which is an empty screen. The red box will appear only when the perspective is commented out.

void main() {
  runApp(new Center(
    child: new Transform(
      transform: new Matrix4.identity()
        ..setEntry(3, 2, 0.001), // perspective transform
      child: new Ancestor( // new child layer
        child: new Container( // something to draw
          color: Colors.red,
            width: 200.0,
            height: 200.0,
          ),
       ),
   ));
}

class Ancestor extends SingleChildRenderObjectWidget {
  const Ancestor({Widget child, Key key}): super(child: child, key: key);
  
  @override
  RenderObject createRenderObject(BuildContext context) {
    return new RenderAncestor();
  }
}
class RenderAncestor extends RenderProxyBox {
  @override
  bool get alwaysNeedsCompositing => true; 

  @override
  void paint(PaintingContext context, Offset offset) {
    context.pushLayer(new OffsetLayer(), super.paint, offset);
  }
}

For reference, left with the perspective applied and right without.

According to the Flutter tool, the layer tree looks like the following - but this is not what is displayed.

flutter: TransformLayer#34ae6
flutter:  │ owner: RenderView#e6b34
flutter:  │ creator: [root]
flutter:  │ offset: Offset(0.0, 0.0)
flutter:  │ transform:
flutter:  │   [0] 3.0,0.0,0.0,0.0
flutter:  │   [1] 0.0,3.0,0.0,0.0
flutter:  │   [2] 0.0,0.0,1.0,0.0
flutter:  │   [3] 0.0,0.0,0.0,1.0
flutter:  │
flutter:  └─child 1: TransformLayer#53916
flutter:    │ offset: Offset(0.0, 0.0)
flutter:    │ transform:
flutter:    │   [0] 1.1875,0.0,0.0,-35.156250000000014
flutter:    │   [1] 0.406,1.0,0.0,-76.12499999999994
flutter:    │   [2] 0.0,0.0,1.0,0.0
flutter:    │   [3] 0.001,0.0,0.0,0.8125
flutter:    │
flutter:    └─child 1: OffsetLayer#87f88
flutter:      │ offset: Offset(0.0, 0.0)
flutter:      │
flutter:      └─child 1: PictureLayer#28202
flutter:          paint bounds: Rect.fromLTRB(29.6, -6090.0, 15187.5, 32886.0)
flutter:

Instead, the DefaultLayerBuilder is always culling the PictureLayer when the perspective transform is applied. The problematic culling rect in question is calculated in DefaultLayerBuilder::pushTransfrom.

engine/flow/layers/default_layer_builder.cc

Lines 37 to 49 in f52955c

    
           void DefaultLayerBuilder::PushTransform(const SkMatrix& sk_matrix) { 
        
             SkMatrix inverse_sk_matrix; 
        
             SkRect cullRect; 
        
             if (sk_matrix.invert(&inverse_sk_matrix)) { 
        
               inverse_sk_matrix.mapRect(&cullRect, cull_rects_.top()); 
        
             } else { 
        
               cullRect = kGiantRect; 
        
             } 
        
             auto layer = std::make_unique<flow::TransformLayer>(); 
        
             layer->set_transform(sk_matrix); 
        
             PushLayer(std::move(layer), cullRect); 
        
           }

With some assorted rotations applied and no perspective, a rect might be produced something like:
Rect.LTRB{-3.33333e+08, -4.71228e+09, 3.33333e+08, 4.71227e+09}. However, as soon as there is any amount of perspective you start getting less reasonable values like Rect.LTRB{-793.003, -13768.9, -792.997, 14504.9}. A matrix with a perspective does go through a separate code path in Skia here, but having checked it out nothing seems obviously wrong. Now, at this point i've been debugging for a while so I haven't really sat down and thought about the math too much (shame!)

The Solution (?)

The simplest solution to me seems to be just don't producing a cullingRect if the transform matrix has any perspective. I'm not sure how much work this particular culling heuristic is buying us, but this change wouldn't effect other common transforms like translations/rotations. I'm also open to exploring other solutions or digging into the linear algebra some more...

(Also I fixed a type error that prevented me from building on the latest xcode beta)

…o fix ios type error on xcode beta

chinmaygarde · 2018-07-09T18:46:58Z

flow/layers/default_layer_builder.cc

-  if (sk_matrix.invert(&inverse_sk_matrix)) {
+  // Perspective projections don't produce rectangles that are useful for
+  // culling for some reason.
+  if (sk_matrix.invert(&inverse_sk_matrix) && !inverse_sk_matrix.hasPerspective()) {


Don't you mean if the sk_matrix has a perspective?

Hixie · 2018-07-09T23:09:49Z

flow/layers/default_layer_builder.cc

  SkRect cullRect;
-  if (sk_matrix.invert(&inverse_sk_matrix)) {
+  // Perspective projections don't produce rectangles that are useful for
+  // culling for some reason.


do we know the reason?

Can you file a bug with the skia folks? Just to let them know, not high priority. Maybe there's a good reason (in which case we can update the comment), maybe it's a real bug.

(and add a link to the bug in this comment)

sounds good, I'll update with a issue when it's filled.

If you file a Skia bug, a reduced test case in https://fiddle.skia.org/ gets most actionable work items.

Do they have a public issue tracker somewhere?

http://go/newskbug

Oh nvm I see the link

Includes: * flutter/engine@fed2ea458 Revert "Remove vmservice_io.main from entry points. (flutter/engine#5625)" (flutter#5711) * flutter/engine@958d2cfc5 Remove trailing white spaces (flutter/engine#5708) * flutter/engine@6db0cc9da Roll src/third_party/skia 1d784ceafa9e..c69c4410be7d (13 commits) (flutter/engine#5709) * flutter/engine@494562442 Roll src/third_party/skia 418e658a6922..1d784ceafa9e (10 commits) (flutter/engine#5707) * flutter/engine@cb27dab2b Roll src/third_party/skia 6d98257725b5..418e658a6922 (1 commits) (flutter/engine#5706) * flutter/engine@2981cc7a3 Roll src/third_party/skia e0a9962b12ad..6d98257725b5 (1 commits) (flutter/engine#5704) * flutter/engine@ad4197280 Roll src/third_party/skia 8fe31406e980..e0a9962b12ad (3 commits) (flutter/engine#5703) * flutter/engine@3b6dedb86 Add anti-alias switch to canvas clip calls (flutter/engine#5670) * flutter/engine@d9a831c55 Roll dart to 84ca27a09ebd6a65cd23ee52d835d89cbe06c574. (flutter/engine#5700) * flutter/engine@d5fce84be revert change to app delegate that wont build on earlier xcodes (flutter/engine#5702) * flutter/engine@45db8f7c5 Roll src/third_party/skia d48897b576e9..8fe31406e980 (4 commits) (flutter/engine#5701) * flutter/engine@d217a9512 Remove cullRect calculation on TransformLayers with a perspective transform. (flutter/engine#5693) * flutter/engine@13d801ac2 Roll src/third_party/skia 1354048c8fa8..d48897b576e9 (8 commits) (flutter/engine#5699) * flutter/engine@1644f3589 Roll src/third_party/skia 373224c9ab3a..1354048c8fa8 (1 commits) (flutter/engine#5698) * flutter/engine@4da7d44f3 Roll src/third_party/skia a391c72a2057..373224c9ab3a (1 commits) (flutter/engine#5697) * flutter/engine@a36207f35 Roll src/third_party/skia 0dd98a59df63..a391c72a2057 (1 commits) (flutter/engine#5696) * flutter/engine@cf14ca0f7 Roll src/third_party/skia a717ca970b25..0dd98a59df63 (1 commits) (flutter/engine#5695) * flutter/engine@89cd29fb1 Roll src/third_party/skia dd962a95c5f2..a717ca970b25 (1 commits) (flutter/engine#5694) * flutter/engine@f52955c25 Roll src/third_party/skia bcc3aec00422..dd962a95c5f2 (1 commits) (flutter/engine#5692) * flutter/engine@6e6512287 Roll src/third_party/skia 4f6ea65f66c9..bcc3aec00422 (2 commits) (flutter/engine#5691) * flutter/engine@e3347680c Roll src/third_party/skia a50205fca5f7..4f6ea65f66c9 (2 commits) (flutter/engine#5690) * flutter/engine@8aa9a8158 Roll src/third_party/skia 6667fb123911..a50205fca5f7 (3 commits) (flutter/engine#5689) * flutter/engine@3c4bf8de3 Roll src/third_party/skia 3d3d8841ff86..6667fb123911 (1 commits) (flutter/engine#5688) * flutter/engine@f5f15b78b Roll src/third_party/skia b95bbba308b9..3d3d8841ff86 (1 commits) (flutter/engine#5687) * flutter/engine@31bd1e29a Roll src/third_party/skia 20b824d9fe49..b95bbba308b9 (1 commits) (flutter/engine#5686) * flutter/engine@37e96b9e6 Roll src/third_party/skia 53f5db5e9833..20b824d9fe49 (1 commits) (flutter/engine#5685) * flutter/engine@b04fc96c2 Roll src/third_party/skia 3f2d909d7029..53f5db5e9833 (1 commits) (flutter/engine#5683) * flutter/engine@686ee99d8 Roll src/third_party/skia 9eb1c7d80fb3..3f2d909d7029 (1 commits) (flutter/engine#5682) * flutter/engine@936a38dda Roll src/third_party/skia 44bad2e86a01..9eb1c7d80fb3 (1 commits) (flutter/engine#5681) * flutter/engine@6f8788625 Roll src/third_party/skia e72c144af687..44bad2e86a01 (1 commits) (flutter/engine#5680) * flutter/engine@23ae68dfd Roll src/third_party/skia a93486b095ea..e72c144af687 (1 commits) (flutter/engine#5679) * flutter/engine@c6ab9b941 Roll src/third_party/skia 5788bb9af9ad..a93486b095ea (1 commits) (flutter/engine#5678) * flutter/engine@aa4afc14b Roll src/third_party/skia 9b80bd5f16e6..5788bb9af9ad (1 commits) (flutter/engine#5677) * flutter/engine@9f9c06234 Roll src/third_party/skia cdefa23a23cf..9b80bd5f16e6 (1 commits) (flutter/engine#5676) * flutter/engine@4128a799c Roll src/third_party/skia 233c65202e69..cdefa23a23cf (1 commits) (flutter/engine#5675) * flutter/engine@74dc963ab Roll src/third_party/skia 6784ffa78e70..233c65202e69 (1 commits) (flutter/engine#5674) * flutter/engine@5724faf86 ensure that bridge is not destroyed when semantics is still enabled (flutter/engine#5672) * flutter/engine@24af9ca6c [fuchsia] Update gn label for fuchsia.ui.scenic (flutter/engine#5673) * flutter/engine@d085f1df2 Do not make an extra submit callback during SurfaceFrame destruction if the frame was already submitted (flutter/engine#5669) * flutter/engine@b66301055 Roll src/third_party/skia 58a1605d2b9b..6784ffa78e70 (8 commits) (flutter/engine#5671) * flutter/engine@2b890c804 Roll src/third_party/skia 34aa059c1502..58a1605d2b9b (6 commits) (flutter/engine#5667) * flutter/engine@2a88ecf17 Roll src/third_party/skia 94d57c477fe1..34aa059c1502 (8 commits) (flutter/engine#5666) * flutter/engine@51785d244 Remove unused Java imports (flutter/engine#5663) * flutter/engine@b157d4f56 Roll dart sdk to 64641d014b77bd410e9aa10558522ae26e4210ee (flutter/engine#5610) * flutter/engine@d05225ed9 Roll src/third_party/skia d7425b58df89..94d57c477fe1 (1 commits) (flutter/engine#5665) * flutter/engine@32736d52c Roll src/third_party/skia a22d924763ab..d7425b58df89 (4 commits) (flutter/engine#5664) * flutter/engine@0fca44477 Roll src/third_party/skia ef21d7e47963..a22d924763ab (3 commits) (flutter/engine#5662) * flutter/engine@a7d0f29c7 Roll src/third_party/skia 9aa30c6ee0e5..ef21d7e47963 (5 commits) (flutter/engine#5661) * flutter/engine@21c7d6a5d Revert "Add antiAlias and saveCount to clipPath and restore (flutter/engine#5638)" (flutter#5660) * flutter/engine@70dcbb591 Roll src/third_party/skia d818ebf4a317..9aa30c6ee0e5 (11 commits) (flutter/engine#5658) * flutter/engine@ad42324a6 Roll src/third_party/skia a219419c9d76..d818ebf4a317 (2 commits) (flutter/engine#5657) * flutter/engine@f2eb83ae2 Roll src/third_party/skia 00d2e8ebcb13..a219419c9d76 (1 commits) (flutter/engine#5655) * flutter/engine@0ec766946 Roll src/third_party/skia 8451daabb23d..00d2e8ebcb13 (1 commits) (flutter/engine#5654) * flutter/engine@061e899b5 Support all keyboard actions. (flutter/engine#11344) (flutter#5620) * flutter/engine@bc6b2501c Roll src/third_party/skia 34024a7c478c..8451daabb23d (1 commits) (flutter/engine#5653) * flutter/engine@df4dffb10 Roll src/third_party/skia 75e69028956d..34024a7c478c (1 commits) (flutter/engine#5652) * flutter/engine@d8770d440 Roll src/third_party/skia cf863fb9b446..75e69028956d (1 commits) (flutter/engine#5651) * flutter/engine@b88a8b3d5 Roll src/third_party/skia b7b9d02ac020..cf863fb9b446 (1 commits) (flutter/engine#5650) * flutter/engine@2261ccf87 [fuchsia] Update scenic lib path. (flutter/engine#5649) * flutter/engine@4c4ef987a [fuchsia] Rename scenic_lib => scenic (flutter/engine#5648) * flutter/engine@f0c21f327 Roll src/third_party/skia 184d408b646b..b7b9d02ac020 (7 commits) (flutter/engine#5646) * flutter/engine@a2bf80590 Add antiAlias and saveCount to clipPath and restore (flutter/engine#5638) * flutter/engine@9e450d116 Roll src/third_party/skia c91fe3ab1c5d..184d408b646b (10 commits) (flutter/engine#5645) * flutter/engine@e6639963f Roll src/third_party/skia eb8f8106f38c..c91fe3ab1c5d (5 commits) (flutter/engine#5644) * flutter/engine@fecd66f34 Roll src/third_party/skia 723b1f6ef941..eb8f8106f38c (1 commits) (flutter/engine#5643) * flutter/engine@4466d61a9 Remove vmservice_io.main from entry points. (flutter/engine#5625) * flutter/engine@f279dfe30 Roll src/third_party/skia 7e2327b133db..723b1f6ef941 (1 commits) (flutter/engine#5642) * flutter/engine@a885bd4f9 Roll src/third_party/skia 24d18ced1ad7..7e2327b133db (6 commits) (flutter/engine#5641) * flutter/engine@ad1bd4743 Roll src/third_party/skia a1e5630183c1..24d18ced1ad7 (7 commits) (flutter/engine#5639)

…nsform. (#5693)

do not produce a cullRect from a TransformLayer with perspective. Als…

63f194e

…o fix ios type error on xcode beta

googlebot added the cla: yes label Jul 8, 2018

jonahwilliams requested a review from chinmaygarde July 9, 2018 15:29

chinmaygarde reviewed Jul 9, 2018

View reviewed changes

Update default_layer_builder.cc

4108b4a

chinmaygarde approved these changes Jul 9, 2018

View reviewed changes

jonahwilliams merged commit d217a95 into flutter:master Jul 9, 2018

jonahwilliams deleted the cullRect branch July 9, 2018 19:52

Hixie reviewed Jul 9, 2018

View reviewed changes

cbracken mentioned this pull request Jul 11, 2018

Roll engine to fed2ea458ed49088d33eddabc546ba56d600c717 flutter/flutter#19238

Closed

bkonyi pushed a commit that referenced this pull request Jul 13, 2018

Remove cullRect calculation on TransformLayers with a perspective tra…

7b7362e

…nsform. (#5693)

jonahwilliams mentioned this pull request Apr 9, 2019

Rotating a ListView around X causes it to dissapear flutter/flutter#30819

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Remove cullRect calculation on TransformLayers with a perspective transform. #5693

Remove cullRect calculation on TransformLayers with a perspective transform. #5693

Uh oh!

jonahwilliams commented Jul 8, 2018 •

edited

Loading

Uh oh!

chinmaygarde Jul 9, 2018

Uh oh!

jonahwilliams Jul 9, 2018

Uh oh!

Hixie Jul 9, 2018

Uh oh!

jonahwilliams Jul 9, 2018

Uh oh!

Hixie Jul 10, 2018

Uh oh!

Hixie Jul 10, 2018

Uh oh!

jonahwilliams Jul 10, 2018

Uh oh!

chinmaygarde Jul 10, 2018

Uh oh!

jonahwilliams Jul 10, 2018

Uh oh!

chinmaygarde Jul 10, 2018

Uh oh!

jonahwilliams Jul 10, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

	void DefaultLayerBuilder::PushTransform(const SkMatrix& sk_matrix) {
	SkMatrix inverse_sk_matrix;
	SkRect cullRect;
	if (sk_matrix.invert(&inverse_sk_matrix)) {
	inverse_sk_matrix.mapRect(&cullRect, cull_rects_.top());
	} else {
	cullRect = kGiantRect;
	}

	auto layer = std::make_unique<flow::TransformLayer>();
	layer->set_transform(sk_matrix);
	PushLayer(std::move(layer), cullRect);
	}

Remove cullRect calculation on TransformLayers with a perspective transform. #5693

Remove cullRect calculation on TransformLayers with a perspective transform. #5693

Uh oh!

Conversation

jonahwilliams commented Jul 8, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

The problem

The Solution (?)

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!

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

jonahwilliams commented Jul 8, 2018 •

edited

Loading