From 661d96ba280f094e83a412fa825f20bc9c3e0b7b Mon Sep 17 00:00:00 2001 From: Evan Date: Wed, 17 Jun 2026 10:19:04 -0700 Subject: [PATCH] Fix structure cost double-counting units under construction (#4320) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Problem The ghost/build-menu price of a structure can show the wrong (inflated) cost. Concretely: a player who owns a **captured** city and then starts building their **first** city sees the 3rd-city price (**500k**) for that build instead of the 2nd-city price (**250k**). ## Root cause Structure cost scales as `2^(units built) × base` (city: 125k / 250k / 500k …), counted via: ```ts Math.min(player.unitsOwned(type), player.unitsConstructed(type)) ``` The `Math.min` is deliberate — it caps the count at how many you've actually **built**, so **captured** units (owned but not built) don't inflate the price. `unitsConstructed()` defeated that by double-counting in-progress builds: ```ts const built = this.numUnitsConstructed[type] ?? 0; // already includes the building unit let constructing = 0; for (const unit of this._units) { if (unit.type() !== type) continue; if (!unit.isUnderConstruction()) continue; constructing++; // counts the SAME unit again } return constructing + built; // doubled ``` `recordUnitConstructed()` is called in `buildUnit()` the moment the unit is created — while it is still under construction — so `numUnitsConstructed` already accounts for in-progress builds. The extra loop counted them a second time. With one captured city + one city under construction: `unitsOwned = 2`, double-counted `unitsConstructed = 2`, so `Math.min(2, 2) = 2` → 500k. Without the double-count it's `Math.min(2, 1) = 1` → 250k. ✅ The redundant loop is a leftover from #2378, which removed the separate `UnitType.Construction` unit. Back then in-progress builds were a distinct unit type **not** recorded in `numUnitsConstructed`, so the loop was needed; afterward it became a pure double-count. This is a long-standing latent bug — present identically on `v31` — not a recent regression. ## Fix `unitsConstructed()` now just returns `numUnitsConstructed[type]`, which already includes under-construction builds. ## Tests `tests/economy/ConstructionCost.test.ts` covers both: - pure case (first city under construction) → still 250k - captured city + first city under construction → was 500k, now 250k (fails without the fix with `expected 2 to be 1`) All related suites (economy, PlayerImpl, nation structure behavior, upgrades, MIRV pricing, stats) — 144 tests — pass. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-authored-by: Claude Opus 4.8 (1M context) --- src/core/game/PlayerImpl.ts | 15 ++--- tests/economy/ConstructionCost.test.ts | 78 ++++++++++++++++++++++++++ 2 files changed, 83 insertions(+), 10 deletions(-) create mode 100644 tests/economy/ConstructionCost.test.ts diff --git a/src/core/game/PlayerImpl.ts b/src/core/game/PlayerImpl.ts index 5d216f29b..3cfe71034 100644 --- a/src/core/game/PlayerImpl.ts +++ b/src/core/game/PlayerImpl.ts @@ -416,17 +416,12 @@ export class PlayerImpl implements Player { } } - // Count of units built by the player, including construction + // Count of units built by the player, including those still under + // construction. recordUnitConstructed() is called in buildUnit() the moment a + // unit is created (while still under construction), so numUnitsConstructed + // already accounts for in-progress builds — don't re-count them. unitsConstructed(type: UnitType): number { - const built = this.numUnitsConstructed[type] ?? 0; - let constructing = 0; - for (const unit of this._units) { - if (unit.type() !== type) continue; - if (!unit.isUnderConstruction()) continue; - constructing++; - } - const total = constructing + built; - return total; + return this.numUnitsConstructed[type] ?? 0; } // Count of units owned by the player, not including construction diff --git a/tests/economy/ConstructionCost.test.ts b/tests/economy/ConstructionCost.test.ts new file mode 100644 index 000000000..6dafedc7f --- /dev/null +++ b/tests/economy/ConstructionCost.test.ts @@ -0,0 +1,78 @@ +import { ConstructionExecution } from "../../src/core/execution/ConstructionExecution"; +import { + Game, + Player, + PlayerInfo, + PlayerType, + UnitType, +} from "../../src/core/game/Game"; +import { setup } from "../util/Setup"; + +// Regression test: the ghost/build-menu price of a structure must not double-count +// a player's first structure while it is still under construction. +// +// The cost scales as 2^(units built) * base: 1st city 125k, 2nd 250k, 3rd 500k. +// Cost uses Math.min(unitsOwned, unitsConstructed) so CAPTURED units (owned but +// not built) don't inflate the price. unitsConstructed used to also loop over +// under-construction units, double-counting them and defeating that Math.min — +// a captured city plus a first city under construction showed 500k (3rd-city +// price) instead of 250k. +describe("Structure cost while under construction", () => { + let game: Game; + let player: Player; + let other: Player; + + const builderInfo = new PlayerInfo( + "builder", + PlayerType.Human, + null, + "builder_id", + ); + const otherInfo = new PlayerInfo("other", PlayerType.Human, null, "other_id"); + + beforeEach(async () => { + game = await setup( + "plains", + { infiniteGold: false, instantBuild: false, infiniteTroops: true }, + [builderInfo, otherInfo], + ); + player = game.player(builderInfo.id); + other = game.player(otherInfo.id); + player.conquer(game.ref(0, 10)); + other.conquer(game.ref(15, 15)); + player.addGold(100_000_000n); + other.addGold(100_000_000n); + }); + + function buildFirstCityUnderConstruction() { + game.addExecution( + new ConstructionExecution(player, UnitType.City, game.ref(0, 10)), + ); + game.executeNextTick(); // init + game.executeNextTick(); // build unit + setUnderConstruction(true) + const built = player + .units(UnitType.City) + .find((u) => u.tile() === game.ref(0, 10)); + expect(built?.isUnderConstruction()).toBe(true); + } + + test("first city under construction does not double-count itself", () => { + buildFirstCityUnderConstruction(); + // One built city (under construction) → next city is the 2nd → 250k. + expect(player.unitsConstructed(UnitType.City)).toBe(1); + expect(game.unitInfo(UnitType.City).cost(game, player)).toBe(250_000n); + }); + + test("captured city does not inflate the price of a city under construction", () => { + // 'other' builds a city; 'player' captures it (owns it without building it). + const captured = other.buildUnit(UnitType.City, game.ref(15, 15), {}); + player.captureUnit(captured); + + buildFirstCityUnderConstruction(); + + // Player has BUILT exactly one city (still under construction). The captured + // city must not count toward build cost, so the next city is still 250k. + expect(player.unitsConstructed(UnitType.City)).toBe(1); + expect(game.unitInfo(UnitType.City).cost(game, player)).toBe(250_000n); + }); +});