fix(simulation): address PR #171 review — all findings processed (T-987)
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<BuildingPropertyTag> 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) <noreply@anthropic.com>
This commit is contained in:
@@ -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<ShellVoxelPos, ShellVoxel> = 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.
|
||||
|
||||
Reference in New Issue
Block a user