From f527288a2c6b723e35309a573237ee531eb131ec Mon Sep 17 00:00:00 2001 From: Brandon DeRosier Date: Tue, 26 Mar 2024 19:47:47 -0700 Subject: [PATCH 1/5] [Impeller] Use the scissor to limit all draws by clip coverage. --- impeller/entity/entity_pass.cc | 39 +++++++++++++++++++---- impeller/entity/entity_pass_clip_stack.cc | 22 +++++++------ impeller/entity/entity_pass_clip_stack.h | 28 +++++++++++----- impeller/entity/entity_pass_unittests.cc | 35 ++++++++++++-------- 4 files changed, 87 insertions(+), 37 deletions(-) diff --git a/impeller/entity/entity_pass.cc b/impeller/entity/entity_pass.cc index c128dd06e9747..702fc67d4b291 100644 --- a/impeller/entity/entity_pass.cc +++ b/impeller/entity/entity_pass.cc @@ -25,6 +25,7 @@ #include "impeller/entity/inline_pass_context.h" #include "impeller/geometry/color.h" #include "impeller/geometry/rect.h" +#include "impeller/geometry/size.h" #include "impeller/renderer/command_buffer.h" #ifdef IMPELLER_DEBUG @@ -752,6 +753,25 @@ EntityPass::EntityResult EntityPass::GetEntityForElement( FML_UNREACHABLE(); } +static void SetClipScissor(std::optional clip_coverage, + RenderPass& pass, + Point global_pass_position) { + if constexpr (!ContentContext::kEnableStencilThenCover) { + return; + } + // Set the scissor to the clip coverage area. We do this prior to rendering + // the clip itself and all its contents. + IRect scissor; + if (clip_coverage.has_value()) { + clip_coverage = clip_coverage->Shift(-global_pass_position); + scissor = IRect::MakeLTRB(std::floor(clip_coverage->GetLeft()), + std::floor(clip_coverage->GetTop()), + std::ceil(clip_coverage->GetRight()), + std::ceil(clip_coverage->GetBottom())); + } + pass.SetScissor(scissor); +} + bool EntityPass::RenderElement(Entity& element_entity, size_t clip_depth_floor, InlinePassContext& pass_context, @@ -771,8 +791,10 @@ bool EntityPass::RenderElement(Entity& element_entity, // Restore any clips that were recorded before the backdrop filter was // applied. auto& replay_entities = clip_coverage_stack.GetReplayEntities(); - for (const auto& entity : replay_entities) { - if (!entity.Render(renderer, *result.pass)) { + for (const auto& replay : replay_entities) { + SetClipScissor(clip_coverage_stack.CurrentClipCoverage(), *result.pass, + global_pass_position); + if (!replay.entity.Render(renderer, *result.pass)) { VALIDATION_LOG << "Failed to render entity for clip restore."; } } @@ -826,11 +848,14 @@ bool EntityPass::RenderElement(Entity& element_entity, element_entity.GetContents()->SetCoverageHint( Rect::Intersection(element_coverage_hint, current_clip_coverage)); - if (!clip_coverage_stack.AppendClipCoverage(clip_coverage, element_entity, - clip_depth_floor, - global_pass_position)) { - // If the entity's coverage change did not change the clip coverage, we - // don't need to render it. + bool should_render = clip_coverage_stack.ApplyClipState( + clip_coverage, element_entity, clip_depth_floor, global_pass_position); + + SetClipScissor(clip_coverage_stack.CurrentClipCoverage(), *result.pass, + global_pass_position); + + if (!should_render) { + // `ApplyClipState` returns false if the Entity can be safely skipped. return true; } diff --git a/impeller/entity/entity_pass_clip_stack.cc b/impeller/entity/entity_pass_clip_stack.cc index 5d126acdcbc94..ccab1ab218438 100644 --- a/impeller/entity/entity_pass_clip_stack.cc +++ b/impeller/entity/entity_pass_clip_stack.cc @@ -49,19 +49,19 @@ EntityPassClipStack::GetClipCoverageLayers() const { return subpass_state_.back().clip_coverage; } -bool EntityPassClipStack::AppendClipCoverage( - Contents::ClipCoverage clip_coverage, +bool EntityPassClipStack::ApplyClipState( + Contents::ClipCoverage global_clip_coverage, Entity& entity, size_t clip_depth_floor, Point global_pass_position) { auto& subpass_state = GetCurrentSubpassState(); - switch (clip_coverage.type) { + switch (global_clip_coverage.type) { case Contents::ClipCoverage::Type::kNoChange: break; case Contents::ClipCoverage::Type::kAppend: { auto op = CurrentClipCoverage(); subpass_state.clip_coverage.push_back( - ClipCoverageLayer{.coverage = clip_coverage.coverage, + ClipCoverageLayer{.coverage = global_clip_coverage.coverage, .clip_depth = entity.GetClipDepth() + 1}); FML_DCHECK(subpass_state.clip_coverage.back().clip_depth == @@ -100,7 +100,7 @@ bool EntityPassClipStack::AppendClipCoverage( if constexpr (ContentContext::kEnableStencilThenCover) { // Skip all clip restores when stencil-then-cover is enabled. if (subpass_state.clip_coverage.back().coverage.has_value()) { - RecordEntity(entity, clip_coverage.type); + RecordEntity(entity, global_clip_coverage.type, Rect()); } return false; } @@ -130,19 +130,22 @@ bool EntityPassClipStack::AppendClipCoverage( #endif entity.SetClipDepth(entity.GetClipDepth() - clip_depth_floor); - RecordEntity(entity, clip_coverage.type); + RecordEntity(entity, global_clip_coverage.type, + subpass_state.clip_coverage.back().coverage); return true; } void EntityPassClipStack::RecordEntity(const Entity& entity, - Contents::ClipCoverage::Type type) { + Contents::ClipCoverage::Type type, + std::optional clip_coverage) { auto& subpass_state = GetCurrentSubpassState(); switch (type) { case Contents::ClipCoverage::Type::kNoChange: return; case Contents::ClipCoverage::Type::kAppend: - subpass_state.rendered_clip_entities.push_back(entity.Clone()); + subpass_state.rendered_clip_entities.push_back( + {.entity = entity.Clone(), .clip_coverage = clip_coverage}); break; case Contents::ClipCoverage::Type::kRestore: if (!subpass_state.rendered_clip_entities.empty()) { @@ -157,7 +160,8 @@ EntityPassClipStack::GetCurrentSubpassState() { return subpass_state_.back(); } -const std::vector& EntityPassClipStack::GetReplayEntities() const { +const std::vector& +EntityPassClipStack::GetReplayEntities() const { return subpass_state_.back().rendered_clip_entities; } diff --git a/impeller/entity/entity_pass_clip_stack.h b/impeller/entity/entity_pass_clip_stack.h index bfd7f5ba83b8e..2d6cd919af00e 100644 --- a/impeller/entity/entity_pass_clip_stack.h +++ b/impeller/entity/entity_pass_clip_stack.h @@ -6,6 +6,8 @@ #define FLUTTER_IMPELLER_ENTITY_ENTITY_PASS_CLIP_STACK_H_ #include "impeller/entity/contents/contents.h" +#include "impeller/entity/entity.h" +#include "impeller/geometry/rect.h" namespace impeller { @@ -21,6 +23,11 @@ struct ClipCoverageLayer { /// stencil buffer is left in an identical state. class EntityPassClipStack { public: + struct ReplayResult { + Entity entity; + std::optional clip_coverage; + }; + /// Create a new [EntityPassClipStack] with an initialized coverage rect. explicit EntityPassClipStack(const Rect& initial_coverage_rect); @@ -34,24 +41,29 @@ class EntityPassClipStack { bool HasCoverage() const; - /// Returns true if entity should be rendered. - bool AppendClipCoverage(Contents::ClipCoverage clip_coverage, - Entity& entity, - size_t clip_depth_floor, - Point global_pass_position); + /// @brief Applies the current clip state to an Entity. If the given Entity + /// is a clip operation, then the clip state is updated accordingly. + /// + /// @return true if the Entity should be rendered. + bool ApplyClipState(Contents::ClipCoverage global_clip_coverage, + Entity& entity, + size_t clip_depth_floor, + Point global_pass_position); // Visible for testing. - void RecordEntity(const Entity& entity, Contents::ClipCoverage::Type type); + void RecordEntity(const Entity& entity, + Contents::ClipCoverage::Type type, + std::optional clip_coverage); // Visible for testing. - const std::vector& GetReplayEntities() const; + const std::vector& GetReplayEntities() const; // Visible for testing. const std::vector GetClipCoverageLayers() const; private: struct SubpassState { - std::vector rendered_clip_entities; + std::vector rendered_clip_entities; std::vector clip_coverage; }; diff --git a/impeller/entity/entity_pass_unittests.cc b/impeller/entity/entity_pass_unittests.cc index ce2a66a82dc7c..3f6c0cb3504bd 100644 --- a/impeller/entity/entity_pass_unittests.cc +++ b/impeller/entity/entity_pass_unittests.cc @@ -17,16 +17,24 @@ TEST(EntityPassClipStackTest, CanPushAndPopEntities) { EXPECT_TRUE(recorder.GetReplayEntities().empty()); Entity entity; - recorder.RecordEntity(entity, Contents::ClipCoverage::Type::kAppend); + recorder.RecordEntity(entity, Contents::ClipCoverage::Type::kAppend, + Rect::MakeLTRB(0, 0, 100, 100)); EXPECT_EQ(recorder.GetReplayEntities().size(), 1u); - recorder.RecordEntity(entity, Contents::ClipCoverage::Type::kAppend); + recorder.RecordEntity(entity, Contents::ClipCoverage::Type::kAppend, + Rect::MakeLTRB(0, 0, 50, 50)); EXPECT_EQ(recorder.GetReplayEntities().size(), 2u); - - recorder.RecordEntity(entity, Contents::ClipCoverage::Type::kRestore); + ASSERT_TRUE(recorder.GetReplayEntities()[0].clip_coverage.has_value()); + ASSERT_TRUE(recorder.GetReplayEntities()[1].clip_coverage.has_value()); + EXPECT_EQ(recorder.GetReplayEntities()[0].clip_coverage.value(), + Rect::MakeLTRB(0, 0, 100, 100)); + EXPECT_EQ(recorder.GetReplayEntities()[1].clip_coverage.value(), + Rect::MakeLTRB(0, 0, 50, 50)); + + recorder.RecordEntity(entity, Contents::ClipCoverage::Type::kRestore, Rect()); EXPECT_EQ(recorder.GetReplayEntities().size(), 1u); - recorder.RecordEntity(entity, Contents::ClipCoverage::Type::kRestore); + recorder.RecordEntity(entity, Contents::ClipCoverage::Type::kRestore, Rect()); EXPECT_TRUE(recorder.GetReplayEntities().empty()); } @@ -37,7 +45,7 @@ TEST(EntityPassClipStackTest, CanPopEntitiesSafely) { EXPECT_TRUE(recorder.GetReplayEntities().empty()); Entity entity; - recorder.RecordEntity(entity, Contents::ClipCoverage::Type::kRestore); + recorder.RecordEntity(entity, Contents::ClipCoverage::Type::kRestore, Rect()); EXPECT_TRUE(recorder.GetReplayEntities().empty()); } @@ -48,7 +56,8 @@ TEST(EntityPassClipStackTest, CanAppendNoChange) { EXPECT_TRUE(recorder.GetReplayEntities().empty()); Entity entity; - recorder.RecordEntity(entity, Contents::ClipCoverage::Type::kNoChange); + recorder.RecordEntity(entity, Contents::ClipCoverage::Type::kNoChange, + Rect()); EXPECT_TRUE(recorder.GetReplayEntities().empty()); } @@ -61,7 +70,7 @@ TEST(EntityPassClipStackTest, AppendCoverageNoChange) { EXPECT_EQ(recorder.GetClipCoverageLayers()[0].clip_depth, 0u); Entity entity; - recorder.AppendClipCoverage( + recorder.ApplyClipState( Contents::ClipCoverage{ .type = Contents::ClipCoverage::Type::kNoChange, .coverage = std::nullopt, @@ -82,7 +91,7 @@ TEST(EntityPassClipStackTest, AppendAndRestoreClipCoverage) { // Push a clip. Entity entity; entity.SetClipDepth(0); - recorder.AppendClipCoverage( + recorder.ApplyClipState( Contents::ClipCoverage{ .type = Contents::ClipCoverage::Type::kAppend, .coverage = Rect::MakeLTRB(50, 50, 55, 55), @@ -97,7 +106,7 @@ TEST(EntityPassClipStackTest, AppendAndRestoreClipCoverage) { // Restore the clip. entity.SetClipDepth(0); - recorder.AppendClipCoverage( + recorder.ApplyClipState( Contents::ClipCoverage{ .type = Contents::ClipCoverage::Type::kRestore, .coverage = Rect::MakeLTRB(50, 50, 55, 55), @@ -120,7 +129,7 @@ TEST(EntityPassClipStackTest, UnbalancedRestore) { // Restore the clip. Entity entity; entity.SetClipDepth(0); - recorder.AppendClipCoverage( + recorder.ApplyClipState( Contents::ClipCoverage{ .type = Contents::ClipCoverage::Type::kRestore, .coverage = Rect::MakeLTRB(50, 50, 55, 55), @@ -143,7 +152,7 @@ TEST(EntityPassClipStackTest, ClipAndRestoreWithSubpasses) { // Push a clip. Entity entity; entity.SetClipDepth(0u); - recorder.AppendClipCoverage( + recorder.ApplyClipState( Contents::ClipCoverage{ .type = Contents::ClipCoverage::Type::kAppend, .coverage = Rect::MakeLTRB(50, 50, 55, 55), @@ -163,7 +172,7 @@ TEST(EntityPassClipStackTest, ClipAndRestoreWithSubpasses) { Rect::MakeLTRB(50, 50, 55, 55)); entity.SetClipDepth(1); - recorder.AppendClipCoverage( + recorder.ApplyClipState( Contents::ClipCoverage{ .type = Contents::ClipCoverage::Type::kAppend, .coverage = Rect::MakeLTRB(54, 54, 55, 55), From c8bce527b894a5ec4b413752301cafbbee2d18bf Mon Sep 17 00:00:00 2001 From: Brandon DeRosier Date: Tue, 26 Mar 2024 20:24:14 -0700 Subject: [PATCH 2/5] Only update the scissor when the clip stack changes. --- impeller/entity/entity_pass.cc | 17 +++++---- impeller/entity/entity_pass_clip_stack.cc | 17 +++++---- impeller/entity/entity_pass_clip_stack.h | 15 ++++---- impeller/entity/entity_pass_unittests.cc | 44 +++++++++++++++-------- 4 files changed, 59 insertions(+), 34 deletions(-) diff --git a/impeller/entity/entity_pass.cc b/impeller/entity/entity_pass.cc index 702fc67d4b291..03d45bfb9df90 100644 --- a/impeller/entity/entity_pass.cc +++ b/impeller/entity/entity_pass.cc @@ -848,14 +848,17 @@ bool EntityPass::RenderElement(Entity& element_entity, element_entity.GetContents()->SetCoverageHint( Rect::Intersection(element_coverage_hint, current_clip_coverage)); - bool should_render = clip_coverage_stack.ApplyClipState( - clip_coverage, element_entity, clip_depth_floor, global_pass_position); - - SetClipScissor(clip_coverage_stack.CurrentClipCoverage(), *result.pass, - global_pass_position); + EntityPassClipStack::ClipStateResult clip_state_result = + clip_coverage_stack.ApplyClipState(clip_coverage, element_entity, + clip_depth_floor, + global_pass_position); + + if (clip_state_result.clip_did_change) { + SetClipScissor(clip_coverage_stack.CurrentClipCoverage(), *result.pass, + global_pass_position); + } - if (!should_render) { - // `ApplyClipState` returns false if the Entity can be safely skipped. + if (!clip_state_result.should_render) { return true; } diff --git a/impeller/entity/entity_pass_clip_stack.cc b/impeller/entity/entity_pass_clip_stack.cc index ccab1ab218438..67bb02d8e8f4d 100644 --- a/impeller/entity/entity_pass_clip_stack.cc +++ b/impeller/entity/entity_pass_clip_stack.cc @@ -49,11 +49,13 @@ EntityPassClipStack::GetClipCoverageLayers() const { return subpass_state_.back().clip_coverage; } -bool EntityPassClipStack::ApplyClipState( +EntityPassClipStack::ClipStateResult EntityPassClipStack::ApplyClipState( Contents::ClipCoverage global_clip_coverage, Entity& entity, size_t clip_depth_floor, Point global_pass_position) { + ClipStateResult result = {.should_render = false, .clip_did_change = false}; + auto& subpass_state = GetCurrentSubpassState(); switch (global_clip_coverage.type) { case Contents::ClipCoverage::Type::kNoChange: @@ -63,6 +65,7 @@ bool EntityPassClipStack::ApplyClipState( subpass_state.clip_coverage.push_back( ClipCoverageLayer{.coverage = global_clip_coverage.coverage, .clip_depth = entity.GetClipDepth() + 1}); + result.clip_did_change = true; FML_DCHECK(subpass_state.clip_coverage.back().clip_depth == subpass_state.clip_coverage.front().clip_depth + @@ -71,14 +74,14 @@ bool EntityPassClipStack::ApplyClipState( 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 false; + return result; } } break; case Contents::ClipCoverage::Type::kRestore: { if (subpass_state.clip_coverage.back().clip_depth <= entity.GetClipDepth()) { // Drop clip restores that will do nothing. - return false; + return result; } auto restoration_index = entity.GetClipDepth() - @@ -96,18 +99,19 @@ bool EntityPassClipStack::ApplyClipState( restore_coverage = restore_coverage->Shift(-global_pass_position); } subpass_state.clip_coverage.resize(restoration_index + 1); + result.clip_did_change = true; if constexpr (ContentContext::kEnableStencilThenCover) { // Skip all clip restores when stencil-then-cover is enabled. if (subpass_state.clip_coverage.back().coverage.has_value()) { RecordEntity(entity, global_clip_coverage.type, Rect()); } - return false; + return result; } if (!subpass_state.clip_coverage.back().coverage.has_value()) { // Running this restore op won't make anything renderable, so skip it. - return false; + return result; } auto restore_contents = @@ -133,7 +137,8 @@ bool EntityPassClipStack::ApplyClipState( RecordEntity(entity, global_clip_coverage.type, subpass_state.clip_coverage.back().coverage); - return true; + result.should_render = true; + return result; } void EntityPassClipStack::RecordEntity(const Entity& entity, diff --git a/impeller/entity/entity_pass_clip_stack.h b/impeller/entity/entity_pass_clip_stack.h index 2d6cd919af00e..838e54ebb773f 100644 --- a/impeller/entity/entity_pass_clip_stack.h +++ b/impeller/entity/entity_pass_clip_stack.h @@ -28,6 +28,11 @@ class EntityPassClipStack { std::optional clip_coverage; }; + struct ClipStateResult { + bool should_render = false; + bool clip_did_change = false; + }; + /// Create a new [EntityPassClipStack] with an initialized coverage rect. explicit EntityPassClipStack(const Rect& initial_coverage_rect); @@ -43,12 +48,10 @@ class EntityPassClipStack { /// @brief Applies the current clip state to an Entity. If the given Entity /// is a clip operation, then the clip state is updated accordingly. - /// - /// @return true if the Entity should be rendered. - bool ApplyClipState(Contents::ClipCoverage global_clip_coverage, - Entity& entity, - size_t clip_depth_floor, - Point global_pass_position); + ClipStateResult ApplyClipState(Contents::ClipCoverage global_clip_coverage, + Entity& entity, + size_t clip_depth_floor, + Point global_pass_position); // Visible for testing. void RecordEntity(const Entity& entity, diff --git a/impeller/entity/entity_pass_unittests.cc b/impeller/entity/entity_pass_unittests.cc index 3f6c0cb3504bd..89305c0a47ef1 100644 --- a/impeller/entity/entity_pass_unittests.cc +++ b/impeller/entity/entity_pass_unittests.cc @@ -70,12 +70,14 @@ TEST(EntityPassClipStackTest, AppendCoverageNoChange) { EXPECT_EQ(recorder.GetClipCoverageLayers()[0].clip_depth, 0u); Entity entity; - recorder.ApplyClipState( + EntityPassClipStack::ClipStateResult result = recorder.ApplyClipState( Contents::ClipCoverage{ .type = Contents::ClipCoverage::Type::kNoChange, .coverage = std::nullopt, }, entity, 0, Point(0, 0)); + EXPECT_TRUE(result.should_render); + EXPECT_FALSE(result.clip_did_change); EXPECT_EQ(recorder.GetClipCoverageLayers()[0].coverage, Rect::MakeSize(Size::MakeWH(100, 100))); @@ -91,12 +93,14 @@ TEST(EntityPassClipStackTest, AppendAndRestoreClipCoverage) { // Push a clip. Entity entity; entity.SetClipDepth(0); - recorder.ApplyClipState( + 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); ASSERT_EQ(recorder.GetClipCoverageLayers().size(), 2u); EXPECT_EQ(recorder.GetClipCoverageLayers()[1].coverage, @@ -129,12 +133,14 @@ TEST(EntityPassClipStackTest, UnbalancedRestore) { // Restore the clip. Entity entity; entity.SetClipDepth(0); - recorder.ApplyClipState( + EntityPassClipStack::ClipStateResult result = recorder.ApplyClipState( Contents::ClipCoverage{ .type = Contents::ClipCoverage::Type::kRestore, .coverage = Rect::MakeLTRB(50, 50, 55, 55), }, entity, 0, Point(0, 0)); + EXPECT_FALSE(result.should_render); + EXPECT_FALSE(result.clip_did_change); ASSERT_EQ(recorder.GetClipCoverageLayers().size(), 1u); EXPECT_EQ(recorder.GetClipCoverageLayers()[0].coverage, @@ -152,12 +158,16 @@ TEST(EntityPassClipStackTest, ClipAndRestoreWithSubpasses) { // Push a clip. Entity entity; entity.SetClipDepth(0u); - recorder.ApplyClipState( - Contents::ClipCoverage{ - .type = Contents::ClipCoverage::Type::kAppend, - .coverage = Rect::MakeLTRB(50, 50, 55, 55), - }, - entity, 0, Point(0, 0)); + { + 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); + } ASSERT_EQ(recorder.GetClipCoverageLayers().size(), 2u); EXPECT_EQ(recorder.GetClipCoverageLayers()[1].coverage, @@ -172,12 +182,16 @@ TEST(EntityPassClipStackTest, ClipAndRestoreWithSubpasses) { Rect::MakeLTRB(50, 50, 55, 55)); entity.SetClipDepth(1); - recorder.ApplyClipState( - Contents::ClipCoverage{ - .type = Contents::ClipCoverage::Type::kAppend, - .coverage = Rect::MakeLTRB(54, 54, 55, 55), - }, - entity, 0, Point(0, 0)); + { + EntityPassClipStack::ClipStateResult result = recorder.ApplyClipState( + Contents::ClipCoverage{ + .type = Contents::ClipCoverage::Type::kAppend, + .coverage = Rect::MakeLTRB(54, 54, 55, 55), + }, + entity, 0, Point(0, 0)); + EXPECT_TRUE(result.should_render); + EXPECT_TRUE(result.clip_did_change); + } EXPECT_EQ(recorder.GetClipCoverageLayers()[1].coverage, Rect::MakeLTRB(54, 54, 55, 55)); From 2c3b97f38282855506948496e6ae00902c1c25dc Mon Sep 17 00:00:00 2001 From: Brandon DeRosier Date: Tue, 26 Mar 2024 23:32:21 -0700 Subject: [PATCH 3/5] disable optional check --- impeller/entity/entity_pass_unittests.cc | 2 ++ 1 file changed, 2 insertions(+) diff --git a/impeller/entity/entity_pass_unittests.cc b/impeller/entity/entity_pass_unittests.cc index 89305c0a47ef1..137cbd2e4ad22 100644 --- a/impeller/entity/entity_pass_unittests.cc +++ b/impeller/entity/entity_pass_unittests.cc @@ -26,10 +26,12 @@ TEST(EntityPassClipStackTest, CanPushAndPopEntities) { EXPECT_EQ(recorder.GetReplayEntities().size(), 2u); ASSERT_TRUE(recorder.GetReplayEntities()[0].clip_coverage.has_value()); ASSERT_TRUE(recorder.GetReplayEntities()[1].clip_coverage.has_value()); + // NOLINTBEGIN(bugprone-unchecked-optional-access) EXPECT_EQ(recorder.GetReplayEntities()[0].clip_coverage.value(), Rect::MakeLTRB(0, 0, 100, 100)); EXPECT_EQ(recorder.GetReplayEntities()[1].clip_coverage.value(), Rect::MakeLTRB(0, 0, 50, 50)); + // NOLINTEND(bugprone-unchecked-optional-access) recorder.RecordEntity(entity, Contents::ClipCoverage::Type::kRestore, Rect()); EXPECT_EQ(recorder.GetReplayEntities().size(), 1u); From a59a70653ed159dd9285b0ccfbeecd03c148b4d2 Mon Sep 17 00:00:00 2001 From: Brandon DeRosier Date: Wed, 27 Mar 2024 05:05:00 -0700 Subject: [PATCH 4/5] Limit the scissor to the pass size. --- impeller/entity/entity_pass.cc | 2 ++ 1 file changed, 2 insertions(+) diff --git a/impeller/entity/entity_pass.cc b/impeller/entity/entity_pass.cc index 03d45bfb9df90..7f4854396579e 100644 --- a/impeller/entity/entity_pass.cc +++ b/impeller/entity/entity_pass.cc @@ -768,6 +768,8 @@ static void SetClipScissor(std::optional clip_coverage, std::floor(clip_coverage->GetTop()), std::ceil(clip_coverage->GetRight()), std::ceil(clip_coverage->GetBottom())); + scissor = scissor.Intersection(IRect::MakeSize(pass.GetRenderTargetSize())) + .value_or(IRect()); } pass.SetScissor(scissor); } From 64ae64ddaf75e164022da4c6ff636d8e5a5932ac Mon Sep 17 00:00:00 2001 From: Brandon DeRosier Date: Wed, 27 Mar 2024 10:33:04 -0700 Subject: [PATCH 5/5] Jonah review --- impeller/entity/entity_pass.cc | 7 +++---- impeller/entity/entity_pass_clip_stack.h | 4 ++++ 2 files changed, 7 insertions(+), 4 deletions(-) diff --git a/impeller/entity/entity_pass.cc b/impeller/entity/entity_pass.cc index 7f4854396579e..26a923e5a93e2 100644 --- a/impeller/entity/entity_pass.cc +++ b/impeller/entity/entity_pass.cc @@ -764,10 +764,8 @@ static void SetClipScissor(std::optional clip_coverage, IRect scissor; if (clip_coverage.has_value()) { clip_coverage = clip_coverage->Shift(-global_pass_position); - scissor = IRect::MakeLTRB(std::floor(clip_coverage->GetLeft()), - std::floor(clip_coverage->GetTop()), - std::ceil(clip_coverage->GetRight()), - std::ceil(clip_coverage->GetBottom())); + scissor = IRect::RoundOut(clip_coverage.value()); + // The scissor rect must not exceed the size of the render target. scissor = scissor.Intersection(IRect::MakeSize(pass.GetRenderTargetSize())) .value_or(IRect()); } @@ -856,6 +854,7 @@ bool EntityPass::RenderElement(Entity& element_entity, global_pass_position); if (clip_state_result.clip_did_change) { + // We only need to update the pass scissor if the clip state has changed. SetClipScissor(clip_coverage_stack.CurrentClipCoverage(), *result.pass, global_pass_position); } diff --git a/impeller/entity/entity_pass_clip_stack.h b/impeller/entity/entity_pass_clip_stack.h index 838e54ebb773f..d5181d86b9907 100644 --- a/impeller/entity/entity_pass_clip_stack.h +++ b/impeller/entity/entity_pass_clip_stack.h @@ -29,7 +29,11 @@ class EntityPassClipStack { }; struct ClipStateResult { + /// Whether or not the Entity should be rendered. If false, the Entity may + /// be safely skipped. bool should_render = false; + /// Whether or not the current clip coverage changed during the call to + /// `ApplyClipState`. bool clip_did_change = false; };