From 3ade7372beb0e776450e5aab1cf227f719fc256c Mon Sep 17 00:00:00 2001 From: Brandon DeRosier Date: Mon, 24 Jul 2023 21:15:14 -0700 Subject: [PATCH 1/2] Revert "Revert "[Impeller] Fix MatrixFilter multiplication ordering for subpasses." (#43978)" This reverts commit 176457dac6e9b1506da8a7f9e12c799837d5e06a. --- impeller/aiks/aiks_unittests.cc | 25 +++++++---- .../filters/matrix_filter_contents.cc | 44 ++++++++++++++++--- impeller/entity/entity_pass.cc | 34 +++++++------- 3 files changed, 73 insertions(+), 30 deletions(-) diff --git a/impeller/aiks/aiks_unittests.cc b/impeller/aiks/aiks_unittests.cc index 888617e7fcae7..8f307ea678a7c 100644 --- a/impeller/aiks/aiks_unittests.cc +++ b/impeller/aiks/aiks_unittests.cc @@ -2908,15 +2908,22 @@ TEST_P(AiksTest, DrawPictureWithText) { TEST_P(AiksTest, MatrixBackdropFilter) { Canvas canvas; - canvas.SaveLayer({}, std::nullopt, - [](const FilterInput::Ref& input, - const Matrix& effect_transform, bool is_subpass) { - return FilterContents::MakeMatrixFilter( - input, Matrix::MakeTranslation(Vector2(100, 100)), {}, - Matrix(), true); - }); - canvas.DrawCircle(Point(100, 100), 100, - {.color = Color::Green(), .blend_mode = BlendMode::kPlus}); + canvas.DrawPaint({.color = Color::Black()}); + canvas.SaveLayer({}, std::nullopt); + { + canvas.DrawCircle( + Point(200, 200), 100, + {.color = Color::Green(), .blend_mode = BlendMode::kPlus}); + // Should render a second intersecting circle, offset by 100, 100. + canvas.SaveLayer({}, std::nullopt, + [](const FilterInput::Ref& input, + const Matrix& effect_transform, bool is_subpass) { + return FilterContents::MakeMatrixFilter( + input, Matrix::MakeTranslation(Vector2(100, 100)), + {}, Matrix(), true); + }); + canvas.Restore(); + } canvas.Restore(); ASSERT_TRUE(OpenPlaygroundHere(canvas.EndRecordingAsPicture())); diff --git a/impeller/entity/contents/filters/matrix_filter_contents.cc b/impeller/entity/contents/filters/matrix_filter_contents.cc index b9c9c2424df7d..7737bc9d5d739 100644 --- a/impeller/entity/contents/filters/matrix_filter_contents.cc +++ b/impeller/entity/contents/filters/matrix_filter_contents.cc @@ -34,11 +34,45 @@ std::optional MatrixFilterContents::RenderFilter( return std::nullopt; } - auto& transform = is_subpass_ ? effect_transform : entity.GetTransformation(); - snapshot->transform = transform * // - matrix_ * // - transform.Invert() * // - snapshot->transform; + if (is_subpass_) { + // There are two special quirks with how Matrix filters behave when used as + // subpass backdrop filters: + // + // 1. For subpass backdrop filters, the snapshot transform is always just a + // translation that positions the parent pass texture correctly relative + // to the subpass texture. However, this translation always needs to be + // applied in screen space. + // + // Since we know the snapshot transform will always have an identity + // basis in this case, we safely reverse the order and apply the filter's + // matrix within the snapshot transform space. + // + // 2. The filter's matrix needs to be applied within the space defined by + // the scene's current transformation matrix (CTM). For example: If the + // CTM is scaled up, then translations applied by the matrix should be + // magnified accordingly. + // + // To accomplish this, we sandwitch the filter's matrix within the CTM in + // both cases. But notice that for the subpass backdrop filter case, we + // use the "effect transform" instead of the Entity's transform! + // + // That's because in the subpass backdrop filter case, the Entity's + // transform isn't actually the captured CTM of the scene like it usually + // is; instead, it's just a screen space translation that offsets the + // backdrop texture (as mentioned above). And so we sneak the subpass's + // captured CTM in through the effect transform. + // + + snapshot->transform = snapshot->transform * // + effect_transform * // + matrix_ * // + effect_transform.Invert(); + } else { + snapshot->transform = entity.GetTransformation() * // + matrix_ * // + entity.GetTransformation().Invert() * // + snapshot->transform; + } snapshot->sampler_descriptor = sampler_descriptor_; return Entity::FromSnapshot(snapshot, entity.GetBlendMode(), entity.GetStencilDepth()); diff --git a/impeller/entity/entity_pass.cc b/impeller/entity/entity_pass.cc index bbd20ec6efc3b..1d32e1d67da6f 100644 --- a/impeller/entity/entity_pass.cc +++ b/impeller/entity/entity_pass.cc @@ -465,12 +465,12 @@ EntityPass::EntityResult EntityPass::GetEntityForElement( return EntityPass::EntityResult::Skip(); } - std::shared_ptr backdrop_filter_contents = nullptr; + std::shared_ptr subpass_backdrop_filter_contents = nullptr; if (subpass->backdrop_filter_proc_) { auto texture = pass_context.GetTexture(); // Render the backdrop texture before any of the pass elements. const auto& proc = subpass->backdrop_filter_proc_; - backdrop_filter_contents = + subpass_backdrop_filter_contents = proc(FilterInput::Make(std::move(texture)), subpass->xformation_, /*is_subpass*/ true); @@ -507,9 +507,10 @@ EntityPass::EntityResult EntityPass::GetEntityForElement( return EntityPass::EntityResult::Skip(); } - auto subpass_coverage = (subpass->flood_clip_ || backdrop_filter_contents) - ? coverage_limit - : GetSubpassCoverage(*subpass, coverage_limit); + auto subpass_coverage = + (subpass->flood_clip_ || subpass_backdrop_filter_contents) + ? coverage_limit + : GetSubpassCoverage(*subpass, coverage_limit); if (!subpass_coverage.has_value()) { return EntityPass::EntityResult::Skip(); } @@ -532,17 +533,18 @@ EntityPass::EntityResult EntityPass::GetEntityForElement( // Stencil textures aren't shared between EntityPasses (as much of the // time they are transient). - if (!subpass->OnRender(renderer, // renderer - root_pass_size, // root_pass_size - subpass_target, // pass_target - subpass_coverage->origin, // global_pass_position - subpass_coverage->origin - - global_pass_position, // local_pass_position - ++pass_depth, // pass_depth - stencil_coverage_stack, // stencil_coverage_stack - subpass->stencil_depth_, // stencil_depth_floor - backdrop_filter_contents // backdrop_filter_contents - )) { + if (!subpass->OnRender( + renderer, // renderer + root_pass_size, // root_pass_size + subpass_target, // pass_target + subpass_coverage->origin, // global_pass_position + subpass_coverage->origin - + global_pass_position, // local_pass_position + ++pass_depth, // pass_depth + stencil_coverage_stack, // stencil_coverage_stack + subpass->stencil_depth_, // stencil_depth_floor + subpass_backdrop_filter_contents // backdrop_filter_contents + )) { // Validation error messages are triggered for all `OnRender()` failure // cases. return EntityPass::EntityResult::Failure(); From b46351a9fde0a67ee60943deb78b559671c4c403 Mon Sep 17 00:00:00 2001 From: Brandon DeRosier Date: Mon, 24 Jul 2023 21:56:14 -0700 Subject: [PATCH 2/2] Fix matrix filter; make the golden much better --- impeller/aiks/aiks_unittests.cc | 17 ++++-- .../filters/matrix_filter_contents.cc | 59 +++++++------------ 2 files changed, 32 insertions(+), 44 deletions(-) diff --git a/impeller/aiks/aiks_unittests.cc b/impeller/aiks/aiks_unittests.cc index 8f307ea678a7c..34dd2b6dd440d 100644 --- a/impeller/aiks/aiks_unittests.cc +++ b/impeller/aiks/aiks_unittests.cc @@ -2911,16 +2911,21 @@ TEST_P(AiksTest, MatrixBackdropFilter) { canvas.DrawPaint({.color = Color::Black()}); canvas.SaveLayer({}, std::nullopt); { - canvas.DrawCircle( - Point(200, 200), 100, - {.color = Color::Green(), .blend_mode = BlendMode::kPlus}); - // Should render a second intersecting circle, offset by 100, 100. + canvas.DrawCircle(Point(200, 200), 100, + {.color = Color::Green().WithAlpha(0.5), + .blend_mode = BlendMode::kPlus}); + // Should render a second circle, centered on the bottom-right-most edge of + // the circle. canvas.SaveLayer({}, std::nullopt, [](const FilterInput::Ref& input, const Matrix& effect_transform, bool is_subpass) { + Matrix matrix = + Matrix::MakeTranslation(Vector2(1, 1) * + (100 + 100 * k1OverSqrt2)) * + Matrix::MakeScale(Vector2(1, 1) * 0.2) * + Matrix::MakeTranslation(Vector2(-100, -100)); return FilterContents::MakeMatrixFilter( - input, Matrix::MakeTranslation(Vector2(100, 100)), - {}, Matrix(), true); + input, matrix, {}, Matrix(), true); }); canvas.Restore(); } diff --git a/impeller/entity/contents/filters/matrix_filter_contents.cc b/impeller/entity/contents/filters/matrix_filter_contents.cc index 7737bc9d5d739..81d1b51beaec9 100644 --- a/impeller/entity/contents/filters/matrix_filter_contents.cc +++ b/impeller/entity/contents/filters/matrix_filter_contents.cc @@ -34,45 +34,28 @@ std::optional MatrixFilterContents::RenderFilter( return std::nullopt; } - if (is_subpass_) { - // There are two special quirks with how Matrix filters behave when used as - // subpass backdrop filters: - // - // 1. For subpass backdrop filters, the snapshot transform is always just a - // translation that positions the parent pass texture correctly relative - // to the subpass texture. However, this translation always needs to be - // applied in screen space. - // - // Since we know the snapshot transform will always have an identity - // basis in this case, we safely reverse the order and apply the filter's - // matrix within the snapshot transform space. - // - // 2. The filter's matrix needs to be applied within the space defined by - // the scene's current transformation matrix (CTM). For example: If the - // CTM is scaled up, then translations applied by the matrix should be - // magnified accordingly. - // - // To accomplish this, we sandwitch the filter's matrix within the CTM in - // both cases. But notice that for the subpass backdrop filter case, we - // use the "effect transform" instead of the Entity's transform! - // - // That's because in the subpass backdrop filter case, the Entity's - // transform isn't actually the captured CTM of the scene like it usually - // is; instead, it's just a screen space translation that offsets the - // backdrop texture (as mentioned above). And so we sneak the subpass's - // captured CTM in through the effect transform. - // + // The filter's matrix needs to be applied within the space defined by the + // scene's current transformation matrix (CTM). For example: If the CTM is + // scaled up, then translations applied by the matrix should be magnified + // accordingly. + // + // To accomplish this, we sandwitch the filter's matrix within the CTM in both + // cases. But notice that for the subpass backdrop filter case, we use the + // "effect transform" instead of the Entity's transform! + // + // That's because in the subpass backdrop filter case, the Entity's transform + // isn't actually the captured CTM of the scene like it usually is; instead, + // it's just a screen space translation that offsets the backdrop texture (as + // mentioned above). And so we sneak the subpass's captured CTM in through the + // effect transform. + + auto transform = is_subpass_ ? effect_transform.Basis() + : entity.GetTransformation().Basis(); + snapshot->transform = transform * // + matrix_ * // + transform.Invert() * // + snapshot->transform; - snapshot->transform = snapshot->transform * // - effect_transform * // - matrix_ * // - effect_transform.Invert(); - } else { - snapshot->transform = entity.GetTransformation() * // - matrix_ * // - entity.GetTransformation().Invert() * // - snapshot->transform; - } snapshot->sampler_descriptor = sampler_descriptor_; return Entity::FromSnapshot(snapshot, entity.GetBlendMode(), entity.GetStencilDepth());