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

[Impeller] EntityPass::Clone needs to clone harder #45313

Merged
merged 2 commits into from
Sep 1, 2023
Merged

[Impeller] EntityPass::Clone needs to clone harder #45313

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
fe838c9
[Impeller] make clone ... clone
jonahwilliams Aug 31, 2023
68931d1
++
jonahwilliams Aug 31, 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
36 changes: 36 additions & 0 deletions impeller/aiks/aiks_unittests.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3089,6 +3089,42 @@ TEST_P(AiksTest, CanCanvasDrawPicture) {
ASSERT_TRUE(OpenPlaygroundHere(canvas.EndRecordingAsPicture()));
}

TEST_P(AiksTest, CanCanvasDrawPictureWithAdvancedBlend) {
Canvas subcanvas;
subcanvas.DrawRect(
Rect::MakeLTRB(-100, -50, 100, 50),
{.color = Color::CornflowerBlue(), .blend_mode = BlendMode::kColorDodge});
auto picture = subcanvas.EndRecordingAsPicture();

Canvas canvas;
canvas.Translate({200, 200});
canvas.DrawPicture(picture);

ASSERT_TRUE(OpenPlaygroundHere(canvas.EndRecordingAsPicture()));
}

TEST_P(AiksTest, CanCanvasDrawPictureWithBackdropFilter) {
Canvas subcanvas;
subcanvas.SaveLayer({}, {},
[](const FilterInput::Ref& input,
const Matrix& effect_transform, bool is_subpass) {
return FilterContents::MakeGaussianBlur(
input, Sigma(20.0), Sigma(20.0));
});
auto image = std::make_shared<Image>(CreateTextureForFixture("kalimba.jpg"));
Paint paint;
paint.color = Color::Red();
subcanvas.DrawImage(image, Point::MakeXY(100.0, 100.0), paint);

auto picture = subcanvas.EndRecordingAsPicture();

Canvas canvas;
canvas.Translate({200, 200});
canvas.DrawPicture(picture);

ASSERT_TRUE(OpenPlaygroundHere(canvas.EndRecordingAsPicture()));
}

TEST_P(AiksTest, DrawPictureWithText) {
Canvas subcanvas;
ASSERT_TRUE(RenderTextInCanvasSkia(
Expand Down
4 changes: 2 additions & 2 deletions impeller/aiks/canvas.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -542,9 +542,9 @@ void Canvas::SaveLayer(const Paint& paint,
// Only apply opacity peephole on default blending.
if (paint.blend_mode == BlendMode::kSourceOver) {
new_layer_pass.SetDelegate(
std::make_unique<OpacityPeepholePassDelegate>(paint));
std::make_shared<OpacityPeepholePassDelegate>(paint));
} else {
new_layer_pass.SetDelegate(std::make_unique<PaintPassDelegate>(paint));
new_layer_pass.SetDelegate(std::make_shared<PaintPassDelegate>(paint));
}
}

Expand Down
12 changes: 11 additions & 1 deletion impeller/entity/entity_pass.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,7 @@ EntityPass::EntityPass() = default;

EntityPass::~EntityPass() = default;

void EntityPass::SetDelegate(std::unique_ptr<EntityPassDelegate> delegate) {
void EntityPass::SetDelegate(std::shared_ptr<EntityPassDelegate> delegate) {
if (!delegate) {
return;
}
Expand Down Expand Up @@ -1023,6 +1023,16 @@ std::unique_ptr<EntityPass> EntityPass::Clone() const {

auto pass = std::make_unique<EntityPass>();
pass->SetElements(std::move(new_elements));
pass->backdrop_filter_reads_from_pass_texture_ =
backdrop_filter_reads_from_pass_texture_;
pass->advanced_blend_reads_from_pass_texture_ =
advanced_blend_reads_from_pass_texture_;
Copy link
Member

Choose a reason for hiding this comment

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

(These read-counts are what's important for DrawPicture.)

Copy link
Member Author

Choose a reason for hiding this comment

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

You need the readcounts, the delegate, and the backdrop proc or wonderous will crash.

Copy link
Member

Choose a reason for hiding this comment

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

That shouldn't be the case if the cloned pass is being used for DrawPicture (the elements are copied inline and these values should get discarded).

Copy link
Member

Choose a reason for hiding this comment

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

For the delegate and backdrop proc, that is. The read counts are important.

Copy link
Member Author

Choose a reason for hiding this comment

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

I can confirm that wonderous does not work correctly without copying the backdrop prop and entity delegate. Maybe the entity delegate doesn't need to be copied but it does need to be set to a non-default value.

Let me double check that.

Copy link
Member

@bdero bdero Sep 1, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Ah nevermind, we don't need these for the base pass/inline merge, but we definitely need to copy them for the recursive clone, otherwise Very Bad Things will happen:

engine/impeller/entity/entity_pass.cc

Line 1015 in 494fd7f

new_elements.push_back(subpass->get()->Clone());

pass->backdrop_filter_proc_ = backdrop_filter_proc_;
pass->blend_mode_ = blend_mode_;
pass->delegate_ = delegate_;
// Note: I tried also adding flood clip and bounds limit but one of the
// two caused rendering in wonderous to break. It's 10:51 PM, and I'm
// ready to move on.
Copy link
Member

Choose a reason for hiding this comment

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

Follow-up bug for investigating? The bounds limit might need to be transformed when the pass is applied with DrawPicture.

return pass;
}

Expand Down
4 changes: 2 additions & 2 deletions impeller/entity/entity_pass.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,7 @@ class EntityPass {

~EntityPass();

void SetDelegate(std::unique_ptr<EntityPassDelegate> delgate);
void SetDelegate(std::shared_ptr<EntityPassDelegate> delgate);

/// @brief Set the bounds limit, which is provided by the user when creating
/// a SaveLayer. This is a hint that allows the user to communicate
Expand Down Expand Up @@ -278,7 +278,7 @@ class EntityPass {

BackdropFilterProc backdrop_filter_proc_ = nullptr;

std::unique_ptr<EntityPassDelegate> delegate_ =
std::shared_ptr<EntityPassDelegate> delegate_ =
EntityPassDelegate::MakeDefault();

FML_DISALLOW_COPY_AND_ASSIGN(EntityPass);
Expand Down