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.

Sign up for GitHub

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

[ios_platform_view] more precision when determine if a clip rrect is necessary #38965

Merged
merged 3 commits into from
Jan 18, 2023
Merged

[ios_platform_view] more precision when determine if a clip rrect is necessary #38965

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
bd92e72
draft
Jan 17, 2023
fb26979
update scenario
Jan 18, 2023
2c44fa0
test
Jan 18, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
63 changes: 47 additions & 16 deletions shell/platform/darwin/ios/framework/Source/FlutterPlatformViews.mm
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,20 +32,52 @@ - (BOOL)flt_hasFirstResponderInViewHierarchySubtree {
}
@end

// Determines if the final `clipBounds` from a clipRect/clipRRect/clipPath mutator contains the
// Determines if the `clip_rect` from a clipRect mutator contains the
// `platformview_boundingrect`.
//
// `clip_bounds` is the bounding rect of the rect/rrect/path in the clipRect/clipRRect/clipPath
// mutator. This rect is in its own coordinate space. The rect needs to be transformed by
// `clip_rect` is in its own coordinate space. The rect needs to be transformed by
// `transform_matrix` to be in the coordinate space where the PlatformView is displayed.
//
// `platformview_boundingrect` is the final bounding rect of the PlatformView in the coordinate
// space where the PlatformView is displayed.
static bool ClipBoundsContainsPlatformViewBoundingRect(const SkRect& clip_bounds,
const SkRect& platformview_boundingrect,
const SkMatrix& transform_matrix) {
SkRect transforme_clip_bounds = transform_matrix.mapRect(clip_bounds);
return transforme_clip_bounds.contains(platformview_boundingrect);
static bool ClipRectContainsPlatformViewBoundingRect(const SkRect& clip_rect,
const SkRect& platformview_boundingrect,
const SkMatrix& transform_matrix) {
SkRect transformed_rect = transform_matrix.mapRect(clip_rect);
return transformed_rect.contains(platformview_boundingrect);
}

// Determines if the `clipRRect` from a clipRRect mutator contains the
// `platformview_boundingrect`.
//
// `clip_rrect` is in its own coordinate space. The rrect needs to be transformed by
// `transform_matrix` to be in the coordinate space where the PlatformView is displayed.
//
// `platformview_boundingrect` is the final bounding rect of the PlatformView in the coordinate
// space where the PlatformView is displayed.
static bool ClipRRectContainsPlatformViewBoundingRect(const SkRRect& clip_rrect,
const SkRect& platformview_boundingrect,
const SkMatrix& transform_matrix) {
SkVector upper_left = clip_rrect.radii(SkRRect::Corner::kUpperLeft_Corner);
SkVector upper_right = clip_rrect.radii(SkRRect::Corner::kUpperRight_Corner);
SkVector lower_right = clip_rrect.radii(SkRRect::Corner::kLowerRight_Corner);
SkVector lower_left = clip_rrect.radii(SkRRect::Corner::kLowerLeft_Corner);
SkScalar transformed_upper_left_x = transform_matrix.mapRadius(upper_left.x());
SkScalar transformed_upper_left_y = transform_matrix.mapRadius(upper_left.y());
SkScalar transformed_upper_right_x = transform_matrix.mapRadius(upper_right.x());
SkScalar transformed_upper_right_y = transform_matrix.mapRadius(upper_right.y());
SkScalar transformed_lower_right_x = transform_matrix.mapRadius(lower_right.x());
SkScalar transformed_lower_right_y = transform_matrix.mapRadius(lower_right.y());
SkScalar transformed_lower_left_x = transform_matrix.mapRadius(lower_left.x());
SkScalar transformed_lower_left_y = transform_matrix.mapRadius(lower_left.y());
SkRect transformed_clip_rect = transform_matrix.mapRect(clip_rrect.rect());
SkRRect transformed_rrect;
SkVector corners[] = {{transformed_upper_left_x, transformed_upper_left_y},
{transformed_upper_right_x, transformed_upper_right_y},
{transformed_lower_right_x, transformed_lower_right_y},
{transformed_lower_left_x, transformed_lower_left_y}};
transformed_rrect.setRectRadii(transformed_clip_rect, corners);
return transformed_rrect.contains(platformview_boundingrect);
Copy link
Contributor

Choose a reason for hiding this comment

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

i assume this contains actually take its radii into consideration (rather than just checking rectangular bounds)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep, it does. the new scenario app test covers it. The new unit test also covered it. I had to use (-1, -1) as the origin for the clipRRect with a 1 radius to avoid clipping the PlatformView at (0, 0) origin.

}

namespace flutter {
Expand Down Expand Up @@ -450,28 +482,27 @@ static bool ClipBoundsContainsPlatformViewBoundingRect(const SkRect& clip_bounds
break;
}
case kClipRect: {
if (ClipBoundsContainsPlatformViewBoundingRect((*iter)->GetRect(), bounding_rect,
transformMatrix)) {
if (ClipRectContainsPlatformViewBoundingRect((*iter)->GetRect(), bounding_rect,
transformMatrix)) {
break;
}
[maskView clipRect:(*iter)->GetRect() matrix:transformMatrix];
clipView.maskView = maskView;
break;
}
case kClipRRect: {
if (ClipBoundsContainsPlatformViewBoundingRect((*iter)->GetRRect().getBounds(),
bounding_rect, transformMatrix)) {
if (ClipRRectContainsPlatformViewBoundingRect((*iter)->GetRRect(), bounding_rect,
transformMatrix)) {
break;
}
[maskView clipRRect:(*iter)->GetRRect() matrix:transformMatrix];
clipView.maskView = maskView;
break;
}
case kClipPath: {
if (ClipBoundsContainsPlatformViewBoundingRect((*iter)->GetPath().getBounds(),
bounding_rect, transformMatrix)) {
break;
}
// TODO(cyanglaz): Find a way to pre-determine if path contains the PlatformView boudning
Copy link
Contributor

Choose a reason for hiding this comment

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

this is probably a hard geometry problem. but i bet most clipping actions are just simple shapes like RRect

// rect. See `ClipRRectContainsPlatformViewBoundingRect`.
// https://github.com/flutter/flutter/issues/118650
[maskView clipPath:(*iter)->GetPath() matrix:transformMatrix];
clipView.maskView = maskView;
break;
Expand Down
81 changes: 71 additions & 10 deletions shell/platform/darwin/ios/framework/Source/FlutterPlatformViewsTest.mm
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1510,25 +1510,23 @@ - (void)testClipsDoNotInterceptWithPlatformViewShouldNotAddMaskView {

UIView* mockFlutterView = [[[UIView alloc] initWithFrame:CGRectMake(0, 0, 30, 30)] autorelease];
flutterPlatformViewsController->SetFlutterView(mockFlutterView);
// Create embedded view params
// Create embedded view params.
flutter::MutatorsStack stack;
// Layer tree always pushes a screen scale factor to the stack
// Layer tree always pushes a screen scale factor to the stack.
SkMatrix screenScaleMatrix =
SkMatrix::Scale([UIScreen mainScreen].scale, [UIScreen mainScreen].scale);
stack.PushTransform(screenScaleMatrix);
SkMatrix translateMatrix = SkMatrix::Translate(5, 5);
// The platform view's rect for this test will be (5, 5, 10, 10)
// The platform view's rect for this test will be (5, 5, 10, 10).
stack.PushTransform(translateMatrix);
// Push a clip rect, big enough to contain the entire platform view bound
// Push a clip rect, big enough to contain the entire platform view bound.
SkRect rect = SkRect::MakeXYWH(0, 0, 25, 25);
stack.PushClipRect(rect);
// Push a clip rrect, big enough to contain the entire platform view bound
SkRect rect_for_rrect = SkRect::MakeXYWH(0, 0, 24, 24);
// Push a clip rrect, big enough to contain the entire platform view bound without clipping it.
// Make the origin (-1, -1) so that the top left rounded corner isn't clipping the PlatformView.
SkRect rect_for_rrect = SkRect::MakeXYWH(-1, -1, 25, 25);
SkRRect rrect = SkRRect::MakeRectXY(rect_for_rrect, 1, 1);
stack.PushClipRRect(rrect);
// Push a clip path, big enough to contain the entire platform view bound
SkPath path = SkPath::RRect(SkRect::MakeXYWH(0, 0, 23, 23), 1, 1);
stack.PushClipPath(path);

auto embeddedViewParams = std::make_unique<flutter::EmbeddedViewParams>(
SkMatrix::Concat(screenScaleMatrix, translateMatrix), SkSize::Make(5, 5), stack);
Expand All @@ -1542,10 +1540,73 @@ - (void)testClipsDoNotInterceptWithPlatformViewShouldNotAddMaskView {

[mockFlutterView setNeedsLayout];
[mockFlutterView layoutIfNeeded];

XCTAssertNil(childClippingView.maskView);
}

- (void)testClipRRectOnlyHasCornersInterceptWithPlatformViewShouldAddMaskView {
flutter::FlutterPlatformViewsTestMockPlatformViewDelegate mock_delegate;
auto thread_task_runner = CreateNewThread("FlutterPlatformViewsTest");
flutter::TaskRunners runners(/*label=*/self.name.UTF8String,
/*platform=*/thread_task_runner,
/*raster=*/thread_task_runner,
/*ui=*/thread_task_runner,
/*io=*/thread_task_runner);
auto flutterPlatformViewsController = std::make_shared<flutter::FlutterPlatformViewsController>();
auto platform_view = std::make_unique<flutter::PlatformViewIOS>(
/*delegate=*/mock_delegate,
/*rendering_api=*/flutter::IOSRenderingAPI::kSoftware,
/*platform_views_controller=*/flutterPlatformViewsController,
/*task_runners=*/runners);

FlutterPlatformViewsTestMockFlutterPlatformFactory* factory =
[[FlutterPlatformViewsTestMockFlutterPlatformFactory new] autorelease];
flutterPlatformViewsController->RegisterViewFactory(
factory, @"MockFlutterPlatformView",
FlutterPlatformViewGestureRecognizersBlockingPolicyEager);
FlutterResult result = ^(id result) {
};
flutterPlatformViewsController->OnMethodCall(
[FlutterMethodCall
methodCallWithMethodName:@"create"
arguments:@{@"id" : @2, @"viewType" : @"MockFlutterPlatformView"}],
result);

XCTAssertNotNil(gMockPlatformView);

UIView* mockFlutterView = [[[UIView alloc] initWithFrame:CGRectMake(0, 0, 30, 30)] autorelease];
flutterPlatformViewsController->SetFlutterView(mockFlutterView);
// Create embedded view params
flutter::MutatorsStack stack;
// Layer tree always pushes a screen scale factor to the stack.
SkMatrix screenScaleMatrix =
SkMatrix::Scale([UIScreen mainScreen].scale, [UIScreen mainScreen].scale);
stack.PushTransform(screenScaleMatrix);
SkMatrix translateMatrix = SkMatrix::Translate(5, 5);
// The platform view's rect for this test will be (5, 5, 10, 10).
stack.PushTransform(translateMatrix);

// Push a clip rrect, the rect of the rrect is the same as the PlatformView of the corner should.
// clip the PlatformView.
SkRect rect_for_rrect = SkRect::MakeXYWH(0, 0, 10, 10);
SkRRect rrect = SkRRect::MakeRectXY(rect_for_rrect, 1, 1);
stack.PushClipRRect(rrect);

auto embeddedViewParams = std::make_unique<flutter::EmbeddedViewParams>(
SkMatrix::Concat(screenScaleMatrix, translateMatrix), SkSize::Make(5, 5), stack);

flutterPlatformViewsController->PrerollCompositeEmbeddedView(2, std::move(embeddedViewParams));
flutterPlatformViewsController->CompositeEmbeddedView(2);
gMockPlatformView.backgroundColor = UIColor.redColor;
XCTAssertTrue([gMockPlatformView.superview.superview isKindOfClass:ChildClippingView.class]);
ChildClippingView* childClippingView = (ChildClippingView*)gMockPlatformView.superview.superview;
[mockFlutterView addSubview:childClippingView];

[mockFlutterView setNeedsLayout];
[mockFlutterView layoutIfNeeded];

XCTAssertNotNil(childClippingView.maskView);
}

- (void)testClipRect {
flutter::FlutterPlatformViewsTestMockPlatformViewDelegate mock_delegate;
auto thread_task_runner = CreateNewThread("FlutterPlatformViewsTest");
Expand Down
8 changes: 8 additions & 0 deletions testing/scenario_app/ios/Scenarios/Scenarios.xcodeproj/project.pbxproj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,8 @@
6816DB9E231750ED00A51400 /* GoldenPlatformViewTests.m in Sources */ = {isa = PBXBuildFile; fileRef = 6816DB9D231750ED00A51400 /* GoldenPlatformViewTests.m */; };
6816DBA12317573300A51400 /* GoldenImage.m in Sources */ = {isa = PBXBuildFile; fileRef = 6816DBA02317573300A51400 /* GoldenImage.m */; };
6816DBA42318358200A51400 /* GoldenTestManager.m in Sources */ = {isa = PBXBuildFile; fileRef = 6816DBA32318358200A51400 /* GoldenTestManager.m */; };
685B9F392977B73100B45442 /* golden_platform_view_large_cliprrect_with_transform_iPhone 8_16.0_simulator.png in Resources */ = {isa = PBXBuildFile; fileRef = 685B9F372977B73100B45442 /* golden_platform_view_large_cliprrect_with_transform_iPhone 8_16.0_simulator.png */; };
685B9F3A2977B73100B45442 /* golden_platform_view_large_cliprrect_iPhone 8_16.0_simulator.png in Resources */ = {isa = PBXBuildFile; fileRef = 685B9F382977B73100B45442 /* golden_platform_view_large_cliprrect_iPhone 8_16.0_simulator.png */; };
687AF8E9291EBDE0003912C7 /* golden_platform_view_clippath_with_transform_iPhone 8_16.0_simulator.png in Resources */ = {isa = PBXBuildFile; fileRef = 687AF8E8291EBDE0003912C7 /* golden_platform_view_clippath_with_transform_iPhone 8_16.0_simulator.png */; };
68A5B63423EB71D300BDBCDB /* PlatformViewGestureRecognizerTests.m in Sources */ = {isa = PBXBuildFile; fileRef = 68A5B63323EB71D300BDBCDB /* PlatformViewGestureRecognizerTests.m */; };
68D4017D2564859300ECD91A /* ContinuousTexture.m in Sources */ = {isa = PBXBuildFile; fileRef = 68D4017C2564859300ECD91A /* ContinuousTexture.m */; };
Expand Down Expand Up @@ -151,6 +153,8 @@
6816DBA02317573300A51400 /* GoldenImage.m */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.c.objc; path = GoldenImage.m; sourceTree = "<group>"; };
6816DBA22318358200A51400 /* GoldenTestManager.h */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.c.h; path = GoldenTestManager.h; sourceTree = "<group>"; };
6816DBA32318358200A51400 /* GoldenTestManager.m */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.c.objc; path = GoldenTestManager.m; sourceTree = "<group>"; };
685B9F372977B73100B45442 /* golden_platform_view_large_cliprrect_with_transform_iPhone 8_16.0_simulator.png */ = {isa = PBXFileReference; lastKnownFileType = image.png; path = "golden_platform_view_large_cliprrect_with_transform_iPhone 8_16.0_simulator.png"; sourceTree = "<group>"; };
685B9F382977B73100B45442 /* golden_platform_view_large_cliprrect_iPhone 8_16.0_simulator.png */ = {isa = PBXFileReference; lastKnownFileType = image.png; path = "golden_platform_view_large_cliprrect_iPhone 8_16.0_simulator.png"; sourceTree = "<group>"; };
687AF8E8291EBDE0003912C7 /* golden_platform_view_clippath_with_transform_iPhone 8_16.0_simulator.png */ = {isa = PBXFileReference; lastKnownFileType = image.png; path = "golden_platform_view_clippath_with_transform_iPhone 8_16.0_simulator.png"; sourceTree = "<group>"; };
68A5B63323EB71D300BDBCDB /* PlatformViewGestureRecognizerTests.m */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.c.objc; path = PlatformViewGestureRecognizerTests.m; sourceTree = "<group>"; };
68D4017B2564859300ECD91A /* ContinuousTexture.h */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.c.h; path = ContinuousTexture.h; sourceTree = "<group>"; };
Expand Down Expand Up @@ -296,6 +300,8 @@
F7B464DC2759D02B00079189 /* Goldens */ = {
isa = PBXGroup;
children = (
685B9F382977B73100B45442 /* golden_platform_view_large_cliprrect_iPhone 8_16.0_simulator.png */,
685B9F372977B73100B45442 /* golden_platform_view_large_cliprrect_with_transform_iPhone 8_16.0_simulator.png */,
68D50041291ED8CD001ACFE1 /* golden_platform_view_cliprect_with_transform_iPhone 8_16.0_simulator.png */,
68D5003D291ED645001ACFE1 /* golden_platform_view_cliprrect_with_transform_iPhone 8_16.0_simulator.png */,
687AF8E8291EBDE0003912C7 /* golden_platform_view_clippath_with_transform_iPhone 8_16.0_simulator.png */,
Expand Down Expand Up @@ -452,10 +458,12 @@
F7B464F32759D0A900079189 /* golden_platform_view_multiple_background_foreground_iPhone 8_16.0_simulator.png in Resources */,
F7B464F72759D0A900079189 /* golden_platform_view_rotate_iPhone 8_16.0_simulator.png in Resources */,
F7B464ED2759D0A900079189 /* golden_platform_view_cliprrect_iPhone 8_16.0_simulator.png in Resources */,
685B9F3A2977B73100B45442 /* golden_platform_view_large_cliprrect_iPhone 8_16.0_simulator.png in Resources */,
68D5003F291ED645001ACFE1 /* golden_platform_view_cliprrect_with_transform_iPhone 8_16.0_simulator.png in Resources */,
F7B464EB2759D0A900079189 /* golden_two_platform_views_with_other_backdrop_filter_iPhone 8_16.0_simulator.png in Resources */,
F7B464F42759D0A900079189 /* golden_platform_view_with_other_backdrop_filter_iPhone 8_16.0_simulator.png in Resources */,
687AF8E9291EBDE0003912C7 /* golden_platform_view_clippath_with_transform_iPhone 8_16.0_simulator.png in Resources */,
685B9F392977B73100B45442 /* golden_platform_view_large_cliprrect_with_transform_iPhone 8_16.0_simulator.png in Resources */,
F769EB53276312BB007AC10F /* golden_platform_view_cliprect_iPhone 8_16.0_simulator.png in Resources */,
F7B464EF2759D0A900079189 /* golden_platform_view_multiple_iPhone 8_16.0_simulator.png in Resources */,
);
Expand Down
3 changes: 3 additions & 0 deletions testing/scenario_app/ios/Scenarios/Scenarios/AppDelegate.m
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,8 +43,11 @@ - (BOOL)application:(UIApplication*)application
@"platform_view_multiple_background_foreground",
@"--platform-view-cliprect" : @"platform_view_cliprect",
@"--platform-view-cliprrect" : @"platform_view_cliprrect",
@"--platform-view-large-cliprrect" : @"platform_view_large_cliprrect",
@"--platform-view-clippath" : @"platform_view_clippath",
@"--platform-view-cliprrect-with-transform" : @"platform_view_cliprrect_with_transform",
@"--platform-view-large-cliprrect-with-transform" :
@"platform_view_large_cliprrect_with_transform",
@"--platform-view-cliprect-with-transform" : @"platform_view_cliprect_with_transform",
@"--platform-view-clippath-with-transform" : @"platform_view_clippath_with_transform",
@"--platform-view-transform" : @"platform_view_transform",
Expand Down
3 changes: 3 additions & 0 deletions testing/scenario_app/ios/Scenarios/ScenariosUITests/GoldenTestManager.m
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,8 +29,11 @@ - (instancetype)initWithLaunchArg:(NSString*)launchArg {
@"platform_view_multiple_background_foreground",
@"--platform-view-cliprect" : @"platform_view_cliprect",
@"--platform-view-cliprrect" : @"platform_view_cliprrect",
@"--platform-view-large-cliprrect" : @"platform_view_large_cliprrect",
@"--platform-view-clippath" : @"platform_view_clippath",
@"--platform-view-cliprrect-with-transform" : @"platform_view_cliprrect_with_transform",
@"--platform-view-large-cliprrect-with-transform" :
@"platform_view_large_cliprrect_with_transform",
@"--platform-view-cliprect-with-transform" : @"platform_view_cliprect_with_transform",
@"--platform-view-clippath-with-transform" : @"platform_view_clippath_with_transform",
@"--platform-view-transform" : @"platform_view_transform",
Expand Down
36 changes: 36 additions & 0 deletions testing/scenario_app/ios/Scenarios/ScenariosUITests/PlatformViewUITests.m
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -116,6 +116,24 @@ - (void)testPlatformView {

@end

@interface PlatformViewMutationLargeClipRRectTests : GoldenPlatformViewTests

@end

@implementation PlatformViewMutationLargeClipRRectTests

- (instancetype)initWithInvocation:(NSInvocation*)invocation {
GoldenTestManager* manager =
[[GoldenTestManager alloc] initWithLaunchArg:@"--platform-view-large-cliprrect"];
return [super initWithManager:manager invocation:invocation];
}

- (void)testPlatformView {
[self checkPlatformViewGolden];
}

@end

@interface PlatformViewMutationClipPathTests : GoldenPlatformViewTests

@end
Expand Down Expand Up @@ -170,6 +188,24 @@ - (void)testPlatformView {

@end

@interface PlatformViewMutationLargeClipRRectWithTransformTests : GoldenPlatformViewTests

@end

@implementation PlatformViewMutationLargeClipRRectWithTransformTests

- (instancetype)initWithInvocation:(NSInvocation*)invocation {
GoldenTestManager* manager = [[GoldenTestManager alloc]
initWithLaunchArg:@"--platform-view-large-cliprrect-with-transform"];
return [super initWithManager:manager invocation:invocation];
}

- (void)testPlatformView {
[self checkPlatformViewGolden];
}

@end

@interface PlatformViewMutationClipPathWithTransformTests : GoldenPlatformViewTests

@end
Expand Down
Binary file added ...enariosUITests/golden_platform_view_large_cliprrect_iPhone 8_16.0_simulator.png
View file
Open in desktop
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Binary file added ...golden_platform_view_large_cliprrect_with_transform_iPhone 8_16.0_simulator.png
View file
Open in desktop
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading