Improve random spawn #2503
Improve random spawn #2503
Conversation
…y wasn't getting deleted
WalkthroughAdds configurable minimum spawn distance, records players' spawn tiles, and updates bot/player/fake-human spawn logic to enforce proximity checks and set spawn tiles before producing spawn executions. Changes
Sequence DiagramsequenceDiagram
participant Spawner as Spawner (Bot/Player/Fake)
participant Config as Config
participant Game as GameState
participant Player as Player
participant Caller as Caller
Spawner->>Config: minDistanceBetweenPlayers()
Config-->>Spawner: distance
Spawner->>Game: query players' spawnTile()
Game-->>Spawner: list of TileRefs
Spawner->>Spawner: evaluate candidate tile vs list
alt conflict (within min distance)
Spawner-->>Caller: abort spawn (null)
else no conflict
Spawner->>Player: setSpawnTile(tile)
Player-->>Player: store _spawnTile
Spawner->>Spawner: create SpawnExecution(player, tile)
Spawner-->>Caller: return SpawnExecution
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (2)
🧰 Additional context used🧠 Learnings (3)📚 Learning: 2025-11-12T23:11:34.445ZApplied to files:
📚 Learning: 2025-10-20T20:15:28.858ZApplied to files:
📚 Learning: 2025-10-20T11:02:16.969ZApplied to files:
🔇 Additional comments (1)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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
}
| 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?
|
|
||
| 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?
2 participants
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