Skip to content
This repository was archived by the owner on Feb 25, 2025. It is now read-only.

Conversation

@WasserEsser
Copy link
Contributor

@WasserEsser WasserEsser commented Dec 10, 2021

SceneBuilder opacity layers are not currently supported for Android platform-views using hybrid-composition.
This PR implements it.

Note that even though #58426 mentions opacity being implemented, it does not seem to be the case.

Fixes #93757
Advances #58426

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.

Copy link
Contributor

@cyanglaz cyanglaz left a comment

Choose a reason for hiding this comment

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

LGTM on the mutator stack code.
I'm not confident enough to review the code related to android paint context.

CC @blasten as the second reviewer.

Comment on lines 148 to 150
Paint transparency = new Paint();
transparency.setAlpha((int) (mutatorsStack.getFinalOpacity() * 255));
this.setLayerType(LAYER_TYPE_HARDWARE, transparency);
Copy link
Contributor

Choose a reason for hiding this comment

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

CC @blasten for confirming this is OK

Copy link

Choose a reason for hiding this comment

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

does it need to create a new Paint object on every draw call? does this work?

In the constructor:

paint = new Paint();
setLayerType(LAYER_TYPE_HARDWARE, paint);

onDraw:

paint.setAlpha((int) (mutatorsStack.getFinalOpacity() * 255));

Copy link

Choose a reason for hiding this comment

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

seems like this should work from looking at the android code, but please give it a try and confirm

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@blasten You're right, we don't need to create the paint object on every draw call.
We do however need to set the layer type every time, moving it into the constructor does not seem to be enough.

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 have updated the code regarding this

@flutter-dashboard
Copy link

This pull request executed golden file tests, but it has not been updated in a while (20+ days). Test results from Gold expire after as many days, so this pull request will need to be updated with a fresh commit in order to get results from Gold.

@WasserEsser
Copy link
Contributor Author

@cyanglaz @cbracken

I rebased it so the golden tests are up to date.

@cyanglaz cyanglaz requested a review from blasten January 13, 2022 18:03
@cyanglaz
Copy link
Contributor

@WasserEsser Which golden test are you talking about? Since the opacity is supported in this change, I suppose there should be an updated golden image in the scenario test (https://github.com/flutter/engine/tree/main/testing/scenario_app) in this PR. I didn't see an update on the golden image.

@cyanglaz
Copy link
Contributor

adding @blasten for a 2nd review.

@WasserEsser
Copy link
Contributor Author

@cyanglaz I was just referring to the comment the flutter bot made about the tests expiring as the PR is somewhat older.

@cyanglaz
Copy link
Contributor

@WasserEsser Ok, so it is unrelated to what I said about the scenario tests then. Are you familiar with the scenario tests? There should be a README to help you set it up.

@WasserEsser
Copy link
Contributor Author

@cyanglaz Yep, although I couldn't get them to run the last time I've tried. I'll try to create one later tonight.

@chinmaygarde
Copy link
Member

@WasserEsser Any update on the test?

Copy link

@blasten blasten left a comment

Choose a reason for hiding this comment

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

LGTM

@blasten
Copy link

blasten commented Jan 20, 2022

The scenario app tests on Android are currently broken unfortunately. flutter/flutter#90401.

There's an opacity test already, but it won't fail because this test was disabled. https://github.com/flutter/engine/blob/main/testing/scenario_app/android/reports/screenshots/dev.flutter.scenariosui.PlatformViewUiTests__testPlatformViewOpacity.png

@WasserEsser
Copy link
Contributor Author

@blasten Ah that's why I couldn't get them to work.

@WasserEsser WasserEsser requested a review from cyanglaz January 20, 2022 22:18
@blasten blasten added the waiting for tree to go green This PR is approved and tested, but waiting for the tree to be green to land. label Jan 20, 2022
@fluttergithubbot
Copy link
Contributor

This pull request is not suitable for automatic merging in its current state.

  • Please get at least one approved review if you are already a member or two member reviews if you are not a member before re-applying this label. Reviewers: If you left a comment approving, please use the "approve" review action instead.

@fluttergithubbot fluttergithubbot removed the waiting for tree to go green This PR is approved and tested, but waiting for the tree to be green to land. label Jan 20, 2022
@cyanglaz cyanglaz added the waiting for tree to go green This PR is approved and tested, but waiting for the tree to be green to land. label Jan 20, 2022
@fluttergithubbot fluttergithubbot merged commit 730b469 into flutter:main Jan 20, 2022
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jan 21, 2022
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jan 21, 2022
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jan 21, 2022
zanderso added a commit that referenced this pull request Jan 23, 2022
zanderso added a commit that referenced this pull request Jan 24, 2022
…ition" (#31002)

* Revert "Support opacity layers for platform-views using hybrid-composition (#30264)"

This reverts commit 730b469.

* Empty
itsjustkevin pushed a commit that referenced this pull request Jan 28, 2022
…ition" (#31002)

* Revert "Support opacity layers for platform-views using hybrid-composition (#30264)"

This reverts commit 730b469.

* Empty
itsjustkevin added a commit that referenced this pull request Jan 28, 2022
* Revert "Support opacity layers for platform-views using hybrid-composition" (#31002)

* Revert "Support opacity layers for platform-views using hybrid-composition (#30264)"

This reverts commit 730b469.

* Empty

* Revert "[fuchsia] Switch from core-jit to core snapshots. (#30744)" (#31065)

This reverts commit a193f08.

Co-authored-by: Zachary Anderson <[email protected]>
Co-authored-by: David Worsham <[email protected]>
renyou pushed a commit that referenced this pull request Jan 28, 2022
* Revert "Support opacity layers for platform-views using hybrid-composition" (#31002)

* Revert "Support opacity layers for platform-views using hybrid-composition (#30264)"

This reverts commit 730b469.

* Empty

* Revert "[fuchsia] Switch from core-jit to core snapshots. (#30744)" (#31065)

This reverts commit a193f08.

Co-authored-by: Zachary Anderson <[email protected]>
Co-authored-by: David Worsham <[email protected]>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

platform-android waiting for tree to go green This PR is approved and tested, but waiting for the tree to be green to land.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[android] FadeTransitions do not work with platform views using hybrid composition

5 participants