Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Impeller] Add golden for clipped+transformed blur. #48886

Merged
merged 4 commits into from
Dec 14, 2023
Merged

[Impeller] Add golden for clipped+transformed blur. #48886

Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
106116d
[Impeller] Add golden for clipped+transformed blur.
bdero Dec 11, 2023
485c5cf
Center the clip over the blurred image
bdero Dec 12, 2023
3467a9f
Aaron review
bdero Dec 13, 2023
7f4738d
merge...
bdero Dec 13, 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
29 changes: 29 additions & 0 deletions impeller/aiks/aiks_unittests.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4555,5 +4555,34 @@ TEST_P(AiksTest, GaussianBlurWithoutDecalSupport) {
ASSERT_TRUE(OpenPlaygroundHere(canvas.EndRecordingAsPicture()));
}

// Smoketest to catch issues with the coverage hint.
// Draws a rotated blurred image within a rectangle clip. The center of the clip
// rectangle is the center of the rotated image. The entire area of the clip
// rectangle should be filled with opaque colors output by the blur.
TEST_P(AiksTest, GaussianBlurRotatedAndClipped) {
Copy link
Member

Choose a reason for hiding this comment

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

Can you make this test be more obvious while testing the same thing? For example, rotate the picture of boston 90 degrees and clip off the left and right sides equally so that we get the square just in the middle? That way it is easier to validate that it is correct.

Copy link
Member Author

@bdero bdero Dec 11, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

In order to be a good smoke test for the coverage hint:

  • the image should be drawn with a non-aligned basis (weird rotation or skew),
  • the clip should unevenly clip the surface (some sides should be clipped more than the others),
  • both the blurred source rectangle and the coverage hint should be non-square rectangles of different size.

I think if the before/after comparison shows obvious differences from the current golden, that'd be enough to be suitably useful and spark investigation?

Copy link
Member

Choose a reason for hiding this comment

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

I think that's good when we talk about making sure current behavior is maintained. The problem is that it's hard to know if this is correct behavior. I can't read the test and look at the image and pretty confidently know that is the correct rendering.

I think this would be a bit easier to follow if you rotated around the center of the texture. Rotating around an anchor that isn't the centroid or one of the corners is already hard to calculate in your head.

Copy link
Member Author

Choose a reason for hiding this comment

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

Alrighty, I changed it so the center of the clip is the center of the blurred image and added some notes about the expected output to a comment before the test:

image

Copy link
Member

Choose a reason for hiding this comment

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

This is what I had in mind. I think this tests everything you want and is imo way more simple. I can easily communicate what it should look like:

TEST_P(AiksTest, GaussianBlurRotatedAndClipped) {
  Canvas canvas;
  std::shared_ptr<Texture> boston = CreateTextureForFixture("boston.jpg");
  Rect bounds =
      Rect::MakeXYWH(0, 0, boston->GetSize().width, boston->GetSize().height);
  Vector2 image_center = Vector2(bounds.GetSize() / 2);
  Paint paint = {.image_filter =
                     ImageFilter::MakeBlur(Sigma(20.0), Sigma(20.0),
                                           FilterContents::BlurStyle::kNormal,
                                           Entity::TileMode::kDecal)};
  Vector2 clip_size = {200, 100};
  canvas.Scale(GetContentScale());
  canvas.ClipRect(Rect::MakeLTRB(512, 384, 512, 384).Expand(clip_size));
  canvas.Translate({512, 384, 0});
  canvas.Scale({0.5, 0.5, 1});
  canvas.Rotate(Degrees(25));

  canvas.DrawImageRect(std::make_shared<Image>(boston), /*source=*/bounds,
                       /*dest=*/bounds.Shift(-image_center), paint);

  ASSERT_TRUE(OpenPlaygroundHere(canvas.EndRecordingAsPicture()));
}

Copy link
Member Author

Choose a reason for hiding this comment

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

sgtm, I went with this and adjusted the clip slightly to make it cover a smaller area of the image.

Canvas canvas;
std::shared_ptr<Texture> boston = CreateTextureForFixture("boston.jpg");
Rect bounds =
Rect::MakeXYWH(0, 0, boston->GetSize().width, boston->GetSize().height);
Vector2 image_center = Vector2(bounds.GetSize() / 2);
Paint paint = {.image_filter =
ImageFilter::MakeBlur(Sigma(20.0), Sigma(20.0),
FilterContents::BlurStyle::kNormal,
Entity::TileMode::kDecal)};
Vector2 clip_size = {150, 75};
Vector2 center = Vector2(1024, 768) / 2;
canvas.Scale(GetContentScale());
canvas.ClipRect(
Rect::MakeLTRB(center.x, center.y, center.x, center.y).Expand(clip_size));
canvas.Translate({center.x, center.y, 0});
canvas.Scale({0.6, 0.6, 1});
canvas.Rotate(Degrees(25));

canvas.DrawImageRect(std::make_shared<Image>(boston), /*source=*/bounds,
/*dest=*/bounds.Shift(-image_center), paint);

ASSERT_TRUE(OpenPlaygroundHere(canvas.EndRecordingAsPicture()));
}

} // namespace testing
} // namespace impeller