Skip to content
This repository was archived by the owner on Feb 25, 2025. It is now read-only.

[Impeller] disable entity culling by default. #48717

Merged
auto-submit merged 4 commits into from
Dec 6, 2023
Merged

[Impeller] disable entity culling by default. #48717

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
bd1cedb
[Impeller] disable entity culling by default.
Dec 6, 2023
6603682
add test.
Dec 6, 2023
7f4628a
dont accidentally cull clips
Dec 6, 2023
76184cc
adjust so it can be tested still.
Dec 6, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 3 additions & 4 deletions impeller/entity/contents/clip_contents.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,9 +62,8 @@ Contents::ClipCoverage ClipContents::GetClipCoverage(
FML_UNREACHABLE();
}

bool ClipContents::ShouldRender(
const Entity& entity,
const std::optional<Rect>& clip_coverage) const {
bool ClipContents::ShouldRender(const Entity& entity,
const std::optional<Rect> clip_coverage) const {
return true;
}

Expand Down Expand Up @@ -161,7 +160,7 @@ Contents::ClipCoverage ClipRestoreContents::GetClipCoverage(

bool ClipRestoreContents::ShouldRender(
const Entity& entity,
const std::optional<Rect>& clip_coverage) const {
const std::optional<Rect> clip_coverage) const {
return true;
}

Expand Down
4 changes: 2 additions & 2 deletions impeller/entity/contents/clip_contents.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@ class ClipContents final : public Contents {

// |Contents|
bool ShouldRender(const Entity& entity,
const std::optional<Rect>& clip_coverage) const override;
const std::optional<Rect> clip_coverage) const override;

// |Contents|
bool Render(const ContentContext& renderer,
Expand Down Expand Up @@ -78,7 +78,7 @@ class ClipRestoreContents final : public Contents {

// |Contents|
bool ShouldRender(const Entity& entity,
const std::optional<Rect>& clip_coverage) const override;
const std::optional<Rect> clip_coverage) const override;

// |Contents|
bool Render(const ContentContext& renderer,
Expand Down
3 changes: 1 addition & 2 deletions impeller/entity/contents/contents.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -131,11 +131,10 @@ bool Contents::ApplyColorFilter(
}

bool Contents::ShouldRender(const Entity& entity,
const std::optional<Rect>& clip_coverage) const {
const std::optional<Rect> clip_coverage) const {
if (!clip_coverage.has_value()) {
return false;
}

auto coverage = GetCoverage(entity);
if (!coverage.has_value()) {
return false;
Expand Down
2 changes: 1 addition & 1 deletion impeller/entity/contents/contents.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -123,7 +123,7 @@ class Contents {
const std::string& label = "Snapshot") const;

virtual bool ShouldRender(const Entity& entity,
const std::optional<Rect>& clip_coverage) const;
const std::optional<Rect> clip_coverage) const;

//----------------------------------------------------------------------------
/// @brief Return the color source's intrinsic size, if available.
Expand Down
4 changes: 4 additions & 0 deletions impeller/entity/entity.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,7 +71,11 @@ Contents::ClipCoverage Entity::GetClipCoverage(
}

bool Entity::ShouldRender(const std::optional<Rect>& clip_coverage) const {
#ifdef IMPELLER_CONTENT_CULLING
Copy link
Member

@gaaclarke gaaclarke Dec 6, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we have a planned end-of-line for this flag? If we decide we don't want ShouldRender, we should just rip it out. It's going to atrophy otherwise.

Copy link
Member

@bdero bdero Dec 6, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We're going to turn this on in impeller-cmake for now (which isn't yet located in the engine repo) since AAOS isn't using DisplayList and this is the only frustum culling mechanism in Impeller.

return contents_->ShouldRender(*this, clip_coverage);
#else
return true;
#endif // IMPELLER_CONTENT_CULLING
}

void Entity::SetContents(std::shared_ptr<Contents> contents) {
Expand Down
14 changes: 14 additions & 0 deletions impeller/entity/entity_unittests.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1605,6 +1605,20 @@ TEST_P(EntityTest, SolidFillShouldRenderIsCorrect) {
}
}

TEST_P(EntityTest, DoesNotCullEntitiesByDefault) {
auto fill = std::make_shared<SolidColorContents>();
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.

Expand Down