From 7597e3165edb18ab54d33a73cc0421c6ac0f72d4 Mon Sep 17 00:00:00 2001 From: Jeroen Schweitzer Date: Sun, 28 Jun 2026 10:24:30 +0200 Subject: [PATCH] =?UTF-8?q?fix(simulation):=20address=20PR=20#171=20review?= =?UTF-8?q?=20=E2=80=94=20all=20findings=20processed=20(T-987)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Every review finding fixed (no non-blocking parking lot): - shell.rs: debug_assert the D-110 floor-range i8 invariant (H1); clarify the ground_offset fallback comment (T1) and the roof_z None-case (T3); clarify the sub-chunk clip test comment (T4); add a test for the elevated/no-ground-floor fallback (H2). - scale.rs: add CHUNKS_PER_BLOCK (= BLOCK_M/CHUNK_M) + compile-time asserts; used by shell.rs fill_chunk's sub_chunk bounds assert (T5). - gen_queue.rs: document why FillChunk's Vec needs no Box (large_enum_variant non-issue) (T2). clippy --all-targets -D warnings clean; 1572 lib tests pass (+1). Co-Authored-By: Claude Opus 4.8 (1M context) --- server/src/atlas/gen_queue.rs | 3 +++ server/src/atlas/scale.rs | 5 +++++ server/src/atlas/shell.rs | 40 +++++++++++++++++++++++++++++++---- 3 files changed, 44 insertions(+), 4 deletions(-) diff --git a/server/src/atlas/gen_queue.rs b/server/src/atlas/gen_queue.rs index 67dc1a6ed..ef58c3382 100644 --- a/server/src/atlas/gen_queue.rs +++ b/server/src/atlas/gen_queue.rs @@ -133,6 +133,9 @@ pub enum GenWorkItem { sub_chunk: (u8, u8), /// Covering block's building tags, pre-resolved from the cached /// `QuarterWorldState`. Empty for an open/un-built block (→ empty shell). + /// A `Vec` is three words on the stack regardless of element size (the tags + /// live behind the pointer), so — unlike `AnalyzeBody`'s boxed `BodyParams` — + /// this variant needs no `Box` to stay clippy `large_enum_variant`-clean. block_tags: Vec, }, } diff --git a/server/src/atlas/scale.rs b/server/src/atlas/scale.rs index 21dd624e8..9b0b4d08c 100644 --- a/server/src/atlas/scale.rs +++ b/server/src/atlas/scale.rs @@ -45,6 +45,9 @@ pub const REGION_M: i32 = 204_800; /// Voxels per chunk edge (64 m / 1 m). A chunk is 64×64 voxels. pub const VOXELS_PER_CHUNK: i32 = CHUNK_M / VOXEL_M; +/// Chunks per block edge (128 m / 64 m = 2). A 128 m block is 2×2 chunks — the +/// sub-chunk grid the on-demand fill (D-230) addresses within a block. +pub const CHUNKS_PER_BLOCK: i32 = BLOCK_M / CHUNK_M; /// Chunks per district edge (2 048 m / 64 m = 32 = `1 << 5`). pub const CHUNKS_PER_DISTRICT: i32 = DISTRICT_M / CHUNK_M; /// `log2(CHUNKS_PER_DISTRICT)` — chunk→district is an arithmetic shift (floors @@ -55,6 +58,8 @@ pub const CHUNK_DISTRICT_SHIFT: u32 = 5; pub const DISTRICTS_PER_REGION: i32 = REGION_M / DISTRICT_M; // Compile-time ladder invariants — the build fails if a rung stops nesting. +const _: () = assert!(CHUNKS_PER_BLOCK == 2); +const _: () = assert!(CHUNKS_PER_BLOCK * CHUNK_M == BLOCK_M); const _: () = assert!(CHUNKS_PER_DISTRICT == 1 << CHUNK_DISTRICT_SHIFT); const _: () = assert!(CHUNKS_PER_DISTRICT * CHUNK_M == DISTRICT_M); const _: () = assert!(DISTRICTS_PER_REGION * DISTRICT_M == REGION_M); diff --git a/server/src/atlas/shell.rs b/server/src/atlas/shell.rs index 910100e16..bd61bbfd9 100644 --- a/server/src/atlas/shell.rs +++ b/server/src/atlas/shell.rs @@ -53,7 +53,7 @@ use std::collections::BTreeMap; use serde::{Deserialize, Serialize}; -use crate::atlas::scale::{CHUNK_M, VOXELS_PER_CHUNK}; +use crate::atlas::scale::{CHUNKS_PER_BLOCK, CHUNK_M, VOXELS_PER_CHUNK}; use crate::simulation::generator::{BuildingPropertyTag, TileRect}; /// One structural shell voxel material (D-230). @@ -137,6 +137,10 @@ pub fn fill_chunk( sub_chunk: (u8, u8), block_tags: &[BuildingPropertyTag], ) -> FilledChunk { + debug_assert!( + (sub_chunk.0 as i32) < CHUNKS_PER_BLOCK && (sub_chunk.1 as i32) < CHUNKS_PER_BLOCK, + "sub_chunk {sub_chunk:?} outside the block's {CHUNKS_PER_BLOCK}×{CHUNKS_PER_BLOCK} chunk grid" + ); let mut voxels: BTreeMap = BTreeMap::new(); // Block-local tile range covered by this 64 m sub-chunk quadrant. @@ -176,6 +180,14 @@ fn shell_derive_into( let footprint = &tag.footprint; let extent = &tag.extent; + // D-110 floor indices must fit i8 so the top-floor comparison and the roof + // derivation below cannot wrap. The generator caps floor counts well under this; + // the assert pins the invariant so a future change can't silently drop the roof. + debug_assert!( + extent.base_floor as i16 + extent.floor_count as i16 - 1 <= i8::MAX as i16, + "building floor range exceeds i8 — roof derivation would wrap" + ); + // Footprint block-local tile span (inclusive lo, exclusive hi). let fp_lo_x = footprint.origin.0 as i32; let fp_lo_y = footprint.origin.1 as i32; @@ -192,8 +204,10 @@ fn shell_derive_into( } // Quarter-ground z origin (D-110): subtract the ground floor's building-relative - // base so floor 0 bottom lands at z = 0. If the building has no floor 0 (all - // basement / all elevated — unusual), fall back to the building bottom (= 0). + // base so floor 0 bottom lands at z = 0. `FloorExtent` addresses floors relative to + // `base_floor` (whose bottom is always its own 0), so when a building has no floor 0 + // (all-basement / all-elevated — unreachable from the generator today) the fallback + // of 0 applies no shift: the building-relative z passes through unchanged. let ground_offset = extent .voxel_range_for_floor(0) .map(|(lo, _)| lo) @@ -235,6 +249,8 @@ fn shell_derive_into( } // Roof cap: one voxel layer above the topmost floor, over the full footprint. + // `roof_z` is `None` only if the top floor's `voxel_range_for_floor` returned `None` + // (impossible for a well-formed `FloorExtent`) — in that case no roof is emitted. if let Some(rz) = roof_z { for tx in lo_x..hi_x { for ty in lo_y..hi_y { @@ -362,6 +378,21 @@ mod tests { assert_eq!(fc.get(0, 0, -1), ShellVoxel::Wall); } + #[test] + fn elevated_building_with_no_ground_floor_anchors_at_its_own_bottom() { + // base_floor = 2, no floor 0 → ground_offset falls back to 0, so the building's + // own bottom maps to chunk-z 0 (no shift). 2 floors × 3 voxels, then a roof. + let fc = fill_chunk(1, (0, 0), (0, 0), &[tag((0, 0), (3, 3), 2, 2)]); + // Lowest present floor's interior slab sits at chunk-z 0. + assert_eq!(fc.get(1, 1, 0), ShellVoxel::FloorSlab); + // Second floor's slab one storey up (z 3). + assert_eq!(fc.get(1, 1, 3), ShellVoxel::FloorSlab); + // Perimeter wall from the bottom. + assert_eq!(fc.get(0, 0, 0), ShellVoxel::Wall); + // Roof one voxel above the two storeys (z 6). + assert_eq!(fc.get(1, 1, 6), ShellVoxel::Roof); + } + #[test] fn one_wide_building_is_all_wall() { // 1×4 footprint — every tile is perimeter, so all Wall (no interior slab). @@ -381,7 +412,8 @@ mod tests { let left = fill_chunk(1, (0, 0), (0, 0), std::slice::from_ref(&building)); let right = fill_chunk(1, (0, 0), (1, 0), std::slice::from_ref(&building)); - // Left sub-chunk holds block-local x 60..64 → chunk-local x 60..64. + // Left sub-chunk (0,0): the window origin is 0, so here chunk-local == block-local + // (x 60..64). The right sub-chunk below is the general case where they differ. assert_ne!(left.voxel_count(), 0); assert!(left.voxels.keys().all(|(x, _, _)| (60..64).contains(x))); // Right sub-chunk holds block-local x 64..68 → chunk-local x 0..4.