Add more asserts to check matrix validity by liyuqian · Pull Request #31701 · flutter/flutter

liyuqian · 2019-04-26T23:29:28Z

Description

These will help identify where the matrix starts to get wrong.

Also fixed RenderFittexBox to no longer paint with empty child which previously triggered invalid matrix computations (NaN with dividing by 0). See also #7489

Related Issues

#31650
#31700
#7431

Tests

RenderFittedBox does not paint with empty sizes

Checklist

Before you create this PR confirm that it meets all requirements listed below by checking the relevant checkboxes ([x]). This will ensure a smooth and quick review process.

I read the Contributor Guide and followed the process outlined there for submitting PRs.
I signed the CLA.
I read and followed the Flutter Style Guide, including Features we expect every widget to implement.
I updated/added relevant documentation (doc comments with ///).
All existing and new tests are passing.
The analyzer (flutter analyze --flutter-repo) does not report any problems on my PR.
I am willing to follow-up on review comments in a timely manner.

Breaking Change

Does your PR require Flutter developers to manually update their apps to accommodate your change?

Yes, this is a breaking change (Please read Handling breaking changes). Replace this with a link to the e-mail where you asked for input on this proposed change.
No, this is not a breaking change.

These will help identify where the matrix starts to get wrong. Related issue: flutter#31650

goderbauer · 2019-04-29T12:52:06Z

Looks like some tests are failing?

Piinks

Does adding asserts constitute a potentially breaking change?

liyuqian · 2019-04-29T17:47:44Z

@goderbauer yes, we had a diving by zero issue which is fixed in the newest patch.

@Piinks : assert won't break their release/profile build, and it will help them debugging the debug build. So I guess that it's not a breaking change?

dnfield · 2019-05-01T22:12:35Z

packages/flutter/lib/src/rendering/proxy_box.dart

      final FittedSizes sizes = applyBoxFit(_fit, childSize, size);
-      final double scaleX = sizes.destination.width / sizes.source.width;
-      final double scaleY = sizes.destination.height / sizes.source.height;
+      final double scaleX = _div_handle_zero(sizes.destination.width, sizes.source.width);


This seems like it should be assert(sizes.source.width != 0), and maybe assert(sizes.source.width.isFinite), no?

That's a possible solution too. We'd have to modify test/rendering/proxy_box_test.dart: RenderFittedBox paint to not send a 0 source width though. Let me see how that test can be modified.

Is it because it's sending in Size.zero for constraints?

Why did this test pass before but fail now? Your change doesn't seem to have anything to do with it.

(It does look like the intention here is to have a size.zero be a no-op though)

Piinks · 2019-05-03T18:14:12Z

packages/flutter/lib/src/rendering/proxy_box.dart

      final FittedSizes sizes = applyBoxFit(_fit, childSize, size);
-      final double scaleX = sizes.destination.width / sizes.source.width;
-      final double scaleY = sizes.destination.height / sizes.source.height;
+      final double scaleX = _divHandleZero(sizes.destination.width, sizes.source.width);


Uber nit:

Suggested change

final double scaleX = _divHandleZero(sizes.destination.width, sizes.source.width);

final double scaleX = _divisionHandleZero(sizes.destination.width, sizes.source.width);

to avoid abbreviations

Piinks · 2019-05-03T18:14:16Z

packages/flutter/lib/src/rendering/proxy_box.dart

-      final double scaleX = sizes.destination.width / sizes.source.width;
-      final double scaleY = sizes.destination.height / sizes.source.height;
+      final double scaleX = _divHandleZero(sizes.destination.width, sizes.source.width);
+      final double scaleY = _divHandleZero(sizes.destination.height, sizes.source.height);


Uber nit:

Suggested change

final double scaleY = _divHandleZero(sizes.destination.height, sizes.source.height);

final double scaleX = _divisionHandleZero(sizes.destination.width, sizes.source.width);

to avoid abbreviations

Thanks @dnfield and @Piinks for your comments! I've now dropped the _devHandleZero. Instead, I fixed the test and make RenderFittedBox to no longer paint with empty sizes. Add @jason-simmons for review as he wrote the test originally to fix a similar issue with NaN and empty size.

Update the test accordingly See flutter#7489 and flutter#7431 for the original purpose of the test.

dnfield · 2019-05-03T22:34:03Z

packages/flutter/lib/src/rendering/proxy_box.dart

  @override
  void paint(PaintingContext context, Offset offset) {
-    if (size.isEmpty)
+    if (size.isEmpty || child.size.isEmpty)


Should we be checking for isFinite as well?

Oh, I see, we check that in _updatePaintData

tvolkert · 2019-05-09T15:37:36Z

Was this not worth merging?

liyuqian · 2019-05-09T17:30:01Z

That was a mistake of my local script... Reopen and merge now.

This reverts commit 0dc290c.

)" (flutter#32496)" This reverts commit 549b412.

This relands #31701 with missing const added This reverts commit 549b412.

Add more asserts to check matrix validity

a402ee3

These will help identify where the matrix starts to get wrong. Related issue: flutter#31650

liyuqian requested a review from tvolkert April 26, 2019 23:29

googlebot added the cla: yes label Apr 26, 2019

liyuqian mentioned this pull request Apr 26, 2019

Framework shouldn't send invalid matrix to engine #31700

Closed

goderbauer added the framework flutter/packages/flutter repository. See also f: labels. label Apr 29, 2019

Piinks reviewed Apr 29, 2019

View reviewed changes

Fix dividing by zero

acf2285

dnfield reviewed May 1, 2019

View reviewed changes

Fix analyze

fa68c32

Piinks reviewed May 3, 2019

View reviewed changes

liyuqian requested a review from jason-simmons May 3, 2019 22:17

Do not paint FittedBox with empty child

1257521

Update the test accordingly See flutter#7489 and flutter#7431 for the original purpose of the test.

liyuqian force-pushed the check_transform_matrix branch from a4edf0e to 1257521 Compare May 3, 2019 22:22

dnfield reviewed May 3, 2019

View reviewed changes

liyuqian closed this May 8, 2019

liyuqian deleted the check_transform_matrix branch May 8, 2019 21:20

liyuqian restored the check_transform_matrix branch May 9, 2019 17:29

liyuqian reopened this May 9, 2019

jason-simmons approved these changes May 9, 2019

View reviewed changes

dnfield approved these changes May 9, 2019

View reviewed changes

liyuqian merged commit 0dc290c into flutter:master May 10, 2019

jonahwilliams pushed a commit that referenced this pull request May 10, 2019

Revert "Add more asserts to check matrix validity (#31701)"

d408856

This reverts commit 0dc290c.

jonahwilliams mentioned this pull request May 10, 2019

Revert "Add more asserts to check matrix validity" #32496

Merged

jonahwilliams pushed a commit that referenced this pull request May 10, 2019

Revert "Add more asserts to check matrix validity (#31701)" (#32496)

549b412

liyuqian mentioned this pull request May 10, 2019

Reland matrix check #32521

Merged

liyuqian added a commit to liyuqian/flutter that referenced this pull request May 10, 2019

Revert "Revert "Add more asserts to check matrix validity (flutter#31701

1f8a1f4

)" (flutter#32496)" This reverts commit 549b412.

liyuqian added a commit that referenced this pull request May 13, 2019

Reland matrix check (#32521)

06f86c8

This relands #31701 with missing const added This reverts commit 549b412.

github-actions bot locked as resolved and limited conversation to collaborators Aug 6, 2021

	final double scaleX = _divHandleZero(sizes.destination.width, sizes.source.width);
	final double scaleX = _divisionHandleZero(sizes.destination.width, sizes.source.width);

	final double scaleY = _divHandleZero(sizes.destination.height, sizes.source.height);
	final double scaleX = _divisionHandleZero(sizes.destination.width, sizes.source.width);

Conversation

liyuqian commented Apr 26, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Related Issues

Tests

Checklist

Breaking Change

Uh oh!

goderbauer commented Apr 29, 2019

Uh oh!

Piinks left a comment

Choose a reason for hiding this comment

Uh oh!

liyuqian commented Apr 29, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tvolkert commented May 9, 2019

Uh oh!

liyuqian commented May 9, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

liyuqian commented Apr 26, 2019 •

edited

Loading