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

[Impeller] fix incorrect origins for mesh gradient computation. #54762

Merged
merged 3 commits into from
Aug 26, 2024

Conversation

jonahwilliams
Copy link
Member

@jonahwilliams jonahwilliams commented Aug 24, 2024

Fixes flutter/flutter#153964

Changing the origin of the rect used to render a shader could break shaders that expect to render at particular coordinates based on the input vertices. The snapshot functionality correctly handles translating a texture, so the translation was never necessary to begin with.

@jonahwilliams jonahwilliams marked this pull request as ready for review August 26, 2024 16:53
out vec4 frag_color;

// Output solid red if frag position is within LTRB rectangle.
void main() {
Copy link
Member Author

Choose a reason for hiding this comment

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

This only outputs red if the rendered coordiantes match the expected coordinates.

Copy link
Member

@gaaclarke gaaclarke left a comment

Choose a reason for hiding this comment

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

lgtm, just one question about uninitialized frag_color.

// The blue rectangle drawn under them should not be visible.
TEST_P(AiksTest,
DrawVerticesTextureCoordinatesWithFragmentShaderNonZeroOrigin) {
std::vector<SkPoint> positions_lt = {
Copy link
Member

Choose a reason for hiding this comment

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

No abbreviations ("lt").

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

void main() {
if (FlutterFragCoord().x >= ltrb.x && FlutterFragCoord().x <= ltrb.z &&
FlutterFragCoord().y >= ltrb.y && FlutterFragCoord().y <= ltrb.w) {
frag_color = vec4(1.0, 0.0, 0.0, 1.0);
Copy link
Member

Choose a reason for hiding this comment

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

Wait, what is frag_color if it's outside of the box? Shouldn't that be explicit here?

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 can set it to transparent black, I think that is implied already.

Copy link
Member Author

Choose a reason for hiding this comment

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

updated

Copy link
Member

Choose a reason for hiding this comment

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

Considering we are cross compiling the shaders it doesn't hurt, thanks.

builder.DrawRect(SkRect::MakeLTRB(200, 200, 250, 250), rect_paint);
builder.DrawVertices(vertices_lt, flutter::DlBlendMode::kSrcOver, paint);

ASSERT_TRUE(OpenPlaygroundHere(builder.Build()));
Copy link
Member

Choose a reason for hiding this comment

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

Heads up you are going to need to add the included golden file to the list.

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 usually just wait for the check to fail first and then copy the string.

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

Copy link
Member

@gaaclarke gaaclarke left a comment

Choose a reason for hiding this comment

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

lgtm assuming the goldens look good to jonah

@jonahwilliams jonahwilliams added the autosubmit Merge PR when tree becomes green via auto submit App label Aug 26, 2024
@auto-submit auto-submit bot merged commit b41ca79 into flutter:main Aug 26, 2024
30 checks passed
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Aug 27, 2024
auto-submit bot pushed a commit to flutter/flutter that referenced this pull request Aug 27, 2024
…ions) (#154153)

Manual roll requested by [email protected]

flutter/engine@f645ca5...b41ca79

2024-08-26 [email protected] [Impeller] fix incorrect origins for mesh gradient computation. (flutter/engine#54762)
2024-08-26 [email protected] Change the `ci/analyze.sh` script to analyze _all_ of the engine (flutter/engine#54779)
2024-08-26 [email protected] Use GNI group instead of hardcoding PNG codecs source files. (flutter/engine#54781)
2024-08-26 [email protected] [ios] Fix text input edit rotor accessibility (flutter/engine#54351)
2024-08-26 [email protected] Ensure orchestrators aren't assigned to 32 core machines (flutter/engine#54754)

If this roll has caused a breakage, revert this CL and stop the roller
using the controls here:
https://autoroll.skia.org/r/flutter-engine-flutter-autoroll
Please CC [email protected],[email protected],[email protected] on the revert to ensure that a human
is aware of the problem.

To file a bug in Flutter: https://github.com/flutter/flutter/issues/new/choose

To report a problem with the AutoRoller itself, please file a bug:
https://issues.skia.org/issues/new?component=1389291&template=1850622

Documentation for the AutoRoller is here:
https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
jonahwilliams added a commit to jonahwilliams/engine that referenced this pull request Aug 28, 2024
…ter#54762)

Fixes flutter/flutter#153964

Changing the origin of the rect used to render a shader could break shaders that expect to render at particular coordinates based on the input vertices. The snapshot functionality correctly handles translating a texture, so the translation was never necessary to begin with.
Buchimi pushed a commit to Buchimi/flutter that referenced this pull request Sep 2, 2024
…ions) (flutter#154153)

Manual roll requested by [email protected]

flutter/engine@f645ca5...b41ca79

2024-08-26 [email protected] [Impeller] fix incorrect origins for mesh gradient computation. (flutter/engine#54762)
2024-08-26 [email protected] Change the `ci/analyze.sh` script to analyze _all_ of the engine (flutter/engine#54779)
2024-08-26 [email protected] Use GNI group instead of hardcoding PNG codecs source files. (flutter/engine#54781)
2024-08-26 [email protected] [ios] Fix text input edit rotor accessibility (flutter/engine#54351)
2024-08-26 [email protected] Ensure orchestrators aren't assigned to 32 core machines (flutter/engine#54754)

If this roll has caused a breakage, revert this CL and stop the roller
using the controls here:
https://autoroll.skia.org/r/flutter-engine-flutter-autoroll
Please CC [email protected],[email protected],[email protected] on the revert to ensure that a human
is aware of the problem.

To file a bug in Flutter: https://github.com/flutter/flutter/issues/new/choose

To report a problem with the AutoRoller itself, please file a bug:
https://issues.skia.org/issues/new?component=1389291&template=1850622

Documentation for the AutoRoller is here:
https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
auto-submit bot pushed a commit that referenced this pull request Sep 10, 2024
…54762) (#54843)

Fixes flutter/flutter#153964

Changing the origin of the rect used to render a shader could break shaders that expect to render at particular coordinates based on the input vertices. The snapshot functionality correctly handles translating a texture, so the translation was never necessary to begin with.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
autosubmit Merge PR when tree becomes green via auto submit App e: impeller
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Impeller breaks mesh gradients
2 participants