From 2189a9addd05dcf6265de763b03a1fe6f8c32ea4 Mon Sep 17 00:00:00 2001 From: Jeroen Schweitzer Date: Mon, 23 Feb 2026 20:20:00 +0100 Subject: [PATCH] =?UTF-8?q?fix(simulation):=20address=20PR=20#56=20review?= =?UTF-8?q?=20=E2=80=94=20pipeline=20extraction,=20ActiveDialogue,=20range?= =?UTF-8?q?=20check?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Tyre #1: Extract run_dialogue_pipeline() shared helper — eliminates ~60 lines of duplication between process_talk_interaction and process_dialogue_response (L1-L4 pipeline). Hoshe #1: process_dialogue_response now updates ActiveDialogue with current tick on follow-up selection — prevents stale started_tick. Tyre #4: process_dialogue_response now updates InteractionMemory on follow-up — multi-turn conversations are visible in history. Hoshe #6 / Tyre #6: handle_dialogue_response adds server-side range check (CLOSE_RANGE), matching Talk/Confront pattern (D-010 info boundary). Hoshe #2: Weighted selection fallback replaced with unreachable!() — score_line always returns >= 1, so the fallback was dead code. Hoshe #3: assert!(false, ...) → panic!() in serialization.rs (clippy). Hoshe #4: SetFacing and TeleportToHub added to roundtrip test. Hoshe #5: setup_dialogue_response_world inlined (trivial pass-through). Tyre #2: Doc comment on DialogueCooldownTracker explains per-player-global design choice (line IDs are NPC-scoped per D-035, no collision risk). Tyre #3: CONFRONTATION_LINES comment updated with TODO for D-028/D-035 migration. Tyre #5: DialogueResponse fixture added for cross-language GDScript testing (input_dialogue_response.msgpack). Co-Authored-By: Claude Opus 4.6 --- .../msgpack/input_dialogue_response.msgpack | 1 + server/src/simulation/dialogue.rs | 221 +++++++++--------- server/src/simulation/input.rs | 24 +- server/tests/gen_fixtures.rs | 13 ++ server/tests/serialization.rs | 7 +- 5 files changed, 151 insertions(+), 115 deletions(-) create mode 100644 client/tests/fixtures/msgpack/input_dialogue_response.msgpack diff --git a/client/tests/fixtures/msgpack/input_dialogue_response.msgpack b/client/tests/fixtures/msgpack/input_dialogue_response.msgpack new file mode 100644 index 000000000..593da0708 --- /dev/null +++ b/client/tests/fixtures/msgpack/input_dialogue_response.msgpack @@ -0,0 +1 @@ +tick,actionDialogueResponsetarget_entity_id*response_idkael-davan_d_001 \ No newline at end of file diff --git a/server/src/simulation/dialogue.rs b/server/src/simulation/dialogue.rs index a542af343..0a6b6cd42 100644 --- a/server/src/simulation/dialogue.rs +++ b/server/src/simulation/dialogue.rs @@ -79,6 +79,11 @@ impl Default for CurrentMood { /// /// Prevents the same line from being selected within LINE_COOLDOWN_TICKS. /// Entries older than the cooldown window are pruned each query. +/// +/// Design note: this is player-global, not per-NPC. Line IDs are NPC-scoped +/// per D-035 (`{template}_{d|m|e}_{###}`), so cross-NPC collisions don't occur +/// in practice. If a future sprint introduces shared line IDs across roles, +/// the key should become `(StableId, line_id)` instead. #[derive(Component, Debug, Default)] pub struct DialogueCooldownTracker { used: std::collections::BTreeMap, // line_id → tick_used (D-041) @@ -321,11 +326,9 @@ pub fn select_dialogue_line<'a>( return None; } - // Weighted random selection + // Weighted random selection — score_line always returns >= 1 (base score), + // so total_weight > 0 is guaranteed when scored is non-empty. let total_weight: u32 = scored.iter().map(|(_, s)| s).sum(); - if total_weight == 0 { - return None; - } let mut roll = rng.random_range(0..total_weight); for (line, weight) in &scored { @@ -335,8 +338,70 @@ pub fn select_dialogue_line<'a>( roll -= weight; } - // Fallback (shouldn't reach here with valid weights) - Some(scored.last().unwrap().0) + unreachable!("weighted selection with total_weight > 0 must select a line") +} + +// --------------------------------------------------------------------------- +// Shared pipeline: Layers 1-4 +// --------------------------------------------------------------------------- + +/// Run the full D-028 four-layer dialogue pipeline and return a selected line. +/// +/// Shared by `process_talk_interaction` and `process_dialogue_response` to +/// avoid duplicating the L1-L4 query + scoring logic. Callers handle the +/// result differently (initial Talk sets ActiveDialogue; follow-up may clear it). +fn run_dialogue_pipeline<'a>( + line_pool: &'a crate::content::line_pool::LinePoolIndex, + location: &str, + role: &str, + relationship: RelationshipState, + confidence: crate::knowledge::types::KnowledgeConfidence, + day_phase: crate::simulation::time::DayPhase, + interaction_mem: Option<&InteractionMemory>, + npc_mood: Option, + cooldown: &DialogueCooldownTracker, + tick: u64, + rng: &mut impl Rng, +) -> Option<&'a IndexedDialogueLine> { + // Layer 1: Access tiers from relationship + let access_tiers = available_access_tiers(relationship); + + // Layer 2: Derive active situations from game state + let mut situations = derive_situations(day_phase, relationship); + + // Layer 2 extension: first_meeting / repeated_visit from InteractionMemory (#325, D-028) + if let Some(mem) = interaction_mem { + if mem.is_first_meeting() { + situations.push(Situation::FirstMeeting); + } else if mem.is_repeated_visit() { + situations.push(Situation::RepeatedVisit); + } + } + + // Layer 3: Trust tier from relationship + confidence (D-075) + let trust = relationship_to_trust(relationship, confidence); + + // Query Layers 1-3: collect candidates across all available access tiers + let mut candidates: Vec<&IndexedDialogueLine> = Vec::new(); + let mut seen_ids: BTreeSet<&str> = BTreeSet::new(); + + for access in &access_tiers { + let results = line_pool.query_dialogue(location, role, *access, &situations, trust); + for line in results { + if seen_ids.insert(&line.id) { + candidates.push(line); + } + } + } + + if candidates.is_empty() { + return None; + } + + // Layer 4: Topic + mood weighted selection + let active_topics: Vec = Vec::new(); // v0.1: no topic context yet + + select_dialogue_line(&candidates, npc_mood, &active_topics, cooldown, tick, rng) } // --------------------------------------------------------------------------- @@ -415,73 +480,26 @@ pub fn process_talk_interaction( .map(|sid| observer_kg.relationship_with(&sid)) .unwrap_or(RelationshipState::Unknown); - // Layer 1: Access tiers from relationship - let access_tiers = available_access_tiers(relationship); - - // Layer 2: Derive active situations from game state - let mut situations = derive_situations(time.day_phase(), relationship); - - // Layer 2 extension: first_meeting / repeated_visit from InteractionMemory (#325, D-028) - if let Some(ref mem) = interaction_mem_opt { - if mem.is_first_meeting() { - situations.push(Situation::FirstMeeting); - } else if mem.is_repeated_visit() { - situations.push(Situation::RepeatedVisit); - } - } - - // Layer 3: Trust tier from relationship + confidence (D-075) // Default to Suspects for unknown NPCs — no KG entry means no basis for // deeper dialogue, which correctly yields Surface trust tier. let confidence = target_stable .and_then(|sid| observer_kg.confidence_of(&sid)) .unwrap_or(crate::knowledge::types::KnowledgeConfidence::Suspects); - let trust = relationship_to_trust(relationship, confidence); - // Query Layers 1-3: collect candidates across all available access tiers - let mut candidates: Vec<&IndexedDialogueLine> = Vec::new(); - let mut seen_ids: BTreeSet<&str> = BTreeSet::new(); - - for access in &access_tiers { - let results = line_pool.0.query_dialogue( - &profile.location, - &profile.role, - *access, - &situations, - trust, - ); - for line in results { - // Deduplicate across access tiers (BTreeSet for deterministic iteration) - if seen_ids.insert(&line.id) { - candidates.push(line); - } - } - } - - if candidates.is_empty() { - tracing::debug!( - "No dialogue lines available for {}/{} (access={:?}, situations={:?}, trust={:?})", - profile.location, - profile.role, - access_tiers, - situations, - trust, - ); - commands.entity(player_entity).remove::(); - return; - } - - // Layer 4: Topic + mood weighted selection let npc_mood = mood_opt.map(|m| m.0); - let active_topics: Vec = Vec::new(); // v0.1: no topic context yet // Prune old cooldown entries cooldown.prune(time.tick); - let selected = select_dialogue_line( - &candidates, + let selected = run_dialogue_pipeline( + &line_pool.0, + &profile.location, + &profile.role, + relationship, + confidence, + time.day_phase(), + interaction_mem_opt.as_deref(), npc_mood, - &active_topics, &cooldown, time.tick, &mut rng.rng, @@ -668,7 +686,7 @@ pub fn process_walk_away( /// Hardcoded confrontation monologue lines (D-063). /// Fired as a monologue spike when the player delivers a confrontation. -/// Future: move to content pools with trigger="confrontation_delivered". +/// TODO: move to content pools with trigger="confrontation_delivered" (D-028/D-035). const CONFRONTATION_LINES: &[(&str, &str)] = &[ ( "confront_01", @@ -812,9 +830,10 @@ pub fn process_dialogue_response( ), With, >, - npc_query: Query<( + mut npc_query: Query<( &DialogueProfile, Option<&CurrentMood>, + Option<&mut InteractionMemory>, Option<&NpcName>, Option<&NpcColorIndex>, )>, @@ -841,8 +860,10 @@ pub fn process_dialogue_response( let Some(line_pool) = line_pool else { return }; - // Look up NPC dialogue profile, mood, name, and color - let Ok((profile, mood_opt, npc_name_opt, color_idx_opt)) = npc_query.get(target) else { + // Look up NPC dialogue profile, mood, interaction history, name, and color + let Ok((profile, mood_opt, mut interaction_mem_opt, npc_name_opt, color_idx_opt)) = + npc_query.get_mut(target) + else { tracing::debug!( "DialogueResponse target {:?} has no DialogueProfile — cannot select follow-up", target @@ -856,54 +877,24 @@ pub fn process_dialogue_response( .map(|sid| observer_kg.relationship_with(&sid)) .unwrap_or(RelationshipState::Unknown); - // Layer 1: Access tiers from relationship - let access_tiers = available_access_tiers(relationship); - - // Layer 2: Derive active situations from game state - let situations = derive_situations(time.day_phase(), relationship); - - // Layer 3: Trust tier from relationship + confidence (D-075) let confidence = target_stable .and_then(|sid| observer_kg.confidence_of(&sid)) .unwrap_or(crate::knowledge::types::KnowledgeConfidence::Suspects); - let trust = relationship_to_trust(relationship, confidence); - // Query Layers 1-3: collect candidates across all available access tiers - let mut candidates: Vec<&IndexedDialogueLine> = Vec::new(); - let mut seen_ids: BTreeSet<&str> = BTreeSet::new(); - - for access in &access_tiers { - let results = line_pool.0.query_dialogue( - &profile.location, - &profile.role, - *access, - &situations, - trust, - ); - for line in results { - if seen_ids.insert(&line.id) { - candidates.push(line); - } - } - } - - tracing::debug!( - response_id = response_id.as_str(), - candidate_count = candidates.len(), - "DialogueResponse: running follow-up pipeline" - ); - - // Layer 4: Topic + mood weighted selection let npc_mood = mood_opt.map(|m| m.0); - let active_topics: Vec = Vec::new(); // v0.1: no topic context // Prune old cooldown entries cooldown.prune(time.tick); - let selected = select_dialogue_line( - &candidates, + let selected = run_dialogue_pipeline( + &line_pool.0, + &profile.location, + &profile.role, + relationship, + confidence, + time.day_phase(), + interaction_mem_opt.as_deref(), npc_mood, - &active_topics, &cooldown, time.tick, &mut rng.rng, @@ -943,12 +934,24 @@ pub fn process_dialogue_response( cooldown.record(&line.id, time.tick); + // Update ActiveDialogue with current tick — prevents stale started_tick + commands.entity(player_entity).insert(ActiveDialogue { + target, + interaction_type: crate::knowledge::events::InteractionType::Talk, + started_tick: time.tick, + }); + // Trust progression: follow-up dialogue warms the NPC trust_queue.push(TrustEvent::TalkCompleted { npc: target, player: player_entity, }); + // Interaction tracking: record follow-up as a talk event + if let Some(ref mut mem) = interaction_mem_opt { + mem.record_talk(time.tick); + } + tracing::debug!( "Follow-up selected: id={}, response_id={}, location={}, role={}", line.id, @@ -969,7 +972,6 @@ pub fn process_dialogue_response( commands.entity(player_entity).remove::(); } } - } // --------------------------------------------------------------------------- @@ -2463,14 +2465,9 @@ mod tests { // === DialogueResponse Tests (#539) === - fn setup_dialogue_response_world() -> World { - let mut world = setup_dialogue_world(); - world - } - #[test] fn process_dialogue_response_selects_follow_up_line() { - let mut world = setup_dialogue_response_world(); + let mut world = setup_dialogue_world(); let index = build_test_line_pool(); world.insert_resource(LinePoolIndexResource(index)); @@ -2517,7 +2514,7 @@ mod tests { #[test] fn process_dialogue_response_clears_active_dialogue_when_no_lines() { - let mut world = setup_dialogue_response_world(); + let mut world = setup_dialogue_world(); // Build pool with only one line — it will be on cooldown let mut index = LinePoolIndex::default(); @@ -2602,7 +2599,7 @@ mod tests { #[test] fn process_dialogue_response_no_profile_is_noop() { - let mut world = setup_dialogue_response_world(); + let mut world = setup_dialogue_world(); // NPC without DialogueProfile let npc = world.spawn((Npc, TilePosition::new(5, 5, 0))).id(); diff --git a/server/src/simulation/input.rs b/server/src/simulation/input.rs index b91a6a0be..91457a830 100644 --- a/server/src/simulation/input.rs +++ b/server/src/simulation/input.rs @@ -288,6 +288,7 @@ pub fn process_player_input( &mut commands, ®istry, &player_query, + &all_positions, target_entity_id, response_id, ); @@ -472,6 +473,10 @@ fn handle_talk( /// Handle DialogueResponse action: set DialogueResponseRequest marker (#539). /// The follow-up dialogue pipeline runs in process_dialogue_response (dialogue.rs). +/// +/// Range check: same CLOSE_RANGE as Talk/Confront (D-010 info boundary). The player +/// must still be near the NPC to continue a conversation — walking away mid-dialogue +/// should not allow remote follow-ups. #[allow(clippy::type_complexity)] fn handle_dialogue_response( commands: &mut Commands, @@ -485,10 +490,11 @@ fn handle_dialogue_response( ), With, >, + all_positions: &Query<&TilePosition>, target_entity_id: u64, response_id: &str, ) { - let Ok((player_entity, _, _, _)) = player_query.single() else { + let Ok((player_entity, player_pos, _, _)) = player_query.single() else { return; }; @@ -498,6 +504,22 @@ fn handle_dialogue_response( return; }; + // Server-side range check: reject DialogueResponse if target moved out of 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_entity_id, + distance, + "DialogueResponse: target out of range (max {})", + crate::simulation::interaction::CLOSE_RANGE, + ); + return; + } + } + commands .entity(player_entity) .insert(crate::simulation::dialogue::DialogueResponseRequest { diff --git a/server/tests/gen_fixtures.rs b/server/tests/gen_fixtures.rs index 51d1f03c5..86a7346a9 100644 --- a/server/tests/gen_fixtures.rs +++ b/server/tests/gen_fixtures.rs @@ -253,6 +253,19 @@ fn generate_msgpack_fixtures() { &rmp_serde::to_vec_named(&input_batch).unwrap(), ); + // PlayerInput: DialogueResponse (#539) + let input_dialogue_response = PlayerInput { + tick: 300, + action: PlayerAction::DialogueResponse { + target_entity_id: 42, + response_id: "kael-davan_d_001".to_string(), + }, + }; + write_fixture( + "input_dialogue_response", + &rmp_serde::to_vec_named(&input_dialogue_response).unwrap(), + ); + // Diagonal movement fixtures (clockwise: NE, SE, SW, NW) for (name, action) in [ ("input_move_northeast", PlayerAction::MoveNortheast), diff --git a/server/tests/serialization.rs b/server/tests/serialization.rs index f93478dc5..e3bdb38e7 100644 --- a/server/tests/serialization.rs +++ b/server/tests/serialization.rs @@ -107,6 +107,10 @@ fn all_player_action_variants_roundtrip() { PlayerAction::ToggleStanceUp, PlayerAction::ToggleStanceDown, PlayerAction::WalkAway, + PlayerAction::SetFacing { + facing: "north".to_string(), + }, + PlayerAction::TeleportToHub, PlayerAction::DialogueResponse { target_entity_id: 42, response_id: "kael-davan_d_001".to_string(), @@ -173,8 +177,7 @@ fn all_fixtures_deserialize() { rmp_serde::from_slice::(&bytes) .unwrap_or_else(|e| panic!("deserialize boundary raw fixture {}: {}", name, e)); } else { - assert!( - false, + panic!( "unknown fixture naming convention: {} — add a deserialization branch for this prefix", name );