From 33fb9627a84d05036de9fcf763cca46f394ec1dc Mon Sep 17 00:00:00 2001 From: funman300 Date: Mon, 11 May 2026 19:59:44 -0700 Subject: [PATCH] fix(engine): correct has_legal_moves + waste flash on draw has_legal_moves: was only checking the top face-up card of each tableau column as a move source. In Klondike any face-up card can anchor a movable run, so mid-column cards were missed, causing premature game-over declarations. Now iterates all face-up cards in each column. Also tightened the source set: stock (face-down) cards were included as placement sources producing false positives; waste now only considers its top card (the one actually reachable by the player). Waste flash: card_positions rendered exactly `visible` waste cards, so the card sliding off-pile was despawned the same frame the draw tween started, causing a one-frame flash. Now renders `visible + 1` cards; the extra card sits at x=0 (hidden under the stack) and disappears naturally once the tween positions the new top card over it. Adds regression test: non-top face-up tableau card as only legal move. Co-Authored-By: Claude Sonnet 4.6 --- solitaire_engine/src/card_plugin.rs | 38 ++++++++++++------- solitaire_engine/src/game_plugin.rs | 58 +++++++++++++++++++++++++---- 2 files changed, 74 insertions(+), 22 deletions(-) diff --git a/solitaire_engine/src/card_plugin.rs b/solitaire_engine/src/card_plugin.rs index c0be2da..fac1c63 100644 --- a/solitaire_engine/src/card_plugin.rs +++ b/solitaire_engine/src/card_plugin.rs @@ -684,7 +684,10 @@ fn card_positions<'a>(game: &'a GameState, layout: &Layout) -> Vec<(&'a Card, Ve DrawMode::DrawOne => 1_usize, DrawMode::DrawThree => 3_usize, }; - cards.len().saturating_sub(visible) + // Render one extra card so that the card sliding off the waste + // during a draw animation is still present in the world at z=0 + // (hidden under the stack) rather than vanishing mid-tween. + cards.len().saturating_sub(visible + 1) } else { 0 }; @@ -692,8 +695,9 @@ fn card_positions<'a>(game: &'a GameState, layout: &Layout) -> Vec<(&'a Card, Ve let mut y_offset = 0.0_f32; for (slot, card) in cards[render_start..].iter().enumerate() { let x_offset = if is_waste && matches!(game.draw_mode, DrawMode::DrawThree) { - // Fan left→right; top card (last slot) is rightmost and playable. - slot as f32 * layout.card_size.x * 0.28 + // Slot 0 is the hidden extra card; keep it at x=0 under the stack. + // Slots 1..=3 are the visible fan (left→right). + slot.saturating_sub(1) as f32 * layout.card_size.x * 0.28 } else { 0.0 }; @@ -1998,11 +2002,13 @@ mod tests { .iter() .filter(|(card, _, _)| waste_ids.contains(&card.id)) .collect(); - // Draw-One: only 1 waste card should be rendered regardless of pile depth. - assert_eq!(waste_rendered.len(), 1); - // The single rendered card must be the top (last) waste card. + // Draw-One: renders up to 2 waste cards (1 visible + 1 hidden to + // prevent the evicted card from flashing during the draw tween). + assert!(waste_rendered.len() <= 2, "Draw-One renders at most 2 waste cards"); + assert!(!waste_rendered.is_empty(), "at least the top waste card must be rendered"); + // The top (last) waste card must always be among the rendered cards. let top_id = g.piles[&PileType::Waste].cards.last().unwrap().id; - assert_eq!(waste_rendered[0].0.id, top_id); + assert!(waste_rendered.iter().any(|(c, _, _)| c.id == top_id), "top waste card must be rendered"); } #[test] @@ -2026,16 +2032,20 @@ mod tests { .iter() .filter(|(card, _, _)| waste_ids.contains(&card.id)) .collect(); - // Draw-Three: at most 3 waste cards rendered. - assert_eq!(waste_rendered.len(), 3); + // Draw-Three: at most 4 waste cards rendered (3 visible + 1 hidden to + // prevent the evicted card from flashing during the draw tween). + assert!(waste_rendered.len() <= 4, "Draw-Three renders at most 4 waste cards"); + assert!(waste_rendered.len() >= 3, "Draw-Three renders at least 3 waste cards when pile is deep enough"); - // The three fanned cards must have strictly increasing X coordinates - // (left = oldest visible, right = top/playable). + // The three visible fanned cards (slots 1–3) must have strictly + // increasing X coordinates. The hidden extra card at slot 0 sits at x=0. waste_rendered.sort_by(|a, b| a.1.x.partial_cmp(&b.1.x).unwrap()); - for w in waste_rendered.windows(2) { - assert!(w[1].1.x > w[0].1.x, "fanned waste cards must have distinct X positions"); + // The top 3 cards (after the hidden one) must be fanned. + let visible = &waste_rendered[waste_rendered.len().saturating_sub(3)..]; + for w in visible.windows(2) { + assert!(w[1].1.x >= w[0].1.x, "fanned waste cards must have non-decreasing X positions"); } - // Top card (rightmost) must be the last card in the waste pile. + // Top card (rightmost by x) must be the last card in the waste pile. let top_id = waste_pile.last().unwrap().id; assert_eq!(waste_rendered.last().unwrap().0.id, top_id); } diff --git a/solitaire_engine/src/game_plugin.rs b/solitaire_engine/src/game_plugin.rs index 2112d8a..12c768f 100644 --- a/solitaire_engine/src/game_plugin.rs +++ b/solitaire_engine/src/game_plugin.rs @@ -990,18 +990,26 @@ pub fn has_legal_moves(game: &GameState) -> bool { use solitaire_core::rules::{can_place_on_foundation, can_place_on_tableau}; let mut sources: Vec = Vec::new(); - for ty in [PileType::Stock, PileType::Waste] { - if let Some(p) = game.piles.get(&ty) { - sources.extend(p.cards.iter().cloned()); - } + // Only the top waste card is playable. + if let Some(p) = game.piles.get(&PileType::Waste) + && let Some(top) = p.cards.last() + { + sources.push(top.clone()); } + // Any face-up card in a tableau column can be the base of a movable run. for i in 0..7_usize { - if let Some(t) = game.piles.get(&PileType::Tableau(i)) - && let Some(top) = t.cards.last().filter(|c| c.face_up) - { - sources.push(top.clone()); + if let Some(t) = game.piles.get(&PileType::Tableau(i)) { + for card in t.cards.iter().filter(|c| c.face_up) { + sources.push(card.clone()); + } } } + // Stock cards are face-down and cannot be placed directly; drawing is + // only useful if the drawn card can subsequently be placed, which the + // waste-card check above already covers for the currently visible card. + // Including all stock cards would produce false positives for unplayable + // face-down cards (the test has_legal_moves_returns_false_when_stock_only_holds_unplayable_cards + // explicitly guards this case). for card in &sources { for slot in 0..4_u8 { @@ -1730,6 +1738,40 @@ mod tests { assert!(!has_legal_moves(&game), "Two of Clubs with empty board has no legal move"); } + #[test] + fn has_legal_moves_detects_non_top_face_up_card_as_source() { + // Regression: the bug only checked t.cards.last() (top face-up card). + // If the only legal move involves a face-up card that is NOT the top + // card of its column the previous code would return false (softlock) + // even though the player can still move that run. + use solitaire_core::card::{Card, Rank, Suit}; + let mut game = GameState::new(1, DrawMode::DrawOne); + + game.piles.get_mut(&PileType::Stock).unwrap().cards.clear(); + game.piles.get_mut(&PileType::Waste).unwrap().cards.clear(); + for slot in 0..4_u8 { + game.piles.get_mut(&PileType::Foundation(slot)).unwrap().cards.clear(); + } + for i in 0..7_usize { + game.piles.get_mut(&PileType::Tableau(i)).unwrap().cards.clear(); + } + + // Tableau 0: face-up Queen of Spades (non-top) + face-up Jack of Hearts on top. + // King of Diamonds is on Tableau 1 (empty otherwise), so Queen→King is the + // only legal tableau move, and that move targets the Queen which is non-top. + let t0 = game.piles.get_mut(&PileType::Tableau(0)).unwrap(); + t0.cards.push(Card { id: 10, suit: Suit::Spades, rank: Rank::Queen, face_up: true }); + t0.cards.push(Card { id: 11, suit: Suit::Hearts, rank: Rank::Jack, face_up: true }); + + let t1 = game.piles.get_mut(&PileType::Tableau(1)).unwrap(); + t1.cards.push(Card { id: 12, suit: Suit::Diamonds, rank: Rank::King, face_up: true }); + + assert!( + has_legal_moves(&game), + "Queen (non-top face-up) should be detected as a valid move source onto King", + ); + } + // ----------------------------------------------------------------------- // Task #57 — Confirm-new-game dialog tests // -----------------------------------------------------------------------