From f2bf47d25699965a0572cd99fad17d0d795d8b8b Mon Sep 17 00:00:00 2001 From: Brandon DeRosier Date: Tue, 5 Apr 2022 13:12:17 -0700 Subject: [PATCH] Fix SaveLayer/entity subpass behavioral problems (#111) --- impeller/aiks/aiks_playground.cc | 11 +++++++-- impeller/aiks/aiks_playground.h | 5 ++++ impeller/aiks/aiks_unittests.cc | 33 ++++++++++++++++++++++++- impeller/aiks/canvas.cc | 5 ++-- impeller/entity/contents/contents.cc | 7 +----- impeller/entity/entity.cc | 10 +++----- impeller/entity/entity_pass.cc | 33 ++++++++++++++++++++----- impeller/entity/entity_pass.h | 4 ++- impeller/entity/entity_unittests.cc | 8 ++++++ impeller/geometry/geometry_unittests.cc | 13 +++++++--- impeller/geometry/rect.h | 16 +++++++++++- 11 files changed, 116 insertions(+), 29 deletions(-) diff --git a/impeller/aiks/aiks_playground.cc b/impeller/aiks/aiks_playground.cc index 572b761f83ab5..ff6d3585b9a8b 100644 --- a/impeller/aiks/aiks_playground.cc +++ b/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); }); } diff --git a/impeller/aiks/aiks_playground.h b/impeller/aiks/aiks_playground.h index a77622b012ad9..4a7288fd18471 100644 --- a/impeller/aiks/aiks_playground.h +++ b/impeller/aiks/aiks_playground.h @@ -5,6 +5,7 @@ #pragma once #include "flutter/fml/macros.h" +#include "impeller/aiks/aiks_context.h" #include "impeller/aiks/picture.h" #include "impeller/playground/playground.h" @@ -12,12 +13,16 @@ namespace impeller { class AiksPlayground : public Playground { public: + using AiksPlaygroundCallback = std::function; + AiksPlayground(); ~AiksPlayground(); bool OpenPlaygroundHere(const Picture& picture); + bool OpenPlaygroundHere(AiksPlaygroundCallback callback); + private: FML_DISALLOW_COPY_AND_ASSIGN(AiksPlayground); }; diff --git a/impeller/aiks/aiks_unittests.cc b/impeller/aiks/aiks_unittests.cc index 73b5039e032c7..2b74ae21d6650 100644 --- a/impeller/aiks/aiks_unittests.cc +++ b/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 diff --git a/impeller/aiks/canvas.cc b/impeller/aiks/canvas.cc index a4afb84e09b94..8a7e001a214a9 100644 --- a/impeller/aiks/canvas.cc +++ b/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()); 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)); } diff --git a/impeller/entity/contents/contents.cc b/impeller/entity/contents/contents.cc index 83fa02514ca84..33c489efb1be4 100644 --- a/impeller/entity/contents/contents.cc +++ b/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 Contents::RenderToTexture( diff --git a/impeller/entity/entity.cc b/impeller/entity/entity.cc index 417d61636d902..95d0e66528458 100644 --- a/impeller/entity/entity.cc +++ b/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) { diff --git a/impeller/entity/entity_pass.cc b/impeller/entity/entity_pass.cc index 914da3ebedcd7..a0e4878ece11b 100644 --- a/impeller/entity/entity_pass.cc +++ b/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 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,14 +108,23 @@ EntityPass* EntityPass::AddSubpass(std::unique_ptr 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; @@ -118,7 +132,7 @@ bool EntityPass::Render(ContentContext& renderer, 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; } diff --git a/impeller/entity/entity_pass.h b/impeller/entity/entity_pass.h index f4dc5cf424d61..07a48e3530ef0 100644 --- a/impeller/entity/entity_pass.h +++ b/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 iterator); diff --git a/impeller/entity/entity_unittests.cc b/impeller/entity/entity_unittests.cc index c4804d0567700..31a68a1aea1f6 100644 --- a/impeller/entity/entity_unittests.cc +++ b/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()); + entity.SetPath({}); + ASSERT_TRUE(entity.GetCoverage()->IsZero()); + ASSERT_TRUE(entity.GetTransformedPathBounds().IsZero()); +} + } // namespace testing } // namespace impeller diff --git a/impeller/geometry/geometry_unittests.cc b/impeller/geometry/geometry_unittests.cc index fbdd2c9c7d6b1..b43d1ffdf53c0 100644 --- a/impeller/geometry/geometry_unittests.cc +++ b/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 r = Rect::MakePointBounds({}); + ASSERT_FALSE(r.has_value()); + } } TEST(GeometryTest, CubicPathComponentPolylineDoesNotIncludePointOne) { diff --git a/impeller/geometry/rect.h b/impeller/geometry/rect.h index dd12a5c69a697..514cb9c90ffff 100644 --- a/impeller/geometry/rect.h +++ b/impeller/geometry/rect.h @@ -9,6 +9,7 @@ #include #include +#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 MakePointBounds( const std::vector>& 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();