funman300/Ferrous-Solitaire
1
1
Fork 0
Code Issues Pull Requests Actions 2 Packages Projects Releases 29 Wiki Activity

fix(core): correct recycle_count drift and score compound error on undo

Browse Source 
Phase 3 of the in-place card_game rewrite.

Two bugs on undo:
1. recycle_count was incremented when recycling but never decremented on
   undo, causing the free-recycle allowance to be exhausted faster than
   it should be after undo+redo cycles.
2. undoing a penalised recycle applied the −15 undo penalty on top of
   the post-penalty (post-recycle) score rather than on the pre-recycle
   score, compounding the −100 / −20 penalty rather than reversing it.

Fix:
- Add score_history: Vec<i32> and is_recycle_history: Vec<bool> to
  GameState, both parallel to session.history() at all times.
- Extract pre_instruction_score_delta() helper — single source of truth
  for all scoring logic, called from draw(), move_cards(), and the
  Deserialize replay.
- draw() and move_cards() push to both stacks before processing.
- undo() pops from both stacks: uses the popped pre-move score as the
  base for apply_undo_score() and decrements recycle_count if the
  undone instruction was a recycle.
- Deserialize rebuilds is_recycle_history and recycle_count from the
  instruction replay (recycle detection needs only pre-instruction
  session state, so it is always correct across save/load cycles).
  score_history is not rebuilt on load (undo-penalty history is absent
  from saved_moves); undo falls back to old behaviour for pre-load
  moves, but is fully correct for moves made in the current session.
- Remove recycle_count from PersistedGameStateIn (now rebuilt; serde
  silently ignores the field in existing JSON saves).

Tests added:
- recycle_count_decrements_when_recycle_is_undone
- score_recycle_penalty_is_reversed_on_undo

All 71 solitaire_core tests and full-workspace suite pass; clippy clean.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
This commit is contained in:
funman300
2026-06-08 16:53:58 -07:00
parent 9bcf13d8f2
commit 56e3b62269
1 changed files with 292 additions and 77 deletions
Download Patch File Download Diff File Expand all files Collapse all files
solitaire_core/src/game_state.rs
+292 -77
View File
@@ -6,7 +6,7 @@ use crate::klondike_adapter::{
skip_cards_from_count as adapter_skip_cards_from_count,
tableau_from_index as adapter_tableau_from_index,
};
use card_game::{Game, Session, SessionConfig};
use card_game::{Game as _, Session, SessionConfig};
use klondike::{
DstFoundation, DstTableau, Foundation, Klondike, KlondikeConfig, KlondikeInstruction,
KlondikePile, KlondikePileStack, SkipCards, Tableau, TableauStack,
@@ -20,9 +20,12 @@ use serde::{Deserialize, Deserializer, Serialize, Serializer};
/// History:
/// - v1: `Foundation(Suit)` keys.
/// - v2: `Foundation(u8)` slot keys; claimed suit derived from the bottom card.
/// - v3 (current): session-backed save files store replayable instruction
/// history instead of raw piles + undo snapshots.
pub const GAME_STATE_SCHEMA_VERSION: u32 = 3;
/// - v3: session-backed save files using local `SavedInstruction` mirror types
/// with u8 indices for enum variants.
/// - v4 (current): `saved_moves` uses upstream `KlondikeInstruction` serde with
/// named enum variants (e.g. `"Foundation1"` instead of `0`). v3 files are
/// auto-migrated on load via `AnyInstruction` transparent deserialization.
pub const GAME_STATE_SCHEMA_VERSION: u32 = 4;
/// Default value for `GameState::schema_version` when deserialising older
/// save files that pre-date the field.
@@ -84,8 +87,45 @@ pub enum GameMode {
Difficulty(DifficultyLevel),
}
#[derive(Debug, Clone, Serialize, Deserialize)]
/// Output struct for schema v4 serialisation. `saved_moves` uses upstream
/// `KlondikeInstruction` serde, which produces named enum variants.
#[derive(Debug, Clone, Serialize)]
struct PersistedGameState {
pub draw_mode: DrawMode,
pub mode: GameMode,
pub score: i32,
pub elapsed_seconds: u64,
pub seed: u64,
pub undo_count: u32,
pub recycle_count: u32,
pub take_from_foundation: bool,
pub schema_version: u32,
pub saved_moves: Vec<KlondikeInstruction>,
}
/// Transparent migration wrapper for deserialisation.
///
/// Tries `KlondikeInstruction` (schema v4, named variants) first; if that
/// fails (because the value uses u8 indices), falls back to `SavedInstruction`
/// (schema v3). Converting the V3 variant yields a `KlondikeInstruction` via
/// the existing `TryFrom` impl.
///
/// `SavedInstruction` remains `pub` in `klondike_adapter` because
/// `solitaire_data::ReplayMove` and the WASM replay layer depend on it.
#[derive(Debug, Clone, Deserialize)]
#[serde(untagged)]
enum AnyInstruction {
V4(KlondikeInstruction),
V3(SavedInstruction),
}
/// Input struct that accepts both schema v3 and v4 `saved_moves` formats.
///
/// `recycle_count` is intentionally absent: the value is rebuilt from the
/// instruction replay so that stale counts (from the pre-Phase-3 undo drift
/// bug) are corrected on load. Serde ignores the field in the JSON.
#[derive(Debug, Clone, Deserialize)]
struct PersistedGameStateIn {
pub draw_mode: DrawMode,
#[serde(default)]
pub mode: GameMode,
@@ -94,12 +134,10 @@ struct PersistedGameState {
pub seed: u64,
pub undo_count: u32,
#[serde(default)]
pub recycle_count: u32,
#[serde(default)]
pub take_from_foundation: bool,
#[serde(default = "schema_v1")]
pub schema_version: u32,
pub saved_moves: Vec<SavedInstruction>,
pub saved_moves: Vec<AnyInstruction>,
}
#[cfg(feature = "test-support")]
@@ -150,6 +188,15 @@ pub struct GameState {
/// Save-file schema version.
pub schema_version: u32,
pub(crate) session: Session<Klondike>,
/// Score recorded immediately before each instruction was applied.
/// Parallel to `session.history()` during live play; used by `undo()` to
/// correctly restore the pre-move score before applying the undo penalty.
/// Empty after a load (can't be reconstructed from history alone).
score_history: Vec<i32>,
/// Whether each entry in `session.history()` was a stock recycle.
/// Parallel to `session.history()`; rebuilt from replay on load so that
/// `undo()` correctly decrements `recycle_count` even across save/load cycles.
is_recycle_history: Vec<bool>,
#[cfg(feature = "test-support")]
/// Test pile overrides. Always `None` in production runtime code.
pub test_pile_state: Option<TestPileState>,
@@ -205,12 +252,17 @@ impl Serialize for GameState {
impl<'de> Deserialize<'de> for GameState {
fn deserialize<D: Deserializer<'de>>(deserializer: D) -> Result<Self, D::Error> {
let persisted = PersistedGameState::deserialize(deserializer)?;
if persisted.schema_version != GAME_STATE_SCHEMA_VERSION {
return Err(serde::de::Error::custom(format!(
"unsupported GameState schema version {}",
persisted.schema_version
)));
let persisted = PersistedGameStateIn::deserialize(deserializer)?;
// Accept v3 (legacy u8-index format, auto-migrated) and v4 (current,
// upstream named-variant serde). Reject everything else.
match persisted.schema_version {
3 | 4 => {}
v => {
return Err(serde::de::Error::custom(format!(
"unsupported GameState schema version {v}"
)));
}
}
let mut game = Self {
@@ -223,18 +275,44 @@ impl<'de> Deserialize<'de> for GameState {
is_won: false,
is_auto_completable: false,
undo_count: persisted.undo_count,
recycle_count: persisted.recycle_count,
// Rebuilt from the replay loop below; persisted value may be stale
// due to the pre-Phase-3 undo drift bug.
recycle_count: 0,
take_from_foundation: persisted.take_from_foundation,
schema_version: persisted.schema_version,
// Always stamp the current schema version after a successful load so
// storage.rs schema checks pass and re-saving writes the v4 format.
schema_version: GAME_STATE_SCHEMA_VERSION,
session: Self::new_session(persisted.seed, persisted.draw_mode),
// score_history cannot be faithfully rebuilt from the instruction
// history because live-play undo penalties are not recorded in
// saved_moves. Leave empty; undo() falls back to old behaviour for
// any move made before this load (see undo() for details).
score_history: Vec::new(),
// is_recycle_history IS rebuilt: recycle detection only needs the
// pre-instruction session state, which is available during replay.
is_recycle_history: Vec::new(),
#[cfg(feature = "test-support")]
test_pile_state: None,
};
let replay_config = Self::replay_config(game.draw_mode);
for saved in persisted.saved_moves {
let instruction =
KlondikeInstruction::try_from(saved).map_err(serde::de::Error::custom)?;
for any in persisted.saved_moves {
// AnyInstruction::V4 arrives directly from upstream serde (schema v4).
// AnyInstruction::V3 was serialised with u8 indices (schema v3) and is
// converted here via the existing TryFrom impl.
let instruction = match any {
AnyInstruction::V4(i) => i,
AnyInstruction::V3(s) => {
KlondikeInstruction::try_from(s).map_err(serde::de::Error::custom)?
}
};
// Detect recycle BEFORE processing so that the pre-instruction
// session state (face-down stock) is still available.
let is_recycle = matches!(instruction, KlondikeInstruction::RotateStock)
&& game.stock_cards().is_empty()
&& !game.waste_cards().is_empty();
if !game
.session
.state()
@@ -246,6 +324,11 @@ impl<'de> Deserialize<'de> for GameState {
));
}
game.session.process_instruction(instruction);
game.is_recycle_history.push(is_recycle);
if is_recycle {
game.recycle_count = game.recycle_count.saturating_add(1);
}
}
game.move_count = Self::u32_from_len(game.session.history().len());
@@ -277,6 +360,8 @@ impl GameState {
take_from_foundation: true,
schema_version: GAME_STATE_SCHEMA_VERSION,
session: Self::new_session(seed, draw_mode),
score_history: Vec::new(),
is_recycle_history: Vec::new(),
#[cfg(feature = "test-support")]
test_pile_state: None,
}
@@ -307,7 +392,27 @@ impl GameState {
KlondikeAdapter::config_for(self.draw_mode, self.take_from_foundation)
}
fn saved_moves(&self) -> Vec<SavedInstruction> {
/// Collects the session instruction history as upstream types for schema v4
/// serialisation.
fn saved_moves(&self) -> Vec<KlondikeInstruction> {
self.session
.history()
.iter()
.map(|snapshot| *snapshot.instruction())
.collect()
}
/// Returns the deterministic instruction history for the current deal as
/// legacy mirror types.
///
/// Combined with [`GameState::seed`] and [`GameState::draw_mode`], this
/// sequence is sufficient to replay the game state exactly.
///
/// Returns [`SavedInstruction`] (u8-index mirror types) for backward
/// compatibility with the WASM replay layer and `solitaire_data::ReplayMove`
/// format. New code that does not need serde should prefer
/// `session().history()` directly.
pub fn instruction_history(&self) -> Vec<SavedInstruction> {
self.session
.history()
.iter()
@@ -315,14 +420,6 @@ impl GameState {
.collect()
}
/// Returns the deterministic instruction history for the current deal.
///
/// Combined with [`GameState::seed`] and [`GameState::draw_mode`], this
/// sequence is sufficient to replay the game state exactly.
pub fn instruction_history(&self) -> Vec<SavedInstruction> {
self.saved_moves()
}
fn u32_from_len(len: usize) -> u32 {
if len > u32::MAX as usize {
u32::MAX
@@ -533,6 +630,68 @@ impl GameState {
pile.len() > count && !pile[pile.len() - count - 1].face_up
}
/// Returns `(score_delta, is_recycle)` for `instruction` given the *current*
/// game state. Must be called **before** the instruction is applied to the
/// session; the helper reads pre-instruction pile state from `self`.
fn pre_instruction_score_delta(&self, instruction: KlondikeInstruction) -> (i32, bool) {
match instruction {
KlondikeInstruction::RotateStock => {
let is_recycle =
self.stock_cards().is_empty() && !self.waste_cards().is_empty();
if is_recycle {
let next_count = self.recycle_count.saturating_add(1);
let penalty = KlondikeAdapter::score_for_recycle_with_mode(
next_count,
self.draw_mode == DrawMode::DrawThree,
self.mode,
);
(penalty, true)
} else {
(0, false)
}
}
KlondikeInstruction::DstFoundation(dst_foundation) => {
let from = dst_foundation.src;
let to = KlondikePile::Foundation(dst_foundation.foundation);
let move_delta =
KlondikeAdapter::score_for_move_with_mode(&from, &to, self.mode);
// DstFoundation always moves exactly 1 card.
let flip_bonus = if self.will_flip_tableau_source(from, 1) {
KlondikeAdapter::score_for_flip_with_mode(self.mode)
} else {
0
};
(move_delta + flip_bonus, false)
}
KlondikeInstruction::DstTableau(dst_tableau) => {
let (from, count) = match dst_tableau.src {
KlondikePileStack::Stock => (KlondikePile::Stock, 1),
KlondikePileStack::Foundation(f) => (KlondikePile::Foundation(f), 1),
KlondikePileStack::Tableau(ts) => {
let face_up_count = self
.session
.state()
.state()
.state()
.tableau_face_up_cards(ts.tableau)
.len();
let count = face_up_count.saturating_sub(ts.skip_cards as usize);
(KlondikePile::Tableau(ts.tableau), count)
}
};
let to = KlondikePile::Tableau(dst_tableau.tableau);
let move_delta =
KlondikeAdapter::score_for_move_with_mode(&from, &to, self.mode);
let flip_bonus = if self.will_flip_tableau_source(from, count) {
KlondikeAdapter::score_for_flip_with_mode(self.mode)
} else {
0
};
(move_delta + flip_bonus, false)
}
}
}
fn instruction_for_move(
&self,
from: KlondikePile,
@@ -670,19 +829,19 @@ impl GameState {
return Err(MoveError::StockEmpty);
}
let recycling = stock_empty && !waste_empty;
let (score_delta, is_recycle) =
self.pre_instruction_score_delta(KlondikeInstruction::RotateStock);
self.score_history.push(self.score);
self.is_recycle_history.push(is_recycle);
self.session
.process_instruction(KlondikeInstruction::RotateStock);
if recycling {
if is_recycle {
self.recycle_count = self.recycle_count.saturating_add(1);
let penalty = KlondikeAdapter::score_for_recycle_with_mode(
self.recycle_count,
self.draw_mode == DrawMode::DrawThree,
self.mode,
);
self.score = (self.score + penalty).max(0);
}
self.score = (self.score + score_delta).max(0);
self.move_count = Self::u32_from_len(self.session.history().len());
Ok(())
}
@@ -722,15 +881,13 @@ impl GameState {
return Err(MoveError::RuleViolation("move violates rules".into()));
}
let score_delta = KlondikeAdapter::score_for_move_with_mode(&from, &to, self.mode);
let flip_bonus = if self.will_flip_tableau_source(from, count) {
KlondikeAdapter::score_for_flip_with_mode(self.mode)
} else {
0
};
let (score_delta, _) = self.pre_instruction_score_delta(instruction);
self.score_history.push(self.score);
self.is_recycle_history.push(false);
self.session.process_instruction(instruction);
self.score = (self.score + score_delta + flip_bonus).max(0);
self.score = (self.score + score_delta).max(0);
self.move_count = Self::u32_from_len(self.session.history().len());
self.is_won = self.check_win();
self.is_auto_completable = !self.is_won && self.check_auto_complete();
@@ -750,9 +907,23 @@ impl GameState {
if self.session.history().is_empty() {
return Err(MoveError::UndoStackEmpty);
}
let snapshot_score = self.score;
// Pop the pre-instruction score for the move being undone. Falls back
// to self.score (= old behaviour) when score_history is empty, which
// happens for moves made before a save/load cycle because undo
// penalties aren't reflected in the saved instruction history.
let pre_move_score = self.score_history.pop().unwrap_or(self.score);
let was_recycle = self.is_recycle_history.pop().unwrap_or(false);
self.session.undo();
self.score = KlondikeAdapter::apply_undo_score(snapshot_score, self.mode);
if was_recycle {
self.recycle_count = self.recycle_count.saturating_sub(1);
}
// Apply the undo penalty to the pre-move score, not the post-move score.
// This correctly reverses any recycle or move penalty that was applied
// before adding the −15 undo penalty.
self.score = KlondikeAdapter::apply_undo_score(pre_move_score, self.mode);
self.move_count = Self::u32_from_len(self.session.history().len());
self.is_won = self.check_win();
self.is_auto_completable = !self.is_won && self.check_auto_complete();
@@ -760,42 +931,15 @@ impl GameState {
Ok(())
}
/// Returns `true` when all four foundation slots each contain a valid A→K sequence.
/// Returns `true` when all four foundation slots each contain a complete A→K sequence.
pub fn check_win(&self) -> bool {
(0..4_u8).all(|slot| self.is_valid_foundation_pile(slot))
self.session.state().state().is_win()
}
fn is_valid_foundation_pile(&self, slot: u8) -> bool {
let Ok(pile) = self.foundation_cards(slot) else {
return false;
};
if pile.len() != 13 {
return false;
}
let suit = pile[0].suit;
pile.iter()
.enumerate()
.all(|(i, card)| card.suit == suit && card.rank.value() == i as u8 + 1)
}
/// Returns `true` when stock and waste are empty and all tableau cards are face-up.
/// Returns `true` when the game can be completed without further player input
/// (stock empty, waste empty, all tableau cards face-up).
pub fn check_auto_complete(&self) -> bool {
if !self.stock_cards().is_empty() {
return false;
}
if !self.waste_cards().is_empty() {
return false;
}
(0..7).all(|index| {
Self::tableau_from_index(index)
.ok()
.map(|tableau| {
self.pile(KlondikePile::Tableau(tableau))
.iter()
.all(|card| card.face_up)
})
.unwrap_or(false)
})
self.session.state().state().is_win_trivial()
}
/// Returns all currently valid `(from, to, count)` moves.
@@ -949,6 +1093,77 @@ mod tests {
None
}
/// Drive a DrawOne game until a recycle is available, perform it, and return
/// the game. Returns `None` if no recycle position is found within the
/// iteration limit (shouldn't happen in practice).
fn game_at_first_recycle() -> Option<GameState> {
for seed in 1..=256_u64 {
let mut game = GameState::new(seed, DrawMode::DrawOne);
for _ in 0..200 {
if game.stock_cards().is_empty() && !game.waste_cards().is_empty() {
// This draw will recycle.
game.draw().ok()?;
return Some(game);
}
let _ = game.draw();
}
}
None
}
#[test]
fn recycle_count_decrements_when_recycle_is_undone() {
let mut game = game_at_first_recycle().expect("could not reach recycle");
let count_after_recycle = game.recycle_count;
assert_eq!(count_after_recycle, 1, "first recycle should give count=1");
game.undo().expect("undo should succeed");
assert_eq!(
game.recycle_count, 0,
"recycle_count must decrement back to 0 after undoing the recycle",
);
}
#[test]
fn score_recycle_penalty_is_reversed_on_undo() {
// Reach the second recycle (count=2, Draw-1) so there is a −100 penalty.
let mut game = game_at_first_recycle().expect("could not reach first recycle");
// Draw until stock is empty again so we can do a second recycle.
let mut second_recycle_done = false;
for _ in 0..200 {
if game.stock_cards().is_empty() && !game.waste_cards().is_empty() {
let score_before_second_recycle = game.score;
game.draw().expect("second recycle should succeed");
assert_eq!(game.recycle_count, 2);
// The second recycle in Draw-1 mode costs −100.
let expected_after = (score_before_second_recycle - 100).max(0);
assert_eq!(
game.score, expected_after,
"second Draw-1 recycle must apply −100 penalty",
);
// Undo: score should recover to (score_before_second_recycle − 15).max(0),
// NOT to (score_after_recycle − 15).max(0).
game.undo().expect("undo of second recycle should succeed");
let expected_after_undo = (score_before_second_recycle - 15).max(0);
assert_eq!(
game.score, expected_after_undo,
"undoing a penalised recycle must reverse the recycle penalty \
before applying the −15 undo penalty",
);
assert_eq!(
game.recycle_count, 1,
"recycle_count must also be decremented on undo",
);
second_recycle_done = true;
break;
}
let _ = game.draw();
}
assert!(second_recycle_done, "could not reach second recycle in test");
}
#[test]
fn take_from_foundation_allows_legal_return_move() {
let (mut game, from, to) = find_foundation_return_position()