[go: nahoru, domu]
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

Jump to bottom

[Impeller] make sure to render TiledTextureContents if the geometry doesn't fit in the texture's bounds. #51393

Merged
merged 3 commits into from
Mar 18, 2024
Merged

[Impeller] make sure to render TiledTextureContents if the geometry doesn't fit in the texture's bounds. #51393

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
76873af
[Impeller] make sure to render TiledTextureContents if the geometry d…
gaaclarke Mar 13, 2024
ae03211
updated golden
gaaclarke Mar 13, 2024
47f185f
golden typo
gaaclarke Mar 16, 2024
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
35 changes: 35 additions & 0 deletions impeller/aiks/aiks_blur_unittests.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1146,5 +1146,40 @@ TEST_P(AiksTest, GaussianBlurMipMapSolidColor) {
#endif
}

TEST_P(AiksTest, MaskBlurDoesntStretchContents) {
Scalar sigma = 70;
auto callback = [&](AiksContext& renderer) -> std::optional<Picture> {
if (AiksTest::ImGuiBegin("Controls", nullptr,
ImGuiWindowFlags_AlwaysAutoResize)) {
ImGui::SliderFloat("Sigma", &sigma, 0, 500);
ImGui::End();
}
Canvas canvas;
canvas.Scale(GetContentScale());
canvas.DrawPaint({.color = Color(0.1, 0.1, 0.1, 1.0)});

std::shared_ptr<Texture> boston = CreateTextureForFixture("boston.jpg");
ColorSource image_source = ColorSource::MakeImage(
boston, Entity::TileMode::kRepeat, Entity::TileMode::kRepeat, {}, {});

canvas.Transform(Matrix::MakeTranslation({100, 100, 0}) *
Matrix::MakeScale({0.5, 0.5, 1.0}));
Paint paint = {
.color_source = image_source,
.mask_blur_descriptor =
Paint::MaskBlurDescriptor{
.style = FilterContents::BlurStyle::kNormal,
.sigma = Sigma(sigma),
},
};
canvas.DrawRect(
Rect::MakeXYWH(0, 0, boston->GetSize().width, boston->GetSize().height),
paint);

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

} // namespace testing
} // namespace impeller
1 change: 0 additions & 1 deletion impeller/aiks/paint.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -183,7 +183,6 @@ std::shared_ptr<FilterContents> Paint::MaskBlurDescriptor::CreateMaskBlur(
}
color_source_contents->SetGeometry(
Geometry::MakeRect(*expanded_local_bounds));

std::shared_ptr<Contents> color_contents = color_source_contents;

/// 4. Apply the user set color filter on the GPU, if applicable.
Expand Down
6 changes: 5 additions & 1 deletion impeller/entity/contents/tiled_texture_contents.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -228,8 +228,12 @@ std::optional<Snapshot> TiledTextureContents::RenderToSnapshot(
bool msaa_enabled,
int32_t mip_count,
const std::string& label) const {
std::optional<Rect> geometry_coverage = GetGeometry()->GetCoverage({});
if (GetInverseEffectTransform().IsIdentity() &&
GetGeometry()->IsAxisAlignedRect()) {
GetGeometry()->IsAxisAlignedRect() &&
(!geometry_coverage.has_value() ||
Rect::MakeSize(texture_->GetSize())
.Contains(geometry_coverage.value()))) {
Comment on lines +231 to +236
Copy link
Member

Choose a reason for hiding this comment

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

I don't fully understand this, could you explain it a bit more? I know for the regular texture contents we have some src/dst checking.

Since tiled texture contents is an arbitrary geometry, we have the axis aligned rect check first. What is the additional check doing?

Copy link
Member Author

Choose a reason for hiding this comment

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

When the geometry is not contained in the texture that means we have to render texcoords outside of 0..1, so we can't take the fast pass of avoiding rendering. We need the clamping, decal, etc.

If you check out the issue this is where we could hack this conditional to give us the correct result by sending in Scale(1.001, 1.001, 1) instead of Identity() for the effect transform because the fast pass conditional was incorrectly ignoring the geometry and the full render was correct.

Copy link
Member

Choose a reason for hiding this comment

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

But isn't that expressed in the coverage and the scaling below?

Copy link
Member Author

Choose a reason for hiding this comment

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

Demonstrably, no. If you run the golden test without this patch it will render with the incorrect texture coords, instead stretching them to fill the whole geometry. That flow through the code lacks the math the express the effect transform correctly.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think since the effect transform was identity it was assumed it could be ignored, but it's not the case when the geometry is expanded.

Copy link
Member

Choose a reason for hiding this comment

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

Oh I agree its not working! But, this code should be able to render a stretched rectangle right? It seems like there is a bug in the math there that we could fix?

Copy link
Member Author

Choose a reason for hiding this comment

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

I played around with it again and I don't think so. The Snapshot flow lacks the expressivity to represent rendering texcoords outside of 0..1. It's not a bug, but a limit of the abstraction. That's why it only works when rendering a subset inside of the texture.

If you set the scale to 1.0 you can see it renders at the correct size without the halo we need and there is no lever to pull to make it appear.

We can file an issue to redesign Snapshot it will probably require having a matrix for the uvs and the vertices?

Copy link
Member

Choose a reason for hiding this comment

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

Ack. The snapshot has a transform, but it assumes a srcRect. We could add support for that, but it isn't necessary here.

auto coverage = GetCoverage(entity);
if (!coverage.has_value()) {
return std::nullopt;
Expand Down
3 changes: 3 additions & 0 deletions testing/impeller_golden_tests_output.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -525,6 +525,9 @@ impeller_Play_AiksTest_ImageFilteredUnboundedSaveLayerWithUnboundedContents_Vulk
impeller_Play_AiksTest_LinearToSrgbFilterSubpassCollapseOptimization_Metal.png
impeller_Play_AiksTest_LinearToSrgbFilterSubpassCollapseOptimization_OpenGLES.png
impeller_Play_AiksTest_LinearToSrgbFilterSubpassCollapseOptimization_Vulkan.png
impeller_Play_AiksTest_MaskBlurDoesntStretchContents_Metal.png
impeller_Play_AiksTest_MaskBlurDoesntStretchContents_OpenGLES.png
impeller_Play_AiksTest_MaskBlurDoesntStretchContents_Vulkan.png
impeller_Play_AiksTest_MaskBlurTexture_Metal.png
impeller_Play_AiksTest_MaskBlurTexture_OpenGLES.png
impeller_Play_AiksTest_MaskBlurTexture_Vulkan.png
Expand Down