Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Impeller] switch Rect fields to LTRB implementation #49816

Merged
merged 7 commits into from
Jan 19, 2024

Conversation

flar
Copy link
Contributor

@flar flar commented Jan 16, 2024

Switches the Rect class implementation to an LTRB (Left, Top, Right, Bottom) set of fields for efficiency in performing rectangle operations. Additionally, the methods have been somewhat hardened to the following issues:

  • NaN values should always act as if the rect is empty
  • Saturated math methods are added to avoid overflow for integer rects
  • Normalized treatment of empty rectangles in rect/rect operations
    • empty.Union(anything) == anything
    • empty.Intersection(anything) == empty
    • empty.IntersectsWith(anything) == false
    • empty.Contains(anything) == false
    • non-empty.Contains(empty) == true
  • MakeMaximum now returns finite rectangles which should produce non-nan values from all getters
  • A lot more testing of all of these behaviors within the unit tests

In addition, the new rectangle is closer to what DisplayList and the Skia backend have been using all along so we might be able to switch to using Impeller geometry inside DisplayList to reduce overhead for both backends.

Copy link
Member

@jonahwilliams jonahwilliams left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've only taken a quick look through the good, but overall it looks great.

LMK if there is anything I can do to help get this landed.

impeller/geometry/rect.h Outdated Show resolved Hide resolved
impeller/geometry/rect.h Outdated Show resolved Hide resolved
impeller/geometry/rect_unittests.cc Outdated Show resolved Hide resolved
@flar
Copy link
Contributor Author

flar commented Jan 17, 2024

I've only taken a quick look through the good, but overall it looks great.

LMK if there is anything I can do to help get this landed.

It looks like the tests are failing on Intel platforms due to overflow casts from the ceil method returning the wrong answer. It looks like I'll need to add some saturated::Floor and Ceil methods to work around this.

Also, only on the mac_host_engine, not on the Intel Linux builds, it looks like the saturated Add and Sub are failing to saturate correctly.

I'm looking into the failure that happens on Intel Linux with a cloud VM, but I'll have to find access to an Intel Mac somewhere to figure out why the Add and Sub aren't saturating.

@flar
Copy link
Contributor Author

flar commented Jan 17, 2024

I've implemented saturated casts which should help with most of the previous failures, but I still have to track down the failures in saturated Add/Sub on Intel Macs.

@flar flar force-pushed the impeller-rect-ltrb-implementation branch 2 times, most recently from cb6c46b to ee8f6cc Compare January 18, 2024 19:21
@flar flar force-pushed the impeller-rect-ltrb-implementation branch from 7b8dc38 to ddd0764 Compare January 19, 2024 04:26
Copy link
Member

@jonahwilliams jonahwilliams left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@flar flar added the autosubmit Merge PR when tree becomes green via auto submit App label Jan 19, 2024
@auto-submit auto-submit bot merged commit c953c83 into flutter:main Jan 19, 2024
29 checks passed
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jan 19, 2024
auto-submit bot pushed a commit to flutter/flutter that referenced this pull request Jan 19, 2024
…141886)

flutter/engine@538975f...c953c83

2024-01-19 flar@google.com [Impeller] switch Rect fields to LTRB implementation (flutter/engine#49816)
2024-01-19 68449066+zijiehe-google-com@users.noreply.github.com [Fuchsia] Redo - Use chromium test-scripts to download images and execute tests (flutter/engine#49847)
2024-01-19 skia-flutter-autoroll@skia.org Roll Skia from fb3f61d932e6 to 2536dc6fef1d (2 revisions) (flutter/engine#49892)

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 jonahwilliams@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 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 e: impeller
Projects
No open projects
Status: ✅ Done
Development

Successfully merging this pull request may close these issues.

3 participants