Skip to content

Commit b58059a

Browse files
MarkYavyschimke
MarkYav
and
yschimke
authored
Lottie grandchild fix v2 (#2802)
* Added test to showcase that the current code contains a bug: grandparent layer is not rendered. * Fix missing grandparent Lottie layer transforms (#2795) Replaced the shallow parent mapping with a topological traversal algorithm (buildAncestorTransforms) to accurately capture the entire hierarchy of ancestral transforms for each layer. Previously, Lottie layers completely discarded transforms generated past their immediate parent, breaking deep Lottie hierarchies. Providing the entire ancestral lineage as a sequential list natively preserves the top-down cascading render sequence, letting the RemoteCanvas apply each layer's intrinsic rotation, translation, and custom anchor points accurately. Additionally, wrapped the heavy hierarchical list allocations in a remember(animation.layers) block in LottieAnimation to prevent Garbage Collection churn and performance lags during recompositions. * added screenshots for the screenshot test * fixed imports and removed outdated TODO * Altered the test to better showcase the bug fix. 1. Red Square (Root layer): Drawn in the top-left at [0, 0]. 2. Blue Square (Parent layer): Shifts its local child to the top-right at [96, 0]. 3. Green Square (Child layer): Shifts its local child down by [0, 96]. Because the Green Square is a child of the blue square, it inherits the [96, 0] translation and applies its own [0, 96] translation. This means if the Lottie engine correctly parses all 3 tiers of the hierarchy, the Green Square correctly lands in the bottom-right corner. If the bug was still present (where it ignorantly dropped the grandparent's transform), the Green Square would wrongly render in the bottom-left corner. * Update parentChain golden screenshot for ancestral transforms fix --------- Co-authored-by: Yuri Schimke <yuri@schimke.ee> Co-authored-by: Yuri Schimke <yschimke@google.com>
1 parent 7557707 commit b58059a

7 files changed

Lines changed: 226 additions & 23 deletions

File tree

‎remotecompose/lottie/src/debug/res/raw/grandparent.json‎

Lines changed: 147 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,147 @@
1+
{
2+
"v": "0.0.0",
3+
"fr": 60,
4+
"ip": 0,
5+
"op": 60,
6+
"w": 128,
7+
"h": 128,
8+
"nm": "grandparent",
9+
"author": "Google Horologist",
10+
"description": "Synthetic file to test 3-deep nested layer transforms.",
11+
"layers": [
12+
{
13+
"ty": 4,
14+
"nm": "Grandparent Layer",
15+
"ind": 1,
16+
"ip": 0,
17+
"op": 60,
18+
"ks": {
19+
"a": { "a": 0, "k": [0, 0, 0] },
20+
"p": { "a": 0, "k": [0, 0, 0] },
21+
"r": { "a": 0, "k": 0 },
22+
"s": { "a": 0, "k": [100, 100, 100] },
23+
"o": { "a": 0, "k": 100 }
24+
},
25+
"shapes": [
26+
{
27+
"ty": "gr",
28+
"nm": "Top Left Square Group",
29+
"it": [
30+
{
31+
"ty": "rc",
32+
"nm": "Square",
33+
"p": { "a": 0, "k": [0, 0] },
34+
"s": { "a": 0, "k": [32, 32] },
35+
"r": { "a": 0, "k": 0 }
36+
},
37+
{
38+
"ty": "fl",
39+
"nm": "Fill Red",
40+
"c": { "a": 0, "k": [0.8, 0.2, 0.2, 1.0] },
41+
"o": { "a": 0, "k": 100 }
42+
},
43+
{
44+
"ty": "tr",
45+
"nm": "Transform",
46+
"a": { "a": 0, "k": [0, 0] },
47+
"p": { "a": 0, "k": [16, 16] },
48+
"s": { "a": 0, "k": [100, 100] },
49+
"r": { "a": 0, "k": 0 },
50+
"o": { "a": 0, "k": 100 }
51+
}
52+
]
53+
}
54+
]
55+
},
56+
{
57+
"ty": 4,
58+
"nm": "Parent Layer",
59+
"parent": 1,
60+
"ind": 2,
61+
"ip": 0,
62+
"op": 60,
63+
"ks": {
64+
"a": { "a": 0, "k": [0, 0, 0] },
65+
"p": { "a": 0, "k": [96, 0, 0] },
66+
"r": { "a": 0, "k": 0 },
67+
"s": { "a": 0, "k": [100, 100, 100] },
68+
"o": { "a": 0, "k": 100 }
69+
},
70+
"shapes": [
71+
{
72+
"ty": "gr",
73+
"nm": "Top Right Square Group",
74+
"it": [
75+
{
76+
"ty": "rc",
77+
"nm": "Square",
78+
"p": { "a": 0, "k": [0, 0] },
79+
"s": { "a": 0, "k": [32, 32] },
80+
"r": { "a": 0, "k": 0 }
81+
},
82+
{
83+
"ty": "fl",
84+
"nm": "Fill Blue",
85+
"c": { "a": 0, "k": [0.2, 0.2, 0.8, 1.0] },
86+
"o": { "a": 0, "k": 100 }
87+
},
88+
{
89+
"ty": "tr",
90+
"nm": "Transform",
91+
"a": { "a": 0, "k": [0, 0] },
92+
"p": { "a": 0, "k": [16, 16] },
93+
"s": { "a": 0, "k": [100, 100] },
94+
"r": { "a": 0, "k": 0 },
95+
"o": { "a": 0, "k": 100 }
96+
}
97+
]
98+
}
99+
]
100+
},
101+
{
102+
"ty": 4,
103+
"nm": "Child Layer",
104+
"parent": 2,
105+
"ind": 3,
106+
"ip": 0,
107+
"op": 60,
108+
"ks": {
109+
"a": { "a": 0, "k": [0, 0, 0] },
110+
"p": { "a": 0, "k": [0, 96, 0] },
111+
"r": { "a": 0, "k": 0 },
112+
"s": { "a": 0, "k": [100, 100, 100] },
113+
"o": { "a": 0, "k": 100 }
114+
},
115+
"shapes": [
116+
{
117+
"ty": "gr",
118+
"nm": "Bottom Right Square Group",
119+
"it": [
120+
{
121+
"ty": "rc",
122+
"nm": "Square",
123+
"p": { "a": 0, "k": [0, 0] },
124+
"s": { "a": 0, "k": [32, 32] },
125+
"r": { "a": 0, "k": 0 }
126+
},
127+
{
128+
"ty": "fl",
129+
"nm": "Fill Green",
130+
"c": { "a": 0, "k": [0.2, 0.8, 0.4, 1.0] },
131+
"o": { "a": 0, "k": 100 }
132+
},
133+
{
134+
"ty": "tr",
135+
"nm": "Transform",
136+
"a": { "a": 0, "k": [0, 0] },
137+
"p": { "a": 0, "k": [16, 16] },
138+
"s": { "a": 0, "k": [100, 100] },
139+
"r": { "a": 0, "k": 0 },
140+
"o": { "a": 0, "k": 100 }
141+
}
142+
]
143+
}
144+
]
145+
}
146+
]
147+
}

‎remotecompose/lottie/src/main/java/com/google/android/horologist/remotecompose/lottie/LottieAnimation.kt‎

Lines changed: 45 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -34,6 +34,8 @@ import androidx.compose.runtime.remember
3434
import androidx.compose.runtime.staticCompositionLocalOf
3535
import androidx.compose.ui.platform.LocalContext
3636
import com.google.android.horologist.remotecompose.lottie.format.Animation
37+
import com.google.android.horologist.remotecompose.lottie.format.GraphicElement.Transform
38+
import com.google.android.horologist.remotecompose.lottie.format.Layer
3739
import com.google.android.horologist.remotecompose.lottie.renderer.Layer
3840

3941
/**
@@ -136,10 +138,11 @@ internal fun LottieAnimation(
136138
)
137139

138140
CompositionLocalProvider(LocalAnimationSettings provides animationSettings) {
139-
val parentTransforms =
140-
animation.layers
141-
.filter { l -> l.index != null && l.transform != null }
142-
.associate { l -> Pair(l.index!!, l.transform!!) }
141+
// We remember this topologically sorted map because traversing the graph and tracking
142+
// hierarchical lists is an expensive allocation operation. Caching it guarantees it executes
143+
// strictly once per Lottie load, preventing heavy GC churn during recompositions.
144+
val ancestorTransforms =
145+
remember(animation.layers) { buildAncestorTransforms(animation.layers) }
143146

144147
val lottieWidth = animation.width.rf
145148
val lottieHeight = animation.height.rf
@@ -168,8 +171,45 @@ internal fun LottieAnimation(
168171
contentAlignment = RemoteAlignment.Center,
169172
) {
170173
for (layer in animation.layers) {
171-
Layer(layer, parentTransforms, null)
174+
Layer(layer, ancestorTransforms, null)
172175
}
173176
}
174177
}
175178
}
179+
180+
private fun buildAncestorTransforms(layers: List<Layer>): Map<Int, List<Transform>> {
181+
val map = mutableMapOf<Int, List<Transform>>()
182+
val childrenMap = layers.groupBy { it.parent }
183+
184+
val roots = childrenMap[null] ?: emptyList()
185+
for (layer in roots) {
186+
populateAncestorTransforms(layer, emptyList(), childrenMap, map)
187+
}
188+
189+
return map
190+
}
191+
192+
private fun populateAncestorTransforms(
193+
layer: Layer,
194+
currentStack: List<Transform>,
195+
childrenMap: Map<Int?, List<Layer>>,
196+
outMap: MutableMap<Int, List<Transform>>,
197+
) {
198+
val layerIndex = layer.index
199+
if (layerIndex != null) {
200+
outMap[layerIndex] = currentStack
201+
}
202+
203+
val layerTransform = layer.transform
204+
val nextStack =
205+
if (layerTransform != null) {
206+
currentStack + layerTransform
207+
} else {
208+
currentStack
209+
}
210+
211+
val children = childrenMap[layerIndex] ?: emptyList()
212+
for (child in children) {
213+
populateAncestorTransforms(child, nextStack, childrenMap, outMap)
214+
}
215+
}

‎remotecompose/lottie/src/main/java/com/google/android/horologist/remotecompose/lottie/renderer/Layer.kt‎

Lines changed: 14 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -25,25 +25,23 @@ import com.google.android.horologist.remotecompose.lottie.format.LayerType
2525
/** A Layer in the Lottie composition */
2626
@Composable
2727
@RemoteComposable
28-
internal fun Layer(layer: Layer, parentTransforms: Map<Int, Transform>, transform: Transform?) {
29-
val transformStack =
30-
if (layer.parent == null || !parentTransforms.containsKey(layer.parent)) {
31-
mutableListOf()
28+
internal fun Layer(
29+
layer: Layer,
30+
parentTransforms: Map<Int, List<Transform>>,
31+
transform: Transform?,
32+
) {
33+
val ancestorStack = parentTransforms[layer.index] ?: emptyList()
34+
35+
val completeStack =
36+
if (transform != null) {
37+
listOf(transform) + ancestorStack
3238
} else {
33-
mutableListOf(parentTransforms[layer.parent]!!)
39+
ancestorStack
3440
}
3541

36-
// TODO: Replace passing a transform param in and applying it to the transform stack with
37-
// graphicsLayer transforms in the calling composable, once the ANDROID_NATIVE player supports
38-
// graphicsLayer
39-
// (b/408913726)
40-
if (transform != null) {
41-
transformStack.add(0, transform)
42-
}
43-
4442
when (layer.type) {
4543
LayerType.Null -> {} // No-op - null layers are used to apply parent transforms.
46-
LayerType.Shape -> ShapeLayer(layer as Layer.ShapeLayer, transformStack)
44+
LayerType.Shape -> ShapeLayer(layer as Layer.ShapeLayer, completeStack)
4745
}
4846
}
4947

@@ -55,8 +53,9 @@ internal fun ShapeLayer(layer: Layer.ShapeLayer, transformStack: List<Transform?
5553
return
5654
}
5755

56+
val safeStack = transformStack?.filterNotNull() ?: emptyList()
5857
val updatedTransformStack =
59-
listOfNotNull(layer.transform, *(transformStack ?: listOf()).toTypedArray()).reversed()
58+
if (layer.transform != null) safeStack + layer.transform else safeStack
6059

6160
// TODO: Check start & end frame to see if we should be rendering
6261
RenderShapes(layer.shapes, updatedTransformStack)

‎remotecompose/lottie/src/test/java/com/google/android/horologist/remotecompose/lottie/LottieBasicScreenshotTest.kt‎

Lines changed: 12 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -36,6 +36,17 @@ class LottieBasicScreenshotTest : WearScreenshotTest() {
3636
}
3737
}
3838

39-
composeRule.onNodeWithTag("Box").captureRoboImage(testName(""))
39+
composeRule.onNodeWithTag("Box").captureRoboImage(testName("_geometry"))
40+
}
41+
42+
@Test
43+
fun grandparent() {
44+
composeRule.setContent {
45+
Box(modifier = Modifier.background(Color.White).testTag("Box")) {
46+
LottiePreview(R.raw.grandparent, clock = SettableRemoteClock())
47+
}
48+
}
49+
50+
composeRule.onNodeWithTag("Box").captureRoboImage(testName("_grandparent"))
4051
}
4152
}

‎remotecompose/lottie/src/test/screenshots/LottieBasicScreenshotTest_large_round_geometry.png‎

Lines changed: 3 additions & 0 deletions
Loading

‎remotecompose/lottie/src/test/screenshots/LottieBasicScreenshotTest_large_round_grandparent.png‎

Lines changed: 3 additions & 0 deletions
Loading

‎remotecompose/lottie/src/test/screenshots/LottieFeatureDiffScreenshotTest_parentChain.png‎

Lines changed: 2 additions & 2 deletions
Loading

0 commit comments

Comments
 (0)