From 993e850c9f28f90097eb317f75c074901970e6c6 Mon Sep 17 00:00:00 2001 From: Brandon DeRosier Date: Thu, 5 Oct 2023 13:15:35 -0700 Subject: [PATCH 1/3] [Impeller] Track clip coverage per-pass when not collapsing. --- impeller/aiks/aiks_unittests.cc | 28 +++++++++++++------ .../filters/matrix_filter_contents.cc | 2 +- impeller/entity/entity_pass.cc | 20 ++++++++----- 3 files changed, 34 insertions(+), 16 deletions(-) diff --git a/impeller/aiks/aiks_unittests.cc b/impeller/aiks/aiks_unittests.cc index 695a7b26a1de2..1cacea253f7f0 100644 --- a/impeller/aiks/aiks_unittests.cc +++ b/impeller/aiks/aiks_unittests.cc @@ -3619,15 +3619,27 @@ TEST_P(AiksTest, MatrixImageFilterMagnify) { canvas.Translate({600, -200}); canvas.SaveLayer({ .image_filter = std::make_shared( - Matrix{ - 2, 0, 0, 0, // - 0, 2, 0, 0, // - 0, 0, 2, 0, // - 0, 0, 0, 1 // - }, - SamplerDescriptor{}), + Matrix::MakeScale({2, 2, 2}), SamplerDescriptor{}), + }); + canvas.DrawImage(image, {0, 0}, + Paint{.color = Color::White().WithAlpha(0.5)}); + canvas.Restore(); + + ASSERT_TRUE(OpenPlaygroundHere(canvas.EndRecordingAsPicture())); +} + +// Render a white circle at the top left corner of the screen. +TEST_P(AiksTest, MatrixImageFilterDoesntCullWhenTranslatedFromOffscreen) { + Canvas canvas; + canvas.Scale(GetContentScale()); + canvas.Translate({100, 100}); + // Draw a circle in a SaveLayer at -300, but move it back on-screen with a + // +300 translation applied by a SaveLayer image filter. + canvas.SaveLayer({ + .image_filter = std::make_shared( + Matrix::MakeTranslation({300, 0}), SamplerDescriptor{}), }); - canvas.DrawImage(image, {0, 0}, Paint{.color = Color(1.0, 1.0, 1.0, 0.5)}); + canvas.DrawCircle({-300, 0}, 100, {.color = Color::White()}); 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 d88edc298e3b9..c16bcdb88d698 100644 --- a/impeller/entity/contents/filters/matrix_filter_contents.cc +++ b/impeller/entity/contents/filters/matrix_filter_contents.cc @@ -45,7 +45,7 @@ std::optional MatrixFilterContents::RenderFilter( // 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 + // 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! // diff --git a/impeller/entity/entity_pass.cc b/impeller/entity/entity_pass.cc index c6af10254f7d6..bf183479a9385 100644 --- a/impeller/entity/entity_pass.cc +++ b/impeller/entity/entity_pass.cc @@ -198,12 +198,10 @@ std::optional EntityPass::GetSubpassCoverage( std::shared_ptr image_filter = subpass.delegate_->WithImageFilter(Rect(), subpass.xformation_); - // If the filter graph transforms the basis of the subpass, then its space - // has deviated too much from the parent pass to safely intersect with the - // pass coverage limit. - coverage_limit = - (image_filter && !image_filter->IsTranslationOnly() ? std::nullopt - : coverage_limit); + // If the subpass has an image filter, then its coverage space may deviate + // from the parent pass and make intersecting with the pass coverage limit + // unsafe. + coverage_limit = image_filter ? std::nullopt : coverage_limit; auto entities_coverage = subpass.GetElementsCoverage(coverage_limit); // The entities don't cover anything. There is nothing to do. @@ -641,6 +639,14 @@ EntityPass::EntityResult EntityPass::GetEntityForElement( auto subpass_capture = capture.CreateChild("EntityPass"); subpass_capture.AddRect("Coverage", *subpass_coverage, {.readonly = true}); + // Start non-collapsed subpasses with a fresh clip coverage stack limited by + // the subpass coverage. This is important because image filters applied to + // save layers may transform the subpass texture after its rendered, causing + // parent clip coverage to get misaligned with the actual area that the + // subpass will affect in the parent pass. + ClipCoverageStack subpass_clip_coverage_stack = {ClipCoverageLayer{ + .coverage = subpass_coverage, .clip_depth = subpass->clip_depth_}}; + // Stencil textures aren't shared between EntityPasses (as much of the // time they are transient). if (!subpass->OnRender( @@ -652,7 +658,7 @@ EntityPass::EntityResult EntityPass::GetEntityForElement( subpass_coverage->origin - global_pass_position, // local_pass_position ++pass_depth, // pass_depth - clip_coverage_stack, // clip_coverage_stack + subpass_clip_coverage_stack, // clip_coverage_stack subpass->clip_depth_, // clip_depth_floor subpass_backdrop_filter_contents // backdrop_filter_contents )) { From d09eba180d61a8a4d1cd8634832292df1b89c2fe Mon Sep 17 00:00:00 2001 From: Brandon DeRosier Date: Thu, 5 Oct 2023 14:58:20 -0700 Subject: [PATCH 2/3] Address comments --- impeller/aiks/aiks_unittests.cc | 21 ++++++++++++++++++++- impeller/entity/entity_pass.cc | 6 +++--- impeller/entity/entity_pass.h | 11 ++++++++++- 3 files changed, 33 insertions(+), 5 deletions(-) diff --git a/impeller/aiks/aiks_unittests.cc b/impeller/aiks/aiks_unittests.cc index 1cacea253f7f0..1a50dbe3e9406 100644 --- a/impeller/aiks/aiks_unittests.cc +++ b/impeller/aiks/aiks_unittests.cc @@ -3639,7 +3639,26 @@ TEST_P(AiksTest, MatrixImageFilterDoesntCullWhenTranslatedFromOffscreen) { .image_filter = std::make_shared( Matrix::MakeTranslation({300, 0}), SamplerDescriptor{}), }); - canvas.DrawCircle({-300, 0}, 100, {.color = Color::White()}); + canvas.DrawCircle({-300, 0}, 100, {.color = Color::Green()}); + canvas.Restore(); + + ASSERT_TRUE(OpenPlaygroundHere(canvas.EndRecordingAsPicture())); +} + +// Render a white circle at the top left corner of the screen. +TEST_P(AiksTest, + MatrixImageFilterDoesntCullWhenScaledAndTranslatedFromOffscreen) { + Canvas canvas; + canvas.Scale(GetContentScale()); + canvas.Translate({100, 100}); + // Draw a circle in a SaveLayer at -300, but move it back on-screen with a + // +300 translation applied by a SaveLayer image filter. + canvas.SaveLayer({ + .image_filter = std::make_shared( + Matrix::MakeTranslation({300, 0}) * Matrix::MakeScale({2, 2, 2}), + SamplerDescriptor{}), + }); + canvas.DrawCircle({-150, 0}, 50, {.color = Color::Green()}); canvas.Restore(); ASSERT_TRUE(OpenPlaygroundHere(canvas.EndRecordingAsPicture())); diff --git a/impeller/entity/entity_pass.cc b/impeller/entity/entity_pass.cc index bf183479a9385..bb065b959e146 100644 --- a/impeller/entity/entity_pass.cc +++ b/impeller/entity/entity_pass.cc @@ -641,9 +641,9 @@ EntityPass::EntityResult EntityPass::GetEntityForElement( // Start non-collapsed subpasses with a fresh clip coverage stack limited by // the subpass coverage. This is important because image filters applied to - // save layers may transform the subpass texture after its rendered, causing - // parent clip coverage to get misaligned with the actual area that the - // subpass will affect in the parent pass. + // save layers may transform the subpass texture after it's rendered, + // causing parent clip coverage to get misaligned with the actual area that + // the subpass will affect in the parent pass. ClipCoverageStack subpass_clip_coverage_stack = {ClipCoverageLayer{ .coverage = subpass_coverage, .clip_depth = subpass->clip_depth_}}; diff --git a/impeller/entity/entity_pass.h b/impeller/entity/entity_pass.h index 522f4f9562b6e..fd30118d05d85 100644 --- a/impeller/entity/entity_pass.h +++ b/impeller/entity/entity_pass.h @@ -142,7 +142,16 @@ class EntityPass { void SetEnableOffscreenCheckerboard(bool enabled); //---------------------------------------------------------------------------- - /// @brief Get the coverage of an unfiltered subpass. + /// @brief Computes the coverage of a given subpass. This is used to + /// determine the texture size of a given subpass before it's rendered + /// to and passed through the subpass ImageFilter, if any. + /// + /// @param[in] subpass The EntityPass for which to compute + /// pre-filteredcoverage. + /// @param[in] coverage_limit Confines coverage to a specified area. This + /// hint is used to trim coverage to the root + /// framebuffer area. `std::nullopt` means no + /// coverage. /// std::optional GetSubpassCoverage( const EntityPass& subpass, From 886d61e0f4c2b9836b51674fb22cbace895839de Mon Sep 17 00:00:00 2001 From: Brandon DeRosier Date: Fri, 6 Oct 2023 02:18:22 -0700 Subject: [PATCH 3/3] Improve docstrings --- impeller/entity/entity_pass.h | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/impeller/entity/entity_pass.h b/impeller/entity/entity_pass.h index fd30118d05d85..261d3272b14aa 100644 --- a/impeller/entity/entity_pass.h +++ b/impeller/entity/entity_pass.h @@ -150,8 +150,13 @@ class EntityPass { /// pre-filteredcoverage. /// @param[in] coverage_limit Confines coverage to a specified area. This /// hint is used to trim coverage to the root - /// framebuffer area. `std::nullopt` means no - /// coverage. + /// framebuffer area. `std::nullopt` means there + /// is no limit. + /// + /// @return The screen space pixel area that the subpass contents will render + /// into, prior to being transformed by the subpass ImageFilter, if + /// any. `std::nullopt` means rendering the subpass will have no + /// effect on the color attachment. /// std::optional GetSubpassCoverage( const EntityPass& subpass,