Configurable ceasefire time #2460
Conversation
|
Warning Rate limit exceeded@ryanbarlow97 has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 28 minutes and 1 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (13)
WalkthroughThis pull request introduces a configurable spawn immunity duration feature to the game. It adds UI controls in host and single-player lobbies, a new visual ceasefire timer component, updates the game configuration schema, and implements spawn immunity enforcement across attack and transport execution logic to prevent unit actions during the spawn phase. Changes
Sequence DiagramsequenceDiagram
actor User
participant UI as Host/Single Player UI
participant Config as GameConfig
participant Execution as Execution Logic<br/>(Attack/Transport)
participant Render as CeasefireTimer
User->>UI: Input spawn immunity duration
UI->>Config: Update spawnImmunityDuration (in tenths)
rect rgb(200, 230, 255)
Note over Execution: During spawn phase
Execution->>Config: Check spawnImmunityDuration<br/>& numSpawnPhaseTurns
Execution->>Execution: Calculate immunity window<br/>(spawn phase turns + immunity)
Execution->>Execution: Compare vs current tick
end
alt Immunity active
Execution->>Execution: Block attack / transport
else Immunity expired
Execution->>Execution: Allow action
end
rect rgb(200, 230, 255)
Note over Render: Render phase
Render->>Config: Read immunity config & ticks
Render->>Render: Calculate progress ratio
Render->>Render: Determine visibility
end
Render->>User: Display progress bar (if active)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks✅ Passed checks (3 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
src/client/graphics/layers/CeasefireTimer.ts (2)
9-9: Use definite assignment assertion for the game property.The
gameproperty is assigned byGameRendererafter construction but TypeScript doesn't know this. Use the definite assignment assertion operator to clarify the initialization pattern:- public game: GameView; + public game!: GameView;
44-46: Extract minimum duration threshold to a named constant.The check
ceasefireDuration <= 5 * 10uses a magic number. Extract this to a named constant for clarity:+const MIN_CEASEFIRE_DURATION_TO_SHOW = 5 * 10; // 5 seconds in ticks + @customElement("ceasefire-timer") export class CeasefireTimer extends LitElement implements Layer { // ... tick() { // ... - if (ceasefireDuration <= 5 * 10 || this.game.inSpawnPhase()) { + if (ceasefireDuration <= MIN_CEASEFIRE_DURATION_TO_SHOW || this.game.inSpawnPhase()) { this.setInactive(); return; }src/client/SinglePlayerModal.ts (1)
588-588: Consider adding UI control for spawn immunity duration.The spawn immunity duration is hardcoded to
5 * 10(5 seconds) for single player games. Since the PR description mentions "Integration into HostLobbyModal and SingleplayerModal will be changed later," this is likely intentional for now.However, for consistency with other configurable options in this modal (bots, max timer, etc.), consider adding a UI input for this setting in a follow-up change.
src/core/game/TransportShipUtils.ts (1)
11-19: Consider centralizing spawn immunity logic.The spawn immunity check is now spread across multiple files:
AttackExecution.ts(lines 93-105)TransportShipExecution.ts(lines 92-103)TransportShipUtils.ts(lines 11-19, this file)This creates maintenance risk—if the spawn immunity rules change, you'll need to update multiple locations.
Consider extracting spawn immunity logic into a centralized helper:
// src/core/game/SpawnImmunityUtils.ts export function isProtectedBySpawnImmunity( game: Game, target: Player | TerraNullius ): boolean { if (!target.isPlayer()) return false; const targetPlayer = target as Player; if (targetPlayer.type() !== PlayerType.Human) return false; return ( game.config().numSpawnPhaseTurns() + game.config().spawnImmunityDuration() > game.ticks() ); }Then use it consistently:
if (isProtectedBySpawnImmunity(game, targetOwner)) { return false; }This ensures the rules stay consistent and are easier to test and maintain.
src/client/HostLobbyModal.ts (1)
528-546: Add blur handler and consider debouncing.The spawn immunity input implementation is solid, but consider two improvements:
Missing blur handler: The max timer input (lines 519-521) has both
@inputand@blurhandlers, but the spawn immunity input only has@input. Add a blur handler for consistency and to ensure validation happens when the user tabs out.Inconsistent debouncing: The bots input (lines 685-704) uses debouncing to avoid excessive API calls. The spawn immunity input triggers
putGameConfig()on every input event, which could be inefficient if the user is typing multiple digits.Apply this diff to add blur handling and optional debouncing:
<label for="spawn-immunity-duration" class="option-card" > <input type="number" id="spawn-immunity-duration" min="0" max="300" step="1" .value=${String(this.spawnImmunityDurationSeconds)} style="width: 60px; color: black; text-align: right; border-radius: 8px;" @input=${this.handleSpawnImmunityDurationInput} + @blur=${this.handleSpawnImmunityDurationBlur} @keydown=${this.handleSpawnImmunityDurationKeyDown} /> <div class="option-card-title"> <span>${translateText("host_modal.spawn_immunity_duration")}</span> </div> </label>And add the blur handler:
private handleSpawnImmunityDurationBlur(e: Event) { const input = e.target as HTMLInputElement; const value = parseInt(input.value, 10); if (Number.isNaN(value) || value < 0) { input.value = "0"; this.spawnImmunityDurationSeconds = 0; } else if (value > 300) { input.value = "300"; this.spawnImmunityDurationSeconds = 300; } this.putGameConfig(); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (18)
resources/lang/debug.json(1 hunks)resources/lang/en.json(1 hunks)src/client/HostLobbyModal.ts(5 hunks)src/client/SinglePlayerModal.ts(1 hunks)src/client/graphics/GameRenderer.ts(3 hunks)src/client/graphics/layers/CeasefireTimer.ts(1 hunks)src/client/index.html(1 hunks)src/core/Schemas.ts(1 hunks)src/core/configuration/DefaultConfig.ts(1 hunks)src/core/execution/AttackExecution.ts(1 hunks)src/core/execution/TransportShipExecution.ts(2 hunks)src/core/execution/WarshipExecution.ts(2 hunks)src/core/game/PlayerImpl.ts(3 hunks)src/core/game/TransportShipUtils.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 (19)
📚 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/server/GameManager.tssrc/core/execution/AttackExecution.tssrc/core/execution/TransportShipExecution.tssrc/core/execution/WarshipExecution.tssrc/client/HostLobbyModal.tssrc/core/Schemas.tssrc/core/configuration/DefaultConfig.tstests/util/Setup.tssrc/core/game/PlayerImpl.ts
📚 Learning: 2025-08-12T00:31:50.144Z
Learnt from: scottanderson
Repo: openfrontio/OpenFrontIO PR: 1752
File: src/core/game/Game.ts:750-752
Timestamp: 2025-08-12T00:31:50.144Z
Learning: In the OpenFrontIO codebase, changes to the PlayerInteraction interface (like adding canDonateGold and canDonateTroops flags) do not require corresponding updates to src/core/Schemas.ts or server serialization code.
Applied to files:
src/server/GameServer.tssrc/core/game/TransportShipUtils.tssrc/core/Schemas.tssrc/client/SinglePlayerModal.tssrc/core/game/PlayerImpl.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/AttackExecution.tssrc/core/execution/TransportShipExecution.tssrc/core/execution/WarshipExecution.tssrc/core/game/PlayerImpl.ts
📚 Learning: 2025-10-20T11:02:16.969Z
Learnt from: sambokai
Repo: openfrontio/OpenFrontIO PR: 2225
File: src/core/execution/FakeHumanExecution.ts:57-57
Timestamp: 2025-10-20T11:02:16.969Z
Learning: In src/core/execution/FakeHumanExecution.ts, the correct MIRV victory denial thresholds are VICTORY_DENIAL_TEAM_THRESHOLD = 0.8 (80% for team games) and VICTORY_DENIAL_INDIVIDUAL_THRESHOLD = 0.65 (65% for individual players), not 0.85 and 0.7 as might be mentioned in some documentation.
Applied to files:
src/core/execution/AttackExecution.tssrc/core/execution/TransportShipExecution.tssrc/core/game/PlayerImpl.ts
📚 Learning: 2025-10-27T09:47:26.395Z
Learnt from: sambokai
Repo: openfrontio/OpenFrontIO PR: 2225
File: src/core/execution/FakeHumanExecution.ts:770-795
Timestamp: 2025-10-27T09:47:26.395Z
Learning: In src/core/execution/FakeHumanExecution.ts, the selectSteamrollStopTarget() method intentionally allows MIRV targeting when secondHighest city count is 0 (e.g., nuclear endgame scenarios where structures are destroyed). This is valid game design—"if you can afford it, you're good to go"—and should not be flagged as requiring a guard condition.
Applied to files:
src/core/execution/AttackExecution.tssrc/core/game/PlayerImpl.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/AttackExecution.ts
📚 Learning: 2025-08-24T11:13:08.495Z
Learnt from: DevelopingTom
Repo: openfrontio/OpenFrontIO PR: 1900
File: src/core/execution/SAMLauncherExecution.ts:103-111
Timestamp: 2025-08-24T11:13:08.495Z
Learning: In SAMLauncherExecution.ts, the cached target bug can only occur if: 1) SAM is not on cooldown when nuke is in range, 2) SAM goes on cooldown right after computing trajectory, 3) SAM becomes available again before nuke explodes. This is not possible with current cooldown values but the fix is still valuable for robustness.
Applied to files:
src/core/execution/AttackExecution.tssrc/core/game/PlayerImpl.ts
📚 Learning: 2025-11-01T00:24:33.860Z
Learnt from: FloPinguin
Repo: openfrontio/OpenFrontIO PR: 2161
File: src/core/execution/FakeHumanExecution.ts:670-678
Timestamp: 2025-11-01T00:24:33.860Z
Learning: In OpenFrontIO, PlayerType.Bot entities cannot be in teams and do not have friendliness relationships. Only PlayerType.Human and PlayerType.FakeHuman can participate in teams and alliances. Therefore, when targeting bot-owned tiles, friendliness checks like `owner.isFriendly(this.player)` are unnecessary and meaningless for bots.
Applied to files:
src/core/execution/AttackExecution.tssrc/core/game/PlayerImpl.ts
📚 Learning: 2025-05-21T04:10:33.435Z
Learnt from: scottanderson
Repo: openfrontio/OpenFrontIO PR: 784
File: src/core/game/StatsImpl.ts:34-38
Timestamp: 2025-05-21T04:10:33.435Z
Learning: In the codebase, PlayerStats is defined as a type inferred from a Zod schema that is marked as optional, which means PlayerStats already includes undefined as a possible type (PlayerStats | undefined).
Applied to files:
src/core/execution/TransportShipExecution.tssrc/core/Schemas.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/core/execution/TransportShipExecution.tssrc/core/game/PlayerImpl.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.json
📚 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/graphics/GameRenderer.tssrc/client/graphics/layers/CeasefireTimer.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/index.htmlsrc/client/HostLobbyModal.tssrc/client/SinglePlayerModal.tssrc/client/graphics/layers/CeasefireTimer.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.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/client/HostLobbyModal.ts
📚 Learning: 2025-05-21T04:10:33.435Z
Learnt from: scottanderson
Repo: openfrontio/OpenFrontIO PR: 784
File: src/core/game/StatsImpl.ts:34-38
Timestamp: 2025-05-21T04:10:33.435Z
Learning: In the codebase, PlayerStats is defined as `z.infer<typeof PlayerStatsSchema>` where PlayerStatsSchema has `.optional()` applied at the object level, making PlayerStats a union type that already includes undefined (PlayerStats | undefined).
Applied to files:
src/core/Schemas.ts
📚 Learning: 2025-08-23T07:48:19.060Z
Learnt from: ElMelchizedek
Repo: openfrontio/OpenFrontIO PR: 1876
File: src/core/execution/FakeHumanExecution.ts:470-473
Timestamp: 2025-08-23T07:48:19.060Z
Learning: In FakeHumanExecution.ts DefensePost placement logic, returning -Infinity from structureSpawnTileValue when no sampled border tiles neighbor enemies is intentional. The logic samples up to 50 border tiles as a heuristic - if none are adjacent to enemies, it assumes DefensePost placement is unnecessary and aborts the entire placement attempt rather than continuing to evaluate individual tiles.
Applied to files:
src/core/game/PlayerImpl.ts
📚 Learning: 2025-08-29T16:16:11.309Z
Learnt from: BrewedCoffee
Repo: openfrontio/OpenFrontIO PR: 1957
File: src/core/execution/PlayerExecution.ts:40-52
Timestamp: 2025-08-29T16:16:11.309Z
Learning: In OpenFrontIO PlayerExecution.ts, when Defense Posts are captured due to tile ownership changes, the intended behavior is to first call u.decreaseLevel() to downgrade them, then still transfer them to the capturing player via captureUnit(). This is not a bug - Defense Posts should be both downgraded and captured, not destroyed outright.
Applied to files:
src/core/game/PlayerImpl.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/game/PlayerImpl.ts
🧬 Code graph analysis (7)
src/server/GameServer.ts (2)
src/core/configuration/DefaultConfig.ts (1)
gameConfig(266-268)src/server/MapPlaylist.ts (1)
gameConfig(79-106)
src/core/execution/AttackExecution.ts (2)
src/core/Schemas.ts (1)
Player(117-117)src/core/game/Game.ts (1)
Player(526-662)
src/core/execution/TransportShipExecution.ts (1)
src/core/game/Game.ts (1)
Player(526-662)
src/core/game/TransportShipUtils.ts (2)
src/core/game/Game.ts (2)
Game(664-748)Player(526-662)src/core/game/GameMap.ts (1)
TileRef(3-3)
src/client/HostLobbyModal.ts (1)
src/client/Utils.ts (1)
translateText(92-147)
src/core/game/PlayerImpl.ts (2)
src/core/game/GameImpl.ts (1)
owner(183-185)src/core/game/GameView.ts (3)
owner(109-111)owner(636-638)tile(106-108)
src/client/graphics/layers/CeasefireTimer.ts (2)
src/client/graphics/layers/Layer.ts (1)
Layer(1-7)src/core/game/GameView.ts (1)
GameView(448-774)
🔇 Additional comments (21)
resources/lang/en.json (1)
252-252: LGTM!The localization key is clear and accurately describes the feature. The "(seconds)" suffix helps users understand the unit of measurement.
resources/lang/debug.json (1)
148-148: LGTM!The debug localization entry correctly follows the pattern of returning the key path as the translation value.
src/client/graphics/layers/CeasefireTimer.ts (2)
30-64: Well-structured timer logic.The tick method correctly handles the ceasefire timing window:
- Hides during spawn phase (before ceasefire starts)
- Shows only when ceasefire duration is meaningful (> 5 seconds)
- Calculates progress ratio based on elapsed ticks
- Adjusts position to avoid overlapping with team ownership bar
77-92: Clean render implementation.The render method efficiently returns an empty template when inactive and renders a smooth progress bar with CSS transitions when active.
src/client/graphics/GameRenderer.ts (2)
236-242: LGTM! Consistent layer integration pattern.The ceasefire timer initialization follows the same pattern as other layers like
spawnTimer, with proper type checking and error logging.
272-272: LGTM! Appropriate layer positioning.The ceasefire timer is correctly positioned in the layers array after
spawnTimerand beforeleaderboard, ensuring proper render order for the UI elements.src/server/GameManager.ts (1)
62-62: LGTM! Consistent default configuration.The default spawn immunity duration of
5 * 10(5 seconds) matches the values inSinglePlayerModal.tsandMapPlaylist.ts, providing consistent defaults across game modes.src/server/GameServer.ts (1)
121-123: LGTM! Standard config update pattern.The spawn immunity duration update follows the established pattern for partial config updates, checking for
undefinedbefore applying the change.src/server/MapPlaylist.ts (1)
103-103: LGTM! Consistent public game configuration.The spawn immunity duration default for public games matches the values in
GameManager.tsandSinglePlayerModal.ts, ensuring consistent behavior across all game modes.src/client/index.html (1)
407-407: LGTM!The custom element follows the existing pattern of declarative custom elements in the HTML. Placement alongside
<spawn-timer>is logical for a timer-related UI component.tests/util/Setup.ts (1)
67-67: LGTM!Setting
spawnImmunityDuration: 0as the default for tests is appropriate. This ensures tests don't have unintended spawn immunity behavior unless explicitly configured.src/client/HostLobbyModal.ts (3)
45-45: LGTM!The default of 5 seconds matches the previously hardcoded value in
DefaultConfig.ts(50 ticks = 5 seconds), ensuring backward compatibility.
711-726: LGTM!The input validation is thorough:
- Keydown handler prevents typing invalid characters at input time
- Input handler strips invalid characters and validates the range (0-300)
- Conversion to ticks (*10) correctly aligns with the schema's "10 per second" comment
817-817: LGTM!The conversion from seconds to ticks (
* 10) correctly aligns with the schema documentation stating ticks are at "10 per second".src/core/configuration/DefaultConfig.ts (1)
262-264: LGTM!Making spawn immunity duration configurable via
_gameConfigfollows the pattern used for other config values in this class. The field is required in the schema (no.optional()marker), so it is always defined and safe to access.src/core/execution/AttackExecution.ts (1)
93-105: Code is correct—spawn immunity protection is intentionally Human-only.The existing pattern across the codebase confirms spawn immunity protection applies only to
PlayerType.Human. This is by design:
- AttackExecution (line 96): Human-only guard
- TransportShipExecution (line 99): Human-only guard
- PlayerImpl.nukeSpawn (line 995): No guard—blocks ALL players from nuking (global game rule, not protection)
FakeHuman does not receive spawn immunity protection because it represents AI, not a real player. Spawn immunity exists to give real players (Human) time to learn during the spawn phase. AI players—whether FakeHuman or Bot—do not need this learning protection.
src/core/execution/TransportShipExecution.ts (1)
92-103: No changes needed. Code follows established codebase pattern.The spawn immunity check in
TransportShipExecution.ts(lines 99) correctly protects onlyPlayerType.Human, exactly matching the pattern used throughout the codebase:
AttackExecution.ts(lines 96-99): checkstargetPlayer.type() === PlayerType.HumanTransportShipUtils.ts(line 16): checkstargetOwner.type() === PlayerType.HumanPlayerImpl.tscanAttack() (line 1190): checksowner.type() === PlayerType.HumanFakeHuman players are intentionally excluded from spawn immunity protection. This is consistent game design—the immunity shields human players while bots remain vulnerable during early game.
src/core/Schemas.ts (1)
175-175: No changes required. The spawnImmunityDuration field is already provided at all GameConfig construction sites.All verified GameConfig construction locations include
spawnImmunityDuration:
- tests/util/Setup.ts sets it to 0
- src/server/GameManager.ts defaults to 50 ticks (5 * 10)
- src/server/MapPlaylist.ts sets it to 50 ticks
- src/client/SinglePlayerModal.ts sets it to 50 ticks
- src/client/HostLobbyModal.ts dynamically converts from user input
The original review incorrectly identified a breaking change based on incomplete output. The schema field is not optional, but all code that creates GameConfig objects already provides it, so validation succeeds.
Likely an incorrect or invalid review comment.
src/core/execution/WarshipExecution.ts (1)
68-85: Ceasefire guard for warships looks correct.Thanks for clearing targets and skipping the trade-ship hunt while the ceasefire window lasts; this keeps warships passive without disturbing patrol flow.
src/core/game/PlayerImpl.ts (2)
994-1001: Nuke spawn guard covers the missile ban cleanly.The early return keeps silos idle during the ceasefire, so we stay in line with the no-missile rule without touching later launch logic.
1187-1212: Guarding human targets matches the ceasefire rule.Caching the owner and gating only Human targets blocks PvP until the timer expires while still letting folks push into neutral land.
| ${translateText("host_modal.max_timer")} | ||
| </div> | ||
| </label> | ||
There was a problem hiding this comment.
can this have a checkbox, like game timer?
also i think minutes is a better unit than seconds
There was a problem hiding this comment.
yes, the issue with that is that it would be checked by default, and might be confusing to users a little bit because the 5 sec spawn immunity was not exposed in any UI previously
in the follow up PR for the other options, the card in the host modal is changed to have a slider
if the slider is kept, a max value or individiual step sizes are required. or i revert to number input and go for minutes, your call
| if (this.target.isPlayer()) { | ||
| const targetPlayer = this.target as Player; | ||
| if ( | ||
| targetPlayer.type() === PlayerType.Human && | ||
| this.mg.config().numSpawnPhaseTurns() + | ||
| this.mg.config().spawnImmunityDuration() > | ||
| this.mg.ticks() | ||
| this.mg.ticks() |
There was a problem hiding this comment.
instead i think we should do:
if(!this.player.canAttack(targetPlayer) {
...
this.active = false
return
}
| if ( | ||
| this.target.isPlayer() && | ||
| this.mg.config().numSpawnPhaseTurns() + | ||
| this.mg.config().spawnImmunityDuration() > | ||
| this.mg.ticks() | ||
| ) { | ||
| const targetPlayer = this.target as Player; | ||
| if (targetPlayer.type() === PlayerType.Human) { | ||
| this.active = false; | ||
| return; | ||
| } | ||
| } |
There was a problem hiding this comment.
same here, use canAttack. so we keep all the logic in one place
| const spawnImmunityActive = this.isSpawnImmunityActive(); | ||
| if (spawnImmunityActive) { | ||
| this.warship.setTargetUnit(undefined); | ||
| } else { | ||
| this.warship.setTargetUnit(this.findTargetUnit()); | ||
| if (this.warship.targetUnit()?.type() === UnitType.TradeShip) { | ||
| this.huntDownTradeShip(); | ||
| return; | ||
| } |
There was a problem hiding this comment.
same here, we should be using canAttack
There was a problem hiding this comment.
in the current implementation, warships are completely passive, even in regards to warships / transports from nations, but attacking nations is possible. if we move the check into canAttack for warships, that changes. fine for me if that's what we want, pls confirm
| if ( | ||
| game.config().numSpawnPhaseTurns() + game.config().spawnImmunityDuration() > | ||
| game.ticks() | ||
| ) { | ||
| const targetOwner = game.owner(tile); | ||
| if (targetOwner.isPlayer() && targetOwner.type() === PlayerType.Human) { | ||
| return false; | ||
| } | ||
| } |
There was a problem hiding this comment.
same here, can we use canAttack?
| // startingGold: z.number().int().min(0).max(10*1000*1000), // maybe in steps instead? good way of setting max? default 0? | ||
| // incomeMultiplier: z.number().min(0).max(10), |
There was a problem hiding this comment.
please remove these comments
| maxTimerValue: z.number().int().min(1).max(120).optional(), | ||
| // startingGold: z.number().int().min(0).max(10*1000*1000), // maybe in steps instead? good way of setting max? default 0? | ||
| // incomeMultiplier: z.number().min(0).max(10), | ||
| spawnImmunityDuration: z.number().int().min(0).max(3000), // In ticks (10 per second) |
There was a problem hiding this comment.
make optional, do we need a max?
| randomSpawn: false, | ||
| gameMode: GameMode.FFA, | ||
| bots: 400, | ||
| spawnImmunityDuration: 5 * 10, |
There was a problem hiding this comment.
let's remove this option, in Config we can use a default if not set.
| private isSpawnImmunityActive(): boolean { | ||
| return ( | ||
| this.mg.config().numSpawnPhaseTurns() + | ||
| this.mg.config().spawnImmunityDuration() > | ||
| this.mg.ticks() | ||
| ); | ||
| } |
There was a problem hiding this comment.
this should be moved to Game i think.
4 participants
Split off from #2458 so its easier to review.
The integration into the
HostLobbyModalandSingleplayerModalare changed later (in #2461)Description:
Includes the following additional option when creating lobbies:
Please complete the following:
I don't think tests are required for these features, I'm happy to implement them if a reviewer disagrees.
Please put your Discord username so you can be contacted if a bug or regression is found:
newyearnewphil / [CYN] super mario