Skip to content

Conversation

@gspencergoog
Copy link
Contributor

@gspencergoog gspencergoog commented Aug 21, 2022

This reverts commit b3aba4d because it breaks Google tests.

Clearly, I didn't find all of the instances. Also, looks like this time we have some rounding error issues or something. The last time it failed, I didn't see any of that. Reverting until I understand why that is.

b/243217872

cc @bernaferrari

@flutter-dashboard flutter-dashboard bot added d: api docs Issues with https://api.flutter.dev/ d: examples Sample code and demos documentation f: material design flutter/packages/flutter/material repository. framework flutter/packages/flutter repository. See also f: labels. c: contributor-productivity Team-specific productivity, code health, technical debt. labels Aug 21, 2022
@CaseyHillers CaseyHillers added the autosubmit Merge PR when tree becomes green via auto submit App label Aug 21, 2022
@CaseyHillers
Copy link
Contributor

Merging as it's a revert.

@CaseyHillers CaseyHillers merged commit 957a8da into flutter:master Aug 21, 2022
@bernaferrari
Copy link
Contributor

Is there any way I can help?

@gspencergoog
Copy link
Contributor Author

@bernaferrari No, I think it requires access to the failing tests to fix. The problem appears to be something along the lines of some rounding or an off-by-one error. Some thin borders disappear entirely, and others are just off by a bit (less than a pixel). I'll see if I can make a repro case that we can use as a regression test.

@bernaferrari
Copy link
Contributor

bernaferrari commented Aug 21, 2022

If I remember well, one thing we removed was if (width == 0) drawRRect (now it draws DRRect, no special case. In the perfect world, it wouldn't even be drawn). It is the only thing I can think of. Feel free to try adding it again (or skipping!) and see how the test behaves.

That's the only thing I can think.

@bernaferrari
Copy link
Contributor

Try on RoundedRectangleBorder.paint:

if (width == 0.0) {
  canvas.drawRRect(borderRadius.resolve(textDirection).toRRect(rect), side.toPaint());
} else {
// ...
}

or

if (width == 0.0) return;

bernaferrari pushed a commit to bernaferrari/flutter that referenced this pull request Aug 22, 2022
@bernaferrari
Copy link
Contributor

bernaferrari commented Aug 22, 2022

I went ahead and made this: #109975
If you could test on Google internal and see if the issue still exists, it would be great. I don't know what happened to the bot that did this before.

I have two theories: drawDRRect has a bug and the person that made width == 0 knew this. Or, drawRRect when width == 0 does something which is better than not drawing anything (or drawing the 'wrong' way).

@gspencergoog
Copy link
Contributor Author

I don't know what happened to the bot that did this before.

It doesn't kick in until the PR has been approved.

I have two theories: drawDRRect has a bug and the person that made width == 0 knew this. Or, drawRRect when width == 0 does something which is better than not drawing anything (or drawing the 'wrong' way).

It doesn't have a bug, but I think that perhaps the anti-aliasing behavior is different between the two. I'll take a look.

@bernaferrari
Copy link
Contributor

It doesn't kick in until the PR has been approved.

That's how it worked, but now it doesn't. You merged your and I merged mine without the Google testing happening. It only happened after.

@gspencergoog gspencergoog deleted the revert_pr branch October 7, 2022 22:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

autosubmit Merge PR when tree becomes green via auto submit App c: contributor-productivity Team-specific productivity, code health, technical debt. d: api docs Issues with https://api.flutter.dev/ d: examples Sample code and demos f: material design flutter/packages/flutter/material repository. framework flutter/packages/flutter repository. See also f: labels.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants