Read transitions during composition instead of layout/drawing
This CL moves the read of the current transition during composition and
passes it to the element() Modifier. This ensures that new values of the
current transition are first seen during composition, then during
layout/drawing. See b/341072461 for details.
This CL is expected to have a slight negative impact on performance.
Preliminary experiments show a noticeable increase of P99 frame time in
the STL benchmarks and a very small increase in dropped frames. See the
dashboard at http://shortn/_x4sPtuA694.
Bug: 341072461
Test: atest PlatformSceneTransitionLayoutTests
Test: atest ElementTest
Flag: com.android.systemui.scene_container
Change-Id: Ie02f4ed0d6b1910a235637c6cb072a7ce2639b17
diff --git a/packages/SystemUI/compose/scene/src/com/android/compose/animation/scene/Element.kt b/packages/SystemUI/compose/scene/src/com/android/compose/animation/scene/Element.kt
index 20742ee..72da3b8 100644
--- a/packages/SystemUI/compose/scene/src/com/android/compose/animation/scene/Element.kt
+++ b/packages/SystemUI/compose/scene/src/com/android/compose/animation/scene/Element.kt
@@ -127,7 +127,14 @@
layoutImpl: SceneTransitionLayoutImpl,
scene: Scene,
key: ElementKey,
-): Modifier = this.then(ElementModifier(layoutImpl, scene, key)).testTag(key.testTag)
+): Modifier {
+ // Make sure that we read the current transitions during composition and not during
+ // layout/drawing.
+ // TODO(b/341072461): Revert this and read the current transitions in ElementNode directly once
+ // we can ensure that SceneTransitionLayoutImpl will compose new scenes first.
+ val currentTransitions = layoutImpl.state.currentTransitions
+ return then(ElementModifier(layoutImpl, currentTransitions, scene, key)).testTag(key.testTag)
+}
/**
* An element associated to [ElementNode]. Note that this element does not support updates as its
@@ -135,18 +142,20 @@
*/
private data class ElementModifier(
private val layoutImpl: SceneTransitionLayoutImpl,
+ private val currentTransitions: List<TransitionState.Transition>,
private val scene: Scene,
private val key: ElementKey,
) : ModifierNodeElement<ElementNode>() {
- override fun create(): ElementNode = ElementNode(layoutImpl, scene, key)
+ override fun create(): ElementNode = ElementNode(layoutImpl, currentTransitions, scene, key)
override fun update(node: ElementNode) {
- node.update(layoutImpl, scene, key)
+ node.update(layoutImpl, currentTransitions, scene, key)
}
}
internal class ElementNode(
private var layoutImpl: SceneTransitionLayoutImpl,
+ private var currentTransitions: List<TransitionState.Transition>,
private var scene: Scene,
private var key: ElementKey,
) : Modifier.Node(), DrawModifierNode, ApproachLayoutModifierNode {
@@ -202,10 +211,13 @@
fun update(
layoutImpl: SceneTransitionLayoutImpl,
+ currentTransitions: List<TransitionState.Transition>,
scene: Scene,
key: ElementKey,
) {
check(layoutImpl == this.layoutImpl && scene == this.scene)
+ this.currentTransitions = currentTransitions
+
removeNodeFromSceneState()
val prevElement = this.element
@@ -236,7 +248,7 @@
measurable: Measurable,
constraints: Constraints,
): MeasureResult {
- val transitions = layoutImpl.state.currentTransitions
+ val transitions = currentTransitions
val transition = elementTransition(element, transitions)
// If this element is not supposed to be laid out now, either because it is not part of any
@@ -270,7 +282,7 @@
}
override fun ContentDrawScope.draw() {
- val transition = elementTransition(element, layoutImpl.state.currentTransitions)
+ val transition = elementTransition(element, currentTransitions)
val drawScale = getDrawScale(layoutImpl, scene, element, transition, sceneState)
if (drawScale == Scale.Default) {
drawContent()
@@ -615,7 +627,6 @@
)
}
-@OptIn(ExperimentalComposeUiApi::class)
private fun ApproachMeasureScope.measure(
layoutImpl: SceneTransitionLayoutImpl,
scene: Scene,
diff --git a/packages/SystemUI/compose/scene/src/com/android/compose/animation/scene/SceneTransitionLayoutState.kt b/packages/SystemUI/compose/scene/src/com/android/compose/animation/scene/SceneTransitionLayoutState.kt
index 4e3a032..cb4d582 100644
--- a/packages/SystemUI/compose/scene/src/com/android/compose/animation/scene/SceneTransitionLayoutState.kt
+++ b/packages/SystemUI/compose/scene/src/com/android/compose/animation/scene/SceneTransitionLayoutState.kt
@@ -26,8 +26,10 @@
import androidx.compose.runtime.LaunchedEffect
import androidx.compose.runtime.SideEffect
import androidx.compose.runtime.Stable
+import androidx.compose.runtime.getValue
+import androidx.compose.runtime.mutableStateOf
import androidx.compose.runtime.remember
-import androidx.compose.runtime.snapshots.SnapshotStateList
+import androidx.compose.runtime.setValue
import androidx.compose.ui.util.fastAll
import androidx.compose.ui.util.fastFilter
import androidx.compose.ui.util.fastForEach
@@ -380,8 +382,9 @@
* 2. A list with one or more [TransitionState.Transition], when we are transitioning.
*/
@VisibleForTesting
- internal val transitionStates: MutableList<TransitionState> =
- SnapshotStateList<TransitionState>().apply { add(TransitionState.Idle(initialScene)) }
+ internal var transitionStates: List<TransitionState> by
+ mutableStateOf(listOf(TransitionState.Idle(initialScene)))
+ private set
override val transitionState: TransitionState
get() = transitionStates.last()
@@ -465,7 +468,7 @@
if (!enableInterruptions) {
// Set the current transition.
check(transitionStates.size == 1)
- transitionStates[0] = transition
+ transitionStates = listOf(transition)
return
}
@@ -473,7 +476,7 @@
is TransitionState.Idle -> {
// Replace [Idle] by [transition].
check(transitionStates.size == 1)
- transitionStates[0] = transition
+ transitionStates = listOf(transition)
}
is TransitionState.Transition -> {
// Force the current transition to finish to currentScene.
@@ -497,11 +500,11 @@
// we end up only with the new transition after appending it.
check(transitionStates.size == 1)
check(transitionStates[0] is TransitionState.Idle)
- transitionStates.clear()
+ transitionStates = listOf(transition)
+ } else {
+ // Append the new transition.
+ transitionStates = transitionStates + transition
}
-
- // Append the new transition.
- transitionStates.add(transition)
}
}
}
@@ -571,6 +574,7 @@
return
}
+ val transitionStates = this.transitionStates
if (!transitionStates.contains(transition)) {
// This transition was already removed from transitionStates.
return
@@ -589,25 +593,40 @@
var lastRemovedIdleScene: SceneKey? = null
// Remove all first n finished transitions.
- while (transitionStates.isNotEmpty()) {
- val firstTransition = transitionStates[0]
- if (!finishedTransitions.contains(firstTransition)) {
+ var i = 0
+ val nStates = transitionStates.size
+ while (i < nStates) {
+ val t = transitionStates[i]
+ if (!finishedTransitions.contains(t)) {
// Stop here.
break
}
- // Remove the transition from the list and from the set of finished transitions.
- transitionStates.removeAt(0)
- lastRemovedIdleScene = finishedTransitions.remove(firstTransition)
+ // Remove the transition from the set of finished transitions.
+ lastRemovedIdleScene = finishedTransitions.remove(t)
+ i++
}
// If all transitions are finished, we are idle.
- if (transitionStates.isEmpty()) {
+ if (i == nStates) {
check(finishedTransitions.isEmpty())
- transitionStates.add(TransitionState.Idle(checkNotNull(lastRemovedIdleScene)))
+ this.transitionStates = listOf(TransitionState.Idle(checkNotNull(lastRemovedIdleScene)))
+ } else if (i > 0) {
+ this.transitionStates = transitionStates.subList(fromIndex = i, toIndex = nStates)
}
}
+ fun snapToScene(scene: SceneKey) {
+ // Force finish all transitions.
+ while (currentTransitions.isNotEmpty()) {
+ val transition = transitionStates[0] as TransitionState.Transition
+ finishTransition(transition, transition.currentScene)
+ }
+
+ check(transitionStates.size == 1)
+ transitionStates = listOf(TransitionState.Idle(scene))
+ }
+
private fun finishActiveTransitionLinks(idleScene: SceneKey) {
val previousTransition = this.transitionState as? TransitionState.Transition ?: return
for ((link, linkedTransition) in activeTransitionLinks) {
@@ -748,17 +767,6 @@
override fun CoroutineScope.onChangeScene(scene: SceneKey) {
setTargetScene(scene, coroutineScope = this)
}
-
- override fun snapToScene(scene: SceneKey) {
- // Force finish all transitions.
- while (currentTransitions.isNotEmpty()) {
- val transition = transitionStates[0] as TransitionState.Transition
- finishTransition(transition, transition.currentScene)
- }
-
- check(transitionStates.size == 1)
- transitionStates[0] = TransitionState.Idle(scene)
- }
}
private const val TAG = "SceneTransitionLayoutState"
diff --git a/packages/SystemUI/compose/scene/tests/src/com/android/compose/animation/scene/ElementTest.kt b/packages/SystemUI/compose/scene/tests/src/com/android/compose/animation/scene/ElementTest.kt
index e19dc96..2c4fa5d 100644
--- a/packages/SystemUI/compose/scene/tests/src/com/android/compose/animation/scene/ElementTest.kt
+++ b/packages/SystemUI/compose/scene/tests/src/com/android/compose/animation/scene/ElementTest.kt
@@ -63,11 +63,13 @@
import com.android.compose.animation.scene.TestScenes.SceneB
import com.android.compose.animation.scene.TestScenes.SceneC
import com.android.compose.animation.scene.subjects.assertThat
+import com.android.compose.test.assertSizeIsEqualTo
import com.google.common.truth.Truth.assertThat
import kotlinx.coroutines.CoroutineScope
import kotlinx.coroutines.launch
import kotlinx.coroutines.test.runTest
import org.junit.Assert.assertThrows
+import org.junit.Ignore
import org.junit.Rule
import org.junit.Test
import org.junit.runner.RunWith
@@ -237,9 +239,9 @@
changeScene(SceneC)
}
- at(2 * frameDuration) { onElement(TestElements.Bar).assertIsNotDisplayed() }
+ at(3 * frameDuration) { onElement(TestElements.Bar).assertIsNotDisplayed() }
- at(3 * frameDuration) { onElement(TestElements.Bar).assertDoesNotExist() }
+ at(4 * frameDuration) { onElement(TestElements.Bar).assertDoesNotExist() }
}
}
@@ -578,6 +580,7 @@
}
@Test
+ @Ignore("b/341072461")
fun existingElementsDontRecomposeWhenTransitionStateChanges() {
var fooCompositions = 0
@@ -603,6 +606,39 @@
}
}
+ @Test
+ // TODO(b/341072461): Remove this test.
+ fun layoutGetsCurrentTransitionStateFromComposition() {
+ val state =
+ MutableSceneTransitionLayoutStateImpl(
+ SceneA,
+ transitions {
+ from(SceneA, to = SceneB) {
+ scaleSize(TestElements.Foo, width = 2f, height = 2f)
+ }
+ }
+ )
+
+ rule.setContent {
+ SceneTransitionLayout(state) {
+ scene(SceneA) { Box(Modifier.element(TestElements.Foo).size(20.dp)) }
+ scene(SceneB) {}
+ }
+ }
+
+ // Pause the clock to block recompositions.
+ rule.mainClock.autoAdvance = false
+
+ // Change the current transition.
+ state.startTransition(
+ transition(from = SceneA, to = SceneB, progress = { 0.5f }),
+ transitionKey = null,
+ )
+
+ // The size of Foo should still be 20dp given that the new state was not composed yet.
+ rule.onNode(isElement(TestElements.Foo)).assertSizeIsEqualTo(20.dp, 20.dp)
+ }
+
private fun setupOverscrollScenario(
layoutWidth: Dp,
layoutHeight: Dp,
diff --git a/packages/SystemUI/compose/scene/tests/src/com/android/compose/animation/scene/SceneTransitionLayoutTest.kt b/packages/SystemUI/compose/scene/tests/src/com/android/compose/animation/scene/SceneTransitionLayoutTest.kt
index 692c18b..3751a22 100644
--- a/packages/SystemUI/compose/scene/tests/src/com/android/compose/animation/scene/SceneTransitionLayoutTest.kt
+++ b/packages/SystemUI/compose/scene/tests/src/com/android/compose/animation/scene/SceneTransitionLayoutTest.kt
@@ -76,12 +76,13 @@
/** The content under test. */
@Composable
- private fun TestContent() {
+ private fun TestContent(enableInterruptions: Boolean = true) {
layoutState =
updateSceneTransitionLayoutState(
currentScene,
{ currentScene = it },
- EmptyTestTransitions
+ EmptyTestTransitions,
+ enableInterruptions = enableInterruptions,
)
SceneTransitionLayout(
@@ -219,7 +220,7 @@
@Test
fun testSharedElement() {
- rule.setContent { TestContent() }
+ rule.setContent { TestContent(enableInterruptions = false) }
// In scene A, the shared element SharedFoo() is at the top end of the layout and has a size
// of 50.dp.