[Impeller] fix typo in setup for fast elliptical rrect blurs

[flar]

This typo was discovered while converting the AIKS tests to DisplayList - the new test executes correctly, but the old test generated bad goldens due to this typo. Fixing the typo to fix the golden prior to landing the test conversion.

The fix should be covered by existing tests - in fact the change is correcting an already bad golden test to correct behavior.

[flutter-dashboard]

It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact "@test-exemption-reviewer" in the #hackers channel in Chat (don't just cc them here, they won't see it! Use Discord!).

If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix?

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

[flar]

Filed for a test exemption in Discord hackers channel...

[gaaclarke]

lgtm (waiting to make sure that it changes the golden for approval)

[jonahwilliams]

LGTM!

[gaaclarke]

The change is getting picked up in the golden tests, lgtm.

(I wouldn't consider this test exempt, it has existing tests that cover it).

[flutter-dashboard]

Golden file changes have been found for this pull request. Click here to view and triage (e.g. because this is an intentional change).

If you are still iterating on this change and are not ready to resolve the images on the Flutter Gold dashboard, consider marking this PR as a draft pull request above. You will still be able to view image results on the dashboard, commenting will be silenced, and the check will not try to resolve itself until marked ready for review.

Changes reported for pull request #53673 at sha 47857ae

[gaaclarke]

Looking at the code, this is obviously right. But looking at the golden's, the old code seems to draw something that makes more sense. Maybe we need to update the test?

---

[flar]

Looking at the code, this is obviously right. But looking at the golden's, the old code seems to draw something that makes more sense. Maybe we need to update the test?

I don't understand what you mean by "more sense" - do you mean something that looks more aesthetically pleasing? This test is testing that the numbers behave as you'd expect from the numbers, which it does.

The old code was not drawing what the numbers told it to draw.
The new code is drawing exactly what the numbers were requesting.

Updating the test to not draw these edge cases would leave the edge cases untested.

[flar]

The change is getting picked up in the golden tests, lgtm.

(I wouldn't consider this test exempt, it has existing tests that cover it).

It isn't automatically exempt as it isn't just comment changes or reformatting code.

But, it is fully tested by existing tests so a manual test exemption can be requested from the test exemption gatekeepers, which I've done.

[flar]

Note to those granting a test exemption - I'm leaving the goldens untriaged so that you can verify that the updated behavior is caught by existing tests. The new values of the goldens are the correct(ed) output...

[gaaclarke]

I don't understand what you mean by "more sense" - do you mean something that looks more aesthetically pleasing? This test is testing that the numbers behave as you'd expect from the numbers, which it does.

"More sense" in the fact that the array of rendered images is varying in a dimension that a user might care about, like the others in that test. The dimension that is being changed here seems niche. I'm wondering if there is a better test is all.

Updating the test to not draw these edge cases would leave the edge cases untested.

Updating the test could mean adding variants. I don't recommend removing edge cases you care about.

It isn't automatically exempt as it isn't just comment changes or reformatting code.

But, it is fully tested by existing tests so a manual test exemption can be requested from the test exemption gatekeepers, which I've done.

Sorry, I was trying to convey that this should not need an exemption because there is proof that there is test coverage because of the golden changes, not that this requires something else to meet the testing requirement.

[flar]

I don't understand what you mean by "more sense" - do you mean something that looks more aesthetically pleasing? This test is testing that the numbers behave as you'd expect from the numbers, which it does.

"More sense" in the fact that the array of rendered images is varying in a dimension that a user might care about, like the others in that test. The dimension that is being changed here seems niche. I'm wondering if there is a better test is all.

Ah, this row is testing that increasingly oblong ellipse corners look increasingly oblong. It could probably make their oblong-ness more obvious to the naked eye the vertical radius is stuck at 5.0 and the horizontal radius varies from, I think, 5.0 to 25.0. I originally tried a few different scenarios and they were less visible than this.

(And then, of course, I made a change to the code and the oblongness disappeared and I didn't notice, but that was probably more about me thinking that the last update was minor and forgetting what that specific row was supposed to be accomplishing when I reviewed it.)

Updating the test to not draw these edge cases would leave the edge cases untested.

Updating the test could mean adding variants. I don't recommend removing edge cases you care about.

Maybe a grid where the H/V radii vary across and down independently - it would make a kind of symmetric distortion that might be more visible?

It isn't automatically exempt as it isn't just comment changes or reformatting code.
But, it is fully tested by existing tests so a manual test exemption can be requested from the test exemption gatekeepers, which I've done.

Sorry, I was trying to convey that this should not need an exemption because there is proof that there is test coverage because of the golden changes, not that this requires something else to meet the testing requirement.

Ah, yes, exactly. I hadn't read it that way.

[stuartmorgan]

test-exempt: fixes existing test

[flutter-dashboard]

Golden file changes are available for triage from new commit, Click here to view.

Changes reported for pull request #53673 at sha 43e1638