Skip to content

Commit

Permalink
Merge changes Ic8bd91b3,I371c8b93,Id9ba18e7 into main
Browse files Browse the repository at this point in the history 
* changes:
  Prevent NPE when placing fading elements
  Move TransitionKey to Transition
  Fix interruption + overscroll bug
  • Loading branch information
Jordan Demeulenaere authored and Android (Google) Code Review committed Jun 10, 2024
2 parents 35753dd + e3c540a commit 6f5a4cd
Show file tree
Hide file tree
Showing 9 changed files with 260 additions and 88 deletions.
8 changes: 5 additions & 3 deletions packages/SystemUI/compose/scene/src/com/android/compose/animation/scene/AnimateToScene.kt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -109,8 +109,7 @@ internal fun CoroutineScope.animateToScene(
layoutState.transitions.interruptionHandler.onInterruption(
transitionState,
target,
)
?: DefaultInterruptionHandler.onInterruption(transitionState, target)
) ?: DefaultInterruptionHandler.onInterruption(transitionState, target)

val animateFrom = interruptionResult.animateFrom
if (
Expand Down Expand Up @@ -159,6 +158,7 @@ private fun CoroutineScope.animate(
val transition =
if (reversed) {
OneOffTransition(
key = transitionKey,
fromScene = targetScene,
toScene = fromScene,
currentScene = targetScene,
Expand All @@ -167,6 +167,7 @@ private fun CoroutineScope.animate(
)
} else {
OneOffTransition(
key = transitionKey,
fromScene = fromScene,
toScene = targetScene,
currentScene = targetScene,
Expand All @@ -178,7 +179,7 @@ private fun CoroutineScope.animate(
// Change the current layout state to start this new transition. This will compute the
// TransformationSpec associated to this transition, which we need to initialize the Animatable
// that will actually animate it.
layoutState.startTransition(transition, transitionKey, chain)
layoutState.startTransition(transition, chain)

// The transition now contains the transformation spec that we should use to instantiate the
// Animatable.
Expand Down Expand Up @@ -207,6 +208,7 @@ private fun CoroutineScope.animate(
}

private class OneOffTransition(
override val key: TransitionKey?,
fromScene: SceneKey,
toScene: SceneKey,
override val currentScene: SceneKey,
Expand Down
4 changes: 2 additions & 2 deletions packages/SystemUI/compose/scene/src/com/android/compose/animation/scene/DraggableHandler.kt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -257,7 +257,7 @@ private class DragControllerImpl(

fun updateTransition(newTransition: SwipeTransition, force: Boolean = false) {
if (isDrivingTransition || force) {
layoutState.startTransition(newTransition, newTransition.key)
layoutState.startTransition(newTransition)
}

swipeTransition = newTransition
Expand Down Expand Up @@ -555,7 +555,7 @@ private class SwipeTransition(
val layoutImpl: SceneTransitionLayoutImpl,
val layoutState: BaseSceneTransitionLayoutState,
val coroutineScope: CoroutineScope,
val key: TransitionKey?,
override val key: TransitionKey?,
val _fromScene: Scene,
val _toScene: Scene,
val userActionDistanceScope: UserActionDistanceScope,
Expand Down
135 changes: 109 additions & 26 deletions packages/SystemUI/compose/scene/src/com/android/compose/animation/scene/Element.kt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -280,7 +280,7 @@ internal class ElementNode(
constraints: Constraints,
): MeasureResult {
val transitions = currentTransitions
val transition = elementTransition(element, transitions)
val transition = elementTransition(layoutImpl, element, transitions)

// If this element is not supposed to be laid out now, either because it is not part of any
// ongoing transition or the other scene of its transition is overscrolling, then lay out
Expand Down Expand Up @@ -318,13 +318,10 @@ internal class ElementNode(
val targetOffsetInScene = lookaheadScopeCoordinates.localLookaheadPositionOf(coords)

// No need to place the element in this scene if we don't want to draw it anyways.
if (!shouldPlaceElement(layoutImpl, scene, element, transition)) {
if (!shouldPlaceElement(layoutImpl, scene.key, element, transition)) {
sceneState.lastOffset = Offset.Unspecified
sceneState.lastScale = Scale.Unspecified
sceneState.lastAlpha = Element.AlphaUnspecified

sceneState.clearValuesBeforeInterruption()
sceneState.clearInterruptionDeltas()
return
}

Expand Down Expand Up @@ -353,7 +350,17 @@ internal class ElementNode(
getValueBeforeInterruption = { sceneState.offsetBeforeInterruption },
setValueBeforeInterruption = { sceneState.offsetBeforeInterruption = it },
getInterruptionDelta = { sceneState.offsetInterruptionDelta },
setInterruptionDelta = { sceneState.offsetInterruptionDelta = it },
setInterruptionDelta = { delta ->
setPlacementInterruptionDelta(
element = element,
sceneState = sceneState,
transition = transition,
delta = delta,
setter = { sceneState, delta ->
sceneState.offsetInterruptionDelta = delta
},
)
},
diff = { a, b -> a - b },
add = { a, b, bProgress -> a + b * bProgress },
)
Expand All @@ -363,7 +370,7 @@ internal class ElementNode(
val offset = (interruptedOffset - currentOffset).round()
if (
isElementOpaque(scene, element, transition) &&
interruptedAlpha(layoutImpl, transition, sceneState, alpha = 1f) == 1f
interruptedAlpha(layoutImpl, element, transition, sceneState, alpha = 1f) == 1f
) {
sceneState.lastAlpha = 1f

Expand All @@ -374,13 +381,17 @@ internal class ElementNode(
} else {
placeable.placeWithLayer(offset) {
// This layer might still run on its own (outside of the placement phase) even
// if this element is not placed anymore, so we need to double check again here
// before calling [elementAlpha] (which will update [SceneState.lastAlpha]). We
// also need to recompute the current transition to make sure that we are using
// the current transition and not a reference to an old one. See b/343138966 for
// details.
val transition = elementTransition(element, currentTransitions)
if (!shouldPlaceElement(layoutImpl, scene, element, transition)) {
// if this element is not placed or composed anymore, so we need to double check
// again here before calling [elementAlpha] (which will update
// [SceneState.lastAlpha]). We also need to recompute the current transition to
// make sure that we are using the current transition and not a reference to an
// old one. See b/343138966 for details.
if (_element == null) {
return@placeWithLayer
}

val transition = elementTransition(layoutImpl, element, currentTransitions)
if (!shouldPlaceElement(layoutImpl, scene.key, element, transition)) {
return@placeWithLayer
}

Expand All @@ -394,7 +405,7 @@ internal class ElementNode(
override fun ContentDrawScope.draw() {
element.wasDrawnInAnyScene = true

val transition = elementTransition(element, currentTransitions)
val transition = elementTransition(layoutImpl, element, currentTransitions)
val drawScale = getDrawScale(layoutImpl, scene, element, transition, sceneState)
if (drawScale == Scale.Default) {
drawContent()
Expand Down Expand Up @@ -435,6 +446,7 @@ internal class ElementNode(
* its scenes contains the element.
*/
private fun elementTransition(
layoutImpl: SceneTransitionLayoutImpl,
element: Element,
transitions: List<TransitionState.Transition>,
): TransitionState.Transition? {
Expand All @@ -448,7 +460,7 @@ private fun elementTransition(

if (transition != previousTransition && transition != null && previousTransition != null) {
// The previous transition was interrupted by another transition.
prepareInterruption(element, transition, previousTransition)
prepareInterruption(layoutImpl, element, transition, previousTransition)
} else if (transition == null && previousTransition != null) {
// The transition was just finished.
element.sceneStates.values.forEach {
Expand All @@ -461,18 +473,43 @@ private fun elementTransition(
}

private fun prepareInterruption(
layoutImpl: SceneTransitionLayoutImpl,
element: Element,
transition: TransitionState.Transition,
previousTransition: TransitionState.Transition,
) {
val sceneStates = element.sceneStates
sceneStates[previousTransition.fromScene]?.selfUpdateValuesBeforeInterruption()
sceneStates[previousTransition.toScene]?.selfUpdateValuesBeforeInterruption()
sceneStates[transition.fromScene]?.selfUpdateValuesBeforeInterruption()
sceneStates[transition.toScene]?.selfUpdateValuesBeforeInterruption()
fun updatedSceneState(key: SceneKey): Element.SceneState? {
return sceneStates[key]?.also { it.selfUpdateValuesBeforeInterruption() }
}

val previousFromState = updatedSceneState(previousTransition.fromScene)
val previousToState = updatedSceneState(previousTransition.toScene)
val fromState = updatedSceneState(transition.fromScene)
val toState = updatedSceneState(transition.toScene)

reconcileStates(element, previousTransition)
reconcileStates(element, transition)

// Remove the interruption values to all scenes but the scene(s) where the element will be
// placed, to make sure that interruption deltas are computed only right after this interruption
// is prepared.
fun maybeCleanPlacementValuesBeforeInterruption(sceneState: Element.SceneState) {
if (!shouldPlaceElement(layoutImpl, sceneState.scene, element, transition)) {
sceneState.offsetBeforeInterruption = Offset.Unspecified
sceneState.alphaBeforeInterruption = Element.AlphaUnspecified
sceneState.scaleBeforeInterruption = Scale.Unspecified

sceneState.offsetInterruptionDelta = Offset.Zero
sceneState.alphaInterruptionDelta = 0f
sceneState.scaleInterruptionDelta = Scale.Zero
}
}

previousFromState?.let { maybeCleanPlacementValuesBeforeInterruption(it) }
previousToState?.let { maybeCleanPlacementValuesBeforeInterruption(it) }
fromState?.let { maybeCleanPlacementValuesBeforeInterruption(it) }
toState?.let { maybeCleanPlacementValuesBeforeInterruption(it) }
}

/**
Expand Down Expand Up @@ -579,9 +616,38 @@ private inline fun <T> computeInterruptedValue(
}
}

/**
* Set the interruption delta of a *placement/drawing*-related value (offset, alpha, scale). This
* ensures that the delta is also set on the other scene in the transition for shared elements, so
* that there is no jump cut if the scene where the element is placed has changed.
*/
private inline fun <T> setPlacementInterruptionDelta(
element: Element,
sceneState: Element.SceneState,
transition: TransitionState.Transition?,
delta: T,
setter: (Element.SceneState, T) -> Unit,
) {
// Set the interruption delta on the current scene.
setter(sceneState, delta)

if (transition == null) {
return
}

// If the element is shared, also set the delta on the other scene so that it is used by that
// scene if we start overscrolling it and change the scene where the element is placed.
val otherScene =
if (sceneState.scene == transition.fromScene) transition.toScene else transition.fromScene
val otherSceneState = element.sceneStates[otherScene] ?: return
if (isSharedElementEnabled(element.key, transition)) {
setter(otherSceneState, delta)
}
}

private fun shouldPlaceElement(
layoutImpl: SceneTransitionLayoutImpl,
scene: Scene,
scene: SceneKey,
element: Element,
transition: TransitionState.Transition?,
): Boolean {
Expand All @@ -592,7 +658,7 @@ private fun shouldPlaceElement(

// Don't place the element in this scene if this scene is not part of the current element
// transition.
if (scene.key != transition.fromScene && scene.key != transition.toScene) {
if (scene != transition.fromScene && scene != transition.toScene) {
return false
}

Expand All @@ -610,7 +676,7 @@ private fun shouldPlaceElement(

return shouldPlaceOrComposeSharedElement(
layoutImpl,
scene.key,
scene,
element.key,
transition,
)
Expand Down Expand Up @@ -740,13 +806,14 @@ private fun elementAlpha(
element.sceneStates.forEach { it.value.alphaBeforeInterruption = 0f }
}

val interruptedAlpha = interruptedAlpha(layoutImpl, transition, sceneState, alpha)
val interruptedAlpha = interruptedAlpha(layoutImpl, element, transition, sceneState, alpha)
sceneState.lastAlpha = interruptedAlpha
return interruptedAlpha
}

private fun interruptedAlpha(
layoutImpl: SceneTransitionLayoutImpl,
element: Element,
transition: TransitionState.Transition?,
sceneState: Element.SceneState,
alpha: Float,
Expand All @@ -760,7 +827,15 @@ private fun interruptedAlpha(
getValueBeforeInterruption = { sceneState.alphaBeforeInterruption },
setValueBeforeInterruption = { sceneState.alphaBeforeInterruption = it },
getInterruptionDelta = { sceneState.alphaInterruptionDelta },
setInterruptionDelta = { sceneState.alphaInterruptionDelta = it },
setInterruptionDelta = { delta ->
setPlacementInterruptionDelta(
element = element,
sceneState = sceneState,
transition = transition,
delta = delta,
setter = { sceneState, delta -> sceneState.alphaInterruptionDelta = delta },
)
},
diff = { a, b -> a - b },
add = { a, b, bProgress -> a + b * bProgress },
)
Expand Down Expand Up @@ -867,7 +942,15 @@ private fun ContentDrawScope.getDrawScale(
getValueBeforeInterruption = { sceneState.scaleBeforeInterruption },
setValueBeforeInterruption = { sceneState.scaleBeforeInterruption = it },
getInterruptionDelta = { sceneState.scaleInterruptionDelta },
setInterruptionDelta = { sceneState.scaleInterruptionDelta = it },
setInterruptionDelta = { delta ->
setPlacementInterruptionDelta(
element = element,
sceneState = sceneState,
transition = transition,
delta = delta,
setter = { sceneState, delta -> sceneState.scaleInterruptionDelta = delta },
)
},
diff = { a, b ->
Scale(
scaleX = a.scaleX - b.scaleX,
Expand Down
19 changes: 12 additions & 7 deletions ...temUI/compose/scene/src/com/android/compose/animation/scene/SceneTransitionLayoutState.kt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -225,6 +225,12 @@ sealed interface TransitionState {
/** The scene this transition is going to. Can't be the same as fromScene */
val toScene: SceneKey,
) : TransitionState {
/**
* The key of this transition. This should usually be null, but it can be specified to use a
* specific set of transformations associated to this transition.
*/
open val key: TransitionKey? = null

/**
* The progress of the transition. This is usually in the `[0; 1]` range, but it can also be
* less than `0` or greater than `1` when using transitions with a spring AnimationSpec or
Expand Down Expand Up @@ -455,11 +461,7 @@ internal abstract class BaseSceneTransitionLayoutState(
*
* Important: you *must* call [finishTransition] once the transition is finished.
*/
internal fun startTransition(
transition: TransitionState.Transition,
transitionKey: TransitionKey? = null,
chain: Boolean = true,
) {
internal fun startTransition(transition: TransitionState.Transition, chain: Boolean = true) {
checkThread()

// Compute the [TransformationSpec] when the transition starts.
Expand All @@ -469,7 +471,9 @@ internal abstract class BaseSceneTransitionLayoutState(

// Update the transition specs.
transition.transformationSpec =
transitions.transitionSpec(fromScene, toScene, key = transitionKey).transformationSpec()
transitions
.transitionSpec(fromScene, toScene, key = transition.key)
.transformationSpec()
if (orientation != null) {
transition.updateOverscrollSpecs(
fromSpec = transitions.overscrollSpec(fromScene, orientation),
Expand Down Expand Up @@ -568,9 +572,10 @@ internal abstract class BaseSceneTransitionLayoutState(
originalTransition = transitionState,
fromScene = targetCurrentScene,
toScene = matchingLink.targetTo,
key = matchingLink.targetTransitionKey,
)

stateLink.target.startTransition(linkedTransition, matchingLink.targetTransitionKey)
stateLink.target.startTransition(linkedTransition)
activeTransitionLinks[stateLink] = linkedTransition
}
}
Expand Down
2 changes: 2 additions & 0 deletions ...compose/scene/src/com/android/compose/animation/scene/transition/link/LinkedTransition.kt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
package com.android.compose.animation.scene.transition.link

import com.android.compose.animation.scene.SceneKey
import com.android.compose.animation.scene.TransitionKey
import com.android.compose.animation.scene.TransitionState
import kotlinx.coroutines.Job

Expand All @@ -25,6 +26,7 @@ internal class LinkedTransition(
private val originalTransition: TransitionState.Transition,
fromScene: SceneKey,
toScene: SceneKey,
override val key: TransitionKey? = null,
) : TransitionState.Transition(fromScene, toScene) {

override val currentScene: SceneKey
Expand Down
Loading

0 comments on commit 6f5a4cd

Please sign in to comment.