jpmschweitzer/settled-reach
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

refactor(simulation): address PR #32 review — 14 items from Hoshe + Tyre

Browse Source 
Hoshe (code quality):
- Remove dead RoomMember component from reset.rs
- Remove execute_reset (dual API trap); plan_reset is sole production path
- .unwrap() → .expect() on reset_plate in setup_gauntlet boot path
- Add 10s read timeout to TCP runtime test (prevents hangs)
- Register player in EntityRegistry in runtime boot test
- Document room_at z-range and corridor overlap assumptions
- Derive entity count from EXPECTED_ENTITY_COUNT constant (was hardcoded 24)
- Add reset plate (49-51) verification to stable_id_ranges_match_spec
- Add debounce exact boundary test (tick 9 rejected, tick 10 accepted)

Tyre (architecture):
- Gate test_world rooms/constants/setup behind "gauntlet" feature (default-on);
  reset module stays always-compiled (production dependency via input system)
- Document setup_gauntlet scheduler bypass for future tracking
- Extract runtime TCP test to content_runtime.rs (separate failure modes)

507 tests passing.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
This commit is contained in:
jpmschweitzer
2026-02-18 09:45:17 +01:00
co-authored by Claude Opus 4.6
parent e66352e0ea
commit 09eb6d7582
8 changed files with 281 additions and 251 deletions
Download Patch File Download Diff File Expand all files Collapse all files
server/src/lib.rs
+3
View File
@@ -9,4 +9,7 @@ pub mod npc;
pub mod perception;
pub mod simulation;
pub mod storyteller;
// test_world::reset is always compiled (used by simulation::input).
// Room definitions, constants, and setup_gauntlet are gated behind
// the "gauntlet" feature (default-on) to allow stripping from release builds.
pub mod test_world;
server/src/main.rs
+6
View File
@@ -116,7 +116,13 @@ fn main() {
// Gauntlet test world for --test-mode, proof room for normal mode.
if test_mode {
#[cfg(feature = "gauntlet")]
settled_reach_server::test_world::setup_gauntlet(&mut app);
#[cfg(not(feature = "gauntlet"))]
{
eprintln!("--test-mode requires the 'gauntlet' feature");
std::process::exit(1);
}
} else {
setup_proof_room(&mut app);
}
server/src/test_world/constants.rs
+19
View File
@@ -156,6 +156,13 @@ pub const ROOMS: &[GauntletRoom] = &[
/// Look up which room a position falls in.
/// Returns the first room whose bounding box contains the position.
///
/// Assumptions:
/// - All rooms are at z=0 (single-floor Gauntlet). Positions on other
/// z-levels will never match.
/// - Room bounding boxes must not overlap (verified by `rooms_do_not_overlap`
/// test). Corridors are NOT rooms and intentionally fall outside all
/// bounding boxes — `room_at` returns None for corridor positions.
pub fn room_at(pos: &TilePosition) -> Option<&'static GauntletRoom> {
ROOMS.iter().find(|r| {
pos.x >= r.origin.x
@@ -182,6 +189,17 @@ pub const INTERACTION_GALLERY_STABLE_IDS: (u64, u64) = (24, 28);
pub const PAUSE_CHAMBER_STABLE_IDS: (u64, u64) = (29, 29);
pub const DIALOGUE_ROOM_STABLE_IDS: (u64, u64) = (30, 33);
pub const CROWD_PLAZA_STABLE_IDS: (u64, u64) = (34, 48);
pub const RESET_PLATE_STABLE_IDS: (u64, u64) = (49, 51);
/// Number of actively-spawned entities in the current Gauntlet build.
/// Derived from StableId ranges of built rooms + player + reset plates.
/// Reserved (unbuilt) rooms do not contribute entities.
pub const EXPECTED_ENTITY_COUNT: usize = 1 // player (StableId 0)
+ (HUB_STABLE_IDS.1 - HUB_STABLE_IDS.0 + 1) as usize
+ (OCCLUSION_STABLE_IDS.1 - OCCLUSION_STABLE_IDS.0 + 1) as usize
+ (INVENTORY_STABLE_IDS.1 - INVENTORY_STABLE_IDS.0 + 1) as usize
+ (PAUSE_CHAMBER_STABLE_IDS.1 - PAUSE_CHAMBER_STABLE_IDS.0 + 1) as usize
+ (RESET_PLATE_STABLE_IDS.1 - RESET_PLATE_STABLE_IDS.0 + 1) as usize;
#[cfg(test)]
mod tests {
@@ -289,6 +307,7 @@ mod tests {
PAUSE_CHAMBER_STABLE_IDS,
DIALOGUE_ROOM_STABLE_IDS,
CROWD_PLAZA_STABLE_IDS,
RESET_PLATE_STABLE_IDS,
];
for (i, a) in ranges.iter().enumerate() {
for (j, b) in ranges.iter().enumerate() {
server/src/test_world/mod.rs
+48 -8
View File
@@ -5,6 +5,11 @@
//! and manual QA sessions. Loaded instead of content/ when the server runs
//! the Gauntlet map.
//!
//! Module structure:
//! - `reset` — always compiled (used by simulation::input in production)
//! - `constants`, `rooms`, `setup_gauntlet` — gated behind "gauntlet" feature
//! (default-on; strip with --no-default-features for release builds)
//!
//! Architecture (per workshop-outcomes.md Section 4):
//! - Rooms are defined as Rust builder functions (hybrid YAML + Rust inject)
//! - WalkabilityMap covers the full Gauntlet bounds (0-116 x 0-124)
@@ -23,28 +28,45 @@
//! Crowd Plaza: 34-48 (reserved, not yet built)
//! Reset plates: 49-51 (Occlusion, Inventory, Pause)
#[cfg(feature = "gauntlet")]
pub mod constants;
pub mod reset;
#[cfg(feature = "gauntlet")]
pub mod rooms;
#[cfg(feature = "gauntlet")]
use bevy_app::prelude::*;
#[cfg(feature = "gauntlet")]
use crate::knowledge::registry::{EntityRegistry, StableEntityId};
#[cfg(feature = "gauntlet")]
use crate::knowledge::KnowledgeGraph;
#[cfg(feature = "gauntlet")]
use crate::knowledge::types::StableId;
#[cfg(feature = "gauntlet")]
use crate::perception::cognitive_delay::CognitiveDelay;
#[cfg(feature = "gauntlet")]
use crate::perception::vision_cone::Facing;
#[cfg(feature = "gauntlet")]
use crate::simulation::interaction::{Interactable, NearbyInteractionBuffer};
#[cfg(feature = "gauntlet")]
use crate::simulation::inventory::ItemName;
#[cfg(feature = "gauntlet")]
use crate::simulation::listening::ListeningFocus;
#[cfg(feature = "gauntlet")]
use crate::simulation::monologue::{MonologueBuffer, MonologueState, SprintAnomalyQueue};
#[cfg(feature = "gauntlet")]
use crate::simulation::movement::{PlayerCharacter, TilePosition, WalkabilityMap};
#[cfg(feature = "gauntlet")]
use crate::simulation::stance::{MovementProfile, PlayerMoveCooldown};
#[cfg(feature = "gauntlet")]
use reset::{RoomResetTrigger, RoomSnapshots};
#[cfg(feature = "gauntlet")]
/// Map bounds for the full Gauntlet world.
pub const MAP_WIDTH: i32 = 117;
#[cfg(feature = "gauntlet")]
pub const MAP_HEIGHT: i32 = 125;
/// Set up the Gauntlet test world.
@@ -52,6 +74,12 @@ pub const MAP_HEIGHT: i32 = 125;
/// Creates the full WalkabilityMap (blocked by default), carves room
/// interiors and corridors, spawns the player and all room entities
/// in canonical StableId order.
///
/// NOTE: This bypasses the normal startup system scheduler — entities are
/// spawned directly into the World rather than via startup systems. This
/// is intentional for deterministic test setups but should be revisited
/// if Gauntlet is ever served by the production startup pipeline.
#[cfg(feature = "gauntlet")]
pub fn setup_gauntlet(app: &mut App) {
// Start with a fully blocked map, then carve rooms and corridors.
let mut walkability = WalkabilityMap::new_blocked(MAP_WIDTH, MAP_HEIGHT, 1);
@@ -137,9 +165,9 @@ pub fn setup_gauntlet(app: &mut App) {
// Spawned at corridor entrances per workshop-outcomes.md Section 8.
// Each plate triggers reset of its associated room.
let reset_plates: &[(&str, TilePosition)] = &[
("occlusion_corridor", constants::OCCLUSION_CORRIDOR.reset_plate.unwrap()),
("inventory_warehouse", constants::INVENTORY_WAREHOUSE.reset_plate.unwrap()),
("pause_chamber", constants::PAUSE_CHAMBER.reset_plate.unwrap()),
("occlusion_corridor", constants::OCCLUSION_CORRIDOR.reset_plate.expect("occlusion_corridor should have a reset_plate")),
("inventory_warehouse", constants::INVENTORY_WAREHOUSE.reset_plate.expect("inventory_warehouse should have a reset_plate")),
("pause_chamber", constants::PAUSE_CHAMBER.reset_plate.expect("pause_chamber should have a reset_plate")),
];
for &(room_name, pos) in reset_plates {
let entity = app
@@ -191,6 +219,7 @@ pub fn setup_gauntlet(app: &mut App) {
app.insert_resource(registry);
}
#[cfg(feature = "gauntlet")]
/// Carve the walkable interior of a room.
/// Room has 2-tile-thick walls; interior starts 2 tiles inside each boundary.
fn carve_room_interior(wm: &mut WalkabilityMap, ox: i32, oy: i32, w: i32, h: i32) {
@@ -201,6 +230,7 @@ fn carve_room_interior(wm: &mut WalkabilityMap, ox: i32, oy: i32, w: i32, h: i32
}
}
#[cfg(feature = "gauntlet")]
/// Carve a corridor as a fully walkable rectangle.
/// Corridors are 6 tiles wide with walls on both sides — we carve the
/// interior 2 tiles in from each edge (leaving 2-tile walkable center).
@@ -215,7 +245,7 @@ fn carve_corridor(wm: &mut WalkabilityMap, ox: i32, oy: i32, w: i32, h: i32) {
}
}
#[cfg(test)]
#[cfg(all(test, feature = "gauntlet"))]
mod tests {
use super::*;
use crate::knowledge::registry::EntityRegistry;
@@ -230,10 +260,11 @@ mod tests {
setup_gauntlet(&mut app);
let registry = app.world().resource::<EntityRegistry>();
// Player (0) + Hub signs (1-4) + Occlusion (9-12) + Inventory (13-23) + Pause (29)
// + Reset plates (49-51)
// = 1 + 4 + 4 + 11 + 1 + 3 = 24 entities
assert_eq!(registry.len(), 24);
assert_eq!(
registry.len(),
constants::EXPECTED_ENTITY_COUNT,
"entity count should match EXPECTED_ENTITY_COUNT derived from StableId ranges"
);
}
#[test]
@@ -322,5 +353,14 @@ mod tests {
// Pause Chamber at 29
assert!(registry.to_entity(&StableId(29)).is_some(), "Pause Chamber at StableId 29");
// Reset plates at 49-51
for id in constants::RESET_PLATE_STABLE_IDS.0..=constants::RESET_PLATE_STABLE_IDS.1 {
assert!(
registry.to_entity(&StableId(id)).is_some(),
"Reset plate at StableId {}",
id
);
}
}
}
server/src/test_world/reset.rs
+48 -115
View File
@@ -6,11 +6,14 @@
//!
//! Components:
//! - `RoomResetTrigger` — marks an entity as a reset plate for a room
//! - `RoomMember` — tags entities with their source room for filtering
//!
//! Resource:
//! - `RoomSnapshots` — stores tick-0 entity positions per room
//!
//! Production path: `plan_reset` returns planned changes as a Vec, which
//! the input system applies via Commands (see simulation::input). This
//! avoids exclusive World access and is scheduler-friendly.
//!
//! Spec (workshop-outcomes.md Section 8):
//! - Trigger: Interact with reset plate entity (verb "Reset")
//! - Resets: entity positions, carried items from room returned to floor
@@ -20,7 +23,6 @@
use bevy_ecs::prelude::*;
use std::collections::BTreeMap;
use crate::simulation::inventory::{CarriedBy, InventorySlot};
use crate::simulation::movement::TilePosition;
/// Debounce cooldown in ticks between resets of the same room.
@@ -34,13 +36,6 @@ pub struct RoomResetTrigger {
pub room_name: String,
}
/// Tags an entity as belonging to a specific room.
/// Used during reset to identify which entities to restore.
#[derive(Component, Debug, Clone)]
pub struct RoomMember {
pub room_name: String,
}
/// Snapshot of a single entity's initial position.
#[derive(Debug, Clone)]
struct EntitySnapshot {
@@ -88,6 +83,9 @@ impl RoomSnapshots {
/// Plan a room reset for use with Commands (system-friendly).
/// Returns the list of (entity, position, is_floor_item) changes to apply,
/// or None if debounced or unknown room. Updates debounce tracking.
///
/// This is the canonical production API — the input system applies the
/// returned changes via Commands to avoid exclusive World access.
pub fn plan_reset(
&mut self,
room_name: &str,
@@ -109,62 +107,6 @@ impl RoomSnapshots {
Some(changes)
}
/// Execute a room reset. Restores entity positions and returns
/// floor items to their original locations.
///
/// Returns the number of entities restored, or None if the room
/// has no snapshot or debounce hasn't elapsed.
pub fn execute_reset(
&mut self,
room_name: &str,
current_tick: u64,
world: &mut World,
) -> Option<usize> {
if !self.can_reset(room_name, current_tick) {
tracing::info!(
room_name,
"Room reset debounced (last reset tick: {:?})",
self.last_reset_tick.get(room_name)
);
return None;
}
let snapshot = self.snapshots.get(room_name)?;
let mut restored = 0;
for snap in &snapshot.entities {
// Check if entity still exists
if world.get_entity(snap.entity).is_err() {
continue;
}
if snap.is_floor_item {
// Floor item: remove CarriedBy/InventorySlot if carried,
// restore TilePosition to original location
let mut entity_mut = world.entity_mut(snap.entity);
entity_mut.remove::<CarriedBy>();
entity_mut.remove::<InventorySlot>();
entity_mut.insert(snap.position);
} else {
// NPC or other entity: just restore position
world.entity_mut(snap.entity).insert(snap.position);
}
restored += 1;
}
self.last_reset_tick
.insert(room_name.to_string(), current_tick);
tracing::info!(
room_name,
restored,
current_tick,
"Room reset executed"
);
Some(restored)
}
}
#[cfg(test)]
@@ -172,86 +114,77 @@ mod tests {
use super::*;
#[test]
fn record_and_reset_restores_position() {
fn plan_reset_returns_correct_changes() {
let mut world = World::new();
let entity = world.spawn(TilePosition::new(10, 20, 0)).id();
let mut snapshots = RoomSnapshots::default();
// Record initial position
snapshots.record("test_room", entity, TilePosition::new(10, 20, 0), false);
// Move entity
*world.get_mut::<TilePosition>(entity).unwrap() = TilePosition::new(50, 50, 0);
assert_eq!(world.get::<TilePosition>(entity).unwrap().x, 50);
// Reset
let restored = snapshots.execute_reset("test_room", 0, &mut world);
assert_eq!(restored, Some(1));
assert_eq!(world.get::<TilePosition>(entity).unwrap().x, 10);
assert_eq!(world.get::<TilePosition>(entity).unwrap().y, 20);
let changes = snapshots.plan_reset("test_room", 0);
assert!(changes.is_some());
let changes = changes.unwrap();
assert_eq!(changes.len(), 1);
assert_eq!(changes[0].0, entity);
assert_eq!(changes[0].1, TilePosition::new(10, 20, 0));
assert!(!changes[0].2); // not a floor item
}
#[test]
fn reset_restores_floor_item() {
fn plan_reset_includes_floor_items() {
let mut world = World::new();
world.init_resource::<crate::knowledge::EntityRegistry>();
// Floor item starts on ground
let item = world
.spawn(TilePosition::new(5, 5, 0))
.id();
let item = world.spawn(TilePosition::new(5, 5, 0)).id();
let mut snapshots = RoomSnapshots::default();
snapshots.record("warehouse", item, TilePosition::new(5, 5, 0), true);
// Simulate Take: remove TilePosition, add CarriedBy + InventorySlot
world.entity_mut(item).remove::<TilePosition>();
world
.entity_mut(item)
.insert((CarriedBy(crate::knowledge::types::StableId(0)), InventorySlot(0)));
assert!(world.get::<TilePosition>(item).is_none());
assert!(world.get::<CarriedBy>(item).is_some());
// Reset
let restored = snapshots.execute_reset("warehouse", 0, &mut world);
assert_eq!(restored, Some(1));
// Item should be back on the ground
assert_eq!(
world.get::<TilePosition>(item).unwrap(),
&TilePosition::new(5, 5, 0)
);
assert!(world.get::<CarriedBy>(item).is_none());
assert!(world.get::<InventorySlot>(item).is_none());
let changes = snapshots.plan_reset("warehouse", 0).unwrap();
assert_eq!(changes.len(), 1);
assert!(changes[0].2); // is a floor item
}
#[test]
fn debounce_prevents_rapid_reset() {
fn plan_reset_debounces() {
let mut world = World::new();
let entity = world.spawn(TilePosition::new(10, 20, 0)).id();
let mut snapshots = RoomSnapshots::default();
snapshots.record("test_room", entity, TilePosition::new(10, 20, 0), false);
// First reset at tick 0
let result = snapshots.execute_reset("test_room", 0, &mut world);
assert_eq!(result, Some(1));
assert!(snapshots.plan_reset("test_room", 0).is_some());
// Second reset at tick 5 — should be debounced
let result = snapshots.execute_reset("test_room", 5, &mut world);
assert_eq!(result, None);
assert!(snapshots.plan_reset("test_room", 5).is_none());
// Third reset at tick 10 — should succeed
let result = snapshots.execute_reset("test_room", 10, &mut world);
assert_eq!(result, Some(1));
assert!(snapshots.plan_reset("test_room", 10).is_some());
}
#[test]
fn unknown_room_returns_none() {
fn plan_reset_debounce_exact_boundary() {
let mut world = World::new();
let entity = world.spawn(TilePosition::new(10, 20, 0)).id();
let mut snapshots = RoomSnapshots::default();
let result = snapshots.execute_reset("nonexistent", 0, &mut world);
assert_eq!(result, None);
snapshots.record("test_room", entity, TilePosition::new(10, 20, 0), false);
// Reset at tick 0
assert!(snapshots.plan_reset("test_room", 0).is_some());
// Tick 9: exactly one tick before debounce expires — must be rejected
assert!(
snapshots.plan_reset("test_room", 9).is_none(),
"tick 9 should be rejected (debounce is 10 ticks)"
);
// Tick 10: exact debounce boundary — must be accepted
assert!(
snapshots.plan_reset("test_room", 10).is_some(),
"tick 10 should be accepted (debounce elapsed)"
);
}
#[test]
fn plan_reset_unknown_room_returns_none() {
let mut snapshots = RoomSnapshots::default();
assert!(snapshots.plan_reset("nonexistent", 0).is_none());
}
#[test]