fix(simulation): address PR #39 review — 3 warnings + 6 suggestions

Warnings fixed:
- contraband.rs: scan event now always emits even when NPC already
  knows (was skipped by early `continue`). Contract matches doc.
- test_world/mod.rs: ScanEventBuffer added to player spawn bundle
  so check_contraband_scan doesn't silently no-op in gauntlet mode.
- npc/mod.rs → simulation/mod.rs: moved check_contraband_scan
  registration to SimulationPlugin (operates on player inventory and
  snapshot pipeline, consistent with process_talk_interaction).

Suggestions addressed:
- cross_room_transitions.rs T1: clarified standalone position vs
  constants.rs observer position in comment.
- dialogue.rs: Vec<&str> dedup replaced with BTreeSet<&str> for
  deterministic iteration (project convention).
- contraband.rs: added test for multiple simultaneous ScanAuthority
  NPCs in range (564 tests total).
- dialogue.rs: doc-comment on relationship_to_trust explaining
  KnowledgeConfidence ordering and Suspects default.
- cross_room_transitions.rs T5: noted direct KG API usage vs full
  perception system.
- sprint_gauntlet.rs: documented intentional Contentment { level: 0 }.
- content_scaling.rs: noted GAUNTLET_NPC_COUNT is manually maintained.
- contraband.rs: doc-comment on cross-plugin registration rationale.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
This commit is contained in:
2026-02-19 12:23:02 +01:00
co-authored by Claude Opus 4.6
parent bee93963d9
commit 258b266f15
8 changed files with 135 additions and 44 deletions
+95 -26
View File
@@ -68,7 +68,12 @@ impl ScanEventBuffer {
/// 1. Query player's carried items for Contraband marker
/// 2. If found and NPC doesn't already know: update NPC's KnowledgeGraph
/// with HasContraband fact (KnowsDetails, DirectObservation source)
/// 3. Emit ScanEvent to the player's ScanEventBuffer
/// 3. Emit ScanEvent to the player's ScanEventBuffer (always, regardless of
/// detection result or prior knowledge — client renders the scan animation)
///
/// Registered in SimulationPlugin (not NpcPlugin) because it operates on
/// player inventory and writes to the snapshot pipeline. Consistent with
/// process_talk_interaction and other cross-entity systems.
///
/// System ordering: after validate_movement, before compute_observer_snapshot.
#[allow(clippy::type_complexity)]
@@ -110,34 +115,32 @@ pub fn check_contraband_scan(
let fact_id = FactId(format!("contraband.detected_{}", player_sid.0));
if has_contraband {
// Skip if NPC already knows about this player's contraband
if npc_kg.fact_at_least(&fact_id, KnowledgeConfidence::KnowsDetails) {
continue;
// Only update KG on first detection (idempotent — don't overwrite existing fact)
if !npc_kg.fact_at_least(&fact_id, KnowledgeConfidence::KnowsDetails) {
npc_kg.facts.insert(
fact_id,
FactKnowledge {
confidence: KnowledgeConfidence::KnowsDetails,
source: KnowledgeSource::DirectObservation { tick: time.tick },
state: KnowledgeState::Active,
acquired_tick: time.tick,
},
);
// Also ensure the NPC has entity knowledge of the player
npc_kg.observe_entity(player_sid, player_pos, time.tick);
tracing::info!(
npc_id = npc_sid.0,
player_id = player_sid.0,
tick = time.tick,
"Contraband detected: NPC scanned player and found contraband"
);
}
// Update NPC's KG: record HasContraband fact
npc_kg.facts.insert(
fact_id,
FactKnowledge {
confidence: KnowledgeConfidence::KnowsDetails,
source: KnowledgeSource::DirectObservation { tick: time.tick },
state: KnowledgeState::Active,
acquired_tick: time.tick,
},
);
// Also ensure the NPC has entity knowledge of the player
npc_kg.observe_entity(player_sid, player_pos, time.tick);
tracing::info!(
npc_id = npc_sid.0,
player_id = player_sid.0,
tick = time.tick,
"Contraband detected: NPC scanned player and found contraband"
);
}
// Emit scan event regardless (client renders the scan itself)
// Emit scan event regardless of detection or prior knowledge
// (client renders the scan animation itself)
scan_buffer.push(ScanEvent {
scanner_entity_id: npc_sid.0,
detected_contraband: has_contraband,
@@ -475,6 +478,72 @@ mod tests {
let npc_kg = world.get::<KnowledgeGraph>(npc).unwrap();
let fact = npc_kg.facts.get(&fact_id).unwrap();
assert_eq!(fact.acquired_tick, 0, "should not overwrite existing knowledge");
// Scan event should still fire even though NPC already knew
let mut buffer = world.get_mut::<ScanEventBuffer>(player).unwrap();
let events = buffer.take();
assert_eq!(events.len(), 1, "scan event should emit even for already-known contraband");
assert!(events[0].detected_contraband);
}
#[test]
fn multiple_scan_authority_npcs_each_emit_event() {
let mut world = setup_world();
let player = world
.spawn((
PlayerCharacter,
TilePosition::new(5, 5, 0),
KnowledgeGraph::new(),
ScanEventBuffer::default(),
))
.id();
let player_sid = world.resource_mut::<EntityRegistry>().register(player);
world.spawn((
CarriedBy(player_sid),
ItemName("Lattice Component".into()),
InventorySlot(0),
Contraband,
));
// Two NPCs with ScanAuthority, both in range
let npc1 = world
.spawn((
Npc,
TilePosition::new(5, 6, 0),
KnowledgeGraph::new(),
ScanAuthority,
))
.id();
world.resource_mut::<EntityRegistry>().register(npc1);
let npc2 = world
.spawn((
Npc,
TilePosition::new(6, 5, 0),
KnowledgeGraph::new(),
ScanAuthority,
))
.id();
world.resource_mut::<EntityRegistry>().register(npc2);
let mut schedule = bevy_ecs::schedule::Schedule::default();
schedule.add_systems(check_contraband_scan);
schedule.run(&mut world);
// Both NPCs should have KG entries
let fact_id = FactId(format!("contraband.detected_{}", player_sid.0));
let npc1_kg = world.get::<KnowledgeGraph>(npc1).unwrap();
assert!(npc1_kg.fact_at_least(&fact_id, KnowledgeConfidence::KnowsDetails));
let npc2_kg = world.get::<KnowledgeGraph>(npc2).unwrap();
assert!(npc2_kg.fact_at_least(&fact_id, KnowledgeConfidence::KnowsDetails));
// Both should emit separate scan events
let mut buffer = world.get_mut::<ScanEventBuffer>(player).unwrap();
let events = buffer.take();
assert_eq!(events.len(), 2, "each ScanAuthority NPC should emit a scan event");
assert!(events.iter().all(|e| e.detected_contraband));
}
#[test]