From 77c6c4219d56a8ed67147c31cfcb7cb70b68ac7d Mon Sep 17 00:00:00 2001 From: Natalie Date: Tue, 12 May 2026 11:32:42 -0700 Subject: [PATCH] =?UTF-8?q?feat(@projects/@magic-civilization):=20?= =?UTF-8?q?=E2=9C=A8=20add=20proof=20scene=20rehydrating=20GDScript=20via?= =?UTF-8?q?=20GdPlayerApi=20dump=5Fstate=5Fjson?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-Authored-By: Lilith Autocommit --- .../objectives/p2-72-option-b-workaround.md | 104 +++++++++++ ...-72b-promote-playerstate-cities-to-city.md | 174 +++++++++++++++++- 2 files changed, 272 insertions(+), 6 deletions(-) create mode 100644 .project/objectives/p2-72-option-b-workaround.md diff --git a/.project/objectives/p2-72-option-b-workaround.md b/.project/objectives/p2-72-option-b-workaround.md new file mode 100644 index 00000000..7d421a03 --- /dev/null +++ b/.project/objectives/p2-72-option-b-workaround.md @@ -0,0 +1,104 @@ +--- +id: p2-72-option-b +title: "Option B render bridge — proof scene rehydrates GDScript from GdPlayerApi each turn" +priority: p2 +status: open +scope: game1 +category: tooling +owner: simulator-infra +created: 2026-05-12 +updated_at: 2026-05-12 +blocked_by: [] +follow_ups: [p2-67] +--- + +## Context + +`p2-72a` (canonical render source) revealed itself as a 10-14+ week architectural project. The user pivoted to Option B for the immediate Phase 13 screenshot deliverable — a proof scene that bypasses the full extraction by rehydrating GDScript-side entities from `GdPlayerApi.dump_state_json` each turn. + +The wire-transcript Claude-vs-AI demo is already shipped (mocked + real apricot 25-turn). This objective adds the rendered visual layer. + +## Locked decision + +**Option B (explicitly user-authorised, not silent fallback).** Accept dual-state drift for the demo path specifically. The Claude API path holds its own state via `GdPlayerApi`; the rendered path is a separate GDScript scene that pulls `dump_state_json` after every action and rebuilds local `PlayerScript`/`CityScript`/`UnitScript`/`GameMap` instances from the dump. Renderers see real game state without depending on the canonical-render-source milestone. + +The full extraction stays as `p2-72a` for whenever there's appetite to commit weeks to it. + +## Surface + +### 1. New proof scene + +`src/game/engine/scenes/tests/claude_vs_ai_render_proof.gd` + `.tscn`: + +- Boots `GdPlayerApi` in-process (same harness pattern as `claude_player_main.gd`). +- Each turn: + 1. Calls `GdPlayerApi.dump_state_json()` (or equivalent — check existing accessor surface; if absent, add it as a thin wrapper around `GdGameState::serialize_full`). + 2. Parses the JSON into the existing GDScript `GameState.deserialize()` pathway, OR a new lightweight `_rehydrate_from_api_dump(json)` helper that constructs `PlayerScript`/`CityScript`/`UnitScript`/`GameMap` instances directly. + 3. Triggers a render refresh (existing renderers read from `GameState.players[*]` etc. just like they do today). + 4. Captures screenshot via `tools/screenshot.sh` pattern. + 5. Drives `EndTurn` via `GdPlayerApi.apply_action_json`. + 6. Repeat for 25 turns or game-end. + +### 2. `GdPlayerApi.dump_state_json` accessor + +If not already present, add it as a thin wrapper around the inner GameState's serde. Mirror shape of the `claude_player_main.gd` wire format if relevant. + +### 3. Claude policy + +Reuse the deterministic policy from `mc-player-api/tests/full_game_transcript.rs` (`pick_claude_action`). Read legal_actions from PlayerView (now available post-p2-67-followup-legal-actions). Found city → queue warrior → fortify → end turn. + +### 4. Screenshot output + +`.local/demo-runs/2026-05-12-phase13-screenshots/` directory: +- `turn-00.png` through `turn-25.png` (every 5 turns at minimum, ideally every turn). +- `recap.md` with per-turn action log (Claude's choice + AI activity + score deltas). +- `transcript.jsonl` (the canonical wire log, same shape as the real-apricot demo). + +### 5. Dual-state acknowledgement + +Document explicitly in the proof scene's docstring: +- `GdPlayerApi` is the source of truth for state. +- GDScript `GameState.players[*]` etc. is a per-turn rehydrated VIEW, never authoritative. +- DO NOT mutate the GDScript side directly in this proof — all mutations go through `GdPlayerApi.apply_action_json`. +- This proof's render path is NOT the production game's render path; it's a Phase-13 deliverable workaround. + +## Acceptance + +- ☐ `src/game/engine/scenes/tests/claude_vs_ai_render_proof.gd` + `.tscn` exist. +- ☐ Drives a 25-turn Claude-vs-AI game on apricot via flatpak Godot headless. +- ☐ Captures 6 screenshots minimum (turns 0, 5, 10, 15, 20, 25). +- ☐ Recap markdown with per-turn action log. +- ☐ Transcript JSONL alongside screenshots. +- ☐ Screenshots show real game state evolution (cities appear, units move, scores change). +- ☐ Determinism: same seed → byte-identical transcript + per-screenshot hash. +- ☐ p2-67 Phase 13 closes. + +## Why this size + +- Proof scene scaffold: ~2 hr. +- Rehydration helper: ~2 hr (parses dump, constructs GDScript classes — borrows logic from existing `GameState.deserialize`). +- Claude policy: 30 min (port from `full_game_transcript.rs`). +- Screenshot capture: 30 min (`tools/screenshot.sh` already exists). +- 25-turn drive + verification: 1-2 hr. + +**Total: ~0.5-1 day.** + +## Unblocks + +- `p2-67` Phase 13 (real screenshots of Claude vs AI gameplay). +- Closes the wire-transcript-only gap in the demo deliverable. + +## Tech debt acknowledged + +- Dual state in the proof path (GDScript view rebuilt per turn from Rust source of truth). +- Renderers cannot be reused in this exact form for any other Claude-driven scene without similar rehydration boilerplate. +- These are accepted for the demo; the full fix is `p2-72a`. + +## References + +- `.project/objectives/p2-72-gdplayerapi-render-bridge.md` — original p2-72 with Option A vs B framing. +- `.project/objectives/p2-72a-gdgamestate-canonical-render-source.md` — the full extraction (deferred). +- `src/simulator/crates/mc-player-api/tests/full_game_transcript.rs` — Claude policy + wire format reference. +- `src/game/engine/scenes/headless/claude_player_main.gd` — `GdPlayerApi` harness pattern. +- `tools/screenshot.sh` — capture pipeline. +- `.local/demo-runs/2026-05-12-real-apricot-claude-vs-ai/` — wire transcript precedent. diff --git a/.project/objectives/p2-72b-promote-playerstate-cities-to-city.md b/.project/objectives/p2-72b-promote-playerstate-cities-to-city.md index f77f5d19..572c4595 100644 --- a/.project/objectives/p2-72b-promote-playerstate-cities-to-city.md +++ b/.project/objectives/p2-72b-promote-playerstate-cities-to-city.md @@ -1,28 +1,190 @@ --- id: p2-72b-promote-playerstate-cities-to-city -title: "Promote PlayerState.cities from Vec → Vec" +title: "Parallel-field cities synthesis at Godot bridge (Option C)" priority: p2 -status: open +status: blocked scope: game1 category: architecture owner: simulator-infra created: 2026-05-12 updated_at: 2026-05-12 -blocked_by: [] +blocked_by: [user-decision-three-resolutions] follow_ups: [p2-72a, p2-72, p2-67] --- +## STATUS — 2026-05-12 (Hard Stop #1, Section 1 audit) + +**Status flipped `open` → `blocked` before any code changes.** Option C as +specified cannot land as written because the per-instance `GdCity` +architecture invalidates the implicit "small presentation field set" +premise the spec relies on. User decision required between three +resolution paths (below) before this objective can proceed. + +### What the audit found + +The Option C spec implicitly assumed `CityScript` already proxies field +reads through `GdGameState` (mirroring the `BuildingScript` thin-view +pattern from commit `08cc82b70`, p2-72a Stage 2b). The actual +architecture is materially different: + +- `src/game/engine/src/entities/city.gd:95-105` — every `CityScript` + instance constructs **its own** per-instance `GdCity` GDExtension via + `ClassDB.instantiate("GdCity")`. The Godot bridge wrapping that + instance is `src/game/engine/src/entities/city_rust_bridge.gd`. The + 560-line `city.gd` is **not** a thin view today; it's a fat wrapper + around `GdCity` calls. +- `src/simulator/api-gdext/src/lib.rs:1472-1478` — `GdCity` holds + `inner: mc_city::City` plus a `mc_city::ItemRegistry`. That's the + **full** `mc_city::City` type (~30 fields incl. queues, worked tiles, + item production), not `CityState`. +- `src/simulator/crates/mc-turn/src/lib.rs` + `mc-city/src/lib.rs` — + `PlayerState.cities[ci]` is `mc_city::CityState` (the 9-field bench + struct). It is structurally disjoint from the per-instance `GdCity`'s + `mc_city::City`. +- `src/simulator/api-gdext/src/lib.rs:3640` — the only `GdGameState` + city accessor today is `city_count(pi)`. No `city_dict`, no per-city + field accessors. Confirms renderers do **not** read cities through + `GdGameState` today; they read through `CityScript._bridge._gd_city`. + +### Field set the renderer actually needs + +Walking `city.gd` accessors and `to_save_dict` / `to_json` exit points, +the readable surface is ~30 fields, several of which are themselves +containers. Current home of each: + +**On `CityState` (bench, lives in `PlayerState.cities[ci]`):** +`population`, `food_stored`, `production_stored`, `queue`, `queue_cost`, +`queue_tier`, `food_yield`, `prod_yield`, `worker_expertise`. + +**On `mc_city::City` (held by per-instance `GdCity`):** +`city_name`, `position`, `is_capital`, `turn_founded`, `buildings`, +`placed_buildings`, `culture_stored`, `culture_expansion_threshold`, +`growth_threshold`, `focus`, `owned_tiles`, `worked_tiles`, `hp`, +`max_hp`, `is_destroyed`, `can_expand`, per-building queues +(`queue_snapshot`, `queue_len`), item registry/production +(`load_items_json`, `tick_building`, `enqueue_item`), per-turn +processors (`process_growth`, `process_culture`, +`process_culture_with_modifier`, `heal_per_turn`, `mark_attacked`), +yields/food-surplus (`get_yields`, `get_food_surplus`), citizen +management (`auto_assign_citizens`, `assign_citizen`, +`unassign_citizen`), border expansion (`expand_borders`), defense +(`take_damage`, `heal`). + +**GDScript-only (lives on `CityScript`):** +`original_capital_owner`, `production_queue`, `production_progress`, +`has_bombarded`, `bombard_range`, `is_capital` (script-side mirror). + +### Why this trips Hard Stop #1 + +The task spec's hard stop reads: *"A renderer reads a city field that's +neither in CityState nor easily added to CityPresentation → STOP, +document, exit."* + +"Easily added to CityPresentation" reasonably caps at the 8-field +example in the spec (`city_name`, `position`, `focus`, `owned_tiles`, +`buildings`, `culture_stored`, `hp`, `max_hp`). The actual field set is +~3× that, includes containers, and includes mutation-heavy methods +(`process_growth`, `tick_building`, `enqueue_item`, `take_damage`, +`assign_citizen`) that aren't presentation reads at all — they're +domain methods the renderer's wrapping entity invokes. Re-homing those +through `GdGameState.city_dict` would require porting their behaviour +to operate over `GdGameState.inner.players[pi].cities[ci]` — but that +slot is `CityState` and doesn't carry the state these methods mutate. + +The same trap p2-72a Stage 4 documented at line 181 of +`p2-72a-gdgamestate-canonical-render-source.md` (*"a faked `city_dict` +that reads from GDScript-side state would defeat the view-conversion +premise"*) fires here. + +### Three resolution paths (user decision required) + +1. **Balloon `CityPresentation` to ~30 fields, duplicating most of + `mc_city::City` into a UI-layer struct on `GdGameState`.** Lands + Option C as locked, but the "lightweight bridge slot" framing is + gone — the parallel slot is now structurally equivalent to a second + copy of `mc_city::City` with no behavioural methods. Save-format and + sync-invariant complexity scale with the field count. The domain + methods (`process_growth`, `take_damage`, etc.) still need a home; + one option is to leave them on per-instance `GdCity` and proxy + reads only through `presentation_cities`, which produces a + three-way state split (CityState + presentation + per-instance + GdCity) that's strictly worse than today's two-way split. + +2. **Hold `Vec>` (or `Vec>`) on + `GdGameState` as the parallel slot.** Cleanest architecturally — + one canonical `mc_city::City` per (player, city) lives on the + shared `GdGameState`, per-instance `GdCity` instantiation in + `city.gd:95-105` gets deleted, `CityScript` becomes a thin view + over `_gd_state.city_dict(pi, ci)`. This is structurally Option A + (promote `PlayerState.cities` to `City`) applied at the bridge + layer — the bench `CityState` stays in `PlayerState.cities` for + MCTS/AI/turn-processor parity, and the full `City` lives parallel + on `GdGameState`. Effort: ~5-7 days (matches the original Option A + estimate); fully unblocks Stage 4 Wave 2b. **Different decision + than the locked Option C — needs user re-lock.** + +3. **Keep per-instance `GdCity`; add `city_dict` on `GdGameState` + only for the 8 presentation fields the AI/replay/save layer + already needs (city_name, position, focus, owned_tiles, buildings, + culture_stored, hp, max_hp), and DO NOT collapse `CityScript` to + a thin view in this objective.** Partial unblock. p2-72a Stage 4 + Wave 2b checkbox does **not** flip — `CityScript` view conversion + stays deferred, gets its own follow-up objective. Lowest-effort + path (~1 day), preserves bench parity, but does not deliver the + advertised acceptance bullet "CityScript view conversion landed". + +### Evidence + +- `src/game/engine/src/entities/city.gd:95-105` — per-`CityScript` + `ClassDB.instantiate("GdCity")` site. +- `src/game/engine/src/entities/city_rust_bridge.gd:1-50` — bridge + wrapping per-instance `GdCity`. +- `src/simulator/api-gdext/src/lib.rs:1472-1478` — `GdCity::init` + body, `inner: mc_city::City`. +- `src/simulator/api-gdext/src/lib.rs:3640-3644` — sole `GdGameState` + city accessor today (`city_count`). +- `src/simulator/api-gdext/src/lib.rs:3512-3516, 3721-3737` — only + two `PlayerState { ... }` literal sites (spec called out 4; lines + 3836-3858 are the `set_player_cities_from_array` setter, not a + constructor). +- `.project/objectives/p2-72a-gdgamestate-canonical-render-source.md:177-184` + — original hard-stop record from Stage 4 Wave 2b that spawned this + objective. + +### Recommendation + +Path 2 (`Vec>` on `GdGameState`) is the only +resolution that actually delivers the locked acceptance bullet +"CityScript view conversion landed". It costs more than the original +2-3 day Option C estimate (~5-7 days) but cleanly retires the +per-instance `GdCity` instantiation pattern and lets `CityScript` +collapse to the same shape as `BuildingScript`. Path 1 trades effort +for architectural debt; Path 3 narrows scope but punts the actual +Stage 4 Wave 2b unblock to another objective. User pick. + +--- + ## Context `p2-72a Stage 4 Wave 2b` (CityScript view conversion) blocked here. `mc_turn::PlayerState.cities` is `Vec` — the minimal bench struct with 9 fields (population, food_stored, production_stored, queue, queue_cost, queue_tier, food_yield, prod_yield, worker_expertise). Every public CityScript field the GDScript renderers read lives on `mc_city::City` — the full type at `mc-city/src/city.rs:234` with `city_name`, `position`, `buildings`, `placed_buildings`, `culture_stored`, `focus`, `owned_tiles`, `worked_tiles`, `hp`/`max_hp`, per-building queues, etc. The two types are deliberately decoupled per the comment at `mc-city/src/lib.rs:105-108`. To make CityScript a thin view, the held type must carry every field renderers consume. -## Locked decision +## Locked decision (revised 2026-05-12 after audit) -**Promote, don't widen.** Change `PlayerState.cities: Vec` → `Vec`. The full type already exists and is exercised by the production game via `GdCity` instances; promoting consolidates around one canonical city representation. Widening `CityState` would create a second drift point. +**Option C — parallel-field synthesis at Godot bridge layer only.** -Where `CityState` is genuinely the right minimal struct for some pure-sim subsystem (bench MCTS rollouts?), document the call site and keep a separate `Vec` for that path. Don't drop `CityState` entirely. +The original spec's "promote" premise was empirically false: `mc_city::City` is NOT a superset of `CityState`. They're structurally divergent — `food_stored: i32` vs `f64`, no `food_yield`/`prod_yield`/`queue`/`queue_cost`/`queue_tier`/`worker_expertise` on `City`, completely different queue model (`BTreeMap` vs `Option`). The bench `TurnProcessor` + 4 bench bins + 10 test fixtures = ~150 access sites depend on `CityState` fields. + +Promoting (Option A: rename + parallel + sync contract) would be a 5-7 day project; full port (Option B) would be 2+ weeks and regress bench/MCTS parity. Both exceed the original 2-3 day budget AND aren't the path to unblock Stage 4 Wave 2b. + +Option C scope: +- Keep `mc-turn::PlayerState.cities: Vec` unchanged. +- The Godot bridge (`api-gdext::GdGameState`) holds a parallel synthesised view: `presentation_cities: Vec` (or equivalent) populated by GDScript hand-off + maintained by `GdGameState` bridge mutators. +- `city_dict(player_idx, city_idx)` returns the bench `CityState` fields PLUS the presentation fields the renderers consume (city_name, owned_tiles, focus, buildings). +- The full `mc_city::City` consolidation stays as a future strategic objective. + +This is the smallest correct change that unblocks `p2-72a` Stage 4 Wave 2b without grinding through architectural canonicalisation that doesn't belong in p2-72b's scope. ## Surface