From 9f755f2a623b80bfc97c2cd7061ee997c964b8d9 Mon Sep 17 00:00:00 2001 From: Jeroen Schweitzer Date: Wed, 22 Apr 2026 08:54:21 +0200 Subject: [PATCH] feat(simulation): persist SelectedBookmark across save/load (#863, #862) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Adds Serialize/Deserialize to SelectedBookmark and wires it into SaveStateV1 so a loaded game remembers which bookmark and starting location the player picked. Replaces the TODO at bookmark/mod.rs:95 (originally deferred to Sprint 37 alongside #614). Also refactors BookmarkPlugin to accept an injected BookmarkRegistry via BookmarkPlugin::new(registry) (#862). The Default constructor still wires the canonical tycoon registry — injection is for tests and future TOML loading. Flagged in PR #132 review as a follow-up. Updates bookmark spec §4.4 to remove the v0.2-deferred scope note. Co-Authored-By: Claude Opus 4.6 --- docs/architecture/sprint-36-bookmark-spec.md | 27 +++---- server/src/bookmark/mod.rs | 64 +++++++++++---- server/src/simulation/save_io.rs | 83 +++++++++++++++++++- server/src/simulation/save_state.rs | 62 +++++++++++++++ 4 files changed, 203 insertions(+), 33 deletions(-) diff --git a/docs/architecture/sprint-36-bookmark-spec.md b/docs/architecture/sprint-36-bookmark-spec.md index f36e639e8..e9f1ea2f7 100644 --- a/docs/architecture/sprint-36-bookmark-spec.md +++ b/docs/architecture/sprint-36-bookmark-spec.md @@ -213,15 +213,12 @@ Note: `ConfirmBookmark` is the **trigger** for transitioning from the character- ```rust /// The confirmed bookmark selection for the current session. -/// Populated when `ConfirmBookmark` is processed. `None` during the -/// character-creation phase (before confirm) and always `None` in a -/// fresh session. +/// Populated when `ConfirmBookmark` is processed. `None` fields during the +/// character-creation phase (before confirm) and in a fresh session. /// -/// **v0.2 scope: transient only.** Not serialized — save/load of -/// `SelectedBookmark` is deferred to Sprint 37 (follow-up ticket -/// filed alongside #614). Add `Serialize`/`Deserialize` derives and -/// wire into `SaveState` when that ticket is claimed. -#[derive(Resource, Debug, Clone, Default)] +/// Serialized into `SaveStateV1.selected_bookmark` (#863) so that a loaded +/// game remembers which bookmark and starting location were chosen. +#[derive(Resource, Debug, Clone, Default, Serialize, Deserialize)] pub struct SelectedBookmark { pub bookmark_id: Option, pub starting_location_id: Option, @@ -231,15 +228,11 @@ pub struct SelectedBookmark { Downstream systems (apartment generator, skill seeder) read from this resource. -**Save/load scope (v0.2 deferred):** `SelectedBookmark` is transient for -v0.2 — it lives in-memory from `ConfirmBookmark` through session end and -is not persisted. A reload after quit returns the player to the -character-creation screen. Promotion to persistent state (adding -`Serialize`/`Deserialize` and threading into `SaveState` / #553) is -tracked in a follow-up ticket for Sprint 37. `SelectedBookmark` must -carry an inline `// TODO(sprint-37): serialize — see #` -comment in `server/src/bookmark/mod.rs` pointing at the follow-up so the -omission is greppable. +**Save/load scope (Sprint 37, #863):** `SelectedBookmark` is persisted into +`SaveStateV1.selected_bookmark`. After `load_from_file` completes, the resource +reflects the bookmark confirmed at session-start. Saves created before Sprint 37 +will deserialize the field as `SelectedBookmark::default()` (both fields `None`) +via `#[serde(default)]` on the `SaveStateV1` field. ## 5. Content source — how bookmarks get into the registry diff --git a/server/src/bookmark/mod.rs b/server/src/bookmark/mod.rs index 23e21b254..30aec9153 100644 --- a/server/src/bookmark/mod.rs +++ b/server/src/bookmark/mod.rs @@ -20,7 +20,11 @@ use crate::knowledge::{resolve_culture, CultureResolver}; /// /// Built once during `BookmarkPlugin::build`, read-only at runtime. /// `BTreeMap` for deterministic iteration (D-010 principle 4, D-041). -#[derive(Resource, Debug, Default)] +/// +/// `Clone` is required by `BookmarkPlugin::build` which moves the registry into +/// the Bevy `App` via `insert_resource` while retaining the value from `self` +/// (#862 injection pattern). +#[derive(Resource, Debug, Default, Clone)] pub struct BookmarkRegistry { entries: BTreeMap, } @@ -88,18 +92,15 @@ impl BookmarkRegistry { } /// The confirmed bookmark selection for the current session. -/// Populated when `ConfirmBookmark` is processed. `None` during the -/// character-creation phase (before confirm) and always `None` in a -/// fresh session. /// -/// **v0.2 scope: transient only.** Not serialized — save/load of -/// `SelectedBookmark` is deferred to Sprint 37 (follow-up ticket -/// filed alongside #614). Add `Serialize`/`Deserialize` derives and -/// wire into `SaveState` when that ticket is claimed. +/// Populated when `ConfirmBookmark` is processed. `None` fields during the +/// character-creation phase (before confirm) and in a fresh session. +/// +/// Serialized into `SaveStateV1.selected_bookmark` (#863) so that a loaded +/// game remembers which bookmark and starting location were chosen. /// /// Downstream systems (apartment generator, skill seeder) read from this resource. -// TODO(sprint-37): serialize — see #863 -#[derive(Resource, Debug, Clone, Default)] +#[derive(Resource, Debug, Clone, Default, serde::Serialize, serde::Deserialize)] pub struct SelectedBookmark { pub bookmark_id: Option, pub starting_location_id: Option, @@ -109,14 +110,47 @@ pub struct SelectedBookmark { /// /// Registers `BookmarkRegistry`, `SelectedBookmark`, and the startup system that /// stages the initial catalog in `SnapshotBuffer` for tick-0 delivery. -pub struct BookmarkPlugin; +/// +/// **Default construction** uses the canonical tycoon registry: +/// ```ignore +/// app.add_plugins(BookmarkPlugin::default()); +/// ``` +/// +/// **Injection** substitutes a custom registry (for tests and future TOML loading): +/// ```ignore +/// let mut registry = BookmarkRegistry::default(); +/// // populate ... +/// app.add_plugins(BookmarkPlugin::new(registry)); +/// ``` +pub struct BookmarkPlugin { + registry: BookmarkRegistry, +} + +impl BookmarkPlugin { + /// Create a plugin with an injected registry. + /// + /// Useful in tests (inject a minimal registry with fixed entries) and + /// for future TOML loading (caller constructs the registry from disk, + /// then hands it to the plugin). + pub fn new(registry: BookmarkRegistry) -> Self { + Self { registry } + } +} + +impl Default for BookmarkPlugin { + /// Default plugin pre-populates the canonical tycoon registry via + /// `register_default_bookmarks`. Equivalent to the v0.2 behaviour before + /// injection was introduced. + fn default() -> Self { + let mut registry = BookmarkRegistry::default(); + register_default_bookmarks(&mut registry); + Self { registry } + } +} impl Plugin for BookmarkPlugin { fn build(&self, app: &mut App) { - let mut registry = BookmarkRegistry::default(); - register_default_bookmarks(&mut registry); - - app.insert_resource(registry) + app.insert_resource(self.registry.clone()) .init_resource::() .add_systems(Startup, prime_initial_catalog); diff --git a/server/src/simulation/save_io.rs b/server/src/simulation/save_io.rs index f101421b2..26248ef22 100644 --- a/server/src/simulation/save_io.rs +++ b/server/src/simulation/save_io.rs @@ -14,6 +14,7 @@ use std::path::{Path, PathBuf}; use bevy_ecs::prelude::*; use thiserror::Error; +use crate::bookmark::SelectedBookmark; use crate::bridge::types::SaveLoadResultWire; use crate::bridge::types::SnapshotBuffer; use crate::knowledge::graph::KnowledgeGraph; @@ -160,6 +161,10 @@ pub fn save_to_file(path: &Path, world: &mut World) -> Result<(), SaveLoadError> last_activation_tick: world .get_resource::() .and_then(|a| a.last_activation_tick), + selected_bookmark: world + .get_resource::() + .cloned() + .unwrap_or_default(), }; let bytes = state @@ -285,6 +290,10 @@ pub fn load_from_file(path: &Path, world: &mut World) -> Result<(), SaveLoadErro last_activation_tick: state.last_activation_tick, }); + // Restore bookmark selection (#863) — the player's confirmed bookmark and + // starting location survive save/load so downstream systems stay consistent. + world.insert_resource(state.selected_bookmark); + // Reset event queues and transient buffers — prevent stale events/history // from the pre-load world leaking into the post-load simulation. world.insert_resource(ContaminationEventQueue::default()); @@ -415,8 +424,11 @@ mod tests { static COUNTER: AtomicU64 = AtomicU64::new(0); fn temp_path() -> PathBuf { + // Include the process ID so nextest processes (each starts COUNTER at 0) + // do not collide on the same filename when running concurrently. + let pid = std::process::id(); let id = COUNTER.fetch_add(1, Ordering::Relaxed); - std::env::temp_dir().join(format!("settled_reach_save_io_test_{}.msgpack", id)) + std::env::temp_dir().join(format!("settled_reach_save_io_test_{}_{}.msgpack", pid, id)) } fn minimal_world() -> World { @@ -623,6 +635,7 @@ mod tests { contamination_active: false, activated_count: 0, last_activation_tick: None, + selected_bookmark: SelectedBookmark::default(), }; let bytes = bad_state.to_bytes().expect("serialize"); let path = temp_path(); @@ -955,4 +968,72 @@ mod tests { let _ = std::fs::remove_file(&path); } + + // ----------------------------------------------------------------------- + // SelectedBookmark round-trip (#863) + // ----------------------------------------------------------------------- + + #[test] + fn save_load_preserves_selected_bookmark() { + let mut world = minimal_world(); + + // Set a bookmark selection before saving. + world.insert_resource(SelectedBookmark { + bookmark_id: Some("tycoon".to_string()), + starting_location_id: Some("new-stockholm".to_string()), + }); + + let path = temp_path(); + save_to_file(&path, &mut world).expect("save"); + + // Clear the resource to prove load restores it, not the pre-existing value. + world.insert_resource(SelectedBookmark::default()); + assert!( + world + .resource::() + .bookmark_id + .is_none(), + "bookmark must be cleared before load" + ); + + load_from_file(&path, &mut world).expect("load"); + + let restored = world.resource::(); + assert_eq!( + restored.bookmark_id.as_deref(), + Some("tycoon"), + "bookmark_id must survive round-trip" + ); + assert_eq!( + restored.starting_location_id.as_deref(), + Some("new-stockholm"), + "starting_location_id must survive round-trip" + ); + + let _ = std::fs::remove_file(&path); + } + + #[test] + fn save_load_selected_bookmark_defaults_when_unset() { + // Save without a bookmark selection (default = both None). + // Load must produce SelectedBookmark::default(), not error. + let mut world = minimal_world(); + // SelectedBookmark not explicitly inserted — should default to no selection. + + let path = temp_path(); + save_to_file(&path, &mut world).expect("save"); + load_from_file(&path, &mut world).expect("load"); + + let restored = world.resource::(); + assert!( + restored.bookmark_id.is_none(), + "unset bookmark_id must survive as None" + ); + assert!( + restored.starting_location_id.is_none(), + "unset starting_location_id must survive as None" + ); + + let _ = std::fs::remove_file(&path); + } } diff --git a/server/src/simulation/save_state.rs b/server/src/simulation/save_state.rs index 685c5c810..d6148a16f 100644 --- a/server/src/simulation/save_state.rs +++ b/server/src/simulation/save_state.rs @@ -51,6 +51,7 @@ use crate::npc::{ PersonalityTraits, Relationships, Secret, SecretSeverity, SkillSet, TellSystem, ToleranceThreshold, Want, WantKind, }; +use crate::bookmark::SelectedBookmark; use crate::simulation::modification::Modification; use crate::simulation::movement::TilePosition; use crate::simulation::time::TickRate; @@ -122,6 +123,11 @@ pub struct SaveStateV1 { /// `None` if no activation yet. Persisted alongside `activated_count`. #[serde(default)] pub last_activation_tick: Option, + /// Bookmark and starting location confirmed by the player at session start (#863, #614). + /// Both fields are `None` in saves created before #863 or before character creation + /// completes. Defaults to `SelectedBookmark::default()` for backward compatibility. + #[serde(default)] + pub selected_bookmark: SelectedBookmark, } /// Per-NPC state snapshot for `SaveStateV1`. @@ -435,6 +441,7 @@ mod tests { contamination_active: false, activated_count: 0, last_activation_tick: None, + selected_bookmark: crate::bookmark::SelectedBookmark::default(), } } @@ -854,6 +861,7 @@ mod tests { contamination_active: false, activated_count: 0, last_activation_tick: None, + selected_bookmark: crate::bookmark::SelectedBookmark::default(), }; let bytes = save.to_bytes().expect("serialize"); @@ -878,6 +886,60 @@ mod tests { assert!(result.is_err(), "must panic without StableEntityId"); } + // ----------------------------------------------------------------------- + // SelectedBookmark round-trip (#863) + // ----------------------------------------------------------------------- + + #[test] + fn selected_bookmark_with_values_survives_roundtrip() { + // Spec (#863): SelectedBookmark persisted in SaveStateV1 must survive a + // full MessagePack serialize → deserialize cycle with field values intact. + let mut state = minimal_save_state(); + state.selected_bookmark = crate::bookmark::SelectedBookmark { + bookmark_id: Some("tycoon".to_string()), + starting_location_id: Some("GJ 35".to_string()), + }; + + let bytes = state.to_bytes().expect("serialize"); + let recovered = SaveStateV1::from_bytes(&bytes).expect("deserialize"); + + assert_eq!( + recovered.selected_bookmark.bookmark_id, + Some("tycoon".to_string()), + "bookmark_id must survive roundtrip" + ); + assert_eq!( + recovered.selected_bookmark.starting_location_id, + Some("GJ 35".to_string()), + "starting_location_id must survive roundtrip" + ); + + // Idempotent re-serialize: bytes must be stable + let bytes2 = recovered.to_bytes().expect("re-serialize"); + assert_eq!(bytes, bytes2, "SelectedBookmark roundtrip must be idempotent"); + } + + #[test] + fn selected_bookmark_default_survives_roundtrip() { + // Spec (#863): saves from before Sprint 37 (both fields None) must load + // cleanly via serde(default) on the SaveStateV1 field. + let state = minimal_save_state(); + assert!(state.selected_bookmark.bookmark_id.is_none()); + assert!(state.selected_bookmark.starting_location_id.is_none()); + + let bytes = state.to_bytes().expect("serialize"); + let recovered = SaveStateV1::from_bytes(&bytes).expect("deserialize"); + + assert!( + recovered.selected_bookmark.bookmark_id.is_none(), + "default bookmark_id (None) must roundtrip" + ); + assert!( + recovered.selected_bookmark.starting_location_id.is_none(), + "default starting_location_id (None) must roundtrip" + ); + } + // ----------------------------------------------------------------------- // Modifications stub round-trip (#567, D-111/D-112) // -----------------------------------------------------------------------