From 5753d0d4abe0138c326682808581d4628bad5543 Mon Sep 17 00:00:00 2001 From: Natalie Date: Fri, 15 May 2026 21:17:16 -0700 Subject: [PATCH] =?UTF-8?q?fix(api):=20=F0=9F=90=9B=20validate=20negative?= =?UTF-8?q?=20axial=20coords=20in=20tile=20updates?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-Authored-By: Lilith Autocommit --- ...10l-followup-update-tile-negative-axial.md | 62 +++++++++++ src/simulator/api-gdext/src/ai.rs | 102 +++++++++++++----- 2 files changed, 135 insertions(+), 29 deletions(-) create mode 100644 .project/objectives/p2-10l-followup-update-tile-negative-axial.md diff --git a/.project/objectives/p2-10l-followup-update-tile-negative-axial.md b/.project/objectives/p2-10l-followup-update-tile-negative-axial.md new file mode 100644 index 00000000..96e1bfc5 --- /dev/null +++ b/.project/objectives/p2-10l-followup-update-tile-negative-axial.md @@ -0,0 +1,62 @@ +--- +id: p2-10l-followup-update-tile-negative-axial +title: "GdAiController::update_tile rejects negative axial coordinates" +priority: p2 +status: partial +scope: game1 +category: bug +owner: simulator-infra +created: 2026-05-15 +updated_at: 2026-05-15 +blocked_by: [] +follow_ups: [p2-10l-followup-gdai-set-map] +--- + +## Context + +After `p1-29c-followup-empty-params-json-regression` was fixed (added missing `GameState.get_effective_yield_mult`), the apricot smoke batch `20260515_192445` reached **2/10 PASS** (vs 0/12 before). Real progress — games now run hundreds of turns instead of crashing on turn 1. + +But the 8 remaining seed failures all stem from a NEW error spam introduced by `p2-10l-followup-gdai-set-map`: + +``` +ERROR: GdAiController::update_tile: negative hex (25, -2) +ERROR: GdAiController::update_tile: negative hex (29, -5) +ERROR: GdAiController::update_tile: negative hex (28, -3) +… +``` + +Caller passes (col, row) using **axial** coordinates (where `r` can be negative). The validator at `src/simulator/api-gdext/src/ai.rs:449-456` rejects `col < 0 || row < 0`, then the storage is offset-indexed (`row * width + col`), so the validator is correct given current storage but the caller is using the wrong coordinate system OR `update_tile`'s contract is mis-specified. + +## Why this matters + +Each rejected `update_tile` means the AI sees a stale map for any tile with negative axial `r`. In a typical map that's roughly half the playable area. p2-10l-followup-gdai-set-map's stated value — feeding live tile state to the AI for tactical decisions — is silently undermined. + +## Acceptance + +- ✓ Trace the caller — actual paths are `src/game/engine/src/modules/ai/ai_turn_bridge.gd` + (`_on_tile_improved`, `_on_city_border_expanded`, `_on_tile_pillaged`, + `_on_terrain_transformed`, `_on_city_captured`) and + `src/game/engine/src/modules/ai/ai_turn_bridge_state.gd::build_single_tile_json`. + EventBus signals carry **axial** `Vector2i` from `tile.position` (entity-positions + are axial throughout the engine — see `src/map/game_map.gd:4,18` "indexed by axial"). + Negative `r` is legitimate across roughly half the map. +- ✓ Decided canonical: validator accepts axial `(col, row)` and locates the cached + tile by `hex` field equality — matching the consumer-side `tile_at` linear search + in `mc-ai/src/tactical/settle.rs:288-294` (whose doc-comment is explicit that the + storage contract is "row-major of length width*height", with **no** stride-order + invariant). The previous `row * width + col` index math was a divergent invention + and is removed. +- ✓ Updated `src/simulator/api-gdext/src/ai.rs::update_tile` (no more negative-axial + rejection, linear search by `hex`) and added regression test + `update_tile_finds_negative_axial_r`. +- ☐ Verify with apricot smoke batch — gate ≥8/10 PASS (allow 2 environmental). + +## Notes + +The two seeds that DID pass (seed2 and seed8 from the 20260515_192445 batch) likely happened to use map regions with all-positive axial r, by chance. Confirm by inspecting their `turn_stats.jsonl` city positions. + +## References + +- Failing batch: `apricot:~/.cache/mc-batches/20260515_192445/` +- Validator site: `src/simulator/api-gdext/src/ai.rs:449-456` +- set_map landed under: `p2-10l-followup-gdai-set-map.md` diff --git a/src/simulator/api-gdext/src/ai.rs b/src/simulator/api-gdext/src/ai.rs index b79e2ad8..914feacc 100644 --- a/src/simulator/api-gdext/src/ai.rs +++ b/src/simulator/api-gdext/src/ai.rs @@ -437,35 +437,26 @@ impl GdAiController { /// mutation (improvement built, border expanded, terrain transformed, /// etc.). `tile_json` is the serialized form of a single `TacticalTile`. /// + /// `(col, row)` is the **axial** hex coordinate `(q, r)`. `r` is + /// legitimately negative across roughly half the map, so this method + /// MUST NOT reject negative inputs. The cached map is addressed by the + /// `hex` field of each `TacticalTile` (matching the consumer-side + /// `tile_at` linear search in `mc-ai`), not by row-major index — the + /// contract documented in `mc-ai/src/tactical/settle.rs:288-294` is + /// "row-major storage of length width*height", with no stride-order + /// invariant. Linear search by `hex == (col, row)` is the canonical + /// lookup. + /// /// Silently no-ops when no map has been pushed yet (the bridge guards on /// `GameState._ai_map_initialized`, but mass-replay paths can call this /// out of order). Logs a `godot_error!` on JSON parse failure or when - /// `(col, row)` is out of range — never silently corrupts the cache. + /// `(col, row)` is not present in the cached map — never silently + /// corrupts the cache. #[func] fn update_tile(&mut self, col: i32, row: i32, tile_json: GString) { let Some(map) = self.cached_map.as_mut() else { return; }; - if col < 0 || row < 0 { - godot_error!( - "GdAiController::update_tile: negative hex ({}, {})", - col, - row - ); - return; - } - let c = col as u32; - let r = row as u32; - if c >= map.width || r >= map.height { - godot_error!( - "GdAiController::update_tile: hex ({}, {}) out of bounds for {}x{} map", - col, - row, - map.width, - map.height - ); - return; - } let source = tile_json.to_string(); let tile: TacticalTile = match serde_json::from_str(&source) { Ok(t) => t, @@ -474,15 +465,17 @@ impl GdAiController { return; } }; - let idx = (r as usize) * (map.width as usize) + (c as usize); - if idx < map.tiles.len() { - map.tiles[idx] = tile; - } else { - godot_error!( - "GdAiController::update_tile: index {} >= tiles.len() {}", - idx, + let key = (col, row); + match map.tiles.iter_mut().find(|t| t.hex == key) { + Some(slot) => *slot = tile, + None => godot_error!( + "GdAiController::update_tile: hex ({}, {}) not in cached map ({}x{}, {} tiles)", + col, + row, + map.width, + map.height, map.tiles.len() - ); + ), } } @@ -915,6 +908,57 @@ mod tests { ); } + /// Construct a minimal `TacticalMap` directly (bypassing the + /// `#[godot_api]` `set_map` shim — that requires a Godot runtime to + /// instantiate `GdAiController`). The behaviour under test — + /// `update_tile`'s linear search by `hex` — is a pure function on the + /// map vector. + fn replace_by_hex(map: &mut TacticalMap, key: (i32, i32), new_tile: TacticalTile) -> bool { + match map.tiles.iter_mut().find(|t| t.hex == key) { + Some(slot) => { + *slot = new_tile; + true + } + None => false, + } + } + + fn tile_with(hex: (i32, i32), biome: &str) -> TacticalTile { + TacticalTile { + hex, + biome: biome.into(), + yields: (0, 0, 0), + resource: None, + is_coast: false, + owner: None, + } + } + + #[test] + fn update_tile_finds_negative_axial_r() { + // Regression — p2-10l-followup-update-tile-negative-axial: + // the validator must accept negative axial `r` (legitimately produced + // by `axial_to_offset`'s inverse across half the map) and locate the + // tile by `hex` field equality, mirroring `mc-ai::tactical::settle::tile_at`. + let mut map = TacticalMap { + width: 4, + height: 4, + tiles: vec![ + tile_with((0, 0), "plains"), + tile_with((1, -1), "plains"), + tile_with((2, -1), "plains"), + tile_with((3, -2), "plains"), + ], + }; + assert!(replace_by_hex(&mut map, (1, -1), tile_with((1, -1), "forest"))); + assert_eq!(map.tiles[1].biome, "forest"); + assert!(replace_by_hex(&mut map, (3, -2), tile_with((3, -2), "hills"))); + assert_eq!(map.tiles[3].biome, "hills"); + // Missing hex returns false — caller logs godot_error! rather than + // corrupting the cache. + assert!(!replace_by_hex(&mut map, (99, 99), tile_with((99, 99), "x"))); + } + #[test] fn stats_payload_for_emits_canonical_dict_shape() { let json = stats_payload_for("Attack");