From 5179536acdcb95bc16cc751ea64e45c6fea73063 Mon Sep 17 00:00:00 2001 From: "auto-submit[bot]" Date: Wed, 27 Mar 2024 20:48:03 +0000 Subject: [PATCH] Revert "[Impeller] Use the scissor to limit all draws by clip coverage. (#51698)" This reverts commit e0f86b293cb0ea197c98dd53ef18e43021e4a028. --- impeller/entity/entity_pass.cc | 43 +++----------- impeller/entity/entity_pass_clip_stack.cc | 37 +++++------- impeller/entity/entity_pass_clip_stack.h | 35 +++-------- impeller/entity/entity_pass_unittests.cc | 71 ++++++++--------------- 4 files changed, 52 insertions(+), 134 deletions(-) diff --git a/impeller/entity/entity_pass.cc b/impeller/entity/entity_pass.cc index 26a923e5a93e2..c128dd06e9747 100644 --- a/impeller/entity/entity_pass.cc +++ b/impeller/entity/entity_pass.cc @@ -25,7 +25,6 @@ #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 @@ -753,25 +752,6 @@ 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::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()); - } - pass.SetScissor(scissor); -} - bool EntityPass::RenderElement(Entity& element_entity, size_t clip_depth_floor, InlinePassContext& pass_context, @@ -791,10 +771,8 @@ 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& replay : replay_entities) { - SetClipScissor(clip_coverage_stack.CurrentClipCoverage(), *result.pass, - global_pass_position); - if (!replay.entity.Render(renderer, *result.pass)) { + for (const auto& entity : replay_entities) { + if (!entity.Render(renderer, *result.pass)) { VALIDATION_LOG << "Failed to render entity for clip restore."; } } @@ -848,18 +826,11 @@ bool EntityPass::RenderElement(Entity& element_entity, element_entity.GetContents()->SetCoverageHint( Rect::Intersection(element_coverage_hint, current_clip_coverage)); - 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) { - // We only need to update the pass scissor if the clip state has changed. - SetClipScissor(clip_coverage_stack.CurrentClipCoverage(), *result.pass, - global_pass_position); - } - - if (!clip_state_result.should_render) { + 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. return true; } diff --git a/impeller/entity/entity_pass_clip_stack.cc b/impeller/entity/entity_pass_clip_stack.cc index 67bb02d8e8f4d..5d126acdcbc94 100644 --- a/impeller/entity/entity_pass_clip_stack.cc +++ b/impeller/entity/entity_pass_clip_stack.cc @@ -49,23 +49,20 @@ EntityPassClipStack::GetClipCoverageLayers() const { return subpass_state_.back().clip_coverage; } -EntityPassClipStack::ClipStateResult EntityPassClipStack::ApplyClipState( - Contents::ClipCoverage global_clip_coverage, +bool EntityPassClipStack::AppendClipCoverage( + Contents::ClipCoverage 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) { + switch (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 = global_clip_coverage.coverage, + ClipCoverageLayer{.coverage = 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 + @@ -74,14 +71,14 @@ EntityPassClipStack::ClipStateResult 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 result; + return false; } } break; case Contents::ClipCoverage::Type::kRestore: { if (subpass_state.clip_coverage.back().clip_depth <= entity.GetClipDepth()) { // Drop clip restores that will do nothing. - return result; + return false; } auto restoration_index = entity.GetClipDepth() - @@ -99,19 +96,18 @@ EntityPassClipStack::ClipStateResult 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()); + RecordEntity(entity, clip_coverage.type); } - return result; + return false; } if (!subpass_state.clip_coverage.back().coverage.has_value()) { // Running this restore op won't make anything renderable, so skip it. - return result; + return false; } auto restore_contents = @@ -134,23 +130,19 @@ EntityPassClipStack::ClipStateResult EntityPassClipStack::ApplyClipState( #endif entity.SetClipDepth(entity.GetClipDepth() - clip_depth_floor); - RecordEntity(entity, global_clip_coverage.type, - subpass_state.clip_coverage.back().coverage); + RecordEntity(entity, clip_coverage.type); - result.should_render = true; - return result; + return true; } void EntityPassClipStack::RecordEntity(const Entity& entity, - Contents::ClipCoverage::Type type, - std::optional clip_coverage) { + Contents::ClipCoverage::Type type) { 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 = entity.Clone(), .clip_coverage = clip_coverage}); + subpass_state.rendered_clip_entities.push_back(entity.Clone()); break; case Contents::ClipCoverage::Type::kRestore: if (!subpass_state.rendered_clip_entities.empty()) { @@ -165,8 +157,7 @@ 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 d5181d86b9907..bfd7f5ba83b8e 100644 --- a/impeller/entity/entity_pass_clip_stack.h +++ b/impeller/entity/entity_pass_clip_stack.h @@ -6,8 +6,6 @@ #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 { @@ -23,20 +21,6 @@ struct ClipCoverageLayer { /// stencil buffer is left in an identical state. class EntityPassClipStack { public: - struct ReplayResult { - Entity entity; - std::optional clip_coverage; - }; - - 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; - }; - /// Create a new [EntityPassClipStack] with an initialized coverage rect. explicit EntityPassClipStack(const Rect& initial_coverage_rect); @@ -50,27 +34,24 @@ class EntityPassClipStack { bool HasCoverage() const; - /// @brief Applies the current clip state to an Entity. If the given Entity - /// is a clip operation, then the clip state is updated accordingly. - ClipStateResult ApplyClipState(Contents::ClipCoverage global_clip_coverage, - Entity& entity, - size_t clip_depth_floor, - Point global_pass_position); + /// Returns true if entity should be rendered. + bool AppendClipCoverage(Contents::ClipCoverage 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, - std::optional clip_coverage); + void RecordEntity(const Entity& entity, Contents::ClipCoverage::Type type); // 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 137cbd2e4ad22..ce2a66a82dc7c 100644 --- a/impeller/entity/entity_pass_unittests.cc +++ b/impeller/entity/entity_pass_unittests.cc @@ -17,26 +17,16 @@ TEST(EntityPassClipStackTest, CanPushAndPopEntities) { EXPECT_TRUE(recorder.GetReplayEntities().empty()); Entity entity; - recorder.RecordEntity(entity, Contents::ClipCoverage::Type::kAppend, - Rect::MakeLTRB(0, 0, 100, 100)); + recorder.RecordEntity(entity, Contents::ClipCoverage::Type::kAppend); EXPECT_EQ(recorder.GetReplayEntities().size(), 1u); - recorder.RecordEntity(entity, Contents::ClipCoverage::Type::kAppend, - Rect::MakeLTRB(0, 0, 50, 50)); + recorder.RecordEntity(entity, Contents::ClipCoverage::Type::kAppend); 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()); + + recorder.RecordEntity(entity, Contents::ClipCoverage::Type::kRestore); EXPECT_EQ(recorder.GetReplayEntities().size(), 1u); - recorder.RecordEntity(entity, Contents::ClipCoverage::Type::kRestore, Rect()); + recorder.RecordEntity(entity, Contents::ClipCoverage::Type::kRestore); EXPECT_TRUE(recorder.GetReplayEntities().empty()); } @@ -47,7 +37,7 @@ TEST(EntityPassClipStackTest, CanPopEntitiesSafely) { EXPECT_TRUE(recorder.GetReplayEntities().empty()); Entity entity; - recorder.RecordEntity(entity, Contents::ClipCoverage::Type::kRestore, Rect()); + recorder.RecordEntity(entity, Contents::ClipCoverage::Type::kRestore); EXPECT_TRUE(recorder.GetReplayEntities().empty()); } @@ -58,8 +48,7 @@ TEST(EntityPassClipStackTest, CanAppendNoChange) { EXPECT_TRUE(recorder.GetReplayEntities().empty()); Entity entity; - recorder.RecordEntity(entity, Contents::ClipCoverage::Type::kNoChange, - Rect()); + recorder.RecordEntity(entity, Contents::ClipCoverage::Type::kNoChange); EXPECT_TRUE(recorder.GetReplayEntities().empty()); } @@ -72,14 +61,12 @@ TEST(EntityPassClipStackTest, AppendCoverageNoChange) { EXPECT_EQ(recorder.GetClipCoverageLayers()[0].clip_depth, 0u); Entity entity; - EntityPassClipStack::ClipStateResult result = recorder.ApplyClipState( + recorder.AppendClipCoverage( 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))); @@ -95,14 +82,12 @@ TEST(EntityPassClipStackTest, AppendAndRestoreClipCoverage) { // Push a clip. Entity entity; entity.SetClipDepth(0); - EntityPassClipStack::ClipStateResult result = recorder.ApplyClipState( + recorder.AppendClipCoverage( 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, @@ -112,7 +97,7 @@ TEST(EntityPassClipStackTest, AppendAndRestoreClipCoverage) { // Restore the clip. entity.SetClipDepth(0); - recorder.ApplyClipState( + recorder.AppendClipCoverage( Contents::ClipCoverage{ .type = Contents::ClipCoverage::Type::kRestore, .coverage = Rect::MakeLTRB(50, 50, 55, 55), @@ -135,14 +120,12 @@ TEST(EntityPassClipStackTest, UnbalancedRestore) { // Restore the clip. Entity entity; entity.SetClipDepth(0); - EntityPassClipStack::ClipStateResult result = recorder.ApplyClipState( + recorder.AppendClipCoverage( 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, @@ -160,16 +143,12 @@ TEST(EntityPassClipStackTest, ClipAndRestoreWithSubpasses) { // Push a clip. Entity entity; entity.SetClipDepth(0u); - { - 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); - } + recorder.AppendClipCoverage( + Contents::ClipCoverage{ + .type = Contents::ClipCoverage::Type::kAppend, + .coverage = Rect::MakeLTRB(50, 50, 55, 55), + }, + entity, 0, Point(0, 0)); ASSERT_EQ(recorder.GetClipCoverageLayers().size(), 2u); EXPECT_EQ(recorder.GetClipCoverageLayers()[1].coverage, @@ -184,16 +163,12 @@ TEST(EntityPassClipStackTest, ClipAndRestoreWithSubpasses) { Rect::MakeLTRB(50, 50, 55, 55)); entity.SetClipDepth(1); - { - 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); - } + recorder.AppendClipCoverage( + Contents::ClipCoverage{ + .type = Contents::ClipCoverage::Type::kAppend, + .coverage = Rect::MakeLTRB(54, 54, 55, 55), + }, + entity, 0, Point(0, 0)); EXPECT_EQ(recorder.GetClipCoverageLayers()[1].coverage, Rect::MakeLTRB(54, 54, 55, 55));