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

[Impeller] Make IsEmpty methods on Rect and Size NaN-aware#47725

Merged
auto-submit[bot] merged 2 commits intoflutter:mainfrom
flar:impeller-Rect-Size-IsEmpty-NaN
Nov 7, 2023
Merged

[Impeller] Make IsEmpty methods on Rect and Size NaN-aware#47725
auto-submit[bot] merged 2 commits intoflutter:mainfrom
flar:impeller-Rect-Size-IsEmpty-NaN

Conversation

@flar
Copy link
Contributor

@flar flar commented Nov 6, 2023

Removes unused or redundant overloads and fixes the IsEmpty method on Size to be both simpler (2 comparisons vs. 4) and NaN-aware. Rect automatically uses the new code via delegation to the Size object.

@flar
Copy link
Contributor Author

flar commented Nov 6, 2023

Specific tests are added to test the IsEmpty methods. A couple of other files were using redundant overloaded methods that I deleted for simplicity, and while I could not find any direct unit tests for those methods, they are likely tested by any rendering code. One of the files is used for image decompression and should be covered by any image decompression tests.

The test cases were added first and showed the following failures before updating the methods:

Failures before the fix 
Note: Google Test filter = *IsEmpty*
[==========] Running 4 tests from 1 test suite.
[----------] Global test environment set-up.
[----------] 4 tests from SizeTest
[ RUN      ] SizeTest.RectIsEmpty
../../flutter/impeller/geometry/rect_unittests.cc:77: Failure
Value of: Rect::MakeXYWH(1.5, 2.3, 10.5, nan).IsEmpty()
  Actual: false
Expected: true
../../flutter/impeller/geometry/rect_unittests.cc:78: Failure
Value of: Rect::MakeXYWH(1.5, 2.3, nan, 7.2).IsEmpty()
  Actual: false
Expected: true
../../flutter/impeller/geometry/rect_unittests.cc:79: Failure
Value of: Rect::MakeXYWH(1.5, 2.3, nan, nan).IsEmpty()
  Actual: false
Expected: true
[  FAILED  ] SizeTest.RectIsEmpty (0 ms)
[ RUN      ] SizeTest.IRectIsEmpty
[       OK ] SizeTest.IRectIsEmpty (0 ms)
[ RUN      ] SizeTest.SizeIsEmpty
../../flutter/impeller/geometry/size_unittests.cc:31: Failure
Value of: Size(10.5, nan).IsEmpty()
  Actual: false
Expected: true
../../flutter/impeller/geometry/size_unittests.cc:32: Failure
Value of: Size(nan, 7.2).IsEmpty()
  Actual: false
Expected: true
../../flutter/impeller/geometry/size_unittests.cc:33: Failure
Value of: Size(nan, nan).IsEmpty()
  Actual: false
Expected: true
[  FAILED  ] SizeTest.SizeIsEmpty (0 ms)
[ RUN      ] SizeTest.ISizeIsEmpty
[       OK ] SizeTest.ISizeIsEmpty (0 ms)
[----------] 4 tests from SizeTest (0 ms total)

[----------] Global test environment tear-down
[==========] 4 tests from 1 test suite ran. (0 ms total)
[  PASSED  ] 2 tests.
[  FAILED  ] 2 tests, listed below:
[  FAILED  ] SizeTest.RectIsEmpty
[  FAILED  ] SizeTest.SizeIsEmpty
Results after the fix 
Note: Google Test filter = *IsEmpty*
[==========] Running 4 tests from 1 test suite.
[----------] Global test environment set-up.
[----------] 4 tests from SizeTest
[ RUN      ] SizeTest.RectIsEmpty
[       OK ] SizeTest.RectIsEmpty (0 ms)
[ RUN      ] SizeTest.IRectIsEmpty
[       OK ] SizeTest.IRectIsEmpty (0 ms)
[ RUN      ] SizeTest.SizeIsEmpty
[       OK ] SizeTest.SizeIsEmpty (0 ms)
[ RUN      ] SizeTest.ISizeIsEmpty
[       OK ] SizeTest.ISizeIsEmpty (0 ms)
[----------] 4 tests from SizeTest (0 ms total)

[----------] Global test environment tear-down
[==========] 4 tests from 1 test suite ran. (0 ms total)
[  PASSED  ] 4 tests.

@flar flar added the autosubmit Merge PR when tree becomes green via auto submit App label Nov 7, 2023
@auto-submit auto-submit bot merged commit 3e3be5e into flutter:main Nov 7, 2023
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Nov 7, 2023
Merged
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Nov 7, 2023
@engine-flutter-autoroll
3e3be5e [Impeller] Make IsEmpty methods on Rect and Size NaN-aware (f…
b139695 
…lutter/engine#47725)
auto-submit bot pushed a commit to flutter/flutter that referenced this pull request Nov 7, 2023
@engine-flutter-autoroll
Roll Flutter Engine from f8961d2 to 3e3be5e (1 revision) (#138039)
18812ff 
flutter/engine@f8961d2...3e3be5e

2023-11-07 [email protected] [Impeller] Make IsEmpty methods on Rect and Size NaN-aware (flutter/engine#47725)

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
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@chinmaygarde chinmaygarde Awaiting requested review from chinmaygarde

1 more reviewer

@matanlurey matanlurey matanlurey approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

autosubmit Merge PR when tree becomes green via auto submit App e: impeller

Projects

No open projects
Impeller
Archived in project

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@flar @matanlurey