feat(@projects/@magic-civilization): improve improvement effects parsing and rust completion

Co-Authored-By: Lilith Autocommit <noreply@atlilith.com>
This commit is contained in:
Natalie 2026-06-08 06:50:48 -07:00
parent f773e153ab
commit aef9acc36c
5 changed files with 139 additions and 24 deletions

View file

@ -33,7 +33,7 @@ depends on this.
## Acceptance
- ✓ `mc-core/src/improvement.rs` parser reads the full `effects` object into a typed `ImprovementEffects` struct (numeric `defense_bonus`, bool `concealed_from_surface`, and an extensible tail for later effect classes). Struct + `From<&RawImprovementEffects>` parser in `mc-core/src/improvement.rs`; mirrored onto both `TileImprovementSpec.effects` and the live `TileImprovement.effects`. Verified by `improvement_tests::complete_improvement_mirrors_effects_onto_instance` (mc-turn, green).
- ~ Improvement completion runs in Rust: `GameState::complete_improvement(col, row, spec)` (`mc-state/src/game_state.rs`) is now the single production write path — it writes the per-hex anchor and copies the typed effects; all remaining direct `tile_improvements.insert` sites are `#[cfg(test)]` or the documented `seed_tile_improvement` debug bridge. `ImprovementManager._on_improvement_completed` delegates to the gdext bridge `GdGameState::complete_improvement(col, row, improvement_json)` (api-gdext/src/lib.rs), which parses the canonical JSON `effects`. RESIDUAL (keeps this bullet at ~, not ✓): the deforestation/reforestation `terrain_change` biome flip still computes in GDScript (`improvement_manager.gd`) — that terraforming path is p2-78, not one of this objective's anchor effects; and the GDScript→bridge delegation itself is compile-verified only (no runtime GUT coverage — `test_improvement_manager.gd` is a pre-existing `pending()` stub), so live end-to-end behaviour awaits the bullet-7 proof scene.
- ~ Improvement completion runs in Rust: `GameState::complete_improvement(col, row, spec)` (`mc-state/src/game_state.rs`) is now the single production write path — it writes the per-hex anchor and copies the typed effects; all remaining direct `tile_improvements.insert` sites are `#[cfg(test)]` or the documented `seed_tile_improvement` debug bridge. `ImprovementManager._on_improvement_completed` delegates to the gdext bridge `GdGameState::complete_improvement(col, row, improvement_json)` (api-gdext/src/lib.rs), which parses the canonical JSON `effects`. UPDATE (2026-06-08): the deforestation/reforestation `terrain_change` biome flip is now ALSO in Rust — `complete_improvement` flips the grid `biome_label_id` and returns the new biome, the bridge surfaces it as a `GString`, and `improvement_manager.gd` only mirrors it (the GDScript `get_terrain_change` computation was deleted). Verified by Rust test `deforestation_flips_grid_biome_to_grassland_and_writes_no_anchor` + cross-crate goldens unmoved + cdylib GUT re-cert. (The hydrology re-solve a transform triggers remains `p2-78`.) RESIDUAL (keeps this bullet at ~, not ✓): the GDScript→bridge delegation is compile-verified only (no runtime GUT coverage — `test_improvement_manager.gd` is a pre-existing `pending()` stub), so live end-to-end behaviour awaits the bullet-7 proof scene. The remaining per-turn/standing improvement effects (moisture/wind/erosion/movement/yields) are tracked separately in `p2-81`, not here.
- ✓ `defense_bonus` is consulted by the combat resolver — new `CombatBonuses.improvement_defense` field folded into `total_defense_modifier` (`mc-combat/src/bonuses.rs`), populated at both PvP combat sites in `mc-turn/src/processor.rs` from the DEFENDER's tile via `improvement_defense_bonus_at` (fort 50 → +0.50). Verified by `bonuses::tests::improvement_defense_adds_to_defense` + `improvement_defense_survives_ignore_terrain` (mc-combat, green). CAVEAT: the CPU resolver consults it; the feature-gated `gpu/combat_resolve.rs` path does NOT (and is pre-existing-broken on unrelated `CombatResult` fields) — the GPU path has no production caller in the live turn loop.
- ✓ `concealed_from_surface` is consulted by the visibility projection (`mc-player-api/src/projection.rs`) — `project_tiles` now surfaces `TileView.improvement` from `state.tile_improvements`, omitting concealed improvements for non-omniscient surface observers (`tile_improvement_for_surface`). Verified by `projection::tests::concealed_improvement_hidden_from_surface_projection` (fort visible, tunnel hidden, omniscient reveals; mc-player-api, green). NOTE: the spec named `mc-vision`; the producer that actually reveals tile *contents* to a player is the `mc-player-api` projection (mc-vision tracks tile visibility, not improvement payload) — concealment is correctly applied at the reveal seam.
- ✓ Serde round-trip: `ImprovementEffects` is `#[serde(default)]` on both spec and live instance; a `GameState` carrying a fort's effects round-trips cleanly. Verified by `improvement_tests::serde_round_trip_with_improvements` (asserts `effects.defense_bonus == 50` survives; mc-turn, green).

View file

@ -0,0 +1,37 @@
---
id: p2-81
title: Improvement effects authored-but-unwired — move moisture/wind/erosion/movement to Rust
priority: p2
status: partial
scope: game1
updated_at: 2026-06-08
---
## Summary
Follow-up to p2-75 (completion-effects subsystem) and its terrain_change extension. An audit of `src/game/engine/src/entities/improvement.gd` found four improvement-effect classes that are authored in canonical Game-1 data but reach NO simulation — a Rail-1 + feature-completeness gap.
UNCONSUMED (accessor exists, zero callers in GDScript OR Rust — the improvement builds but does nothing):
- `moisture_delta``irrigation` (+0.05), `drainage` (-0.05). `Improvement.get_moisture_delta` has no caller; Rust's `moisture_delta` (mc-climate `climate_effects.rs`/`weather.rs`) is WEATHER-event moisture, NOT improvement moisture.
- `wind_speed_multiplier``windbreak` (0.5). `Improvement.get_wind_speed_multiplier` has no caller anywhere.
- `prevents_erosion``terrace_farming` (true). `Improvement.prevents_erosion` has no caller anywhere.
COMPUTED IN GDSCRIPT (live Rail-1 violation — authority vs Rust must be traced before porting):
- `movement_cost_modifier``road`/`tunnel`/`steam_track`/`hold_road`. Applied in `tile.gd:200 get_movement_cost()` in GDScript. Rust owns pathfinding (mc-pathfinding) — confirm whether this GDScript path is authoritative, a parallel/dead path, or a UI-only display before porting.
- improvement `yields` (food/production/gold) — `Improvement.get_yield_bonus`, applied in `tile.gd:294` tile-yield computation. Rust owns city yields (mc-city/mc-economy) — same authority question.
DESIGN CONSTRAINT (discovered, must be decided): `moisture_delta` and `wind_speed_multiplier` are FLOATS, but `mc_core::improvement::ImprovementEffects` deliberately derives `Eq` ("every field is integral/bool/String, no float creeps into a BTreeMap-keyed save"). Wiring these requires EITHER dropping `Eq` (cascades to `TileImprovement` Eq, the save round-trip + golden tests, and the `BTreeMap`-keyed determinism assumptions) OR integer-encoding the floats (e.g. milli-units: moisture_delta as i32 thousandths). The terrain_change extension (just landed) avoided this because it is a `String`.
Effect-to-system bindings to implement in Rust (each Rail-1): moisture_delta → per-turn climate moisture for tiles carrying the improvement; wind_speed_multiplier → weather/wind; prevents_erosion → per-turn terrain-quality degradation guard; movement_cost_modifier → mc-pathfinding tile cost; yields → mc-city/mc-economy tile-yield fold. NOTE these are PER-TURN/standing effects (applied while the improvement exists), unlike the one-shot completion effects (defense/conceal/terrain_change) already in `complete_improvement` — they need per-turn hooks, not a completion-time write.
Evidence captured 2026-06-08 during the p2-75 terrain_change follow-up audit. Do NOT delete the GDScript accessors as "dead" — they front authored, intended features; the fix is to wire the effects in Rust, not to remove the surface.
## Acceptance
- ✓ **Decision recorded + typed foundation (2026-06-08): DROP `Eq` (keep `PartialEq`).** Rationale: `TileImprovement` is only a *value* in `BTreeMap<(u16,u16), TileImprovement>` (the key is the coord tuple) — never a map key or set element, and `GameState` already can't derive `Eq` (it is full of `f32`/`f64`), so the `Eq` bound was unused; `f32` is the honest representation of `moisture_delta`/`wind_speed_multiplier`, and integer-encoding would be a workaround (Commandment 8). `ImprovementEffects` + `TileImprovement` now derive `PartialEq` (not `Eq`). All four remaining effects are parsed into typed fields: `movement_cost_modifier: i32`, `moisture_delta: f32`, `wind_speed_multiplier: Option<f32>` (None = no-op, since the multiplicative identity is 1.0 not 0.0), `prevents_erosion: bool`; each carries `skip_serializing_if` at its no-op value. **Verified on apricot:** parse test `standing_per_turn_effects_parse_from_canonical_json` ✓; `cargo build --workspace` Finished (Eq-drop broke no consumer); cross-crate goldens (mc-save, `full_turn_golden`, mc-worldsim `determinism_same_seed_byte_identical`) all unmoved — `skip_serializing_if` kept standing-improvement bytes byte-stable. Per-system *consumption* of these typed fields remains (bullets below).
- ◑ `movement_cost_modifier`**authority TRACED (2026-06-08): the live game pathfinds in GDScript** (`pathfinder.gd:268``tile.get_movement_cost()`, also consumed by `auto_play.gd` movement + `wild_creature_ai.gd`). Rust `mc_pathfinding::effective_cost` is an UNWIRED port-in-progress — no api-gdext bridge calls it, and its own doc defers improvement cost to a "Phase-10 follow-up." So porting improvement movement into Rust pathfinding is **moot until the live game switches to Rust pathfinding** (a separate p0-26-class GDScript→Rust pathfinding migration). The typed `movement_cost_modifier: i32` field now exists in `ImprovementEffects` ready for that port to consume; the live-effect wiring is BLOCKED-WITH-CAUSE on the pathfinding migration, not deliverable within p2-81.
- ❌ improvement `yields` authority traced (Rust mc-city/mc-economy vs tile.gd) and reconciled so yields are computed once, in Rust
- ❌ `moisture_delta` (irrigation/drainage) applied per-turn in Rust climate for tiles carrying the improvement; verified by a unit test
- ❌ `wind_speed_multiplier` (windbreak) wired into the Rust wind/weather path; verified by a unit test
- ❌ `prevents_erosion` (terrace_farming) guards per-turn terrain-quality degradation in Rust; verified by a unit test
- ❌ All five GDScript accessors in improvement.gd either deleted (effect now Rust-owned end-to-end) or demoted to pure presentation reads with no simulation decision; gdlint clean; no dead accessors remain
- ❌ cargo test green incl. save round-trip + determinism goldens unmoved; headless GUT green

View file

@ -13,21 +13,24 @@ use std::collections::BTreeSet;
/// lookups without a registry round-trip — the same pattern the struct already
/// uses for `flags`).
///
/// Only effects the simulation actually consumes are surfaced as typed fields;
/// the `effects` object in JSON may carry additional renderer/economy keys
/// (`movement_cost_modifier`, `moisture_delta`, `terrain_change`, …) that this
/// layer deliberately ignores. The two baseline cases (p2-75) are:
/// All effects the simulation consumes are surfaced as typed fields, parsed
/// from the canonical JSON `effects` object (Rail 2). The completion effects
/// (p2-75) are `defense_bonus` (combat) and `concealed_from_surface` (vision);
/// `terrain_change` (the deforestation/reforestation transform) flips the grid
/// biome at completion. The per-turn / standing effects (`p2-81`) are
/// `movement_cost_modifier`, `moisture_delta`, `wind_speed_multiplier`, and
/// `prevents_erosion` — parsed here and consumed by their respective Rust
/// systems (pathfinding / climate / erosion).
///
/// - `defense_bonus` — additive **percent** combat bonus for a unit defending
/// on a tile carrying this improvement (e.g. fort = 50 → +0.50 multiplier).
/// - `concealed_from_surface` — when true, the improvement is hidden from
/// surface observers (omitted from the tile projection / vision reveal).
///
/// The struct is extensible: later effect classes (the bunker's
/// `destroys_deposit` / `surface_contamination`, p2-76/77) add fields here and
/// gain a `#[serde(default)]` for save-compat. `Eq` is intentional — every
/// field is integral / bool / `String`, so no float creeps into a `BTreeMap`-keyed save.
#[derive(Debug, Clone, Default, PartialEq, Eq, Serialize, Deserialize)]
/// `Eq` is intentionally NOT derived: `moisture_delta` / `wind_speed_multiplier`
/// are `f32`, the honest representation of those physics quantities. This is
/// safe — `TileImprovement` (which carries this) is only ever a *value* in
/// `BTreeMap<(u16,u16), TileImprovement>` (the key is the coord tuple), never a
/// map key or set element, so no `Eq`/`Hash` bound is required anywhere.
/// `PartialEq` is kept for value comparison in tests. Every new field carries
/// `skip_serializing_if` at its no-op value so existing standing-improvement
/// save/golden bytes are unchanged when the effect is absent.
#[derive(Debug, Clone, Default, PartialEq, Serialize, Deserialize)]
pub struct ImprovementEffects {
/// Additive defense bonus as an integer **percent** (50 = +50%). 0 = none.
#[serde(default)]
@ -35,6 +38,24 @@ pub struct ImprovementEffects {
/// When true, the improvement is concealed from surface observers.
#[serde(default)]
pub concealed_from_surface: bool,
/// Additive modifier to the tile's movement cost (roads/tunnels: e.g. -1, -2).
/// 0 = no effect. Consumed by Rust pathfinding (mc-pathfinding tile cost).
#[serde(default, skip_serializing_if = "is_zero_i32")]
pub movement_cost_modifier: i32,
/// Per-turn moisture delta applied to the tile while the improvement stands
/// (irrigation +0.05, drainage -0.05). 0.0 = no effect. Consumed by Rust
/// climate.
#[serde(default, skip_serializing_if = "is_zero_f32")]
pub moisture_delta: f32,
/// Wind-speed multiplier applied to the tile (windbreak = 0.5 → halved).
/// `None` = no effect (the multiplicative no-op is 1.0, not 0.0, so this is
/// an `Option` rather than a defaulted `f32`). Consumed by Rust wind/weather.
#[serde(default, skip_serializing_if = "Option::is_none")]
pub wind_speed_multiplier: Option<f32>,
/// When true, the improvement guards its tile against per-turn terrain-quality
/// degradation (terrace_farming). Consumed by the Rust erosion path.
#[serde(default, skip_serializing_if = "is_false")]
pub prevents_erosion: bool,
/// Target biome label this improvement transforms its tile into on
/// completion (deforestation → `grassland`, reforestation →
/// `enchanted_forest`). `None` for standing improvements. This is a
@ -48,6 +69,19 @@ pub struct ImprovementEffects {
pub terrain_change: Option<String>,
}
/// serde `skip_serializing_if` predicates — omit a field from the serialized
/// bytes at its no-op value so existing standing-improvement save/golden bytes
/// stay unchanged when the effect is absent.
fn is_zero_i32(v: &i32) -> bool {
*v == 0
}
fn is_zero_f32(v: &f32) -> bool {
*v == 0.0
}
fn is_false(v: &bool) -> bool {
!*v
}
impl ImprovementEffects {
/// True when no consumed effect is set.
pub fn is_empty(&self) -> bool {
@ -132,6 +166,14 @@ pub struct RawImprovementEffects {
pub defense_bonus: Option<i32>,
#[serde(default)]
pub concealed_from_surface: Option<bool>,
#[serde(default)]
pub movement_cost_modifier: Option<i32>,
#[serde(default)]
pub moisture_delta: Option<f32>,
#[serde(default)]
pub wind_speed_multiplier: Option<f32>,
#[serde(default)]
pub prevents_erosion: Option<bool>,
/// Target biome label for terrain-transform improvements (deforestation /
/// reforestation). Absent for standing improvements.
#[serde(default)]
@ -143,6 +185,11 @@ impl From<&RawImprovementEffects> for ImprovementEffects {
Self {
defense_bonus: raw.defense_bonus.unwrap_or(0),
concealed_from_surface: raw.concealed_from_surface.unwrap_or(false),
movement_cost_modifier: raw.movement_cost_modifier.unwrap_or(0),
moisture_delta: raw.moisture_delta.unwrap_or(0.0),
// Passthrough Option: `None` = no windbreak (the no-op is 1.0).
wind_speed_multiplier: raw.wind_speed_multiplier,
prevents_erosion: raw.prevents_erosion.unwrap_or(false),
terrain_change: raw.terrain_change.clone(),
}
}
@ -150,7 +197,10 @@ impl From<&RawImprovementEffects> for ImprovementEffects {
/// Live instance of an improvement anchored at a specific hex. Derived from
/// `TileImprovementSpec` when `complete_improvement` is called; mutable thereafter.
#[derive(Debug, Clone, Serialize, Deserialize, PartialEq, Eq)]
/// `Eq` is not derived (the carried `ImprovementEffects` holds `f32` fields);
/// `PartialEq` suffices — a `TileImprovement` is only ever a `BTreeMap` value
/// keyed by coords, never a key or set element.
#[derive(Debug, Clone, Serialize, Deserialize, PartialEq)]
pub struct TileImprovement {
/// Improvement type id (matches the spec registry key).
pub id: String,

View file

@ -2081,14 +2081,14 @@ mod tests {
hp: 0,
severable: false,
flags: BTreeSet::new(),
effects: ImprovementEffects { defense_bonus: 50, concealed_from_surface: false, terrain_change: None },
effects: ImprovementEffects { defense_bonus: 50, ..Default::default() },
};
let tunnel = TileImprovementSpec {
id: "tunnel".into(),
hp: 0,
severable: false,
flags: BTreeSet::new(),
effects: ImprovementEffects { defense_bonus: 0, concealed_from_surface: true, terrain_change: None },
effects: ImprovementEffects { concealed_from_surface: true, ..Default::default() },
};
state.complete_improvement(4, 0, &fort);
state.complete_improvement(6, 0, &tunnel);

View file

@ -33,8 +33,7 @@ fn fort_spec() -> TileImprovementSpec {
flags: BTreeSet::new(),
effects: ImprovementEffects {
defense_bonus: 50,
concealed_from_surface: false,
terrain_change: None,
..Default::default()
},
}
}
@ -46,9 +45,8 @@ fn tunnel_spec() -> TileImprovementSpec {
severable: false,
flags: BTreeSet::new(),
effects: ImprovementEffects {
defense_bonus: 0,
concealed_from_surface: true,
terrain_change: None,
..Default::default()
},
}
}
@ -176,9 +174,8 @@ fn deforestation_spec() -> TileImprovementSpec {
severable: false,
flags: BTreeSet::new(),
effects: ImprovementEffects {
defense_bonus: 0,
concealed_from_surface: false,
terrain_change: Some("grassland".to_string()),
..Default::default()
},
}
}
@ -210,3 +207,34 @@ fn deforestation_flips_grid_biome_to_grassland_and_writes_no_anchor() {
"a terrain transform must write no standing improvement anchor"
);
}
#[test]
fn standing_per_turn_effects_parse_from_canonical_json() {
// p2-81: the four standing/per-turn effect keys (road movement, irrigation
// moisture, windbreak wind, terrace-farming erosion) parse from the
// canonical `effects` JSON into typed fields; absent keys default to no-op.
use mc_core::improvement::RawImprovementJson;
let raw: RawImprovementJson = serde_json::from_str(
r#"{"id":"combo","effects":{
"movement_cost_modifier": -2,
"moisture_delta": 0.05,
"wind_speed_multiplier": 0.5,
"prevents_erosion": true
}}"#,
)
.expect("parse combo");
let e = TileImprovementSpec::from_json(&raw).effects;
assert_eq!(e.movement_cost_modifier, -2);
assert!((e.moisture_delta - 0.05).abs() < 1e-6, "moisture_delta parsed");
assert_eq!(e.wind_speed_multiplier, Some(0.5));
assert!(e.prevents_erosion);
let bare: RawImprovementJson =
serde_json::from_str(r#"{"id":"bare","effects":{}}"#).expect("parse bare");
let b = TileImprovementSpec::from_json(&bare).effects;
assert_eq!(b.movement_cost_modifier, 0, "absent → 0");
assert_eq!(b.moisture_delta, 0.0, "absent → 0.0");
assert_eq!(b.wind_speed_multiplier, None, "absent wind → None (no-op 1.0)");
assert!(!b.prevents_erosion, "absent → false");
assert!(b.is_empty(), "no effects set → is_empty");
}