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

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
auto-submit bot pushed a commit to flutter/flutter that referenced this pull request Nov 7, 2023
…138039)

flutter/engine@f8961d2...3e3be5e

2023-11-07 flar@google.com [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 bdero@google.com,rmistry@google.com,zra@google.com 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.

Labels

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

Projects

No open projects
Archived in project

Development

Successfully merging this pull request may close these issues.

2 participants