Start game instantly after spawning in Singleplayer #2551
Conversation
…o feat-sp-timer fix merge errors
|
|
WalkthroughAdded GameType-aware start delays, spawn-phase detection with a first-human-spawn grace window, and related config accessors; made spawn and fake-human executions respect player existence and spawn-phase rules; applied startDelay in GameManager and updated test server config. Changes
Sequence Diagram(s)sequenceDiagram
participant GM as GameManager
participant CFG as Config / ServerConfig
participant G as Game (GameImpl / GameView)
participant SE as SpawnExecution / FakeHumanExecution
GM->>CFG: startDelay(game.gameConfig.gameType)
CFG-->>GM: delay ms (0 for Singleplayer, 2000 otherwise)
GM->>GM: schedule setTimeout(start, delay)
Note over GM,G: When start runs
GM->>G: start()
G->>G: on tick -> inSpawnPhase(ticks)
G->>CFG: isSpawnPhase(ticks, gameType, firstHumanSpawnTick?)
CFG-->>G: true/false
alt spawn phase == true
G->>SE: run spawn/fake-human executions (guarded by player existence)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used🧠 Learnings (2)📚 Learning: 2025-08-12T00:31:50.144ZApplied to files:
📚 Learning: 2025-10-20T20:15:28.858ZApplied 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 (5)
src/core/execution/SpawnExecution.ts (1)
40-54: Redundant null check on line 46.Inside the
if (player)block at line 40, the checkplayer &&on line 46 is unnecessary sinceplayeris already guaranteed to be truthy.if (player) { player.tiles().forEach((t) => player.relinquish(t)); getSpawnTiles(this.mg, this.tile).forEach((t) => { player.conquer(t); }); - if (player && !player.hasSpawned()) { + if (!player.hasSpawned()) { this.mg.addExecution(new PlayerExecution(player)); if (player.type() === PlayerType.Bot) { this.mg.addExecution(new BotExecution(player)); } player.setHasSpawned(true); } }src/core/game/GameImpl.ts (1)
73-74: Initialize field explicitly.
firstHumanSpawnTurnis declared but not initialized. While the logic handlesundefinedas falsy, explicit initialization improves clarity and prevents strict-mode TypeScript issues.- private firstHumanSpawnTurn: number; - + private firstHumanSpawnTurn: number = 0;src/core/game/GameView.ts (2)
479-480: InitializefirstHumanSpawnTurnto avoid undefined behavior.The field is declared but not initialized. TypeScript will set it to
undefined, but the code treats0as falsy (unset) in the condition at line 665. Initialize it explicitly for clarity.- private firstHumanSpawnTurn: number; - + private firstHumanSpawnTurn: number = 0;
660-680: Side effect in query method; consider separating concerns.
inSpawnPhase()looks like a pure query but mutatesfirstHumanSpawnTurn. This can cause surprising behavior if called multiple times per tick or in different contexts.Consider extracting the spawn detection into a separate method called from
update(), keepinginSpawnPhase()as a pure query.That said, the logic is correct for the intended behavior. The flow:
- If ticks exceed spawn phase turns → not in spawn phase
- For Singleplayer: track first human spawn, then enforce grace period after that tick
- Otherwise → in spawn phase
Minor note on line 666-670: The
Array.from(this._players.values()).some(...)creates a new array each tick until a human spawns. For a small optimization, you could usethis._players.values()directly with a loop or iterator-based check.src/core/configuration/DefaultConfig.ts (1)
175-184: LGTM! Game-type aware timing implementation.The implementation is clean:
- Singleplayer gets 10ms intervals (10x faster) and 0ms start delay
- Multiplayer keeps the existing 100ms intervals and 2000ms delay
The conditional check
gameType && gameType === GameType.Singleplayercan be simplified to justgameType === GameType.Singleplayersince the equality check handles undefined.turnIntervalMs(gameType?: GameType): number { - if (gameType && gameType === GameType.Singleplayer) { + if (gameType === GameType.Singleplayer) { return 10; } return 100; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
src/core/GameRunner.ts(1 hunks)src/core/configuration/Config.ts(4 hunks)src/core/configuration/DefaultConfig.ts(3 hunks)src/core/execution/BotExecution.ts(1 hunks)src/core/execution/FakeHumanExecution.ts(1 hunks)src/core/execution/SpawnExecution.ts(1 hunks)src/core/game/GameImpl.ts(3 hunks)src/core/game/GameView.ts(3 hunks)src/server/GameManager.ts(1 hunks)src/server/GameServer.ts(1 hunks)tests/util/TestServerConfig.ts(2 hunks)
🧰 Additional context used
🧠 Learnings (12)
📓 Common learnings
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.
📚 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/configuration/Config.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:
src/core/configuration/Config.tssrc/core/execution/BotExecution.tssrc/core/GameRunner.tssrc/server/GameManager.tssrc/core/game/GameImpl.tssrc/core/execution/FakeHumanExecution.tssrc/core/configuration/DefaultConfig.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/BotExecution.tssrc/core/GameRunner.tssrc/core/execution/SpawnExecution.tssrc/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/GameRunner.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/GameRunner.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:
src/core/GameRunner.tssrc/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/GameRunner.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:
src/core/GameRunner.tssrc/core/execution/FakeHumanExecution.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/game/GameImpl.tssrc/core/configuration/DefaultConfig.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/game/GameImpl.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/FakeHumanExecution.ts
🧬 Code graph analysis (4)
src/core/game/GameView.ts (1)
src/core/game/GameImpl.ts (1)
player(493-499)
src/core/execution/SpawnExecution.ts (3)
src/core/game/GameImpl.ts (1)
player(493-499)src/core/game/GameView.ts (1)
player(612-618)src/core/execution/Util.ts (1)
getSpawnTiles(4-8)
src/server/GameManager.ts (2)
src/core/configuration/DefaultConfig.ts (1)
startDelay(182-184)tests/util/TestServerConfig.ts (1)
startDelay(67-69)
src/core/execution/FakeHumanExecution.ts (2)
src/core/game/GameImpl.ts (1)
ticks(365-367)src/core/game/GameView.ts (1)
ticks(656-659)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: 🔬 Test
🔇 Additional comments (10)
src/core/GameRunner.ts (1)
118-119: LGTM!Adding
WinCheckExecutionunconditionally during init makes sense. This ensures win-condition checks run as part of the game's initialization flow, regardless of spawn configuration.tests/util/TestServerConfig.ts (1)
67-69: LGTM!The new
startDelaymethod follows the same stub pattern as other methods in this test config class. Consistent with the interface contract.src/server/GameServer.ts (1)
433-434: LGTM!Good refactor: extracting
turnIntervalinto a local variable makes the code clearer. PassinggameTypeenables different tick rates for singleplayer vs. multiplayer, which is the core of this PR's timing changes.src/core/execution/BotExecution.ts (1)
54-57: LGTM!Minor formatting change only. The first-tick attack logic remains intact.
src/core/execution/FakeHumanExecution.ts (1)
152-155: LGTM!The condition
ticks > 1 &&ensures FakeHuman nations can spawn on the first ticks (0 and 1) without waiting for their randomattackTick. This aligns with the PR goal: nations spawn immediately so players cannot predict and exploit spawn positions.src/server/GameManager.ts (1)
91-99: LGTM!Good change. Using
config.startDelay(game.gameConfig.gameType)replaces the hardcoded 2000ms with a game-type-aware value. Singleplayer gets 0ms (instant start), multiplayer keeps 2000ms. The comment at line 91 explains the intent clearly.src/core/game/GameImpl.ts (1)
343-362: LGTM!The Singleplayer grace-period logic is well-structured:
- Exit early if past maximum spawn phase turns.
- For Singleplayer, track when first human spawns and allow a grace period after.
- For other modes, return true (standard spawn phase behavior).
This fulfills the PR goal: players get a short window to adjust placement in Singleplayer while nations spawn immediately.
src/core/configuration/Config.ts (2)
30-32: LGTM! Clean interface additions.The API design is good:
turnIntervalMs(gameType?)keeps backward compatibility with the optional parameterstartDelay(gameType)requires the game type, which is correct since the delay is inherently game-type dependent
77-77: Config interface additions look good.The
turnIntervalMsandnumSingleplayerGracePeriodTurnsmethods align well with the server config interface. The grace period method name is clear about its purpose.Also applies to: 96-96
src/core/configuration/DefaultConfig.ts (1)
283-286: LGTM! Clean delegation to server config.The wrapper correctly passes the optional
gameTypeparameter through to the underlying server config.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/core/configuration/Config.ts (1)
8-9: Spawn / timing API surface looks consistent; only a small type nitThe added
startDelay,turnIntervalMs,numGracePeriodTurns, andisSpawnPhasemethods line up with the implementations inDefaultServerConfig/DefaultConfigand give a clear central place for timing rules. One small optional improvement: you could type theticksparameter ofisSpawnPhaseasTickinstead ofnumberfor consistency with other time-based methods that already use theTickalias.Also applies to: 30-33, 76-101
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/core/configuration/Config.ts(4 hunks)src/core/configuration/DefaultConfig.ts(3 hunks)src/core/game/GameImpl.ts(2 hunks)src/core/game/GameView.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/core/game/GameView.ts
🧰 Additional context used
🧠 Learnings (7)
📚 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/game/GameImpl.tssrc/core/configuration/Config.tssrc/core/configuration/DefaultConfig.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:
src/core/game/GameImpl.tssrc/core/configuration/Config.tssrc/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.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/configuration/DefaultConfig.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/configuration/DefaultConfig.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/configuration/DefaultConfig.ts
📚 Learning: 2025-05-19T06:00:38.007Z
Learnt from: scottanderson
Repo: openfrontio/OpenFrontIO PR: 784
File: src/core/game/StatsImpl.ts:125-134
Timestamp: 2025-05-19T06:00:38.007Z
Learning: In StatsImpl.ts, unused parameters in boat/stats-related methods are intentionally kept for future use and shouldn't be removed.
Applied to files:
src/core/configuration/DefaultConfig.ts
🧬 Code graph analysis (2)
src/core/configuration/Config.ts (2)
src/core/game/GameImpl.ts (1)
ticks(357-359)src/core/game/GameView.ts (1)
ticks(655-658)
src/core/configuration/DefaultConfig.ts (2)
src/core/game/GameImpl.ts (1)
ticks(357-359)src/core/game/GameView.ts (1)
ticks(655-658)
🪛 Biome (2.1.2)
src/core/game/GameImpl.ts
[error] 354-354: This code will never be reached ...
... because this statement will return from the function beforehand
(lint/correctness/noUnreachable)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: 🔬 Test
🔇 Additional comments (1)
src/core/configuration/DefaultConfig.ts (1)
175-181: Timing configuration and start delay wiring look good
DefaultServerConfig.startDelay(0ms for Singleplayer, 2000ms otherwise) matches the PR goal of instant SP start while keeping the existing MP lobby delay, andDefaultConfig.turnIntervalMs()cleanly delegates to the server config so there is one place to change tick duration. No issues from my side here.Also applies to: 280-283
4 participants
Resolves #1041
Replaces #1658, as the old owner seemingly did not want to work on it anymore.
Description:
This PR inproves Singleplayer UX by skipping the wait time after spawning in SP.
Top-level features
Code changes
The main changes are in ClientGameRunner and GameImpl, specifically in the definition of the spawn period.
These files track the turn number when the player clicks, and only starts the game after the number of turns defined in DefaultConfig.
Additionally, this PR changes how the delay before a game is calculated. All of the logic is encapsulated in defaultconfig, and takes gameType as an optional parameter, as per scottanderson's feedback. If no value is provided, the old behaviour persists.
Please complete the following:
Please put your Discord username so you can be contacted if a bug or regression is found:
Lavodan