refactor(simulation): address PR #72 review suggestions

- 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<TriangleId, Entity> 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 <noreply@anthropic.com>
This commit is contained in:
2026-02-25 23:35:23 +01:00
co-authored by Claude Opus 4.6
parent 3a555a2eeb
commit d808d95659
4 changed files with 92 additions and 502 deletions
+15
View File
@@ -699,6 +699,21 @@ pub struct TriangleCrisisEventWire {
pub tick: u64,
}
impl From<crate::content::template::TriangleCrisisEvent> 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 {
+70 -492
View File
@@ -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<RoleId> 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<TriangleId> 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<StableId> =
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<ResolveTriangleQueue>,
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<TriangleId, Entity> = 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::<SimulationTime>().tick = tick;
schedule.run(&mut world);
}
let state = world.get::<TriangleState>(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::<TriangleCrisisEventQueue>();
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::<ResolveTriangleQueue>();
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::<ResolveTriangleQueue>()
.push(ResolveTriangleCommand(triangle_id));
let mut schedule = Schedule::default();
schedule.add_systems(apply_resolve_triangle);
schedule.run(&mut world);
let state = world.get::<TriangleState>(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::<SimulationTime>().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::<TriangleState>(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::<SimulationTime>().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::<TriangleState>(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::<SimulationTime>().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::<TriangleState>(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::<SimulationTime>().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::<TriangleState>(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() {
+6
View File
@@ -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<StableId> 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.
+1 -10
View File
@@ -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 {