Improve random spawn #2503
Improve random spawn #2503
Conversation
…y wasn't getting deleted
WalkthroughAdds Config.minDistanceBetweenPlayers(), converts player spawn state from a boolean to a spawn-tile accessor, moves spawn-tile selection into SpawnExecution (seeded PRNG, lazy search), updates spawners/manager to pass GameID, and adapts/removed tests accordingly. Changes
Sequence DiagramsequenceDiagram
participant Caller as Caller (manager / test)
participant Spawner as Spawner (Bot/Player/Fake)
participant Config as Config
participant Game as GameState
participant SpawnExec as SpawnExecution
participant Player as Player
Caller->>Spawner: request spawn (includes gameID, playerInfo)
Spawner->>Config: minDistanceBetweenPlayers()
Config-->>Spawner: minDistance
Spawner->>Game: query existing players' spawnTile()
Game-->>Spawner: list of spawn tiles
Spawner->>SpawnExec: new SpawnExecution(gameID, playerInfo)
SpawnExec->>SpawnExec: seed PRNG(playerId, gameID)
SpawnExec->>Game: randomSpawnLand() (bounded tries)
alt tile found
SpawnExec->>Player: setSpawnTile(foundTile)
Player-->>SpawnExec: ack
SpawnExec-->>Caller: execution ready
else no tile found
SpawnExec-->>Caller: warn / skip spawn
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
src/core/execution/SpawnExecution.ts (1)
52-54: Safeguard might mask upstream issues.This condition checks if
spawnTile()is undefined and sets it. According to the spawn flow, BotSpawner and PlayerSpawner should set the spawn tile before creating SpawnExecution, so this should always be defined. While defensive programming is good, this safeguard might hide bugs in the spawn flow where spawn tiles aren't properly set upstream.Consider adding a warning log to track when this fallback is used:
if (player.spawnTile() === undefined) { + console.warn(`SpawnExecution: spawn tile was undefined for player ${player.id()}, setting to ${this.tile}`); player.setSpawnTile(this.tile); }src/core/execution/FakeHumanExecution.ts (3)
89-98: CheckGame.player()contract;?? this.mg.addPlayer(...)may never runRight now:
this.player = this.mg.player(this.nation.playerInfo.id) ?? this.mg.addPlayer(this.nation.playerInfo);In
GameImpl.player(id: PlayerID)the method throws if the player is not found, instead of returningundefined. If that is still true, the nullish coalescing fallback toaddPlayer()is dead code andinit()will crash in the “player not yet added” case instead of creating it.If the contract of
Game.player()has not changed to “returnPlayer | undefined”, consider a version that does not depend on it returningundefined, for example:- this.player = - this.mg.player(this.nation.playerInfo.id) ?? - this.mg.addPlayer(this.nation.playerInfo); + const existingPlayer = this.mg + .players() + .find((p) => p.id() === this.nation.playerInfo.id); + this.player = + existingPlayer ?? this.mg.addPlayer(this.nation.playerInfo);(or use a dedicated optional accessor if one exists).
Please confirm the current
GameAPI and adjust accordingly so the “ensure player exists” logic is reliable.
145-173: Spawn tick guard andsetSpawnTilecall look correctThe new guard:
if (this.player === null) { return; }followed by:
if (this.mg.inSpawnPhase()) { const rl = this.randomSpawnLand(); ... this.player.setSpawnTile(rl); this.mg.addExecution(new SpawnExecution(this.nation.playerInfo, rl)); return; }is consistent with the new spawn‑tile tracking flow and ensures FakeHuman players get their
spawnTileset before theSpawnExecutionruns, which matches how other spawn paths in the PR handle it.If
this.playerbeingnullhere is actually an impossible state afterinit(), you may optionally consider throwing instead of silently returning to surface init bugs early, but the current defensive early‑return is acceptable.
634-662: Spawn distance check logic is sound; consider small refinementsThe new proximity check:
const isOtherPlayerSpawnedNearby = this.mg.allPlayers().some((player) => { const spawnTile = player.spawnTile(); if (spawnTile === undefined) { return false; } return ( this.mg.manhattanDist(spawnTile, tile) < this.mg.config().minDistanceBetweenPlayers() ); }); if (isOtherPlayerSpawnedNearby) { continue; }correctly:
- Ignores players without a recorded
spawnTile().- Uses a strict
<comparison, so exactlyminDistanceBetweenPlayers()apart is allowed.- Filters candidates before doing the existing land/owner checks.
Two small, optional cleanups:
- Cache values outside the loop to avoid repeated work:
const minSpawnDist = this.mg.config().minDistanceBetweenPlayers(); const allPlayers = this.mg.allPlayers(); ... const isOtherPlayerSpawnedNearby = allPlayers.some((player) => { ... });
- If design wants to only consider currently active nations, you could additionally filter to alive players (
player.isAlive()), so old dead spawns do not permanently block nearby tiles.These are minor; the current logic is clear and correct.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
src/core/configuration/Config.ts(1 hunks)src/core/configuration/DefaultConfig.ts(1 hunks)src/core/execution/BotSpawner.ts(1 hunks)src/core/execution/ExecutionManager.ts(1 hunks)src/core/execution/FakeHumanExecution.ts(3 hunks)src/core/execution/SpawnExecution.ts(1 hunks)src/core/execution/utils/PlayerSpawner.ts(2 hunks)src/core/game/Game.ts(1 hunks)src/core/game/PlayerImpl.ts(2 hunks)
🧰 Additional context used
🧠 Learnings (11)
📚 Learning: 2025-10-20T20:15:28.858Z
Learnt from: sambokai
Repo: openfrontio/OpenFrontIO PR: 2225
File: src/core/execution/FakeHumanExecution.ts:51-51
Timestamp: 2025-10-20T20:15:28.858Z
Learning: In src/core/execution/FakeHumanExecution.ts, game balance constants like MIRV_COOLDOWN_TICKS, MIRV_HESITATION_ODDS, VICTORY_DENIAL_TEAM_THRESHOLD, VICTORY_DENIAL_INDIVIDUAL_THRESHOLD, and STEAMROLL_CITY_GAP_MULTIPLIER are experimental tuning parameters subject to frequent change during balance testing. Do not flag changes to these values as issues or compare them against previous values.
Applied to files:
src/core/execution/ExecutionManager.tssrc/core/configuration/Config.tssrc/core/configuration/DefaultConfig.tssrc/core/execution/FakeHumanExecution.tssrc/core/execution/utils/PlayerSpawner.ts
📚 Learning: 2025-08-28T22:47:31.406Z
Learnt from: BrewedCoffee
Repo: openfrontio/OpenFrontIO PR: 1957
File: tests/core/executions/PlayerExecution.test.ts:16-39
Timestamp: 2025-08-28T22:47:31.406Z
Learning: In test files, PlayerExecution instances must be manually registered with game.addExecution() because the setup utility doesn't automatically register SpawnExecution, which would normally handle this during the spawn phase in a real game.
Applied to files:
src/core/execution/ExecutionManager.tssrc/core/execution/FakeHumanExecution.tssrc/core/execution/utils/PlayerSpawner.tssrc/core/execution/SpawnExecution.ts
📚 Learning: 2025-08-23T07:48:19.060Z
Learnt from: ElMelchizedek
Repo: openfrontio/OpenFrontIO PR: 1876
File: src/core/execution/FakeHumanExecution.ts:470-473
Timestamp: 2025-08-23T07:48:19.060Z
Learning: In FakeHumanExecution.ts DefensePost placement logic, returning -Infinity from structureSpawnTileValue when no sampled border tiles neighbor enemies is intentional. The logic samples up to 50 border tiles as a heuristic - if none are adjacent to enemies, it assumes DefensePost placement is unnecessary and aborts the entire placement attempt rather than continuing to evaluate individual tiles.
Applied to files:
src/core/game/PlayerImpl.tssrc/core/execution/FakeHumanExecution.tssrc/core/execution/SpawnExecution.tssrc/core/execution/BotSpawner.ts
📚 Learning: 2025-11-12T23:11:34.445Z
Learnt from: MaxHT0x
Repo: openfrontio/OpenFrontIO PR: 2262
File: src/core/configuration/DefaultConfig.ts:806-832
Timestamp: 2025-11-12T23:11:34.445Z
Learning: In src/core/configuration/DefaultConfig.ts, JSDoc documentation for configuration methods should not be added inline, as it was requested that documentation be placed elsewhere in the codebase.
Applied to files:
src/core/configuration/DefaultConfig.ts
📚 Learning: 2025-10-20T11:02:16.969Z
Learnt from: sambokai
Repo: openfrontio/OpenFrontIO PR: 2225
File: src/core/execution/FakeHumanExecution.ts:57-57
Timestamp: 2025-10-20T11:02:16.969Z
Learning: In src/core/execution/FakeHumanExecution.ts, the correct MIRV victory denial thresholds are VICTORY_DENIAL_TEAM_THRESHOLD = 0.8 (80% for team games) and VICTORY_DENIAL_INDIVIDUAL_THRESHOLD = 0.65 (65% for individual players), not 0.85 and 0.7 as might be mentioned in some documentation.
Applied to files:
src/core/configuration/DefaultConfig.tssrc/core/execution/FakeHumanExecution.ts
📚 Learning: 2025-10-18T17:54:01.311Z
Learnt from: sambokai
Repo: openfrontio/OpenFrontIO PR: 2225
File: src/core/execution/FakeHumanExecution.ts:172-173
Timestamp: 2025-10-18T17:54:01.311Z
Learning: In src/core/execution/FakeHumanExecution.ts, MIRVs and ground attacks should not be mutually exclusive. The considerMIRV() method should not short-circuit maybeAttack() - both actions can occur in the same tick.
Applied to files:
src/core/execution/FakeHumanExecution.ts
📚 Learning: 2025-08-23T08:03:44.914Z
Learnt from: ElMelchizedek
Repo: openfrontio/OpenFrontIO PR: 1876
File: src/core/execution/FakeHumanExecution.ts:547-592
Timestamp: 2025-08-23T08:03:44.914Z
Learning: In OpenFrontIO, FakeHumanExecution is used for AI-controlled nations that simulate human behavior, which is distinct from PlayerType.Bot. FakeHumanExecution players are not PlayerType.Bot - they represent more sophisticated AI that mimics human nation behavior, while PlayerType.Bot represents basic AI players.
Applied to files:
src/core/execution/FakeHumanExecution.ts
📚 Learning: 2025-10-27T09:47:26.395Z
Learnt from: sambokai
Repo: openfrontio/OpenFrontIO PR: 2225
File: src/core/execution/FakeHumanExecution.ts:770-795
Timestamp: 2025-10-27T09:47:26.395Z
Learning: In src/core/execution/FakeHumanExecution.ts, the selectSteamrollStopTarget() method intentionally allows MIRV targeting when secondHighest city count is 0 (e.g., nuclear endgame scenarios where structures are destroyed). This is valid game design—"if you can afford it, you're good to go"—and should not be flagged as requiring a guard condition.
Applied to files:
src/core/execution/FakeHumanExecution.ts
📚 Learning: 2025-10-08T17:14:49.369Z
Learnt from: Foorack
Repo: openfrontio/OpenFrontIO PR: 2141
File: src/client/ClientGameRunner.ts:228-234
Timestamp: 2025-10-08T17:14:49.369Z
Learning: In `ClientGameRunner.ts`, the `myPlayer` field is always set when `shouldPreventWindowClose()` is called, so the null check in that method is sufficient without needing to fetch it again from `gameView.playerByClientID()`.
Applied to files:
src/core/execution/FakeHumanExecution.tssrc/core/execution/utils/PlayerSpawner.ts
📚 Learning: 2025-10-08T17:14:49.369Z
Learnt from: Foorack
Repo: openfrontio/OpenFrontIO PR: 2141
File: src/client/ClientGameRunner.ts:228-234
Timestamp: 2025-10-08T17:14:49.369Z
Learning: For the window close confirmation feature in `ClientGameRunner.ts`, the troop count requirement (>10,000 troops) from issue #2137 was intentionally removed because it was arbitrary and troop count can be reported as low despite having significant land. The confirmation now triggers for any alive player regardless of troop count.
Applied to files:
src/core/execution/utils/PlayerSpawner.ts
📚 Learning: 2025-08-12T00:31:50.144Z
Learnt from: scottanderson
Repo: openfrontio/OpenFrontIO PR: 1752
File: src/core/game/Game.ts:750-752
Timestamp: 2025-08-12T00:31:50.144Z
Learning: In the OpenFrontIO codebase, changes to the PlayerInteraction interface (like adding canDonateGold and canDonateTroops flags) do not require corresponding updates to src/core/Schemas.ts or server serialization code.
Applied to files:
src/core/game/Game.ts
🧬 Code graph analysis (6)
src/core/execution/ExecutionManager.ts (3)
src/core/configuration/DefaultConfig.ts (1)
numBots(637-639)src/core/execution/SpawnExecution.ts (1)
SpawnExecution(7-66)src/core/execution/BotSpawner.ts (1)
BotSpawner(9-84)
src/core/game/PlayerImpl.ts (1)
src/core/game/GameMap.ts (1)
TileRef(3-3)
src/core/execution/FakeHumanExecution.ts (5)
src/core/game/GameImpl.ts (1)
player(472-478)src/core/game/GameView.ts (2)
player(607-613)tile(106-108)src/core/game/PlayerImpl.ts (1)
spawnTile(359-361)src/core/game/TrainStation.ts (1)
tile(141-143)src/core/game/UnitImpl.ts (1)
tile(176-178)
src/core/game/Game.ts (2)
src/core/game/PlayerImpl.ts (1)
spawnTile(359-361)src/core/game/GameMap.ts (1)
TileRef(3-3)
src/core/execution/SpawnExecution.ts (2)
src/core/game/GameImpl.ts (1)
player(472-478)src/core/game/GameView.ts (1)
player(607-613)
src/core/execution/BotSpawner.ts (5)
src/core/game/GameImpl.ts (1)
player(472-478)src/core/game/PlayerImpl.ts (1)
spawnTile(359-361)tests/util/Setup.ts (1)
playerInfo(84-86)src/core/game/Game.ts (1)
PlayerInfo(406-420)src/core/execution/SpawnExecution.ts (1)
SpawnExecution(7-66)
🔇 Additional comments (10)
src/core/configuration/Config.ts (1)
156-156: LGTM!Clean API addition. The method name is descriptive and the return type is appropriate for distance measurement.
src/core/configuration/DefaultConfig.ts (1)
621-623: LGTM!Clean implementation of the new config method. The hardcoded value of 30 for minimum spawn distance is acceptable as a game balance constant.
src/core/game/Game.ts (1)
550-551: LGTM!Clean API extension with clear getter and setter methods. The return type
TileRef | undefinedappropriately handles the case where spawn tile might not be set yet.src/core/execution/ExecutionManager.ts (2)
131-131: LGTM!Return type narrowing from
Execution[]toSpawnExecution[]improves type safety and makes the contract clearer. SinceSpawnExecutionimplementsExecution, this is not a breaking change.
135-135: LGTM!Return type narrowing improves type safety and aligns with the actual return value from
PlayerSpawner.spawnPlayers().src/core/execution/utils/PlayerSpawner.ts (1)
72-72: LGTM!Setting spawn tile before creating SpawnExecution establishes the correct order of operations.
src/core/game/PlayerImpl.ts (2)
105-105: LGTM!Clean addition of spawn tile field with appropriate optional type.
355-361: LGTM!Simple and clean getter/setter implementation for spawn tile management.
src/core/execution/BotSpawner.ts (2)
44-59: LGTM!The proximity check correctly validates against all spawned players (not just bots) using the configurable minimum distance. The null check for undefined spawn tiles is appropriate.
61-69: LGTM!Clean spawn flow: creates PlayerInfo, registers the player, sets spawn tile, and returns SpawnExecution. The order ensures the bot is added to the game state with its spawn tile set before creating the execution.
src/core/game/PlayerImpl.ts
Outdated
| } | ||
|
|
||
| hasSpawned(): boolean { | ||
| return this._hasSpawned; |
There was a problem hiding this comment.
I think we can remove this field and do:
hasSpawned(): boolean {
return this._spawnTile !== undefined
}
src/core/execution/SpawnExecution.ts
Outdated
| player.setHasSpawned(true); | ||
|
|
||
| if (player.spawnTile() === undefined) { | ||
| player.setSpawnTile(this.tile); |
There was a problem hiding this comment.
don't we want to update the spawn tile every time it changes?
like what if you have multiple spawn executions for the same player?
There was a problem hiding this comment.
@evanpelle , yes, you are right. Removed this condition.
src/core/execution/BotSpawner.ts
Outdated
|
|
||
| const playerInfo = new PlayerInfo( | ||
| botName, | ||
| PlayerType.Bot, | ||
| null, | ||
| this.random.nextID(), | ||
| ); | ||
| this.gs.addPlayer(playerInfo).setSpawnTile(tile); |
There was a problem hiding this comment.
shouldn't the SpawnExecution handle this?
There was a problem hiding this comment.
@evanpelle , refactored, check again, please.
|
This pull request is stale because it has been open for fourteen days with no activity. If you want to keep this pull request open, add a comment or update the branch. |
|
It is not stale, I will return to it soon. |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/core/execution/FakeHumanExecution.ts (1)
155-168: Fix: spawn-phase logic is gated by attack cadence (can delay/skip spawning).
Move theinSpawnPhase()handling beforeticks % attackRateso FakeHumans always enqueue spawn during spawn phase.tick(ticks: number) { + if (this.player === null) { + return; + } + + if (this.mg.inSpawnPhase()) { + this.mg.addExecution(new SpawnExecution(this.gameID, this.nation.playerInfo)); + return; + } + // Ship tracking if ( - this.player !== null && - this.player.isAlive() && + this.player.isAlive() && this.mg.config().gameConfig().difficulty !== Difficulty.Easy ) { this.trackTransportShipsAndRetaliate(); this.trackTradeShipsAndRetaliate(); } if (ticks % this.attackRate !== this.attackTick) { return; } - if (this.player === null) { - return; - } - - if (this.mg.inSpawnPhase()) { - this.mg.addExecution( - new SpawnExecution(this.gameID, this.nation.playerInfo), - ); - return; - } - if (!this.player.isAlive()) { this.active = false; return; }
🧹 Nitpick comments (7)
tests/Attack.test.ts (1)
149-157: MakeaddPlayerToGame()takegameIDinstead of capturing the file-global.
This keeps the helper pure/easier to reuse (and avoids surprises if tests ever create multiple games with different IDs in this file).function addPlayerToGame( playerInfo: PlayerInfo, game: Game, tile: TileRef, + gameID: GameID, ): Player { game.addPlayer(playerInfo); game.addExecution(new SpawnExecution(gameID, playerInfo, tile)); return game.player(playerInfo.id); }- playerA = addPlayerToGame(playerAInfo, game, game.ref(0, 10)); + playerA = addPlayerToGame(playerAInfo, game, game.ref(0, 10), gameID); ... - playerB = addPlayerToGame(playerBInfo, game, game.ref(0, 10)); + playerB = addPlayerToGame(playerBInfo, game, game.ref(0, 10), gameID); ... - const playerC = addPlayerToGame(playerCInfo, game, game.ref(10, 10)); + const playerC = addPlayerToGame(playerCInfo, game, game.ref(10, 10), gameID);src/core/execution/BotSpawner.ts (1)
24-51: Remove misleadingnull/retry flow (sincespawnBot()can’t fail anymore).
Right nowspawnBot()always returns aSpawnExecution, sotries/nullhandling inspawnBots()is dead code.export class BotSpawner { private random: PseudoRandom; private bots: SpawnExecution[] = []; private nameIndex = 0; constructor( private gs: Game, private gameID: GameID, ) { this.random = new PseudoRandom(simpleHash(gameID)); } spawnBots(numBots: number): SpawnExecution[] { - let tries = 0; while (this.bots.length < numBots) { - if (tries > 10000) { - console.log("too many retries while spawning bots, giving up"); - return this.bots; - } const candidate = this.nextCandidateName(); const spawn = this.spawnBot(candidate.name); - if (spawn !== null) { - // Only use candidate name once bot successfully spawned - if (candidate.source === "list") { - this.nameIndex++; - } - this.bots.push(spawn); - } else { - tries++; - } + // Only use candidate name once bot successfully enqueued + if (candidate.source === "list") this.nameIndex++; + this.bots.push(spawn); } return this.bots; } - spawnBot(botName: string): SpawnExecution | null { + spawnBot(botName: string): SpawnExecution { return new SpawnExecution( this.gameID, new PlayerInfo(botName, PlayerType.Bot, null, this.random.nextID()), ); }tests/core/execution/SpawnExecution.test.ts (3)
5-57: Test intent is good, but the hard-coded “maxPlayers per map” is brittle.
Consider settingminDistanceBetweenPlayers()in the test config (or using a smaller fixed value) so this test doesn’t depend on map-manifest math.
59-89: Avoid asserting an exact “1 player spawned” (likely brittle).
Better invariant: “at least 1 spawned” and “fewer than requested spawned” (or force failure by overridingminDistanceBetweenPlayers()to something huge for this test).
91-110: Strengthen the “jump around” coverage + fix typo.
Right now you only assert the finalspawnTile(). This PR claims to fix “previous territory not deleted”; add an assertion that the first spawn area is no longer owned after the second spawn. Also renamespicific→specific.- test("Spawn on spicific tile", async () => { + test("Spawn on specific tile", async () => { const playerInfo = new PlayerInfo( `player`, PlayerType.Human, `client_id`, `player_id`, ); const game = await setup("half_land_half_ocean", undefined, [playerInfo]); game.addExecution(new SpawnExecution("game_id", playerInfo, 50)); game.addExecution(new SpawnExecution("game_id", playerInfo, 60)); while (game.inSpawnPhase()) { game.executeNextTick(); } - // Should spawn fewer than requested when map is too small expect(game.playerByClientID("client_id")?.spawnTile()).toBe(60); + // Previous territory from first spawn should be relinquished + expect(game.owner(50).isPlayer()).toBe(false); });src/core/execution/utils/PlayerSpawner.ts (1)
13-23: Looks good: PlayerSpawner delegates spawn choice to SpawnExecution.
Optional: ifspawnPlayers()can be called multiple times, consider clearingthis.players(or making it a local array) to avoid duplicate SpawnExecutions.src/core/execution/SpawnExecution.ts (1)
14-14: Consider lowering MAX_SPAWN_TRIES.10,000 attempts is very high. Most valid spawns should succeed in far fewer tries. Consider reducing to 1,000 or even 100 to avoid performance issues on small/crowded maps.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (18)
src/core/execution/BotSpawner.ts(2 hunks)src/core/execution/ExecutionManager.ts(2 hunks)src/core/execution/FakeHumanExecution.ts(3 hunks)src/core/execution/SpawnExecution.ts(4 hunks)src/core/execution/utils/PlayerSpawner.ts(1 hunks)tests/Attack.test.ts(3 hunks)tests/AttackStats.test.ts(2 hunks)tests/DeleteUnitExecution.test.ts(2 hunks)tests/Disconnected.test.ts(3 hunks)tests/Donate.test.ts(8 hunks)tests/MissileSilo.test.ts(2 hunks)tests/TerritoryCapture.test.ts(1 hunks)tests/core/execution/SpawnExecution.test.ts(1 hunks)tests/core/execution/utils/PlayerSpawner.test.ts(0 hunks)tests/core/executions/SAMLauncherExecution.test.ts(2 hunks)tests/core/game/GameImpl.test.ts(3 hunks)tests/economy/ConstructionGold.test.ts(2 hunks)tests/nukes/HydrogenAndMirv.test.ts(2 hunks)
💤 Files with no reviewable changes (1)
- tests/core/execution/utils/PlayerSpawner.test.ts
🧰 Additional context used
🧠 Learnings (17)
📚 Learning: 2025-10-20T20:15:28.858Z
Learnt from: sambokai
Repo: openfrontio/OpenFrontIO PR: 2225
File: src/core/execution/FakeHumanExecution.ts:51-51
Timestamp: 2025-10-20T20:15:28.858Z
Learning: In src/core/execution/FakeHumanExecution.ts, game balance constants like MIRV_COOLDOWN_TICKS, MIRV_HESITATION_ODDS, VICTORY_DENIAL_TEAM_THRESHOLD, VICTORY_DENIAL_INDIVIDUAL_THRESHOLD, and STEAMROLL_CITY_GAP_MULTIPLIER are experimental tuning parameters subject to frequent change during balance testing. Do not flag changes to these values as issues or compare them against previous values.
Applied to files:
tests/nukes/HydrogenAndMirv.test.tstests/TerritoryCapture.test.tssrc/core/execution/ExecutionManager.tstests/core/game/GameImpl.test.tstests/MissileSilo.test.tstests/Donate.test.tstests/Disconnected.test.tstests/Attack.test.tssrc/core/execution/utils/PlayerSpawner.tstests/core/executions/SAMLauncherExecution.test.tstests/economy/ConstructionGold.test.tstests/core/execution/SpawnExecution.test.tssrc/core/execution/FakeHumanExecution.tstests/AttackStats.test.tstests/DeleteUnitExecution.test.tssrc/core/execution/BotSpawner.tssrc/core/execution/SpawnExecution.ts
📚 Learning: 2025-10-18T17:54:01.311Z
Learnt from: sambokai
Repo: openfrontio/OpenFrontIO PR: 2225
File: src/core/execution/FakeHumanExecution.ts:172-173
Timestamp: 2025-10-18T17:54:01.311Z
Learning: In src/core/execution/FakeHumanExecution.ts, MIRVs and ground attacks should not be mutually exclusive. The considerMIRV() method should not short-circuit maybeAttack() - both actions can occur in the same tick.
Applied to files:
tests/nukes/HydrogenAndMirv.test.tstests/core/game/GameImpl.test.tstests/MissileSilo.test.tstests/Attack.test.tssrc/core/execution/utils/PlayerSpawner.tssrc/core/execution/FakeHumanExecution.tssrc/core/execution/SpawnExecution.ts
📚 Learning: 2025-12-13T14:58:22.026Z
Learnt from: scamiv
Repo: openfrontio/OpenFrontIO PR: 2607
File: src/core/execution/PlayerExecution.ts:271-295
Timestamp: 2025-12-13T14:58:22.026Z
Learning: In src/core/execution/PlayerExecution.ts surroundedBySamePlayer(), the `as Player` cast on `mg.playerBySmallID(scan.enemyId)` is intentional. Since scan.enemyId comes from ownerID() on an owned tile and playerBySmallID() only returns Player or undefined, the cast expresses a known invariant. The maintainers prefer loud failures (runtime errors) over silent masking (early returns with guards) for corrupted game state scenarios at trusted call sites.
Applied to files:
tests/nukes/HydrogenAndMirv.test.tstests/TerritoryCapture.test.tssrc/core/execution/ExecutionManager.tstests/core/game/GameImpl.test.tstests/MissileSilo.test.tstests/Donate.test.tstests/Disconnected.test.tstests/Attack.test.tssrc/core/execution/utils/PlayerSpawner.tstests/core/executions/SAMLauncherExecution.test.tstests/economy/ConstructionGold.test.tstests/core/execution/SpawnExecution.test.tssrc/core/execution/FakeHumanExecution.tstests/AttackStats.test.tstests/DeleteUnitExecution.test.tssrc/core/execution/BotSpawner.tssrc/core/execution/SpawnExecution.ts
📚 Learning: 2025-10-27T09:47:26.395Z
Learnt from: sambokai
Repo: openfrontio/OpenFrontIO PR: 2225
File: src/core/execution/FakeHumanExecution.ts:770-795
Timestamp: 2025-10-27T09:47:26.395Z
Learning: In src/core/execution/FakeHumanExecution.ts, the selectSteamrollStopTarget() method intentionally allows MIRV targeting when secondHighest city count is 0 (e.g., nuclear endgame scenarios where structures are destroyed). This is valid game design—"if you can afford it, you're good to go"—and should not be flagged as requiring a guard condition.
Applied to files:
tests/nukes/HydrogenAndMirv.test.tssrc/core/execution/utils/PlayerSpawner.tssrc/core/execution/FakeHumanExecution.ts
📚 Learning: 2025-10-20T11:02:16.969Z
Learnt from: sambokai
Repo: openfrontio/OpenFrontIO PR: 2225
File: src/core/execution/FakeHumanExecution.ts:57-57
Timestamp: 2025-10-20T11:02:16.969Z
Learning: In src/core/execution/FakeHumanExecution.ts, the correct MIRV victory denial thresholds are VICTORY_DENIAL_TEAM_THRESHOLD = 0.8 (80% for team games) and VICTORY_DENIAL_INDIVIDUAL_THRESHOLD = 0.65 (65% for individual players), not 0.85 and 0.7 as might be mentioned in some documentation.
Applied to files:
tests/nukes/HydrogenAndMirv.test.tssrc/core/execution/ExecutionManager.tstests/Attack.test.tssrc/core/execution/utils/PlayerSpawner.tssrc/core/execution/FakeHumanExecution.ts
📚 Learning: 2025-08-28T22:47:31.406Z
Learnt from: BrewedCoffee
Repo: openfrontio/OpenFrontIO PR: 1957
File: tests/core/executions/PlayerExecution.test.ts:16-39
Timestamp: 2025-08-28T22:47:31.406Z
Learning: In test files, PlayerExecution instances must be manually registered with game.addExecution() because the setup utility doesn't automatically register SpawnExecution, which would normally handle this during the spawn phase in a real game.
Applied to files:
tests/nukes/HydrogenAndMirv.test.tstests/TerritoryCapture.test.tssrc/core/execution/ExecutionManager.tstests/core/game/GameImpl.test.tstests/MissileSilo.test.tstests/Donate.test.tstests/Disconnected.test.tstests/Attack.test.tstests/core/executions/SAMLauncherExecution.test.tstests/economy/ConstructionGold.test.tstests/core/execution/SpawnExecution.test.tssrc/core/execution/FakeHumanExecution.tstests/AttackStats.test.tstests/DeleteUnitExecution.test.tssrc/core/execution/BotSpawner.tssrc/core/execution/SpawnExecution.ts
📚 Learning: 2025-08-12T00:31:50.144Z
Learnt from: scottanderson
Repo: openfrontio/OpenFrontIO PR: 1752
File: src/core/game/Game.ts:750-752
Timestamp: 2025-08-12T00:31:50.144Z
Learning: In the OpenFrontIO codebase, changes to the PlayerInteraction interface (like adding canDonateGold and canDonateTroops flags) do not require corresponding updates to src/core/Schemas.ts or server serialization code.
Applied to files:
tests/core/game/GameImpl.test.tstests/Donate.test.tssrc/core/execution/utils/PlayerSpawner.tstests/economy/ConstructionGold.test.tssrc/core/execution/FakeHumanExecution.tstests/AttackStats.test.ts
📚 Learning: 2025-11-26T20:49:29.140Z
Learnt from: scamiv
Repo: openfrontio/OpenFrontIO PR: 2519
File: src/core/game/GameView.ts:516-525
Timestamp: 2025-11-26T20:49:29.140Z
Learning: In GameView.ts, when usesSharedTileState is true (SAB mode), packedTileUpdates contains unpacked tile references as BigInt(tileRef) only, because all tile state lives in the shared Uint16Array. In non-SAB mode, packedTileUpdates contains packed TileUpdate bigints in the format (tileRef << 16n | state), which must be decoded via updateTile(tu). Therefore, Number(tu) is correct in SAB mode and shifting right by 16 bits would be wrong.
Applied to files:
tests/core/game/GameImpl.test.tssrc/core/execution/BotSpawner.ts
📚 Learning: 2025-10-26T15:37:07.732Z
Learnt from: GlacialDrift
Repo: openfrontio/OpenFrontIO PR: 2298
File: src/client/graphics/layers/TerritoryLayer.ts:200-210
Timestamp: 2025-10-26T15:37:07.732Z
Learning: In GameImpl.ts lines 124-139, team assignment logic varies by number of teams: when numPlayerTeams < 8, teams are assigned ColoredTeams values (Red, Blue, Yellow, Green, Purple, Orange, Teal); when numPlayerTeams >= 8, teams are assigned generic string identifiers like "Team 1", "Team 2", etc., which are not members of ColoredTeams.
Applied to files:
tests/Disconnected.test.tssrc/core/execution/BotSpawner.ts
📚 Learning: 2025-10-08T17:14:49.369Z
Learnt from: Foorack
Repo: openfrontio/OpenFrontIO PR: 2141
File: src/client/ClientGameRunner.ts:228-234
Timestamp: 2025-10-08T17:14:49.369Z
Learning: In `ClientGameRunner.ts`, the `myPlayer` field is always set when `shouldPreventWindowClose()` is called, so the null check in that method is sufficient without needing to fetch it again from `gameView.playerByClientID()`.
Applied to files:
src/core/execution/utils/PlayerSpawner.ts
📚 Learning: 2025-10-08T17:14:49.369Z
Learnt from: Foorack
Repo: openfrontio/OpenFrontIO PR: 2141
File: src/client/ClientGameRunner.ts:228-234
Timestamp: 2025-10-08T17:14:49.369Z
Learning: For the window close confirmation feature in `ClientGameRunner.ts`, the troop count requirement (>10,000 troops) from issue #2137 was intentionally removed because it was arbitrary and troop count can be reported as low despite having significant land. The confirmation now triggers for any alive player regardless of troop count.
Applied to files:
src/core/execution/utils/PlayerSpawner.ts
📚 Learning: 2025-11-01T00:24:33.860Z
Learnt from: FloPinguin
Repo: openfrontio/OpenFrontIO PR: 2161
File: src/core/execution/FakeHumanExecution.ts:670-678
Timestamp: 2025-11-01T00:24:33.860Z
Learning: In OpenFrontIO, PlayerType.Bot entities cannot be in teams and do not have friendliness relationships. Only PlayerType.Human and PlayerType.FakeHuman can participate in teams and alliances. Therefore, when targeting bot-owned tiles, friendliness checks like `owner.isFriendly(this.player)` are unnecessary and meaningless for bots.
Applied to files:
src/core/execution/utils/PlayerSpawner.ts
📚 Learning: 2025-08-23T07:48:19.060Z
Learnt from: ElMelchizedek
Repo: openfrontio/OpenFrontIO PR: 1876
File: src/core/execution/FakeHumanExecution.ts:470-473
Timestamp: 2025-08-23T07:48:19.060Z
Learning: In FakeHumanExecution.ts DefensePost placement logic, returning -Infinity from structureSpawnTileValue when no sampled border tiles neighbor enemies is intentional. The logic samples up to 50 border tiles as a heuristic - if none are adjacent to enemies, it assumes DefensePost placement is unnecessary and aborts the entire placement attempt rather than continuing to evaluate individual tiles.
Applied to files:
src/core/execution/utils/PlayerSpawner.tssrc/core/execution/FakeHumanExecution.tssrc/core/execution/BotSpawner.tssrc/core/execution/SpawnExecution.ts
📚 Learning: 2025-08-29T16:16:11.309Z
Learnt from: BrewedCoffee
Repo: openfrontio/OpenFrontIO PR: 1957
File: src/core/execution/PlayerExecution.ts:40-52
Timestamp: 2025-08-29T16:16:11.309Z
Learning: In OpenFrontIO PlayerExecution.ts, when Defense Posts are captured due to tile ownership changes, the intended behavior is to first call u.decreaseLevel() to downgrade them, then still transfer them to the capturing player via captureUnit(). This is not a bug - Defense Posts should be both downgraded and captured, not destroyed outright.
Applied to files:
src/core/execution/utils/PlayerSpawner.ts
📚 Learning: 2025-08-23T08:03:44.914Z
Learnt from: ElMelchizedek
Repo: openfrontio/OpenFrontIO PR: 1876
File: src/core/execution/FakeHumanExecution.ts:547-592
Timestamp: 2025-08-23T08:03:44.914Z
Learning: In OpenFrontIO, FakeHumanExecution is used for AI-controlled nations that simulate human behavior, which is distinct from PlayerType.Bot. FakeHumanExecution players are not PlayerType.Bot - they represent more sophisticated AI that mimics human nation behavior, while PlayerType.Bot represents basic AI players.
Applied to files:
src/core/execution/utils/PlayerSpawner.tssrc/core/execution/FakeHumanExecution.ts
📚 Learning: 2025-10-21T20:06:04.823Z
Learnt from: Saphereye
Repo: openfrontio/OpenFrontIO PR: 2233
File: src/client/HostLobbyModal.ts:891-891
Timestamp: 2025-10-21T20:06:04.823Z
Learning: For the HumansVsNations game mode in `src/client/HostLobbyModal.ts` and related files, the implementation strategy is to generate all nations and adjust their strength for balancing, rather than limiting lobby size based on the number of available nations on the map.
Applied to files:
src/core/execution/utils/PlayerSpawner.tssrc/core/execution/FakeHumanExecution.ts
📚 Learning: 2025-08-24T11:13:08.495Z
Learnt from: DevelopingTom
Repo: openfrontio/OpenFrontIO PR: 1900
File: src/core/execution/SAMLauncherExecution.ts:103-111
Timestamp: 2025-08-24T11:13:08.495Z
Learning: In SAMLauncherExecution.ts, the cached target bug can only occur if: 1) SAM is not on cooldown when nuke is in range, 2) SAM goes on cooldown right after computing trajectory, 3) SAM becomes available again before nuke explodes. This is not possible with current cooldown values but the fix is still valuable for robustness.
Applied to files:
tests/core/executions/SAMLauncherExecution.test.ts
🧬 Code graph analysis (15)
tests/nukes/HydrogenAndMirv.test.ts (2)
src/core/Schemas.ts (1)
GameID(25-25)src/core/execution/SpawnExecution.ts (1)
SpawnExecution(10-118)
tests/TerritoryCapture.test.ts (2)
src/core/Schemas.ts (1)
GameID(25-25)src/core/execution/SpawnExecution.ts (1)
SpawnExecution(10-118)
src/core/execution/ExecutionManager.ts (2)
src/core/execution/SpawnExecution.ts (1)
SpawnExecution(10-118)src/core/execution/BotSpawner.ts (1)
BotSpawner(12-90)
tests/core/game/GameImpl.test.ts (2)
src/core/Schemas.ts (1)
GameID(25-25)src/core/execution/SpawnExecution.ts (1)
SpawnExecution(10-118)
tests/MissileSilo.test.ts (2)
src/core/Schemas.ts (1)
GameID(25-25)src/core/execution/SpawnExecution.ts (1)
SpawnExecution(10-118)
tests/Donate.test.ts (2)
src/core/Schemas.ts (1)
GameID(25-25)src/core/execution/SpawnExecution.ts (1)
SpawnExecution(10-118)
tests/Disconnected.test.ts (2)
src/core/Schemas.ts (1)
GameID(25-25)src/core/execution/SpawnExecution.ts (1)
SpawnExecution(10-118)
tests/Attack.test.ts (3)
src/core/game/Game.ts (1)
Game(668-763)src/core/Schemas.ts (1)
GameID(25-25)src/core/execution/SpawnExecution.ts (1)
SpawnExecution(10-118)
src/core/execution/utils/PlayerSpawner.ts (2)
src/core/Schemas.ts (1)
GameID(25-25)src/core/execution/SpawnExecution.ts (1)
SpawnExecution(10-118)
tests/core/executions/SAMLauncherExecution.test.ts (1)
src/core/Schemas.ts (1)
GameID(25-25)
tests/economy/ConstructionGold.test.ts (2)
src/core/Schemas.ts (1)
GameID(25-25)src/core/execution/SpawnExecution.ts (1)
SpawnExecution(10-118)
tests/core/execution/SpawnExecution.test.ts (4)
src/core/game/Game.ts (1)
PlayerInfo(409-423)src/core/execution/SpawnExecution.ts (1)
SpawnExecution(10-118)tests/util/Setup.ts (2)
playerInfo(84-86)setup(23-82)src/core/game/PlayerImpl.ts (1)
spawnTile(354-356)
src/core/execution/FakeHumanExecution.ts (3)
src/core/game/GameView.ts (1)
gameID(785-787)src/core/Schemas.ts (1)
GameID(25-25)src/core/execution/SpawnExecution.ts (1)
SpawnExecution(10-118)
tests/DeleteUnitExecution.test.ts (2)
src/core/Schemas.ts (1)
GameID(25-25)src/core/execution/SpawnExecution.ts (1)
SpawnExecution(10-118)
src/core/execution/BotSpawner.ts (2)
src/core/game/GameView.ts (1)
gameID(785-787)src/core/Schemas.ts (1)
GameID(25-25)
🔇 Additional comments (30)
tests/TerritoryCapture.test.ts (1)
3-3: Good test update for newSpawnExecution(gameID, ...)signature.
ImportingGameID, declaring agameIDconstant, and passing it intoSpawnExecutionis consistent and keeps the test deterministic.Also applies to: 9-16
tests/economy/ConstructionGold.test.ts (1)
10-10: LGTM: spawn setup now matches the newSpawnExecution(gameID, ...)API.
KeepinggameIDat file scope is nice here since it’s reused acrossbeforeEach.Also applies to: 15-35
tests/nukes/HydrogenAndMirv.test.ts (1)
10-10: LGTM: all spawn points now passgameIDintoSpawnExecution.
This aligns both the defaultbeforeEachand the “fresh game instance” test with the new constructor.Also applies to: 16-27, 61-66
tests/AttackStats.test.ts (1)
4-4: LGTM: deterministic spawns viagameIDin bothSpawnExecutioncalls.
No behavior change beyond keeping the test compiling with the updated API.Also applies to: 9-29
tests/Disconnected.test.ts (1)
14-14: LGTM: updated spawn initialization for both setups (normal + team) to includegameID.
This keeps spawn behavior consistent with the new deterministic seeding.Also applies to: 21-53, 208-210
src/core/execution/ExecutionManager.ts (2)
71-73: Good: spawn intent now passesthis.gameIDtoSpawnExecution.
Matches the new constructor and keeps intent-driven spawns deterministic per game.
131-137: The file name in the review comment is incorrect. The code is inExecutor.ts(withinExecutionManager.ts), notExecutionManager.ts. More importantly, the concern about type friction is unfounded. The return type narrowing toSpawnExecution[]causes no API issues because all callers use the spread operator (...this.execManager.spawnBots()) when passing results togame.addExecution(...exec: Execution[]). SinceSpawnExecutionimplementsExecution, the spread operator correctly distributes the narrower type into the variadic parameter accepting the broader type. The current return type is precise and appropriate.Likely an incorrect or invalid review comment.
tests/Donate.test.ts (4)
5-5: LGTM: troop donation tests now useSpawnExecution(gameID, ...).Also applies to: 10-42
78-79: LGTM: gold donation test updated forSpawnExecution(gameID, ...).Also applies to: 103-106
143-144: LGTM: non-ally troop donation setup updated for the new spawn signature.Also applies to: 168-171
204-205: LGTM: non-ally gold donation setup updated for the new spawn signature.Also applies to: 229-232
tests/Attack.test.ts (1)
12-13: LGTM: updated both attacker/defender spawns to passgameID.Also applies to: 18-66
tests/MissileSilo.test.ts (2)
12-16: Good: tests updated for newSpawnExecution(gameID, ...)signature.
The typedGameIDconstant keeps the callsite consistent with the new API.
45-51: Good: spawn setup now passesgameIDfirst.tests/DeleteUnitExecution.test.ts (2)
12-19: Good: adoptGameIDin test setup.
49-60: Good: both players’ spawns now includegameIDinSpawnExecution.src/core/execution/BotSpawner.ts (1)
17-22: Constructor change looks fine: keepgameIDon the spawner.tests/core/executions/SAMLauncherExecution.test.ts (2)
12-18: Good: test now uses typedGameIDfor spawn setup.
58-79: Good: all SpawnExecution calls updated tonew SpawnExecution(gameID, ...).tests/core/game/GameImpl.test.ts (2)
1-16: Good: introduceGameIDconstant for consistent spawning in tests.
48-59: Good: spawns now usenew SpawnExecution(gameID, ...).src/core/execution/FakeHumanExecution.ts (2)
72-84: Good: PRNG seed now includes both nation id andgameID.
92-96: Good: ensure the nation player exists up-front.src/core/execution/utils/PlayerSpawner.ts (1)
8-11: Good: keepgameIDon PlayerSpawner for SpawnExecution creation.src/core/execution/SpawnExecution.ts (6)
1-9: Imports look good.The new imports support the randomization logic cleanly.
43-48: Verify spawn tile reassignment behavior.The
??=operator meanstileis assigned only once per SpawnExecution instance. If multiple SpawnExecution instances are created for the same player (the scenario mentioned in the past review comment), each instance will pick its own random tile. Within a single instance, the tile won't change across ticks.Is this the intended behavior? The PR description mentions fixing "jump around" cases where previous territory wasn't deleted. Line 50 does relinquish all tiles, which addresses that cleanup. However, clarify whether:
- A single SpawnExecution instance should be reused and update its tile on each tick, OR
- Multiple SpawnExecution instances are created (each with its own tile), and line 50's cleanup handles the territory removal
50-53: Territory cleanup handles respawn correctly.Relinquishing all tiles before conquering spawn tiles ensures previous territory is cleaned up when a player respawns at a new location. This addresses the "jump around" issue mentioned in the PR description.
62-62: Spawn state tracking shifted to spawn tile reference.Setting the spawn tile instead of a boolean
hasSpawnedflag provides more precise state tracking and enables distance calculations for future spawns.
112-117: Clean tile randomization helper.Simple and correct random coordinate generation within map bounds.
73-110: Spawn logic is well-structured.The bounded search properly validates tiles (land, unowned, not border) and enforces minimum distance between players. The code correctly handles undefined spawn tiles—it returns false and continues searching when a player hasn't spawned yet. If no valid tile is found after MAX_SPAWN_TRIES attempts, it returns undefined, which the caller should handle.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/core/execution/SpawnExecution.ts (1)
16-24: Consider hashing the combined string for stronger uniqueness.The current implementation adds two hashes (
simpleHash(playerInfo.id) + simpleHash(gameID)), which could theoretically produce collisions where different player-game pairs yield the same sum. A more robust approach combines the strings before hashing.Apply this diff for a stronger guarantee of unique seeds:
constructor( gameID: GameID, private playerInfo: PlayerInfo, public tile?: TileRef, ) { - this.random = new PseudoRandom( - simpleHash(playerInfo.id) + simpleHash(gameID), - ); + this.random = new PseudoRandom(simpleHash(playerInfo.id + gameID)); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/core/execution/BotSpawner.ts(1 hunks)src/core/execution/SpawnExecution.ts(4 hunks)tests/core/execution/SpawnExecution.test.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (7)
📚 Learning: 2025-08-28T22:47:31.406Z
Learnt from: BrewedCoffee
Repo: openfrontio/OpenFrontIO PR: 1957
File: tests/core/executions/PlayerExecution.test.ts:16-39
Timestamp: 2025-08-28T22:47:31.406Z
Learning: In test files, PlayerExecution instances must be manually registered with game.addExecution() because the setup utility doesn't automatically register SpawnExecution, which would normally handle this during the spawn phase in a real game.
Applied to files:
tests/core/execution/SpawnExecution.test.tssrc/core/execution/BotSpawner.tssrc/core/execution/SpawnExecution.ts
📚 Learning: 2025-12-13T14:58:22.026Z
Learnt from: scamiv
Repo: openfrontio/OpenFrontIO PR: 2607
File: src/core/execution/PlayerExecution.ts:271-295
Timestamp: 2025-12-13T14:58:22.026Z
Learning: In src/core/execution/PlayerExecution.ts surroundedBySamePlayer(), the `as Player` cast on `mg.playerBySmallID(scan.enemyId)` is intentional. Since scan.enemyId comes from ownerID() on an owned tile and playerBySmallID() only returns Player or undefined, the cast expresses a known invariant. The maintainers prefer loud failures (runtime errors) over silent masking (early returns with guards) for corrupted game state scenarios at trusted call sites.
Applied to files:
tests/core/execution/SpawnExecution.test.tssrc/core/execution/BotSpawner.tssrc/core/execution/SpawnExecution.ts
📚 Learning: 2025-10-20T20:15:28.858Z
Learnt from: sambokai
Repo: openfrontio/OpenFrontIO PR: 2225
File: src/core/execution/FakeHumanExecution.ts:51-51
Timestamp: 2025-10-20T20:15:28.858Z
Learning: In src/core/execution/FakeHumanExecution.ts, game balance constants like MIRV_COOLDOWN_TICKS, MIRV_HESITATION_ODDS, VICTORY_DENIAL_TEAM_THRESHOLD, VICTORY_DENIAL_INDIVIDUAL_THRESHOLD, and STEAMROLL_CITY_GAP_MULTIPLIER are experimental tuning parameters subject to frequent change during balance testing. Do not flag changes to these values as issues or compare them against previous values.
Applied to files:
tests/core/execution/SpawnExecution.test.tssrc/core/execution/BotSpawner.tssrc/core/execution/SpawnExecution.ts
📚 Learning: 2025-11-26T20:49:29.140Z
Learnt from: scamiv
Repo: openfrontio/OpenFrontIO PR: 2519
File: src/core/game/GameView.ts:516-525
Timestamp: 2025-11-26T20:49:29.140Z
Learning: In GameView.ts, when usesSharedTileState is true (SAB mode), packedTileUpdates contains unpacked tile references as BigInt(tileRef) only, because all tile state lives in the shared Uint16Array. In non-SAB mode, packedTileUpdates contains packed TileUpdate bigints in the format (tileRef << 16n | state), which must be decoded via updateTile(tu). Therefore, Number(tu) is correct in SAB mode and shifting right by 16 bits would be wrong.
Applied to files:
src/core/execution/BotSpawner.ts
📚 Learning: 2025-10-26T15:37:07.732Z
Learnt from: GlacialDrift
Repo: openfrontio/OpenFrontIO PR: 2298
File: src/client/graphics/layers/TerritoryLayer.ts:200-210
Timestamp: 2025-10-26T15:37:07.732Z
Learning: In GameImpl.ts lines 124-139, team assignment logic varies by number of teams: when numPlayerTeams < 8, teams are assigned ColoredTeams values (Red, Blue, Yellow, Green, Purple, Orange, Teal); when numPlayerTeams >= 8, teams are assigned generic string identifiers like "Team 1", "Team 2", etc., which are not members of ColoredTeams.
Applied to files:
src/core/execution/BotSpawner.tssrc/core/execution/SpawnExecution.ts
📚 Learning: 2025-10-20T11:02:16.969Z
Learnt from: sambokai
Repo: openfrontio/OpenFrontIO PR: 2225
File: src/core/execution/FakeHumanExecution.ts:57-57
Timestamp: 2025-10-20T11:02:16.969Z
Learning: In src/core/execution/FakeHumanExecution.ts, the correct MIRV victory denial thresholds are VICTORY_DENIAL_TEAM_THRESHOLD = 0.8 (80% for team games) and VICTORY_DENIAL_INDIVIDUAL_THRESHOLD = 0.65 (65% for individual players), not 0.85 and 0.7 as might be mentioned in some documentation.
Applied to files:
src/core/execution/SpawnExecution.ts
📚 Learning: 2025-08-23T07:48:19.060Z
Learnt from: ElMelchizedek
Repo: openfrontio/OpenFrontIO PR: 1876
File: src/core/execution/FakeHumanExecution.ts:470-473
Timestamp: 2025-08-23T07:48:19.060Z
Learning: In FakeHumanExecution.ts DefensePost placement logic, returning -Infinity from structureSpawnTileValue when no sampled border tiles neighbor enemies is intentional. The logic samples up to 50 border tiles as a heuristic - if none are adjacent to enemies, it assumes DefensePost placement is unnecessary and aborts the entire placement attempt rather than continuing to evaluate individual tiles.
Applied to files:
src/core/execution/SpawnExecution.ts
🧬 Code graph analysis (3)
tests/core/execution/SpawnExecution.test.ts (5)
src/core/game/Game.ts (1)
PlayerInfo(409-423)src/core/execution/SpawnExecution.ts (1)
SpawnExecution(10-120)tests/util/Setup.ts (2)
playerInfo(84-86)setup(23-82)src/server/GameManager.ts (1)
game(25-27)src/core/game/PlayerImpl.ts (1)
spawnTile(354-356)
src/core/execution/BotSpawner.ts (3)
src/core/Schemas.ts (1)
GameID(25-25)src/core/Util.ts (1)
simpleHash(64-72)src/core/execution/SpawnExecution.ts (1)
SpawnExecution(10-120)
src/core/execution/SpawnExecution.ts (5)
src/core/PseudoRandom.ts (1)
PseudoRandom(3-76)src/core/Schemas.ts (1)
GameID(25-25)src/core/game/GameMap.ts (1)
TileRef(3-3)src/core/Util.ts (1)
simpleHash(64-72)src/core/game/PlayerImpl.ts (1)
spawnTile(354-356)
🔇 Additional comments (8)
tests/core/execution/SpawnExecution.test.ts (2)
8-57: LGTM! Comprehensive spawn validation test.The parameterized test properly validates spawn mechanics across multiple maps with varying player capacities. The assertions correctly verify:
- Each player gets a valid land tile (not border)
- All player pairs maintain minimum distance requirements
- The spawn phase completes successfully
59-89: LGTM! Proper validation of spawn failure on crowded maps.The test correctly validates that SpawnExecution respects map capacity and fails gracefully when insufficient valid spawn locations exist.
src/core/execution/BotSpawner.ts (3)
17-22: LGTM! Clean TypeScript parameter property.The constructor properly stores gameID for use in spawn execution construction.
24-36: LGTM! Simplified spawn logic delegates tile validation to SpawnExecution.The refactored method correctly:
- Creates the requested number of bot spawns
- Increments nameIndex only for list-based names (not random names)
- Unconditionally adds spawns (tile validation now handled by SpawnExecution)
38-43: LGTM! Correctly constructs SpawnExecution with gameID.The method now returns a non-nullable SpawnExecution and delegates tile selection to SpawnExecution's internal logic.
src/core/execution/SpawnExecution.ts (3)
3-5: LGTM! Clean imports and field declarations.The new dependencies support deterministic random spawn selection with bounded retry attempts.
Also applies to: 11-11, 14-14
45-50: LGTM! Clean lazy spawn tile assignment with proper error handling.The nullish coalescing operator (
??=) elegantly handles lazy tile assignment, and the early return with warning appropriately handles spawn failures on crowded maps.Also applies to: 64-64
114-119: LGTM! Simple and correct random tile generation.The helper correctly generates random coordinates within map bounds using the deterministic PRNG.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/core/execution/SpawnExecution.ts (1)
30-36: Redundant assignment ofthis.active.Line 31 sets
this.active = false, then line 34 sets it again. The second assignment is unnecessary since it's already false.Apply this diff to remove redundancy:
tick(ticks: number) { this.active = false; if (!this.mg.inSpawnPhase()) { - this.active = false; return; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/core/execution/SpawnExecution.ts(4 hunks)tests/core/execution/SpawnExecution.test.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/core/execution/SpawnExecution.test.ts
🧰 Additional context used
🧠 Learnings (11)
📚 Learning: 2025-10-20T20:15:28.858Z
Learnt from: sambokai
Repo: openfrontio/OpenFrontIO PR: 2225
File: src/core/execution/FakeHumanExecution.ts:51-51
Timestamp: 2025-10-20T20:15:28.858Z
Learning: In src/core/execution/FakeHumanExecution.ts, game balance constants like MIRV_COOLDOWN_TICKS, MIRV_HESITATION_ODDS, VICTORY_DENIAL_TEAM_THRESHOLD, VICTORY_DENIAL_INDIVIDUAL_THRESHOLD, and STEAMROLL_CITY_GAP_MULTIPLIER are experimental tuning parameters subject to frequent change during balance testing. Do not flag changes to these values as issues or compare them against previous values.
Applied to files:
src/core/execution/SpawnExecution.ts
📚 Learning: 2025-12-13T14:58:22.026Z
Learnt from: scamiv
Repo: openfrontio/OpenFrontIO PR: 2607
File: src/core/execution/PlayerExecution.ts:271-295
Timestamp: 2025-12-13T14:58:22.026Z
Learning: In src/core/execution/PlayerExecution.ts surroundedBySamePlayer(), the `as Player` cast on `mg.playerBySmallID(scan.enemyId)` is intentional. Since scan.enemyId comes from ownerID() on an owned tile and playerBySmallID() only returns Player or undefined, the cast expresses a known invariant. The maintainers prefer loud failures (runtime errors) over silent masking (early returns with guards) for corrupted game state scenarios at trusted call sites.
Applied to files:
src/core/execution/SpawnExecution.ts
📚 Learning: 2025-10-20T11:02:16.969Z
Learnt from: sambokai
Repo: openfrontio/OpenFrontIO PR: 2225
File: src/core/execution/FakeHumanExecution.ts:57-57
Timestamp: 2025-10-20T11:02:16.969Z
Learning: In src/core/execution/FakeHumanExecution.ts, the correct MIRV victory denial thresholds are VICTORY_DENIAL_TEAM_THRESHOLD = 0.8 (80% for team games) and VICTORY_DENIAL_INDIVIDUAL_THRESHOLD = 0.65 (65% for individual players), not 0.85 and 0.7 as might be mentioned in some documentation.
Applied to files:
src/core/execution/SpawnExecution.ts
📚 Learning: 2025-10-26T15:37:07.732Z
Learnt from: GlacialDrift
Repo: openfrontio/OpenFrontIO PR: 2298
File: src/client/graphics/layers/TerritoryLayer.ts:200-210
Timestamp: 2025-10-26T15:37:07.732Z
Learning: In GameImpl.ts lines 124-139, team assignment logic varies by number of teams: when numPlayerTeams < 8, teams are assigned ColoredTeams values (Red, Blue, Yellow, Green, Purple, Orange, Teal); when numPlayerTeams >= 8, teams are assigned generic string identifiers like "Team 1", "Team 2", etc., which are not members of ColoredTeams.
Applied to files:
src/core/execution/SpawnExecution.ts
📚 Learning: 2025-08-23T07:48:19.060Z
Learnt from: ElMelchizedek
Repo: openfrontio/OpenFrontIO PR: 1876
File: src/core/execution/FakeHumanExecution.ts:470-473
Timestamp: 2025-08-23T07:48:19.060Z
Learning: In FakeHumanExecution.ts DefensePost placement logic, returning -Infinity from structureSpawnTileValue when no sampled border tiles neighbor enemies is intentional. The logic samples up to 50 border tiles as a heuristic - if none are adjacent to enemies, it assumes DefensePost placement is unnecessary and aborts the entire placement attempt rather than continuing to evaluate individual tiles.
Applied to files:
src/core/execution/SpawnExecution.ts
📚 Learning: 2025-10-08T17:14:49.369Z
Learnt from: Foorack
Repo: openfrontio/OpenFrontIO PR: 2141
File: src/client/ClientGameRunner.ts:228-234
Timestamp: 2025-10-08T17:14:49.369Z
Learning: In `ClientGameRunner.ts`, the `myPlayer` field is always set when `shouldPreventWindowClose()` is called, so the null check in that method is sufficient without needing to fetch it again from `gameView.playerByClientID()`.
Applied to files:
src/core/execution/SpawnExecution.ts
📚 Learning: 2025-10-08T17:14:49.369Z
Learnt from: Foorack
Repo: openfrontio/OpenFrontIO PR: 2141
File: src/client/ClientGameRunner.ts:228-234
Timestamp: 2025-10-08T17:14:49.369Z
Learning: For the window close confirmation feature in `ClientGameRunner.ts`, the troop count requirement (>10,000 troops) from issue #2137 was intentionally removed because it was arbitrary and troop count can be reported as low despite having significant land. The confirmation now triggers for any alive player regardless of troop count.
Applied to files:
src/core/execution/SpawnExecution.ts
📚 Learning: 2025-10-27T09:47:26.395Z
Learnt from: sambokai
Repo: openfrontio/OpenFrontIO PR: 2225
File: src/core/execution/FakeHumanExecution.ts:770-795
Timestamp: 2025-10-27T09:47:26.395Z
Learning: In src/core/execution/FakeHumanExecution.ts, the selectSteamrollStopTarget() method intentionally allows MIRV targeting when secondHighest city count is 0 (e.g., nuclear endgame scenarios where structures are destroyed). This is valid game design—"if you can afford it, you're good to go"—and should not be flagged as requiring a guard condition.
Applied to files:
src/core/execution/SpawnExecution.ts
📚 Learning: 2025-11-29T22:22:37.178Z
Learnt from: scamiv
Repo: openfrontio/OpenFrontIO PR: 2539
File: src/core/game/GameImpl.ts:520-542
Timestamp: 2025-11-29T22:22:37.178Z
Learning: In GameImpl.ts, neighborsWithDiag() and forEachNeighborWithDiag() intentionally duplicate coordinate iteration logic. They serve different purposes: forEachNeighborWithDiag() is a zero-allocation hot-path optimization while neighborsWithDiag() is a convenience method that returns an array. Refactoring one to call the other would add callback/closure allocations and indirection overhead, defeating the performance goals.
Applied to files:
src/core/execution/SpawnExecution.ts
📚 Learning: 2025-10-18T17:54:01.311Z
Learnt from: sambokai
Repo: openfrontio/OpenFrontIO PR: 2225
File: src/core/execution/FakeHumanExecution.ts:172-173
Timestamp: 2025-10-18T17:54:01.311Z
Learning: In src/core/execution/FakeHumanExecution.ts, MIRVs and ground attacks should not be mutually exclusive. The considerMIRV() method should not short-circuit maybeAttack() - both actions can occur in the same tick.
Applied to files:
src/core/execution/SpawnExecution.ts
📚 Learning: 2025-08-28T22:47:31.406Z
Learnt from: BrewedCoffee
Repo: openfrontio/OpenFrontIO PR: 1957
File: tests/core/executions/PlayerExecution.test.ts:16-39
Timestamp: 2025-08-28T22:47:31.406Z
Learning: In test files, PlayerExecution instances must be manually registered with game.addExecution() because the setup utility doesn't automatically register SpawnExecution, which would normally handle this during the spawn phase in a real game.
Applied to files:
src/core/execution/SpawnExecution.ts
🧬 Code graph analysis (1)
src/core/execution/SpawnExecution.ts (6)
src/core/game/Game.ts (3)
Execution(354-359)Game(668-763)PlayerInfo(409-423)src/core/PseudoRandom.ts (1)
PseudoRandom(3-76)src/core/Schemas.ts (1)
GameID(25-25)src/core/game/GameMap.ts (1)
TileRef(3-3)src/core/Util.ts (1)
simpleHash(64-72)src/core/game/PlayerImpl.ts (1)
spawnTile(354-356)
🔇 Additional comments (5)
src/core/execution/SpawnExecution.ts (5)
10-14: Clean class setup with sensible constants.The
MAX_SPAWN_TRIESlimit of 10,000 is a good safeguard against infinite loops during spawn tile search. The privaterandomfield keeps RNG state encapsulated.
16-24: Unique seeding per player addresses the collision issue.The seed now combines
playerInfo.idandgameIDhashes, ensuring each player gets a distinct random sequence. This correctly addresses the prior review feedback.
45-64: Lazy tile assignment and territory cleanup look correct.The
??=operator cleanly handles both explicit tile and random spawn cases. Therelinquishloop on line 52 properly clears previous territory when a player "jumps around" - this addresses the PR objective. CallingsetSpawnTile()at the end correctly tracks the spawn location.
75-115: Random spawn logic is clean and correctly excludes current player.The method properly filters out the spawning player from the distance check (line 93), which addresses the earlier review feedback. The validation sequence (land check → owner check → border check → distance check) is logical and efficient since early checks are cheaper.
117-122: Simple and correct random tile helper.The
nextIntcalls correctly use exclusive upper bounds matching the map dimensions.
Description:
This is a previously approved PR with an additional commit that fixes case when nations change spawn & jump around, their previous territory wasn't getting deleted.
Please complete the following:
Please put your Discord username so you can be contacted if a bug or regression is found:
nikolaj_mykola