From edba128d440447c87c9dff87c1a5461265a4e6da Mon Sep 17 00:00:00 2001 From: Jim Graham Date: Wed, 1 Nov 2023 19:03:09 -0700 Subject: [PATCH] [Impeller] Move all remaining Rect construction to named factories (flutter/engine#47582) The named factories provide better readability and do not imply an internal storage format (which will be changing soon). --- engine/src/flutter/impeller/aiks/canvas.cc | 5 +-- .../impeller/entity/contents/clip_contents.cc | 7 ++-- .../border_mask_blur_filter_contents.cc | 4 +-- ...rectional_gaussian_blur_filter_contents.cc | 10 ++---- .../filters/morphology_filter_contents.cc | 4 +-- .../entity/contents/tiled_texture_contents.cc | 2 +- .../flutter/impeller/entity/entity_pass.cc | 10 +++--- .../entity/geometry/cover_geometry.cc | 4 +-- .../entity/geometry/stroke_path_geometry.cc | 4 +-- .../impeller/geometry/geometry_unittests.cc | 30 ----------------- engine/src/flutter/impeller/geometry/path.cc | 2 +- .../flutter/impeller/geometry/path_builder.cc | 2 +- engine/src/flutter/impeller/geometry/rect.h | 15 ++++----- .../impeller/geometry/rect_unittests.cc | 32 +++++++++++++++++++ .../impeller/golden_tests/golden_tests.cc | 2 +- .../playground/imgui/imgui_impl_impeller.cc | 8 ++--- .../flutter/impeller/renderer/blit_pass.cc | 2 +- .../flutter/impeller/renderer/render_pass.cc | 2 +- .../src/flutter/impeller/renderer/snapshot.cc | 2 +- .../backends/stb/text_frame_stb.cc | 6 ++-- .../impeller/typographer/text_frame.cc | 12 +++---- 21 files changed, 80 insertions(+), 85 deletions(-) diff --git a/engine/src/flutter/impeller/aiks/canvas.cc b/engine/src/flutter/impeller/aiks/canvas.cc index 2308d5385fb..0cac288dd10 100644 --- a/engine/src/flutter/impeller/aiks/canvas.cc +++ b/engine/src/flutter/impeller/aiks/canvas.cc @@ -271,8 +271,9 @@ void Canvas::DrawRRect(Rect rect, Scalar corner_radius, const Paint& paint) { void Canvas::DrawCircle(Point center, Scalar radius, const Paint& paint) { Size half_size(radius, radius); - if (AttemptDrawBlurredRRect(Rect(center - half_size, half_size * 2), radius, - paint)) { + if (AttemptDrawBlurredRRect( + Rect::MakeOriginSize(center - half_size, half_size * 2), radius, + paint)) { return; } auto circle_path = diff --git a/engine/src/flutter/impeller/entity/contents/clip_contents.cc b/engine/src/flutter/impeller/entity/contents/clip_contents.cc index 241049a0788..c6c173171f3 100644 --- a/engine/src/flutter/impeller/entity/contents/clip_contents.cc +++ b/engine/src/flutter/impeller/entity/contents/clip_contents.cc @@ -93,7 +93,7 @@ bool ClipContents::Render(const ContentContext& renderer, { DEBUG_COMMAND_INFO(cmd, "Difference Clip (Increment)"); - auto points = Rect(Size(pass.GetRenderTargetSize())).GetPoints(); + auto points = Rect::MakeSize(pass.GetRenderTargetSize()).GetPoints(); auto vertices = VertexBufferBuilder{} .AddVertices({{points[0]}, {points[1]}, {points[2]}, {points[3]}}) @@ -188,8 +188,9 @@ bool ClipRestoreContents::Render(const ContentContext& renderer, // Create a rect that covers either the given restore area, or the whole // render target texture. - auto ltrb = restore_coverage_.value_or(Rect(Size(pass.GetRenderTargetSize()))) - .GetLTRB(); + auto ltrb = + restore_coverage_.value_or(Rect::MakeSize(pass.GetRenderTargetSize())) + .GetLTRB(); VertexBufferBuilder vtx_builder; vtx_builder.AddVertices({ {Point(ltrb[0], ltrb[1])}, diff --git a/engine/src/flutter/impeller/entity/contents/filters/border_mask_blur_filter_contents.cc b/engine/src/flutter/impeller/entity/contents/filters/border_mask_blur_filter_contents.cc index 849ffa7b5be..196256c1484 100644 --- a/engine/src/flutter/impeller/entity/contents/filters/border_mask_blur_filter_contents.cc +++ b/engine/src/flutter/impeller/entity/contents/filters/border_mask_blur_filter_contents.cc @@ -164,9 +164,7 @@ std::optional BorderMaskBlurFilterContents::GetFilterCoverage( auto transformed_blur_vector = transform.TransformDirection(Vector2(Radius{sigma_x_}.radius, 0)).Abs() + transform.TransformDirection(Vector2(0, Radius{sigma_y_}.radius)).Abs(); - auto extent = coverage->size + transformed_blur_vector * 2; - return Rect(coverage->origin - transformed_blur_vector, - Size(extent.x, extent.y)); + return coverage->Expand(transformed_blur_vector); } std::optional BorderMaskBlurFilterContents::GetFilterSourceCoverage( diff --git a/engine/src/flutter/impeller/entity/contents/filters/directional_gaussian_blur_filter_contents.cc b/engine/src/flutter/impeller/entity/contents/filters/directional_gaussian_blur_filter_contents.cc index fe5647b1a9a..94145261e57 100644 --- a/engine/src/flutter/impeller/entity/contents/filters/directional_gaussian_blur_filter_contents.cc +++ b/engine/src/flutter/impeller/entity/contents/filters/directional_gaussian_blur_filter_contents.cc @@ -106,12 +106,10 @@ std::optional DirectionalGaussianBlurFilterContents::RenderFilter( std::optional expanded_coverage_hint; if (coverage_hint.has_value()) { auto r = - Size(transformed_blur_radius_length, transformed_blur_radius_length) + Point(transformed_blur_radius_length, transformed_blur_radius_length) .Abs(); expanded_coverage_hint = - is_second_pass_ ? coverage_hint - : Rect(coverage_hint.value().origin - r, - Size(coverage_hint.value().size + r * 2)); + is_second_pass_ ? coverage_hint : coverage_hint->Expand(r); } auto input_snapshot = inputs[0]->GetSnapshot("GaussianBlur", renderer, entity, expanded_coverage_hint); @@ -317,9 +315,7 @@ std::optional DirectionalGaussianBlurFilterContents::GetFilterCoverage( auto transformed_blur_vector = transform.TransformDirection(blur_direction_ * Radius{blur_sigma_}.radius) .Abs(); - auto extent = coverage->size + transformed_blur_vector * 2; - return Rect(coverage->origin - transformed_blur_vector, - Size(extent.x, extent.y)); + return coverage->Expand(transformed_blur_vector); } } // namespace impeller diff --git a/engine/src/flutter/impeller/entity/contents/filters/morphology_filter_contents.cc b/engine/src/flutter/impeller/entity/contents/filters/morphology_filter_contents.cc index 79175b04435..3816a4edc7b 100644 --- a/engine/src/flutter/impeller/entity/contents/filters/morphology_filter_contents.cc +++ b/engine/src/flutter/impeller/entity/contents/filters/morphology_filter_contents.cc @@ -96,7 +96,7 @@ std::optional DirectionalMorphologyFilterContents::RenderFilter( auto transformed_radius = transform.TransformDirection(direction_ * radius_.radius); auto transformed_texture_vertices = - Rect(Size(input_snapshot->texture->GetSize())) + Rect::MakeSize(input_snapshot->texture->GetSize()) .GetTransformedPoints(input_snapshot->transform); auto transformed_texture_width = transformed_texture_vertices[0].GetDistance( @@ -189,7 +189,7 @@ std::optional DirectionalMorphologyFilterContents::GetFilterCoverage( if (size.x < 0 || size.y < 0) { return Rect::MakeSize(Size(0, 0)); } - return Rect(origin, Size(size.x, size.y)); + return Rect::MakeOriginSize(origin, Size(size.x, size.y)); } std::optional diff --git a/engine/src/flutter/impeller/entity/contents/tiled_texture_contents.cc b/engine/src/flutter/impeller/entity/contents/tiled_texture_contents.cc index 22984b478fb..2d18ad1ffa5 100644 --- a/engine/src/flutter/impeller/entity/contents/tiled_texture_contents.cc +++ b/engine/src/flutter/impeller/entity/contents/tiled_texture_contents.cc @@ -128,7 +128,7 @@ bool TiledTextureContents::Render(const ContentContext& renderer, auto& host_buffer = pass.GetTransientsBuffer(); auto geometry_result = GetGeometry()->GetPositionUVBuffer( - Rect({0, 0}, Size(texture_size)), GetInverseEffectTransform(), renderer, + Rect::MakeSize(texture_size), GetInverseEffectTransform(), renderer, entity, pass); bool uses_emulated_tile_mode = UsesEmulatedTileMode(renderer.GetDeviceCapabilities()); diff --git a/engine/src/flutter/impeller/entity/entity_pass.cc b/engine/src/flutter/impeller/entity/entity_pass.cc index af4e46b5d03..f37bb4d4c1c 100644 --- a/engine/src/flutter/impeller/entity/entity_pass.cc +++ b/engine/src/flutter/impeller/entity/entity_pass.cc @@ -569,11 +569,11 @@ EntityPass::EntityResult EntityPass::GetEntityForElement( // The maximum coverage of the subpass. Subpasses textures should never // extend outside the parent pass texture or the current clip coverage. - auto coverage_limit = - Rect(global_pass_position, Size(pass_context.GetPassTarget() - .GetRenderTarget() - .GetRenderTargetSize())) - .Intersection(clip_coverage_back.value()); + auto coverage_limit = Rect::MakeOriginSize(global_pass_position, + Size(pass_context.GetPassTarget() + .GetRenderTarget() + .GetRenderTargetSize())) + .Intersection(clip_coverage_back.value()); if (!coverage_limit.has_value()) { capture.CreateChild("Subpass Entity (Skipped: Empty coverage limit A)"); return EntityPass::EntityResult::Skip(); diff --git a/engine/src/flutter/impeller/entity/geometry/cover_geometry.cc b/engine/src/flutter/impeller/entity/geometry/cover_geometry.cc index 42d3060ebf3..cc7793a0ec4 100644 --- a/engine/src/flutter/impeller/entity/geometry/cover_geometry.cc +++ b/engine/src/flutter/impeller/entity/geometry/cover_geometry.cc @@ -15,7 +15,7 @@ CoverGeometry::~CoverGeometry() = default; GeometryResult CoverGeometry::GetPositionBuffer(const ContentContext& renderer, const Entity& entity, RenderPass& pass) { - auto rect = Rect(Size(pass.GetRenderTargetSize())); + auto rect = Rect::MakeSize(pass.GetRenderTargetSize()); constexpr uint16_t kRectIndicies[4] = {0, 1, 2, 3}; auto& host_buffer = pass.GetTransientsBuffer(); return GeometryResult{ @@ -44,7 +44,7 @@ GeometryResult CoverGeometry::GetPositionUVBuffer( const ContentContext& renderer, const Entity& entity, RenderPass& pass) { - auto rect = Rect(Size(pass.GetRenderTargetSize())); + auto rect = Rect::MakeSize(pass.GetRenderTargetSize()); return ComputeUVGeometryForRect(rect, texture_coverage, effect_transform, renderer, entity, pass); } diff --git a/engine/src/flutter/impeller/entity/geometry/stroke_path_geometry.cc b/engine/src/flutter/impeller/entity/geometry/stroke_path_geometry.cc index 144fddcef13..9f857d5808a 100644 --- a/engine/src/flutter/impeller/entity/geometry/stroke_path_geometry.cc +++ b/engine/src/flutter/impeller/entity/geometry/stroke_path_geometry.cc @@ -522,9 +522,7 @@ std::optional StrokePathGeometry::GetCoverage( .TransformDirection(Vector2(max_radius, max_radius) * std::max(stroke_width_, min_size)) .Abs(); - return Rect(path_coverage.origin - max_radius_xy, - Size(path_coverage.size.width + max_radius_xy.x * 2, - path_coverage.size.height + max_radius_xy.y * 2)); + return path_coverage.Expand(max_radius_xy); } } // namespace impeller diff --git a/engine/src/flutter/impeller/geometry/geometry_unittests.cc b/engine/src/flutter/impeller/geometry/geometry_unittests.cc index d81ec54131c..6dc6bd0f741 100644 --- a/engine/src/flutter/impeller/geometry/geometry_unittests.cc +++ b/engine/src/flutter/impeller/geometry/geometry_unittests.cc @@ -1580,36 +1580,6 @@ TEST(GeometryTest, CanConvertBetweenDegressAndRadians) { } } -TEST(GeometryTest, RectMakeSize) { - { - Size s(100, 200); - Rect r = Rect::MakeSize(s); - Rect expected = Rect::MakeLTRB(0, 0, 100, 200); - ASSERT_RECT_NEAR(r, expected); - } - - { - ISize s(100, 200); - Rect r = Rect::MakeSize(s); - Rect expected = Rect::MakeLTRB(0, 0, 100, 200); - ASSERT_RECT_NEAR(r, expected); - } - - { - Size s(100, 200); - IRect r = IRect::MakeSize(s); - IRect expected = IRect::MakeLTRB(0, 0, 100, 200); - ASSERT_EQ(r, expected); - } - - { - ISize s(100, 200); - IRect r = IRect::MakeSize(s); - IRect expected = IRect::MakeLTRB(0, 0, 100, 200); - ASSERT_EQ(r, expected); - } -} - TEST(GeometryTest, RectUnion) { { Rect a = Rect::MakeXYWH(100, 100, 100, 100); diff --git a/engine/src/flutter/impeller/geometry/path.cc b/engine/src/flutter/impeller/geometry/path.cc index cfd770ad4a6..6158c06bd2b 100644 --- a/engine/src/flutter/impeller/geometry/path.cc +++ b/engine/src/flutter/impeller/geometry/path.cc @@ -425,7 +425,7 @@ void Path::ComputeBounds() { auto min = min_max->first; auto max = min_max->second; const auto difference = max - min; - computed_bounds_ = Rect{min.x, min.y, difference.x, difference.y}; + computed_bounds_ = Rect::MakeXYWH(min.x, min.y, difference.x, difference.y); } std::optional Path::GetTransformedBoundingBox( diff --git a/engine/src/flutter/impeller/geometry/path_builder.cc b/engine/src/flutter/impeller/geometry/path_builder.cc index da851dd24cb..9495cb5328c 100644 --- a/engine/src/flutter/impeller/geometry/path_builder.cc +++ b/engine/src/flutter/impeller/geometry/path_builder.cc @@ -196,7 +196,7 @@ PathBuilder& PathBuilder::AddRect(Rect rect) { } PathBuilder& PathBuilder::AddCircle(const Point& c, Scalar r) { - return AddOval(Rect{c.x - r, c.y - r, 2.0f * r, 2.0f * r}); + return AddOval(Rect::MakeXYWH(c.x - r, c.y - r, 2.0f * r, 2.0f * r)); } PathBuilder& PathBuilder::AddRoundedRect(Rect rect, Scalar radius) { diff --git a/engine/src/flutter/impeller/geometry/rect.h b/engine/src/flutter/impeller/geometry/rect.h index 108aca97e43..3f6cdceb049 100644 --- a/engine/src/flutter/impeller/geometry/rect.h +++ b/engine/src/flutter/impeller/geometry/rect.h @@ -25,14 +25,6 @@ struct TRect { constexpr TRect() : origin({0, 0}), size({0, 0}) {} - constexpr TRect(TSize size) : origin({0.0, 0.0}), size(size) {} - - constexpr TRect(TPoint origin, TSize size) - : origin(origin), size(size) {} - - constexpr TRect(Type x, Type y, Type width, Type height) - : origin(x, y), size(width, height) {} - constexpr static TRect MakeLTRB(Type left, Type top, Type right, @@ -363,6 +355,13 @@ struct TRect { const std::optional b) { return a.has_value() ? Intersection(a.value(), b) : b; } + + private: + constexpr TRect(Type x, Type y, Type width, Type height) + : origin(x, y), size(width, height) {} + + constexpr TRect(TPoint origin, TSize size) + : origin(origin), size(size) {} }; using Rect = TRect; diff --git a/engine/src/flutter/impeller/geometry/rect_unittests.cc b/engine/src/flutter/impeller/geometry/rect_unittests.cc index f24edd58dda..583c04fee64 100644 --- a/engine/src/flutter/impeller/geometry/rect_unittests.cc +++ b/engine/src/flutter/impeller/geometry/rect_unittests.cc @@ -6,6 +6,8 @@ #include "flutter/impeller/geometry/rect.h" +#include "flutter/impeller/geometry/geometry_asserts.h" + namespace impeller { namespace testing { @@ -23,5 +25,35 @@ TEST(RectTest, RectOriginSizeGetters) { } } +TEST(RectTest, RectMakeSize) { + { + Size s(100, 200); + Rect r = Rect::MakeSize(s); + Rect expected = Rect::MakeLTRB(0, 0, 100, 200); + ASSERT_RECT_NEAR(r, expected); + } + + { + ISize s(100, 200); + Rect r = Rect::MakeSize(s); + Rect expected = Rect::MakeLTRB(0, 0, 100, 200); + ASSERT_RECT_NEAR(r, expected); + } + + { + Size s(100, 200); + IRect r = IRect::MakeSize(s); + IRect expected = IRect::MakeLTRB(0, 0, 100, 200); + ASSERT_EQ(r, expected); + } + + { + ISize s(100, 200); + IRect r = IRect::MakeSize(s); + IRect expected = IRect::MakeLTRB(0, 0, 100, 200); + ASSERT_EQ(r, expected); + } +} + } // namespace testing } // namespace impeller diff --git a/engine/src/flutter/impeller/golden_tests/golden_tests.cc b/engine/src/flutter/impeller/golden_tests/golden_tests.cc index 9919aa5160f..84b6c6fcda0 100644 --- a/engine/src/flutter/impeller/golden_tests/golden_tests.cc +++ b/engine/src/flutter/impeller/golden_tests/golden_tests.cc @@ -78,7 +78,7 @@ TEST_F(GoldenTests, ConicalGradient) { paint.stroke_width = 0.0; paint.style = Paint::Style::kFill; - canvas.DrawRect(Rect(10, 10, 250, 250), paint); + canvas.DrawRect(Rect::MakeXYWH(10, 10, 250, 250), paint); Picture picture = canvas.EndRecordingAsPicture(); auto aiks_context = diff --git a/engine/src/flutter/impeller/playground/imgui/imgui_impl_impeller.cc b/engine/src/flutter/impeller/playground/imgui/imgui_impl_impeller.cc index ada5f83dfa3..449c9c7cf76 100644 --- a/engine/src/flutter/impeller/playground/imgui/imgui_impl_impeller.cc +++ b/engine/src/flutter/impeller/playground/imgui/imgui_impl_impeller.cc @@ -145,12 +145,12 @@ void ImGui_ImplImpeller_RenderDrawData(ImDrawData* draw_data, auto buffer = bd->context->GetResourceAllocator()->CreateBuffer(buffer_desc); buffer->SetLabel(impeller::SPrintF("ImGui vertex+index buffer")); - auto display_rect = - impeller::Rect(draw_data->DisplayPos.x, draw_data->DisplayPos.y, - draw_data->DisplaySize.x, draw_data->DisplaySize.y); + auto display_rect = impeller::Rect::MakeXYWH( + draw_data->DisplayPos.x, draw_data->DisplayPos.y, + draw_data->DisplaySize.x, draw_data->DisplaySize.y); auto viewport = impeller::Viewport{ - .rect = impeller::Rect( + .rect = impeller::Rect::MakeXYWH( display_rect.origin.x * draw_data->FramebufferScale.x, display_rect.origin.y * draw_data->FramebufferScale.y, display_rect.size.width * draw_data->FramebufferScale.x, diff --git a/engine/src/flutter/impeller/renderer/blit_pass.cc b/engine/src/flutter/impeller/renderer/blit_pass.cc index 340ec36bd81..b8644d1dcc7 100644 --- a/engine/src/flutter/impeller/renderer/blit_pass.cc +++ b/engine/src/flutter/impeller/renderer/blit_pass.cc @@ -66,7 +66,7 @@ bool BlitPass::AddCopy(std::shared_ptr source, // Clip the destination image. source_region = source_region->Intersection( - IRect(-destination_origin, destination->GetSize())); + IRect::MakeOriginSize(-destination_origin, destination->GetSize())); if (!source_region.has_value()) { return true; // Nothing to blit. } diff --git a/engine/src/flutter/impeller/renderer/render_pass.cc b/engine/src/flutter/impeller/renderer/render_pass.cc index d07ce2bfaa7..3f9c0fc556a 100644 --- a/engine/src/flutter/impeller/renderer/render_pass.cc +++ b/engine/src/flutter/impeller/renderer/render_pass.cc @@ -50,7 +50,7 @@ bool RenderPass::AddCommand(Command&& command) { } if (command.scissor.has_value()) { - auto target_rect = IRect({}, render_target_.GetRenderTargetSize()); + auto target_rect = IRect::MakeSize(render_target_.GetRenderTargetSize()); if (!target_rect.Contains(command.scissor.value())) { VALIDATION_LOG << "Cannot apply a scissor that lies outside the bounds " "of the render target."; diff --git a/engine/src/flutter/impeller/renderer/snapshot.cc b/engine/src/flutter/impeller/renderer/snapshot.cc index f5614e72cd6..a73a2102de7 100644 --- a/engine/src/flutter/impeller/renderer/snapshot.cc +++ b/engine/src/flutter/impeller/renderer/snapshot.cc @@ -12,7 +12,7 @@ std::optional Snapshot::GetCoverage() const { if (!texture) { return std::nullopt; } - return Rect(Size(texture->GetSize())).TransformBounds(transform); + return Rect::MakeSize(texture->GetSize()).TransformBounds(transform); } std::optional Snapshot::GetUVTransform() const { diff --git a/engine/src/flutter/impeller/typographer/backends/stb/text_frame_stb.cc b/engine/src/flutter/impeller/typographer/backends/stb/text_frame_stb.cc index e1d1dd0d1c6..23811b7eae8 100644 --- a/engine/src/flutter/impeller/typographer/backends/stb/text_frame_stb.cc +++ b/engine/src/flutter/impeller/typographer/backends/stb/text_frame_stb.cc @@ -55,9 +55,9 @@ std::shared_ptr MakeTextFrameSTB( std::optional result; for (const auto& glyph_position : run.GetGlyphPositions()) { - Rect glyph_rect = - Rect(glyph_position.position + glyph_position.glyph.bounds.origin, - glyph_position.glyph.bounds.size); + Rect glyph_rect = Rect::MakeOriginSize( + glyph_position.position + glyph_position.glyph.bounds.origin, + glyph_position.glyph.bounds.size); result = result.has_value() ? result->Union(glyph_rect) : glyph_rect; } diff --git a/engine/src/flutter/impeller/typographer/text_frame.cc b/engine/src/flutter/impeller/typographer/text_frame.cc index 3f82c1d239a..6d3406e39f3 100644 --- a/engine/src/flutter/impeller/typographer/text_frame.cc +++ b/engine/src/flutter/impeller/typographer/text_frame.cc @@ -45,14 +45,14 @@ bool TextFrame::MaybeHasOverlapping() const { // accumulated bounds rect. This gives faster but less precise information // on text runs. auto first_position = glyph_positions[0]; - auto overlapping_rect = - Rect(first_position.position + first_position.glyph.bounds.origin, - first_position.glyph.bounds.size); + auto overlapping_rect = Rect::MakeOriginSize( + first_position.position + first_position.glyph.bounds.origin, + first_position.glyph.bounds.size); for (auto i = 1u; i < glyph_positions.size(); i++) { auto glyph_position = glyph_positions[i]; - auto glyph_rect = - Rect(glyph_position.position + glyph_position.glyph.bounds.origin, - glyph_position.glyph.bounds.size); + auto glyph_rect = Rect::MakeOriginSize( + glyph_position.position + glyph_position.glyph.bounds.origin, + glyph_position.glyph.bounds.size); auto intersection = glyph_rect.Intersection(overlapping_rect); if (intersection.has_value()) { return true;