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

fix(simulation): address PR #26 review comments (13 items)

Browse Source 
Warnings fixed:
- Add WalkAway to all_player_action_variants_roundtrip test
- Warn and skip on unresolvable speaker_entity_id (was silent 0)
- Change MonologueState.shown_ids from Vec to HashSet (O(1) lookup)
- Add cross-plugin ordering: trigger_recognition_monologue after
  detect_anomalies (latent determinism bug)
- Add TODO for unreachable Secret trust tier

Suggestions addressed:
- Server-side range check for Talk verb in handle_talk (CLOSE_RANGE)
- Emit IncompleteInteraction before overwriting ActiveDialogue
- Add different_seed_produces_different_replay determinism test
- Replace panic with assert for unknown fixture naming convention
- Fix duplicate "Observe" label: ExamineNpc now uses "Examine NPC"
- Change DialogueCooldownTracker.used from Vec to BTreeMap (D-041)
- Add .after(process_talk_interaction) to process_walk_away ordering
- Collapse dead conditional in main.rs (both branches identical)

468 tests pass, 0 failures.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
This commit is contained in:
jpmschweitzer
2026-02-17 18:12:45 +01:00
co-authored by Claude Opus 4.6
parent a5ab8238e1
commit 12d1fd505e
8 changed files with 223 additions and 34 deletions
Download Patch File Download Diff File Expand all files Collapse all files
server/src/bridge/mod.rs
+4 -2
View File
@@ -168,13 +168,15 @@ impl Plugin for BridgePlugin {
crate::simulation::monologue::trigger_monologue
.after(crate::simulation::movement::validate_movement),
crate::simulation::monologue::trigger_recognition_monologue
.after(crate::simulation::monologue::trigger_monologue),
.after(crate::simulation::monologue::trigger_monologue)
.after(crate::perception::anomaly::detect_anomalies),
crate::simulation::monologue::process_sprint_anomaly_monologue
.after(crate::simulation::monologue::trigger_recognition_monologue),
crate::simulation::dialogue::process_talk_interaction
.after(crate::simulation::input::process_player_input),
crate::simulation::dialogue::process_walk_away
.after(crate::simulation::input::process_player_input),
.after(crate::simulation::input::process_player_input)
.after(crate::simulation::dialogue::process_talk_interaction),
crate::perception::observer::compute_observer_snapshot
.after(crate::perception::observer::compute_visibility_geometry)
.after(crate::simulation::interaction::compute_nearby_interactions)
server/src/main.rs
+2 -7
View File
@@ -114,13 +114,8 @@ fn main() {
// Override SimRng with the chosen seed (SimulationPlugin defaults to seed 0)
app.insert_resource(settled_reach_server::simulation::rng::SimRng::new(seed));
if test_mode {
// Gauntlet content: deferred until Gauntlet loader exists.
// For now, fall back to the proof room setup.
setup_proof_room(&mut app);
} else {
setup_proof_room(&mut app);
}
// Gauntlet content loader is future scope — proof room for all modes.
setup_proof_room(&mut app);
tracing::info!(
"Simulation initialized (seed={}, test_mode={})",
server/src/simulation/dialogue.rs
+41 -11
View File
@@ -73,26 +73,26 @@ impl Default for CurrentMood {
/// Entries older than the cooldown window are pruned each query.
#[derive(Component, Debug, Default)]
pub struct DialogueCooldownTracker {
used: Vec<(String, u64)>, // (line_id, tick_used)
used: std::collections::BTreeMap<String, u64>, // line_id → tick_used (D-041)
}
impl DialogueCooldownTracker {
/// Record that a line was used at the given tick.
pub fn record(&mut self, line_id: &str, tick: u64) {
self.used.push((line_id.to_string(), tick));
self.used.insert(line_id.to_string(), tick);
}
/// Check if a line is on cooldown at the given tick.
pub fn is_on_cooldown(&self, line_id: &str, tick: u64) -> bool {
self.used
.iter()
.any(|(id, used_tick)| id == line_id && tick.saturating_sub(*used_tick) < LINE_COOLDOWN_TICKS)
.get(line_id)
.is_some_and(|used_tick| tick.saturating_sub(*used_tick) < LINE_COOLDOWN_TICKS)
}
/// Prune entries older than the cooldown window.
pub fn prune(&mut self, tick: u64) {
self.used
.retain(|(_, used_tick)| tick.saturating_sub(*used_tick) < LINE_COOLDOWN_TICKS);
.retain(|_, used_tick| tick.saturating_sub(*used_tick) < LINE_COOLDOWN_TICKS);
}
}
@@ -161,6 +161,10 @@ pub fn available_access_tiers(relationship: RelationshipState) -> Vec<AccessTier
/// v0.1 mapping:
/// - Friendly → Real (relationship depth unlocks deeper trust)
/// - All others → Surface
///
/// TODO: TrustTier::Secret is currently unreachable. It should gate on
/// KG confidence (e.g., KnowsDetails+ for a specific secret topic) rather
/// than RelationshipState alone. Tracked for Phase 2 narrative expansion.
pub fn relationship_to_trust(relationship: RelationshipState) -> TrustTier {
match relationship {
RelationshipState::Friendly => TrustTier::Real,
@@ -296,6 +300,7 @@ pub fn process_talk_interaction(
line_pool: Option<Res<LinePoolIndexResource>>,
registry: Res<EntityRegistry>,
mut rng: ResMut<SimRng>,
mut event_queue: ResMut<crate::knowledge::KnowledgeEventQueue>,
mut player_query: Query<
(
Entity,
@@ -303,6 +308,7 @@ pub fn process_talk_interaction(
&TalkRequest,
&mut DialogueResponseBuffer,
&mut DialogueCooldownTracker,
Option<&ActiveDialogue>,
),
With<PlayerCharacter>,
>,
@@ -312,7 +318,7 @@ pub fn process_talk_interaction(
return;
};
let Ok((player_entity, observer_kg, talk_request, mut response_buffer, mut cooldown)) =
let Ok((player_entity, observer_kg, talk_request, mut response_buffer, mut cooldown, active_dialogue_opt)) =
player_query.single_mut()
else {
return;
@@ -396,17 +402,40 @@ pub fn process_talk_interaction(
);
if let Some(line) = selected {
// Resolve wire ID for the speaker
let speaker_wire_id = registry.to_stable(target).map(|s| s.0).unwrap_or(0);
// Resolve wire ID for the speaker — skip if target not in registry
let Some(speaker_stable) = registry.to_stable(target) else {
tracing::warn!(
"Talk target {:?} not in EntityRegistry — cannot resolve wire ID, skipping dialogue",
target
);
commands.entity(player_entity).remove::<TalkRequest>();
return;
};
response_buffer.response = Some(DialogueResponseEvent {
line_id: line.id.clone(),
text: line.text.clone(),
speaker_entity_id: speaker_wire_id,
speaker_entity_id: speaker_stable.0,
});
cooldown.record(&line.id, time.tick);
// Emit IncompleteInteraction if overwriting an existing dialogue session
if let Some(prev) = active_dialogue_opt {
event_queue.push(crate::knowledge::KnowledgeEvent {
observer: player_entity,
tick: time.tick,
event_type: crate::knowledge::KnowledgeEventType::IncompleteInteraction {
target: prev.target,
interaction_type: prev.interaction_type,
},
});
tracing::debug!(
"Overwriting active {:?} dialogue — emitted IncompleteInteraction",
prev.interaction_type,
);
}
// Track active dialogue for walk-away detection (D-064)
commands.entity(player_entity).insert(ActiveDialogue {
target,
@@ -417,7 +446,7 @@ pub fn process_talk_interaction(
tracing::debug!(
"Dialogue selected: id={}, speaker={}, location={}, role={}",
line.id,
speaker_wire_id,
speaker_stable.0,
profile.location,
profile.role,
);
@@ -687,7 +716,7 @@ mod tests {
tracker.record("recent", LINE_COOLDOWN_TICKS);
tracker.prune(LINE_COOLDOWN_TICKS);
assert_eq!(tracker.used.len(), 1);
assert_eq!(tracker.used[0].0, "recent");
assert!(tracker.used.contains_key("recent"));
}
// -- Selection tests -----------------------------------------------------
@@ -778,6 +807,7 @@ mod tests {
world.init_resource::<SimulationTime>();
world.insert_resource(SimRng::new(42));
world.init_resource::<EntityRegistry>();
world.init_resource::<crate::knowledge::KnowledgeEventQueue>();
world
}
server/src/simulation/input.rs
+19 -2
View File
@@ -90,6 +90,7 @@ pub fn process_player_input(
With<PlayerCharacter>,
>,
inventory_items: Query<(Entity, &CarriedBy, &ItemName, &InventorySlot)>,
all_positions: Query<&TilePosition>,
) {
let current_tick = time.tick;
let paused = time.paused();
@@ -189,7 +190,7 @@ pub fn process_player_input(
handle_place(&mut commands, &registry, &player_query, target_entity_id);
}
Some("Talk") => {
handle_talk(&mut commands, &registry, &player_query, target_entity_id);
handle_talk(&mut commands, &registry, &player_query, &all_positions, target_entity_id);
}
_ => {
tracing::info!(
@@ -328,6 +329,7 @@ fn handle_take(
/// Handle Talk verb: set TalkRequest marker on the player entity for the target NPC.
/// The actual dialogue pipeline runs in process_talk_interaction (dialogue.rs).
/// Server-side range check: Talk requires CLOSE_RANGE (same as interaction system).
#[allow(clippy::type_complexity)]
fn handle_talk(
commands: &mut Commands,
@@ -341,6 +343,7 @@ fn handle_talk(
),
With<PlayerCharacter>,
>,
all_positions: &Query<&TilePosition>,
target_entity_id: Option<u64>,
) {
let Some(target_id) = target_entity_id else {
@@ -348,7 +351,7 @@ fn handle_talk(
return;
};
let Ok((player_entity, _, _, _)) = player_query.single() else {
let Ok((player_entity, player_pos, _, _)) = player_query.single() else {
return;
};
@@ -358,6 +361,20 @@ fn handle_talk(
return;
};
// Server-side range check: reject Talk if target is beyond close range
if let Ok(target_pos) = all_positions.get(target_entity) {
let distance = player_pos.manhattan_distance(target_pos).unwrap_or(u32::MAX);
if distance > crate::simulation::interaction::CLOSE_RANGE {
tracing::info!(
target_id,
distance,
"Talk: target out of range (max {})",
crate::simulation::interaction::CLOSE_RANGE,
);
return;
}
}
commands
.entity(player_entity)
.insert(crate::simulation::dialogue::TalkRequest {
server/src/simulation/interaction.rs
+2 -2
View File
@@ -219,7 +219,7 @@ pub fn compute_nearby_interactions(
});
verbs.push(VerbOption {
kind: VerbKind::ExamineNpc,
label: "Observe".into(),
label: "Examine NPC".into(),
priority: 2,
available: true,
});
@@ -227,7 +227,7 @@ pub fn compute_nearby_interactions(
// Mid range: only Examine NPC (Talk requires close range)
verbs.push(VerbOption {
kind: VerbKind::ExamineNpc,
label: "Observe".into(),
label: "Examine NPC".into(),
priority: 1,
available: true,
});
server/src/simulation/monologue.rs
+6 -4
View File
@@ -8,6 +8,8 @@
// When sprinting past a Contradicted entity, a delayed "double-take" monologue
// fires retroactively. Detection in observer pipeline, processing here.
use std::collections::HashSet;
use bevy_ecs::prelude::*;
use rand::Rng;
@@ -81,7 +83,7 @@ pub struct MonologueState {
/// Whether the enter_location monologue has fired this session.
pub entered: bool,
/// IDs of lines already shown (dedup within session).
pub shown_ids: Vec<String>,
pub shown_ids: HashSet<String>,
/// Character type for pool filtering. v0.1: always "detective".
pub character: String,
}
@@ -93,7 +95,7 @@ impl Default for MonologueState {
last_position: None,
idle_ticks: 0,
entered: false,
shown_ids: Vec::new(),
shown_ids: HashSet::new(),
// v0.1: default to detective; character selection sets this
character: "detective".to_string(),
}
@@ -337,7 +339,7 @@ pub fn trigger_recognition_monologue(
duration_seconds: DISPLAY_DURATION,
});
state.shown_ids.push(id.clone());
state.shown_ids.insert(id.clone());
state.last_fired_tick = time.tick;
// Mark this pending recognition as having fired its monologue
@@ -453,7 +455,7 @@ pub fn trigger_monologue(
duration_seconds: DISPLAY_DURATION,
});
state.shown_ids.push(id.to_string());
state.shown_ids.insert(id.to_string());
state.last_fired_tick = time.tick;
// Reset idle counter so time_idle doesn't fire again immediately
state.idle_ticks = 0;
server/tests/determinism.rs
+142 -4
View File
@@ -9,6 +9,7 @@
//! - Fix D (#458): Movers sorted by Entity::to_bits() in collision resolution
use bevy_app::prelude::*;
use bevy_ecs::prelude::Entity;
use settled_reach_server::bridge::types::*;
use settled_reach_server::bridge::{BridgePlugin, SnapshotBuffer};
use settled_reach_server::knowledge::registry::EntityRegistry;
@@ -342,7 +343,144 @@ fn gauntlet_deterministic_replay() {
}
}
// Note: a `different_seed_produces_different_replay` test is deferred until
// the monologue/dialogue systems consume SimRng during the test window.
// Currently the proof room with idle inputs doesn't trigger random events,
// so different seeds produce identical outputs (correct but untestable).
/// Different seeds must produce different outputs when the simulation exercises SimRng.
///
/// The proof room NPCs don't have DialogueProfile, so Talk alone won't trigger
/// dialogue selection (which consumes SimRng). However, monologue content pools
/// may fire during idle ticks if ContentPlugin is loaded with matching lines.
///
/// This test builds a variant setup with dialogue-capable NPCs and a minimal
/// line pool, then sends Talk inputs to exercise the weighted random selection
/// path (select_dialogue_line) which consumes SimRng.
#[test]
fn different_seed_produces_different_replay() {
use settled_reach_server::content::line_pool::{
AccessTier, IndexedDialogueLine, IndexedDialoguePool, LinePoolIndex, Mood, Situation,
TrustTier,
};
use settled_reach_server::content::LinePoolIndexResource;
use settled_reach_server::simulation::dialogue::{
CurrentMood, DialogueCooldownTracker, DialogueProfile, DialogueResponseBuffer,
};
/// Build a deterministic app with dialogue-capable NPCs.
fn build_app_with_dialogue(seed: u64) -> App {
let mut app = build_deterministic_app(seed);
// Add DialogueResponseBuffer + DialogueCooldownTracker to the player
// (safe: compute_observer_snapshot uses Option<&mut DialogueResponseBuffer>)
{
let mut q = app
.world_mut()
.query_filtered::<Entity, bevy_ecs::query::With<PlayerCharacter>>();
let player = q.single(app.world()).unwrap();
app.world_mut().entity_mut(player).insert((
DialogueResponseBuffer::default(),
DialogueCooldownTracker::default(),
));
}
// Add DialogueProfile + CurrentMood to NPC2 (at 14,18 — visible to player)
// NPC2 is the 3rd entity registered (index 2) but we find it by position.
{
let mut q = app.world_mut().query::<(Entity, &TilePosition)>();
let npc2 = q
.iter(app.world())
.find(|(_, pos)| pos.x == 14 && pos.y == 18)
.map(|(e, _)| e)
.expect("NPC2 at (14,18) should exist");
app.world_mut().entity_mut(npc2).insert((
DialogueProfile {
location: "the-terminal".to_string(),
role: "dock-worker".to_string(),
},
CurrentMood(Mood::Comfortable),
));
}
// Insert a line pool with multiple lines so weighted selection is non-trivial
let mut index = LinePoolIndex::default();
let lines: Vec<IndexedDialogueLine> = (0..10)
.map(|i| IndexedDialogueLine {
id: format!("test_line_{:03}", i),
text: format!("Line variant {}.", i),
role: "dock-worker".to_string(),
access: vec![AccessTier::Public],
trust: TrustTier::Surface,
situation: vec![Situation::Routine],
topic: vec![],
mood: if i % 2 == 0 {
vec![Mood::Comfortable]
} else {
vec![]
},
tags: vec![],
knowledge_grant: None,
})
.collect();
let pool = IndexedDialoguePool {
location: "the-terminal".to_string(),
role: "dock-worker".to_string(),
lines,
};
index.dialogue.insert(
("the-terminal".to_string(), "dock-worker".to_string()),
pool,
);
app.insert_resource(LinePoolIndexResource(index));
app
}
// Resolve NPC2's StableId for Talk input (it's the 3rd registered entity, sid=2)
let npc2_sid = 2u64;
let inputs: Vec<Vec<PlayerInput>> = vec![
vec![], // tick 0: idle
vec![PlayerInput {
tick: 1,
action: PlayerAction::Interact {
target_entity_id: Some(npc2_sid),
verb: Some("Talk".to_string()),
},
}],
vec![], // tick 2: idle
vec![], // tick 3: idle
];
let mut run_a = build_app_with_dialogue(42);
let mut run_b = build_app_with_dialogue(9999);
let mut snapshots_a = Vec::new();
let mut snapshots_b = Vec::new();
for tick_inputs in &inputs {
for app_ref in [&mut run_a, &mut run_b] {
let mut queue = app_ref
.world_mut()
.resource_mut::<settled_reach_server::simulation::input::InputQueue>();
for input in tick_inputs {
queue.push(input.clone());
}
}
run_a.update();
run_b.update();
for (app_ref, snaps) in [(&run_a, &mut snapshots_a), (&run_b, &mut snapshots_b)] {
let buffer = app_ref.world().resource::<SnapshotBuffer>();
if let Some(snapshot) = &buffer.snapshot {
let bytes = rmp_serde::to_vec_named(snapshot).expect("serialize snapshot");
snaps.push(bytes);
}
}
}
// At least one snapshot should differ between the two seeds
let any_different = snapshots_a
.iter()
.zip(snapshots_b.iter())
.any(|(a, b)| a != b);
assert!(
any_different,
"Different seeds should produce at least one different snapshot when dialogue exercises SimRng"
);
}
server/tests/serialization.rs
+7 -2
View File
@@ -98,6 +98,7 @@ fn all_player_action_variants_roundtrip() {
PlayerAction::SetTickRate(TickRate::Half),
PlayerAction::ToggleStanceUp,
PlayerAction::ToggleStanceDown,
PlayerAction::WalkAway,
];
for action in actions {
@@ -159,7 +160,11 @@ fn all_fixtures_deserialize() {
rmp_serde::from_slice::<u64>(&bytes)
.unwrap_or_else(|e| panic!("deserialize boundary raw fixture {}: {}", name, e));
} else {
panic!("unknown fixture naming convention: {}", name);
assert!(
false,
"unknown fixture naming convention: {} — add a deserialization branch for this prefix",
name
);
}
count += 1;
}
@@ -533,7 +538,7 @@ fn pending_recognition_wire_roundtrip() {
#[test]
fn all_verb_kind_variants_roundtrip() {
let all_verbs = [
(VerbKind::ExamineNpc, "Observe"),
(VerbKind::ExamineNpc, "Examine NPC"),
(VerbKind::Talk, "Talk"),
(VerbKind::Observe, "Observe"),
(VerbKind::Read, "Read"),