Fix SaveLayer/entity subpass behavioral problems (#111)

Author: bdero

impeller/aiks/aiks_playground.cc

@@ -13,6 +13,13 @@ AiksPlayground::AiksPlayground() = default;
 AiksPlayground::~AiksPlayground() = default;
 
 bool AiksPlayground::OpenPlaygroundHere(const Picture& picture) {
+  return OpenPlaygroundHere(
+      [&picture](AiksContext& renderer, RenderPass& pass) -> bool {
+        return renderer.Render(picture, pass);
+      });
+}
+
+bool AiksPlayground::OpenPlaygroundHere(AiksPlaygroundCallback callback) {
   if (!Playground::is_enabled()) {
     return true;
   }
@@ -24,8 +31,8 @@ bool AiksPlayground::OpenPlaygroundHere(const Picture& picture) {
   }
 
   return Playground::OpenPlaygroundHere(
-      [&renderer, &picture](RenderPass& pass) -> bool {
-        return renderer.Render(picture, pass);
+      [&renderer, &callback](RenderPass& pass) -> bool {
+        return callback(renderer, pass);
       });
 }
 

impeller/aiks/aiks_playground.h

@@ -5,19 +5,24 @@
 #pragma once
 
 #include "flutter/fml/macros.h"
+#include "impeller/aiks/aiks_context.h"
 #include "impeller/aiks/picture.h"
 #include "impeller/playground/playground.h"
 
 namespace impeller {
 
 class AiksPlayground : public Playground {
  public:
+  using AiksPlaygroundCallback = std::function<bool(AiksContext& renderer, RenderPass& pass)>;
+
   AiksPlayground();
 
   ~AiksPlayground();
 
   bool OpenPlaygroundHere(const Picture& picture);
 
+  bool OpenPlaygroundHere(AiksPlaygroundCallback callback);
+
  private:
   FML_DISALLOW_COPY_AND_ASSIGN(AiksPlayground);
 };

impeller/aiks/aiks_unittests.cc

@@ -9,6 +9,7 @@
 #include "impeller/aiks/image.h"
 #include "impeller/geometry/geometry_unittests.h"
 #include "impeller/geometry/path_builder.h"
+#include "impeller/playground/widgets.h"
 #include "impeller/typographer/backends/skia/text_frame_skia.h"
 #include "impeller/typographer/backends/skia/text_render_context_skia.h"
 #include "third_party/skia/include/core/SkData.h"
@@ -516,8 +517,38 @@ TEST_F(AiksTest, PathsShouldHaveUniformAlpha) {
     }
     canvas.Translate({-240, 60});
   }
+}
 
-  ASSERT_TRUE(OpenPlaygroundHere(canvas.EndRecordingAsPicture()));
+TEST_F(AiksTest, CoverageOriginShouldBeAccountedForInSubpasses) {
+  auto callback = [](AiksContext& renderer, RenderPass& pass) {
+    Canvas canvas;
+    Paint alpha;
+    alpha.color = Color::Red().WithAlpha(0.5);
+
+    auto current = Point{25, 25};
+    const auto offset = Point{25, 25};
+    const auto size = Size(100, 100);
+
+    auto [b0, b1] = IMPELLER_PLAYGROUND_LINE(Point(40, 40), Point(160, 160), 10,
+                                             Color::White(), Color::White());
+    auto bounds = Rect::MakeLTRB(b0.x, b0.y, b1.x, b1.y);
+
+    canvas.DrawRect(bounds, Paint{.color = Color::Yellow(),
+                                  .stroke_width = 5.0f,
+                                  .style = Paint::Style::kStroke});
+
+    canvas.SaveLayer(alpha, bounds);
+
+    canvas.DrawRect({current, size}, Paint{.color = Color::Red()});
+    canvas.DrawRect({current += offset, size}, Paint{.color = Color::Green()});
+    canvas.DrawRect({current += offset, size}, Paint{.color = Color::Blue()});
+
+    canvas.Restore();
+
+    return renderer.Render(canvas.EndRecordingAsPicture(), pass);
+  };
+
+  ASSERT_TRUE(OpenPlaygroundHere(callback));
 }
 
 }  // namespace testing

impeller/aiks/canvas.cc

@@ -264,14 +264,13 @@ size_t Canvas::GetStencilDepth() const {
 
 void Canvas::Save(bool create_subpass) {
   auto entry = CanvasStackEntry{};
+  entry.xformation = xformation_stack_.back().xformation;
+  entry.stencil_depth = xformation_stack_.back().stencil_depth;
   if (create_subpass) {
     entry.is_subpass = true;
     current_pass_ = GetCurrentPass().AddSubpass(std::make_unique<EntityPass>());
     current_pass_->SetTransformation(xformation_stack_.back().xformation);
     current_pass_->SetStencilDepth(xformation_stack_.back().stencil_depth);
-  } else {
-    entry.xformation = xformation_stack_.back().xformation;
-    entry.stencil_depth = xformation_stack_.back().stencil_depth;
   }
   xformation_stack_.emplace_back(std::move(entry));
 }

impeller/entity/contents/contents.cc

@@ -30,12 +30,7 @@ Contents::Contents() = default;
 Contents::~Contents() = default;
 
 Rect Contents::GetBounds(const Entity& entity) const {
-  const auto& transform = entity.GetTransformation();
-  auto points = entity.GetPath().GetBoundingBox()->GetPoints();
-  for (uint i = 0; i < points.size(); i++) {
-    points[i] = transform * points[i];
-  }
-  return Rect::MakePointBounds({points.begin(), points.end()});
+  return entity.GetTransformedPathBounds();
 }
 
 std::optional<Snapshot> Contents::RenderToTexture(

impeller/entity/entity.cc

@@ -31,13 +31,11 @@ void Entity::SetPath(Path path) {
 }
 
 Rect Entity::GetTransformedPathBounds() const {
-  auto points = GetPath().GetBoundingBox()->GetPoints();
-
-  const auto& transform = GetTransformation();
-  for (uint i = 0; i < points.size(); i++) {
-    points[i] = transform * points[i];
+  auto bounds = GetPath().GetBoundingBox();
+  if (!bounds.has_value()) {
+    return Rect();
   }
-  return Rect::MakePointBounds({points.begin(), points.end()});
+  return bounds->TransformBounds(GetTransformation());
 }
 
 void Entity::SetAddsToCoverage(bool adds) {

impeller/entity/entity_pass.cc

@@ -5,10 +5,12 @@
 #include "impeller/entity/entity_pass.h"
 
 #include "flutter/fml/trace_event.h"
+#include "impeller/base/validation.h"
 #include "impeller/entity/contents/content_context.h"
 #include "impeller/geometry/path_builder.h"
 #include "impeller/renderer/command_buffer.h"
 #include "impeller/renderer/render_pass.h"
+#include "impeller/renderer/texture.h"
 
 namespace impeller {
 
@@ -78,6 +80,9 @@ std::optional<Rect> EntityPass::GetSubpassCoverage(
   if (!delegate_coverage.has_value()) {
     return entities_coverage;
   }
+  // The delegate coverage hint is in given in local space, so apply the subpass
+  // transformation.
+  delegate_coverage = delegate_coverage->TransformBounds(subpass.xformation_);
 
   // If the delegate tells us the coverage is smaller than it needs to be, then
   // great. OTOH, if the delegate is being wasteful, limit coverage to what is
@@ -103,22 +108,31 @@ EntityPass* EntityPass::AddSubpass(std::unique_ptr<EntityPass> pass) {
 }
 
 bool EntityPass::Render(ContentContext& renderer,
-                        RenderPass& parent_pass) const {
+                        RenderPass& parent_pass,
+                        Point position) const {
   TRACE_EVENT0("impeller", "EntityPass::Render");
 
-  for (const auto& entity : entities_) {
+  for (Entity entity : entities_) {
+    if (!position.IsZero()) {
+      // If the pass image is going to be rendered with a non-zero position,
+      // apply the negative translation to entity copies before rendering them
+      // so that they'll end up rendering to the correct on-screen position.
+      entity.SetTransformation(Matrix::MakeTranslation(Vector3(-position)) *
+                               entity.GetTransformation());
+    }
     if (!entity.Render(renderer, parent_pass)) {
       return false;
     }
   }
+
   for (const auto& subpass : subpasses_) {
     if (delegate_->CanElide()) {
       continue;
     }
 
     if (delegate_->CanCollapseIntoParentPass()) {
       // Directly render into the parent pass and move on.
-      if (!subpass->Render(renderer, parent_pass)) {
+      if (!subpass->Render(renderer, parent_pass, position)) {
         return false;
       }
       continue;
@@ -178,7 +192,7 @@ bool EntityPass::Render(ContentContext& renderer,
 
     sub_renderpass->SetLabel("OffscreenPass");
 
-    if (!subpass->Render(renderer, *sub_renderpass)) {
+    if (!subpass->Render(renderer, *sub_renderpass, subpass_coverage->origin)) {
       return false;
     }
 
@@ -191,10 +205,17 @@ bool EntityPass::Render(ContentContext& renderer,
     }
 
     Entity entity;
-    entity.SetPath(PathBuilder{}.AddRect(subpass_coverage.value()).TakePath());
+    entity.SetPath(PathBuilder{}
+                       .AddRect(Rect::MakeSize(subpass_coverage->size))
+                       .TakePath());
     entity.SetContents(std::move(offscreen_texture_contents));
     entity.SetStencilDepth(stencil_depth_);
-    entity.SetTransformation(xformation_);
+    // Once we have filters being applied for SaveLayer, some special sauce
+    // may be needed here (or in PaintPassDelegate) to ensure the filter
+    // parameters are transformed by the `xformation_` matrix, while continuing
+    // to apply only the subpass offset to the offscreen texture.
+    entity.SetTransformation(
+        Matrix::MakeTranslation(Vector3(subpass_coverage->origin - position)));
     if (!entity.Render(renderer, parent_pass)) {
       return false;
     }

impeller/entity/entity_pass.h

@@ -48,7 +48,9 @@ class EntityPass {
 
   EntityPass* GetSuperpass() const;
 
-  bool Render(ContentContext& renderer, RenderPass& parent_pass) const;
+  bool Render(ContentContext& renderer,
+              RenderPass& parent_pass,
+              Point position = Point()) const;
 
   void IterateAllEntities(std::function<bool(Entity&)> iterator);
 

impeller/entity/entity_unittests.cc

@@ -779,5 +779,13 @@ TEST_F(EntityTest, SetBlendMode) {
   ASSERT_EQ(entity.GetBlendMode(), Entity::BlendMode::kClear);
 }
 
+TEST_F(EntityTest, ContentsGetBoundsForEmptyPathReturnsZero) {
+  Entity entity;
+  entity.SetContents(std::make_shared<SolidColorContents>());
+  entity.SetPath({});
+  ASSERT_TRUE(entity.GetCoverage()->IsZero());
+  ASSERT_TRUE(entity.GetTransformedPathBounds().IsZero());
+}
+
 }  // namespace testing
 }  // namespace impeller

impeller/geometry/geometry_unittests.cc

@@ -839,9 +839,16 @@ TEST(GeometryTest, RectGetPoints) {
 }
 
 TEST(GeometryTest, RectMakePointBounds) {
-  auto r = Rect::MakePointBounds({Point(1, 5), Point(4, -1), Point(0, 6)});
-  auto expected = Rect(0, -1, 4, 7);
-  ASSERT_RECT_NEAR(r, expected);
+  {
+    Rect r =
+        Rect::MakePointBounds({Point(1, 5), Point(4, -1), Point(0, 6)}).value();
+    auto expected = Rect(0, -1, 4, 7);
+    ASSERT_RECT_NEAR(r, expected);
+  }
+  {
+    std::optional<Rect> r = Rect::MakePointBounds({});
+    ASSERT_FALSE(r.has_value());
+  }
 }
 
 TEST(GeometryTest, CubicPathComponentPolylineDoesNotIncludePointOne) {

impeller/geometry/rect.h

@@ -9,6 +9,7 @@
 #include <ostream>
 #include <vector>
 
+#include "impeller/geometry/matrix.h"
 #include "impeller/geometry/point.h"
 #include "impeller/geometry/scalar.h"
 #include "impeller/geometry/size.h"
@@ -51,8 +52,11 @@ struct TRect {
     return TRect(0.0, 0.0, size.width, size.height);
   }
 
-  constexpr static TRect MakePointBounds(
+  constexpr static std::optional<TRect> MakePointBounds(
       const std::vector<TPoint<Type>>& points) {
+    if (points.empty()) {
+      return std::nullopt;
+    }
     auto left = points[0].x;
     auto top = points[0].y;
     auto right = points[0].x;
@@ -143,6 +147,16 @@ struct TRect {
             TPoint(right, bottom)};
   }
 
+  /// @brief  Creates a new bounding box that contains this transformed
+  ///         rectangle.
+  constexpr TRect TransformBounds(const Matrix& transform) const {
+    auto points = GetPoints();
+    for (uint i = 0; i < points.size(); i++) {
+      points[i] = transform * points[i];
+    }
+    return TRect::MakePointBounds({points.begin(), points.end()}).value();
+  }
+
   constexpr TRect Union(const TRect& o) const {
     auto this_ltrb = GetLTRB();
     auto other_ltrb = o.GetLTRB();