From d04690cbe01edca4b565bf199858de1be4bd0bb7 Mon Sep 17 00:00:00 2001 From: Jim Graham Date: Tue, 7 Nov 2023 20:37:19 -0800 Subject: [PATCH] [Impeller] Add Rect::GetNormalizingTransform to handle UV coordinate conversion (flutter/engine#47775) Three places in the code were manually computing the UV coordinates relative to a texture coverage rectangle while also transforming the points. This change will make it easier both to compute the UV conversion matrix and also to consolidate it with the transform that was already being applied to streamline the total computations. --- .../flutter/impeller/aiks/aiks_unittests.cc | 46 +++++- .../entity/geometry/fill_path_geometry.cc | 13 +- .../impeller/entity/geometry/geometry.cc | 5 +- .../entity/geometry/vertices_geometry.cc | 8 +- engine/src/flutter/impeller/geometry/rect.h | 29 ++++ .../impeller/geometry/rect_unittests.cc | 150 +++++++++++++++++- 6 files changed, 225 insertions(+), 26 deletions(-) diff --git a/engine/src/flutter/impeller/aiks/aiks_unittests.cc b/engine/src/flutter/impeller/aiks/aiks_unittests.cc index b646c6dbe0b..3191257a14d 100644 --- a/engine/src/flutter/impeller/aiks/aiks_unittests.cc +++ b/engine/src/flutter/impeller/aiks/aiks_unittests.cc @@ -206,12 +206,23 @@ void CanRenderTiledTexture(AiksTest* aiks_test, canvas.DrawRect(Rect::MakeXYWH(0, 0, 600, 600), paint); } - // Should not change the image. - PathBuilder path_builder; - path_builder.AddCircle({150, 150}, 150); - path_builder.AddRoundedRect(Rect::MakeLTRB(300, 300, 600, 600), 10); - paint.style = Paint::Style::kFill; - canvas.DrawPath(path_builder.TakePath(), paint); + { + // Should not change the image. + PathBuilder path_builder; + path_builder.AddCircle({150, 150}, 150); + path_builder.AddRoundedRect(Rect::MakeLTRB(300, 300, 600, 600), 10); + paint.style = Paint::Style::kFill; + canvas.DrawPath(path_builder.TakePath(), paint); + } + + { + // Should not change the image. Tests the Convex short-cut code. + PathBuilder path_builder; + path_builder.AddCircle({150, 450}, 150); + path_builder.SetConvexity(Convexity::kConvex); + paint.style = Paint::Style::kFill; + canvas.DrawPath(path_builder.TakePath(), paint); + } ASSERT_TRUE(aiks_test->OpenPlaygroundHere(canvas.EndRecordingAsPicture())); } @@ -3744,6 +3755,29 @@ TEST_P(AiksTest, VerticesGeometryUVPositionData) { ASSERT_TRUE(OpenPlaygroundHere(canvas.EndRecordingAsPicture())); } +// Regression test for https://github.com/flutter/flutter/issues/135441 . +TEST_P(AiksTest, VerticesGeometryUVPositionDataWithTranslate) { + Canvas canvas; + Paint paint; + auto texture = CreateTextureForFixture("table_mountain_nx.png"); + + paint.color_source = ColorSource::MakeImage( + texture, Entity::TileMode::kClamp, Entity::TileMode::kClamp, {}, + Matrix::MakeTranslation({100.0, 100.0})); + + auto vertices = {Point(0, 0), Point(texture->GetSize().width, 0), + Point(0, texture->GetSize().height)}; + std::vector indices = {0u, 1u, 2u}; + std::vector texture_coordinates = {}; + std::vector vertex_colors = {}; + auto geometry = std::make_shared( + vertices, indices, texture_coordinates, vertex_colors, + Rect::MakeLTRB(0, 0, 1, 1), VerticesGeometry::VertexMode::kTriangleStrip); + + canvas.DrawVertices(geometry, BlendMode::kSourceOver, paint); + ASSERT_TRUE(OpenPlaygroundHere(canvas.EndRecordingAsPicture())); +} + TEST_P(AiksTest, ClearBlendWithBlur) { Canvas canvas; Paint white; diff --git a/engine/src/flutter/impeller/entity/geometry/fill_path_geometry.cc b/engine/src/flutter/impeller/entity/geometry/fill_path_geometry.cc index 1153572f25c..4cd025aef04 100644 --- a/engine/src/flutter/impeller/entity/geometry/fill_path_geometry.cc +++ b/engine/src/flutter/impeller/entity/geometry/fill_path_geometry.cc @@ -82,6 +82,9 @@ GeometryResult FillPathGeometry::GetPositionUVBuffer( RenderPass& pass) { using VS = TextureFillVertexShader; + auto uv_transform = + texture_coverage.GetNormalizingTransform() * effect_transform; + if (path_.GetFillType() == FillType::kNonZero && // path_.IsConvex()) { auto [points, indices] = TessellateConvex( @@ -93,9 +96,7 @@ GeometryResult FillPathGeometry::GetPositionUVBuffer( for (auto i = 0u; i < points.size(); i++) { VS::PerVertexData data; data.position = points[i]; - data.texture_coords = effect_transform * - (points[i] - texture_coverage.origin) / - texture_coverage.size; + data.texture_coords = uv_transform * points[i]; vertex_builder.AppendVertex(data); } for (auto i = 0u; i < indices.size(); i++) { @@ -116,16 +117,14 @@ GeometryResult FillPathGeometry::GetPositionUVBuffer( auto tesselation_result = renderer.GetTessellator()->Tessellate( path_.GetFillType(), path_.CreatePolyline(entity.GetTransformation().GetMaxBasisLength()), - [&vertex_builder, &texture_coverage, &effect_transform]( + [&vertex_builder, &uv_transform]( const float* vertices, size_t vertices_count, const uint16_t* indices, size_t indices_count) { for (auto i = 0u; i < vertices_count * 2; i += 2) { VS::PerVertexData data; Point vtx = {vertices[i], vertices[i + 1]}; data.position = vtx; - data.texture_coords = effect_transform * - (vtx - texture_coverage.origin) / - texture_coverage.size; + data.texture_coords = uv_transform * vtx; vertex_builder.AppendVertex(data); } FML_DCHECK(vertex_builder.GetVertexCount() == vertices_count); diff --git a/engine/src/flutter/impeller/entity/geometry/geometry.cc b/engine/src/flutter/impeller/entity/geometry/geometry.cc index cff1770f134..29d880a86c8 100644 --- a/engine/src/flutter/impeller/entity/geometry/geometry.cc +++ b/engine/src/flutter/impeller/entity/geometry/geometry.cc @@ -75,12 +75,13 @@ GeometryResult ComputeUVGeometryForRect(Rect source_rect, RenderPass& pass) { auto& host_buffer = pass.GetTransientsBuffer(); + auto uv_transform = + texture_coverage.GetNormalizingTransform() * effect_transform; std::vector data(8); auto points = source_rect.GetPoints(); for (auto i = 0u, j = 0u; i < 8; i += 2, j++) { data[i] = points[j]; - data[i + 1] = effect_transform * (points[j] - texture_coverage.origin) / - texture_coverage.size; + data[i + 1] = uv_transform * points[j]; } return GeometryResult{ diff --git a/engine/src/flutter/impeller/entity/geometry/vertices_geometry.cc b/engine/src/flutter/impeller/entity/geometry/vertices_geometry.cc index 23a1c32a5f0..42af1e49c0a 100644 --- a/engine/src/flutter/impeller/entity/geometry/vertices_geometry.cc +++ b/engine/src/flutter/impeller/entity/geometry/vertices_geometry.cc @@ -227,8 +227,8 @@ GeometryResult VerticesGeometry::GetPositionUVBuffer( auto index_count = indices_.size(); auto vertex_count = vertices_.size(); - auto size = texture_coverage.size; - auto origin = texture_coverage.origin; + auto uv_transform = + texture_coverage.GetNormalizingTransform() * effect_transform; auto has_texture_coordinates = HasTextureCoordinates(); std::vector vertex_data(vertex_count); { @@ -236,9 +236,7 @@ GeometryResult VerticesGeometry::GetPositionUVBuffer( auto vertex = vertices_[i]; auto texture_coord = has_texture_coordinates ? texture_coordinates_[i] : vertices_[i]; - auto uv = - effect_transform * Point((texture_coord.x - origin.x) / size.width, - (texture_coord.y - origin.y) / size.height); + auto uv = uv_transform * texture_coord; // From experimentation we need to clamp these values to < 1.0 or else // there can be flickering. vertex_data[i] = { diff --git a/engine/src/flutter/impeller/geometry/rect.h b/engine/src/flutter/impeller/geometry/rect.h index fa33757318d..4c5fbcefffc 100644 --- a/engine/src/flutter/impeller/geometry/rect.h +++ b/engine/src/flutter/impeller/geometry/rect.h @@ -214,6 +214,35 @@ struct TRect { return TRect::MakePointBounds(points.begin(), points.end()).value(); } + /// @brief Constructs a Matrix that will map all points in the coordinate + /// space of the rectangle into a new normalized coordinate space + /// where the upper left corner of the rectangle maps to (0, 0) + /// and the lower right corner of the rectangle maps to (1, 1). + /// + /// Empty and non-finite rectangles will return a zero-scaling + /// transform that maps all points to (0, 0). + constexpr Matrix GetNormalizingTransform() const { + if (!IsEmpty()) { + Scalar sx = 1.0 / size.width; + Scalar sy = 1.0 / size.height; + Scalar tx = origin.x * -sx; + Scalar ty = origin.y * -sy; + + // Exclude NaN and infinities and either scale underflowing to zero + if (sx != 0.0 && sy != 0.0 && 0.0 * sx * sy * tx * ty == 0.0) { + // clang-format off + return Matrix( sx, 0.0f, 0.0f, 0.0f, + 0.0f, sy, 0.0f, 0.0f, + 0.0f, 0.0f, 1.0f, 0.0f, + tx, ty, 0.0f, 1.0f); + // clang-format on + } + } + + // Map all coordinates to the origin. + return Matrix::MakeScale({0.0f, 0.0f, 1.0f}); + } + constexpr TRect Union(const TRect& o) const { auto this_ltrb = GetLTRB(); auto other_ltrb = o.GetLTRB(); diff --git a/engine/src/flutter/impeller/geometry/rect_unittests.cc b/engine/src/flutter/impeller/geometry/rect_unittests.cc index 27bedfab38f..c9abe94b124 100644 --- a/engine/src/flutter/impeller/geometry/rect_unittests.cc +++ b/engine/src/flutter/impeller/geometry/rect_unittests.cc @@ -14,14 +14,14 @@ namespace testing { TEST(RectTest, RectOriginSizeGetters) { { Rect r = Rect::MakeOriginSize({10, 20}, {50, 40}); - ASSERT_EQ(r.GetOrigin(), Point(10, 20)); - ASSERT_EQ(r.GetSize(), Size(50, 40)); + EXPECT_EQ(r.GetOrigin(), Point(10, 20)); + EXPECT_EQ(r.GetSize(), Size(50, 40)); } { Rect r = Rect::MakeLTRB(10, 20, 50, 40); - ASSERT_EQ(r.GetOrigin(), Point(10, 20)); - ASSERT_EQ(r.GetSize(), Size(40, 20)); + EXPECT_EQ(r.GetOrigin(), Point(10, 20)); + EXPECT_EQ(r.GetSize(), Size(40, 20)); } } @@ -44,14 +44,152 @@ TEST(RectTest, RectMakeSize) { Size s(100, 200); IRect r = IRect::MakeSize(s); IRect expected = IRect::MakeLTRB(0, 0, 100, 200); - ASSERT_EQ(r, expected); + EXPECT_EQ(r, expected); } { ISize s(100, 200); IRect r = IRect::MakeSize(s); IRect expected = IRect::MakeLTRB(0, 0, 100, 200); - ASSERT_EQ(r, expected); + EXPECT_EQ(r, expected); + } +} + +TEST(RectTest, RectGetNormalizingTransform) { + { + // Checks for expected matrix values + + auto r = Rect::MakeXYWH(100, 200, 200, 400); + + EXPECT_EQ(r.GetNormalizingTransform(), + Matrix::MakeScale({0.005, 0.0025, 1.0}) * + Matrix::MakeTranslation({-100, -200})); + } + + { + // Checks for expected transformation of points relative to the rect + + auto r = Rect::MakeLTRB(300, 500, 400, 700); + auto m = r.GetNormalizingTransform(); + + // The 4 corners of the rect => (0, 0) to (1, 1) + EXPECT_EQ(m * Point(300, 500), Point(0, 0)); + EXPECT_EQ(m * Point(400, 500), Point(1, 0)); + EXPECT_EQ(m * Point(400, 700), Point(1, 1)); + EXPECT_EQ(m * Point(300, 700), Point(0, 1)); + + // The center => (0.5, 0.5) + EXPECT_EQ(m * Point(350, 600), Point(0.5, 0.5)); + + // Outside the 4 corners => (-1, -1) to (2, 2) + EXPECT_EQ(m * Point(200, 300), Point(-1, -1)); + EXPECT_EQ(m * Point(500, 300), Point(2, -1)); + EXPECT_EQ(m * Point(500, 900), Point(2, 2)); + EXPECT_EQ(m * Point(200, 900), Point(-1, 2)); + } + + { + // Checks for behavior with empty rects + + auto zero = Matrix::MakeScale({0.0, 0.0, 1.0}); + + // Empty for width and/or height == 0 + EXPECT_EQ(Rect::MakeXYWH(10, 10, 0, 10).GetNormalizingTransform(), zero); + EXPECT_EQ(Rect::MakeXYWH(10, 10, 10, 0).GetNormalizingTransform(), zero); + EXPECT_EQ(Rect::MakeXYWH(10, 10, 0, 0).GetNormalizingTransform(), zero); + + // Empty for width and/or height < 0 + EXPECT_EQ(Rect::MakeXYWH(10, 10, -1, 10).GetNormalizingTransform(), zero); + EXPECT_EQ(Rect::MakeXYWH(10, 10, 10, -1).GetNormalizingTransform(), zero); + EXPECT_EQ(Rect::MakeXYWH(10, 10, -1, -1).GetNormalizingTransform(), zero); + } + + { + // Checks for behavior with non-finite rects + + auto z = Matrix::MakeScale({0.0, 0.0, 1.0}); + auto nan = std::numeric_limits::quiet_NaN(); + auto inf = std::numeric_limits::infinity(); + + // Non-finite for width and/or height == nan + EXPECT_EQ(Rect::MakeXYWH(10, 10, nan, 10).GetNormalizingTransform(), z); + EXPECT_EQ(Rect::MakeXYWH(10, 10, 10, nan).GetNormalizingTransform(), z); + EXPECT_EQ(Rect::MakeXYWH(10, 10, nan, nan).GetNormalizingTransform(), z); + + // Non-finite for width and/or height == inf + EXPECT_EQ(Rect::MakeXYWH(10, 10, inf, 10).GetNormalizingTransform(), z); + EXPECT_EQ(Rect::MakeXYWH(10, 10, 10, inf).GetNormalizingTransform(), z); + EXPECT_EQ(Rect::MakeXYWH(10, 10, inf, inf).GetNormalizingTransform(), z); + + // Non-finite for width and/or height == -inf + EXPECT_EQ(Rect::MakeXYWH(10, 10, -inf, 10).GetNormalizingTransform(), z); + EXPECT_EQ(Rect::MakeXYWH(10, 10, 10, -inf).GetNormalizingTransform(), z); + EXPECT_EQ(Rect::MakeXYWH(10, 10, -inf, -inf).GetNormalizingTransform(), z); + + // Non-finite for origin X and/or Y == nan + EXPECT_EQ(Rect::MakeXYWH(nan, 10, 10, 10).GetNormalizingTransform(), z); + EXPECT_EQ(Rect::MakeXYWH(10, nan, 10, 10).GetNormalizingTransform(), z); + EXPECT_EQ(Rect::MakeXYWH(nan, nan, 10, 10).GetNormalizingTransform(), z); + + // Non-finite for origin X and/or Y == inf + EXPECT_EQ(Rect::MakeXYWH(inf, 10, 10, 10).GetNormalizingTransform(), z); + EXPECT_EQ(Rect::MakeXYWH(10, inf, 10, 10).GetNormalizingTransform(), z); + EXPECT_EQ(Rect::MakeXYWH(inf, inf, 10, 10).GetNormalizingTransform(), z); + + // Non-finite for origin X and/or Y == -inf + EXPECT_EQ(Rect::MakeXYWH(-inf, 10, 10, 10).GetNormalizingTransform(), z); + EXPECT_EQ(Rect::MakeXYWH(10, -inf, 10, 10).GetNormalizingTransform(), z); + EXPECT_EQ(Rect::MakeXYWH(-inf, -inf, 10, 10).GetNormalizingTransform(), z); + } +} + +TEST(RectTest, IRectGetNormalizingTransform) { + { + // Checks for expected matrix values + + auto r = IRect::MakeXYWH(100, 200, 200, 400); + + EXPECT_EQ(r.GetNormalizingTransform(), + Matrix::MakeScale({0.005, 0.0025, 1.0}) * + Matrix::MakeTranslation({-100, -200})); + } + + { + // Checks for expected transformation of points relative to the rect + + auto r = IRect::MakeLTRB(300, 500, 400, 700); + auto m = r.GetNormalizingTransform(); + + // The 4 corners of the rect => (0, 0) to (1, 1) + EXPECT_EQ(m * Point(300, 500), Point(0, 0)); + EXPECT_EQ(m * Point(400, 500), Point(1, 0)); + EXPECT_EQ(m * Point(400, 700), Point(1, 1)); + EXPECT_EQ(m * Point(300, 700), Point(0, 1)); + + // The center => (0.5, 0.5) + EXPECT_EQ(m * Point(350, 600), Point(0.5, 0.5)); + + // Outside the 4 corners => (-1, -1) to (2, 2) + EXPECT_EQ(m * Point(200, 300), Point(-1, -1)); + EXPECT_EQ(m * Point(500, 300), Point(2, -1)); + EXPECT_EQ(m * Point(500, 900), Point(2, 2)); + EXPECT_EQ(m * Point(200, 900), Point(-1, 2)); + } + + { + // Checks for behavior with empty rects + + auto zero = Matrix::MakeScale({0.0, 0.0, 1.0}); + + // Empty for width and/or height == 0 + EXPECT_EQ(IRect::MakeXYWH(10, 10, 0, 10).GetNormalizingTransform(), zero); + EXPECT_EQ(IRect::MakeXYWH(10, 10, 10, 0).GetNormalizingTransform(), zero); + EXPECT_EQ(IRect::MakeXYWH(10, 10, 0, 0).GetNormalizingTransform(), zero); + + // Empty for width and/or height < 0 + EXPECT_EQ(IRect::MakeXYWH(10, 10, -1, 10).GetNormalizingTransform(), zero); + EXPECT_EQ(IRect::MakeXYWH(10, 10, 10, -1).GetNormalizingTransform(), zero); + EXPECT_EQ(IRect::MakeXYWH(10, 10, -1, -1).GetNormalizingTransform(), zero); } }