jpmschweitzer/settled-reach
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

feat(simulation): wire D-199 economic read-set into generation context (#954) #145

Closed
jpmschweitzer wants to merge 0 commits from layer2-read-set into main
pull from: layer2-read-set
merge into: :main
Conversation 2 Commits - Files Changed -
jpmschweitzer commented 2026-05-31 13:58:32 +02:00
Owner
Copy Link
Copy Source

#954 — Layer 2: wire economic read-set into generation context

The clean half of Layer 2 (per the split agreed during refinement). The "main product" derivation (#982) is deferred pending design decisions on three data gaps; this PR does the part that's fully specified and unblocks Layer 3.

What it does

GenerateSkeleton now builds a real CityGenerationContext from a body's economy instead of stub defaults.

  • New CityContextReader (server/src/atlas/city_context_reader.rs): one JOIN across atlas_city_names / bodies / system_factions to read the 6-field D-199 set — economic_role, prosperity_baseline, population, dominant_faction, founding_age_years, settlement_class. prosperity_baseline derived per D-197 (role base + pop-log bonus + seed noise).
  • GenWorkItem::GenerateSkeleton carries the resolved context; run_work_item builds the real context.
  • Abort-on-missing for required fields (founding_age_years, settlement_class); dominant_faction nullable.

Deliberately left at defaults (documented, deferred)

  • dominant_bulk_class, dominant_production_ubiquity — need the dominant-commodity derivation (#982), design-blocked (no location-bound flag set, output is global-not-per-city, 5-vs-4 ubiquity mapping undefined).
  • trait_selection — empty stub until the trait catalog (#1005).
  • morphology_zone + several fields (road_entry_directions, founding_orientation, footprint_radius_km, world_tier, political_archetype, surrounding_biome) — Layer-3 attractor placement / D-204 / D-214 work.

Verification

16 new tests (D-197 derivation units + read-set integration + determinism + abort-on-missing). cargo check/clippy --all-targets -- -D warnings clean; 1279 lib tests pass; fmt clean.

🤖 Generated with Claude Code

## #954 — Layer 2: wire economic read-set into generation context The clean half of Layer 2 (per the split agreed during refinement). The "main product" derivation (#982) is deferred pending design decisions on three data gaps; this PR does the part that's fully specified and unblocks Layer 3. ### What it does `GenerateSkeleton` now builds a real `CityGenerationContext` from a body's economy instead of stub defaults. - New `CityContextReader` (`server/src/atlas/city_context_reader.rs`): one JOIN across `atlas_city_names` / `bodies` / `system_factions` to read the **6-field D-199 set** — `economic_role`, `prosperity_baseline`, `population`, `dominant_faction`, `founding_age_years`, `settlement_class`. `prosperity_baseline` derived per D-197 (role base + pop-log bonus + seed noise). - `GenWorkItem::GenerateSkeleton` carries the resolved context; `run_work_item` builds the real context. - Abort-on-missing for required fields (`founding_age_years`, `settlement_class`); `dominant_faction` nullable. ### Deliberately left at defaults (documented, deferred) - `dominant_bulk_class`, `dominant_production_ubiquity` — need the dominant-commodity derivation (#982), **design-blocked** (no location-bound flag set, output is global-not-per-city, 5-vs-4 ubiquity mapping undefined). - `trait_selection` — empty stub until the trait catalog (#1005). - `morphology_zone` + several fields (`road_entry_directions`, `founding_orientation`, `footprint_radius_km`, `world_tier`, `political_archetype`, `surrounding_biome`) — Layer-3 attractor placement / D-204 / D-214 work. ### Verification 16 new tests (D-197 derivation units + read-set integration + determinism + abort-on-missing). `cargo check`/`clippy --all-targets -- -D warnings` clean; 1279 lib tests pass; fmt clean. 🤖 Generated with [Claude Code](https://claude.com/claude-code)
jpmschweitzer added 1 commit 2026-05-31 13:58:32 +02:00
feat(simulation): wire D-199 economic read-set into generation context (#954) a854b89aa0 
Layer 2 clean half. GenerateSkeleton now builds a CityGenerationContext
from a body's real economic read-set instead of stub defaults.

- New CityContextReader (server/src/atlas/city_context_reader.rs): reads
  the 6-field D-199 set (economic_role, prosperity_baseline, population,
  dominant_faction, founding_age_years, settlement_class) via one JOIN;
  prosperity_baseline derived per D-197.
- GenWorkItem::GenerateSkeleton carries the resolved context; run_work_item
  builds the real context instead of DistrictWorldState::default().
- Left at defaults (deferred, documented): dominant_bulk_class /
  dominant_production_ubiquity (#982, design-blocked), trait_selection
  (#1005), morphology_zone + Layer-3 fields (attractor placement / D-204 /
  D-214).

16 new tests; cargo check/clippy --all-targets -D warnings clean; 1279
lib tests pass; fmt clean.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
jpmschweitzer commented 2026-05-31 14:08:30 +02:00
Author
Owner
Copy Link
Copy Source

Review: layer2-read-set → main (code) — CHANGES REQUESTED

Reviewers: Hoshe (code quality), Tyre (architecture). Both REQUEST_CHANGES.

Build is healthy — cargo check/clippy --all-targets -- -D warnings clean, 1279 lib tests pass, the all_fixtures_deserialize failure is pre-existing on main (not a regression). The JOIN and 6-field D-199 read-set are correct; D-197 role table + clamp match; deferred fields (dominant_*, trait_selection, morphology_zone, Layer-3 fields) have clean documented seams. Two real bugs to fix.

# File Issue By
1 server/src/atlas/city_context_reader.rs (prosperity_noise) Noise is always negative. Divides the RNG value by u32::MAX, but AtlasRng::next_u32() only produces 31-bit values [0, 2^31−1], so max raw ≈ 0.4999 → noise lands in [−0.05, ~0) and is never positive. D-197 requires symmetric ±0.05. Every city gets a slight prosperity penalty. Fix: normalize by 2147483647.0_f32 (or (1u64<<31) as f32). Add a test asserting a positive sample is achievable (current prosperity_noise_in_range passes on the subset and hides the asymmetry). Hoshe
2 server/src/atlas/city_context_reader.rs:256-257 Determinism comment contradicts the code. Comment claims D-010 compliance "no f64 log calls with platform-dependent rounding," then calls f64::log10().floor(). Real boundary risk at pop 9M/99M/999M where the arg is exactly a power of 10 and floor may differ across platforms, shifting the pop bonus by 0.04. Fix with integer magnitude (while v >= 10 { v /= 10; n += 1 }). The identical pattern exists at server/src/atlas/district_mix.rs:31 — recommend fixing both in this pass (or a follow-up ticket), and remove the false comment either way. Tyre

Nice-to-have (worth folding in):

  • city_context_reader.rs:255 — the "integer arithmetic, no f64 log" comment is false; remove/correct (same line as #2).
  • Document why prosperity_noise seeds root → Layer3Settlement directly, skipping the D-224 canonical root → Body → Layer3Settlement (city_id is globally unique, no body_id at read time) — prevents the next reader second-guessing it.
  • parse_settlement_class maps unknown text to MissingField — slightly misleading if the schema later adds a variant.

🤖 review via /pr-review

## Review: layer2-read-set → main (code) — **CHANGES REQUESTED** Reviewers: **Hoshe** (code quality), **Tyre** (architecture). Both REQUEST_CHANGES. Build is healthy — `cargo check`/`clippy --all-targets -- -D warnings` clean, 1279 lib tests pass, the `all_fixtures_deserialize` failure is pre-existing on main (not a regression). The JOIN and 6-field D-199 read-set are correct; D-197 role table + clamp match; deferred fields (`dominant_*`, `trait_selection`, `morphology_zone`, Layer-3 fields) have clean documented seams. Two real bugs to fix. | # | File | Issue | By | |---|------|-------|----| | 1 | `server/src/atlas/city_context_reader.rs` (`prosperity_noise`) | **Noise is always negative.** Divides the RNG value by `u32::MAX`, but `AtlasRng::next_u32()` only produces 31-bit values `[0, 2^31−1]`, so max raw ≈ 0.4999 → noise lands in `[−0.05, ~0)` and is never positive. D-197 requires symmetric ±0.05. Every city gets a slight prosperity penalty. Fix: normalize by `2147483647.0_f32` (or `(1u64<<31) as f32`). Add a test asserting a positive sample is achievable (current `prosperity_noise_in_range` passes on the subset and hides the asymmetry). | Hoshe | | 2 | `server/src/atlas/city_context_reader.rs:256-257` | **Determinism comment contradicts the code.** Comment claims D-010 compliance "no f64 log calls with platform-dependent rounding," then calls `f64::log10().floor()`. Real boundary risk at pop 9M/99M/999M where the arg is exactly a power of 10 and `floor` may differ across platforms, shifting the pop bonus by 0.04. Fix with integer magnitude (`while v >= 10 { v /= 10; n += 1 }`). The identical pattern exists at `server/src/atlas/district_mix.rs:31` — recommend fixing both in this pass (or a follow-up ticket), and remove the false comment either way. | Tyre | Nice-to-have (worth folding in): - `city_context_reader.rs:255` — the "integer arithmetic, no f64 log" comment is false; remove/correct (same line as #2). - Document why `prosperity_noise` seeds `root → Layer3Settlement` directly, skipping the D-224 canonical `root → Body → Layer3Settlement` (city_id is globally unique, no body_id at read time) — prevents the next reader second-guessing it. - `parse_settlement_class` maps unknown text to `MissingField` — slightly misleading if the schema later adds a variant. 🤖 review via /pr-review
jpmschweitzer added 1 commit 2026-05-31 14:31:34 +02:00
fix(simulation): prosperity in basis points + shared log10_floor helper (#954) d6e37c0c0e 
Address PR #145 review. Convert prosperity from f32 to integer basis
points (matching the existing prosperity_bps, D-010 integer-only),
removing both float-determinism bugs by construction:

- New server/src/bps.rs: log10_floor (integer order-of-magnitude) +
  bps_to_f32 edge helper, with boundary tests.
- city_context_reader: prosperity_baseline_bps (u32, 0-10000). role base
  + pop bonus (400*log10_floor, cap 1200) + symmetric noise via integer
  modulo (fixes the always-negative bug) + clamp [1000,9500]. No floats.
- CityGenerationContext.prosperity_baseline -> prosperity_baseline_bps;
  updated the two test context builders. Not serialized — no wire break.
- district_mix population_tier now uses log10_floor (same determinism
  bug class as the comment claimed to avoid).
- Tests in bps + assert positive noise is achievable (the case the old
  test hid).

cargo check/clippy --all-targets -D warnings clean; 1291 lib tests pass;
fmt clean.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
jpmschweitzer commented 2026-05-31 14:33:52 +02:00
Author
Owner
Copy Link
Copy Source

Re-review (commit d6e37c0c0) — APPROVED

Both reviewers re-verified against the bps conversion.

Hoshe (code quality): APPROVE — noise is genuinely symmetric now ((next_u32() % 1001) - 500 → [−500,+500]; prosperity_noise_positive_is_achievable guards the regression the old test hid). Float fully removed from the prosperity path (log10_floor integer loop). D-197 formula matches in bps across role base / pop bonus / clamp. clippy --all-targets -- -D warnings clean; 1291 lib tests pass (+3).

Tyre (architecture): APPROVE — no f64::log10 left in either file; prosperity_baseline_bps: u32 matches the existing InteriorDescriptor.prosperity_bps convention (the codebase's own standard, finally applied here); the duplicated bug is unified into one shared bps::log10_floor; CityGenerationContext is in-process only (Rayon queue), so the field rename breaks no wire/snapshot type; determinism now holds by construction, not by libm behaviour. Also credited the fix for catching the latent always-negative noise bug. "Clean to merge."

The earlier nice-to-haves (false comment, seed-path doc) are resolved; parse_settlement_class unknown→MissingField remains a non-blocking nit.

Clean to merge.

🤖 re-review via /pr-review

## Re-review (commit `d6e37c0c0`) — **APPROVED** Both reviewers re-verified against the bps conversion. **Hoshe (code quality): APPROVE** — noise is genuinely symmetric now (`(next_u32() % 1001) - 500` → [−500,+500]; `prosperity_noise_positive_is_achievable` guards the regression the old test hid). Float fully removed from the prosperity path (`log10_floor` integer loop). D-197 formula matches in bps across role base / pop bonus / clamp. `clippy --all-targets -- -D warnings` clean; 1291 lib tests pass (+3). **Tyre (architecture): APPROVE** — no `f64::log10` left in either file; `prosperity_baseline_bps: u32` matches the existing `InteriorDescriptor.prosperity_bps` convention (the codebase's own standard, finally applied here); the duplicated bug is unified into one shared `bps::log10_floor`; `CityGenerationContext` is in-process only (Rayon queue), so the field rename breaks no wire/snapshot type; determinism now holds by construction, not by libm behaviour. Also credited the fix for catching the latent always-negative noise bug. "Clean to merge." The earlier nice-to-haves (false comment, seed-path doc) are resolved; `parse_settlement_class` unknown→MissingField remains a non-blocking nit. Clean to merge. 🤖 re-review via /pr-review
jpmschweitzer closed this pull request 2026-05-31 14:35:30 +02:00

Pull request closed

This pull request cannot be reopened because the branch was deleted.
Sign in to join this conversation.
Reviewers
No Reviewers
Labels
No items
No labels
Milestone
No items
No Milestone
Projects
Clear projects
No projects
Assignees
Clear assignees
jpmschweitzer (Jeroen Schweitzer)
No Assignees
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: jpmschweitzer/settled-reach#145