From df63862e6f9d18644bd140e3c963326959d9feef Mon Sep 17 00:00:00 2001 From: jonahwilliams Date: Wed, 21 Aug 2024 19:57:48 -0700 Subject: [PATCH 1/4] [Impeller] fix clip culling with exp canvas. --- impeller/aiks/experimental_canvas.cc | 19 ++++++++++++++++ impeller/entity/contents/clip_contents.cc | 27 ++++++++++++++--------- impeller/entity/contents/clip_contents.h | 2 ++ 3 files changed, 37 insertions(+), 11 deletions(-) diff --git a/impeller/aiks/experimental_canvas.cc b/impeller/aiks/experimental_canvas.cc index b9cd19a12afcd..2bf9b4c9ead85 100644 --- a/impeller/aiks/experimental_canvas.cc +++ b/impeller/aiks/experimental_canvas.cc @@ -10,6 +10,7 @@ #include "impeller/base/validation.h" #include "impeller/core/allocator.h" #include "impeller/core/formats.h" +#include "impeller/entity/contents/clip_contents.h" #include "impeller/entity/contents/framebuffer_blend_contents.h" #include "impeller/entity/contents/text_contents.h" #include "impeller/entity/entity.h" @@ -431,6 +432,14 @@ void ExperimentalCanvas::SaveLayer( entry.rendering_mode = Entity::RenderingMode::kSubpassAppendSnapshotTransform; transform_stack_.emplace_back(entry); + // The DisplayList bounds/rtree doesn't account for filters applied to parent + // layers, and so sub-DisplayLists are getting culled as if no filters are + // applied. + // See also: https://github.com/flutter/flutter/issues/139294 + if (paint.image_filter) { + transform_stack_.back().cull_rect = std::nullopt; + } + // Start non-collapsed subpasses with a fresh clip coverage stack limited by // the subpass coverage. This is important because image filters applied to // save layers may transform the subpass texture after it's rendered, @@ -774,6 +783,16 @@ void ExperimentalCanvas::AddClipEntityToCurrentPass(Entity entity) { current_clip_coverage->Shift(-GetGlobalPassPosition()); } + // Skip rendering the clip if it is fully contained by the current render + // target. + const std::shared_ptr& clip_contents = + std::static_pointer_cast(entity.GetContents()); + if (clip_contents->CanSkip( + render_passes_.back().inline_pass_context->GetTexture()->GetSize(), + entity.GetTransform())) { + return; + } + auto clip_coverage = entity.GetClipCoverage(current_clip_coverage); if (clip_coverage.coverage.has_value()) { clip_coverage.coverage = diff --git a/impeller/entity/contents/clip_contents.cc b/impeller/entity/contents/clip_contents.cc index f13f452734732..1ec8e9448016c 100644 --- a/impeller/entity/contents/clip_contents.cc +++ b/impeller/entity/contents/clip_contents.cc @@ -82,6 +82,20 @@ bool ClipContents::CanInheritOpacity(const Entity& entity) const { void ClipContents::SetInheritedOpacity(Scalar opacity) {} +bool ClipContents::CanSkip(ISize render_pass_size, const Matrix& ctm) const { + if (clip_op_ == Entity::ClipOperation::kIntersect && + geometry_->IsAxisAlignedRect() && ctm.IsTranslationScaleOnly()) { + std::optional coverage = geometry_->GetCoverage(ctm); + if (coverage.has_value() && + coverage->Contains(Rect::MakeSize(render_pass_size))) { + // Skip axis-aligned intersect clips that cover the whole render target + // since they won't draw anything to the depth buffer. + return true; + } + } + return false; +} + bool ClipContents::Render(const ContentContext& renderer, const Entity& entity, RenderPass& pass) const { @@ -91,17 +105,8 @@ bool ClipContents::Render(const ContentContext& renderer, using VS = ClipPipeline::VertexShader; - if (clip_op_ == Entity::ClipOperation::kIntersect && - geometry_->IsAxisAlignedRect() && - entity.GetTransform().IsTranslationScaleOnly()) { - std::optional coverage = - geometry_->GetCoverage(entity.GetTransform()); - if (coverage.has_value() && - coverage->Contains(Rect::MakeSize(pass.GetRenderTargetSize()))) { - // Skip axis-aligned intersect clips that cover the whole render target - // since they won't draw anything to the depth buffer. - return true; - } + if (CanSkip(pass.GetRenderTargetSize(), entity.GetTransform())) { + return true; } VS::FrameInfo info; diff --git a/impeller/entity/contents/clip_contents.h b/impeller/entity/contents/clip_contents.h index 5481298a2a8b2..34758eef408aa 100644 --- a/impeller/entity/contents/clip_contents.h +++ b/impeller/entity/contents/clip_contents.h @@ -25,6 +25,8 @@ class ClipContents final : public Contents { void SetClipOperation(Entity::ClipOperation clip_op); + bool CanSkip(ISize render_pass_size, const Matrix& ctm) const; + // |Contents| std::optional GetCoverage(const Entity& entity) const override; From 1783fc806325de01a16b052ebaa57450100b7bb9 Mon Sep 17 00:00:00 2001 From: jonahwilliams Date: Thu, 22 Aug 2024 18:06:27 -0700 Subject: [PATCH 2/4] ++ --- impeller/aiks/experimental_canvas.cc | 4 +--- impeller/entity/contents/clip_contents.h | 2 +- impeller/entity/contents/contents.cc | 4 ++++ impeller/entity/contents/contents.h | 7 ++++++ impeller/entity/entity_unittests.cc | 29 ++++++++++++++++++++++++ 5 files changed, 42 insertions(+), 4 deletions(-) diff --git a/impeller/aiks/experimental_canvas.cc b/impeller/aiks/experimental_canvas.cc index 2bf9b4c9ead85..748ea7a32ed16 100644 --- a/impeller/aiks/experimental_canvas.cc +++ b/impeller/aiks/experimental_canvas.cc @@ -785,9 +785,7 @@ void ExperimentalCanvas::AddClipEntityToCurrentPass(Entity entity) { // Skip rendering the clip if it is fully contained by the current render // target. - const std::shared_ptr& clip_contents = - std::static_pointer_cast(entity.GetContents()); - if (clip_contents->CanSkip( + if (entity.GetContents()->CanSkip( render_passes_.back().inline_pass_context->GetTexture()->GetSize(), entity.GetTransform())) { return; diff --git a/impeller/entity/contents/clip_contents.h b/impeller/entity/contents/clip_contents.h index 34758eef408aa..5b69a41524319 100644 --- a/impeller/entity/contents/clip_contents.h +++ b/impeller/entity/contents/clip_contents.h @@ -25,7 +25,7 @@ class ClipContents final : public Contents { void SetClipOperation(Entity::ClipOperation clip_op); - bool CanSkip(ISize render_pass_size, const Matrix& ctm) const; + bool CanSkip(ISize render_pass_size, const Matrix& transform) const override; // |Contents| std::optional GetCoverage(const Entity& entity) const override; diff --git a/impeller/entity/contents/contents.cc b/impeller/entity/contents/contents.cc index 421713761175e..fb3e1061d5ec7 100644 --- a/impeller/entity/contents/contents.cc +++ b/impeller/entity/contents/contents.cc @@ -182,4 +182,8 @@ void Contents::SetColorSourceSize(Size size) { color_source_size_ = size; } +bool Contents::CanSkip(ISize render_pass_size, const Matrix& transform) const { + return false; +} + } // namespace impeller diff --git a/impeller/entity/contents/contents.h b/impeller/entity/contents/contents.h index c26d1d56a49c8..92e7053d01a4c 100644 --- a/impeller/entity/contents/contents.h +++ b/impeller/entity/contents/contents.h @@ -83,6 +83,13 @@ class Contents { /// void SetCoverageHint(std::optional coverage_hint); + //---------------------------------------------------------------------------- + /// @brief Whether this entity can be entirely skipped during rendering. + /// + /// TODO(jonahwilliams): remove this method which was only added for clipping + /// once experimental canvas lands. + virtual bool CanSkip(ISize render_pass_size, const Matrix& transform) const; + const std::optional& GetCoverageHint() const; //---------------------------------------------------------------------------- diff --git a/impeller/entity/entity_unittests.cc b/impeller/entity/entity_unittests.cc index 92408fd7da6e2..2d7510f99b392 100644 --- a/impeller/entity/entity_unittests.cc +++ b/impeller/entity/entity_unittests.cc @@ -1532,6 +1532,35 @@ TEST_P(EntityTest, ClipContentsShouldRenderIsCorrect) { } } +TEST_P(EntityTest, ClipContentsCanSkip) { + // Intersect Clips can be skipped if the render target coverage is fully + // inside of them. + + auto clip = std::make_shared(); + clip->SetGeometry(Geometry::MakeRect(Rect::MakeLTRB(0, 0, 100, 100))); + clip->SetClipOperation(Entity::ClipOperation::kIntersect); + + // Fully Contained + EXPECT_TRUE(clip->CanSkip(ISize::MakeWH(50, 50), {})); + + // Not Intersect + clip->SetClipOperation(Entity::ClipOperation::kDifference); + EXPECT_FALSE(clip->CanSkip(ISize::MakeWH(50, 50), {})); + + // Not Contained + clip->SetClipOperation(Entity::ClipOperation::kIntersect); + EXPECT_FALSE(clip->CanSkip(ISize::MakeWH(200, 200), {})); + + // Not Scale Translate + clip->SetClipOperation(Entity::ClipOperation::kIntersect); + EXPECT_FALSE( + clip->CanSkip(ISize::MakeWH(50, 50), Matrix::MakeRotationX(Radians(2)))); + + // Not Axis Aligned Rectangle. + clip->SetGeometry(Geometry::MakeOval(Rect::MakeLTRB(0, 0, 50, 50))); + EXPECT_FALSE(clip->CanSkip(ISize::MakeWH(50, 50), {})); +} + TEST_P(EntityTest, ClipContentsGetClipCoverageIsCorrect) { // Intersection: No stencil coverage, no geometry. { From 9464c0035777c1254e8d94b876a96bdc0563441a Mon Sep 17 00:00:00 2001 From: jonahwilliams Date: Wed, 28 Aug 2024 13:05:52 -0700 Subject: [PATCH 3/4] move clip skipping logic into clip stack. --- impeller/aiks/experimental_canvas.cc | 15 +---- impeller/entity/contents/clip_contents.cc | 18 ------ impeller/entity/contents/clip_contents.h | 2 - impeller/entity/contents/contents.cc | 4 -- impeller/entity/contents/contents.h | 7 --- impeller/entity/entity_pass_clip_stack.cc | 24 +++++-- impeller/entity/entity_pass_unittests.cc | 76 +++++++++++++++++++++++ impeller/entity/entity_unittests.cc | 29 --------- 8 files changed, 97 insertions(+), 78 deletions(-) diff --git a/impeller/aiks/experimental_canvas.cc b/impeller/aiks/experimental_canvas.cc index 748ea7a32ed16..49af55d4e6af2 100644 --- a/impeller/aiks/experimental_canvas.cc +++ b/impeller/aiks/experimental_canvas.cc @@ -432,10 +432,8 @@ void ExperimentalCanvas::SaveLayer( entry.rendering_mode = Entity::RenderingMode::kSubpassAppendSnapshotTransform; transform_stack_.emplace_back(entry); - // The DisplayList bounds/rtree doesn't account for filters applied to parent - // layers, and so sub-DisplayLists are getting culled as if no filters are - // applied. - // See also: https://github.com/flutter/flutter/issues/139294 + // The current clip aiks clip culling can not handle image filters. + // Remove this once we've migrated to exp canvas and removed it. if (paint.image_filter) { transform_stack_.back().cull_rect = std::nullopt; } @@ -761,6 +759,7 @@ void ExperimentalCanvas::AddClipEntityToCurrentPass(Entity entity) { auto transform = entity.GetTransform(); entity.SetTransform( Matrix::MakeTranslation(Vector3(-GetGlobalPassPosition())) * transform); + // Ideally the clip depth would be greater than the current rendering // depth because any rendering calls that follow this clip operation will // pre-increment the depth and then be rendering above our clip depth, @@ -783,14 +782,6 @@ void ExperimentalCanvas::AddClipEntityToCurrentPass(Entity entity) { current_clip_coverage->Shift(-GetGlobalPassPosition()); } - // Skip rendering the clip if it is fully contained by the current render - // target. - if (entity.GetContents()->CanSkip( - render_passes_.back().inline_pass_context->GetTexture()->GetSize(), - entity.GetTransform())) { - return; - } - auto clip_coverage = entity.GetClipCoverage(current_clip_coverage); if (clip_coverage.coverage.has_value()) { clip_coverage.coverage = diff --git a/impeller/entity/contents/clip_contents.cc b/impeller/entity/contents/clip_contents.cc index 1ec8e9448016c..62fa39d3639e5 100644 --- a/impeller/entity/contents/clip_contents.cc +++ b/impeller/entity/contents/clip_contents.cc @@ -82,20 +82,6 @@ bool ClipContents::CanInheritOpacity(const Entity& entity) const { void ClipContents::SetInheritedOpacity(Scalar opacity) {} -bool ClipContents::CanSkip(ISize render_pass_size, const Matrix& ctm) const { - if (clip_op_ == Entity::ClipOperation::kIntersect && - geometry_->IsAxisAlignedRect() && ctm.IsTranslationScaleOnly()) { - std::optional coverage = geometry_->GetCoverage(ctm); - if (coverage.has_value() && - coverage->Contains(Rect::MakeSize(render_pass_size))) { - // Skip axis-aligned intersect clips that cover the whole render target - // since they won't draw anything to the depth buffer. - return true; - } - } - return false; -} - bool ClipContents::Render(const ContentContext& renderer, const Entity& entity, RenderPass& pass) const { @@ -105,10 +91,6 @@ bool ClipContents::Render(const ContentContext& renderer, using VS = ClipPipeline::VertexShader; - if (CanSkip(pass.GetRenderTargetSize(), entity.GetTransform())) { - return true; - } - VS::FrameInfo info; info.depth = GetShaderClipDepth(entity); diff --git a/impeller/entity/contents/clip_contents.h b/impeller/entity/contents/clip_contents.h index 5b69a41524319..5481298a2a8b2 100644 --- a/impeller/entity/contents/clip_contents.h +++ b/impeller/entity/contents/clip_contents.h @@ -25,8 +25,6 @@ class ClipContents final : public Contents { void SetClipOperation(Entity::ClipOperation clip_op); - bool CanSkip(ISize render_pass_size, const Matrix& transform) const override; - // |Contents| std::optional GetCoverage(const Entity& entity) const override; diff --git a/impeller/entity/contents/contents.cc b/impeller/entity/contents/contents.cc index fb3e1061d5ec7..421713761175e 100644 --- a/impeller/entity/contents/contents.cc +++ b/impeller/entity/contents/contents.cc @@ -182,8 +182,4 @@ void Contents::SetColorSourceSize(Size size) { color_source_size_ = size; } -bool Contents::CanSkip(ISize render_pass_size, const Matrix& transform) const { - return false; -} - } // namespace impeller diff --git a/impeller/entity/contents/contents.h b/impeller/entity/contents/contents.h index 92e7053d01a4c..c26d1d56a49c8 100644 --- a/impeller/entity/contents/contents.h +++ b/impeller/entity/contents/contents.h @@ -83,13 +83,6 @@ class Contents { /// void SetCoverageHint(std::optional coverage_hint); - //---------------------------------------------------------------------------- - /// @brief Whether this entity can be entirely skipped during rendering. - /// - /// TODO(jonahwilliams): remove this method which was only added for clipping - /// once experimental canvas lands. - virtual bool CanSkip(ISize render_pass_size, const Matrix& transform) const; - const std::optional& GetCoverageHint() const; //---------------------------------------------------------------------------- diff --git a/impeller/entity/entity_pass_clip_stack.cc b/impeller/entity/entity_pass_clip_stack.cc index eab0565b3d382..1758ded671d71 100644 --- a/impeller/entity/entity_pass_clip_stack.cc +++ b/impeller/entity/entity_pass_clip_stack.cc @@ -60,7 +60,7 @@ EntityPassClipStack::ClipStateResult EntityPassClipStack::ApplyClipState( case Contents::ClipCoverage::Type::kNoChange: break; case Contents::ClipCoverage::Type::kAppend: { - auto op = CurrentClipCoverage(); + auto maybe_coverage = CurrentClipCoverage(); // Compute the previous clip height. size_t previous_clip_height = 0; @@ -72,6 +72,23 @@ EntityPassClipStack::ClipStateResult EntityPassClipStack::ApplyClipState( previous_clip_height = clip_height_floor; } + if (!maybe_coverage.has_value()) { + // Running this append op won't impact the clip buffer because the + // whole screen is already being clipped, so skip it. + return result; + } + auto op = maybe_coverage.value(); + + // If the new clip coverage is bigger than the existing coverage, we + // do not need to change the clip region. + if (global_clip_coverage.coverage.has_value() && + global_clip_coverage.coverage.value().Contains(op)) { + subpass_state.clip_coverage.push_back(ClipCoverageLayer{ + .coverage = op, .clip_height = previous_clip_height + 1}); + + return result; + } + subpass_state.clip_coverage.push_back( ClipCoverageLayer{.coverage = global_clip_coverage.coverage, .clip_height = previous_clip_height + 1}); @@ -81,11 +98,6 @@ EntityPassClipStack::ClipStateResult EntityPassClipStack::ApplyClipState( subpass_state.clip_coverage.front().clip_height + subpass_state.clip_coverage.size() - 1); - if (!op.has_value()) { - // Running this append op won't impact the clip buffer because the - // whole screen is already being clipped, so skip it. - return result; - } } break; case Contents::ClipCoverage::Type::kRestore: { ClipRestoreContents* restore_contents = diff --git a/impeller/entity/entity_pass_unittests.cc b/impeller/entity/entity_pass_unittests.cc index fd25599393a4e..915384a413b33 100644 --- a/impeller/entity/entity_pass_unittests.cc +++ b/impeller/entity/entity_pass_unittests.cc @@ -130,6 +130,82 @@ TEST(EntityPassClipStackTest, AppendAndRestoreClipCoverage) { EXPECT_EQ(recorder.GetReplayEntities().size(), 0u); } +// Append two clip coverages, the second is larger the first. This +// should result in the second clip not requiring any update. +TEST(EntityPassClipStackTest, AppendLargerClipCoverage) { + EntityPassClipStack recorder = + EntityPassClipStack(Rect::MakeLTRB(0, 0, 100, 100)); + + ASSERT_EQ(recorder.GetClipCoverageLayers().size(), 1u); + + // Push a clip. + Entity entity; + EntityPassClipStack::ClipStateResult result = recorder.ApplyClipState( + Contents::ClipCoverage{ + .type = Contents::ClipCoverage::Type::kAppend, + .coverage = Rect::MakeLTRB(50, 50, 55, 55), + }, + entity, 0, Point(0, 0)); + EXPECT_TRUE(result.should_render); + EXPECT_TRUE(result.clip_did_change); + + // Push a clip with larger coverage than the previous state. + result = recorder.ApplyClipState( + Contents::ClipCoverage{ + .type = Contents::ClipCoverage::Type::kAppend, + .coverage = Rect::MakeLTRB(0, 0, 100, 100), + }, + entity, 0, Point(0, 0)); + + EXPECT_FALSE(result.should_render); + EXPECT_FALSE(result.clip_did_change); +} + +TEST(EntityPassClipStackTest, AppendDecreasingSizeClipCoverage) { + EntityPassClipStack recorder = + EntityPassClipStack(Rect::MakeLTRB(0, 0, 100, 100)); + + ASSERT_EQ(recorder.GetClipCoverageLayers().size(), 1u); + + // Push Clips that shrink in size. All should be applied. + Entity entity; + + for (auto i = 1; i < 20; i++) { + EntityPassClipStack::ClipStateResult result = recorder.ApplyClipState( + Contents::ClipCoverage{ + .type = Contents::ClipCoverage::Type::kAppend, + .coverage = Rect::MakeLTRB(i, i, 100 - i, 100 - i), + }, + entity, 0, Point(0, 0)); + EXPECT_TRUE(result.should_render); + EXPECT_TRUE(result.clip_did_change); + EXPECT_EQ(recorder.CurrentClipCoverage(), + Rect::MakeLTRB(i, i, 100 - i, 100 - i)); + } +} + +TEST(EntityPassClipStackTest, AppendIncreasingSizeClipCoverage) { + EntityPassClipStack recorder = + EntityPassClipStack(Rect::MakeLTRB(0, 0, 100, 100)); + + ASSERT_EQ(recorder.GetClipCoverageLayers().size(), 1u); + + // Push Clips that grow in size. All should be skipped. + Entity entity; + + for (auto i = 1; i < 20; i++) { + EntityPassClipStack::ClipStateResult result = recorder.ApplyClipState( + Contents::ClipCoverage{ + .type = Contents::ClipCoverage::Type::kAppend, + .coverage = Rect::MakeLTRB(0 - i, 0 - i, 100 + i, 100 + i), + }, + entity, 0, Point(0, 0)); + EXPECT_FALSE(result.should_render); + EXPECT_FALSE(result.clip_did_change); + EXPECT_EQ(recorder.CurrentClipCoverage(), Rect::MakeLTRB(0, 0, 100, 100)); + } +} + TEST(EntityPassClipStackTest, UnbalancedRestore) { EntityPassClipStack recorder = EntityPassClipStack(Rect::MakeLTRB(0, 0, 100, 100)); diff --git a/impeller/entity/entity_unittests.cc b/impeller/entity/entity_unittests.cc index 2d7510f99b392..92408fd7da6e2 100644 --- a/impeller/entity/entity_unittests.cc +++ b/impeller/entity/entity_unittests.cc @@ -1532,35 +1532,6 @@ TEST_P(EntityTest, ClipContentsShouldRenderIsCorrect) { } } -TEST_P(EntityTest, ClipContentsCanSkip) { - // Intersect Clips can be skipped if the render target coverage is fully - // inside of them. - - auto clip = std::make_shared(); - clip->SetGeometry(Geometry::MakeRect(Rect::MakeLTRB(0, 0, 100, 100))); - clip->SetClipOperation(Entity::ClipOperation::kIntersect); - - // Fully Contained - EXPECT_TRUE(clip->CanSkip(ISize::MakeWH(50, 50), {})); - - // Not Intersect - clip->SetClipOperation(Entity::ClipOperation::kDifference); - EXPECT_FALSE(clip->CanSkip(ISize::MakeWH(50, 50), {})); - - // Not Contained - clip->SetClipOperation(Entity::ClipOperation::kIntersect); - EXPECT_FALSE(clip->CanSkip(ISize::MakeWH(200, 200), {})); - - // Not Scale Translate - clip->SetClipOperation(Entity::ClipOperation::kIntersect); - EXPECT_FALSE( - clip->CanSkip(ISize::MakeWH(50, 50), Matrix::MakeRotationX(Radians(2)))); - - // Not Axis Aligned Rectangle. - clip->SetGeometry(Geometry::MakeOval(Rect::MakeLTRB(0, 0, 50, 50))); - EXPECT_FALSE(clip->CanSkip(ISize::MakeWH(50, 50), {})); -} - TEST_P(EntityTest, ClipContentsGetClipCoverageIsCorrect) { // Intersection: No stencil coverage, no geometry. { From 6a5e0b9f1d9e24582402211d9b050c03528d7048 Mon Sep 17 00:00:00 2001 From: jonahwilliams Date: Wed, 28 Aug 2024 15:23:51 -0700 Subject: [PATCH 4/4] ++ --- impeller/entity/contents/clip_contents.cc | 12 ++++++--- impeller/entity/contents/contents.h | 11 ++++++++ impeller/entity/entity_pass_clip_stack.cc | 7 ++--- impeller/entity/entity_pass_unittests.cc | 33 +++++++++++++++++++++++ 4 files changed, 56 insertions(+), 7 deletions(-) diff --git a/impeller/entity/contents/clip_contents.cc b/impeller/entity/contents/clip_contents.cc index 62fa39d3639e5..cd1fc5c863cff 100644 --- a/impeller/entity/contents/clip_contents.cc +++ b/impeller/entity/contents/clip_contents.cc @@ -53,8 +53,11 @@ Contents::ClipCoverage ClipContents::GetClipCoverage( case Entity::ClipOperation::kDifference: // This can be optimized further by considering cases when the bounds of // the current stencil will shrink. - return {.type = ClipCoverage::Type::kAppend, - .coverage = current_clip_coverage}; + return { + .type = ClipCoverage::Type::kAppend, // + .is_difference_or_non_square = true, // + .coverage = current_clip_coverage // + }; case Entity::ClipOperation::kIntersect: if (!geometry_) { return {.type = ClipCoverage::Type::kAppend, .coverage = std::nullopt}; @@ -64,8 +67,9 @@ Contents::ClipCoverage ClipContents::GetClipCoverage( return {.type = ClipCoverage::Type::kAppend, .coverage = std::nullopt}; } return { - .type = ClipCoverage::Type::kAppend, - .coverage = current_clip_coverage->Intersection(coverage.value()), + .type = ClipCoverage::Type::kAppend, // + .is_difference_or_non_square = !geometry_->IsAxisAlignedRect(), // + .coverage = current_clip_coverage->Intersection(coverage.value()), // }; } FML_UNREACHABLE(); diff --git a/impeller/entity/contents/contents.h b/impeller/entity/contents/contents.h index c26d1d56a49c8..8bffbc66a2951 100644 --- a/impeller/entity/contents/contents.h +++ b/impeller/entity/contents/contents.h @@ -38,6 +38,17 @@ class Contents { enum class Type { kNoChange, kAppend, kRestore }; Type type = Type::kNoChange; + // TODO(jonahwilliams): this should probably use the Entity::ClipOperation + // enum, but that has transitive import errors. + bool is_difference_or_non_square = false; + + /// @brief This coverage is the outer coverage of the clip. + /// + /// For example, if the clip is a circular clip, this is the rectangle that + /// contains the circle and not the rectangle that is contained within the + /// circle. This means that we cannot use the coverage alone to determine if + /// a clip can be culled, and instead also use the somewhat hacky + /// "is_difference_or_non_square" field. std::optional coverage = std::nullopt; }; diff --git a/impeller/entity/entity_pass_clip_stack.cc b/impeller/entity/entity_pass_clip_stack.cc index 1758ded671d71..17df6966315e5 100644 --- a/impeller/entity/entity_pass_clip_stack.cc +++ b/impeller/entity/entity_pass_clip_stack.cc @@ -79,9 +79,10 @@ EntityPassClipStack::ClipStateResult EntityPassClipStack::ApplyClipState( } auto op = maybe_coverage.value(); - // If the new clip coverage is bigger than the existing coverage, we - // do not need to change the clip region. - if (global_clip_coverage.coverage.has_value() && + // If the new clip coverage is bigger than the existing coverage for + // intersect clips, we do not need to change the clip region. + if (!global_clip_coverage.is_difference_or_non_square && + global_clip_coverage.coverage.has_value() && global_clip_coverage.coverage.value().Contains(op)) { subpass_state.clip_coverage.push_back(ClipCoverageLayer{ .coverage = op, .clip_height = previous_clip_height + 1}); diff --git a/impeller/entity/entity_pass_unittests.cc b/impeller/entity/entity_pass_unittests.cc index 915384a413b33..2c24eab20c3fb 100644 --- a/impeller/entity/entity_pass_unittests.cc +++ b/impeller/entity/entity_pass_unittests.cc @@ -161,6 +161,39 @@ TEST(EntityPassClipStackTest, AppendLargerClipCoverage) { EXPECT_FALSE(result.clip_did_change); } +// Since clip entities return the outer coverage we can only cull axis aligned +// rectangles and intersect clips. +TEST(EntityPassClipStackTest, + AppendLargerClipCoverageWithDifferenceOrNonSquare) { + EntityPassClipStack recorder = + EntityPassClipStack(Rect::MakeLTRB(0, 0, 100, 100)); + + ASSERT_EQ(recorder.GetClipCoverageLayers().size(), 1u); + + // Push a clip. + Entity entity; + EntityPassClipStack::ClipStateResult result = recorder.ApplyClipState( + Contents::ClipCoverage{ + .type = Contents::ClipCoverage::Type::kAppend, + .coverage = Rect::MakeLTRB(50, 50, 55, 55), + }, + entity, 0, Point(0, 0)); + EXPECT_TRUE(result.should_render); + EXPECT_TRUE(result.clip_did_change); + + // Push a clip with larger coverage than the previous state. + result = recorder.ApplyClipState( + Contents::ClipCoverage{ + .type = Contents::ClipCoverage::Type::kAppend, + .is_difference_or_non_square = true, + .coverage = Rect::MakeLTRB(0, 0, 100, 100), + }, + entity, 0, Point(0, 0)); + + EXPECT_TRUE(result.should_render); + EXPECT_TRUE(result.clip_did_change); +} + TEST(EntityPassClipStackTest, AppendDecreasingSizeClipCoverage) { EntityPassClipStack recorder = EntityPassClipStack(Rect::MakeLTRB(0, 0, 100, 100));