[macOS] Implement platform view mutators #38699

knopp · 2023-01-07T16:11:40Z

This implements support for platform view mutators currently provided through the embedder API, namely:

Opacity
Clip Rect
Clip RRect
Transform

Notes

Clipping to paths (currently only round rect supported - see below) is done through CAShapeLayer masks, which avoids the need for rasterizing mask layer.
Clipping to round rectangle will only clip to path if one or more more corners are intersected by the view. Otherwise it behaves as if clipped by regular rectangle.

Clipping to arbitrary path is not currently supported by embedder API. However once there is a way to represent it in the embedder API it should be fairly straightforward to add to FlutterMutatorView.

Fixes flutter/flutter#118142

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 and the C++, Objective-C, Java style guides.
I listed at least one issue that this PR fixes in the description above.
I added new tests to check the change I am making or feature I am adding, or Hixie said the PR is test-exempt. See testing the engine for instructions on writing and running engine tests.
I updated/added relevant documentation (doc comments with ///).
I signed the CLA.
All existing and new tests are passing.

If you need help, consider asking for advice on the #hackers-new channel on Discord.

knopp · 2023-01-07T16:16:39Z

LayerPlayground example:

Mutators.mov

chinmaygarde · 2023-01-12T21:10:14Z

@cyanglaz might also be a reviewer with some context on this PR.

cyanglaz

Did a quick first scan through. I will do a detailed review next week.

cyanglaz · 2023-01-12T23:18:41Z

shell/platform/darwin/macos/framework/Source/FlutterMutatorView.h

+
+@interface FlutterMutatorView (Private)
+
+@property(readonly, nonatomic, nonnull) NSMutableArray<NSView*>* pathClipViews;


If the intention is to not mutate the array, it should be declared as NSArray.

This is only only accessed from tests, I'll make it NSArray.

If they are only accessed from tests, we can move the category to the test file instead.

Agreed; we should keep test-specific code with the tests.

I like the idea. I think in other parts of engine we also have private methods that could go into test. Maybe for a follow-up PR.

shell/platform/darwin/macos/framework/Source/FlutterMutatorView.mm

cyanglaz · 2023-01-13T22:33:46Z

shell/platform/darwin/macos/framework/Source/FlutterMutatorView.mm

+
+  NSView* lastView = self;
+
+  for (size_t i = 0; i < paths.count; ++i) {


iOS had a gesture problem when using this approach: flutter/flutter#46167
I'm not sure how drag/scroll events are handled on mac, it might or might not apply to macOS.

I haven't tried to address input yet. It is possible that this won't work with NSGestureRecognizer or rotate/magnify/swipe. Still, I'd want to avoid rasterizing the clip path as much as possible, since it uses additional memory and likely perform quite badly when clip shape changes frequently (i.e. animation).

So if current approach proves problematic and we can't change nesting on fly, alternatively we could always precreate N clip views, where N-1 views would be masked to CAShapeLayer, and the N-th one will contain rest of the paths rasterized.

Maybe dragging a PlatformView is a lot less common on macOS.

At least we should test it and document if the issue does exist.

precreate N clip views, where N-1 views would be masked to CAShapeLayer, and the N-th one will contain rest of the paths rasterized.

This sounds good to me. We can probably also adapt this on iOS to improve performance when there are no complex nested clips.

Another thing is that this PR clips to path only when round rect corners are affected. And in future possibly for path-based clip regions, but there's no way to pass those through embedder API yet. So in many cases, when the resulting clip is a rectangle, clipping is performed by container layer bounds set to intersection of clip rectangles. Which should be quite cheap. While I think the iOS version will clip to path even for rectangular clip? It's been a while, I might be misremembering it.

Another thing is that this PR clips to path only when round rect corners are affected.

Nice, this is a good performance enhancement.

While I think the iOS version will clip to path even for rectangular clip? It's been a while, I might be misremembering it.

Yes you are correct. We can definitely improve iOS performance by doing something similar: setting the frame of the ChildClippingView when the final clip result is a rect, and remove the maskView. This would be a good improvement.

cbracken · 2023-01-13T22:49:54Z

Should get to this over the next couple days - just got back from vacation and catching up on email/reviews. Thanks for sending this!

zanderso · 2023-02-02T21:27:30Z

@cbracken Friendly ping.

cbracken

Apologies for the long turn-around on this. Overall, this looks great. Thanks for authoring. Just the tweaks relating to the test specific properties, other than that lgtm.

chinmaygarde · 2023-02-16T21:05:09Z

May I land this?

knopp · 2023-02-16T21:06:06Z

Yes. This is good to go.

)

…120928) * 7cf63a03f [Impeller] use IPSampleDecal in advanced blends. (flutter/engine#39523) * 9da851557 Remove extraneous if (flutter/engine#39683) * 6602fc753 [macOS] Implement platform view mutators (flutter/engine#38699)

This comment was marked as outdated.

Sign in to view

flutter-dashboard bot added platform-macos needs tests labels Jan 7, 2023

knopp force-pushed the macos_platform_view_mutators branch 3 times, most recently from 8fdc4ad to 8381eb6 Compare January 7, 2023 22:06

knopp changed the title ~~WIP: [macOS] Implement platform view mutators~~ [macOS] Implement platform view mutators Jan 8, 2023

knopp removed the needs tests label Jan 8, 2023

knopp requested a review from cbracken January 8, 2023 19:50

knopp force-pushed the macos_platform_view_mutators branch from 5ea3e49 to 8fd2e80 Compare January 9, 2023 12:21

cyanglaz reviewed Jan 13, 2023

View reviewed changes

knopp force-pushed the macos_platform_view_mutators branch 2 times, most recently from 0769368 to 92872c4 Compare February 9, 2023 12:10

loic-sharma added the affects: desktop label Feb 9, 2023

cbracken approved these changes Feb 9, 2023

View reviewed changes

knopp added 3 commits February 16, 2023 16:30

[macOS] Implement platform view mutators

7b17980

Add FlutterMutatorViewTest

a44dcd5

Move private declaration to test

7470f77

knopp force-pushed the macos_platform_view_mutators branch from 92872c4 to 7470f77 Compare February 16, 2023 15:35

chinmaygarde added the autosubmit Merge PR when tree becomes green via auto submit App label Feb 16, 2023

auto-submit bot merged commit 6602fc7 into flutter:main Feb 16, 2023

engine-flutter-autoroll mentioned this pull request Feb 16, 2023

Roll Flutter Engine from bddfc1c4dcaa to 6602fc753525 (3 revisions) flutter/flutter#120928

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Feb 16, 2023

6602fc753 [macOS] Implement platform view mutators (flutter/engine#38699

b447a2c

)


		@interface FlutterMutatorView (Private)

		@property(readonly, nonatomic, nonnull) NSMutableArray<NSView> pathClipViews;


		NSView* lastView = self;

		for (size_t i = 0; i < paths.count; ++i) {

[macOS] Implement platform view mutators #38699

[macOS] Implement platform view mutators #38699

Uh oh!

Conversation

knopp commented Jan 7, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Notes

Pre-launch Checklist

Uh oh!

This comment was marked as outdated.

knopp commented Jan 7, 2023

Uh oh!

chinmaygarde commented Jan 12, 2023

Uh oh!

cyanglaz left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cbracken commented Jan 13, 2023

Uh oh!

zanderso commented Feb 2, 2023

Uh oh!

cbracken left a comment

Choose a reason for hiding this comment

Uh oh!

chinmaygarde commented Feb 16, 2023

Uh oh!

knopp commented Feb 16, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

knopp commented Jan 7, 2023 •

edited

Loading