From bd1cedbd9bd0958fa36d856c47893ae202beb85f Mon Sep 17 00:00:00 2001 From: jonahwilliams Date: Tue, 5 Dec 2023 21:40:22 -0800 Subject: [PATCH 1/4] [Impeller] disable entity culling by default. --- impeller/entity/contents/clip_contents.cc | 10 ++++------ impeller/entity/contents/clip_contents.h | 4 ++-- impeller/entity/contents/contents.cc | 8 ++------ impeller/entity/contents/contents.h | 2 +- impeller/entity/entity.cc | 9 ++++++++- 5 files changed, 17 insertions(+), 16 deletions(-) diff --git a/impeller/entity/contents/clip_contents.cc b/impeller/entity/contents/clip_contents.cc index 8779ab9b44a49..e8af187dfa604 100644 --- a/impeller/entity/contents/clip_contents.cc +++ b/impeller/entity/contents/clip_contents.cc @@ -62,9 +62,8 @@ Contents::ClipCoverage ClipContents::GetClipCoverage( FML_UNREACHABLE(); } -bool ClipContents::ShouldRender( - const Entity& entity, - const std::optional& clip_coverage) const { +bool ClipContents::ShouldRender(const Entity& entity, + const Rect& clip_coverage) const { return true; } @@ -159,9 +158,8 @@ Contents::ClipCoverage ClipRestoreContents::GetClipCoverage( return {.type = ClipCoverage::Type::kRestore, .coverage = std::nullopt}; } -bool ClipRestoreContents::ShouldRender( - const Entity& entity, - const std::optional& clip_coverage) const { +bool ClipRestoreContents::ShouldRender(const Entity& entity, + const Rect& clip_coverage) const { return true; } diff --git a/impeller/entity/contents/clip_contents.h b/impeller/entity/contents/clip_contents.h index 410e1a7c2ae2f..dc34cf8e195e9 100644 --- a/impeller/entity/contents/clip_contents.h +++ b/impeller/entity/contents/clip_contents.h @@ -35,7 +35,7 @@ class ClipContents final : public Contents { // |Contents| bool ShouldRender(const Entity& entity, - const std::optional& clip_coverage) const override; + const Rect& clip_coverage) const override; // |Contents| bool Render(const ContentContext& renderer, @@ -78,7 +78,7 @@ class ClipRestoreContents final : public Contents { // |Contents| bool ShouldRender(const Entity& entity, - const std::optional& clip_coverage) const override; + const Rect& clip_coverage) const override; // |Contents| bool Render(const ContentContext& renderer, diff --git a/impeller/entity/contents/contents.cc b/impeller/entity/contents/contents.cc index 06178fb601038..b848f72ec45f2 100644 --- a/impeller/entity/contents/contents.cc +++ b/impeller/entity/contents/contents.cc @@ -131,11 +131,7 @@ bool Contents::ApplyColorFilter( } bool Contents::ShouldRender(const Entity& entity, - const std::optional& clip_coverage) const { - if (!clip_coverage.has_value()) { - return false; - } - + const Rect& clip_coverage) const { auto coverage = GetCoverage(entity); if (!coverage.has_value()) { return false; @@ -143,7 +139,7 @@ bool Contents::ShouldRender(const Entity& entity, if (coverage == Rect::MakeMaximum()) { return true; } - return clip_coverage->IntersectsWithRect(coverage.value()); + return clip_coverage.IntersectsWithRect(coverage.value()); } void Contents::SetCoverageHint(std::optional coverage_hint) { diff --git a/impeller/entity/contents/contents.h b/impeller/entity/contents/contents.h index 9c40846cff690..4aac76e0399c6 100644 --- a/impeller/entity/contents/contents.h +++ b/impeller/entity/contents/contents.h @@ -123,7 +123,7 @@ class Contents { const std::string& label = "Snapshot") const; virtual bool ShouldRender(const Entity& entity, - const std::optional& clip_coverage) const; + const Rect& clip_coverage) const; //---------------------------------------------------------------------------- /// @brief Return the color source's intrinsic size, if available. diff --git a/impeller/entity/entity.cc b/impeller/entity/entity.cc index 24bf0efa84958..d61962db8c5a0 100644 --- a/impeller/entity/entity.cc +++ b/impeller/entity/entity.cc @@ -71,7 +71,14 @@ Contents::ClipCoverage Entity::GetClipCoverage( } bool Entity::ShouldRender(const std::optional& clip_coverage) const { - return contents_->ShouldRender(*this, clip_coverage); + if (!clip_coverage.has_value()) { + return false; + } +#ifdef IMPELLER_CONTENT_CULLING + return contents_->ShouldRender(*this, clip_coverage.value()); +#else + return true; +#endif // IMPELLER_CONTENT_CULLING } void Entity::SetContents(std::shared_ptr contents) { From 6603682e6a51e4d81692c19767100b2391d47918 Mon Sep 17 00:00:00 2001 From: jonahwilliams Date: Tue, 5 Dec 2023 23:00:02 -0800 Subject: [PATCH 2/4] add test. --- impeller/entity/entity_unittests.cc | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/impeller/entity/entity_unittests.cc b/impeller/entity/entity_unittests.cc index 520f7a1c203b2..be77915983a0a 100644 --- a/impeller/entity/entity_unittests.cc +++ b/impeller/entity/entity_unittests.cc @@ -1605,6 +1605,20 @@ TEST_P(EntityTest, SolidFillShouldRenderIsCorrect) { } } +TEST_P(EntityTest, DoesNotCullEntitiesByDefault) { + auto fill = std::make_shared(); + fill->SetColor(Color::CornflowerBlue()); + fill->SetGeometry( + Geometry::MakeRect(Rect::MakeLTRB(-1000, -1000, -900, -900))); + + Entity entity; + entity.SetContents(fill); + + // Even though the entity is offscreen, this should still render because we do + // not compute the coverage intersection by default. + EXPECT_TRUE(entity.ShouldRender(Rect::MakeLTRB(0, 0, 100, 100))); +} + TEST_P(EntityTest, ClipContentsShouldRenderIsCorrect) { // For clip ops, `ShouldRender` should always return true. From 7f4628a83fafdfeb2bf4e8d0fe1c4fc8d1de78e9 Mon Sep 17 00:00:00 2001 From: jonahwilliams Date: Wed, 6 Dec 2023 09:11:18 -0800 Subject: [PATCH 3/4] dont accidentally cull clips --- impeller/entity/contents/clip_contents.cc | 7 ++++--- impeller/entity/contents/clip_contents.h | 4 ++-- impeller/entity/contents/contents.cc | 9 ++++++++- impeller/entity/contents/contents.h | 2 +- impeller/entity/entity.cc | 9 +-------- 5 files changed, 16 insertions(+), 15 deletions(-) diff --git a/impeller/entity/contents/clip_contents.cc b/impeller/entity/contents/clip_contents.cc index e8af187dfa604..514d42f5f062f 100644 --- a/impeller/entity/contents/clip_contents.cc +++ b/impeller/entity/contents/clip_contents.cc @@ -63,7 +63,7 @@ Contents::ClipCoverage ClipContents::GetClipCoverage( } bool ClipContents::ShouldRender(const Entity& entity, - const Rect& clip_coverage) const { + const std::optional clip_coverage) const { return true; } @@ -158,8 +158,9 @@ Contents::ClipCoverage ClipRestoreContents::GetClipCoverage( return {.type = ClipCoverage::Type::kRestore, .coverage = std::nullopt}; } -bool ClipRestoreContents::ShouldRender(const Entity& entity, - const Rect& clip_coverage) const { +bool ClipRestoreContents::ShouldRender( + const Entity& entity, + const std::optional clip_coverage) const { return true; } diff --git a/impeller/entity/contents/clip_contents.h b/impeller/entity/contents/clip_contents.h index dc34cf8e195e9..eaf156b018ecd 100644 --- a/impeller/entity/contents/clip_contents.h +++ b/impeller/entity/contents/clip_contents.h @@ -35,7 +35,7 @@ class ClipContents final : public Contents { // |Contents| bool ShouldRender(const Entity& entity, - const Rect& clip_coverage) const override; + const std::optional clip_coverage) const override; // |Contents| bool Render(const ContentContext& renderer, @@ -78,7 +78,7 @@ class ClipRestoreContents final : public Contents { // |Contents| bool ShouldRender(const Entity& entity, - const Rect& clip_coverage) const override; + const std::optional clip_coverage) const override; // |Contents| bool Render(const ContentContext& renderer, diff --git a/impeller/entity/contents/contents.cc b/impeller/entity/contents/contents.cc index b848f72ec45f2..4dfa8c3428c58 100644 --- a/impeller/entity/contents/contents.cc +++ b/impeller/entity/contents/contents.cc @@ -131,7 +131,11 @@ bool Contents::ApplyColorFilter( } bool Contents::ShouldRender(const Entity& entity, - const Rect& clip_coverage) const { + const std::optional clip_coverage) const { +#ifdef IMPELLER_CONTENT_CULLING + if (!clip_coverage.has_value()) { + return false; + } auto coverage = GetCoverage(entity); if (!coverage.has_value()) { return false; @@ -140,6 +144,9 @@ bool Contents::ShouldRender(const Entity& entity, return true; } return clip_coverage.IntersectsWithRect(coverage.value()); +#else + return true; +#endif // IMPELLER_CONTENT_CULLING } void Contents::SetCoverageHint(std::optional coverage_hint) { diff --git a/impeller/entity/contents/contents.h b/impeller/entity/contents/contents.h index 4aac76e0399c6..d1dbb3349beb2 100644 --- a/impeller/entity/contents/contents.h +++ b/impeller/entity/contents/contents.h @@ -123,7 +123,7 @@ class Contents { const std::string& label = "Snapshot") const; virtual bool ShouldRender(const Entity& entity, - const Rect& clip_coverage) const; + const std::optional clip_coverage) const; //---------------------------------------------------------------------------- /// @brief Return the color source's intrinsic size, if available. diff --git a/impeller/entity/entity.cc b/impeller/entity/entity.cc index d61962db8c5a0..24bf0efa84958 100644 --- a/impeller/entity/entity.cc +++ b/impeller/entity/entity.cc @@ -71,14 +71,7 @@ Contents::ClipCoverage Entity::GetClipCoverage( } bool Entity::ShouldRender(const std::optional& clip_coverage) const { - if (!clip_coverage.has_value()) { - return false; - } -#ifdef IMPELLER_CONTENT_CULLING - return contents_->ShouldRender(*this, clip_coverage.value()); -#else - return true; -#endif // IMPELLER_CONTENT_CULLING + return contents_->ShouldRender(*this, clip_coverage); } void Entity::SetContents(std::shared_ptr contents) { From 76184cc2f85e10632cd1bef50a916d19875a6c95 Mon Sep 17 00:00:00 2001 From: jonahwilliams Date: Wed, 6 Dec 2023 09:28:58 -0800 Subject: [PATCH 4/4] adjust so it can be tested still. --- impeller/entity/contents/contents.cc | 6 +----- impeller/entity/entity.cc | 4 ++++ 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/impeller/entity/contents/contents.cc b/impeller/entity/contents/contents.cc index 4dfa8c3428c58..58efcf779815e 100644 --- a/impeller/entity/contents/contents.cc +++ b/impeller/entity/contents/contents.cc @@ -132,7 +132,6 @@ bool Contents::ApplyColorFilter( bool Contents::ShouldRender(const Entity& entity, const std::optional clip_coverage) const { -#ifdef IMPELLER_CONTENT_CULLING if (!clip_coverage.has_value()) { return false; } @@ -143,10 +142,7 @@ bool Contents::ShouldRender(const Entity& entity, if (coverage == Rect::MakeMaximum()) { return true; } - return clip_coverage.IntersectsWithRect(coverage.value()); -#else - return true; -#endif // IMPELLER_CONTENT_CULLING + return clip_coverage->IntersectsWithRect(coverage.value()); } void Contents::SetCoverageHint(std::optional coverage_hint) { diff --git a/impeller/entity/entity.cc b/impeller/entity/entity.cc index 24bf0efa84958..99f74727f04c8 100644 --- a/impeller/entity/entity.cc +++ b/impeller/entity/entity.cc @@ -71,7 +71,11 @@ Contents::ClipCoverage Entity::GetClipCoverage( } bool Entity::ShouldRender(const std::optional& clip_coverage) const { +#ifdef IMPELLER_CONTENT_CULLING return contents_->ShouldRender(*this, clip_coverage); +#else + return true; +#endif // IMPELLER_CONTENT_CULLING } void Entity::SetContents(std::shared_ptr contents) {