From 1d5266a811156bfca9a4bc527ffde5f1530d7d3f Mon Sep 17 00:00:00 2001 From: funman300 Date: Fri, 26 Jun 2026 12:02:42 -0700 Subject: [PATCH] perf(engine): rebuild card children only on appearance change Every move fired StateChangedEvent -> sync_cards, which rebuilt the full visual for all 52 cards: despawning and respawning every child entity (drop-shadow, border frame, and on Android a Text2d corner label needing a glyph re-layout) even for the ~50 cards that did not move. That ~250 entity despawn/spawns plus 52 text re-layouts in a single frame spiked the StateChangedEvent frame and stuttered the slide animation on high-resolution devices (reported on a Galaxy Fold 7). Add a CardChildrenKey component capturing the only inputs the child entities depend on (face_up, card_size, color_blind, high_contrast). update_card_entity now rebuilds children only when that key changes (a flip, resize/fold, or accessibility toggle); a position-only move just updates the Transform. The Sprite is still refreshed every sync (a cheap handle swap), so theme/card-back image changes need no child rebuild. Adds a regression test asserting an appearance-neutral StateChangedEvent no longer despawns/respawns card label children. Co-Authored-By: Claude Opus 4.8 (1M context) --- solitaire_engine/src/card_plugin.rs | 186 +++++++++++++++++++++------- 1 file changed, 144 insertions(+), 42 deletions(-) diff --git a/solitaire_engine/src/card_plugin.rs b/solitaire_engine/src/card_plugin.rs index 2c9bfe5..c883c52 100644 --- a/solitaire_engine/src/card_plugin.rs +++ b/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::()` + 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>, slide_dur: Option>, settings: Option>, - entities: Query<(Entity, &CardEntity, &Transform, Option<&CardAnimation>)>, + entities: Query, card_images: Option>, font_res: Option>, ) { @@ -767,7 +801,7 @@ fn sync_cards_on_change( layout: Option>, slide_dur: Option>, settings: Option>, - entities: Query<(Entity, &CardEntity, &Transform, Option<&CardAnimation>)>, + entities: Query, card_images: Option>, font_res: Option>, ) { @@ -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, card_images: Option<&CardImageSet>, selected_back: usize, font_handle: Option<&Handle>, @@ -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)> = HashMap::new(); - for (entity, marker, transform, anim) in entities.iter() { + let mut existing: HashMap, Option)> = + 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 = 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, card_images: Option<&CardImageSet>, selected_back: usize, font_handle: Option<&Handle>, @@ -1180,44 +1230,58 @@ fn update_card_entity( } } - // Despawn any stale children and re-add the per-card drop shadow plus, - // in solid-colour fallback mode, the label overlay. In image mode the - // rank/suit are baked into the PNG; on Android we also add a large-print - // corner overlay so they are legible at phone scale. - commands.entity(entity).despawn_related::(); - commands.entity(entity).with_children(|b| { - add_card_shadow_child(b, layout.card_size); - }); - commands.entity(entity).with_children(|b| { - add_card_back_frame_child(b, layout.card_size); - }); - if card_images.is_none() { + // Rebuild the card's child visuals (drop-shadow, border frame, and the + // rank/suit label / large-print corner overlay) only when an input that + // affects them actually changed. The child set depends solely on + // `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::(); commands.entity(entity).with_children(|b| { - b.spawn(( - CardLabel, - Text2d::new(label_for(card)), - TextFont { - font_size: layout.card_size.x * FONT_SIZE_FRAC, - ..default() - }, - TextColor(text_colour(card, color_blind, high_contrast)), - Transform::from_xyz(0.0, 0.0, 0.01), - label_visibility(face_up), - )); + add_card_shadow_child(b, layout.card_size); }); - } - if USE_TOUCH_UI_LAYOUT && card_images.is_some() { commands.entity(entity).with_children(|b| { - add_android_corner_label( - b, - card, - face_up, - layout.card_size, - color_blind, - high_contrast, - font_handle, - ); + add_card_back_frame_child(b, layout.card_size); }); + if card_images.is_none() { + commands.entity(entity).with_children(|b| { + b.spawn(( + CardLabel, + Text2d::new(label_for(card)), + TextFont { + font_size: layout.card_size.x * FONT_SIZE_FRAC, + ..default() + }, + TextColor(text_colour(card, color_blind, high_contrast)), + Transform::from_xyz(0.0, 0.0, 0.01), + label_visibility(face_up), + )); + }); + } + if USE_TOUCH_UI_LAYOUT && card_images.is_some() { + commands.entity(entity).with_children(|b| { + add_android_corner_label( + b, + card, + face_up, + layout.card_size, + color_blind, + high_contrast, + font_handle, + ); + }); + } + commands.entity(entity).insert(new_children_key); } } @@ -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 = app + .world_mut() + .query_filtered::>() + .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 = app + .world_mut() + .query_filtered::>() + .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, -- 2.47.3