-
Notifications
You must be signed in to change notification settings - Fork 3.2k
[MaterialContainerTransform] Add copyViewImage to decrease cost #4885
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
base: master
Are you sure you want to change the base?
Conversation
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
@hunterstich can you take a look? also, if we want to make this change I think we'll need to keep the default behavior the same (live view rendering during the transition) and add a flag / option for this new bitmap version. |
@@ -935,11 +936,11 @@ public Animator createAnimator( | |||
final TransitionDrawable transitionDrawable = | |||
new TransitionDrawable( | |||
getPathMotion(), | |||
startView, | |||
copyViewImage(sceneRoot, startView, startView.getParent() instanceof View ? (View) startView.getParent() : startView), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this should become startView.getParent() instanceof View ? copyViewImage(sceneRoot, startView, (View) startView.getParent()) : startView
Maybe better to extract this to a function.
private static View maybeGetViewImageCopy(ViewGroup sceneRoot, View view) {
if (view.getParent() instanceof View) {
return TransitionUtils.copyViewImage(sceneRoot, view, (View) view.getParent());
}
return view;
}
public static View copyViewImage(ViewGroup sceneRoot, View view, View parent) { | ||
Matrix matrix = new Matrix(); | ||
matrix.setTranslate(-parent.getScrollX(), -parent.getScrollY()); | ||
view.transformMatrixToGlobal(matrix); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Method not available on lower APIs. Should become ViewCompat.transformMatrixToGlobal(view, matrix);
.
Matrix matrix = new Matrix(); | ||
matrix.setTranslate(-parent.getScrollX(), -parent.getScrollY()); | ||
view.transformMatrixToGlobal(matrix); | ||
sceneRoot.transformMatrixToLocal(matrix); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also not available on lower APIs. I suggest adding transformMatrixToLocal to ViewUtils, similar to the Transition package.
canvas.concat(matrix); | ||
view.draw(canvas); | ||
picture.endRecording(); | ||
bitmap = Bitmap.createBitmap(picture); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
createBitmap is not available on lower APIs. The Transition package contains an example on how to fix this.
Thanks for starting a pull request on Material Components!
Don't forget:
[Buttons] Updated documentation
closes #1234
Contributing
has more information and tips for a great pull request.
In developing, I found that the MaterialContainerTransform costs more than MaterialSharedAxis, which is has set low-bitmap enough to decrease cost.
Digging deeper, the MaterialSharedAxis is extended from androidx.transition, which has set copyViewToImage to set low-bitmap.
So, I added it into TransitionUtils and MaterialContainerTransform to increase performance.
After

Before
