fix(simulation): address PR #81 review — critical and high-priority issues
Critical fixes: - storyteller: replace .expect() with guard + log in activation_pass (Hoshe #1) - content/loader: validate inverted tile_bounds before iteration (Hoshe #C) - save_io: persist ActivationState on save/load (Tyre #7) High-priority fixes: - storyteller: f64 intermediate for observation_time_ticks scoring (Hoshe #A) - storyteller: deduplicate copresent entities before scoring (Hoshe #B) - storyteller: skip activation_pass at tick 0 (Hoshe #G) - storyteller: explicit .before(advance_tick) ordering (Tyre #9) - save_state: insert EngagementRecord on NPC deserialize (Tyre #10) - save_io: reset TriangleActivatedQueue + MovementHistoryBuffer on load (Tyre #8) - debug: validate teleport target walkability (Hoshe #E) - debug: reject SkipToContamination when tick past delay (Hoshe #F) - debug: DebugEnabled defaults to cfg!(debug_assertions) (Hoshe #3) - debug: log when response overwritten (Hoshe #2) - content/loader: error on tile dimension mismatch (Hoshe #5) - content/types: DistrictMeta.description optional (Hoshe #D) - types: version docs updated to v18 (Tyre #1, #2, #3) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
This commit is contained in:
@@ -15,7 +15,7 @@ use crate::content::template::TriangleState;
|
||||
use crate::knowledge::EntityRegistry;
|
||||
use crate::npc::Npc;
|
||||
use crate::simulation::conversation::NpcName;
|
||||
use crate::simulation::movement::{PlayerCharacter, TilePosition};
|
||||
use crate::simulation::movement::{PlayerCharacter, TilePosition, WalkabilityMap};
|
||||
use crate::simulation::tier::ActiveSim;
|
||||
use crate::simulation::time::SimulationTime;
|
||||
use crate::storyteller::{ContaminationActive, ContaminationEventQueue, CONTAMINATION_DELAY_TICKS};
|
||||
@@ -63,6 +63,7 @@ pub fn handle_debug_commands(
|
||||
mut time: ResMut<SimulationTime>,
|
||||
mut contamination: Option<ResMut<ContaminationActive>>,
|
||||
mut contamination_queue: Option<ResMut<ContaminationEventQueue>>,
|
||||
walkability: Option<Res<WalkabilityMap>>,
|
||||
registry: Res<EntityRegistry>,
|
||||
mut player_query: Query<(Entity, &mut TilePosition), With<PlayerCharacter>>,
|
||||
triangles: Query<(Entity, &TriangleState), With<ActiveSim>>,
|
||||
@@ -113,13 +114,25 @@ pub fn handle_debug_commands(
|
||||
),
|
||||
success: true,
|
||||
},
|
||||
Some(false) if time.tick >= CONTAMINATION_DELAY_TICKS => {
|
||||
// Tick is already past the delay — advancing would be a no-op
|
||||
// or rewinding would break cooldowns. Report error instead.
|
||||
DebugResponsePayload {
|
||||
command: "SkipToContamination".to_string(),
|
||||
text: format!(
|
||||
"Current tick ({}) already past contamination delay ({}). Contamination should fire on next system run — use ForceContaminationActivate if it hasn't.",
|
||||
time.tick, CONTAMINATION_DELAY_TICKS
|
||||
),
|
||||
success: false,
|
||||
}
|
||||
}
|
||||
Some(false) => {
|
||||
let old_tick = time.tick;
|
||||
time.tick = CONTAMINATION_DELAY_TICKS;
|
||||
DebugResponsePayload {
|
||||
command: "SkipToContamination".to_string(),
|
||||
text: format!(
|
||||
"Skipped to contamination delay: tick {} -> {}. Contamination will fire on next system run.",
|
||||
"Advanced tick to contamination threshold: {} -> {}. Contamination will fire on next system run.",
|
||||
old_tick, time.tick
|
||||
),
|
||||
success: true,
|
||||
@@ -128,7 +141,21 @@ pub fn handle_debug_commands(
|
||||
}
|
||||
}
|
||||
DebugCommandKind::TeleportToPosition { x, y, z } => {
|
||||
if let Ok((_, mut pos)) = player_query.single_mut() {
|
||||
let target = TilePosition::new(x, y, z);
|
||||
// Validate target is walkable (if WalkabilityMap available)
|
||||
let walkable = walkability
|
||||
.as_ref()
|
||||
.map_or(true, |wm| wm.can_move_to(&target));
|
||||
if !walkable {
|
||||
DebugResponsePayload {
|
||||
command: format!("TeleportToPosition({}, {}, {})", x, y, z),
|
||||
text: format!(
|
||||
"Target ({}, {}, {}) is not walkable. Player would be stuck in wall/void.",
|
||||
x, y, z
|
||||
),
|
||||
success: false,
|
||||
}
|
||||
} else if let Ok((_, mut pos)) = player_query.single_mut() {
|
||||
let old = (pos.x, pos.y, pos.z);
|
||||
pos.x = x;
|
||||
pos.y = y;
|
||||
@@ -297,6 +324,12 @@ pub fn handle_debug_commands(
|
||||
};
|
||||
|
||||
tracing::debug!(command = %response.command, success = response.success, "Debug command processed");
|
||||
if buffer.pending_debug_response.is_some() {
|
||||
tracing::trace!(
|
||||
"Debug response overwritten by '{}' — earlier response dropped (last-wins per tick)",
|
||||
response.command
|
||||
);
|
||||
}
|
||||
buffer.pending_debug_response = Some(response);
|
||||
}
|
||||
}
|
||||
@@ -420,4 +453,53 @@ mod tests {
|
||||
schedule.run(&mut world);
|
||||
assert!(world.resource::<SnapshotBuffer>().pending_debug_response.is_none());
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn teleport_rejects_unwalkable_target() {
|
||||
let (mut world, mut schedule) = setup_debug_world();
|
||||
let mut map = WalkabilityMap::new(10, 10, 1);
|
||||
map.set_walkable(&TilePosition::new(5, 5, 0), false);
|
||||
world.insert_resource(map);
|
||||
|
||||
world.resource_mut::<DebugCommandBuffer>().push(
|
||||
DebugCommandKind::TeleportToPosition { x: 5, y: 5, z: 0 },
|
||||
);
|
||||
schedule.run(&mut world);
|
||||
|
||||
// Player should NOT have moved
|
||||
let mut q = world.query_filtered::<&TilePosition, With<PlayerCharacter>>();
|
||||
let pos = q.single(&world).unwrap();
|
||||
assert_eq!((pos.x, pos.y, pos.z), (10, 20, 0), "player must not teleport to unwalkable tile");
|
||||
|
||||
let resp = world.resource::<SnapshotBuffer>().pending_debug_response.as_ref().unwrap();
|
||||
assert!(!resp.success);
|
||||
assert!(resp.text.contains("not walkable"));
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn skip_to_contamination_rejects_when_past_delay() {
|
||||
let (mut world, mut schedule) = setup_debug_world();
|
||||
// Set tick past the delay but contamination not yet active
|
||||
world.resource_mut::<SimulationTime>().tick = CONTAMINATION_DELAY_TICKS + 100;
|
||||
|
||||
world.resource_mut::<DebugCommandBuffer>().push(DebugCommandKind::SkipToContamination);
|
||||
schedule.run(&mut world);
|
||||
|
||||
// Tick should NOT have changed (no rewind)
|
||||
assert_eq!(
|
||||
world.resource::<SimulationTime>().tick,
|
||||
CONTAMINATION_DELAY_TICKS + 100,
|
||||
"tick must not rewind"
|
||||
);
|
||||
|
||||
let resp = world.resource::<SnapshotBuffer>().pending_debug_response.as_ref().unwrap();
|
||||
assert!(!resp.success);
|
||||
assert!(resp.text.contains("already past"));
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn debug_enabled_defaults_to_debug_assertions() {
|
||||
let d = DebugEnabled::default();
|
||||
assert_eq!(d.0, cfg!(debug_assertions));
|
||||
}
|
||||
}
|
||||
|
||||
@@ -72,10 +72,11 @@ pub struct StartupMessage {
|
||||
/// v16 adds: triangle_crisis_events (#250, D-087 triangle escalation for future client rendering).
|
||||
/// v17 adds: state_hash (#85, desync detection — fast hash of player pos + NPC count + tick),
|
||||
/// sim_errors (#85, structured error reporting to client).
|
||||
/// v18 adds: debug_response (#580, debug console server — command/response wire).
|
||||
/// Future fields: ambient sound events, HUD state (D-020 expansion).
|
||||
#[derive(Debug, Clone, Serialize, Deserialize)]
|
||||
pub struct ObserverSnapshot {
|
||||
/// Protocol version for forward compatibility. Current: 17.
|
||||
/// Protocol version for forward compatibility. Current: 18.
|
||||
pub version: u8,
|
||||
/// Simulation tick when this snapshot was produced
|
||||
pub tick: u64,
|
||||
@@ -550,13 +551,13 @@ pub struct DebugResponsePayload {
|
||||
/// Whether the debug console is enabled (#580).
|
||||
///
|
||||
/// Set at server startup. Cannot be toggled mid-session via IPC.
|
||||
/// v0.1: defaults to true. Production builds will default to false.
|
||||
/// Defaults to true in debug builds, false in release builds.
|
||||
#[derive(Resource, Debug, Clone)]
|
||||
pub struct DebugEnabled(pub bool);
|
||||
|
||||
impl Default for DebugEnabled {
|
||||
fn default() -> Self {
|
||||
Self(true)
|
||||
Self(cfg!(debug_assertions))
|
||||
}
|
||||
}
|
||||
|
||||
|
||||
Reference in New Issue
Block a user