From 456b4d42e33946dccf9a05d9fb399e15b4ddec97 Mon Sep 17 00:00:00 2001 From: funman300 Date: Mon, 18 May 2026 13:25:13 -0700 Subject: [PATCH] refactor(core): explicit Rank discriminants, checked arithmetic, possible_instructions - Add Rank=1..13 explicit discriminants so `rank as u8 == rank.value()`; collapse 13-arm value() match to `self as u8` - Add Rank::RANKS and Suit::SUITS iteration constants - Add Rank::checked_add / checked_sub (const fn, type-safe boundary enforcement); update rules.rs to use them - Add GameState::possible_instructions() enumerating all valid move_cards triples (foundation for hints/solver) - Fix waste buffer card peeking through during draw-slide animation by setting Visibility::Hidden on the buffer entity in sync_cards Co-Authored-By: Claude Sonnet 4.6 --- solitaire_core/src/card.rs | 121 +++++++++++++++------ solitaire_core/src/game_state.rs | 159 ++++++++++++++++++++++++++++ solitaire_core/src/rules.rs | 12 +-- solitaire_engine/src/card_plugin.rs | 66 +++++++++++- 4 files changed, 314 insertions(+), 44 deletions(-) diff --git a/solitaire_core/src/card.rs b/solitaire_core/src/card.rs index 2b11a5c..ad7bb75 100644 --- a/solitaire_core/src/card.rs +++ b/solitaire_core/src/card.rs @@ -10,6 +10,9 @@ pub enum Suit { } impl Suit { + /// All four suits in declaration order. + pub const SUITS: [Self; 4] = [Self::Clubs, Self::Diamonds, Self::Hearts, Self::Spades]; + /// Returns `true` for red suits (Diamonds, Hearts). pub fn is_red(self) -> bool { matches!(self, Suit::Diamonds | Suit::Hearts) @@ -24,38 +27,63 @@ impl Suit { /// Card rank, Ace through King. #[derive(Debug, Clone, Copy, PartialEq, Eq, Hash, Serialize, Deserialize)] pub enum Rank { - Ace, - Two, - Three, - Four, - Five, - Six, - Seven, - Eight, - Nine, - Ten, - Jack, - Queen, - King, + Ace = 1, + Two = 2, + Three = 3, + Four = 4, + Five = 5, + Six = 6, + Seven = 7, + Eight = 8, + Nine = 9, + Ten = 10, + Jack = 11, + Queen = 12, + King = 13, } impl Rank { + /// All thirteen ranks in ascending order. + pub const RANKS: [Self; 13] = [ + Self::Ace, Self::Two, Self::Three, Self::Four, Self::Five, + Self::Six, Self::Seven, Self::Eight, Self::Nine, Self::Ten, + Self::Jack, Self::Queen, Self::King, + ]; + /// Numeric value: Ace = 1, King = 13. pub fn value(self) -> u8 { - match self { - Rank::Ace => 1, - Rank::Two => 2, - Rank::Three => 3, - Rank::Four => 4, - Rank::Five => 5, - Rank::Six => 6, - Rank::Seven => 7, - Rank::Eight => 8, - Rank::Nine => 9, - Rank::Ten => 10, - Rank::Jack => 11, - Rank::Queen => 12, - Rank::King => 13, + self as u8 + } + + const fn new(n: u8) -> Option { + match n { + 1 => Some(Self::Ace), + 2 => Some(Self::Two), + 3 => Some(Self::Three), + 4 => Some(Self::Four), + 5 => Some(Self::Five), + 6 => Some(Self::Six), + 7 => Some(Self::Seven), + 8 => Some(Self::Eight), + 9 => Some(Self::Nine), + 10 => Some(Self::Ten), + 11 => Some(Self::Jack), + 12 => Some(Self::Queen), + 13 => Some(Self::King), + _ => None, + } + } + + /// Returns the rank `n` steps above `self`, or `None` if it would exceed King. + pub const fn checked_add(self, n: u8) -> Option { + Self::new((self as u8).saturating_add(n)) + } + + /// Returns the rank `n` steps below `self`, or `None` if it would go below Ace. + pub const fn checked_sub(self, n: u8) -> Option { + match (self as u8).checked_sub(n) { + Some(v) => Self::new(v), + None => None, } } } @@ -79,16 +107,43 @@ mod tests { #[test] fn rank_values_are_sequential() { - let ranks = [ - Rank::Ace, Rank::Two, Rank::Three, Rank::Four, Rank::Five, - Rank::Six, Rank::Seven, Rank::Eight, Rank::Nine, Rank::Ten, - Rank::Jack, Rank::Queen, Rank::King, - ]; - for (i, r) in ranks.iter().enumerate() { + for (i, r) in Rank::RANKS.iter().enumerate() { assert_eq!(r.value(), (i + 1) as u8); } } + #[test] + fn rank_as_u8_matches_value() { + for r in Rank::RANKS { + assert_eq!(r as u8, r.value()); + } + } + + #[test] + fn rank_checked_add_boundary() { + assert_eq!(Rank::King.checked_add(1), None); + assert_eq!(Rank::Queen.checked_add(1), Some(Rank::King)); + assert_eq!(Rank::Ace.checked_add(1), Some(Rank::Two)); + assert_eq!(Rank::Five.checked_add(3), Some(Rank::Eight)); + } + + #[test] + fn rank_checked_sub_boundary() { + assert_eq!(Rank::Ace.checked_sub(1), None); + assert_eq!(Rank::Two.checked_sub(1), Some(Rank::Ace)); + assert_eq!(Rank::King.checked_sub(1), Some(Rank::Queen)); + assert_eq!(Rank::Five.checked_sub(3), Some(Rank::Two)); + } + + #[test] + fn suit_suits_contains_all_four() { + assert_eq!(Suit::SUITS.len(), 4); + assert!(Suit::SUITS.contains(&Suit::Clubs)); + assert!(Suit::SUITS.contains(&Suit::Diamonds)); + assert!(Suit::SUITS.contains(&Suit::Hearts)); + assert!(Suit::SUITS.contains(&Suit::Spades)); + } + #[test] fn suit_red_and_black_are_complementary() { for suit in [Suit::Clubs, Suit::Diamonds, Suit::Hearts, Suit::Spades] { diff --git a/solitaire_core/src/game_state.rs b/solitaire_core/src/game_state.rs index 4ba0bcf..d908794 100644 --- a/solitaire_core/src/game_state.rs +++ b/solitaire_core/src/game_state.rs @@ -440,6 +440,91 @@ impl GameState { }) } + /// Returns all currently valid `move_cards` calls as `(from, to, count)` triples. + /// + /// Does not include stock draws — callers check `piles[&PileType::Stock]` directly. + /// Every returned triple is guaranteed to succeed when passed to `move_cards`. + pub fn possible_instructions(&self) -> Vec<(PileType, PileType, usize)> { + if self.is_won { + return Vec::new(); + } + + let mut moves = Vec::new(); + + // Waste top card → foundation or tableau + if let Some(waste_top) = self.piles.get(&PileType::Waste).and_then(|p| p.cards.last()) { + for slot in 0..4_u8 { + if let Some(f) = self.piles.get(&PileType::Foundation(slot)) + && can_place_on_foundation(waste_top, f) + { + moves.push((PileType::Waste, PileType::Foundation(slot), 1)); + } + } + for dst in 0..7_usize { + if let Some(t) = self.piles.get(&PileType::Tableau(dst)) + && can_place_on_tableau(waste_top, t) + { + moves.push((PileType::Waste, PileType::Tableau(dst), 1)); + } + } + } + + // Tableau sources + for src in 0..7_usize { + let Some(src_pile) = self.piles.get(&PileType::Tableau(src)) else { continue }; + if src_pile.cards.is_empty() { + continue; + } + let run_len = src_pile.cards.iter().rev().take_while(|c| c.face_up).count(); + if run_len == 0 { + continue; + } + for count in 1..=run_len { + let seq_start = src_pile.cards.len() - count; + if !is_valid_tableau_sequence(&src_pile.cards[seq_start..]) { + continue; + } + let bottom = &src_pile.cards[seq_start]; + if count == 1 { + for slot in 0..4_u8 { + if let Some(f) = self.piles.get(&PileType::Foundation(slot)) + && can_place_on_foundation(bottom, f) + { + moves.push((PileType::Tableau(src), PileType::Foundation(slot), 1)); + } + } + } + for dst in 0..7_usize { + if dst == src { + continue; + } + if let Some(t) = self.piles.get(&PileType::Tableau(dst)) + && can_place_on_tableau(bottom, t) + { + moves.push((PileType::Tableau(src), PileType::Tableau(dst), count)); + } + } + } + } + + // Foundation top → tableau (only when house rule is enabled) + if self.take_from_foundation { + for slot in 0..4_u8 { + let Some(f) = self.piles.get(&PileType::Foundation(slot)) else { continue }; + let Some(top) = f.cards.last() else { continue }; + for dst in 0..7_usize { + if let Some(t) = self.piles.get(&PileType::Tableau(dst)) + && can_place_on_tableau(top, t) + { + moves.push((PileType::Foundation(slot), PileType::Tableau(dst), 1)); + } + } + } + } + + moves + } + /// Returns the next `(from, to)` move that advances auto-complete, or /// `None` if no such move exists (or `is_auto_completable` is not set). /// @@ -1366,4 +1451,78 @@ mod tests { .unwrap_err(); assert!(matches!(err, MoveError::RuleViolation(_))); } + + // --- possible_instructions --- + + #[test] + fn possible_instructions_empty_when_won() { + let mut g = new_game(); + g.is_won = true; + assert!(g.possible_instructions().is_empty()); + } + + #[test] + fn possible_instructions_includes_ace_to_foundation() { + let mut g = new_game(); + g.piles.get_mut(&PileType::Stock).unwrap().cards.clear(); + g.piles.get_mut(&PileType::Waste).unwrap().cards.clear(); + for i in 0..7 { + g.piles.get_mut(&PileType::Tableau(i)).unwrap().cards.clear(); + } + g.piles.get_mut(&PileType::Tableau(0)).unwrap().cards.push(Card { + id: 1, suit: Suit::Clubs, rank: Rank::Ace, face_up: true, + }); + let moves = g.possible_instructions(); + assert!( + moves.contains(&(PileType::Tableau(0), PileType::Foundation(0), 1)), + "Ace must be moveable to empty foundation slot 0; got {moves:?}" + ); + } + + #[test] + fn possible_instructions_all_valid_on_fresh_game() { + // Every triple returned must actually succeed when applied to a clone of the state. + let g = new_game(); + for (from, to, count) in g.possible_instructions() { + let mut clone = g.clone(); + assert!( + clone.move_cards(from.clone(), to.clone(), count).is_ok(), + "instruction ({from:?}, {to:?}, {count}) from possible_instructions must succeed" + ); + } + } + + #[test] + fn possible_instructions_no_face_down_sources() { + let g = new_game(); + for (from, _, count) in g.possible_instructions() { + if let PileType::Tableau(i) = from { + let pile = &g.piles[&PileType::Tableau(i)]; + let run_len = pile.cards.iter().rev().take_while(|c| c.face_up).count(); + assert!( + count <= run_len, + "count {count} exceeds face-up run {run_len} for Tableau({i})" + ); + } + } + } + + #[test] + fn possible_instructions_waste_top_included() { + let mut g = new_game(); + // Clear board, put a King on waste, and an empty tableau pile — waste→tableau must appear. + g.piles.get_mut(&PileType::Stock).unwrap().cards.clear(); + for i in 0..7 { + g.piles.get_mut(&PileType::Tableau(i)).unwrap().cards.clear(); + } + g.piles.get_mut(&PileType::Waste).unwrap().cards.push(Card { + id: 99, suit: Suit::Spades, rank: Rank::King, face_up: true, + }); + let moves = g.possible_instructions(); + // King goes on any of the 7 empty tableau piles + assert!( + (0..7).any(|dst| moves.contains(&(PileType::Waste, PileType::Tableau(dst), 1))), + "King on waste must be moveable to an empty tableau column" + ); + } } diff --git a/solitaire_core/src/rules.rs b/solitaire_core/src/rules.rs index 9073844..174ac85 100644 --- a/solitaire_core/src/rules.rs +++ b/solitaire_core/src/rules.rs @@ -1,4 +1,4 @@ -use crate::card::Card; +use crate::card::{Card, Rank}; use crate::pile::Pile; /// Returns `true` if `card` can be placed on the foundation `pile`. @@ -12,8 +12,8 @@ use crate::pile::Pile; #[must_use] pub fn can_place_on_foundation(card: &Card, pile: &Pile) -> bool { match pile.cards.last() { - None => card.rank.value() == 1, - Some(top) => card.suit == top.suit && card.rank.value() == top.rank.value() + 1, + None => card.rank == Rank::Ace, + Some(top) => card.suit == top.suit && card.rank.checked_sub(1) == Some(top.rank), } } @@ -23,10 +23,10 @@ pub fn can_place_on_foundation(card: &Card, pile: &Pile) -> bool { #[must_use] pub fn can_place_on_tableau(card: &Card, pile: &Pile) -> bool { match pile.cards.last() { - None => card.rank.value() == 13, + None => card.rank == Rank::King, Some(top) => { top.face_up - && card.rank.value() + 1 == top.rank.value() + && card.rank.checked_add(1) == Some(top.rank) && card.suit.is_red() != top.suit.is_red() } } @@ -41,7 +41,7 @@ pub fn can_place_on_tableau(card: &Card, pile: &Pile) -> bool { #[must_use] pub fn is_valid_tableau_sequence(cards: &[Card]) -> bool { cards.windows(2).all(|w| { - w[0].rank.value() == w[1].rank.value() + 1 && w[0].suit.is_red() != w[1].suit.is_red() + w[0].rank.checked_sub(1) == Some(w[1].rank) && w[0].suit.is_red() != w[1].suit.is_red() }) } diff --git a/solitaire_engine/src/card_plugin.rs b/solitaire_engine/src/card_plugin.rs index 1b1c53d..e78bea8 100644 --- a/solitaire_engine/src/card_plugin.rs +++ b/solitaire_engine/src/card_plugin.rs @@ -691,6 +691,22 @@ fn sync_cards( ) { let positions = card_positions(game, layout); + // The waste buffer card exists only to keep its entity alive while the new + // top card's slide animation plays — it must never be visible to the player. + // Without this, the buffer sits at waste_base uncovered during the animation + // and its rank/suit peek behind the incoming card. + let waste_buffer_id: Option = { + let visible = match game.draw_mode { + DrawMode::DrawOne => 1_usize, + DrawMode::DrawThree => 3_usize, + }; + game.piles + .get(&PileType::Waste) + .filter(|w| w.cards.len() > visible) + .and_then(|w| w.cards.get(w.cards.len().saturating_sub(visible + 1))) + .map(|c| c.id) + }; + // Map card_id -> (Entity, current_translation, has_card_animation) for // in-place updates. The `has_card_animation` flag lets `update_card_entity` // skip the snap/slide path on cards that are already being driven by a @@ -711,17 +727,26 @@ fn sync_cards( } } - // For each card in the current state: spawn or update its entity. + // For each card in the current state: spawn or update its entity, then + // apply visibility. The waste buffer card is hidden so it cannot peek + // behind the incoming top card during the draw slide animation. for (card, position, z) in positions { - match existing.get(&card.id) { + let entity = match existing.get(&card.id) { Some(&(entity, cur, has_anim)) => { update_card_entity( &mut commands, entity, card, position, z, layout, slide_secs, back_colour, color_blind, high_contrast, cur, has_anim, card_images, selected_back, font_handle, - ) + ); + entity } None => spawn_card_entity(&mut commands, card, position, z, layout, back_colour, color_blind, high_contrast, card_images, selected_back, font_handle), - } + }; + let visibility = if waste_buffer_id == Some(card.id) { + Visibility::Hidden + } else { + Visibility::Inherited + }; + commands.entity(entity).insert(visibility); } } @@ -831,7 +856,7 @@ fn spawn_card_entity( card_images: Option<&CardImageSet>, selected_back: usize, font_handle: Option<&Handle>, -) { +) -> Entity { let sprite = card_sprite(card, layout.card_size, back_colour, card_images, selected_back); let mut entity = commands.spawn(( @@ -840,6 +865,7 @@ fn spawn_card_entity( Transform::from_xyz(pos.x, pos.y, z), Visibility::default(), )); + let entity_id = entity.id(); // Every card gets a subtle drop-shadow child so the play surface reads // as physical instead of flat. Spawned in idle state; the drag-tracking // system retunes its offset / alpha when this card joins the dragged @@ -880,6 +906,7 @@ fn spawn_card_entity( // Suppress unused-variable warning when not building for Android. #[cfg(not(target_os = "android"))] let _ = font_handle; + entity_id } #[allow(clippy::too_many_arguments)] @@ -2357,6 +2384,35 @@ mod tests { } } + /// The waste buffer card (slot below top) must be at the *same* XY as the + /// top card so that hiding it (`Visibility::Hidden`) leaves no visible gap. + #[test] + fn waste_draw_one_buffer_card_at_same_xy_as_top() { + use solitaire_core::game_state::DrawMode; + let mut g = GameState::new(42, DrawMode::DrawOne); + // Draw 3 times so the waste pile has 3 cards and the buffer exists. + for _ in 0..3 { + let _ = g.draw(); + } + let waste_ids: std::collections::HashSet = + g.piles[&PileType::Waste].cards.iter().map(|c| c.id).collect(); + let layout = crate::layout::compute_layout(Vec2::new(1280.0, 800.0), 0.0, 0.0, true); + let positions = card_positions(&g, &layout); + let waste_rendered: Vec<_> = positions + .iter() + .filter(|(card, _, _)| waste_ids.contains(&card.id)) + .collect(); + // Buffer (slot 0) + top (slot 1) = 2 rendered waste cards. + assert_eq!(waste_rendered.len(), 2, "Draw-One with 3 waste cards must render exactly 2"); + // Both must share the same XY so that hiding the buffer leaves no gap. + let (_, pos0, _) = waste_rendered[0]; + let (_, pos1, _) = waste_rendered[1]; + assert!( + (pos0.x - pos1.x).abs() < 1e-3 && (pos0.y - pos1.y).abs() < 1e-3, + "buffer and top card must be at the same XY; got buffer={pos0:?} top={pos1:?}" + ); + } + #[test] fn card_positions_tableau_cards_are_fanned_downward() { let g = GameState::new(42, solitaire_core::game_state::DrawMode::DrawOne);