From e1d2adc77c8e94bc0ac28d392c059c8c0b6cf42b Mon Sep 17 00:00:00 2001 From: gaaclarke <30870216+gaaclarke@users.noreply.github.com> Date: Fri, 17 May 2024 17:43:35 -0700 Subject: [PATCH 1/5] Fixes MatrixFilterContents rendering/coverage (#52880) fixes: https://github.com/flutter/flutter/issues/147807 relands https://github.com/flutter/engine/pull/43943 (with fixes that hopefully avoid it being reverted again) [C++, Objective-C, Java style guides]: https://github.com/flutter/engine/blob/main/CONTRIBUTING.md#style --- ci/licenses_golden/excluded_files | 1 + impeller/aiks/aiks_unittests.cc | 34 ++- impeller/aiks/canvas.cc | 6 +- impeller/aiks/experimental_canvas.cc | 6 +- impeller/aiks/paint.cc | 4 +- impeller/aiks/paint_pass_delegate.cc | 4 +- impeller/display_list/dl_golden_unittests.cc | 52 +++++ impeller/entity/BUILD.gn | 1 + .../filters/matrix_filter_contents.cc | 103 ++++++--- .../matrix_filter_contents_unittests.cc | 218 ++++++++++++++++++ impeller/entity/entity.h | 3 +- impeller/entity/entity_pass.cc | 8 +- testing/impeller_golden_tests_output.txt | 3 + 13 files changed, 387 insertions(+), 56 deletions(-) create mode 100644 impeller/entity/contents/filters/matrix_filter_contents_unittests.cc diff --git a/ci/licenses_golden/excluded_files b/ci/licenses_golden/excluded_files index 37cc6dec12598..62ff8bc3471c8 100644 --- a/ci/licenses_golden/excluded_files +++ b/ci/licenses_golden/excluded_files @@ -147,6 +147,7 @@ ../../../flutter/impeller/entity/contents/clip_contents_unittests.cc ../../../flutter/impeller/entity/contents/filters/gaussian_blur_filter_contents_unittests.cc ../../../flutter/impeller/entity/contents/filters/inputs/filter_input_unittests.cc +../../../flutter/impeller/entity/contents/filters/matrix_filter_contents_unittests.cc ../../../flutter/impeller/entity/contents/host_buffer_unittests.cc ../../../flutter/impeller/entity/contents/test ../../../flutter/impeller/entity/contents/tiled_texture_contents_unittests.cc diff --git a/impeller/aiks/aiks_unittests.cc b/impeller/aiks/aiks_unittests.cc index 12c5414ae1b70..a99bff89484b6 100644 --- a/impeller/aiks/aiks_unittests.cc +++ b/impeller/aiks/aiks_unittests.cc @@ -2772,19 +2772,29 @@ TEST_P(AiksTest, VerticesGeometryColorUVPositionDataAdvancedBlend) { } TEST_P(AiksTest, MatrixImageFilterMagnify) { - Canvas canvas; - canvas.Scale(GetContentScale()); - auto image = std::make_shared(CreateTextureForFixture("airplane.jpg")); - canvas.Translate({600, -200}); - canvas.SaveLayer({ - .image_filter = std::make_shared( - Matrix::MakeScale({2, 2, 2}), SamplerDescriptor{}), - }); - canvas.DrawImage(image, {0, 0}, - Paint{.color = Color::White().WithAlpha(0.5)}); - canvas.Restore(); + Scalar scale = 2.0; + auto callback = [&](AiksContext& renderer) -> std::optional { + if (AiksTest::ImGuiBegin("Controls", nullptr, + ImGuiWindowFlags_AlwaysAutoResize)) { + ImGui::SliderFloat("Scale", &scale, 1, 2); + ImGui::End(); + } + Canvas canvas; + canvas.Scale(GetContentScale()); + auto image = + std::make_shared(CreateTextureForFixture("airplane.jpg")); + canvas.Translate({600, -200}); + canvas.SaveLayer({ + .image_filter = std::make_shared( + Matrix::MakeScale({scale, scale, 1}), SamplerDescriptor{}), + }); + canvas.DrawImage(image, {0, 0}, + Paint{.color = Color::White().WithAlpha(0.5)}); + canvas.Restore(); + return canvas.EndRecordingAsPicture(); + }; - ASSERT_TRUE(OpenPlaygroundHere(canvas.EndRecordingAsPicture())); + ASSERT_TRUE(OpenPlaygroundHere(callback)); } // Render a white circle at the top left corner of the screen. diff --git a/impeller/aiks/canvas.cc b/impeller/aiks/canvas.cc index 4692d1109884d..ace34951073a8 100644 --- a/impeller/aiks/canvas.cc +++ b/impeller/aiks/canvas.cc @@ -225,7 +225,7 @@ void Canvas::Save(bool create_subpass, entry.cull_rect = transform_stack_.back().cull_rect; entry.clip_height = transform_stack_.back().clip_height; if (create_subpass) { - entry.rendering_mode = Entity::RenderingMode::kSubpass; + entry.rendering_mode = Entity::RenderingMode::kBackdropSubpass; auto subpass = std::make_unique(); if (backdrop_filter) { EntityPass::BackdropFilterProc backdrop_filter_proc = @@ -261,7 +261,9 @@ bool Canvas::Restore() { current_pass_->PopClips(num_clips, current_depth_); if (transform_stack_.back().rendering_mode == - Entity::RenderingMode::kSubpass) { + Entity::RenderingMode::kBackdropSubpass || + transform_stack_.back().rendering_mode == + Entity::RenderingMode::kImageFilterSubpass) { current_pass_->SetClipDepth(++current_depth_); current_pass_ = GetCurrentPass().GetSuperpass(); FML_DCHECK(current_pass_); diff --git a/impeller/aiks/experimental_canvas.cc b/impeller/aiks/experimental_canvas.cc index 7837b0ca9fb90..b8df394423dfc 100644 --- a/impeller/aiks/experimental_canvas.cc +++ b/impeller/aiks/experimental_canvas.cc @@ -230,7 +230,7 @@ void ExperimentalCanvas::SaveLayer( << entry.clip_depth << " <=? " << transform_stack_.back().clip_depth << " after allocating " << total_content_depth; entry.clip_height = transform_stack_.back().clip_height; - entry.rendering_mode = Entity::RenderingMode::kSubpass; + entry.rendering_mode = Entity::RenderingMode::kBackdropSubpass; transform_stack_.emplace_back(entry); auto inline_pass = std::make_unique( @@ -272,7 +272,9 @@ bool ExperimentalCanvas::Restore() { current_depth_ = transform_stack_.back().clip_depth; if (transform_stack_.back().rendering_mode == - Entity::RenderingMode::kSubpass) { + Entity::RenderingMode::kBackdropSubpass || + transform_stack_.back().rendering_mode == + Entity::RenderingMode::kImageFilterSubpass) { auto inline_pass = std::move(inline_pass_contexts_.back()); SaveLayerState save_layer_state = save_layer_state_.back(); diff --git a/impeller/aiks/paint.cc b/impeller/aiks/paint.cc index 1efb587601e0d..32252b386a5b9 100644 --- a/impeller/aiks/paint.cc +++ b/impeller/aiks/paint.cc @@ -67,8 +67,8 @@ std::shared_ptr Paint::WithFilters( std::shared_ptr Paint::WithFiltersForSubpassTarget( std::shared_ptr input, const Matrix& effect_transform) const { - auto image_filter = - WithImageFilter(input, effect_transform, Entity::RenderingMode::kSubpass); + auto image_filter = WithImageFilter( + input, effect_transform, Entity::RenderingMode::kImageFilterSubpass); if (image_filter) { input = image_filter; } diff --git a/impeller/aiks/paint_pass_delegate.cc b/impeller/aiks/paint_pass_delegate.cc index 34610488f5a24..2879075e73063 100644 --- a/impeller/aiks/paint_pass_delegate.cc +++ b/impeller/aiks/paint_pass_delegate.cc @@ -51,7 +51,7 @@ std::shared_ptr PaintPassDelegate::WithImageFilter( const FilterInput::Variant& input, const Matrix& effect_transform) const { return paint_.WithImageFilter(input, effect_transform, - Entity::RenderingMode::kSubpass); + Entity::RenderingMode::kImageFilterSubpass); } /// OpacityPeepholePassDelegate @@ -153,7 +153,7 @@ std::shared_ptr OpacityPeepholePassDelegate::WithImageFilter( const FilterInput::Variant& input, const Matrix& effect_transform) const { return paint_.WithImageFilter(input, effect_transform, - Entity::RenderingMode::kSubpass); + Entity::RenderingMode::kImageFilterSubpass); } } // namespace impeller diff --git a/impeller/display_list/dl_golden_unittests.cc b/impeller/display_list/dl_golden_unittests.cc index 81f9e70b33d04..89f04aa4b55ea 100644 --- a/impeller/display_list/dl_golden_unittests.cc +++ b/impeller/display_list/dl_golden_unittests.cc @@ -13,6 +13,7 @@ namespace testing { using impeller::PlaygroundBackend; using impeller::PlaygroundTest; +using impeller::Point; INSTANTIATE_PLAYGROUND_SUITE(DlGoldenTest); @@ -48,5 +49,56 @@ TEST_P(DlGoldenTest, CanRenderImage) { ASSERT_TRUE(OpenPlaygroundHere(builder.Build())); } +// Asserts that subpass rendering of MatrixImageFilters works. +// https://github.com/flutter/flutter/issues/147807 +TEST_P(DlGoldenTest, Bug147807) { + Point content_scale = GetContentScale(); + auto draw = [content_scale](DlCanvas* canvas, + const std::vector>& images) { + canvas->Transform2DAffine(content_scale.x, 0, 0, 0, content_scale.y, 0); + DlPaint paint; + paint.setColor(DlColor(0xfffef7ff)); + canvas->DrawRect(SkRect::MakeLTRB(0, 0, 375, 667), paint); + paint.setColor(DlColor(0xffff9800)); + canvas->DrawRect(SkRect::MakeLTRB(0, 0, 187.5, 333.5), paint); + paint.setColor(DlColor(0xff9c27b0)); + canvas->DrawRect(SkRect::MakeLTRB(187.5, 0, 375, 333.5), paint); + paint.setColor(DlColor(0xff4caf50)); + canvas->DrawRect(SkRect::MakeLTRB(0, 333.5, 187.5, 667), paint); + paint.setColor(DlColor(0xfff44336)); + canvas->DrawRect(SkRect::MakeLTRB(187.5, 333.5, 375, 667), paint); + + canvas->Save(); + { + canvas->ClipRRect( + SkRRect::MakeOval(SkRect::MakeLTRB(201.25, 10, 361.25, 170)), + DlCanvas::ClipOp::kIntersect, true); + SkRect save_layer_bounds = SkRect::MakeLTRB(201.25, 10, 361.25, 170); + DlMatrixImageFilter backdrop(SkMatrix::MakeAll(3, 0, -280, // + 0, 3, -920, // + 0, 0, 1), + DlImageSampling::kLinear); + canvas->SaveLayer(&save_layer_bounds, /*paint=*/nullptr, &backdrop); + { + canvas->Translate(201.25, 10); + auto paint = DlPaint() + .setAntiAlias(true) + .setColor(DlColor(0xff2196f3)) + .setStrokeWidth(5) + .setDrawStyle(DlDrawStyle::kStroke); + canvas->DrawCircle(SkPoint::Make(80, 80), 80, paint); + } + canvas->Restore(); + } + canvas->Restore(); + }; + + DisplayListBuilder builder; + std::vector> images; + draw(&builder, images); + + ASSERT_TRUE(OpenPlaygroundHere(builder.Build())); +} + } // namespace testing } // namespace flutter diff --git a/impeller/entity/BUILD.gn b/impeller/entity/BUILD.gn index c529d46684393..fbf419da1abf5 100644 --- a/impeller/entity/BUILD.gn +++ b/impeller/entity/BUILD.gn @@ -241,6 +241,7 @@ impeller_component("entity_unittests") { "contents/clip_contents_unittests.cc", "contents/filters/gaussian_blur_filter_contents_unittests.cc", "contents/filters/inputs/filter_input_unittests.cc", + "contents/filters/matrix_filter_contents_unittests.cc", "contents/host_buffer_unittests.cc", "contents/tiled_texture_contents_unittests.cc", "entity_pass_target_unittests.cc", diff --git a/impeller/entity/contents/filters/matrix_filter_contents.cc b/impeller/entity/contents/filters/matrix_filter_contents.cc index 9681e8a6d6ad5..1c73cd37d4dbd 100644 --- a/impeller/entity/contents/filters/matrix_filter_contents.cc +++ b/impeller/entity/contents/filters/matrix_filter_contents.cc @@ -28,6 +28,26 @@ void MatrixFilterContents::SetSamplerDescriptor(SamplerDescriptor desc) { sampler_descriptor_ = std::move(desc); } +namespace { +Matrix CalculateSubpassTransform(const Matrix& snapshot_transform, + const Matrix& effect_transform, + const Matrix& matrix, + Entity::RenderingMode rendering_mode) { + if (rendering_mode == Entity::RenderingMode::kBackdropSubpass) { + return snapshot_transform * // + effect_transform * // + matrix * // + effect_transform.Invert(); + } else { + FML_DCHECK(rendering_mode == Entity::RenderingMode::kImageFilterSubpass); + return effect_transform * // + matrix * // + effect_transform.Invert() * // + snapshot_transform; + } +} +} // namespace + std::optional MatrixFilterContents::RenderFilter( const FilterInput::Vector& inputs, const ContentContext& renderer, @@ -40,29 +60,43 @@ std::optional MatrixFilterContents::RenderFilter( return std::nullopt; } - // The filter's matrix needs to be applied within the space defined by the - // scene's current transform matrix (CTM). For example: If the CTM is - // scaled up, then translations applied by the matrix should be magnified - // accordingly. - // - // To accomplish this, we sandwich 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 = rendering_mode_ == Entity::RenderingMode::kSubpass - ? effect_transform - : entity.GetTransform(); - snapshot->transform = transform * // - matrix_ * // - transform.Invert() * // - snapshot->transform; - + if (rendering_mode_ == Entity::RenderingMode::kImageFilterSubpass || + rendering_mode_ == Entity::RenderingMode::kBackdropSubpass) { + // 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 = CalculateSubpassTransform( + snapshot->transform, effect_transform, matrix_, rendering_mode_); + } else { + snapshot->transform = entity.GetTransform() * // + matrix_ * // + entity.GetTransform().Invert() * // + snapshot->transform; + } snapshot->sampler_descriptor = sampler_descriptor_; if (!snapshot.has_value()) { return std::nullopt; @@ -91,17 +125,24 @@ std::optional MatrixFilterContents::GetFilterCoverage( return std::nullopt; } - auto coverage = inputs[0]->GetCoverage(entity); + std::optional coverage = inputs[0]->GetCoverage(entity); if (!coverage.has_value()) { return std::nullopt; } - auto& m = rendering_mode_ == Entity::RenderingMode::kSubpass - ? effect_transform - : inputs[0]->GetTransform(entity); - auto transform = m * // - matrix_ * // - m.Invert(); // - return coverage->TransformBounds(transform); + + Matrix input_transform = inputs[0]->GetTransform(entity); + if (rendering_mode_ == Entity::RenderingMode::kImageFilterSubpass || + rendering_mode_ == Entity::RenderingMode::kBackdropSubpass) { + Rect coverage_bounds = coverage->TransformBounds(input_transform.Invert()); + Matrix transform = CalculateSubpassTransform( + input_transform, effect_transform, matrix_, rendering_mode_); + return coverage_bounds.TransformBounds(transform); + } else { + Matrix transform = input_transform * // + matrix_ * // + input_transform.Invert(); // + return coverage->TransformBounds(transform); + } } } // namespace impeller diff --git a/impeller/entity/contents/filters/matrix_filter_contents_unittests.cc b/impeller/entity/contents/filters/matrix_filter_contents_unittests.cc new file mode 100644 index 0000000000000..3c2a3afd41ccd --- /dev/null +++ b/impeller/entity/contents/filters/matrix_filter_contents_unittests.cc @@ -0,0 +1,218 @@ +// Copyright 2013 The Flutter Authors. All rights reserved. +// Use of this source code is governed by a BSD-style license that can be +// found in the LICENSE file. + +#include "flutter/testing/testing.h" +#include "gmock/gmock.h" +#include "impeller/entity/contents/filters/matrix_filter_contents.h" +#include "impeller/entity/entity_playground.h" +#include "impeller/geometry/geometry_asserts.h" + +namespace impeller { +namespace testing { + +class MatrixFilterContentsTest : public EntityPlayground { + public: + /// Create a texture that has been cleared to transparent black. + std::shared_ptr MakeTexture(ISize size) { + std::shared_ptr command_buffer = + GetContentContext()->GetContext()->CreateCommandBuffer(); + if (!command_buffer) { + return nullptr; + } + + auto render_target = GetContentContext()->MakeSubpass( + "Clear Subpass", size, command_buffer, + [](const ContentContext&, RenderPass&) { return true; }); + + if (!GetContentContext() + ->GetContext() + ->GetCommandQueue() + ->Submit(/*buffers=*/{command_buffer}) + .ok()) { + return nullptr; + } + + if (render_target.ok()) { + return render_target.value().GetRenderTargetTexture(); + } + return nullptr; + } +}; + +INSTANTIATE_PLAYGROUND_SUITE(MatrixFilterContentsTest); + +TEST(MatrixFilterContentsTest, Create) { + MatrixFilterContents contents; + EXPECT_TRUE(contents.IsTranslationOnly()); +} + +TEST(MatrixFilterContentsTest, CoverageEmpty) { + MatrixFilterContents contents; + FilterInput::Vector inputs = {}; + Entity entity; + std::optional coverage = + contents.GetFilterCoverage(inputs, entity, /*effect_transform=*/Matrix()); + ASSERT_FALSE(coverage.has_value()); +} + +TEST(MatrixFilterContentsTest, CoverageSimple) { + MatrixFilterContents contents; + FilterInput::Vector inputs = { + FilterInput::Make(Rect::MakeLTRB(10, 10, 110, 110))}; + Entity entity; + std::optional coverage = + contents.GetFilterCoverage(inputs, entity, /*effect_transform=*/Matrix()); + + ASSERT_EQ(coverage, Rect::MakeLTRB(10, 10, 110, 110)); +} + +TEST(MatrixFilterContentsTest, Coverage2x) { + MatrixFilterContents contents; + contents.SetMatrix(Matrix::MakeScale({2.0, 2.0, 1.0})); + FilterInput::Vector inputs = { + FilterInput::Make(Rect::MakeXYWH(10, 10, 100, 100))}; + Entity entity; + std::optional coverage = + contents.GetFilterCoverage(inputs, entity, /*effect_transform=*/Matrix()); + + ASSERT_EQ(coverage, Rect::MakeXYWH(20, 20, 200, 200)); +} + +TEST(MatrixFilterContentsTest, Coverage2xEffect) { + MatrixFilterContents contents; + FilterInput::Vector inputs = { + FilterInput::Make(Rect::MakeXYWH(10, 10, 100, 100))}; + Entity entity; + std::optional coverage = contents.GetFilterCoverage( + inputs, entity, /*effect_transform=*/Matrix::MakeScale({2.0, 2.0, 1.0})); + + ASSERT_EQ(coverage, Rect::MakeXYWH(10, 10, 100, 100)); +} + +namespace { +void expectRenderCoverageEqual(const std::optional& result, + const std::optional contents_coverage, + const Rect& expected) { + EXPECT_TRUE(result.has_value()); + if (result.has_value()) { + EXPECT_EQ(result.value().GetBlendMode(), BlendMode::kSourceOver); + std::optional result_coverage = result.value().GetCoverage(); + EXPECT_TRUE(result_coverage.has_value()); + EXPECT_TRUE(contents_coverage.has_value()); + if (result_coverage.has_value() && contents_coverage.has_value()) { + EXPECT_TRUE(RectNear(contents_coverage.value(), expected)); + EXPECT_TRUE(RectNear(result_coverage.value(), expected)); + } + } +} +} // namespace + +TEST_P(MatrixFilterContentsTest, RenderCoverageMatchesGetCoverageIdentity) { + std::shared_ptr texture = MakeTexture(ISize(100, 100)); + MatrixFilterContents contents; + contents.SetInputs({FilterInput::Make(texture)}); + + Entity entity; + entity.SetTransform(Matrix::MakeTranslation({100, 200, 0})); + + std::shared_ptr renderer = GetContentContext(); + std::optional result = + contents.GetEntity(*renderer, entity, /*coverage_hint=*/{}); + expectRenderCoverageEqual(result, contents.GetCoverage(entity), + Rect::MakeXYWH(100, 200, 100, 100)); +} + +TEST_P(MatrixFilterContentsTest, RenderCoverageMatchesGetCoverageTranslate) { + std::shared_ptr texture = MakeTexture(ISize(100, 100)); + MatrixFilterContents contents; + contents.SetInputs({FilterInput::Make(texture)}); + contents.SetMatrix(Matrix::MakeTranslation({50, 100, 0})); + contents.SetEffectTransform(Matrix::MakeScale({2, 2, 1})); + + Entity entity; + entity.SetTransform(Matrix::MakeTranslation({100, 200, 0})); + + std::shared_ptr renderer = GetContentContext(); + std::optional result = + contents.GetEntity(*renderer, entity, /*coverage_hint=*/{}); + expectRenderCoverageEqual(result, contents.GetCoverage(entity), + Rect::MakeXYWH(150, 300, 100, 100)); +} + +TEST_P(MatrixFilterContentsTest, + RenderCoverageMatchesGetCoverageBackdropSubpassTranslate) { + std::shared_ptr texture = MakeTexture(ISize(100, 100)); + MatrixFilterContents contents; + contents.SetInputs({FilterInput::Make(texture)}); + contents.SetMatrix(Matrix::MakeTranslation({50, 100, 0})); + contents.SetEffectTransform(Matrix::MakeScale({2, 2, 1})); + contents.SetRenderingMode(Entity::RenderingMode::kBackdropSubpass); + + Entity entity; + entity.SetTransform(Matrix::MakeTranslation({100, 200, 0})); + + std::shared_ptr renderer = GetContentContext(); + std::optional result = + contents.GetEntity(*renderer, entity, /*coverage_hint=*/{}); + expectRenderCoverageEqual(result, contents.GetCoverage(entity), + Rect::MakeXYWH(200, 400, 100, 100)); +} + +TEST_P(MatrixFilterContentsTest, RenderCoverageMatchesGetCoverageScale) { + std::shared_ptr texture = MakeTexture(ISize(100, 100)); + MatrixFilterContents contents; + contents.SetInputs({FilterInput::Make(texture)}); + contents.SetMatrix(Matrix::MakeScale({3, 3, 1})); + contents.SetEffectTransform(Matrix::MakeScale({2, 2, 1})); + + Entity entity; + entity.SetTransform(Matrix::MakeTranslation({100, 200, 0})); + + std::shared_ptr renderer = GetContentContext(); + std::optional result = + contents.GetEntity(*renderer, entity, /*coverage_hint=*/{}); + expectRenderCoverageEqual(result, contents.GetCoverage(entity), + Rect::MakeXYWH(100, 200, 300, 300)); +} + +TEST_P(MatrixFilterContentsTest, + RenderCoverageMatchesGetCoverageBackdropSubpassScale) { + std::shared_ptr texture = MakeTexture(ISize(100, 100)); + MatrixFilterContents contents; + contents.SetInputs({FilterInput::Make(texture)}); + contents.SetMatrix(Matrix::MakeScale({3, 3, 1})); + contents.SetEffectTransform(Matrix::MakeScale({2, 2, 1})); + contents.SetRenderingMode(Entity::RenderingMode::kBackdropSubpass); + + Entity entity; + entity.SetTransform(Matrix::MakeTranslation({100, 200, 0})); + + std::shared_ptr renderer = GetContentContext(); + std::optional result = + contents.GetEntity(*renderer, entity, /*coverage_hint=*/{}); + expectRenderCoverageEqual(result, contents.GetCoverage(entity), + Rect::MakeXYWH(100, 200, 300, 300)); +} + +TEST_P(MatrixFilterContentsTest, + RenderCoverageMatchesGetCoverageImageFilterSubpassScale) { + std::shared_ptr texture = MakeTexture(ISize(100, 100)); + MatrixFilterContents contents; + contents.SetInputs({FilterInput::Make(texture)}); + contents.SetMatrix(Matrix::MakeScale({3, 3, 1})); + contents.SetEffectTransform(Matrix::MakeScale({2, 2, 1})); + contents.SetRenderingMode(Entity::RenderingMode::kImageFilterSubpass); + + Entity entity; + entity.SetTransform(Matrix::MakeTranslation({100, 200, 0})); + + std::shared_ptr renderer = GetContentContext(); + std::optional result = + contents.GetEntity(*renderer, entity, /*coverage_hint=*/{}); + expectRenderCoverageEqual(result, contents.GetCoverage(entity), + Rect::MakeXYWH(300, 600, 300, 300)); +} + +} // namespace testing +} // namespace impeller diff --git a/impeller/entity/entity.h b/impeller/entity/entity.h index d2d4eb9596a64..293a17867661e 100644 --- a/impeller/entity/entity.h +++ b/impeller/entity/entity.h @@ -33,7 +33,8 @@ class Entity { /// rather than local space, and so some filters (namely, /// MatrixFilterContents) need to interpret the given EffectTransform as the /// current transform matrix. - kSubpass, + kBackdropSubpass, + kImageFilterSubpass, }; /// An enum to define how to repeat, fold, or omit colors outside of the diff --git a/impeller/entity/entity_pass.cc b/impeller/entity/entity_pass.cc index 596f4412daf84..ee28a0dd4f68e 100644 --- a/impeller/entity/entity_pass.cc +++ b/impeller/entity/entity_pass.cc @@ -185,7 +185,7 @@ std::optional EntityPass::GetElementsCoverage( std::shared_ptr backdrop_filter = subpass.backdrop_filter_proc_( FilterInput::Make(accumulated_coverage.value()), - subpass.transform_, Entity::RenderingMode::kSubpass); + subpass.transform_, Entity::RenderingMode::kBackdropSubpass); if (backdrop_filter) { auto backdrop_coverage = backdrop_filter->GetCoverage({}); unfiltered_coverage = @@ -585,9 +585,9 @@ EntityPass::EntityResult EntityPass::GetEntityForElement( auto texture = pass_context.GetTexture(); // Render the backdrop texture before any of the pass elements. const auto& proc = subpass->backdrop_filter_proc_; - subpass_backdrop_filter_contents = - proc(FilterInput::Make(std::move(texture)), - subpass->transform_.Basis(), Entity::RenderingMode::kSubpass); + subpass_backdrop_filter_contents = proc( + FilterInput::Make(std::move(texture)), subpass->transform_.Basis(), + Entity::RenderingMode::kBackdropSubpass); // If the very first thing we render in this EntityPass is a subpass that // happens to have a backdrop filter, than that backdrop filter will end diff --git a/testing/impeller_golden_tests_output.txt b/testing/impeller_golden_tests_output.txt index 1ec4f8151e373..c291136019a77 100644 --- a/testing/impeller_golden_tests_output.txt +++ b/testing/impeller_golden_tests_output.txt @@ -777,6 +777,9 @@ impeller_Play_AiksTest_VerticesGeometryUVPositionDataWithTranslate_Vulkan.png impeller_Play_AiksTest_VerticesGeometryUVPositionData_Metal.png impeller_Play_AiksTest_VerticesGeometryUVPositionData_OpenGLES.png impeller_Play_AiksTest_VerticesGeometryUVPositionData_Vulkan.png +impeller_Play_DlGoldenTest_Bug147807_Metal.png +impeller_Play_DlGoldenTest_Bug147807_OpenGLES.png +impeller_Play_DlGoldenTest_Bug147807_Vulkan.png impeller_Play_DlGoldenTest_CanDrawPaint_Metal.png impeller_Play_DlGoldenTest_CanDrawPaint_OpenGLES.png impeller_Play_DlGoldenTest_CanDrawPaint_Vulkan.png From 2d86fa35b73ee487b43b2118f6c743851d7a73df Mon Sep 17 00:00:00 2001 From: Aaron Clarke Date: Mon, 20 May 2024 13:41:45 -0700 Subject: [PATCH 2/5] fixed framework test widgets.widgets.magnifier.styled --- impeller/aiks/canvas.cc | 6 +++--- impeller/aiks/experimental_canvas.cc | 6 +++--- impeller/aiks/paint.cc | 4 ++-- impeller/aiks/paint_pass_delegate.cc | 4 ++-- .../contents/filters/matrix_filter_contents.cc | 12 ++++++------ .../filters/matrix_filter_contents_unittests.cc | 13 ++++++------- impeller/entity/entity.h | 9 +++++++-- impeller/entity/entity_pass.cc | 7 +++++-- 8 files changed, 34 insertions(+), 27 deletions(-) diff --git a/impeller/aiks/canvas.cc b/impeller/aiks/canvas.cc index ace34951073a8..cc8cb3672f816 100644 --- a/impeller/aiks/canvas.cc +++ b/impeller/aiks/canvas.cc @@ -225,7 +225,7 @@ void Canvas::Save(bool create_subpass, entry.cull_rect = transform_stack_.back().cull_rect; entry.clip_height = transform_stack_.back().clip_height; if (create_subpass) { - entry.rendering_mode = Entity::RenderingMode::kBackdropSubpass; + entry.rendering_mode = Entity::RenderingMode::kClippedSubpass; auto subpass = std::make_unique(); if (backdrop_filter) { EntityPass::BackdropFilterProc backdrop_filter_proc = @@ -261,9 +261,9 @@ bool Canvas::Restore() { current_pass_->PopClips(num_clips, current_depth_); if (transform_stack_.back().rendering_mode == - Entity::RenderingMode::kBackdropSubpass || + Entity::RenderingMode::kClippedSubpass || transform_stack_.back().rendering_mode == - Entity::RenderingMode::kImageFilterSubpass) { + Entity::RenderingMode::kSubpass) { current_pass_->SetClipDepth(++current_depth_); current_pass_ = GetCurrentPass().GetSuperpass(); FML_DCHECK(current_pass_); diff --git a/impeller/aiks/experimental_canvas.cc b/impeller/aiks/experimental_canvas.cc index b8df394423dfc..ed17dd549b05b 100644 --- a/impeller/aiks/experimental_canvas.cc +++ b/impeller/aiks/experimental_canvas.cc @@ -230,7 +230,7 @@ void ExperimentalCanvas::SaveLayer( << entry.clip_depth << " <=? " << transform_stack_.back().clip_depth << " after allocating " << total_content_depth; entry.clip_height = transform_stack_.back().clip_height; - entry.rendering_mode = Entity::RenderingMode::kBackdropSubpass; + entry.rendering_mode = Entity::RenderingMode::kClippedSubpass; transform_stack_.emplace_back(entry); auto inline_pass = std::make_unique( @@ -272,9 +272,9 @@ bool ExperimentalCanvas::Restore() { current_depth_ = transform_stack_.back().clip_depth; if (transform_stack_.back().rendering_mode == - Entity::RenderingMode::kBackdropSubpass || + Entity::RenderingMode::kClippedSubpass || transform_stack_.back().rendering_mode == - Entity::RenderingMode::kImageFilterSubpass) { + Entity::RenderingMode::kSubpass) { auto inline_pass = std::move(inline_pass_contexts_.back()); SaveLayerState save_layer_state = save_layer_state_.back(); diff --git a/impeller/aiks/paint.cc b/impeller/aiks/paint.cc index 32252b386a5b9..1efb587601e0d 100644 --- a/impeller/aiks/paint.cc +++ b/impeller/aiks/paint.cc @@ -67,8 +67,8 @@ std::shared_ptr Paint::WithFilters( std::shared_ptr Paint::WithFiltersForSubpassTarget( std::shared_ptr input, const Matrix& effect_transform) const { - auto image_filter = WithImageFilter( - input, effect_transform, Entity::RenderingMode::kImageFilterSubpass); + auto image_filter = + WithImageFilter(input, effect_transform, Entity::RenderingMode::kSubpass); if (image_filter) { input = image_filter; } diff --git a/impeller/aiks/paint_pass_delegate.cc b/impeller/aiks/paint_pass_delegate.cc index 2879075e73063..34610488f5a24 100644 --- a/impeller/aiks/paint_pass_delegate.cc +++ b/impeller/aiks/paint_pass_delegate.cc @@ -51,7 +51,7 @@ std::shared_ptr PaintPassDelegate::WithImageFilter( const FilterInput::Variant& input, const Matrix& effect_transform) const { return paint_.WithImageFilter(input, effect_transform, - Entity::RenderingMode::kImageFilterSubpass); + Entity::RenderingMode::kSubpass); } /// OpacityPeepholePassDelegate @@ -153,7 +153,7 @@ std::shared_ptr OpacityPeepholePassDelegate::WithImageFilter( const FilterInput::Variant& input, const Matrix& effect_transform) const { return paint_.WithImageFilter(input, effect_transform, - Entity::RenderingMode::kImageFilterSubpass); + Entity::RenderingMode::kSubpass); } } // namespace impeller diff --git a/impeller/entity/contents/filters/matrix_filter_contents.cc b/impeller/entity/contents/filters/matrix_filter_contents.cc index 1c73cd37d4dbd..a6971b584c8f8 100644 --- a/impeller/entity/contents/filters/matrix_filter_contents.cc +++ b/impeller/entity/contents/filters/matrix_filter_contents.cc @@ -33,13 +33,13 @@ Matrix CalculateSubpassTransform(const Matrix& snapshot_transform, const Matrix& effect_transform, const Matrix& matrix, Entity::RenderingMode rendering_mode) { - if (rendering_mode == Entity::RenderingMode::kBackdropSubpass) { + if (rendering_mode == Entity::RenderingMode::kClippedSubpass) { return snapshot_transform * // effect_transform * // matrix * // effect_transform.Invert(); } else { - FML_DCHECK(rendering_mode == Entity::RenderingMode::kImageFilterSubpass); + FML_DCHECK(rendering_mode == Entity::RenderingMode::kSubpass); return effect_transform * // matrix * // effect_transform.Invert() * // @@ -60,8 +60,8 @@ std::optional MatrixFilterContents::RenderFilter( return std::nullopt; } - if (rendering_mode_ == Entity::RenderingMode::kImageFilterSubpass || - rendering_mode_ == Entity::RenderingMode::kBackdropSubpass) { + if (rendering_mode_ == Entity::RenderingMode::kSubpass || + rendering_mode_ == Entity::RenderingMode::kClippedSubpass) { // There are two special quirks with how Matrix filters behave when used as // subpass backdrop filters: // @@ -131,8 +131,8 @@ std::optional MatrixFilterContents::GetFilterCoverage( } Matrix input_transform = inputs[0]->GetTransform(entity); - if (rendering_mode_ == Entity::RenderingMode::kImageFilterSubpass || - rendering_mode_ == Entity::RenderingMode::kBackdropSubpass) { + if (rendering_mode_ == Entity::RenderingMode::kSubpass || + rendering_mode_ == Entity::RenderingMode::kClippedSubpass) { Rect coverage_bounds = coverage->TransformBounds(input_transform.Invert()); Matrix transform = CalculateSubpassTransform( input_transform, effect_transform, matrix_, rendering_mode_); diff --git a/impeller/entity/contents/filters/matrix_filter_contents_unittests.cc b/impeller/entity/contents/filters/matrix_filter_contents_unittests.cc index 3c2a3afd41ccd..2f12e5ec5f06a 100644 --- a/impeller/entity/contents/filters/matrix_filter_contents_unittests.cc +++ b/impeller/entity/contents/filters/matrix_filter_contents_unittests.cc @@ -141,13 +141,13 @@ TEST_P(MatrixFilterContentsTest, RenderCoverageMatchesGetCoverageTranslate) { } TEST_P(MatrixFilterContentsTest, - RenderCoverageMatchesGetCoverageBackdropSubpassTranslate) { + RenderCoverageMatchesGetCoverageClippedSubpassTranslate) { std::shared_ptr texture = MakeTexture(ISize(100, 100)); MatrixFilterContents contents; contents.SetInputs({FilterInput::Make(texture)}); contents.SetMatrix(Matrix::MakeTranslation({50, 100, 0})); contents.SetEffectTransform(Matrix::MakeScale({2, 2, 1})); - contents.SetRenderingMode(Entity::RenderingMode::kBackdropSubpass); + contents.SetRenderingMode(Entity::RenderingMode::kClippedSubpass); Entity entity; entity.SetTransform(Matrix::MakeTranslation({100, 200, 0})); @@ -177,13 +177,13 @@ TEST_P(MatrixFilterContentsTest, RenderCoverageMatchesGetCoverageScale) { } TEST_P(MatrixFilterContentsTest, - RenderCoverageMatchesGetCoverageBackdropSubpassScale) { + RenderCoverageMatchesGetCoverageClippedSubpassScale) { std::shared_ptr texture = MakeTexture(ISize(100, 100)); MatrixFilterContents contents; contents.SetInputs({FilterInput::Make(texture)}); contents.SetMatrix(Matrix::MakeScale({3, 3, 1})); contents.SetEffectTransform(Matrix::MakeScale({2, 2, 1})); - contents.SetRenderingMode(Entity::RenderingMode::kBackdropSubpass); + contents.SetRenderingMode(Entity::RenderingMode::kClippedSubpass); Entity entity; entity.SetTransform(Matrix::MakeTranslation({100, 200, 0})); @@ -195,14 +195,13 @@ TEST_P(MatrixFilterContentsTest, Rect::MakeXYWH(100, 200, 300, 300)); } -TEST_P(MatrixFilterContentsTest, - RenderCoverageMatchesGetCoverageImageFilterSubpassScale) { +TEST_P(MatrixFilterContentsTest, RenderCoverageMatchesGetCoverageSubpassScale) { std::shared_ptr texture = MakeTexture(ISize(100, 100)); MatrixFilterContents contents; contents.SetInputs({FilterInput::Make(texture)}); contents.SetMatrix(Matrix::MakeScale({3, 3, 1})); contents.SetEffectTransform(Matrix::MakeScale({2, 2, 1})); - contents.SetRenderingMode(Entity::RenderingMode::kImageFilterSubpass); + contents.SetRenderingMode(Entity::RenderingMode::kSubpass); Entity entity; entity.SetTransform(Matrix::MakeTranslation({100, 200, 0})); diff --git a/impeller/entity/entity.h b/impeller/entity/entity.h index 293a17867661e..146b91733816d 100644 --- a/impeller/entity/entity.h +++ b/impeller/entity/entity.h @@ -33,8 +33,13 @@ class Entity { /// rather than local space, and so some filters (namely, /// MatrixFilterContents) need to interpret the given EffectTransform as the /// current transform matrix. - kBackdropSubpass, - kImageFilterSubpass, + /// + /// With kClippedSubpass the filters's transform is prepended to the + /// snapshot's transform. With kSubpass the filters's transform is + /// appended + /// to the snapshot's transform. + kClippedSubpass, + kSubpass, }; /// An enum to define how to repeat, fold, or omit colors outside of the diff --git a/impeller/entity/entity_pass.cc b/impeller/entity/entity_pass.cc index ee28a0dd4f68e..65939af7b2605 100644 --- a/impeller/entity/entity_pass.cc +++ b/impeller/entity/entity_pass.cc @@ -185,7 +185,7 @@ std::optional EntityPass::GetElementsCoverage( std::shared_ptr backdrop_filter = subpass.backdrop_filter_proc_( FilterInput::Make(accumulated_coverage.value()), - subpass.transform_, Entity::RenderingMode::kBackdropSubpass); + subpass.transform_, Entity::RenderingMode::kClippedSubpass); if (backdrop_filter) { auto backdrop_coverage = backdrop_filter->GetCoverage({}); unfiltered_coverage = @@ -585,9 +585,12 @@ EntityPass::EntityResult EntityPass::GetEntityForElement( auto texture = pass_context.GetTexture(); // Render the backdrop texture before any of the pass elements. const auto& proc = subpass->backdrop_filter_proc_; + subpass_backdrop_filter_contents = proc( FilterInput::Make(std::move(texture)), subpass->transform_.Basis(), - Entity::RenderingMode::kBackdropSubpass); + subpass->bounds_promise_ == ContentBoundsPromise::kMayClipContents + ? Entity::RenderingMode::kClippedSubpass + : Entity::RenderingMode::kSubpass); // If the very first thing we render in this EntityPass is a subpass that // happens to have a backdrop filter, than that backdrop filter will end From 8378c24af53725eab6ef4e5b2cba5b1bfed61b76 Mon Sep 17 00:00:00 2001 From: Aaron Clarke Date: Mon, 20 May 2024 17:24:19 -0700 Subject: [PATCH 3/5] switched to prepend append --- impeller/entity/entity_pass.cc | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/impeller/entity/entity_pass.cc b/impeller/entity/entity_pass.cc index 65939af7b2605..659766896c8fc 100644 --- a/impeller/entity/entity_pass.cc +++ b/impeller/entity/entity_pass.cc @@ -586,11 +586,12 @@ EntityPass::EntityResult EntityPass::GetEntityForElement( // Render the backdrop texture before any of the pass elements. const auto& proc = subpass->backdrop_filter_proc_; - subpass_backdrop_filter_contents = proc( - FilterInput::Make(std::move(texture)), subpass->transform_.Basis(), - subpass->bounds_promise_ == ContentBoundsPromise::kMayClipContents - ? Entity::RenderingMode::kClippedSubpass - : Entity::RenderingMode::kSubpass); + Matrix subpass_transform_basis = subpass->transform_.Basis(); + subpass_backdrop_filter_contents = + proc(FilterInput::Make(std::move(texture)), subpass_transform_basis, + subpass_transform_basis == subpass->transform_ + ? Entity::RenderingMode::kClippedSubpass + : Entity::RenderingMode::kSubpass); // If the very first thing we render in this EntityPass is a subpass that // happens to have a backdrop filter, than that backdrop filter will end From 373cf526918135d385f649af914d0d038eea14a2 Mon Sep 17 00:00:00 2001 From: Aaron Clarke Date: Mon, 20 May 2024 17:32:32 -0700 Subject: [PATCH 4/5] switched to explicit rendermode names, deduced when they were needed --- impeller/aiks/canvas.cc | 7 ++++--- impeller/aiks/experimental_canvas.cc | 6 +++--- impeller/aiks/paint.cc | 3 ++- impeller/aiks/paint_pass_delegate.cc | 10 ++++++---- .../contents/filters/matrix_filter_contents.cc | 18 ++++++++++++------ .../matrix_filter_contents_unittests.cc | 9 ++++++--- impeller/entity/entity.h | 9 ++------- impeller/entity/entity_pass.cc | 9 ++++++--- 8 files changed, 41 insertions(+), 30 deletions(-) diff --git a/impeller/aiks/canvas.cc b/impeller/aiks/canvas.cc index cc8cb3672f816..6643450598607 100644 --- a/impeller/aiks/canvas.cc +++ b/impeller/aiks/canvas.cc @@ -225,7 +225,8 @@ void Canvas::Save(bool create_subpass, entry.cull_rect = transform_stack_.back().cull_rect; entry.clip_height = transform_stack_.back().clip_height; if (create_subpass) { - entry.rendering_mode = Entity::RenderingMode::kClippedSubpass; + entry.rendering_mode = + Entity::RenderingMode::kSubpassAppendSnapshotTransform; auto subpass = std::make_unique(); if (backdrop_filter) { EntityPass::BackdropFilterProc backdrop_filter_proc = @@ -261,9 +262,9 @@ bool Canvas::Restore() { current_pass_->PopClips(num_clips, current_depth_); if (transform_stack_.back().rendering_mode == - Entity::RenderingMode::kClippedSubpass || + Entity::RenderingMode::kSubpassAppendSnapshotTransform || transform_stack_.back().rendering_mode == - Entity::RenderingMode::kSubpass) { + Entity::RenderingMode::kSubpassPrependSnapshotTransform) { current_pass_->SetClipDepth(++current_depth_); current_pass_ = GetCurrentPass().GetSuperpass(); FML_DCHECK(current_pass_); diff --git a/impeller/aiks/experimental_canvas.cc b/impeller/aiks/experimental_canvas.cc index ed17dd549b05b..5bc7909202f69 100644 --- a/impeller/aiks/experimental_canvas.cc +++ b/impeller/aiks/experimental_canvas.cc @@ -230,7 +230,7 @@ void ExperimentalCanvas::SaveLayer( << entry.clip_depth << " <=? " << transform_stack_.back().clip_depth << " after allocating " << total_content_depth; entry.clip_height = transform_stack_.back().clip_height; - entry.rendering_mode = Entity::RenderingMode::kClippedSubpass; + entry.rendering_mode = Entity::RenderingMode::kSubpassAppendSnapshotTransform; transform_stack_.emplace_back(entry); auto inline_pass = std::make_unique( @@ -272,9 +272,9 @@ bool ExperimentalCanvas::Restore() { current_depth_ = transform_stack_.back().clip_depth; if (transform_stack_.back().rendering_mode == - Entity::RenderingMode::kClippedSubpass || + Entity::RenderingMode::kSubpassAppendSnapshotTransform || transform_stack_.back().rendering_mode == - Entity::RenderingMode::kSubpass) { + Entity::RenderingMode::kSubpassPrependSnapshotTransform) { auto inline_pass = std::move(inline_pass_contexts_.back()); SaveLayerState save_layer_state = save_layer_state_.back(); diff --git a/impeller/aiks/paint.cc b/impeller/aiks/paint.cc index 1efb587601e0d..aa7350c031493 100644 --- a/impeller/aiks/paint.cc +++ b/impeller/aiks/paint.cc @@ -68,7 +68,8 @@ std::shared_ptr Paint::WithFiltersForSubpassTarget( std::shared_ptr input, const Matrix& effect_transform) const { auto image_filter = - WithImageFilter(input, effect_transform, Entity::RenderingMode::kSubpass); + WithImageFilter(input, effect_transform, + Entity::RenderingMode::kSubpassPrependSnapshotTransform); if (image_filter) { input = image_filter; } diff --git a/impeller/aiks/paint_pass_delegate.cc b/impeller/aiks/paint_pass_delegate.cc index 34610488f5a24..17dfe4648fdb8 100644 --- a/impeller/aiks/paint_pass_delegate.cc +++ b/impeller/aiks/paint_pass_delegate.cc @@ -50,8 +50,9 @@ std::shared_ptr PaintPassDelegate::CreateContentsForSubpassTarget( std::shared_ptr PaintPassDelegate::WithImageFilter( const FilterInput::Variant& input, const Matrix& effect_transform) const { - return paint_.WithImageFilter(input, effect_transform, - Entity::RenderingMode::kSubpass); + return paint_.WithImageFilter( + input, effect_transform, + Entity::RenderingMode::kSubpassPrependSnapshotTransform); } /// OpacityPeepholePassDelegate @@ -152,8 +153,9 @@ OpacityPeepholePassDelegate::CreateContentsForSubpassTarget( std::shared_ptr OpacityPeepholePassDelegate::WithImageFilter( const FilterInput::Variant& input, const Matrix& effect_transform) const { - return paint_.WithImageFilter(input, effect_transform, - Entity::RenderingMode::kSubpass); + return paint_.WithImageFilter( + input, effect_transform, + Entity::RenderingMode::kSubpassPrependSnapshotTransform); } } // namespace impeller diff --git a/impeller/entity/contents/filters/matrix_filter_contents.cc b/impeller/entity/contents/filters/matrix_filter_contents.cc index a6971b584c8f8..1ffaddb3dbb9b 100644 --- a/impeller/entity/contents/filters/matrix_filter_contents.cc +++ b/impeller/entity/contents/filters/matrix_filter_contents.cc @@ -33,13 +33,15 @@ Matrix CalculateSubpassTransform(const Matrix& snapshot_transform, const Matrix& effect_transform, const Matrix& matrix, Entity::RenderingMode rendering_mode) { - if (rendering_mode == Entity::RenderingMode::kClippedSubpass) { + if (rendering_mode == + Entity::RenderingMode::kSubpassAppendSnapshotTransform) { return snapshot_transform * // effect_transform * // matrix * // effect_transform.Invert(); } else { - FML_DCHECK(rendering_mode == Entity::RenderingMode::kSubpass); + FML_DCHECK(rendering_mode == + Entity::RenderingMode::kSubpassPrependSnapshotTransform); return effect_transform * // matrix * // effect_transform.Invert() * // @@ -60,8 +62,10 @@ std::optional MatrixFilterContents::RenderFilter( return std::nullopt; } - if (rendering_mode_ == Entity::RenderingMode::kSubpass || - rendering_mode_ == Entity::RenderingMode::kClippedSubpass) { + if (rendering_mode_ == + Entity::RenderingMode::kSubpassPrependSnapshotTransform || + rendering_mode_ == + Entity::RenderingMode::kSubpassAppendSnapshotTransform) { // There are two special quirks with how Matrix filters behave when used as // subpass backdrop filters: // @@ -131,8 +135,10 @@ std::optional MatrixFilterContents::GetFilterCoverage( } Matrix input_transform = inputs[0]->GetTransform(entity); - if (rendering_mode_ == Entity::RenderingMode::kSubpass || - rendering_mode_ == Entity::RenderingMode::kClippedSubpass) { + if (rendering_mode_ == + Entity::RenderingMode::kSubpassPrependSnapshotTransform || + rendering_mode_ == + Entity::RenderingMode::kSubpassAppendSnapshotTransform) { Rect coverage_bounds = coverage->TransformBounds(input_transform.Invert()); Matrix transform = CalculateSubpassTransform( input_transform, effect_transform, matrix_, rendering_mode_); diff --git a/impeller/entity/contents/filters/matrix_filter_contents_unittests.cc b/impeller/entity/contents/filters/matrix_filter_contents_unittests.cc index 2f12e5ec5f06a..6079331d7fd4f 100644 --- a/impeller/entity/contents/filters/matrix_filter_contents_unittests.cc +++ b/impeller/entity/contents/filters/matrix_filter_contents_unittests.cc @@ -147,7 +147,8 @@ TEST_P(MatrixFilterContentsTest, contents.SetInputs({FilterInput::Make(texture)}); contents.SetMatrix(Matrix::MakeTranslation({50, 100, 0})); contents.SetEffectTransform(Matrix::MakeScale({2, 2, 1})); - contents.SetRenderingMode(Entity::RenderingMode::kClippedSubpass); + contents.SetRenderingMode( + Entity::RenderingMode::kSubpassAppendSnapshotTransform); Entity entity; entity.SetTransform(Matrix::MakeTranslation({100, 200, 0})); @@ -183,7 +184,8 @@ TEST_P(MatrixFilterContentsTest, contents.SetInputs({FilterInput::Make(texture)}); contents.SetMatrix(Matrix::MakeScale({3, 3, 1})); contents.SetEffectTransform(Matrix::MakeScale({2, 2, 1})); - contents.SetRenderingMode(Entity::RenderingMode::kClippedSubpass); + contents.SetRenderingMode( + Entity::RenderingMode::kSubpassAppendSnapshotTransform); Entity entity; entity.SetTransform(Matrix::MakeTranslation({100, 200, 0})); @@ -201,7 +203,8 @@ TEST_P(MatrixFilterContentsTest, RenderCoverageMatchesGetCoverageSubpassScale) { contents.SetInputs({FilterInput::Make(texture)}); contents.SetMatrix(Matrix::MakeScale({3, 3, 1})); contents.SetEffectTransform(Matrix::MakeScale({2, 2, 1})); - contents.SetRenderingMode(Entity::RenderingMode::kSubpass); + contents.SetRenderingMode( + Entity::RenderingMode::kSubpassPrependSnapshotTransform); Entity entity; entity.SetTransform(Matrix::MakeTranslation({100, 200, 0})); diff --git a/impeller/entity/entity.h b/impeller/entity/entity.h index 146b91733816d..c65b2ea190dea 100644 --- a/impeller/entity/entity.h +++ b/impeller/entity/entity.h @@ -33,13 +33,8 @@ class Entity { /// rather than local space, and so some filters (namely, /// MatrixFilterContents) need to interpret the given EffectTransform as the /// current transform matrix. - /// - /// With kClippedSubpass the filters's transform is prepended to the - /// snapshot's transform. With kSubpass the filters's transform is - /// appended - /// to the snapshot's transform. - kClippedSubpass, - kSubpass, + kSubpassAppendSnapshotTransform, + kSubpassPrependSnapshotTransform, }; /// An enum to define how to repeat, fold, or omit colors outside of the diff --git a/impeller/entity/entity_pass.cc b/impeller/entity/entity_pass.cc index 659766896c8fc..26691b14052a6 100644 --- a/impeller/entity/entity_pass.cc +++ b/impeller/entity/entity_pass.cc @@ -185,7 +185,8 @@ std::optional EntityPass::GetElementsCoverage( std::shared_ptr backdrop_filter = subpass.backdrop_filter_proc_( FilterInput::Make(accumulated_coverage.value()), - subpass.transform_, Entity::RenderingMode::kClippedSubpass); + subpass.transform_, + Entity::RenderingMode::kSubpassAppendSnapshotTransform); if (backdrop_filter) { auto backdrop_coverage = backdrop_filter->GetCoverage({}); unfiltered_coverage = @@ -589,9 +590,11 @@ EntityPass::EntityResult EntityPass::GetEntityForElement( Matrix subpass_transform_basis = subpass->transform_.Basis(); subpass_backdrop_filter_contents = proc(FilterInput::Make(std::move(texture)), subpass_transform_basis, + // When the subpass has a translation that means the math with + // the snapshot has to be different. subpass_transform_basis == subpass->transform_ - ? Entity::RenderingMode::kClippedSubpass - : Entity::RenderingMode::kSubpass); + ? Entity::RenderingMode::kSubpassAppendSnapshotTransform + : Entity::RenderingMode::kSubpassPrependSnapshotTransform); // If the very first thing we render in this EntityPass is a subpass that // happens to have a backdrop filter, than that backdrop filter will end From b1fef1157c5f1d7d61309359d5c1c72a8cdfc2c0 Mon Sep 17 00:00:00 2001 From: Aaron Clarke Date: Tue, 21 May 2024 11:23:37 -0700 Subject: [PATCH 5/5] brandons feedback --- impeller/entity/entity_pass.cc | 15 +++++++-------- impeller/geometry/matrix.h | 2 ++ impeller/geometry/matrix_unittests.cc | 7 +++++++ 3 files changed, 16 insertions(+), 8 deletions(-) diff --git a/impeller/entity/entity_pass.cc b/impeller/entity/entity_pass.cc index 26691b14052a6..6f4531eedcf3d 100644 --- a/impeller/entity/entity_pass.cc +++ b/impeller/entity/entity_pass.cc @@ -587,14 +587,13 @@ EntityPass::EntityResult EntityPass::GetEntityForElement( // Render the backdrop texture before any of the pass elements. const auto& proc = subpass->backdrop_filter_proc_; - Matrix subpass_transform_basis = subpass->transform_.Basis(); - subpass_backdrop_filter_contents = - proc(FilterInput::Make(std::move(texture)), subpass_transform_basis, - // When the subpass has a translation that means the math with - // the snapshot has to be different. - subpass_transform_basis == subpass->transform_ - ? Entity::RenderingMode::kSubpassAppendSnapshotTransform - : Entity::RenderingMode::kSubpassPrependSnapshotTransform); + subpass_backdrop_filter_contents = proc( + FilterInput::Make(std::move(texture)), subpass->transform_.Basis(), + // When the subpass has a translation that means the math with + // the snapshot has to be different. + subpass->transform_.HasTranslation() + ? Entity::RenderingMode::kSubpassPrependSnapshotTransform + : Entity::RenderingMode::kSubpassAppendSnapshotTransform); // If the very first thing we render in this EntityPass is a subpass that // happens to have a backdrop filter, than that backdrop filter will end diff --git a/impeller/geometry/matrix.h b/impeller/geometry/matrix.h index 057084327bbf0..fd9a2163dd644 100644 --- a/impeller/geometry/matrix.h +++ b/impeller/geometry/matrix.h @@ -331,6 +331,8 @@ struct Matrix { return m[3] != 0 || m[7] != 0 || m[11] != 0 || m[15] != 1; } + constexpr bool HasTranslation() const { return m[12] != 0 || m[13] != 0; } + constexpr bool IsAligned2D(Scalar tolerance = 0) const { if (HasPerspective2D()) { return false; diff --git a/impeller/geometry/matrix_unittests.cc b/impeller/geometry/matrix_unittests.cc index 9eea4ac747855..d83e041eb3fbf 100644 --- a/impeller/geometry/matrix_unittests.cc +++ b/impeller/geometry/matrix_unittests.cc @@ -60,6 +60,13 @@ TEST(MatrixTest, HasPerspective) { // clang-format on } +TEST(MatrixTest, HasTranslation) { + EXPECT_TRUE(Matrix::MakeTranslation({100, 100, 0}).HasTranslation()); + EXPECT_TRUE(Matrix::MakeTranslation({0, 100, 0}).HasTranslation()); + EXPECT_TRUE(Matrix::MakeTranslation({100, 0, 0}).HasTranslation()); + EXPECT_FALSE(Matrix().HasTranslation()); +} + TEST(MatrixTest, IsAligned2D) { EXPECT_TRUE(Matrix().IsAligned2D()); EXPECT_TRUE(Matrix::MakeScale({1.0f, 1.0f, 2.0f}).IsAligned2D());