Merge pull request 'perf(engine): rebuild card children only on appearance change' (#112) from fix/card-move-anim-jank into master

solitaire_engine/src/card_plugin.rs

@@ -195,6 +195,40 @@ pub struct CardEntity {
     pub card: Card,
 }
 
+/// Cached signature of the inputs that determine a card entity's *child*
+/// visuals (drop-shadow, border frame, and the rank/suit label / large-print
+/// corner overlay). Stored on each card so [`update_card_entity`] can skip the
+/// expensive `despawn_related::<Children>()` + child respawn when nothing about
+/// the appearance changed — the common case during a move, where only the
+/// card's position changes. Without this guard every `StateChangedEvent`
+/// rebuilds all 52 cards' children (≈250 entity spawn/despawns plus 52 `Text2d`
+/// glyph re-layouts) in a single frame, which stutters the slide animation on
+/// high-resolution devices.
+///
+/// The children depend only on these four inputs; the card identity is fixed
+/// per entity, and the face/back *image* is handled by the always-refreshed
+/// `Sprite` (a cheap handle swap), so theme/card-back changes need no child
+/// rebuild.
+#[derive(Component, Clone, Copy, PartialEq)]
+struct CardChildrenKey {
+    face_up: bool,
+    card_size: Vec2,
+    color_blind: bool,
+    high_contrast: bool,
+}
+
+/// Query data read by the card-sync systems for each live card entity:
+/// its id, card identity, current transform, any in-flight curve animation,
+/// and its cached child-appearance key. Factored into an alias to keep the
+/// system signatures readable (and satisfy clippy's `type_complexity`).
+type CardSyncData = (
+    Entity,
+    &'static CardEntity,
+    &'static Transform,
+    Option<&'static CardAnimation>,
+    Option<&'static CardChildrenKey>,
+);
+
 /// Render-side index mapping each live board card to its [`CardEntity`].
 ///
 /// Maintained exclusively by [`rebuild_card_entity_index`] in `PostUpdate`,
@@ -732,7 +766,7 @@ fn sync_cards_startup(
     layout: Option<Res<LayoutResource>>,
     slide_dur: Option<Res<EffectiveSlideDuration>>,
     settings: Option<Res<SettingsResource>>,
-    entities: Query<(Entity, &CardEntity, &Transform, Option<&CardAnimation>)>,
+    entities: Query<CardSyncData>,
     card_images: Option<Res<CardImageSet>>,
     font_res: Option<Res<FontResource>>,
 ) {
@@ -767,7 +801,7 @@ fn sync_cards_on_change(
     layout: Option<Res<LayoutResource>>,
     slide_dur: Option<Res<EffectiveSlideDuration>>,
     settings: Option<Res<SettingsResource>>,
-    entities: Query<(Entity, &CardEntity, &Transform, Option<&CardAnimation>)>,
+    entities: Query<CardSyncData>,
     card_images: Option<Res<CardImageSet>>,
     font_res: Option<Res<FontResource>>,
 ) {
@@ -806,7 +840,7 @@ fn sync_cards(
     back_colour: Color,
     color_blind: bool,
     high_contrast: bool,
-    entities: &Query<(Entity, &CardEntity, &Transform, Option<&CardAnimation>)>,
+    entities: &Query<CardSyncData>,
     card_images: Option<&CardImageSet>,
     selected_back: usize,
     font_handle: Option<&Handle<Font>>,
@@ -840,18 +874,24 @@ fn sync_cards(
     //   • end ≠ target  → the game state has changed (e.g. a new game started
     //                      while the win-cascade was mid-flight); cancel the
     //                      stale `CardAnimation` and apply the new position.
-    let mut existing: HashMap<Card, (Entity, Vec3, Option<Vec2>)> = HashMap::new();
+    let mut existing: HashMap<Card, (Entity, Vec3, Option<Vec2>, Option<CardChildrenKey>)> =
-    for (entity, marker, transform, anim) in entities.iter() {
+        HashMap::new();
+    for (entity, marker, transform, anim, children_key) in entities.iter() {
         existing.insert(
             marker.card.clone(),
-            (entity, transform.translation, anim.map(|a| a.end)),
+            (
+                entity,
+                transform.translation,
+                anim.map(|a| a.end),
+                children_key.copied(),
+            ),
         );
     }
 
     let live_ids: HashSet<Card> = positions.iter().map(|(c, _, _)| c.0.clone()).collect();
 
     // Despawn any entity whose card is no longer tracked.
-    for (card, (entity, _, _)) in &existing {
+    for (card, (entity, _, _, _)) in &existing {
         if !live_ids.contains(card) {
             commands.entity(*entity).despawn();
         }
@@ -862,7 +902,7 @@ fn sync_cards(
     // behind the incoming top card during the draw slide animation.
     for ((card, face_up), position, z) in positions {
         let entity = match existing.get(&card) {
-            Some(&(entity, cur, anim_end)) => {
+            Some(&(entity, cur, anim_end, children_key)) => {
                 // If a CardAnimation is in flight, check whether its destination
                 // still matches the game-state target.  If the game moved the card
                 // elsewhere (e.g. new game started during a win-cascade scatter),
@@ -889,6 +929,7 @@ fn sync_cards(
                     high_contrast,
                     cur,
                     has_anim,
+                    children_key,
                     card_images,
                     selected_back,
                     font_handle,
@@ -1114,6 +1155,14 @@ fn spawn_card_entity(
             );
         });
     }
+    // Record the appearance signature so subsequent `update_card_entity` calls
+    // can skip rebuilding these children until one of the inputs changes.
+    entity.insert(CardChildrenKey {
+        face_up,
+        card_size: layout.card_size,
+        color_blind,
+        high_contrast,
+    });
     entity_id
 }
 
@@ -1132,6 +1181,7 @@ fn update_card_entity(
     high_contrast: bool,
     cur: Vec3,
     has_card_animation: bool,
+    existing_children_key: Option<CardChildrenKey>,
     card_images: Option<&CardImageSet>,
     selected_back: usize,
     font_handle: Option<&Handle<Font>>,
@@ -1180,10 +1230,22 @@ fn update_card_entity(
         }
     }
 
-    // Despawn any stale children and re-add the per-card drop shadow plus,
+    // Rebuild the card's child visuals (drop-shadow, border frame, and the
-    // in solid-colour fallback mode, the label overlay. In image mode the
+    // rank/suit label / large-print corner overlay) only when an input that
-    // rank/suit are baked into the PNG; on Android we also add a large-print
+    // affects them actually changed. The child set depends solely on
-    // corner overlay so they are legible at phone scale.
+    // `CardChildrenKey`; the face/back image is carried by the always-refreshed
+    // `Sprite` above, so theme/card-back swaps need no child rebuild. Skipping
+    // this on a position-only move avoids despawning and respawning the child
+    // entities (incl. a `Text2d` glyph re-layout) for all 52 cards on every
+    // `StateChangedEvent` — the spike that stuttered the slide animation on
+    // high-resolution devices.
+    let new_children_key = CardChildrenKey {
+        face_up,
+        card_size: layout.card_size,
+        color_blind,
+        high_contrast,
+    };
+    if existing_children_key != Some(new_children_key) {
         commands.entity(entity).despawn_related::<Children>();
         commands.entity(entity).with_children(|b| {
             add_card_shadow_child(b, layout.card_size);
@@ -1219,6 +1281,8 @@ fn update_card_entity(
                 );
             });
         }
+        commands.entity(entity).insert(new_children_key);
+    }
 }
 
 fn label_for(card: &Card) -> String {
@@ -3216,6 +3280,44 @@ mod tests {
         }
     }
 
+    #[test]
+    fn appearance_neutral_state_change_does_not_rebuild_card_children() {
+        // A StateChangedEvent that doesn't alter any card's appearance — the
+        // common case during a move, for the ~50 cards that didn't move or flip
+        // — must NOT despawn and respawn child entities. Before the
+        // CardChildrenKey guard every StateChangedEvent rebuilt all 52 cards'
+        // children (incl. a Text2d glyph re-layout each), the per-move spike
+        // that stuttered the slide animation on high-resolution devices.
+        let mut app = app();
+
+        let labels_before: HashSet<Entity> = app
+            .world_mut()
+            .query_filtered::<Entity, With<CardLabel>>()
+            .iter(app.world())
+            .collect();
+        assert!(
+            !labels_before.is_empty(),
+            "fixture should have spawned CardLabel children in the fallback path"
+        );
+
+        // Fire a StateChangedEvent without mutating the game: no card moves or
+        // flips, so every card's CardChildrenKey is unchanged.
+        app.world_mut().write_message(StateChangedEvent);
+        app.update();
+
+        let labels_after: HashSet<Entity> = app
+            .world_mut()
+            .query_filtered::<Entity, With<CardLabel>>()
+            .iter(app.world())
+            .collect();
+
+        assert_eq!(
+            labels_before, labels_after,
+            "an appearance-neutral StateChangedEvent must not despawn/respawn card \
+             children — the CardChildrenKey guard should have skipped the rebuild"
+        );
+    }
+
     #[test]
     fn resize_in_place_updates_card_label_font_size() {
         // Capture an arbitrary CardLabel's TextFont.font_size before resize,