Skip to content
This repository was archived by the owner on Feb 25, 2025. It is now read-only.

[Impeller] Ensure known geometry has simple bounds computation. #46623

Merged
merged 3 commits into from
Oct 6, 2023
Merged

[Impeller] Ensure known geometry has simple bounds computation. #46623

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
a144fe2
[Impeller] Ensure known geometry has simple bounds computation.
jonahwilliams Oct 6, 2023
9ebd781
Add unittest.
jonahwilliams Oct 6, 2023
b186d02
++
jonahwilliams Oct 6, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 9 additions & 4 deletions impeller/aiks/canvas.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -252,6 +252,7 @@ void Canvas::DrawRRect(Rect rect, Scalar corner_radius, const Paint& paint) {
auto path = PathBuilder{}
.SetConvexity(Convexity::kConvex)
.AddRoundedRect(rect, corner_radius)
.SetBounds(rect)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So, the gist is that if you don't call SetBounds, it is getting calculated instead? What about rounded rects that have extreme corner radii? Does that become a shape whose bounds are different than the one specified in the draw call?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, normally we use the Skia computed bounds - but the Aiks API doesn't accomidated that. What are you thinking of for extreme corner radii?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Running some tests:

  for (auto i = - 100; i < 100; i += 10) {
    SkRRect rrect = SkRRect::MakeRectXY(SkRect::MakeLTRB(0, 0, 100, 100), i, i);
    SkPath path = SkPath().addRRect(rrect);
    auto bds = path.getBounds();
    FML_LOG(ERROR) << "Bounds for RRect with radius " << i << " -> " << bds.x() << "," << bds.y() << "m" << bds.width() << "," << bds.height();
  }

Result:

[ERROR:flutter/impeller/display_list/dl_unittests.cc(1748)] Bounds for RRect with radius -100 -> 0,0m100,100
[ERROR:flutter/impeller/display_list/dl_unittests.cc(1748)] Bounds for RRect with radius -90 -> 0,0m100,100
[ERROR:flutter/impeller/display_list/dl_unittests.cc(1748)] Bounds for RRect with radius -80 -> 0,0m100,100
[ERROR:flutter/impeller/display_list/dl_unittests.cc(1748)] Bounds for RRect with radius -70 -> 0,0m100,100
[ERROR:flutter/impeller/display_list/dl_unittests.cc(1748)] Bounds for RRect with radius -60 -> 0,0m100,100
[ERROR:flutter/impeller/display_list/dl_unittests.cc(1748)] Bounds for RRect with radius -50 -> 0,0m100,100
[ERROR:flutter/impeller/display_list/dl_unittests.cc(1748)] Bounds for RRect with radius -40 -> 0,0m100,100
[ERROR:flutter/impeller/display_list/dl_unittests.cc(1748)] Bounds for RRect with radius -30 -> 0,0m100,100
[ERROR:flutter/impeller/display_list/dl_unittests.cc(1748)] Bounds for RRect with radius -20 -> 0,0m100,100
[ERROR:flutter/impeller/display_list/dl_unittests.cc(1748)] Bounds for RRect with radius -10 -> 0,0m100,100
[ERROR:flutter/impeller/display_list/dl_unittests.cc(1748)] Bounds for RRect with radius 0 -> 0,0m100,100
[ERROR:flutter/impeller/display_list/dl_unittests.cc(1748)] Bounds for RRect with radius 10 -> 0,0m100,100
[ERROR:flutter/impeller/display_list/dl_unittests.cc(1748)] Bounds for RRect with radius 20 -> 0,0m100,100
[ERROR:flutter/impeller/display_list/dl_unittests.cc(1748)] Bounds for RRect with radius 30 -> 0,0m100,100
[ERROR:flutter/impeller/display_list/dl_unittests.cc(1748)] Bounds for RRect with radius 40 -> 0,0m100,100
[ERROR:flutter/impeller/display_list/dl_unittests.cc(1748)] Bounds for RRect with radius 50 -> 0,0m100,100
[ERROR:flutter/impeller/display_list/dl_unittests.cc(1748)] Bounds for RRect with radius 60 -> 0,0m100,100
[ERROR:flutter/impeller/display_list/dl_unittests.cc(1748)] Bounds for RRect with radius 70 -> 0,0m100,100
[ERROR:flutter/impeller/display_list/dl_unittests.cc(1748)] Bounds for RRect with radius 80 -> 0,0m100,100
[ERROR:flutter/impeller/display_list/dl_unittests.cc(1748)] Bounds for RRect with radius 90 -> 0,0m100,100

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was wondering if when the corner radius was high would it start looking more like a circle that was inset in the rect. LGTM.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did you try this with reversed rects? RRects is one of the areas where Skia was doing "auto-sorting" of the corners of the rects...

.TakePath();
if (paint.style == Paint::Style::kFill) {
Entity entity;
Expand All @@ -273,10 +274,13 @@ void Canvas::DrawCircle(Point center, Scalar radius, const Paint& paint) {
paint)) {
return;
}
auto circle_path = PathBuilder{}
.AddCircle(center, radius)
.SetConvexity(Convexity::kConvex)
.TakePath();
auto circle_path =
PathBuilder{}
.AddCircle(center, radius)
.SetConvexity(Convexity::kConvex)
.SetBounds(Rect::MakeLTRB(center.x - radius, center.y - radius,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this part also have a unit test?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

center.x + radius, center.y + radius))
.TakePath();
DrawPath(circle_path, paint);
}

Expand Down Expand Up @@ -317,6 +321,7 @@ void Canvas::ClipRRect(const Rect& rect,
auto path = PathBuilder{}
.SetConvexity(Convexity::kConvex)
.AddRoundedRect(rect, corner_radius)
.SetBounds(rect)
.TakePath();

std::optional<Rect> inner_rect = (corner_radius * 2 < rect.size.width &&
Expand Down
60 changes: 60 additions & 0 deletions impeller/display_list/dl_unittests.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@
#include "impeller/display_list/dl_dispatcher.h"
#include "impeller/display_list/dl_image_impeller.h"
#include "impeller/display_list/dl_playground.h"
#include "impeller/entity/contents/clip_contents.h"
#include "impeller/entity/contents/solid_color_contents.h"
#include "impeller/entity/contents/solid_rrect_blur_contents.h"
#include "impeller/geometry/constants.h"
Expand Down Expand Up @@ -1681,6 +1682,65 @@ TEST_P(DisplayListTest, DrawVerticesBlendModes) {
ASSERT_TRUE(OpenPlaygroundHere(callback));
}

template <typename Contents>
static std::optional<Rect> GetCoverageOfFirstEntity(const Picture& picture) {
std::optional<Rect> coverage;
picture.pass->IterateAllEntities([&coverage](Entity& entity) {
if (std::static_pointer_cast<Contents>(entity.GetContents())) {
auto contents = std::static_pointer_cast<Contents>(entity.GetContents());
Entity entity;
coverage = contents->GetCoverage(entity);
return false;
}
return true;
});
return coverage;
}

TEST(DisplayListTest, RRectBoundsComputation) {
SkRRect rrect = SkRRect::MakeRectXY(SkRect::MakeLTRB(0, 0, 100, 100), 4, 4);
SkPath path = SkPath().addRRect(rrect);

flutter::DlPaint paint;
flutter::DisplayListBuilder builder;

builder.DrawPath(path, paint);
auto display_list = builder.Build();

DlDispatcher dispatcher;
display_list->Dispatch(dispatcher);
auto picture = dispatcher.EndRecordingAsPicture();

std::optional<Rect> coverage =
GetCoverageOfFirstEntity<SolidColorContents>(picture);

// Validate that the RRect coverage is _exactly_ the same as the input rect.
ASSERT_TRUE(coverage.has_value());
ASSERT_EQ(coverage.value_or(Rect::MakeMaximum()),
Rect::MakeLTRB(0, 0, 100, 100));
}

TEST(DisplayListTest, CircleBoundsComputation) {
SkPath path = SkPath().addCircle(0, 0, 5);

flutter::DlPaint paint;
flutter::DisplayListBuilder builder;

builder.DrawPath(path, paint);
auto display_list = builder.Build();

DlDispatcher dispatcher;
display_list->Dispatch(dispatcher);
auto picture = dispatcher.EndRecordingAsPicture();

std::optional<Rect> coverage =
GetCoverageOfFirstEntity<SolidColorContents>(picture);

ASSERT_TRUE(coverage.has_value());
ASSERT_EQ(coverage.value_or(Rect::MakeMaximum()),
Rect::MakeLTRB(-5, -5, 5, 5));
}

#ifdef IMPELLER_ENABLE_3D
TEST_P(DisplayListTest, SceneColorSource) {
// Load up the scene.
Expand Down