Random spawn #2375
Random spawn #2375
Conversation
|
|
WalkthroughAdds a configurable random-spawn feature: UI toggles, translations, config plumbing, seeded PlayerSpawner that chooses valid spawn tiles, executor integration to emit spawn executions during init, client flow updates for go-to-player, server config handling, and unit tests. Changes
Sequence Diagram(s)sequenceDiagram
participant User as Player UI
participant Modal as Host/Single Modal
participant Server as Game Server
participant Client as ClientGameRunner
participant Executor as ExecutionManager
participant Spawner as PlayerSpawner
participant Game as Game State
User->>Modal: toggle randomSpawn
Modal->>Server: putGameConfig({ randomSpawn: true })
Server-->>Modal: ack
Note over Client,Executor: Game start (config.randomSpawn = true)
Client->>Executor: request spawnPlayers()
Executor->>Spawner: new PlayerSpawner(game, gameID).spawnPlayers()
Spawner->>Spawner: seed PRNG with gameID
loop per human player
Spawner->>Spawner: pick random tile
alt valid tile (land, unowned, not border, >= MIN_DISTANCE)
Spawner->>Executor: add SpawnExecution(player, tile)
else none found after tries
Spawner->>Spawner: log warning
end
end
Spawner-->>Executor: SpawnExecution[]
Executor-->>Client: apply spawn executions
Client->>Client: emit GoToPlayerEvent (local player)
Client-->>User: show assigned spawns
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks✅ Passed checks (2 passed)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (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: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
resources/lang/eo.json (1)
321-321: Unrelated change to performance overlay description.Line 321 modifies
performance_overlay_descby adding "D dum la ludo por ŝalti." This change doesn't appear related to the random spawn feature. If this is an intentional fix or improvement, it would be better to:
- Call it out in the PR description, or
- Submit it as a separate PR for clarity
If it's unintentional, please revert this change.
♻️ Duplicate comments (2)
resources/lang/fi.json (1)
138-138: Non-English locale files should not be updated in this PR.Same issue as other locale files - fi.json should only be updated in dedicated translation PRs by Aotumori, not in regular feature PRs.
Based on learnings
Also applies to: 235-235, 587-588
resources/lang/gl.json (1)
136-136: Non-English locale files should not be updated in this PR.Same issue - gl.json should only be updated in dedicated translation PRs by Aotumori.
Based on learnings
Also applies to: 221-221, 549-550
🧹 Nitpick comments (2)
src/core/execution/PlayerSpawner.ts (2)
63-63: Fix typo in comment."nornally" should be "normally".
26-52: Consider making spawn spacing configurable.The hardcoded 50-pixel minimum distance between spawns might not be appropriate for all map sizes. For smaller maps, spawns might be too crowded; for larger maps, they might be unnecessarily far apart.
Consider deriving the spacing from map dimensions or making it a configuration parameter.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (48)
resources/lang/ar.json(3 hunks)resources/lang/bg.json(3 hunks)resources/lang/bn.json(3 hunks)resources/lang/cs.json(3 hunks)resources/lang/da.json(3 hunks)resources/lang/de.json(3 hunks)resources/lang/debug.json(3 hunks)resources/lang/en.json(3 hunks)resources/lang/eo.json(3 hunks)resources/lang/es.json(3 hunks)resources/lang/fi.json(3 hunks)resources/lang/fr.json(3 hunks)resources/lang/gl.json(3 hunks)resources/lang/he.json(3 hunks)resources/lang/hi.json(3 hunks)resources/lang/hu.json(3 hunks)resources/lang/it.json(3 hunks)resources/lang/ja.json(3 hunks)resources/lang/ko.json(3 hunks)resources/lang/mk.json(3 hunks)resources/lang/nl.json(3 hunks)resources/lang/pl.json(3 hunks)resources/lang/pt-BR.json(3 hunks)resources/lang/pt-PT.json(3 hunks)resources/lang/ru.json(3 hunks)resources/lang/sh.json(3 hunks)resources/lang/sk.json(3 hunks)resources/lang/sl.json(3 hunks)resources/lang/sv-SE.json(3 hunks)resources/lang/tp.json(3 hunks)resources/lang/tr.json(3 hunks)resources/lang/uk.json(3 hunks)resources/lang/zh-CN.json(3 hunks)src/client/ClientGameRunner.ts(1 hunks)src/client/HostLobbyModal.ts(4 hunks)src/client/SinglePlayerModal.ts(4 hunks)src/client/graphics/layers/HeadsUpMessage.ts(1 hunks)src/core/GameRunner.ts(1 hunks)src/core/Schemas.ts(1 hunks)src/core/configuration/Config.ts(1 hunks)src/core/configuration/DefaultConfig.ts(1 hunks)src/core/execution/ExecutionManager.ts(2 hunks)src/core/execution/PlayerSpawner.ts(1 hunks)src/core/game/GameView.ts(1 hunks)src/server/GameManager.ts(1 hunks)src/server/GameServer.ts(1 hunks)src/server/MapPlaylist.ts(1 hunks)tests/util/Setup.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (13)
📚 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/client/HostLobbyModal.tssrc/client/SinglePlayerModal.tstests/util/Setup.tssrc/core/GameRunner.ts
📚 Learning: 2025-06-02T14:27:37.609Z
Learnt from: andrewNiziolek
Repo: openfrontio/OpenFrontIO PR: 1007
File: resources/lang/de.json:115-115
Timestamp: 2025-06-02T14:27:37.609Z
Learning: For OpenFrontIO project: When localization keys are renamed in language JSON files, the maintainers separate technical changes from translation content updates. They wait for community translators to update the actual translation values rather than attempting to translate in the same PR. This allows technical changes to proceed while ensuring accurate translations from native speakers.
Applied to files:
resources/lang/bn.jsonresources/lang/fi.jsonresources/lang/de.jsonresources/lang/uk.jsonresources/lang/he.jsonresources/lang/mk.jsonresources/lang/pt-PT.jsonresources/lang/sl.jsonresources/lang/tr.jsonresources/lang/ko.jsonresources/lang/en.jsonresources/lang/eo.jsonresources/lang/da.jsonresources/lang/zh-CN.jsonresources/lang/sh.jsonresources/lang/debug.json
📚 Learning: 2025-08-16T10:52:08.292Z
Learnt from: TheGiraffe3
Repo: openfrontio/OpenFrontIO PR: 884
File: resources/lang/en.json:456-461
Timestamp: 2025-08-16T10:52:08.292Z
Learning: In OpenFrontIO, translation files in resources/lang/*.json (except en.json) should not be updated in regular PRs. Only dedicated translation PRs titled "mls" and made by Aotumori should update non-English locale files. Regular PRs should only update en.json when adding or modifying translation keys.
Applied to files:
resources/lang/bn.jsonresources/lang/fi.jsonresources/lang/sl.jsonresources/lang/tr.jsonresources/lang/ko.jsonresources/lang/en.jsonresources/lang/eo.json
📚 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/Schemas.tssrc/client/SinglePlayerModal.ts
📚 Learning: 2025-06-09T02:20:43.637Z
Learnt from: VariableVince
Repo: openfrontio/OpenFrontIO PR: 1110
File: src/client/Main.ts:293-295
Timestamp: 2025-06-09T02:20:43.637Z
Learning: In src/client/Main.ts, during game start in the handleJoinLobby callback, UI elements are hidden using direct DOM manipulation with classList.add("hidden") for consistency. This includes modals, buttons, and error divs. The codebase follows this pattern rather than using component APIs for hiding elements during game transitions.
Applied to files:
src/client/HostLobbyModal.tssrc/client/SinglePlayerModal.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/client/HostLobbyModal.tssrc/server/MapPlaylist.ts
📚 Learning: 2025-05-31T18:15:03.445Z
Learnt from: 1brucben
Repo: openfrontio/OpenFrontIO PR: 977
File: src/core/execution/AttackExecution.ts:123-125
Timestamp: 2025-05-31T18:15:03.445Z
Learning: The removeTroops function in PlayerImpl.ts already prevents negative troop counts by using minInt(this._troops, toInt(troops)) to ensure it never removes more troops than available.
Applied to files:
src/client/SinglePlayerModal.ts
📚 Learning: 2025-07-12T08:41:35.101Z
Learnt from: Aotumuri
Repo: openfrontio/OpenFrontIO PR: 1357
File: resources/lang/de.json:523-540
Timestamp: 2025-07-12T08:41:35.101Z
Learning: In OpenFrontIO project localization files, always check the en.json source file before flagging potential spelling errors in other language files, as some keys may intentionally use non-standard spellings that need to be consistent across all translations.
Applied to files:
resources/lang/en.jsonresources/lang/eo.json
📚 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/GameRunner.tssrc/core/execution/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/ExecutionManager.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/client/ClientGameRunner.tssrc/core/GameRunner.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/client/ClientGameRunner.tssrc/core/GameRunner.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/client/ClientGameRunner.ts
🧬 Code graph analysis (5)
src/client/HostLobbyModal.ts (2)
src/client/Utils.ts (1)
translateText(92-147)src/client/LangSelector.ts (1)
translateText(254-274)
src/client/SinglePlayerModal.ts (1)
src/client/Utils.ts (1)
translateText(92-147)
src/core/execution/ExecutionManager.ts (2)
src/core/game/Game.ts (1)
Execution(351-356)src/core/execution/PlayerSpawner.ts (1)
PlayerSpawner(8-74)
src/server/GameServer.ts (2)
src/core/configuration/DefaultConfig.ts (1)
gameConfig(266-268)src/server/MapPlaylist.ts (1)
gameConfig(79-105)
src/core/execution/PlayerSpawner.ts (6)
src/core/PseudoRandom.ts (1)
PseudoRandom(3-76)src/core/execution/SpawnExecution.ts (1)
SpawnExecution(7-61)src/core/game/Game.ts (1)
Game(664-748)src/core/Schemas.ts (1)
GameID(25-25)src/core/Util.ts (1)
simpleHash(64-72)src/core/game/GameMap.ts (1)
TileRef(3-3)
🔇 Additional comments (44)
resources/lang/hu.json (1)
137-137: Translation additions are properly placed and well-formed.The three new keys for random spawn functionality are correctly positioned across the appropriate sections (
single_modal,host_modal,heads_up_message) with Hungarian translations that are grammatically sound and contextually appropriate. The JSON structure remains valid with proper syntax.Note: Verify that all 31 translation files mentioned in the PR summary have been updated with these same keys to maintain consistency across supported languages.
Also applies to: 238-238, 593-594
resources/lang/debug.json (1)
97-97: Localization structure looks good—verify en.json has actual translations.The debug.json changes follow the reference-file pattern with self-referential dot-notation values. Placement is logical (e.g.,
random_spawnslots between related options likeinstant_buildandinfinite_gold), and naming is consistent with existing keys.However, this file only defines the structure. Confirm that
resources/lang/en.jsoncontains the actual English translations for:
single_modal.random_spawnhost_modal.random_spawnheads_up_message.spawn_selected_automaticallyBased on the PR checklist ("Processed any text displayed to the user through translateText() and added it to en.json"), these should already be present in en.json and not just stub keys.
Also applies to: 150-150, 171-172
resources/lang/pt-PT.json (2)
136-136: JSON structure is well-formed across modal sections.The additions to
single_modalandhost_modalfollow proper syntax, maintain consistent key naming (random_spawn), and align with the existing structure of each section.Also applies to: 221-221
549-550: Verify Portuguese translations with native speakers.While the JSON structure is syntactically correct and the translations appear reasonable ("Escolha um local de início" for spawn selection, "O ponto de aparecimento é selecionado automaticamente" for automatic spawning), verify accuracy with native Portuguese speakers or community translators. Based on learnings from prior localization work, maintainers have historically preferred deferring translation content to community contributors rather than translating in the same PR.
resources/lang/sh.json (1)
101-101: Verify translation workflow alignment and review translation accuracy.The PR adds complete Serbo-Croatian translations for the random spawn feature across
single_modal(line 101),host_modal(line 164), and a newheads_up_messageobject (lines 185–188). According to learnings from the project's translation practices, maintainers typically separate technical key additions from translation content updates, allowing community translators to provide accurate translations rather than including them in the same PR.Please clarify:
- Does this approach (full translations included in the PR) align with the project's current localization workflow?
- Have these Serbo-Croatian translations been reviewed by native speakers or the project's translation team?
If the project prefers community-driven translation updates, consider deferring the translation values to be added after the PR merges, or mark them for review by qualified translators.
Based on learnings
Also applies to: 164-164, 185-188
resources/lang/uk.json (1)
137-137: Clarify the localization update approach and verify scope.Based on learnings from the project, maintainers typically separate technical changes (adding localization keys) from translation content updates, allowing community translators to provide accurate translations in their native languages. This PR includes full Ukrainian translations directly.
Additionally, the
host_badgekey at line 239–240 appears unrelated to the "random spawn" feature described in the PR title. Please clarify whether it's intentional or should be addressed in a separate PR.Suggested approach (if aligned with project practice):
Consider replacing the Ukrainian translations with English placeholders or empty strings, allowing community translators to provide accurate Ukrainian translations later:
- "random_spawn": "Випадкове появлення" + "random_spawn": "Random spawn"and
- "choose_spawn": "Виберіть початкове розташування", - "spawn_selected_automatically": "Точка появи вибирається автоматично. Підготуйтеся до бою" + "choose_spawn": "Choose spawn location", + "spawn_selected_automatically": "Spawn selected automatically. Get ready for battle"Please verify this aligns with the project's localization workflow.
[Based on learnings]
Also applies to: 239-241, 593-594
resources/lang/pl.json (3)
138-138: Polish translations for random spawn toggle are clear and consistent.Both instances of
"random_spawn": "Losowe pojawienie"read naturally in Polish and maintain consistency between the single-player and host lobby modes.Also applies to: 235-235
587-588: Heads-up messages are appropriately translated and contextually suitable.The two new messages convey their purpose clearly:
choose_spawninstructs the player to select a spawn location (clear and direct)spawn_selected_automaticallywarns of automatic selection and readiness for battle (appropriate tone and urgency)Both follow the existing message style in the file and use natural, idiomatic Polish.
138-138: Verify that all translation files were updated consistently.The PR summary mentions this change spans ~31 translation files. Please confirm:
- All language files contain the same new keys (
random_spawn,spawn_selected_automatically)- All
choose_spawnkeys were updated with trailing commas (if not present before)- No translation keys are missing or inconsistent across locales
Also applies to: 235-235, 587-588
resources/lang/pt-BR.json (2)
94-94: Translations are clear and appropriately placed.The "Spawn aleatório" translation in both
single_modalandhost_modalsections is idiomatic Portuguese and correctly positioned.Also applies to: 154-154
181-184: Running a final consistency verification across different regions:All 33 language files contain the
heads_up_messagesection with consistent structure.Verification confirms that all language files in
resources/lang/have been updated with the newheads_up_messagesection. The structure is identical across all files, with each containing the two required keys:choose_spawnandspawn_selected_automatically, each with appropriate translations. Coverage is 100% (33 of 33 files) with no missing entries. The Portuguese-Brazil translation in pt-BR.json is accurate and consistent with translations in other languages.resources/lang/cs.json (1)
108-108: Translation additions look good, but check the translation update policy.The new translation keys for random spawn are properly formatted and consistently added. However, based on project learnings, non-English translation files are typically updated only in dedicated translation PRs (titled "mls" by Aotumori). Since this is your first PR, you might want to confirm with the maintainers whether these translations should be:
- Included here as part of the feature rollout, or
- Added to en.json only, with other languages updated in a follow-up translation PR
The actual translation content looks correct for the feature!
Based on learnings
Also applies to: 153-156, 195-196
resources/lang/ru.json (1)
137-137: Translation additions are consistent.Same note as for the Czech translation file - the changes look correct, but please verify with maintainers about the translation update policy for non-English locale files in feature PRs.
Based on learnings
Also applies to: 238-238, 593-594
resources/lang/fr.json (1)
137-137: French translations look accurate.The translations are well-formed and the French text appears appropriate ("Apparition aléatoire" correctly conveys "random spawn"). Same verification request as other translation files regarding the update policy.
Based on learnings
Also applies to: 238-238, 593-594
resources/lang/eo.json (1)
131-131: Random spawn translations consistent with other files.As with other translation files, please verify the translation update policy with maintainers.
Based on learnings
Also applies to: 238-238, 593-594
src/server/GameServer.ts (1)
118-120: LGTM - Config update follows established pattern.The
randomSpawnconfig handling is consistent with all other config fields inupdateGameConfig. Clean implementation.src/client/ClientGameRunner.ts (1)
404-405: LGTM - Correctly prevents manual spawn during random spawn mode.The additional
!this.gameView.isRandomSpawn()condition properly blocks manual spawn selection when random spawn is enabled. The logic is sound: players should only be able to click to spawn when random spawn is disabled.src/core/configuration/DefaultConfig.ts (1)
340-342: LGTM - Config accessor follows class pattern.The
randomSpawn()method is a straightforward accessor that matches the style and placement of surrounding config methods. Clean and consistent.src/server/MapPlaylist.ts (1)
98-98: LGTM - Sensible default for public games.Setting
randomSpawn: falseas the default for public game playlists is appropriate. This preserves the existing manual spawn behavior for public games while allowing private games and single-player to opt into random spawn.src/core/game/GameView.ts (1)
647-649: LGTM! Clean accessor method.The
isRandomSpawn()method follows the established pattern of other configuration accessors in this class. Implementation is straightforward and delegates correctly to the underlying config.src/core/configuration/Config.ts (1)
91-91: LGTM! Interface extension follows conventions.The
randomSpawn()method signature is correctly added to the Config interface and follows the established pattern for boolean configuration flags.src/server/GameManager.ts (1)
59-59: LGTM! Safe default value.Setting
randomSpawn: falseas the default is the correct choice for backward compatibility. This ensures existing behavior is preserved unless explicitly enabled.resources/lang/zh-CN.json (3)
137-137: Translation addition looks correct.The
random_spawnkey is properly added to thesingle_modalsection with appropriate Chinese translation.
239-240: Translation addition looks correct.The
random_spawnkey is properly added to thehost_modalsection. JSON structure is valid with correct comma placement.
593-594: New heads-up message translations added.The new
heads_up_messagesection correctly includes both spawn-related keys with appropriate Chinese translations.resources/lang/sk.json (3)
136-136: Translation addition looks correct.The
random_spawnkey is properly added to thesingle_modalsection with Slovak translation using consistent terminology.
221-221: Translation addition looks correct.The
random_spawnkey is properly added to thehost_modalsection with the same consistent Slovak terminology.
549-550: Heads-up message translations added correctly.The
heads_up_messagesection includes both required keys with appropriate Slovak translations.tests/util/Setup.ts (1)
71-71: LGTM! Test setup properly initialized.Adding
randomSpawn: falsebefore the spread operator is correct. This provides a sensible default while allowing test-specific overrides via_gameConfig.src/client/graphics/layers/HeadsUpMessage.ts (1)
43-45: LGTM! Clean conditional rendering.The ternary operator correctly switches between spawn messages based on the random spawn configuration. The implementation is clean and uses the appropriate translation keys.
src/core/GameRunner.ts (1)
103-105: No issues found—code is correct.Script results confirm:
ExecutionManager.spawnPlayers()correctly returnsExecution[](withSpawnExecution[]as concrete implementation)PlayerSpawner.spawnPlayers()filters forPlayerType.Humanonly, then spawns before bots- Spawn order is intentional and working as designed
The code matches the intended behavior—human players spawn first, then bots if configured.
src/core/Schemas.ts (1)
170-170: LGTM! Schema addition is clean and well-placed.The
randomSpawnboolean field fits naturally alongside the other game configuration options. The placement betweeninstantBuildandmaxPlayersis logical, and using a required boolean (without.optional()) is consistent with similar flags likeinfiniteGoldandinfiniteTroops.resources/lang/nl.json (1)
138-138: Translation workflow concern: non-English locale updatesBased on learnings, the OpenFrontIO project follows a specific translation workflow: regular PRs should only update
en.jsonwhen adding new translation keys, while non-English locale files (tr.json,de.json,bg.json, etc.) should be updated separately in dedicated translation PRs (typically titled "mls" and handled by community translators).This PR updates 7 non-English locale files directly. While the translation keys are structurally consistent across all files, this approach differs from the established workflow. Consider:
- Confirming that
en.jsonwas updated with these keys (it's not in the current review scope)- Checking with maintainers whether updating all locales in the same PR is acceptable for this feature, or if these translation updates should be extracted into a separate PR
The translation structure itself looks correct—all files consistently add
single_modal.random_spawn,host_modal.random_spawn, andheads_up_messageentries that align with the UI changes described in the PR.Based on learnings
Note: This comment applies to all translation files in this PR:
tr.json,de.json,bg.json,tp.json,it.json,ja.json, andnl.json.resources/lang/en.json (1)
138-138: LGTM! Translation keys properly added.The new translation keys for random spawn are correctly added to en.json, which is the appropriate file to update in regular PRs. The keys are well-named and consistent across the single-player and host lobby contexts.
Based on learnings
Also applies to: 266-266, 663-664
src/core/execution/ExecutionManager.ts (2)
23-23: LGTM! Import added correctly.The PlayerSpawner import is properly added and follows the existing import structure.
135-137: LGTM! Method follows existing patterns.The spawnPlayers() method follows the same pattern as the existing spawnBots() method, maintaining consistency in the codebase. The delegation to PlayerSpawner keeps concerns separated cleanly.
src/client/HostLobbyModal.ts (4)
51-51: LGTM! State properly initialized.The randomSpawn state is correctly declared with a sensible default of false, consistent with other boolean options in the modal.
394-408: LGTM! UI option follows existing patterns.The random spawn checkbox follows the same structure and styling as other options in the modal. The translation key is properly used, and the selected class binding is correct.
688-691: LGTM! Event handler is consistent.The handleRandomSpawnChange handler follows the same pattern as other checkbox handlers in the component, properly updating state and calling putGameConfig.
774-774: LGTM! Config propagation is correct.The randomSpawn value is properly included in the game configuration payload sent to the server, consistent with other config options.
src/client/SinglePlayerModal.ts (4)
47-47: LGTM! State properly initialized.The randomSpawn state is correctly declared with a default of false, consistent with the HostLobbyModal implementation.
297-311: LGTM! UI option consistent with host modal.The random spawn checkbox follows the same structure as in HostLobbyModal and is consistent with other options in this modal. Good consistency across the codebase.
460-462: LGTM! Event handler follows pattern.The handleRandomSpawnChange handler is consistent with other checkbox handlers in the component.
587-587: LGTM! Config value properly propagated.The randomSpawn value is correctly included in the game configuration when starting a single-player game.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/core/execution/PlayerSpawner.ts (2)
29-29: Consider making magic numbers configurable.The hardcoded values (100 max tries, 50 minimum distance) work for typical scenarios but could be extracted as named constants or configuration options for better maintainability and testability.
Example refactor:
+ private static readonly MAX_SPAWN_TRIES = 100; + private static readonly MIN_SPAWN_DISTANCE = 50; + randomSpawnLand(): TileRef | null { let tries = 0; - while (tries < 100) { + while (tries < PlayerSpawner.MAX_SPAWN_TRIES) { tries++; const tile = this.randTile(); if ( !this.gm.isLand(tile) || this.gm.hasOwner(tile) || this.gm.isBorder(tile) ) { continue; } let tooCloseToOtherPlayer = false; for (const spawn of this.players) { - if (this.gm.manhattanDist(spawn.tile, tile) < 50) { + if (this.gm.manhattanDist(spawn.tile, tile) < PlayerSpawner.MIN_SPAWN_DISTANCE) { tooCloseToOtherPlayer = true; break; } }Also applies to: 44-44
68-73: Consider escalating spawn failures beyond console warnings.The TODO comment at line 69-70 correctly identifies that spawn failures need better handling. If a human player cannot spawn, silently skipping them could lead to a broken game state. Consider throwing an error, displaying a user-facing message, or implementing fallback logic (e.g., expanding search radius or relaxing constraints).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/core/execution/PlayerSpawner.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 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/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/PlayerSpawner.ts
🧬 Code graph analysis (1)
src/core/execution/PlayerSpawner.ts (6)
src/core/PseudoRandom.ts (1)
PseudoRandom(3-76)src/core/execution/SpawnExecution.ts (1)
SpawnExecution(7-61)src/core/game/Game.ts (1)
Game(664-748)src/core/Schemas.ts (1)
GameID(25-25)src/core/Util.ts (1)
simpleHash(64-72)src/core/game/GameMap.ts (1)
TileRef(3-3)
🔇 Additional comments (4)
src/core/execution/PlayerSpawner.ts (4)
1-17: LGTM!The imports are clean and the constructor properly seeds the RNG using the game ID. This ensures deterministic spawn positions for the same game.
19-24: LGTM!The random tile generation is straightforward and correctly uses the map dimensions.
42-52: Previous critical bug has been fixed.The spacing check now correctly uses a flag pattern to ensure that if any existing spawn is too close, the outer loop continues to the next candidate tile. This fixes the critical logic bug identified in the previous review.
60-79: No action required — review comment based on incorrect assumption.The concern about multiple calls accumulating duplicates is already prevented by design. Each time
execManager.spawnPlayers()is called, a freshPlayerSpawnerinstance is created (ExecutionManager.ts:136), and the method is called only once during game initialization (GameRunner.ts:104). A new instance means a fresh emptyplayersarray, so accumulation cannot occur. The defensive pattern suggested in the review comment is unnecessary.Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
src/core/execution/PlayerSpawner.ts (2)
42-52: Past critical issue has been resolved.The logic bug flagged in the previous review (where
continueonly affected the inner loop) has been correctly fixed using atooCloseToOtherPlayerflag with abreakstatement.
69-69: Past typo has been corrected.The typo "nornally" flagged in the previous review has been fixed to "normally".
🧹 Nitpick comments (2)
src/core/execution/PlayerSpawner.ts (2)
26-58: Consider making the distance threshold configurable.The hardcoded Manhattan distance of 50 may not scale well across different map sizes. On small maps this might be too restrictive, while on large maps it might be too permissive.
Consider deriving the distance from map dimensions (e.g.,
Math.min(this.gm.width(), this.gm.height()) / 4) or exposing it as a config parameter.
60-79: Consider validating that human players exist.The method silently returns an empty array if there are no human players. While this may be intentional, adding an early check or debug log could help surface configuration issues during development.
Example:
spawnPlayers(): SpawnExecution[] { + const humanPlayers = this.gm.allPlayers().filter(p => p.type() === PlayerType.Human); + if (humanPlayers.length === 0) { + console.debug('PlayerSpawner: no human players to spawn'); + return []; + } + - for (const player of this.gm.allPlayers()) { - if (player.type() !== PlayerType.Human) { - continue; - } + for (const player of humanPlayers) {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/core/execution/PlayerSpawner.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 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/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/PlayerSpawner.ts
🧬 Code graph analysis (1)
src/core/execution/PlayerSpawner.ts (6)
src/core/PseudoRandom.ts (1)
PseudoRandom(3-76)src/core/execution/SpawnExecution.ts (1)
SpawnExecution(7-61)src/core/game/Game.ts (1)
Game(664-748)src/core/Schemas.ts (1)
GameID(25-25)src/core/Util.ts (1)
simpleHash(64-72)src/core/game/GameMap.ts (1)
TileRef(3-3)
🔇 Additional comments (3)
src/core/execution/PlayerSpawner.ts (3)
1-7: Imports look good.All necessary types and utilities are imported cleanly.
19-24: Clean and correct random tile selection.The method correctly generates random coordinates within map bounds.
8-17: The review comment is incorrect.The
spawnPlayers()method creates a newPlayerSpawnerinstance each time it's called (as seen inExecutionManager.spawnPlayers()at line 136). Therefore, theplayersarray is always freshly initialized and does not accumulate across calls. The concern about array state persisting between invocations does not apply to the actual codebase usage pattern.Likely an incorrect or invalid review comment.
evanpelle
left a comment
evanpelle
left a comment
There was a problem hiding this comment.
I have trouble finding my spawn. I think we should emit a GoToPlayerEvent at the start so the camera goes to your player
| randomSpawnLand(): TileRef | null { | ||
| let tries = 0; | ||
|
|
||
| while (tries < 100) { |
There was a problem hiding this comment.
This seems like a bit of a naiive solution, as it could easily happen (especially towards the later players) that it won't be able to find a spot just by randomly guessing.
How expensive would it be if we built a low-res grid of possible spawn locations, and then iterated over a random index of those? Such a grid would have 800 elements at most in the world map, but realistically around 600.
i.e. when the map loads, check every 50 tiles verically and horizontally if this is a valid spot, if it is, add it as a tileRef to an array, and then instead of randTile choosing a totally random tile, it will choose possibleSpaces[random.nextInt(0, possibleSpaces.length)]
I don't have the final word on this, just my 5 cents.
There was a problem hiding this comment.
Yes, you’re right about this approach. However, when I checked how this feature could be implemented based on the existing logic in BotSpawner and FakeHumanExecution, I noticed that the same approach is used in both places. To make it more efficient, we would definitely need to rewrite the logic, but that seems to require a major refactor across multiple components and should probably be addressed separately from the current issue.
There was a problem hiding this comment.
I also thought about a possible issue: currently, players and bots don’t take the minimum distance between each other into account. Bots only try to spawn farther from other bots, and players from other players, but not players from bots or bots from players.
I’m not sure how inconvenient this behavior might be, but it could be worth improving in future enhancements of this feature if users start reporting issues related to it.
There was a problem hiding this comment.
Alright, maybe that can be another PR, hopefully 100 tries is enough. I don't think bot proximity should be a big deal, as long as the player has enough space to actually spawn, which should be guaranteed by the tile not being owned.
There was a problem hiding this comment.
checking random land was the fastest way i could come up with, especially on larger maps, tracking available land can be really slow.
There was a problem hiding this comment.
Eventually we may need smarter spawn selection, for example if there are just 3 players, and 2 players spawn right next to each other they are at a disadvantage.
There was a problem hiding this comment.
Eventually we may need smarter spawn selection, for example if there are just 3 players, and 2 players spawn right next to each other they are at a disadvantage.
Yes, we could adjust MIN_SPAWN_DISTANCE depending on the number of players in the game, for example. But I’m not sure, maybe some “unfair” spawns aren’t actually a bug, but rather a feature that adds extra tension to the gameplay. Will see with some feedback :)
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/core/execution/utils/PlayerSpawner.ts (1)
26-58: Naive random selection can fail to find spawns.As noted in previous review, the pure random approach with 100 tries can easily fail, especially for later players or maps with limited land. The probability of success decreases as more spawns are placed, and there's no guarantee of finding a spawn even when valid locations exist.
Consider pre-computing an array of valid spawn tiles at initialization, then sampling from that array without replacement. This would be more reliable and efficient.
🧹 Nitpick comments (1)
src/core/execution/utils/PlayerSpawner.ts (1)
44-44: Hardcoded minimum distance doesn't scale with map size.The fixed Manhattan distance of 50 tiles doesn't account for map dimensions. On smaller maps, this could make it impossible to spawn multiple players, while on larger maps it might not provide enough separation.
Consider making this distance relative to map size (e.g.,
Math.min(50, this.gm.width() / 10)) or loading it from game config.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/client/ClientGameRunner.ts(3 hunks)src/core/execution/ExecutionManager.ts(2 hunks)src/core/execution/utils/PlayerSpawner.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (8)
📚 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/utils/PlayerSpawner.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/execution/ExecutionManager.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/ExecutionManager.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/ExecutionManager.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/ExecutionManager.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/client/ClientGameRunner.ts
📚 Learning: 2025-06-09T02:20:43.637Z
Learnt from: VariableVince
Repo: openfrontio/OpenFrontIO PR: 1110
File: src/client/Main.ts:293-295
Timestamp: 2025-06-09T02:20:43.637Z
Learning: In src/client/Main.ts, during game start in the handleJoinLobby callback, UI elements are hidden using direct DOM manipulation with classList.add("hidden") for consistency. This includes modals, buttons, and error divs. The codebase follows this pattern rather than using component APIs for hiding elements during game transitions.
Applied to files:
src/client/ClientGameRunner.ts
📚 Learning: 2025-10-18T11:00:57.142Z
Learnt from: NewYearNewPhil
Repo: openfrontio/OpenFrontIO PR: 2230
File: src/client/graphics/GameRenderer.ts:269-277
Timestamp: 2025-10-18T11:00:57.142Z
Learning: In src/client/graphics/GameRenderer.ts, the GameRecapCapture implementation does not use setCaptureRenderEnabled on layers. Instead, it uses RecapCaptureSurface.capture() to render capture layers (TerrainLayer, TerritoryLayer, RailroadLayer, StructureIconsLayer, UnitLayer) directly to an off-screen canvas without requiring layer-level capture mode methods.
Applied to files:
src/client/ClientGameRunner.ts
🧬 Code graph analysis (3)
src/core/execution/ExecutionManager.ts (2)
src/core/game/Game.ts (1)
Execution(351-356)src/core/execution/utils/PlayerSpawner.ts (1)
PlayerSpawner(8-80)
src/core/execution/utils/PlayerSpawner.ts (6)
src/core/PseudoRandom.ts (1)
PseudoRandom(3-76)src/core/execution/SpawnExecution.ts (1)
SpawnExecution(7-61)src/core/game/Game.ts (1)
Game(664-748)src/core/Schemas.ts (1)
GameID(25-25)src/core/Util.ts (1)
simpleHash(64-72)src/core/game/GameMap.ts (1)
TileRef(3-3)
src/client/ClientGameRunner.ts (2)
src/core/game/GameView.ts (1)
myPlayer(592-594)src/client/graphics/layers/Leaderboard.ts (1)
GoToPlayerEvent(21-23)
🔇 Additional comments (7)
src/client/ClientGameRunner.ts (3)
50-50: LGTM!The import is correctly placed and necessary for the auto-navigation feature.
319-329: Consider timeout cleanup and timing.The 1-second delay and untracked
setTimeouthave a few concerns:
- If
stop()is called during the delay, the callback will still fire, potentially emitting events after cleanup.- The timing seems arbitrary—if player spawning takes longer than 1 second,
myPlayermight still be null.Consider storing the timeout ID and clearing it in
stop(), and verify that 1 second is sufficient for spawn processing to complete.
418-420: LGTM!Correctly prevents manual spawn selection when random spawn mode is enabled. The logic is sound and well-integrated with existing spawn checks.
src/core/execution/ExecutionManager.ts (2)
29-29: LGTM!Import is correctly placed and follows the existing pattern for BotSpawner.
135-137: LGTM!Clean implementation that mirrors the existing
spawnBots()pattern. The delegation toPlayerSpawnerkeeps the responsibility separation clear.src/core/execution/utils/PlayerSpawner.ts (2)
1-17: LGTM!Clean structure with deterministic randomness seeded from the game ID. This ensures consistent spawn locations across all clients for the same game.
19-24: LGTM!Simple helper that correctly generates random tile coordinates within map bounds.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/core/execution/utils/PlayerSpawner.ts (1)
26-58: Consider making this method private.The method relies on
this.playersfor distance checking, meaning it only works correctly when called duringspawnPlayers(). Making it public exposes internal state dependency and could lead to incorrect behavior if called independently (distance checks would be ineffective with an emptythis.playersarray).Unless there's a specific need to call this from outside the class, making it private would prevent misuse and clarify the intended usage pattern.
Apply this diff to make the method private:
- randomSpawnLand(): TileRef | null { + private randomSpawnLand(): TileRef | null {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
resources/lang/debug.json(3 hunks)resources/lang/en.json(3 hunks)src/client/ClientGameRunner.ts(3 hunks)src/client/graphics/layers/HeadsUpMessage.ts(1 hunks)src/core/execution/utils/PlayerSpawner.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/client/ClientGameRunner.ts
🧰 Additional context used
🧠 Learnings (5)
📚 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/utils/PlayerSpawner.ts
📚 Learning: 2025-06-02T14:27:37.609Z
Learnt from: andrewNiziolek
Repo: openfrontio/OpenFrontIO PR: 1007
File: resources/lang/de.json:115-115
Timestamp: 2025-06-02T14:27:37.609Z
Learning: For OpenFrontIO project: When localization keys are renamed in language JSON files, the maintainers separate technical changes from translation content updates. They wait for community translators to update the actual translation values rather than attempting to translate in the same PR. This allows technical changes to proceed while ensuring accurate translations from native speakers.
Applied to files:
resources/lang/debug.jsonresources/lang/en.json
📚 Learning: 2025-08-16T10:52:08.292Z
Learnt from: TheGiraffe3
Repo: openfrontio/OpenFrontIO PR: 884
File: resources/lang/en.json:456-461
Timestamp: 2025-08-16T10:52:08.292Z
Learning: In OpenFrontIO, translation files in resources/lang/*.json (except en.json) should not be updated in regular PRs. Only dedicated translation PRs titled "mls" and made by Aotumori should update non-English locale files. Regular PRs should only update en.json when adding or modifying translation keys.
Applied to files:
resources/lang/en.json
📚 Learning: 2025-07-12T08:41:35.101Z
Learnt from: Aotumuri
Repo: openfrontio/OpenFrontIO PR: 1357
File: resources/lang/de.json:523-540
Timestamp: 2025-07-12T08:41:35.101Z
Learning: In OpenFrontIO project localization files, always check the en.json source file before flagging potential spelling errors in other language files, as some keys may intentionally use non-standard spellings that need to be consistent across all translations.
Applied to files:
resources/lang/en.json
📚 Learning: 2025-05-30T03:53:52.231Z
Learnt from: scottanderson
Repo: openfrontio/OpenFrontIO PR: 949
File: resources/lang/en.json:8-10
Timestamp: 2025-05-30T03:53:52.231Z
Learning: For the OpenFrontIO project, do not suggest updating translation files in resources/lang/*.json except for en.json. The project has a dedicated translation team that handles all other locale files.
Applied to files:
resources/lang/en.json
🧬 Code graph analysis (1)
src/core/execution/utils/PlayerSpawner.ts (6)
src/core/PseudoRandom.ts (1)
PseudoRandom(3-76)src/core/execution/SpawnExecution.ts (1)
SpawnExecution(7-61)src/core/game/Game.ts (1)
Game(664-748)src/core/Schemas.ts (1)
GameID(25-25)src/core/Util.ts (1)
simpleHash(64-72)src/core/game/GameMap.ts (1)
TileRef(3-3)
🔇 Additional comments (8)
src/client/graphics/layers/HeadsUpMessage.ts (1)
43-45: Clean conditional rendering.The ternary operator clearly handles the two spawn modes. The logic is simple and the translation keys match the localization files.
resources/lang/debug.json (1)
97-97: Debug keys added correctly.All new localization keys follow the debug file pattern where values mirror their key paths. The structure is consistent with existing entries.
Also applies to: 150-150, 171-172
resources/lang/en.json (2)
138-138: Clear UI labels.The "Random spawn" text is simple and descriptive for the toggle controls in both single-player and host lobby contexts.
Also applies to: 266-266
660-663: Informative user messages.The spawn failure error clearly communicates the issue, and the random spawn message explains what's happening during auto-selection. Both messages are direct and functional.
Also applies to: 667-668
src/core/execution/utils/PlayerSpawner.ts (4)
1-6: LGTM!Imports are clean and minimal, with all dependencies properly typed.
8-17: LGTM!Clean class structure with proper encapsulation. Seeding the random number generator with the gameID hash ensures deterministic spawn locations across all clients for the same game—critical for multiplayer consistency.
19-24: LGTM!Simple and correct tile selection logic with appropriate bounds.
60-78: Logic is correct for successful spawns.The incremental building of the
playersarray ensures that distance checking works properly—each new player is validated against all previously spawned players. The filtering forPlayerType.Humanis appropriate.The spawn failure handling at lines 68-72 has already been discussed in previous reviews and acknowledged with a TODO comment.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
src/core/execution/utils/PlayerSpawner.ts (1)
26-58: Consider extracting magic numbers to named constants.The values
100(max tries) and30(minimum distance) are hardcoded. Extracting them to named constants at the class level would improve readability and make future tuning easier.Apply this diff to extract the constants:
export class PlayerSpawner { private random: PseudoRandom; private players: SpawnExecution[] = []; + private static readonly MAX_SPAWN_TRIES = 100; + private static readonly MIN_SPAWN_DISTANCE = 30; constructor( private gm: Game, gameID: GameID, ) { this.random = new PseudoRandom(simpleHash(gameID)); } private randTile(): TileRef { const x = this.random.nextInt(0, this.gm.width()); const y = this.random.nextInt(0, this.gm.height()); return this.gm.ref(x, y); } randomSpawnLand(): TileRef | null { let tries = 0; - while (tries < 100) { + while (tries < PlayerSpawner.MAX_SPAWN_TRIES) { tries++; const tile = this.randTile(); if ( !this.gm.isLand(tile) || this.gm.hasOwner(tile) || this.gm.isBorder(tile) ) { continue; } let tooCloseToOtherPlayer = false; for (const spawn of this.players) { - if (this.gm.manhattanDist(spawn.tile, tile) < 30) { + if (this.gm.manhattanDist(spawn.tile, tile) < PlayerSpawner.MIN_SPAWN_DISTANCE) { tooCloseToOtherPlayer = true; break; } } if (tooCloseToOtherPlayer) { continue; } return tile; } return null; }tests/core/execution/utils/PlayerSpawner.test.ts (2)
5-35: Test structure is good, but validation could be deeper.The parameterized test approach with
test.eachis excellent for covering multiple maps. However, the test only verifies the count of executors, not whether the spawn locations meet the constraints.Consider adding validation that each spawn location:
- Is on land (
gm.isLand(spawn.tile))- Is unowned (
!gm.hasOwner(spawn.tile))- Is not a border tile (
!gm.isBorder(spawn.tile))- Is at least 30 tiles away from other spawns
Here's an example of more thorough validation:
const executors = new PlayerSpawner(game, "game_id").spawnPlayers(); expect(executors.length).toBe(maxPlayers); // Validate each spawn location meets constraints for (const executor of executors) { expect(game.isLand(executor.tile)).toBe(true); expect(game.hasOwner(executor.tile)).toBe(false); expect(game.isBorder(executor.tile)).toBe(false); } // Validate minimum distance between spawns for (let i = 0; i < executors.length; i++) { for (let j = i + 1; j < executors.length; j++) { const distance = game.manhattanDist(executors[i].tile, executors[j].tile); expect(distance).toBeGreaterThanOrEqual(30); } }
8-34: Add test cases for edge scenarios.The current tests only cover the happy path where all players successfully spawn. Consider adding test cases for:
- Spawn failure scenario: Test a map with limited land area and too many players to verify that
spawnPlayers()handles partial failures gracefully.- Single player: Verify spawn works correctly with just one player.
- Crowded map: Test behavior when spawn locations are scarce.
Example test for spawn failure:
test("Handles spawn failure when map is too crowded", async () => { const players: PlayerInfo[] = []; // Try to spawn more players than possible on a small map for (let i = 0; i < 50; i++) { players.push( new PlayerInfo( `player${i}`, PlayerType.Human, `client_id${i}`, `player_id${i}`, ), ); } const game = await setup("half_land_half_ocean", undefined, players); const executors = new PlayerSpawner(game, "game_id").spawnPlayers(); // Should spawn fewer than requested when map is too small expect(executors.length).toBeLessThan(players.length); expect(executors.length).toBeGreaterThan(0); });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/core/execution/utils/PlayerSpawner.ts(1 hunks)tests/core/execution/utils/PlayerSpawner.test.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 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/utils/PlayerSpawner.test.tssrc/core/execution/utils/PlayerSpawner.ts
🧬 Code graph analysis (2)
tests/core/execution/utils/PlayerSpawner.test.ts (3)
src/core/game/Game.ts (1)
PlayerInfo(406-420)tests/util/Setup.ts (1)
setup(23-82)src/core/execution/utils/PlayerSpawner.ts (1)
PlayerSpawner(8-79)
src/core/execution/utils/PlayerSpawner.ts (6)
src/core/PseudoRandom.ts (1)
PseudoRandom(3-76)src/core/execution/SpawnExecution.ts (1)
SpawnExecution(7-61)src/core/game/Game.ts (1)
Game(664-748)src/core/Schemas.ts (1)
GameID(25-25)src/core/Util.ts (1)
simpleHash(64-72)src/core/game/GameMap.ts (1)
TileRef(3-3)
🔇 Additional comments (3)
src/core/execution/utils/PlayerSpawner.ts (3)
12-17: Good use of deterministic randomness.Seeding the PRNG with a hash of the game ID ensures that spawn locations are reproducible for the same game, which is helpful for debugging and fairness. The approach is clean and idiomatic.
19-24: LGTM!The
randTilemethod is straightforward and correct. It properly generates random coordinates within game bounds and converts them to a TileRef.
60-78: Implementation looks correct.The method properly filters for human players, attempts to find spawn locations, and handles failures with appropriate logging. The approach integrates well with the existing SpawnExecution pattern.
src/core/game/GameView.ts
Outdated
| isRandomSpawn(): boolean { | ||
| return this._config.randomSpawn(); | ||
| } |
There was a problem hiding this comment.
can you remove this (and on Game), and leave it in the config. so GameView.config().isRandomSpawn()
There was a problem hiding this comment.
Removed and refactored to use from config.
src/client/ClientGameRunner.ts
Outdated
| this.hasJoined = true; | ||
| console.log("starting game!"); | ||
|
|
||
| if (this.gameView.isRandomSpawn()) { |
There was a problem hiding this comment.
little worried about this if the machine is slow and it takes longer than 1 second to load the game.
There was a problem hiding this comment.
Refactored logic there to mitigate this issue.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/client/ClientGameRunner.ts(3 hunks)src/client/graphics/layers/HeadsUpMessage.ts(1 hunks)src/core/GameRunner.ts(1 hunks)src/core/configuration/Config.ts(1 hunks)src/core/configuration/DefaultConfig.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/core/GameRunner.ts
🧰 Additional context used
🧠 Learnings (5)
📚 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.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/client/ClientGameRunner.ts
📚 Learning: 2025-06-09T02:20:43.637Z
Learnt from: VariableVince
Repo: openfrontio/OpenFrontIO PR: 1110
File: src/client/Main.ts:293-295
Timestamp: 2025-06-09T02:20:43.637Z
Learning: In src/client/Main.ts, during game start in the handleJoinLobby callback, UI elements are hidden using direct DOM manipulation with classList.add("hidden") for consistency. This includes modals, buttons, and error divs. The codebase follows this pattern rather than using component APIs for hiding elements during game transitions.
Applied to files:
src/client/ClientGameRunner.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/client/ClientGameRunner.ts
📚 Learning: 2025-10-18T11:00:57.142Z
Learnt from: NewYearNewPhil
Repo: openfrontio/OpenFrontIO PR: 2230
File: src/client/graphics/GameRenderer.ts:269-277
Timestamp: 2025-10-18T11:00:57.142Z
Learning: In src/client/graphics/GameRenderer.ts, the GameRecapCapture implementation does not use setCaptureRenderEnabled on layers. Instead, it uses RecapCaptureSurface.capture() to render capture layers (TerrainLayer, TerritoryLayer, RailroadLayer, StructureIconsLayer, UnitLayer) directly to an off-screen canvas without requiring layer-level capture mode methods.
Applied to files:
src/client/ClientGameRunner.ts
🧬 Code graph analysis (1)
src/client/ClientGameRunner.ts (4)
src/core/game/GameView.ts (1)
myPlayer(592-594)src/client/Utils.ts (1)
translateText(92-147)src/client/LangSelector.ts (1)
translateText(254-274)src/client/graphics/layers/Leaderboard.ts (1)
GoToPlayerEvent(21-23)
🔇 Additional comments (5)
src/core/configuration/Config.ts (1)
91-91: LGTM! Clean interface addition.The new
isRandomSpawn()method is properly typed and follows the existing interface pattern.src/client/graphics/layers/HeadsUpMessage.ts (1)
43-45: Good conditional rendering logic.The ternary operator cleanly switches between spawn messages based on the game configuration.
src/client/ClientGameRunner.ts (2)
51-51: Clean import addition.Properly imports the event type needed for the random spawn flow.
453-454: Correct guard for random spawn mode.Properly prevents manual spawn intents when random spawn is enabled, ensuring players can't interfere with the automatic spawn process.
src/core/configuration/DefaultConfig.ts (1)
340-342: Clean implementation following existing patterns.The getter properly delegates to the game config property, matching the style of other configuration accessors in this class.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
resources/lang/en.json(3 hunks)src/client/ClientGameRunner.ts(5 hunks)src/core/configuration/Config.ts(1 hunks)src/core/configuration/DefaultConfig.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/core/configuration/Config.ts
- src/core/configuration/DefaultConfig.ts
🧰 Additional context used
🧠 Learnings (8)
📚 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/client/ClientGameRunner.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/client/ClientGameRunner.ts
📚 Learning: 2025-06-09T02:20:43.637Z
Learnt from: VariableVince
Repo: openfrontio/OpenFrontIO PR: 1110
File: src/client/Main.ts:293-295
Timestamp: 2025-06-09T02:20:43.637Z
Learning: In src/client/Main.ts, during game start in the handleJoinLobby callback, UI elements are hidden using direct DOM manipulation with classList.add("hidden") for consistency. This includes modals, buttons, and error divs. The codebase follows this pattern rather than using component APIs for hiding elements during game transitions.
Applied to files:
src/client/ClientGameRunner.ts
📚 Learning: 2025-10-18T11:00:57.142Z
Learnt from: NewYearNewPhil
Repo: openfrontio/OpenFrontIO PR: 2230
File: src/client/graphics/GameRenderer.ts:269-277
Timestamp: 2025-10-18T11:00:57.142Z
Learning: In src/client/graphics/GameRenderer.ts, the GameRecapCapture implementation does not use setCaptureRenderEnabled on layers. Instead, it uses RecapCaptureSurface.capture() to render capture layers (TerrainLayer, TerritoryLayer, RailroadLayer, StructureIconsLayer, UnitLayer) directly to an off-screen canvas without requiring layer-level capture mode methods.
Applied to files:
src/client/ClientGameRunner.ts
📚 Learning: 2025-06-02T14:27:37.609Z
Learnt from: andrewNiziolek
Repo: openfrontio/OpenFrontIO PR: 1007
File: resources/lang/de.json:115-115
Timestamp: 2025-06-02T14:27:37.609Z
Learning: For OpenFrontIO project: When localization keys are renamed in language JSON files, the maintainers separate technical changes from translation content updates. They wait for community translators to update the actual translation values rather than attempting to translate in the same PR. This allows technical changes to proceed while ensuring accurate translations from native speakers.
Applied to files:
resources/lang/en.json
📚 Learning: 2025-08-16T10:52:08.292Z
Learnt from: TheGiraffe3
Repo: openfrontio/OpenFrontIO PR: 884
File: resources/lang/en.json:456-461
Timestamp: 2025-08-16T10:52:08.292Z
Learning: In OpenFrontIO, translation files in resources/lang/*.json (except en.json) should not be updated in regular PRs. Only dedicated translation PRs titled "mls" and made by Aotumori should update non-English locale files. Regular PRs should only update en.json when adding or modifying translation keys.
Applied to files:
resources/lang/en.json
📚 Learning: 2025-07-12T08:41:35.101Z
Learnt from: Aotumuri
Repo: openfrontio/OpenFrontIO PR: 1357
File: resources/lang/de.json:523-540
Timestamp: 2025-07-12T08:41:35.101Z
Learning: In OpenFrontIO project localization files, always check the en.json source file before flagging potential spelling errors in other language files, as some keys may intentionally use non-standard spellings that need to be consistent across all translations.
Applied to files:
resources/lang/en.json
📚 Learning: 2025-05-30T03:53:52.231Z
Learnt from: scottanderson
Repo: openfrontio/OpenFrontIO PR: 949
File: resources/lang/en.json:8-10
Timestamp: 2025-05-30T03:53:52.231Z
Learning: For the OpenFrontIO project, do not suggest updating translation files in resources/lang/*.json except for en.json. The project has a dedicated translation team that handles all other locale files.
Applied to files:
resources/lang/en.json
🧬 Code graph analysis (1)
src/client/ClientGameRunner.ts (3)
src/core/game/GameView.ts (1)
myPlayer(592-594)src/client/Utils.ts (1)
translateText(92-147)src/client/graphics/layers/Leaderboard.ts (1)
GoToPlayerEvent(21-23)
🔇 Additional comments (5)
resources/lang/en.json (1)
138-138: Localization additions look good.All new keys for the random spawn feature are well-structured, clearly worded, and properly placed within their respective sections. The JSON syntax is valid, and the user-facing text is appropriate for the game UI. The approach of updating only en.json aligns with the project's localization policy.
Also applies to: 266-266, 666-674
src/client/ClientGameRunner.ts (4)
51-51: LGTM!The import is necessary for navigating to the player's position after random spawn.
206-206: LGTM!The field is properly typed and initialized for timeout cleanup.
463-467: LGTM!The spawn intent guard correctly prevents manual spawn clicks when random spawn is enabled. The logic properly combines the spawn phase check with the random spawn mode check.
332-362: Localization keys verified—code is ready.The
error_modal.spawn_failed.titleanderror_modal.spawn_failed.descriptionkeys both exist inen.jsonwith correct structure. The random spawn logic handles all cases correctly: it polls during spawn phase, returns early if player is missing, shows error if spawn fails, and emits event when player spawns.
|
@evanpelle , should I enable random spawn for games in the playlist by default? For example, for FFA with some probability, say 25%? What do you think? |
I think we should keep it only for private games when we release so players can test it out, and then once we know it's working well we can add it to public games. |
4 participants
Description:
#2352
This is my first PR in this project, and I’ll continue refining it based on feedback.
Please complete the following:
Please put your Discord username so you can be contacted if a bug or regression is found:
nikolaj_mykola