-
Notifications
You must be signed in to change notification settings - Fork 77
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
Shared transitions #1550
base: main
Are you sure you want to change the base?
Shared transitions #1550
Conversation
Some high level early thoughts
|
Do you have any sense of how much work is involved? I'd like to see a bit more progress before landing this personally, as I think there's going to be tests which are broken. If only just getting the sample looking right to prove it out fully. |
layoutDirection: LayoutDirection, | ||
density: Density, | ||
): Path { | ||
path.reset() |
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.
Use rewind()
it's cheaper for the next outline
*/ | ||
@InternalCircuitApi | ||
@OptIn(ExperimentalSharedTransitionApi::class) | ||
public data object NoOpSharedTransitionScope : SharedTransitionScope { |
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.
Curious on the need for this, vs just not adding the modifiers / layout when not required. Hard to really understand all the details of Circuit but maybe you've explored that option already?
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.
Was initially experimenting with this from the view that a Screen
can be reused regardless of the context. Thinking on it some more its not really needed, a Screen
really shouldn't be getting reused as such.
name = state.name, | ||
photoUrlMemoryCacheKey = state.photoUrlMemoryCacheKey, | ||
modifier = | ||
Modifier.sharedElementWithCallerManagedVisibility( |
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.
Is there no way to expose an AnimatedVisibilityScope
from Navigation framework here, so that you could use the standard shared element modifiers?
I think from a usability perspective for developers, this would be easier to work with than needing to use sharedElementWithCallerManagedVisibility
.
I also think you'd need to use it to pass through the correct Transition object, if looking at supporting predictive back correctly.
Take a look at how its used here in Jetsnack for animating rounded corners as the animation progresses - https://github.com/android/compose-samples/pull/1314/files#diff-cd670f26d99bb64790bdbee3a8965fca378714cb988996912f2e0e137931b72dR156-R163
For a simple example of Shared elements with SeekableTransitionState
, no NavHost usage, we have this snippet that might be helpful to tie your transitions into - android/snippets#273
You may want to consider using context receivers in Kotlin if your library would use an experimental feature (Kotlin/KEEP#259), you could then expose both the AnimatedVisibilityScope and SharedTransitionScope to one function without needing to pass it around.
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.
This case is a bit different as it's dealing with an element being shared across navigation and again as an Overlay is being shown. Need to really work on the Overlay API for this a bit more, currently missing a AnimatedVisibilityScope
from it 😅 Think the goal would be to dynamically switch to the correct AnimatedVisibilityScope
based on whats happening.
Thanks for the SeekableTransitionState
snippet 🙇🏻 The predictive back implementation/api is definitely going to need to provide some of the progress information.
* [SharedElementTransitionScope]. | ||
*/ | ||
@Composable | ||
private fun staticAnimatedVisibilityScope(): AnimatedVisibilityScope { |
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.
This feels like it could potentially introduce some UI inconsistencies, is it possible that a developer somehow relies on the scope to perform a different UI animations unrelated to shared elements? Maybe it'd be less error-prone to rather have a nullable scope instead.
Or have the Nav framework itself always expose the AnimatedVisibilityScope and throw an exception if its not there as something would've gone quite wrong to have that happen?
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 second what Rebecca said. This custom scope will always have the target visibility be EnterExitState.Visible
. That would potentially prevent the shared elements from identifying the target bounds. More specifically when both sharedElement/sharedBounds call sites of the same key report their target visibility to be visible, we would not know which bounds to use as the target bounds since they are both "entering".
Also, consumers of this API may be confused as to why their Modiifer.animateEnterExit
doesn't work. A nullable scope in this case would be more explicit.
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.
Yup, it's pretty broken always being in the single state right now. Going to lean on the nav for providing this.
Love the visuals to aid the PR description! Thanks for showcasing them 🎉 The animations are looking great too - love the back from full screen to the pager. Could the Active Scope (layout.mp4) example at 0:07 be improved to have the image shared to that screen too? It seems like there is a blank screen at that point. Maybe its an image loading issue, perhaps it could be corrected by setting a placeholder cache key to the same as the memory cache key? From details -> list of dogs, the final clipping is applied immediately to the dog, consider animating the rounded corners too as it progresses between the states, the bottom corners can animation from rounded to straight - something similar was done here - https://youtu.be/PR6rz1QUkAM?si=DsGg4huAxgbel9c_&t=989 I do agree about getting predictive back working correctly too - as this may heavily influence your API design more, as developers would probably want to tie into the Transition object to do other surrounding animations, as well as it can help with debugging to be able to seek between the states. |
We're going to need some form of
Think most of the work is in supporting seekable transitions for predictive back, then maybe building some simpler api's on top of that? |
Sounds good to me! I just don't see any tests being ran on CI? |
This comment was marked as outdated.
This comment was marked as outdated.
Looking better. Def game to make whatever changes we need to nav decoration APIs (and overlays) |
918ca4e
to
cb59638
Compare
Some early thoughts from going around the sample before looking through the code
starsampleone.webm
starsample2.webm
starsample3.webm |
Not sure I've run into this?
Changed it a bit with 0d1b246
Nice idea, added with e972508
Fixed with 2779634, the drawbacks of dark mode 😅
Changed the animation timings for both of these. Jumps directly to the first image which fades in, but now won't do an odd cross fade.
Yaaa this has been the main problem. We don't know if there are shared transitions in time to disable the animated content transition. I like the idea of the |
Shared Element API
SharedElementTransitionLayout
andSharedElementTransitionScope
SharedTransitionScope
to child elements (ie Screen) without having to pipe the scope all the way down into Circuit Ui'sAnimatedVisibilityScope
via a newAnimatedNavDecoration
variant ofNavDecoration
AnimatedVisibilityScope
SharedElementTransitionScope
SharedTransitionScope
for the standard shared elements/shared bounds animationsSharedElementTransitionScope
instance can then be access via the composableSharedElementTransitionScope
SharedElementTransitionScope
composable should be used likeSharedTransitionScope
to access thesharedElement
andsharedBound
modifiersgetAnimatedScope
orrequireAnimatedScope
can be used to access a neededAnimatedVisibilityScope
AnimatedVisibilityScope
s can be provided with theProvideAnimatedTransitionScope
composableSharedElementTransitionScope
is availableSharedElementTransitionLayout
SharedElementTransitionLayout
is a layout that creates and provides aSharedElementTransitionScope
SharedElementTransitionLayout
should be setup around the a standard circuit setup, betweenCircuitCompositionLocals
and beforeNavigableCircuitContent
orContentWithOverlays
PreviewSharedElementTransitionLayout
is also provided to help with@Preview
useUpdate star to use SharedElementTransitions
none.mp4
navigation.mp4
overlay.mp4
predictive-back.mp4