From 751924dbc92b04bd022aea446908950f66fa8325 Mon Sep 17 00:00:00 2001 From: gaaclarke <30870216+gaaclarke@users.noreply.github.com> Date: Fri, 28 Mar 2025 12:45:06 -0700 Subject: [PATCH] [impeller] refactored LineContents to make it more testable, added tests (#166035) issue https://github.com/flutter/flutter/issues/165994 This makes the sample radius properly fixed in pixel coordinates. I don't know if we want to scale them based on dpi. This tweak also made it so we no longer have lines that disappear with the setting (width:2, scale:0.2). We have proper unit tests so we can reason about this stuff correctly. ## Pre-launch Checklist - [x] I read the [Contributor Guide] and followed the process outlined there for submitting PRs. - [x] I read the [Tree Hygiene] wiki page, which explains my responsibilities. - [x] I read and followed the [Flutter Style Guide], including [Features we expect every widget to implement]. - [x] I signed the [CLA]. - [x] I listed at least one issue that this PR fixes in the description above. - [x] I updated/added relevant documentation (doc comments with `///`). - [x] I added new tests to check the change I am making, or this PR is [test-exempt]. - [x] I followed the [breaking change policy] and added [Data Driven Fixes] where supported. - [x] All existing and new tests are passing. If you need help, consider asking for advice on the #hackers-new channel on [Discord]. [Contributor Guide]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#overview [Tree Hygiene]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md [test-exempt]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#tests [Flutter Style Guide]: https://github.com/flutter/flutter/blob/main/docs/contributing/Style-guide-for-Flutter-repo.md [Features we expect every widget to implement]: https://github.com/flutter/flutter/blob/main/docs/contributing/Style-guide-for-Flutter-repo.md#features-we-expect-every-widget-to-implement [CLA]: https://cla.developers.google.com/ [flutter/tests]: https://github.com/flutter/tests [breaking change policy]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#handling-breaking-changes [Discord]: https://github.com/flutter/flutter/blob/main/docs/contributing/Chat.md [Data Driven Fixes]: https://github.com/flutter/flutter/blob/main/docs/contributing/Data-driven-Fixes.md --- engine/src/flutter/engine.code-workspace | 4 +- .../display_list/aiks_dl_path_unittests.cc | 40 +++- .../impeller/entity/contents/line_contents.cc | 207 ++++++++++-------- .../impeller/entity/contents/line_contents.h | 11 + .../contents/line_contents_unittests.cc | 83 +++++++ 5 files changed, 242 insertions(+), 103 deletions(-) diff --git a/engine/src/flutter/engine.code-workspace b/engine/src/flutter/engine.code-workspace index c144ddc94c..544677d98b 100644 --- a/engine/src/flutter/engine.code-workspace +++ b/engine/src/flutter/engine.code-workspace @@ -125,7 +125,9 @@ "csetjmp": "cpp", "cfenv": "cpp", "execution": "cpp", - "print": "cpp" + "print": "cpp", + "source_location": "cpp", + "syncstream": "cpp" }, "C_Cpp.default.includePath": [ "${default}", diff --git a/engine/src/flutter/impeller/display_list/aiks_dl_path_unittests.cc b/engine/src/flutter/impeller/display_list/aiks_dl_path_unittests.cc index 8d0e905c96..1ee8ddebab 100644 --- a/engine/src/flutter/impeller/display_list/aiks_dl_path_unittests.cc +++ b/engine/src/flutter/impeller/display_list/aiks_dl_path_unittests.cc @@ -399,18 +399,38 @@ TEST_P(AiksTest, ScaleExperimentAntialiasLines) { DisplayListBuilder builder; builder.Scale(GetContentScale().x, GetContentScale().y); - DlPaint paint; - paint.setColor(DlColor::kGreenYellow()); - paint.setStrokeWidth(line_width); + builder.DrawPaint(DlPaint(DlColor(0xff111111))); - builder.DrawLine(DlPoint(100, 100), DlPoint(350, 100), paint); - builder.DrawLine(DlPoint(100, 100), DlPoint(350, 150), paint); + { + DlPaint paint; + paint.setColor(DlColor::kGreenYellow()); + paint.setStrokeWidth(line_width); - builder.Translate(100, 300); - builder.Scale(scale, scale); - builder.Translate(-100, -300); - builder.DrawLine(DlPoint(100, 300), DlPoint(350, 300), paint); - builder.DrawLine(DlPoint(100, 300), DlPoint(350, 450), paint); + builder.DrawLine(DlPoint(100, 100), DlPoint(350, 100), paint); + builder.DrawLine(DlPoint(100, 100), DlPoint(350, 150), paint); + + builder.Save(); + builder.Translate(100, 300); + builder.Scale(scale, scale); + builder.Translate(-100, -300); + builder.DrawLine(DlPoint(100, 300), DlPoint(350, 300), paint); + builder.DrawLine(DlPoint(100, 300), DlPoint(350, 450), paint); + builder.Restore(); + } + + { + DlPaint paint; + paint.setColor(DlColor::kGreenYellow()); + paint.setStrokeWidth(2.0); + + builder.Save(); + builder.Translate(100, 500); + builder.Scale(0.2, 0.2); + builder.Translate(-100, -500); + builder.DrawLine(DlPoint(100, 500), DlPoint(350, 500), paint); + builder.DrawLine(DlPoint(100, 500), DlPoint(350, 650), paint); + builder.Restore(); + } return builder.Build(); }; diff --git a/engine/src/flutter/impeller/entity/contents/line_contents.cc b/engine/src/flutter/impeller/entity/contents/line_contents.cc index 2c74dabd17..bf4be1d35f 100644 --- a/engine/src/flutter/impeller/entity/contents/line_contents.cc +++ b/engine/src/flutter/impeller/entity/contents/line_contents.cc @@ -24,7 +24,64 @@ using CreateGeometryCallback = const Geometry* geometry)>; const int32_t kCurveResolution = 32; -const float kSampleRadius = 1.f; + +uint8_t DoubleToUint8(double x) { + return static_cast(std::clamp(std::round(x * 255.0), 0.0, 255.0)); +} + +/// See also: CreateGradientTexture +std::shared_ptr CreateCurveTexture( + Scalar width, + Scalar radius, + Scalar scale, + const std::shared_ptr& context) { + // + impeller::TextureDescriptor texture_descriptor; + texture_descriptor.storage_mode = impeller::StorageMode::kHostVisible; + texture_descriptor.format = PixelFormat::kR8UNormInt; + texture_descriptor.size = {kCurveResolution, 1}; + + std::vector curve_data = + LineContents::CreateCurveData(width, radius, scale); + + return CreateTexture(texture_descriptor, curve_data, context, "LineCurve"); +} + +GeometryResult CreateGeometry(const ContentContext& renderer, + const Entity& entity, + RenderPass& pass, + const Geometry* geometry) { + using PerVertexData = LineVertexShader::PerVertexData; + const LineGeometry* line_geometry = + static_cast(geometry); + + auto& transform = entity.GetTransform(); + auto& host_buffer = renderer.GetTransientsBuffer(); + + size_t count = 4; + fml::Status calculate_status; + BufferView vertex_buffer = host_buffer.Emplace( + count * sizeof(PerVertexData), alignof(PerVertexData), + [line_geometry, &transform, &calculate_status](uint8_t* buffer) { + auto vertices = reinterpret_cast(buffer); + calculate_status = LineContents::CalculatePerVertex( + vertices, line_geometry, transform); + }); + if (!calculate_status.ok()) { + return kEmptyResult; + } + + return GeometryResult{ + .type = PrimitiveType::kTriangleStrip, + .vertex_buffer = + { + .vertex_buffer = vertex_buffer, + .vertex_count = count, + .index_type = IndexType::kNone, + }, + .transform = entity.GetShaderTransform(pass), + }; +} struct LineInfo { Vector3 e0; @@ -55,99 +112,10 @@ LineInfo CalculateLineInfo(Point p0, Point p1, Scalar width, Scalar radius) { }; } -uint8_t DoubleToUint8(double x) { - return static_cast(std::clamp(std::round(x * 255.0), 0.0, 255.0)); -} - -/// See also: CreateGradientTexture -std::shared_ptr CreateCurveTexture( - Scalar width, - Scalar radius, - Scalar scale, - const std::shared_ptr& context) { - // - impeller::TextureDescriptor texture_descriptor; - texture_descriptor.storage_mode = impeller::StorageMode::kHostVisible; - texture_descriptor.format = PixelFormat::kR8UNormInt; - texture_descriptor.size = {kCurveResolution, 1}; - - std::vector curve_data; - curve_data.reserve(kCurveResolution); - for (int i = 0; i < kCurveResolution; ++i) { - double norm = (static_cast(i) + 1.0) / 32.0; - double loc = scale * norm * (radius + width / 2.0); - double den = radius * 2.0 + 1.0; - curve_data.push_back(DoubleToUint8(loc / den)); - } - - return CreateTexture(texture_descriptor, curve_data, context, "LineCurve"); -} - -GeometryResult CreateGeometry(const ContentContext& renderer, - const Entity& entity, - RenderPass& pass, - const Geometry* geometry) { - using PerVertexData = LineVertexShader::PerVertexData; - const LineGeometry* line_geometry = - static_cast(geometry); - - auto& transform = entity.GetTransform(); - - Point corners[4]; - if (!LineGeometry::ComputeCorners( - corners, transform, - /*extend_endpoints=*/line_geometry->GetCap() != Cap::kButt, - line_geometry->GetP0(), line_geometry->GetP1(), - line_geometry->GetWidth() + kSampleRadius * 2.0)) { - return kEmptyResult; - } - - auto& host_buffer = renderer.GetTransientsBuffer(); - - size_t count = 4; - Scalar scale = entity.GetTransform().GetMaxBasisLengthXY(); - LineInfo line_info = - CalculateLineInfo(line_geometry->GetP0(), line_geometry->GetP1(), - line_geometry->GetWidth(), kSampleRadius); - BufferView vertex_buffer = host_buffer.Emplace( - count * sizeof(PerVertexData), alignof(PerVertexData), - [&corners, &line_info](uint8_t* buffer) { - auto vertices = reinterpret_cast(buffer); - for (auto& corner : corners) { - *vertices++ = { - .position = corner, - .e0 = line_info.e0, - .e1 = line_info.e1, - .e2 = line_info.e2, - .e3 = line_info.e3, - }; - } - }); - - std::shared_ptr curve_texture = CreateCurveTexture( - line_geometry->GetWidth(), kSampleRadius, scale, renderer.GetContext()); - - SamplerDescriptor sampler_desc; - sampler_desc.min_filter = MinMagFilter::kLinear; - sampler_desc.mag_filter = MinMagFilter::kLinear; - - FS::BindCurve( - pass, curve_texture, - renderer.GetContext()->GetSamplerLibrary()->GetSampler(sampler_desc)); - - return GeometryResult{ - .type = PrimitiveType::kTriangleStrip, - .vertex_buffer = - { - .vertex_buffer = vertex_buffer, - .vertex_count = count, - .index_type = IndexType::kNone, - }, - .transform = entity.GetShaderTransform(pass), - }; -} } // namespace +const Scalar LineContents::kSampleRadius = 1.f; + std::unique_ptr LineContents::Make( std::unique_ptr geometry, Color color) { @@ -167,6 +135,18 @@ bool LineContents::Render(const ContentContext& renderer, FS::FragInfo frag_info; frag_info.color = color_; + Scalar scale = entity.GetTransform().GetMaxBasisLengthXY(); + std::shared_ptr curve_texture = CreateCurveTexture( + geometry_->GetWidth(), kSampleRadius, scale, renderer.GetContext()); + + SamplerDescriptor sampler_desc; + sampler_desc.min_filter = MinMagFilter::kLinear; + sampler_desc.mag_filter = MinMagFilter::kLinear; + + FS::BindCurve( + pass, curve_texture, + renderer.GetContext()->GetSamplerLibrary()->GetSampler(sampler_desc)); + PipelineBuilderCallback pipeline_callback = [&renderer](ContentContextOptions options) { return renderer.GetLinePipeline(options); @@ -188,4 +168,47 @@ std::optional LineContents::GetCoverage(const Entity& entity) const { return geometry_->GetCoverage(entity.GetTransform()); } +std::vector LineContents::CreateCurveData(Scalar width, + Scalar radius, + Scalar scale) { + std::vector curve_data; + curve_data.reserve(kCurveResolution); + // More simply written as rise / run: + // double slope = 1.0 / ((radius * 2) / (scale * width + radius)); + double slope = (scale * width + radius) / (radius * 2); + for (int i = 0; i < kCurveResolution; ++i) { + double norm = + (static_cast(i)) / static_cast(kCurveResolution - 1); + double scaled = slope * norm; + curve_data.push_back(DoubleToUint8(scaled)); + } + return curve_data; +} + +fml::Status LineContents::CalculatePerVertex( + LineVertexShader::PerVertexData* per_vertex, + const LineGeometry* geometry, + const Matrix& entity_transform) { + Point corners[4]; + if (!LineGeometry::ComputeCorners( + corners, entity_transform, + /*extend_endpoints=*/geometry->GetCap() != Cap::kButt, + geometry->GetP0(), geometry->GetP1(), + geometry->GetWidth() + kSampleRadius * 2.0)) { + return fml::Status(fml::StatusCode::kAborted, "No valid corners"); + } + LineInfo line_info = CalculateLineInfo(geometry->GetP0(), geometry->GetP1(), + geometry->GetWidth(), kSampleRadius); + for (auto& corner : corners) { + *per_vertex++ = { + .position = corner, + .e0 = line_info.e0, + .e1 = line_info.e1, + .e2 = line_info.e2, + .e3 = line_info.e3, + }; + } + + return {}; +} } // namespace impeller diff --git a/engine/src/flutter/impeller/entity/contents/line_contents.h b/engine/src/flutter/impeller/entity/contents/line_contents.h index a04ff0bf11..61d7cdb260 100644 --- a/engine/src/flutter/impeller/entity/contents/line_contents.h +++ b/engine/src/flutter/impeller/entity/contents/line_contents.h @@ -9,10 +9,21 @@ #include "flutter/impeller/entity/contents/contents.h" #include "flutter/impeller/entity/geometry/line_geometry.h" +#include "impeller/entity/line.vert.h" namespace impeller { class LineContents : public Contents { public: + static const Scalar kSampleRadius; + static std::vector CreateCurveData(Scalar width, + Scalar radius, + Scalar scale); + + static fml::Status CalculatePerVertex( + LineVertexShader::PerVertexData* per_vertex, + const LineGeometry* geometry, + const Matrix& entity_transform); + static std::unique_ptr Make( std::unique_ptr geometry, Color color); diff --git a/engine/src/flutter/impeller/entity/contents/line_contents_unittests.cc b/engine/src/flutter/impeller/entity/contents/line_contents_unittests.cc index d20b20fcdb..cca5cc82af 100644 --- a/engine/src/flutter/impeller/entity/contents/line_contents_unittests.cc +++ b/engine/src/flutter/impeller/entity/contents/line_contents_unittests.cc @@ -3,11 +3,37 @@ // found in the LICENSE file. #include "impeller/entity/contents/line_contents.h" + +#include + +#include "impeller/geometry/geometry_asserts.h" #include "third_party/googletest/googletest/include/gtest/gtest.h" namespace impeller { namespace testing { +namespace { +float lookup(Scalar x) { + return std::clamp(x, /*lo=*/0.f, /*hi=*/1.f); +} + +// This mirrors the function in line.frag. +float CalculateLine(const LineVertexShader::PerVertexData& per_vertex, + Point position) { + Vector3 pos = Vector3(position.x, position.y, 1.0); + Scalar d[4] = {pos.Dot(per_vertex.e0), pos.Dot(per_vertex.e1), + pos.Dot(per_vertex.e2), pos.Dot(per_vertex.e3)}; + + for (int i = 0; i < 4; ++i) { + if (d[i] < 0.f) { + return 0.0; + } + } + + return lookup(std::min(d[0], d[2])) * lookup(std::min(d[1], d[3])); +} +} // namespace + TEST(LineContents, Create) { Path path; Scalar width = 5.0f; @@ -29,5 +55,62 @@ TEST(LineContents, Create) { } } +TEST(LineContents, CalculatePerVertex) { + LineVertexShader::PerVertexData per_vertex[4]; + auto geometry = std::make_unique( + /*p0=*/Point{100, 100}, // + /*p1=*/Point{200, 100}, // + /*width=*/5.f, // + /*cap=*/Cap::kButt); + Matrix transform; + + fml::Status status = + LineContents::CalculatePerVertex(per_vertex, geometry.get(), transform); + Scalar offset = + (LineContents::kSampleRadius * 2.0 + geometry->GetWidth()) / 2.f; + ASSERT_TRUE(status.ok()); + EXPECT_POINT_NEAR(per_vertex[0].position, Point(100, 100 + offset)); + EXPECT_POINT_NEAR(per_vertex[1].position, Point(200, 100 + offset)); + EXPECT_POINT_NEAR(per_vertex[2].position, Point(100, 100 - offset)); + EXPECT_POINT_NEAR(per_vertex[3].position, Point(200, 100 - offset)); + + for (int i = 1; i < 4; ++i) { + EXPECT_VECTOR3_NEAR(per_vertex[0].e0, per_vertex[i].e0) << i; + EXPECT_VECTOR3_NEAR(per_vertex[0].e1, per_vertex[i].e1) << i; + EXPECT_VECTOR3_NEAR(per_vertex[0].e2, per_vertex[i].e2) << i; + EXPECT_VECTOR3_NEAR(per_vertex[0].e3, per_vertex[i].e3) << i; + } + + EXPECT_EQ(CalculateLine(per_vertex[0], Point(0, 0)), 0.f); + EXPECT_NEAR(CalculateLine(per_vertex[0], Point(150, 100 + offset)), 0.f, + kEhCloseEnough); + EXPECT_NEAR(CalculateLine(per_vertex[0], Point(150, 100 + offset * 0.5)), + 0.5f, kEhCloseEnough); + EXPECT_NEAR(CalculateLine(per_vertex[0], Point(150, 100)), 1.f, + kEhCloseEnough); +} + +TEST(LineContents, CreateCurveData) { + std::vector data = LineContents::CreateCurveData(/*width=*/31, + /*radius=*/1, + /*scale=*/1); + EXPECT_EQ(data.size(), 32u); + EXPECT_NEAR(data[0] / 255.f, 0.f, kEhCloseEnough); + EXPECT_NEAR(data[1] / 255.f, 0.5f, 0.02); + EXPECT_NEAR(data[2] / 255.f, 1.f, kEhCloseEnough); + EXPECT_NEAR(data[3] / 255.f, 1.f, kEhCloseEnough); +} + +TEST(LineContents, CreateCurveDataScaled) { + std::vector data = LineContents::CreateCurveData(/*width=*/15.5, + /*radius=*/1, + /*scale=*/2); + EXPECT_EQ(data.size(), 32u); + EXPECT_NEAR(data[0] / 255.f, 0.f, kEhCloseEnough); + EXPECT_NEAR(data[1] / 255.f, 0.5f, 0.02); + EXPECT_NEAR(data[2] / 255.f, 1.f, kEhCloseEnough); + EXPECT_NEAR(data[3] / 255.f, 1.f, kEhCloseEnough); +} + } // namespace testing } // namespace impeller