refactor(server): address PR #16 review feedback

Hoshe + Tyre review items:
- Use StableId consistently for wire entity_id (H4) across observer,
  observation, interpretation, and interaction systems
- Make NearbyInteractionBuffer.interactions private with take() (H1/H20)
- Add system ordering constraint for compute_nearby_interactions (H5)
- Panic on missing PlayerCharacter in input processing (H2)
- Remove redundant paused field from GameTime (Tyre8)
- Remove #[serde(default)] from nearby_interactions (H3)
- Change NearbyInteraction.distance from f32 to u32 (H8)
- Add sort stability for equal verb priorities (H6)
- Scope constants to pub(crate) (H7)
- Add debug_assert for last_observed_tick ordering (H10)
- Strengthen unregistered entity handling to debug_assert + error (H11)
- Document fractional tick accumulation (Tyre9)
- Extract collect_remembered_entities helper (Tyre2/H17)
- Add half_rate_no_drift_over_10000_frames test (H14)
- Add mid-range and deterministic sort tests (H15)
- Add fixture version assertion (H16)
- Regenerate msgpack fixtures for wire format changes

146 unit + 19 integration tests pass, zero clippy warnings.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
This commit is contained in:
2026-02-12 20:14:35 +01:00
co-authored by Claude Opus 4.6
parent 458be0f621
commit 9943684f2c
19 changed files with 232 additions and 136 deletions
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
+2 -2
View File
@@ -102,7 +102,6 @@ pub fn generate_snapshot(
day: time.day(),
time_of_day: time.time_of_day_minutes(),
day_phase: time.day_phase(),
paused: time.paused(),
tick_rate: time.tick_rate,
};
@@ -214,7 +213,8 @@ impl Plugin for BridgePlugin {
crate::perception::observation::emit_observation_events
.after(crate::perception::observer::compute_observer_snapshot),
send_bridge_snapshot
.after(crate::perception::observer::compute_observer_snapshot),
.after(crate::perception::observer::compute_observer_snapshot)
.after(crate::simulation::interaction::compute_nearby_interactions),
),
);
tracing::debug!("BridgePlugin initialized");
+3 -7
View File
@@ -32,7 +32,6 @@ pub struct ObserverSnapshot {
pub visible_tiles: Vec<VisibleTile>,
/// Entities within interaction range with available verbs (D-060, #404).
/// Sorted by distance (nearest first). v0.1 client reads verbs[0] on the nearest entity.
#[serde(default)]
pub nearby_interactions: Vec<NearbyInteraction>,
}
@@ -46,10 +45,7 @@ pub struct GameTime {
pub time_of_day: u64,
/// Current day phase (Morning/Afternoon/Evening/Night)
pub day_phase: DayPhase,
/// Whether simulation is paused (tick_rate == Paused)
pub paused: bool,
/// Current tick rate state (D-052)
#[serde(default)]
/// Current tick rate state (D-052). Client derives paused from TickRate::Paused.
pub tick_rate: TickRate,
}
@@ -156,8 +152,8 @@ pub struct NearbyInteraction {
pub entity_id: u64,
/// Entity type for client-side verb display
pub entity_type: EntityKind,
/// Manhattan distance from player
pub distance: f32,
/// Manhattan distance from player (integer tiles)
pub distance: u32,
/// Available verbs sorted by priority (index 0 = highest priority)
pub verbs: Vec<VerbOption>,
}
+4 -2
View File
@@ -79,14 +79,16 @@ pub fn process_knowledge_events(
if let Some(stable_id) = registry.to_stable(target) {
observer_kg.observe_entity(stable_id, position, event.tick);
} else {
tracing::warn!("DirectObservation target {:?} not in EntityRegistry", target);
debug_assert!(false, "DirectObservation target {:?} not in EntityRegistry", target);
tracing::error!("DirectObservation target {:?} not in EntityRegistry", target);
}
}
KnowledgeEventType::LeftLOS { target } => {
if let Some(stable_id) = registry.to_stable(target) {
observer_kg.observe_entity_leaving_los(&stable_id, event.tick);
} else {
tracing::warn!("LeftLOS target {:?} not in EntityRegistry", target);
debug_assert!(false, "LeftLOS target {:?} not in EntityRegistry", target);
tracing::error!("LeftLOS target {:?} not in EntityRegistry", target);
}
}
}
+27 -27
View File
@@ -110,39 +110,39 @@ pub fn generate_observation_events(
continue;
}
let entity = Entity::from_bits(visible.entity_id);
// Convert wire StableId back to bevy Entity via registry
let stable_id = StableId(visible.entity_id);
let Some(entity) = registry.to_entity(&stable_id) else {
continue;
};
// Check if this is a new entity (not in observer's knowledge graph)
if let Some(stable_id) = registry.to_stable(entity) {
if !observer_kg.knows_entity(&stable_id) {
// Reconstruct tile position from render coords
let tile_pos = TilePosition::from_render_coords(visible.x, visible.y, visible.z);
event_queue.push(ObservationEvent {
tick: time.tick,
trigger: ObservationTrigger::NewEntity {
entity: stable_id,
location: tile_pos,
},
observer: observer_entity,
});
}
if !observer_kg.knows_entity(&stable_id) {
// Reconstruct tile position from render coords
let tile_pos = TilePosition::from_render_coords(visible.x, visible.y, visible.z);
event_queue.push(ObservationEvent {
tick: time.tick,
trigger: ObservationTrigger::NewEntity {
entity: stable_id,
location: tile_pos,
},
observer: observer_entity,
});
}
// Check routine deviation: visible NPC not at expected location
if let Ok((actual_pos, routine)) = npc_query.get(entity) {
if let Some(expected_pos) = routine.expected_location(current_phase) {
if *actual_pos != expected_pos {
if let Some(stable_id) = registry.to_stable(entity) {
event_queue.push(ObservationEvent {
tick: time.tick,
trigger: ObservationTrigger::RoutineDeviation {
npc: stable_id,
expected: expected_pos,
actual: *actual_pos,
},
observer: observer_entity,
});
}
event_queue.push(ObservationEvent {
tick: time.tick,
trigger: ObservationTrigger::RoutineDeviation {
npc: stable_id,
expected: expected_pos,
actual: *actual_pos,
},
observer: observer_entity,
});
}
}
}
@@ -157,8 +157,8 @@ pub fn generate_observation_events(
continue;
};
// Skip if currently visible
if visible_npc_bits.contains(&entity.to_bits()) {
// Skip if currently visible (visible_npc_bits contains wire StableId values)
if visible_npc_bits.contains(&stable_id.0) {
continue;
}
+10 -8
View File
@@ -33,8 +33,8 @@ pub fn emit_observation_events(
return;
};
// Collect visible entity IDs (Vec — linear search is faster at 5-20 entities)
let visible_entity_ids: Vec<u64> = snapshot
// Collect visible stable IDs (wire entity_id is now StableId, not Entity::to_bits())
let visible_stable_ids: Vec<u64> = snapshot
.entities
.iter()
.filter(|e| !matches!(e.kind, EntityKind::Player))
@@ -47,8 +47,11 @@ pub fn emit_observation_events(
continue;
}
// Look up the bevy Entity from the entity_id (which is Entity::to_bits())
let entity = Entity::from_bits(visible.entity_id);
// Convert wire StableId back to bevy Entity via registry
let stable_id = crate::knowledge::types::StableId(visible.entity_id);
let Some(entity) = registry.to_entity(&stable_id) else {
continue;
};
// Get tile position for knowledge tracking
if let Ok(pos) = entity_positions.get(entity) {
@@ -69,10 +72,9 @@ pub fn emit_observation_events(
continue;
}
// Check if this entity is still visible in the current snapshot
if let Some(entity) = registry.to_entity(stable_id) {
let entity_bits = entity.to_bits();
if !visible_entity_ids.contains(&entity_bits) {
// Check if this entity's StableId is still visible in the current snapshot
if !visible_stable_ids.contains(&stable_id.0) {
if let Some(entity) = registry.to_entity(stable_id) {
event_queue.push(KnowledgeEvent {
observer: observer_entity,
tick: time.tick,
+77 -58
View File
@@ -23,7 +23,7 @@ pub fn compute_observer_snapshot(
time: Res<SimulationTime>,
walkability: Res<WalkabilityMap>,
registry: Res<EntityRegistry>,
interaction_buffer: Res<NearbyInteractionBuffer>,
mut interaction_buffer: ResMut<NearbyInteractionBuffer>,
observer_query: Query<(&TilePosition, Option<&Facing>, &KnowledgeGraph), With<PlayerCharacter>>,
all_entities: Query<(
Entity,
@@ -115,9 +115,13 @@ pub fn compute_observer_snapshot(
RelationshipState::Unknown
};
visible_entity_bits.insert(entity.to_bits());
let wire_id = registry
.to_stable(entity)
.map(|sid| sid.0)
.unwrap_or_else(|| entity.to_bits());
visible_entity_bits.insert(wire_id);
entities.push(VisibleEntity {
entity_id: entity.to_bits(),
entity_id: wire_id,
x: rx,
y: ry,
z: rz,
@@ -129,65 +133,20 @@ pub fn compute_observer_snapshot(
}
// Step 6: Add remembered entities from knowledge graph (#366)
// Entities the observer knows about but can't currently see.
for (stable_id, knowledge) in observer_kg.known_entities_iter() {
// Skip if currently visible (already in the entity list)
if let Some(entity) = registry.to_entity(stable_id) {
if visible_entity_bits.contains(&entity.to_bits()) {
continue;
}
}
// Skip if no known position (never directly observed)
let Some(position) = knowledge.last_known_position else {
continue;
};
// Skip if remembered position is on a different z-level than the observer
if position.z != z {
continue;
}
// Skip if the remembered tile is currently visible — if the player
// can see the tile and the entity isn't there, don't show a ghost.
if visible_positions.contains(&(position.x, position.y)) {
continue;
}
// Direct confidence means the entity should be in LOS — if it isn't,
// that's a transient data inconsistency. Skip rather than show a ghost.
if knowledge.confidence == KnowledgeConfidence::Direct {
continue;
}
let (rx, ry, rz) = position.to_render_coords();
let age_ticks = time.tick.saturating_sub(knowledge.last_observed_tick);
let entity_id = registry
.to_entity(stable_id)
.map(|e| e.to_bits())
.unwrap_or(stable_id.0);
entities.push(VisibleEntity {
entity_id,
x: rx,
y: ry,
z: rz,
kind: EntityKind::Npc, // Remembered entities are NPCs (only NPCs are tracked)
visibility: VisibilitySector::Forward, // Not meaningful for remembered entities
relationship: knowledge.relationship,
observation: EntityVisibility::Remembered {
confidence: knowledge.confidence,
age_ticks,
},
});
}
collect_remembered_entities(
observer_kg,
&visible_entity_bits,
&visible_positions,
z,
time.tick,
&mut entities,
);
// Step 7: Build GameTime from SimulationTime
let game_time = GameTime {
day: time.day(),
time_of_day: time.time_of_day_minutes(),
day_phase: time.day_phase(),
paused: time.paused(),
tick_rate: time.tick_rate,
};
@@ -207,10 +166,70 @@ pub fn compute_observer_snapshot(
player_facing: facing,
entities,
visible_tiles,
nearby_interactions: interaction_buffer.interactions.clone(),
nearby_interactions: interaction_buffer.take(),
});
}
/// Collect remembered entities from the knowledge graph — entities the observer
/// knows about but can't currently see. Filters out: already-visible entities,
/// entities without known positions, wrong z-level, visible-tile ghosts, and
/// transient Direct-confidence inconsistencies.
fn collect_remembered_entities(
observer_kg: &KnowledgeGraph,
visible_ids: &HashSet<u64>,
visible_positions: &HashSet<(i32, i32)>,
observer_z: i32,
current_tick: u64,
entities: &mut Vec<VisibleEntity>,
) {
for (stable_id, knowledge) in observer_kg.known_entities_iter() {
if visible_ids.contains(&stable_id.0) {
continue;
}
let Some(position) = knowledge.last_known_position else {
continue;
};
if position.z != observer_z {
continue;
}
// Tile is visible but entity isn't there — player knows it moved
if visible_positions.contains(&(position.x, position.y)) {
continue;
}
// Direct confidence = should be in LOS; skip transient inconsistency
if knowledge.confidence == KnowledgeConfidence::Direct {
continue;
}
let (rx, ry, rz) = position.to_render_coords();
debug_assert!(
knowledge.last_observed_tick <= current_tick,
"last_observed_tick {} > current tick {}",
knowledge.last_observed_tick,
current_tick,
);
let age_ticks = current_tick.saturating_sub(knowledge.last_observed_tick);
entities.push(VisibleEntity {
entity_id: stable_id.0,
x: rx,
y: ry,
z: rz,
kind: EntityKind::Npc,
visibility: VisibilitySector::Forward,
relationship: knowledge.relationship,
observation: EntityVisibility::Remembered {
confidence: knowledge.confidence,
age_ticks,
},
});
}
}
#[cfg(test)]
mod tests {
use super::*;
@@ -386,7 +405,7 @@ mod tests {
snapshot.game_time.day_phase,
crate::simulation::time::DayPhase::Evening
);
assert!(snapshot.game_time.paused);
assert_eq!(snapshot.game_time.tick_rate, crate::simulation::time::TickRate::Paused);
}
#[test]
+12 -13
View File
@@ -102,17 +102,16 @@ fn apply_move(
dx: i32,
dy: i32,
) {
if let Ok((entity, pos)) = player_query.single() {
commands.entity(entity).insert(MoveIntent {
target: TilePosition::new(pos.x + dx, pos.y + dy, pos.z),
});
// Update facing direction based on movement (D-015 vision cone)
commands
.entity(entity)
.insert(Facing(facing_from_delta(dx, dy)));
} else {
tracing::warn!("No player entity found for movement input");
}
let (entity, pos) = player_query
.single()
.expect("PlayerCharacter entity must exist when processing input");
commands.entity(entity).insert(MoveIntent {
target: TilePosition::new(pos.x + dx, pos.y + dy, pos.z),
});
// Update facing direction based on movement (D-015 vision cone)
commands
.entity(entity)
.insert(Facing(facing_from_delta(dx, dy)));
}
#[cfg(test)]
@@ -220,7 +219,8 @@ mod tests {
}
#[test]
fn process_input_no_player_no_panic() {
#[should_panic(expected = "PlayerCharacter entity must exist")]
fn process_input_no_player_panics() {
let mut world = bevy_ecs::world::World::new();
world.insert_resource(InputQueue::default());
world.insert_resource(SimulationTime::default());
@@ -232,7 +232,6 @@ mod tests {
let mut schedule = bevy_ecs::schedule::Schedule::default();
schedule.add_systems(process_player_input);
// Should not panic
schedule.run(&mut world);
}
+73 -9
View File
@@ -11,8 +11,8 @@ use crate::npc::Npc;
use crate::simulation::movement::{PlayerCharacter, TilePosition};
/// Interaction range thresholds (Manhattan distance, same z-level)
pub const CLOSE_RANGE: u32 = 2;
pub const MID_RANGE: u32 = 5;
pub(crate) const CLOSE_RANGE: u32 = 2;
pub(crate) const MID_RANGE: u32 = 5;
/// Component marking an entity as having available interactions.
/// Attached to NPCs and examinable objects by the world setup or content loader.
@@ -124,13 +124,18 @@ pub fn compute_nearby_interactions(
continue;
}
// Sort by priority (lower number = higher priority)
verbs.sort_by_key(|v| v.priority);
// Sort by priority (lower = higher), then by kind discriminant for stability
verbs.sort_by_key(|v| (v.priority, v.kind as u8));
let wire_id = registry
.to_stable(entity)
.map(|sid| sid.0)
.unwrap_or_else(|| entity.to_bits());
buffer.interactions.push(NearbyInteraction {
entity_id: entity.to_bits(),
entity_id: wire_id,
entity_type,
distance: distance as f32,
distance,
verbs,
});
}
@@ -138,13 +143,22 @@ pub fn compute_nearby_interactions(
// Sort interactions by distance (nearest first)
buffer
.interactions
.sort_by(|a, b| a.distance.partial_cmp(&b.distance).unwrap());
.sort_by_key(|a| a.distance);
}
/// Buffer for nearby interaction results, consumed by snapshot generation
/// Buffer for nearby interaction results, consumed by snapshot generation.
/// Field is private — use `take()` to drain results into the snapshot.
#[derive(Resource, Debug, Default)]
pub struct NearbyInteractionBuffer {
pub interactions: Vec<NearbyInteraction>,
interactions: Vec<NearbyInteraction>,
}
impl NearbyInteractionBuffer {
/// Drain and return interactions, leaving the buffer empty.
/// Avoids cloning per-frame; snapshot owns the Vec after take.
pub fn take(&mut self) -> Vec<NearbyInteraction> {
std::mem::take(&mut self.interactions)
}
}
#[cfg(test)]
@@ -329,4 +343,54 @@ mod tests {
let buffer = world.resource::<NearbyInteractionBuffer>();
assert!(buffer.interactions.is_empty());
}
#[test]
fn poi_npc_at_mid_range_gets_observe_only() {
// POI priority flip only applies at close range — mid range always Observe-only
let mut world = setup_world();
let mut registry = EntityRegistry::new(0);
let npc = world
.spawn((Npc, TilePosition::new(5, 9, 0), Interactable))
.id();
let npc_sid = registry.register(npc);
let mut kg = KnowledgeGraph::new();
kg.observe_entity(npc_sid, TilePosition::new(5, 9, 0), 50);
kg.set_relationship(&npc_sid, RelationshipState::PersonOfInterest);
world.spawn((PlayerCharacter, TilePosition::new(5, 5, 0), kg));
world.insert_resource(registry);
let mut schedule = bevy_ecs::schedule::Schedule::default();
schedule.add_systems(compute_nearby_interactions);
schedule.run(&mut world);
let buffer = world.resource::<NearbyInteractionBuffer>();
assert_eq!(buffer.interactions.len(), 1);
assert_eq!(buffer.interactions[0].verbs.len(), 1);
assert_eq!(buffer.interactions[0].verbs[0].kind, VerbKind::ExamineNpc);
}
#[test]
fn equidistant_npcs_sorted_deterministically() {
let mut world = setup_world();
// Two NPCs at equal distance (1 tile each)
world.spawn((
PlayerCharacter,
TilePosition::new(5, 5, 0),
KnowledgeGraph::new(),
));
world.spawn((Npc, TilePosition::new(6, 5, 0), Interactable));
world.spawn((Npc, TilePosition::new(4, 5, 0), Interactable));
let mut schedule = bevy_ecs::schedule::Schedule::default();
schedule.add_systems(compute_nearby_interactions);
schedule.run(&mut world);
let buffer = world.resource::<NearbyInteractionBuffer>();
assert_eq!(buffer.interactions.len(), 2);
// Both at distance 1 — order should be stable across runs
assert_eq!(buffer.interactions[0].distance, buffer.interactions[1].distance);
}
}
+20
View File
@@ -92,6 +92,11 @@ impl SimulationTime {
/// Advance the simulation tick based on current tick rate.
/// Full: +1 every frame. Half: +1 every 2 frames. Paused: no advance.
///
/// Uses fractional accumulation: each frame adds `tick_rate.scale()` to an
/// internal accumulator. When it reaches 1.0, a tick fires and the accumulator
/// subtracts 1.0. This ensures Half rate produces exactly N/2 ticks over N
/// frames with no floating-point drift (0.5 is exactly representable in f32).
pub fn advance_tick(mut time: ResMut<SimulationTime>) {
let scale = time.tick_rate.scale();
if scale <= 0.0 {
@@ -193,4 +198,19 @@ mod tests {
let time = SimulationTime { tick: 3 * MINUTES_PER_DAY * TICKS_PER_GAME_MINUTE + 100, ..Default::default() };
assert_eq!(time.day(), 3);
}
#[test]
fn half_rate_no_drift_over_10000_frames() {
let mut world = bevy_ecs::world::World::new();
world.insert_resource(SimulationTime { tick_rate: TickRate::Half, ..Default::default() });
let mut schedule = bevy_ecs::schedule::Schedule::default();
schedule.add_systems(advance_tick);
for _ in 0..10_000 {
schedule.run(&mut world);
}
// 10,000 frames at Half rate (0.5) should yield exactly 5,000 ticks
assert_eq!(world.resource::<SimulationTime>().tick, 5_000);
}
}
-1
View File
@@ -39,7 +39,6 @@ fn snapshot_roundtrip_over_unix_socket() {
day: 0,
time_of_day: 0,
day_phase: DayPhase::Morning,
paused: false,
tick_rate: TickRate::Full,
},
player_facing: FacingDirection::North,
-1
View File
@@ -25,7 +25,6 @@ fn snapshot_roundtrip_over_tcp() {
day: 0,
time_of_day: 0,
day_phase: DayPhase::Morning,
paused: false,
tick_rate: TickRate::Full,
},
player_facing: FacingDirection::North,
+1 -1
View File
@@ -68,7 +68,7 @@ fn player_moves_north_through_full_pipeline() {
// v2 fields populated
assert_eq!(snapshot.game_time.day, 0);
assert_eq!(snapshot.game_time.day_phase, settled_reach_server::simulation::time::DayPhase::Morning);
assert!(!snapshot.game_time.paused);
assert_eq!(snapshot.game_time.tick_rate, settled_reach_server::simulation::time::TickRate::Full);
let player_entity = &snapshot.entities[0];
// Player started at (16, 16, 0), moved north (y-1) to (16, 15, 0)
-2
View File
@@ -24,7 +24,6 @@ fn fixture_snapshot(tick: u64, entities: Vec<VisibleEntity>) -> ObserverSnapshot
day: 0,
time_of_day: 0,
day_phase: DayPhase::Morning,
paused: false,
tick_rate: TickRate::Full,
},
player_facing: FacingDirection::North,
@@ -158,7 +157,6 @@ fn generate_msgpack_fixtures() {
day: 1,
time_of_day: 720,
day_phase: DayPhase::Evening,
paused: false,
tick_rate: TickRate::Full,
},
player_facing: FacingDirection::Southeast,
+3 -5
View File
@@ -13,7 +13,6 @@ fn test_snapshot(tick: u64, entities: Vec<VisibleEntity>) -> ObserverSnapshot {
day: 0,
time_of_day: 0,
day_phase: DayPhase::Morning,
paused: false,
tick_rate: TickRate::Full,
},
player_facing: FacingDirection::North,
@@ -127,8 +126,9 @@ fn all_fixtures_deserialize() {
let bytes = fs::read(&path).unwrap_or_else(|_| panic!("read fixture {}", name));
if name.starts_with("snapshot") {
rmp_serde::from_slice::<ObserverSnapshot>(&bytes)
let snap = rmp_serde::from_slice::<ObserverSnapshot>(&bytes)
.unwrap_or_else(|e| panic!("deserialize snapshot fixture {}: {}", name, e));
assert_eq!(snap.version, 4, "fixture {} has wrong version", name);
} else if name.starts_with("input_batch") {
rmp_serde::from_slice::<Vec<PlayerInput>>(&bytes)
.unwrap_or_else(|e| panic!("deserialize batch input fixture {}: {}", name, e));
@@ -186,7 +186,6 @@ fn snapshot_v2_fields_roundtrip() {
day: 3,
time_of_day: 720,
day_phase: DayPhase::Evening,
paused: true,
tick_rate: TickRate::Paused,
},
player_facing: FacingDirection::Southeast,
@@ -224,7 +223,7 @@ fn snapshot_v2_fields_roundtrip() {
assert_eq!(decoded.game_time.day, 3);
assert_eq!(decoded.game_time.time_of_day, 720);
assert_eq!(decoded.game_time.day_phase, DayPhase::Evening);
assert!(decoded.game_time.paused);
assert_eq!(decoded.game_time.tick_rate, TickRate::Paused);
assert_eq!(decoded.player_facing, FacingDirection::Southeast);
assert_eq!(decoded.visible_tiles.len(), 2);
assert_eq!(decoded.visible_tiles[0].visibility, VisibilitySector::Forward);
@@ -254,7 +253,6 @@ fn all_facing_direction_variants_roundtrip() {
day: 0,
time_of_day: 0,
day_phase: DayPhase::Morning,
paused: false,
tick_rate: TickRate::Full,
},
player_facing: dir,