From d808d95659581bd3f3b9f2b32366bc606798b31d Mon Sep 17 00:00:00 2001 From: Jeroen Schweitzer Date: Wed, 25 Feb 2026 23:35:23 +0100 Subject: [PATCH] refactor(simulation): address PR #72 review suggestions MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Hoshe #3: replace O(n²) Vec scan in fallback NPC assignment with BTreeSet; prevent same NPC assigned to two roles in one triangle - Hoshe #4: add From impls for RoleId, TriangleId, StableId, and TriangleCrisisEventWire — eliminate fragile .0 access on newtypes - Hoshe #5: consolidate near-identical unit tests with integration counterparts — keep only unique tests in #[cfg(test)] module - Tyre #3: replace O(N*M) scan in apply_resolve_triangle with BTreeMap index for O(1) per-command lookup - Tyre #4: document &mut World on generate_intra_template_triangles - Observer snapshot uses TriangleCrisisEventWire::from instead of manual field mapping Co-Authored-By: Claude Opus 4.6 --- server/src/bridge/types.rs | 15 + server/src/content/template.rs | 562 ++++---------------------- server/src/knowledge/types.rs | 6 + server/src/perception/observer/mod.rs | 11 +- 4 files changed, 92 insertions(+), 502 deletions(-) diff --git a/server/src/bridge/types.rs b/server/src/bridge/types.rs index 15b329599..e258ee8c7 100644 --- a/server/src/bridge/types.rs +++ b/server/src/bridge/types.rs @@ -699,6 +699,21 @@ pub struct TriangleCrisisEventWire { pub tick: u64, } +impl From for TriangleCrisisEventWire { + fn from(e: crate::content::template::TriangleCrisisEvent) -> Self { + Self { + triangle_id: e.triangle_id.into(), + role_assignments: e + .role_assignments + .into_iter() + .map(|(role, sid)| (String::from(role), u64::from(sid))) + .collect(), + trigger_npc_id: e.trigger_npc.into(), + tick: e.tick, + } + } +} + /// Snapshot buffer resource for staging outgoing ObserverSnapshots #[derive(Resource, Debug, Default)] pub struct SnapshotBuffer { diff --git a/server/src/content/template.rs b/server/src/content/template.rs index 1efb8620a..93527b008 100644 --- a/server/src/content/template.rs +++ b/server/src/content/template.rs @@ -30,7 +30,7 @@ use bevy_ecs::prelude::*; use rand::Rng; use serde::{Deserialize, Serialize}; -use std::collections::BTreeMap; +use std::collections::{BTreeMap, BTreeSet}; use crate::knowledge::registry::StableEntityId; use crate::knowledge::types::StableId; @@ -57,6 +57,12 @@ impl RoleId { } } +impl From for String { + fn from(id: RoleId) -> Self { + id.0 + } +} + /// Trust range constraint for a relationship. /// Both bounds are inclusive: the generated trust value must be in `[min, max]`. #[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] @@ -348,6 +354,12 @@ impl TriangleId { } } +impl From for u64 { + fn from(id: TriangleId) -> Self { + id.0 + } +} + /// Which NPC axis is in tension for a given role in a triangle. /// /// Maps to the D-024 10-axis model. Used to specify which axis diverges @@ -510,6 +522,9 @@ pub struct TriangleGenerationResult { /// closest match is used and a warning is logged. Generation never panics. /// /// All randomness flows through `rng` for determinism (D-010). +/// +/// Takes `&mut World` (inherently single-threaded) because it spawns +/// `TriangleState` entities. Consistent with `spawn.rs` template instantiation. pub fn generate_intra_template_triangles( world: &mut World, template_id: TemplateId, @@ -543,34 +558,41 @@ pub fn generate_intra_template_triangles( for def in defs { let mut role_assignments = BTreeMap::new(); + let mut assigned_npcs = BTreeSet::new(); let mut assignment_ok = true; for role_id in &def.roles { if let Some(&stable_id) = role_to_npc.get(role_id) { - role_assignments.insert(role_id.clone(), stable_id); - } else { - // Fallback: pick the first available NPC not already assigned - let already_assigned: Vec = - role_assignments.values().copied().collect(); - let fallback = role_to_npc - .values() - .find(|sid| !already_assigned.contains(sid)); - - if let Some(&fallback_sid) = fallback { - result.warnings.push(format!( - "Triangle {:?}: no NPC for role '{}' — assigned fallback StableId({})", - def.triangle_id, role_id.0, fallback_sid.0 - )); - role_assignments.insert(role_id.clone(), fallback_sid); + if assigned_npcs.contains(&stable_id) { + // This NPC is already assigned to another role in this triangle. + // Fall through to fallback instead of duplicating. } else { - result.warnings.push(format!( - "Triangle {:?}: no NPC available for role '{}' — skipping triangle", - def.triangle_id, role_id.0 - )); - assignment_ok = false; - break; + role_assignments.insert(role_id.clone(), stable_id); + assigned_npcs.insert(stable_id); + continue; } } + + // Fallback: pick the first available NPC not already assigned to this triangle. + let fallback = role_to_npc + .values() + .find(|sid| !assigned_npcs.contains(sid)); + + if let Some(&fallback_sid) = fallback { + result.warnings.push(format!( + "Triangle {:?}: no NPC for role '{}' — assigned fallback StableId({})", + def.triangle_id, role_id.0, fallback_sid.0 + )); + role_assignments.insert(role_id.clone(), fallback_sid); + assigned_npcs.insert(fallback_sid); + } else { + result.warnings.push(format!( + "Triangle {:?}: no NPC available for role '{}' — skipping triangle", + def.triangle_id, role_id.0 + )); + assignment_ok = false; + break; + } } if !assignment_ok { @@ -753,12 +775,23 @@ pub fn tick_triangle_escalation( /// `Resolved`. D-089: resolution does not cascade to other triangles. pub fn apply_resolve_triangle( mut queue: ResMut, - mut triangles: Query<&mut TriangleState>, + mut triangles: Query<(Entity, &mut TriangleState)>, ) { let commands = queue.drain(); + if commands.is_empty() { + return; + } + + // Build index: O(N) scan once, then O(1) per resolve command. + // Avoids O(N*M) full scan when multiple resolves fire in one tick. + let id_to_entity: BTreeMap = triangles + .iter() + .map(|(entity, state)| (state.triangle_id.clone(), entity)) + .collect(); + for cmd in commands { - for mut state in triangles.iter_mut() { - if state.triangle_id == cmd.0 { + if let Some(&entity) = id_to_entity.get(&cmd.0) { + if let Ok((_, mut state)) = triangles.get_mut(entity) { state.phase = TrianglePhase::Resolved; tracing::info!( "Triangle {:?}: resolved (D-089, no cascade)", @@ -779,153 +812,16 @@ mod tests { // ----------------------------------------------------------------------- // #163 — RoleSchema tests + // (YAML roundtrip, validation, duplicates covered by integration tests + // in tests/template_schema.rs — only unique tests here) // ----------------------------------------------------------------------- - #[test] - fn role_schema_yaml_roundtrip() { - let yaml = r#" -role_id: bartender -required_traits: - - Social - - Honest -skill_focus: - - Persuasion - - Observation -relationship_constraints: - - with_role: waitstaff - kind: Colleague - required_trust: - min: 2 - max: 5 -routine_template: - - phase: morning - location: bar_counter - - phase: evening - location: bar_counter -"#; - - let schema: RoleSchema = serde_yaml::from_str(yaml).expect("deserialize"); - assert_eq!(schema.role_id, RoleId::new("bartender")); - assert_eq!(schema.required_traits.len(), 2); - assert_eq!(schema.skill_focus.len(), 2); - assert_eq!(schema.relationship_constraints.len(), 1); - assert_eq!(schema.routine_template.len(), 2); - - // Re-serialize and verify round-trip - let reserialized = serde_yaml::to_string(&schema).expect("serialize"); - let recovered: RoleSchema = serde_yaml::from_str(&reserialized).expect("re-deserialize"); - assert_eq!(schema, recovered); - } - - #[test] - fn role_schema_validate_catches_self_reference() { - let schema = RoleSchema { - role_id: RoleId::new("guard"), - required_traits: vec![], - skill_focus: vec![], - relationship_constraints: vec![RelationshipConstraint { - with_role: RoleId::new("guard"), - kind: RelationshipKind::Colleague, - required_trust: TrustRange { min: 0, max: 5 }, - }], - routine_template: vec![], - }; - - assert!(schema.validate().is_err()); - } - - #[test] - fn role_schema_validate_catches_bad_trust_range() { - let schema = RoleSchema { - role_id: RoleId::new("guard"), - required_traits: vec![], - skill_focus: vec![], - relationship_constraints: vec![RelationshipConstraint { - with_role: RoleId::new("supervisor"), - kind: RelationshipKind::Superior, - required_trust: TrustRange { min: 5, max: 2 }, - }], - routine_template: vec![], - }; - - assert!(schema.validate().is_err()); - } - - #[test] - fn role_schema_valid_schema_passes() { - let schema = RoleSchema { - role_id: RoleId::new("technician"), - required_traits: vec![PersonalityTrait::Curious], - skill_focus: vec![Skill::Technical], - relationship_constraints: vec![RelationshipConstraint { - with_role: RoleId::new("supervisor"), - kind: RelationshipKind::Subordinate, - required_trust: TrustRange { min: 1, max: 7 }, - }], - routine_template: vec![TemplateRoutineEntry { - phase: "morning".into(), - location: "workshop".into(), - activity: None, - }], - }; - - assert!(schema.validate().is_ok()); - } - - #[test] - fn validate_role_schemas_catches_duplicates() { - let schemas = vec![ - RoleSchema { - role_id: RoleId::new("guard"), - required_traits: vec![], - skill_focus: vec![], - relationship_constraints: vec![], - routine_template: vec![], - }, - RoleSchema { - role_id: RoleId::new("guard"), - required_traits: vec![], - skill_focus: vec![], - relationship_constraints: vec![], - routine_template: vec![], - }, - ]; - - let result = validate_role_schemas_no_duplicate_ids(&schemas); - assert!(result.is_err()); - assert!(result.unwrap_err().contains("guard")); - } - // ----------------------------------------------------------------------- // #164 — SpaceSpec tests + // (YAML roundtrip, min>max covered by integration tests — + // zero_min and valid_passes are unique) // ----------------------------------------------------------------------- - #[test] - fn space_spec_yaml_roundtrip() { - let yaml = r#" -tile_count_min: 30 -tile_count_max: 80 -sightline_zones: - - name: bar_counter - radius: 4 - - name: back_room - radius: 2 -privacy_level: SemiPrivate -traffic_pattern: Destination -"#; - - let spec: SpaceSpec = serde_yaml::from_str(yaml).expect("deserialize"); - assert_eq!(spec.tile_count_min, 30); - assert_eq!(spec.tile_count_max, 80); - assert_eq!(spec.sightline_zones.len(), 2); - assert_eq!(spec.privacy_level, PrivacyLevel::SemiPrivate); - assert_eq!(spec.traffic_pattern, TrafficPattern::Destination); - - let reserialized = serde_yaml::to_string(&spec).expect("serialize"); - let recovered: SpaceSpec = serde_yaml::from_str(&reserialized).expect("re-deserialize"); - assert_eq!(spec, recovered); - } - #[test] fn space_spec_validate_catches_min_gt_max() { let spec = SpaceSpec { @@ -970,21 +866,10 @@ traffic_pattern: Destination // ----------------------------------------------------------------------- // #165 — Single-ownership model tests + // (TemplateId determinism covered by integration tests — + // serialization roundtrips are unique) // ----------------------------------------------------------------------- - #[test] - fn template_id_deterministic() { - let id1 = TemplateId::from_seed_and_slug(42, "bar_grill"); - let id2 = TemplateId::from_seed_and_slug(42, "bar_grill"); - assert_eq!(id1, id2); - - let id3 = TemplateId::from_seed_and_slug(42, "dock_office"); - assert_ne!(id1, id3); - - let id4 = TemplateId::from_seed_and_slug(99, "bar_grill"); - assert_ne!(id1, id4); - } - #[test] fn template_ownership_serialize_roundtrip() { let ownership = TemplateOwnership { @@ -1035,6 +920,8 @@ traffic_pattern: Destination // ----------------------------------------------------------------------- // #106 — Triangle definition schema tests + // (YAML roundtrip, validation, D-087 T1 covered by integration tests — + // order-independence and passive tension are unique) // ----------------------------------------------------------------------- #[test] @@ -1058,98 +945,6 @@ traffic_pattern: Destination assert_ne!(id1, id3); } - #[test] - fn triangle_def_yaml_roundtrip() { - let yaml = r#" -triangle_id: 12345 -roles: - - smuggler - - detective - - informant -conflict_type: SecretExposure -interest_axes: - - Secret - - InformationInventory - - Relationships -relationship_constraints: [] -"#; - - let def: TriangleDef = serde_yaml::from_str(yaml).expect("deserialize"); - assert_eq!(def.triangle_id, TriangleId(12345)); - assert_eq!(def.roles[0], RoleId::new("smuggler")); - assert_eq!(def.conflict_type, ConflictType::SecretExposure); - assert_eq!(def.interest_axes[0], NpcAxis::Secret); - - let reserialized = serde_yaml::to_string(&def).expect("serialize"); - let recovered: TriangleDef = serde_yaml::from_str(&reserialized).expect("re-deserialize"); - assert_eq!(def, recovered); - } - - #[test] - fn triangle_def_validate_catches_duplicate_roles() { - let def = TriangleDef { - triangle_id: TriangleId(1), - roles: [ - RoleId::new("guard"), - RoleId::new("guard"), - RoleId::new("prisoner"), - ], - conflict_type: ConflictType::AuthorityChallenge, - interest_axes: [NpcAxis::Tolerance, NpcAxis::Want, NpcAxis::Contentment], - relationship_constraints: vec![], - }; - - assert!(def.validate().is_err()); - } - - #[test] - fn triangle_def_valid_passes() { - let def = TriangleDef { - triangle_id: TriangleId::from_seed_and_roles( - 42, - &[ - RoleId::new("smuggler"), - RoleId::new("detective"), - RoleId::new("informant"), - ], - ), - roles: [ - RoleId::new("smuggler"), - RoleId::new("detective"), - RoleId::new("informant"), - ], - conflict_type: ConflictType::LoyaltyConflict, - interest_axes: [NpcAxis::Want, NpcAxis::Secret, NpcAxis::Relationships], - relationship_constraints: vec![], - }; - - assert!(def.validate().is_ok()); - } - - #[test] - fn d087_t1_expressible() { - let def = TriangleDef { - triangle_id: TriangleId::from_seed_and_roles( - 1, - &[ - RoleId::new("kael_supplier"), - RoleId::new("smuggler_lead"), - RoleId::new("ring_enforcer"), - ], - ), - roles: [ - RoleId::new("kael_supplier"), - RoleId::new("smuggler_lead"), - RoleId::new("ring_enforcer"), - ], - conflict_type: ConflictType::ResourceCompetition, - interest_axes: [NpcAxis::Want, NpcAxis::Secret, NpcAxis::Tolerance], - relationship_constraints: vec![], - }; - - assert!(def.validate().is_ok()); - } - #[test] fn d087_passive_tension_expressible() { let def = TriangleDef { @@ -1459,227 +1254,10 @@ relationship_constraints: [] entity } - #[test] - fn escalation_simmering_to_active_at_expected_tick() { - let mut world = setup_escalation_world(); - - // Spawn 3 NPCs with different thresholds. NPC B has the lowest (20). - let npc_a_sid = StableId(10); - let npc_b_sid = StableId(11); - let npc_c_sid = StableId(12); - spawn_escalation_npc(&mut world, 10, 30); - spawn_escalation_npc(&mut world, 11, 20); // lowest threshold - spawn_escalation_npc(&mut world, 12, 50); - - let mut role_assignments = BTreeMap::new(); - role_assignments.insert(RoleId::new("a"), npc_a_sid); - role_assignments.insert(RoleId::new("b"), npc_b_sid); - role_assignments.insert(RoleId::new("c"), npc_c_sid); - - let triangle = world - .spawn(( - TriangleState { - triangle_id: TriangleId(100), - role_assignments, - tension: 10, // starting tension - phase: TrianglePhase::Simmering, - tension_rate: 3, // +3 per game-minute - template_id: TemplateId(1), - }, - ActiveSim, - )) - .id(); - - let mut schedule = Schedule::default(); - schedule.add_systems(tick_triangle_escalation); - - // Advance through 60 ticks. - // Escalation fires at tick % 10 == 0: ticks 10, 20, 30, 40, 50, 60. - // - // Tension progression (threshold = 20): - // tick 10: 10 + 3 = 13 (13 > 20? no) - // tick 20: 13 + 3 = 16 (16 > 20? no) - // tick 30: 16 + 3 = 19 (19 > 20? no) - // tick 40: 19 + 3 = 22 (22 > 20? yes → Active!) - // tick 50: 22 + 3 = 25 (Active, continues incrementing) - // tick 60: 25 + 3 = 28 - for tick in 1..=60 { - world.resource_mut::().tick = tick; - schedule.run(&mut world); - } - - let state = world.get::(triangle).unwrap(); - assert_eq!( - state.phase, - TrianglePhase::Active, - "triangle should transition to Active when tension exceeds lowest threshold" - ); - assert_eq!(state.tension, 28, "tension should be 28 after 6 game-minutes"); - - // Verify crisis event - let queue = world.resource::(); - assert_eq!(queue.events.len(), 1, "exactly one crisis event expected"); - assert_eq!(queue.events[0].triangle_id, TriangleId(100)); - assert_eq!( - queue.events[0].trigger_npc, npc_b_sid, - "trigger NPC should be the one with lowest threshold" - ); - assert_eq!(queue.events[0].tick, 40, "crisis should fire at tick 40"); - } - - #[test] - fn resolve_triangle_sets_phase_resolved() { - let mut world = bevy_ecs::world::World::new(); - world.init_resource::(); - - let triangle_id = TriangleId(100); - let triangle = world - .spawn(TriangleState { - triangle_id, - role_assignments: BTreeMap::new(), - tension: 50, - phase: TrianglePhase::Active, - tension_rate: 3, - template_id: TemplateId(1), - }) - .id(); - - world - .resource_mut::() - .push(ResolveTriangleCommand(triangle_id)); - - let mut schedule = Schedule::default(); - schedule.add_systems(apply_resolve_triangle); - schedule.run(&mut world); - - let state = world.get::(triangle).unwrap(); - assert_eq!( - state.phase, - TrianglePhase::Resolved, - "ResolveTriangleCommand must set phase to Resolved" - ); - // D-089: no cascade — only the targeted triangle is affected - assert_eq!(state.tension, 50, "tension should not change on resolve"); - } - - #[test] - fn dormant_triangle_not_escalated() { - let mut world = setup_escalation_world(); - world.resource_mut::().tick = 10; - - let triangle = world - .spawn(( - TriangleState { - triangle_id: TriangleId(100), - role_assignments: BTreeMap::new(), - tension: 10, - phase: TrianglePhase::Dormant, - tension_rate: 3, - template_id: TemplateId(1), - }, - ActiveSim, - )) - .id(); - - let mut schedule = Schedule::default(); - schedule.add_systems(tick_triangle_escalation); - schedule.run(&mut world); - - let state = world.get::(triangle).unwrap(); - assert_eq!( - state.tension, 10, - "Dormant triangle should not have tension incremented" - ); - assert_eq!(state.phase, TrianglePhase::Dormant); - } - - #[test] - fn resolved_triangle_not_escalated() { - let mut world = setup_escalation_world(); - world.resource_mut::().tick = 10; - - let triangle = world - .spawn(( - TriangleState { - triangle_id: TriangleId(100), - role_assignments: BTreeMap::new(), - tension: 50, - phase: TrianglePhase::Resolved, - tension_rate: 3, - template_id: TemplateId(1), - }, - ActiveSim, - )) - .id(); - - let mut schedule = Schedule::default(); - schedule.add_systems(tick_triangle_escalation); - schedule.run(&mut world); - - let state = world.get::(triangle).unwrap(); - assert_eq!( - state.tension, 50, - "Resolved triangle should not have tension incremented (D-089)" - ); - } - - #[test] - fn escalation_skips_non_game_minute_ticks() { - let mut world = setup_escalation_world(); - world.resource_mut::().tick = 7; // 7 % 10 != 0 - - let triangle = world - .spawn(( - TriangleState { - triangle_id: TriangleId(100), - role_assignments: BTreeMap::new(), - tension: 10, - phase: TrianglePhase::Simmering, - tension_rate: 3, - template_id: TemplateId(1), - }, - ActiveSim, - )) - .id(); - - let mut schedule = Schedule::default(); - schedule.add_systems(tick_triangle_escalation); - schedule.run(&mut world); - - let state = world.get::(triangle).unwrap(); - assert_eq!( - state.tension, 10, - "should not escalate on non-game-minute ticks" - ); - } - - #[test] - fn escalation_without_active_sim_marker_skipped() { - let mut world = setup_escalation_world(); - world.resource_mut::().tick = 10; - - // Triangle entity WITHOUT ActiveSim — should not be processed - let triangle = world - .spawn(TriangleState { - triangle_id: TriangleId(100), - role_assignments: BTreeMap::new(), - tension: 10, - phase: TrianglePhase::Simmering, - tension_rate: 3, - template_id: TemplateId(1), - }) - .id(); - - let mut schedule = Schedule::default(); - schedule.add_systems(tick_triangle_escalation); - schedule.run(&mut world); - - let state = world.get::(triangle).unwrap(); - assert_eq!( - state.tension, 10, - "triangle without ActiveSim should not be escalated (D-026)" - ); - } + // escalation_simmering_to_active, resolve, dormant_skip, resolved_skip, + // game-minute-only, active-sim-only, saturation, resolve-targeting — all + // covered by integration tests in tests/triangle_escalation.rs. + // Only active_triangle_continues_incrementing is unique here. #[test] fn active_triangle_continues_incrementing() { diff --git a/server/src/knowledge/types.rs b/server/src/knowledge/types.rs index 76bed1018..ef1e72e3d 100644 --- a/server/src/knowledge/types.rs +++ b/server/src/knowledge/types.rs @@ -18,6 +18,12 @@ use crate::simulation::movement::TilePosition; #[derive(Debug, Clone, Copy, PartialEq, Eq, PartialOrd, Ord, Hash, Serialize, Deserialize)] pub struct StableId(pub u64); +impl From for u64 { + fn from(id: StableId) -> Self { + id.0 + } +} + /// Typed fact identifier for non-entity knowledge. /// Format: "category.topic" (e.g., "contraband.ring_exists"). /// Lexicographic ordering in BTreeMap provides deterministic iteration. diff --git a/server/src/perception/observer/mod.rs b/server/src/perception/observer/mod.rs index 33ce07e07..f1b3de548 100644 --- a/server/src/perception/observer/mod.rs +++ b/server/src/perception/observer/mod.rs @@ -393,16 +393,7 @@ pub fn compute_observer_snapshot( let triangle_crisis_events = crisis_queue .drain() .into_iter() - .map(|e| TriangleCrisisEventWire { - triangle_id: e.triangle_id.0, - role_assignments: e - .role_assignments - .into_iter() - .map(|(role, sid)| (role.0, sid.0)) - .collect(), - trigger_npc_id: e.trigger_npc.0, - tick: e.tick, - }) + .map(TriangleCrisisEventWire::from) .collect(); buffer.snapshot = Some(ObserverSnapshot {