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

Allow FlutterViewController to specify whether its FlutterView is opaque #6570

Merged
merged 9 commits into from
Oct 23, 2018

Conversation

dnfield
Copy link
Contributor

@dnfield dnfield commented Oct 17, 2018

Fixes flutter/flutter#21808

We set CAEAGLLayer.opaque to YES today, which follows Apple's recommendations for performance. We don't do this when we're just using a CALayer for software rendering. This means that an Add2App consumer will see their view as transparent on the simulator but opaque on a real device, and they have no way to change that behavior on device (or on the Simulator for that matter).

This patch makes the behavior consistent on simulator/device (which is technically breaking change for simulator builds), and allows Add2App consumers to specify whether the view should be opaque or not as a property on FlutterViewController.

Two questions:

1. Would be safe to cache the result of [FlutterView flutterViewController]? It now will potentially be called more frequently and is called for each call to drawLayer - which could potentially save us a few isKindOfClass checks. Removing this method altogether and replacing with a delegate
2. We have this functionality on the Android side, but are we going to enable developers to shoot themselves in the foot by degrading performance for this? Yes, this isn't free, but it has some important use cases (such as showing a FlutterViewController over another screen, and avoiding a flicker of content when loading a FlutterViewController).

/cc @cbracken @matthew-carroll @amirh (FYI)

CGFloat screenScale = [UIScreen mainScreen].scale;
layer.contentsScale = screenScale;
layer.rasterizationScale = screenScale;
}
self.layer.opaque = [self flutterViewController].viewIsOpaque;
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a desire to be able to dynamically set a FlutterView from opaque to transparent (or vice-versa)? If not, then couldn't you avoid [self flutterViewController] by having the FlutterViewController initialize FlutterView with the desired opaqueness?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's a good point. the way it's implemented right now, changing this wouldn't dynamically change the FlutterView until a relayout happened (e.g. changing screen orientation or resizing the view).

I think if we went the route you're suggesting, I'd rather have this be an initializer parameter on FlutterViewController as well though, as the property form of it would become even more confusing in that context. That said, I think having it as an initializer parameter might make more sense anyway.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Trying to figure out if/how any of this impacts storyboard usage.

Copy link
Contributor Author

@dnfield dnfield Oct 17, 2018

Choose a reason for hiding this comment

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

Another option (which might allow for more natural iOS programming) is to have an initializer parameter that says whether the FlutterView automatically sets this at all. I'm not really sure why we'd need to set this repeatedly on layoutSubviews.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm now passing the FlutterViewController as a WeakPtr into the FlutterView initializer. I've also wired the property so we can do this dynamically, as @matthew-carroll pointed out we might want to do this when initially loading the view to avoid a flash of content.

* Controls whether the created view will be opaque or not.
*
* Default is `YES`. Note that setting this to `NO` may negatively impact performance
* when using hardware accelleration, and toggling this will trigger a re-layout of the
Copy link
Contributor

Choose a reason for hiding this comment

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

"accelleration" => "acceleration"

id<FlutterScreenshotDelegate> _delegate;

- (instancetype)init {
@throw([NSException exceptionWithName:@"FlutterView must initWithShell" reason:nil userInfo:nil]);
Copy link
Contributor

Choose a reason for hiding this comment

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

initWithDelegate (and elsewhere)

#include "flutter/shell/platform/darwin/ios/ios_surface.h"

@protocol FlutterScreenshotDelegate <NSObject>
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you plan to use this elsewhere? If not, it's clear to me what advantage this provides over passing the FlutterViewController down directly like you did in the previous version.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, but it feels pretty wrong to pass a view controller directly into a view. I was looking at passing in just the shell, and after discussion with @chinmaygarde decided to go with a delegate - this way we avoid trying to share a pointer to the shell, and we avoid tying the ViewController implementation details to the View.

Copy link
Contributor

@jamesderlin jamesderlin left a comment

Choose a reason for hiding this comment

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

LGTM, but you probably should get a review from Chinmay too.

* when using hardware acceleration, and toggling this will trigger a re-layout of the
* view.
*/
@property(nonatomic, getter=isViewOpaque, setter=isViewOpaque:) BOOL isViewOpaque;
Copy link
Member

Choose a reason for hiding this comment

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

Nit: @property(nonatomic, getter=isViewOpaque) BOOL viewOpaque; with the setter being inferred as setIsOpaque is more idiomatic. See -[UIView opaque] for another such use.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@dnfield dnfield merged commit 427915e into flutter:master Oct 23, 2018
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Oct 23, 2018
flutter/engine@4c79e42...2586e94

git log 4c79e42..2586e94 --no-merges --oneline
2586e94 Support all combinations of GetRectsForRange styles (flutter/engine#6591)
e78f86e Fix mac builds. Only Linux and Windows require default GL proc resolvers. (flutter/engine#6641)
52e48ab Fix Windows embedding. Appears that flutter#6523 or flutter#6525 introduced a bug for embedder scenarios causing the window native library to be incorrectly initialized and thus incapable of correctly resolving GL functions.  This change fixes that. (flutter/engine#6624)
c9197e4 Roll src/third_party/skia 25837bf17019..b46c4d0925ad (6 commits) (flutter/engine#6640)
324c21a Roll src/third_party/skia 1b07dffd979d..25837bf17019 (1 commits) (flutter/engine#6639)
5dfbc0a Roll src/third_party/skia e023ae7c5540..1b07dffd979d (1 commits) (flutter/engine#6638)
4a18dff Roll src/third_party/skia ff1aeb953faf..e023ae7c5540 (1 commits) (flutter/engine#6637)
427915e Allow FlutterViewController to specify whether its FlutterView is opaque (flutter/engine#6570)
20c805c Ensure that Scene::toImage renders texture backed images. (flutter/engine#6636)
25d0507 Roll buildtools to 5a9e1b3a0b84a2871f20f85fde665e54a894ba72 (flutter/engine#6633)
4f17d7e Roll src/third_party/skia 327955b1ba19..ff1aeb953faf (13 commits) (flutter/engine#6635)
cdd592f Reland &#39;Pass null instead of &#39;none&#39; locale&#39; (flutter/engine#6632)
2cd8920 Roll src/third_party/skia b1a002e850e1..327955b1ba19 (12 commits) (flutter/engine#6631)
edfe024 13771 - iOS dictation bug (flutter/engine#6607)
cadf440 Roll src/third_party/skia b9998cdceec7..b1a002e850e1 (13 commits) (flutter/engine#6626)

The AutoRoll server is located here: https://autoroll.skia.org/r/flutter-engine-flutter-autoroll

Documentation for the AutoRoller is here:
https://skia.googlesource.com/buildbot/&#43;/master/autoroll/README.md

If the roll is causing failures, please contact the current sheriff, who should
be CC&#39;d on the roll, and stop the roller if necessary.
goderbauer pushed a commit to flutter/flutter that referenced this pull request Oct 23, 2018
flutter/engine@4c79e42...2586e94

git log 4c79e42..2586e94 --no-merges --oneline
2586e94 Support all combinations of GetRectsForRange styles (flutter/engine#6591)
e78f86e Fix mac builds. Only Linux and Windows require default GL proc resolvers. (flutter/engine#6641)
52e48ab Fix Windows embedding. Appears that #6523 or #6525 introduced a bug for embedder scenarios causing the window native library to be incorrectly initialized and thus incapable of correctly resolving GL functions.  This change fixes that. (flutter/engine#6624)
c9197e4 Roll src/third_party/skia 25837bf17019..b46c4d0925ad (6 commits) (flutter/engine#6640)
324c21a Roll src/third_party/skia 1b07dffd979d..25837bf17019 (1 commits) (flutter/engine#6639)
5dfbc0a Roll src/third_party/skia e023ae7c5540..1b07dffd979d (1 commits) (flutter/engine#6638)
4a18dff Roll src/third_party/skia ff1aeb953faf..e023ae7c5540 (1 commits) (flutter/engine#6637)
427915e Allow FlutterViewController to specify whether its FlutterView is opaque (flutter/engine#6570)
20c805c Ensure that Scene::toImage renders texture backed images. (flutter/engine#6636)
25d0507 Roll buildtools to 5a9e1b3a0b84a2871f20f85fde665e54a894ba72 (flutter/engine#6633)
4f17d7e Roll src/third_party/skia 327955b1ba19..ff1aeb953faf (13 commits) (flutter/engine#6635)
cdd592f Reland &#39;Pass null instead of &#39;none&#39; locale&#39; (flutter/engine#6632)
2cd8920 Roll src/third_party/skia b1a002e850e1..327955b1ba19 (12 commits) (flutter/engine#6631)
edfe024 13771 - iOS dictation bug (flutter/engine#6607)
cadf440 Roll src/third_party/skia b9998cdceec7..b1a002e850e1 (13 commits) (flutter/engine#6626)

The AutoRoll server is located here: https://autoroll.skia.org/r/flutter-engine-flutter-autoroll

Documentation for the AutoRoller is here:
https://skia.googlesource.com/buildbot/&#43;/master/autoroll/README.md

If the roll is causing failures, please contact the current sheriff, who should
be CC&#39;d on the roll, and stop the roller if necessary.
Xavjer pushed a commit to Xavjer/flutter that referenced this pull request Oct 24, 2018
flutter/engine@4c79e42...2586e94

git log 4c79e42..2586e94 --no-merges --oneline
2586e94 Support all combinations of GetRectsForRange styles (flutter/engine#6591)
e78f86e Fix mac builds. Only Linux and Windows require default GL proc resolvers. (flutter/engine#6641)
52e48ab Fix Windows embedding. Appears that flutter#6523 or flutter#6525 introduced a bug for embedder scenarios causing the window native library to be incorrectly initialized and thus incapable of correctly resolving GL functions.  This change fixes that. (flutter/engine#6624)
c9197e4 Roll src/third_party/skia 25837bf17019..b46c4d0925ad (6 commits) (flutter/engine#6640)
324c21a Roll src/third_party/skia 1b07dffd979d..25837bf17019 (1 commits) (flutter/engine#6639)
5dfbc0a Roll src/third_party/skia e023ae7c5540..1b07dffd979d (1 commits) (flutter/engine#6638)
4a18dff Roll src/third_party/skia ff1aeb953faf..e023ae7c5540 (1 commits) (flutter/engine#6637)
427915e Allow FlutterViewController to specify whether its FlutterView is opaque (flutter/engine#6570)
20c805c Ensure that Scene::toImage renders texture backed images. (flutter/engine#6636)
25d0507 Roll buildtools to 5a9e1b3a0b84a2871f20f85fde665e54a894ba72 (flutter/engine#6633)
4f17d7e Roll src/third_party/skia 327955b1ba19..ff1aeb953faf (13 commits) (flutter/engine#6635)
cdd592f Reland &flutter#39;Pass null instead of &flutter#39;none&flutter#39; locale&flutter#39; (flutter/engine#6632)
2cd8920 Roll src/third_party/skia b1a002e850e1..327955b1ba19 (12 commits) (flutter/engine#6631)
edfe024 13771 - iOS dictation bug (flutter/engine#6607)
cadf440 Roll src/third_party/skia b9998cdceec7..b1a002e850e1 (13 commits) (flutter/engine#6626)

The AutoRoll server is located here: https://autoroll.skia.org/r/flutter-engine-flutter-autoroll

Documentation for the AutoRoller is here:
https://skia.googlesource.com/buildbot/&#43;/master/autoroll/README.md

If the roll is causing failures, please contact the current sheriff, who should
be CC&flutter#39;d on the roll, and stop the roller if necessary.
@lenaschimmel
Copy link

Thanks for your patch / PR, @dnfield! I thought this would be the solution to use native map views on iOS in my flutter app.

I just tried it, and it did not work exactly as I hoped. Not sure if this is a bug of if I misunderstood how it should be used.

In the simplest test case without a map, I used the default Runner app and modified it. I made a custom ViewController class, extended FlutterViewController and in my init methods I do

    self.isViewOpaque = false;
    self.view.backgroundColor = UIColor.red;

after calling the super.init() method. When I run it on a device, I can see the native red background behind my Flutter widgets. But flutter redraws the widgets, the old rendering is still there and will be overdrawn. I can see this when a widget moves, but also the "debug" badge in the upper right corner will be drawn upon its previous rendering, making the drop shadow darker and darker each time.

I guess the FlutterView would need to somehow clear its surface before drawing once again, and I think this should be the default behavior when viewIsOpaque is false. (Clearing would only be redundant if the FlutterView has binary transperency, i.e. no semi-transparent pixels, and no opaque pixel ever becomes transparent. I think this is an edge case, not a sensible default.)

The same happens if I actually put a map view underneath the flutter view (I tried Map Kit and MapBox). The map is animated properly and I can see it through the FlutterView, but Flutter Widgets leave trails behind when they move.

Is this understandable, or do you need a screen shot?

@amirh
Copy link
Contributor

amirh commented Nov 7, 2018 via email

@lenaschimmel
Copy link

Thanks for the very quick answer, @amirh, it was indeed fixed in #6753.

It took me some time to test it, because the change is not yet in the dev channel and I didn't know that I can also do flutter channel master. It works great!

@dnfield dnfield deleted the opaque_view branch November 14, 2018 19:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants