Physical model layer that can draw shadows for a Material widget #3424

jason-simmons · 2017-02-15T18:00:49Z

No description provided.

jason-simmons · 2017-02-15T18:26:57Z

abarth · 2017-02-15T18:49:53Z

flow/layers/physical_model_layer.cc

+
+  // Add some margin to the paint bounds to leave space for the shadow.
+  SkRect bounds(context->child_paint_bounds);
+  bounds.outset(50.0, 50.0);


Where do these numbers come from? We might need a way to ask SkShadowUtils how much space to reserve for a shadow with certain parameters.

This is a hardcoded estimate of the upper bounds for where DrawShadow will paint. AFAIK Skia doesn't have a way to calculate this precisely. Added a comment.

abarth · 2017-02-15T18:51:03Z

flow/layers/physical_model_layer.cc

+    context->child_paint_bounds.setEmpty();
+
+  // Add some margin to the paint bounds to leave space for the shadow.
+  SkRect bounds(context->child_paint_bounds);


It doesn't seem like the child paint bounds are relevant to computing the paint bounds of this layer. This layer is always going to be rrect_ + whatever margin we need for the shadow.

abarth · 2017-02-15T18:51:56Z

flow/layers/physical_model_layer.cc

+
+void PhysicalModelLayer::Paint(PaintContext& context) {
+  TRACE_EVENT0("flutter", "PhysicalModelLayer::Paint");
+  FTL_DCHECK(!needs_system_composite());


This DCHECK is going to blow up in Fuchsia. We should be able to do a system-composited version of this layer.

It looks like Fuchsia support will require a broader refactoring. Added a TODO.

abarth · 2017-02-15T18:53:58Z

flow/layers/physical_model_layer.cc

+  context.canvas.drawPath(path, paint);
+
+  SkAutoCanvasRestore save(&context.canvas, false);
+  context.canvas.saveLayer(&paint_bounds(), nullptr);


We should check whether the rrect is actually a rect (i.e., whether the radius is zero) and use save() rather than saveLayer() in that case.

Also, using the full paint_bounds is allocating a larger texture than necessary because the child cannot draw outside the clip. Instead, we should use the bounds of the rrect to for the size of the layer we save (skipping the margin we added to the paint bounds for the child).

abarth · 2017-02-15T18:54:55Z

lib/ui/compositing.dart

+  /// Rasterization will be clipped to the given shape.
+  ///
+  /// See [pop] for details about the operation stack.
+  void pushPhysicalModel(RRect rrect, int elevation, Color color) {


I'd make these named arguments because we're likely to extend them over time.

abarth · 2017-02-15T20:18:02Z

lib/ui/compositing.dart

+  /// Rasterization will be clipped to the given shape.
+  ///
+  /// See [pop] for details about the operation stack.
+  void pushPhysicalModel({RRect rrect, int elevation, Color color}) {


We usually put spaces after { and before } here.

abarth · 2017-02-15T20:18:13Z

chinmaygarde · 2017-02-15T23:22:47Z

flow/layers/physical_model_layer.cc

+  }
+
+  SkPaint paint;
+  paint.setColor(SkColorSetA(color_, 0xFF));


Is updating the alpha necessary? Might be unexpected behavior for callers expecting a lighter shadow.

Skia will draw some of the shadow within the bounds of the layer, so we're filling the layer with an opaque color to cover that part of the shadow

chinmaygarde · 2017-02-15T23:23:55Z

lib/ui/compositing/scene_builder.cc

+  if (!cullRect.intersect(rrect.sk_rrect.rect(), m_cullRects.top()))
+    cullRect = SkRect::MakeEmpty();
+
+  std::unique_ptr<flow::PhysicalModelLayer> layer(new flow::PhysicalModelLayer());


Nit: auto layer = std::make_unique<...> might be easier to write.

chinmaygarde · 2017-02-17T00:43:15Z

Can we land this?

…tter#3424)

googlebot added the cla: yes label Feb 15, 2017

jason-simmons force-pushed the physical_model branch from 1598af2 to 0d58d73 Compare February 15, 2017 18:31

abarth reviewed Feb 15, 2017

View reviewed changes

jason-simmons force-pushed the physical_model branch from 0d58d73 to 3dd2c5a Compare February 15, 2017 20:15

abarth reviewed Feb 15, 2017

View reviewed changes

jason-simmons force-pushed the physical_model branch from 3dd2c5a to b48ac29 Compare February 15, 2017 21:00

chinmaygarde approved these changes Feb 15, 2017

View reviewed changes

jason-simmons force-pushed the physical_model branch 2 times, most recently from bcf90c2 to 472f5ba Compare February 17, 2017 00:58

Physical model layer that can draw shadows for a Material widget

ab7b1e5

jason-simmons force-pushed the physical_model branch from 472f5ba to ab7b1e5 Compare February 17, 2017 01:02

jason-simmons merged commit a44647a into flutter:master Feb 17, 2017

Hixie mentioned this pull request Feb 27, 2017

Back button memory usage benchmark 40x regression flutter/flutter#8429

Closed

goderbauer pushed a commit to goderbauer/engine that referenced this pull request Mar 9, 2017

Physical model layer that can draw shadows for a Material widget (flu…

c8e105f

…tter#3424)

Physical model layer that can draw shadows for a Material widget #3424

Physical model layer that can draw shadows for a Material widget #3424

Uh oh!

Conversation

jason-simmons commented Feb 15, 2017

Uh oh!

jason-simmons commented Feb 15, 2017

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!

abarth commented Feb 15, 2017

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!

chinmaygarde commented Feb 17, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants