From edabadf4e3eea645b540e63f6463f8cd1354c031 Mon Sep 17 00:00:00 2001 From: Tong Mu Date: Thu, 22 May 2025 15:40:54 -0700 Subject: [PATCH] [Engine] Fix an edge case of RoundSuperellipseParam::Contains with sharp corners (#167977) This PR fixes an edge case where `RoundSuperellipseParam::Contains` would incorrectly judge a point if the point is outside a sharp corner. Although this case would have been rejected by the rect filter, `RSuperellipse.contains` (which is called by `dart:ui`) did not use a rect filter, causing this bug to affect Dart only. This PR also adds rect filter to it. ## Pre-launch Checklist - [ ] I read the [Contributor Guide] and followed the process outlined there for submitting PRs. - [ ] I read the [Tree Hygiene] wiki page, which explains my responsibilities. - [ ] I read and followed the [Flutter Style Guide], including [Features we expect every widget to implement]. - [ ] I signed the [CLA]. - [ ] I listed at least one issue that this PR fixes in the description above. - [ ] I updated/added relevant documentation (doc comments with `///`). - [ ] I added new tests to check the change I am making, or this PR is [test-exempt]. - [ ] I followed the [breaking change policy] and added [Data Driven Fixes] where supported. - [ ] All existing and new tests are passing. If you need help, consider asking for advice on the #hackers-new channel on [Discord]. [Contributor Guide]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#overview [Tree Hygiene]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md [test-exempt]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#tests [Flutter Style Guide]: https://github.com/flutter/flutter/blob/main/docs/contributing/Style-guide-for-Flutter-repo.md [Features we expect every widget to implement]: https://github.com/flutter/flutter/blob/main/docs/contributing/Style-guide-for-Flutter-repo.md#features-we-expect-every-widget-to-implement [CLA]: https://cla.developers.google.com/ [flutter/tests]: https://github.com/flutter/tests [breaking change policy]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#handling-breaking-changes [Discord]: https://github.com/flutter/flutter/blob/main/docs/contributing/Chat.md [Data Driven Fixes]: https://github.com/flutter/flutter/blob/main/docs/contributing/Data-driven-Fixes.md --- .../geometry/round_superellipse_param.cc | 40 ++++++++++++------- .../geometry/round_superellipse_unittests.cc | 18 +++++++++ .../flutter/lib/ui/painting/rsuperellipse.cc | 6 ++- .../flutter/testing/dart/geometry_test.dart | 14 +++++++ 4 files changed, 62 insertions(+), 16 deletions(-) diff --git a/engine/src/flutter/impeller/geometry/round_superellipse_param.cc b/engine/src/flutter/impeller/geometry/round_superellipse_param.cc index 8f282d89082..d67873b67da 100644 --- a/engine/src/flutter/impeller/geometry/round_superellipse_param.cc +++ b/engine/src/flutter/impeller/geometry/round_superellipse_param.cc @@ -17,9 +17,11 @@ Scalar Split(Scalar left, Scalar right, Scalar ratio_left, Scalar ratio_right) { return (left * ratio_right + right * ratio_left) / (ratio_left + ratio_right); } -// Return the same Point, but each NaN coordinate is replaced by 1. -inline Point ReplanceNaNWithOne(Point in) { - return Point{std::isnan(in.x) ? 1 : in.x, std::isnan(in.y) ? 1 : in.y}; +// Return the same Point, but each NaN coordinate is replaced by that of +// `default_size`. +inline Point ReplanceNaNWithDefault(Point in, Size default_size) { + return Point{std::isnan(in.x) ? default_size.width : in.x, + std::isnan(in.y) ? default_size.height : in.y}; } // Swap the x and y coordinate of a point. @@ -192,11 +194,15 @@ RoundSuperellipseParam::Octant ComputeOctant(Point center, // radii. // // The `corner` is the coordinate of the corner point in the same coordinate -// space as `center`, which specifies both the half size of the bounding box and -// which quadrant the curve should be. +// space as `center`, which specifies the half size of the bounding box. +// +// The `sign` is a vector of {±1, ±1} that specifies which quadrant the curve +// should be, which should have the same sign as `corner - center` except that +// the latter may have a 0. RoundSuperellipseParam::Quadrant ComputeQuadrant(Point center, Point corner, - Size in_radii) { + Size in_radii, + Size sign) { Point corner_vector = corner - center; Size radii = in_radii.Abs(); @@ -207,7 +213,8 @@ RoundSuperellipseParam::Quadrant ComputeQuadrant(Point center, Scalar norm_radius = radii.MinDimension(); Size forward_scale = norm_radius == 0 ? Size{1, 1} : radii / norm_radius; Point norm_half_size = corner_vector.Abs() / forward_scale; - Point signed_scale = ReplanceNaNWithOne(corner_vector / norm_half_size); + Point signed_scale = + ReplanceNaNWithDefault(corner_vector / norm_half_size, sign); // Each quadrant curve is composed of two octant curves, each of which belongs // to a square-like rounded rectangle. For the two octants to connect at the @@ -469,7 +476,7 @@ RoundSuperellipseParam RoundSuperellipseParam::MakeBoundsRadii( // treatment on border containment and therefore is not `all_corners_same`. return RoundSuperellipseParam{ .top_right = ComputeQuadrant(bounds.GetCenter(), bounds.GetRightTop(), - radii.top_right), + radii.top_right, {-1, 1}), .all_corners_same = true, }; } @@ -484,15 +491,18 @@ RoundSuperellipseParam RoundSuperellipseParam::MakeBoundsRadii( radii.top_left.height, radii.bottom_left.height); return RoundSuperellipseParam{ - .top_right = ComputeQuadrant(Point{top_split, right_split}, - bounds.GetRightTop(), radii.top_right), + .top_right = + ComputeQuadrant(Point{top_split, right_split}, bounds.GetRightTop(), + radii.top_right, {1, -1}), .bottom_right = ComputeQuadrant(Point{bottom_split, right_split}, - bounds.GetRightBottom(), radii.bottom_right), - .bottom_left = ComputeQuadrant(Point{bottom_split, left_split}, - bounds.GetLeftBottom(), radii.bottom_left), - .top_left = ComputeQuadrant(Point{top_split, left_split}, - bounds.GetLeftTop(), radii.top_left), + bounds.GetRightBottom(), radii.bottom_right, {1, 1}), + .bottom_left = + ComputeQuadrant(Point{bottom_split, left_split}, + bounds.GetLeftBottom(), radii.bottom_left, {-1, 1}), + .top_left = + ComputeQuadrant(Point{top_split, left_split}, bounds.GetLeftTop(), + radii.top_left, {-1, -1}), .all_corners_same = false, }; } diff --git a/engine/src/flutter/impeller/geometry/round_superellipse_unittests.cc b/engine/src/flutter/impeller/geometry/round_superellipse_unittests.cc index 86324be1f48..95d9273bbb9 100644 --- a/engine/src/flutter/impeller/geometry/round_superellipse_unittests.cc +++ b/engine/src/flutter/impeller/geometry/round_superellipse_unittests.cc @@ -5,6 +5,7 @@ #include "gtest/gtest.h" #include "flutter/impeller/geometry/round_superellipse.h" +#include "flutter/impeller/geometry/round_superellipse_param.h" #include "flutter/impeller/geometry/geometry_asserts.h" @@ -692,5 +693,22 @@ TEST(RoundSuperellipseTest, SlimDiagnalContains) { #undef CHECK_POINT_AND_MIRRORS } +TEST(RoundSuperellipseTest, PointsOutsideOfSharpCorner) { + Rect bounds = Rect::MakeLTRB(196.0f, 0.0f, 294.0f, 28.0f); + // Regression test for a case where RoundSuperellipseParam::Contains + // previously failed. Although the bounding rect filter of + // `RoundSuperellipse::Contains` would reject this point, this test ensures + // the internal logic of RoundSuperellipseParam::Contains is now correct. + auto rr = RoundSuperellipseParam::MakeBoundsRadii( + bounds, { + .top_left = Size(0.0, 0.0), + .top_right = Size(3.0, 3.0), + .bottom_left = Size(0.0, 0.0), + .bottom_right = Size(3.0, 3.0), + }); + + EXPECT_FALSE(rr.Contains(Point{147.0, 14.0})); +} + } // namespace testing } // namespace impeller diff --git a/engine/src/flutter/lib/ui/painting/rsuperellipse.cc b/engine/src/flutter/lib/ui/painting/rsuperellipse.cc index 9f193954dc8..c8e39ce54f3 100644 --- a/engine/src/flutter/lib/ui/painting/rsuperellipse.cc +++ b/engine/src/flutter/lib/ui/painting/rsuperellipse.cc @@ -81,7 +81,11 @@ impeller::RoundSuperellipseParam RSuperellipse::param() const { } bool RSuperellipse::contains(double x, double y) { - return param().Contains(DlPoint(SafeNarrow(x), SafeNarrow(y))); + DlPoint point(SafeNarrow(x), SafeNarrow(y)); + if (!bounds_.Contains(point)) { + return false; + } + return param().Contains(point); } } // namespace flutter diff --git a/engine/src/flutter/testing/dart/geometry_test.dart b/engine/src/flutter/testing/dart/geometry_test.dart index ac6897f4691..54b346489e1 100644 --- a/engine/src/flutter/testing/dart/geometry_test.dart +++ b/engine/src/flutter/testing/dart/geometry_test.dart @@ -704,6 +704,20 @@ void main() { checkDiagnalPoints(const Offset(21.05, -20.92)); checkDiagnalPoints(const Offset(40.0, 5.68)); }); + + test('RSuperellipse.contains is correct for points outside of a sharp corner', () { + expect( + RSuperellipse.fromLTRBAndCorners( + 196.0, + 0.0, + 294.0, + 28.0, + topRight: const Radius.circular(3.0), + bottomRight: const Radius.circular(3.0), + ).contains(const Offset(147.0, 14.0)), + isFalse, + ); + }); } void checkPointWithOffset(RSuperellipse rse, Offset inPoint, Offset outwardOffset) {