Donate to e Foundation | Murena handsets with /e/OS | Own a part of Murena! Learn more

Commit d8ce27e6 authored by Jordan Demeulenaere's avatar Jordan Demeulenaere
Browse files

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
parent 944c3298

packages/SystemUI/compose/scene/src/com/android/compose/animation/scene/Element.kt

+17 −6
Original line number Diff line number Diff line
@@ -127,7 +127,14 @@ internal fun Modifier.element( 
    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 @@ internal fun Modifier.element( 
 */

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 @@ internal class ElementNode( 


    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 @@ internal class ElementNode( 
        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 @@ internal class ElementNode( 
    }



    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 @@ private fun interruptedAlpha( 
    )

}



@OptIn(ExperimentalComposeUiApi::class)

private fun ApproachMeasureScope.measure(

    layoutImpl: SceneTransitionLayoutImpl,

    scene: Scene,

packages/SystemUI/compose/scene/src/com/android/compose/animation/scene/SceneTransitionLayoutState.kt

+36 −28
Original line number Diff line number Diff line
@@ -26,8 +26,10 @@ import androidx.compose.runtime.Composable 
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 @@ internal abstract class BaseSceneTransitionLayoutState( 
     * 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 @@ internal abstract class BaseSceneTransitionLayoutState( 
        if (!enableInterruptions) {

            // Set the current transition.

            check(transitionStates.size == 1)

            transitionStates[0] = transition

            transitionStates = listOf(transition)

            return

        }
@@ -473,7 +476,7 @@ internal abstract class BaseSceneTransitionLayoutState( 
            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 @@ internal abstract class BaseSceneTransitionLayoutState( 
                    // 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.add(transition)

                    transitionStates = transitionStates + transition

                }

            }

        }

    }
@@ -571,6 +574,7 @@ internal abstract class BaseSceneTransitionLayoutState( 
            return

        }



        val transitionStates = this.transitionStates

        if (!transitionStates.contains(transition)) {

            // This transition was already removed from transitionStates.

            return
@@ -589,23 +593,38 @@ internal abstract class BaseSceneTransitionLayoutState( 
        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) {
@@ -748,17 +767,6 @@ internal class MutableSceneTransitionLayoutStateImpl( 
    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"

packages/SystemUI/compose/scene/tests/src/com/android/compose/animation/scene/ElementTest.kt

+38 −2
Original line number Diff line number Diff line
@@ -63,11 +63,13 @@ import com.android.compose.animation.scene.TestScenes.SceneA 
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 @@ class ElementTest { 
                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 @@ class ElementTest { 
    }



    @Test

    @Ignore("b/341072461")

    fun existingElementsDontRecomposeWhenTransitionStateChanges() {

        var fooCompositions = 0
@@ -603,6 +606,39 @@ class ElementTest { 
        }

    }



    @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,

packages/SystemUI/compose/scene/tests/src/com/android/compose/animation/scene/SceneTransitionLayoutTest.kt

+4 −3
Original line number Diff line number Diff line
@@ -76,12 +76,13 @@ class SceneTransitionLayoutTest { 


    /** 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 @@ class SceneTransitionLayoutTest { 


    @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.