v30 nuke wars preparation: Disable boats & Team spawn zones#3263
v30 nuke wars preparation: Disable boats & Team spawn zones#3263FloPinguin wants to merge 9 commits intomainfrom
Conversation
FloPinguin
changed the title
|
Important Review skippedThis PR was authored by the user configured for CodeRabbit reviews. By default, CodeRabbit skips reviewing PRs authored by this user. It's recommended to use a dedicated user account to post CodeRabbit review feedback. To trigger a single review, invoke the You can disable this status message by setting the Use the checkbox below for a quick retry:
WalkthroughAdds team-based rectangular spawn areas: new SpawnArea types and TeamGameSpawnAreas, map/manifest fields, loader scaling for Compact maps, Game plumbing to store/expose team areas, spawn logic constrained to team areas when present, plus a boat unit label and UI option. Changes
Sequence Diagram(s)sequenceDiagram
participant Loader as MapLoader (TerrainMapLoader)
participant Runner as GameRunner
participant Game as GameImpl
participant Nation as NationExecution
participant Spawn as SpawnExecution
Loader->>Loader: load manifest\nextract teamGameSpawnAreas
Loader->>Loader: scale areas if mapSize == Compact
Runner->>Game: createGame(..., teamGameSpawnAreas)
Game->>Game: store _teamGameSpawnAreas
Nation->>Game: teamSpawnArea(team)
Game-->>Nation: return SpawnArea[] or undefined
alt nation spawn outside team area or needs constrained spawn
Nation->>Spawn: schedule spawn with team area
Spawn->>Game: teamSpawnArea(team)
Game-->>Spawn: return area
Spawn->>Spawn: pick random tile within area
Spawn->>Nation: place unit
else no team area
Nation->>Nation: use normal spawn logic
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/core/game/TerrainMapLoader.ts (1)
43-44:⚠️ Potential issue | 🟠 MajorCompact-map spawn-area scaling is silently bypassed for cached map types
loadedMapsis keyed only byGameMapType(line 43–44). Iffourislandsis first loaded asNormalsize, the cache stores the full-sizeteamGameSpawnAreas({x:750, y:0, …}). A laterCompact-size load hits the early return and returns the same unscaled areas. At runtime,randTilewould generate coordinates in the range[0, 1500)on a750×750map, producing out-of-bounds tile refs on every team-area spawn attempt.The same cache-key gap exists for
gameMap/miniGameMapselection (pre-existing), but this PR adds new behavior that is immediately incorrect when triggered.The minimal fix is to include
mapSizein the cache key:🔧 Proposed fix
-const loadedMaps = new Map<GameMapType, TerrainMapData>(); +const loadedMaps = new Map<string, TerrainMapData>(); export async function loadTerrainMap( map: GameMapType, mapSize: GameMapSize, terrainMapFileLoader: GameMapLoader, ): Promise<TerrainMapData> { - const cached = loadedMaps.get(map); + const cacheKey = `${map}:${mapSize}`; + const cached = loadedMaps.get(cacheKey); if (cached !== undefined) return cached; // ... - loadedMaps.set(map, result); + loadedMaps.set(cacheKey, result); return result; }Also applies to: 70-83
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/game/TerrainMapLoader.ts` around lines 43 - 44, The cache for loadedMaps is keyed only by GameMapType, causing Compact vs Normal loads to share unscaled teamGameSpawnAreas; update TerrainMapLoader to incorporate mapSize into the cache key (e.g., use a composite key of GameMapType + mapSize) so that loadedMaps.get(...) and loadedMaps.set(...) distinguish sizes, and apply the same composite-key change to the other cache usages around the gameMap/miniGameMap selection (lines ~70–83) so randTile and teamGameSpawnAreas are generated/stored per (map, mapSize) combination.
🧹 Nitpick comments (2)
src/core/execution/SpawnExecution.ts (1)
81-133:getTeamSpawnArea()is re-evaluated on every loop iteration despite returning a constant valueThe spawn area for a given player/team does not change during
randomSpawnLand(). CallinggetTeamSpawnArea()(and thereforehasPlayer(),player(),team(),teamSpawnArea()) up toMAX_SPAWN_TRIEStimes adds needless overhead and reduces readability. Computing it once before the loop also keepsrandTile()a pure coordinate-generator.♻️ Proposed refactor
private randomSpawnLand(): TileRef | undefined { let tries = 0; + const spawnArea = this.getTeamSpawnArea(); while (tries < SpawnExecution.MAX_SPAWN_TRIES) { tries++; - const tile = this.randTile(); + const tile = this.randTile(spawnArea); // ... } return; } - private randTile(): TileRef { - const area = this.getTeamSpawnArea(); - if (area) { + private randTile(area?: SpawnArea): TileRef { + if (area) { const x = this.random.nextInt(area.x, area.x + area.width); const y = this.random.nextInt(area.y, area.y + area.height); return this.mg.ref(x, y); } const x = this.random.nextInt(0, this.mg.width()); const y = this.random.nextInt(0, this.mg.height()); return this.mg.ref(x, y); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/execution/SpawnExecution.ts` around lines 81 - 133, In randomSpawnLand(), cache the team spawn area once before the while loop instead of calling getTeamSpawnArea() repeatedly via randTile(); compute const area = this.getTeamSpawnArea() (or similar) once and pass that area into randTile (or make randTile accept an optional area parameter) so randTile becomes a pure coordinate generator that uses the provided area when present; update randomSpawnLand() to call randTile(area) each iteration and leave MAX_SPAWN_TRIES, random, and other logic unchanged.src/core/game/TerrainMapLoader.ts (1)
4-6: Re-exportingSpawnArea/TeamGameSpawnAreasfromTerrainMapLoadercreates a duplicate import path
SpawnArea.tsis the canonical source. All changed files in this PR import directly fromSpawnArea.ts. Theexport type { SpawnArea, TeamGameSpawnAreas }here adds a second path (TerrainMapLoader) for the same types with no current consumer. If left in place, future code may import from either location inconsistently. Consider removing the re-export and letting callers depend onSpawnArea.tsdirectly.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/game/TerrainMapLoader.ts` around lines 4 - 6, Remove the re-export that creates a duplicate import path: delete the line "export type { SpawnArea, TeamGameSpawnAreas }" from TerrainMapLoader.ts so SpawnArea and TeamGameSpawnAreas remain exported only from SpawnArea.ts; keep the local import if TerrainMapLoader still uses those types, and if any files were accidentally importing the types from TerrainMapLoader, change them to import from SpawnArea.ts instead.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/core/game/GameImpl.ts`:
- Around line 800-814: The field playerTeams is declared as Team[] but can be
uninitialized at runtime; update its declaration to be explicitly optional
(Team[] | undefined) or initialize it to an empty array (Team[] = []) so
TypeScript reflects the runtime possibility, then adjust any code that relies on
it (e.g., teamSpawnArea and populateTeams) to handle the new type (use non-null
checks or assign before use) so the existing runtime guard in teamSpawnArea
remains meaningful to the type system.
---
Outside diff comments:
In `@src/core/game/TerrainMapLoader.ts`:
- Around line 43-44: The cache for loadedMaps is keyed only by GameMapType,
causing Compact vs Normal loads to share unscaled teamGameSpawnAreas; update
TerrainMapLoader to incorporate mapSize into the cache key (e.g., use a
composite key of GameMapType + mapSize) so that loadedMaps.get(...) and
loadedMaps.set(...) distinguish sizes, and apply the same composite-key change
to the other cache usages around the gameMap/miniGameMap selection (lines
~70–83) so randTile and teamGameSpawnAreas are generated/stored per (map,
mapSize) combination.
---
Nitpick comments:
In `@src/core/execution/SpawnExecution.ts`:
- Around line 81-133: In randomSpawnLand(), cache the team spawn area once
before the while loop instead of calling getTeamSpawnArea() repeatedly via
randTile(); compute const area = this.getTeamSpawnArea() (or similar) once and
pass that area into randTile (or make randTile accept an optional area
parameter) so randTile becomes a pure coordinate generator that uses the
provided area when present; update randomSpawnLand() to call randTile(area) each
iteration and leave MAX_SPAWN_TRIES, random, and other logic unchanged.
In `@src/core/game/TerrainMapLoader.ts`:
- Around line 4-6: Remove the re-export that creates a duplicate import path:
delete the line "export type { SpawnArea, TeamGameSpawnAreas }" from
TerrainMapLoader.ts so SpawnArea and TeamGameSpawnAreas remain exported only
from SpawnArea.ts; keep the local import if TerrainMapLoader still uses those
types, and if any files were accidentally importing the types from
TerrainMapLoader, change them to import from SpawnArea.ts instead.
github-project-automation
bot
moved this from Triage
to Development
in OpenFront Release Management
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/core/game/GameImpl.ts (1)
800-814: Remove dead!this.playerTeamsguard.
playerTeamsis now initialized to[](line 98), so!this.playerTeamsis alwaysfalse— an empty array is truthy in JavaScript. The!areascheck on line 806 already handles the non-team-mode case (whenplayerTeamsis[],numTeamsis0, and_teamGameSpawnAreas["0"]isundefined).♻️ Remove dead guard
teamSpawnArea(team: Team): SpawnArea | undefined { - if (!this._teamGameSpawnAreas || !this.playerTeams) { + if (!this._teamGameSpawnAreas) { return undefined; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/game/GameImpl.ts` around lines 800 - 814, In teamSpawnArea, remove the dead guard that checks !this.playerTeams: since playerTeams is always initialized to an array, the check is redundant; update the function (referencing teamSpawnArea, this.playerTeams, and this._teamGameSpawnAreas) to only early-return when this._teamGameSpawnAreas is falsy, leaving the later logic (numTeams, areas lookup, teamIndex bounds check) unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/core/game/GameImpl.ts`:
- Around line 800-814: In teamSpawnArea, remove the dead guard that checks
!this.playerTeams: since playerTeams is always initialized to an array, the
check is redundant; update the function (referencing teamSpawnArea,
this.playerTeams, and this._teamGameSpawnAreas) to only early-return when
this._teamGameSpawnAreas is falsy, leaving the later logic (numTeams, areas
lookup, teamIndex bounds check) unchanged.
…ry playerTeams check
…ction refactor(TerrainMapLoader): clean up imports by removing unused SpawnArea export
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/core/game/TerrainMapLoader.ts (1)
41-42:⚠️ Potential issue | 🟡 MinorCache is keyed by
maponly, not bymapSize— stale data risk.The module-level
loadedMapscache usesGameMapTypeas the sole key (line 13). However, the cached data includes mapSize-dependent values: nations are scaled when mapSize is Compact (lines 59-65), and teamGameSpawnAreas are scaled conditionally (lines 70-81).If the same map is loaded with different mapSize values in one process, the second call returns the first call's cached result, including its (un)scaled nations and spawn areas. This is incorrect.
This is a pre-existing issue, but the new spawn-area scaling makes it more visible. Consider fixing the cache key to include mapSize:
new Map<[GameMapType, GameMapSize], TerrainMapData>().🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/game/TerrainMapLoader.ts` around lines 41 - 42, The cache is currently keyed only by GameMapType (loadedMaps) so calls to loadTerrainMap (or the loader function that reads `map` and `mapSize`) can return stale data when the same map is requested with different GameMapSize; change loadedMaps to use a composite key that includes mapSize (e.g., Map<[GameMapType, GameMapSize], TerrainMapData> or a serialized string key `${map}:${mapSize}`), update the lookup where `const cached = loadedMaps.get(map)` to use the composite key, and update where results are stored into loadedMaps to use the same composite key so scaled nations and teamGameSpawnAreas are cached per map+size combination (ensure all references to loadedMaps.get/set use the new key format).
🧹 Nitpick comments (2)
src/core/game/TerrainMapLoader.ts (1)
68-81: Scaling logic is consistent with nation coordinates — good. One small note onMath.floorfor width/height.When you halve
widthandheightwithMath.floor, odd values lose a pixel (e.g., width 11 becomes 5, not 5.5). This shrinks the spawn area slightly. For large areas this is fine, but if any area dimension is 1, it becomes 0, which would makerandTilecallnextInt(x, x)— a degenerate range. Probably never happens in practice, but aMath.max(..., 1)guard would be cheap insurance.Optional: guard against zero-sized dimensions
scaled[key] = areas.map((a) => ({ x: Math.floor(a.x / 2), y: Math.floor(a.y / 2), - width: Math.floor(a.width / 2), - height: Math.floor(a.height / 2), + width: Math.max(Math.floor(a.width / 2), 1), + height: Math.max(Math.floor(a.height / 2), 1), }));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/game/TerrainMapLoader.ts` around lines 68 - 81, The scaling block for teamGameSpawnAreas can produce zero width/height when halving odd small dimensions; update the scaling in TerrainMapLoader where teamGameSpawnAreas is computed (the loop that builds scaled) to clamp width and height to at least 1 (e.g., replace Math.floor(a.width / 2) and Math.floor(a.height / 2) with Math.max(Math.floor(...), 1)) so spawn areas never become zero-sized; keep x/y as Math.floor(a.x/2) but ensure width/height use the Math.max guard to avoid degenerate ranges later when randTile or similar functions consume these areas.src/core/execution/SpawnExecution.ts (1)
135-145: ThehasPlayerguard on line 136 is always true at the call site — harmless but redundant.When
randomSpawnLand()is called fromtick()(line 51), the player has already been added at lines 40–44. SohasPlayercan never be false here. This is safe defensive code — not a problem, just noting it in case you want to simplify.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/execution/SpawnExecution.ts` around lines 135 - 145, The hasPlayer() check in getTeamSpawnArea() is redundant at the call site (randomSpawnLand() invoked from tick() after the player is added), so remove the unnecessary guard and simplify the method: directly call this.mg.player(this.playerInfo.id), retrieve team via player.team(), return undefined if team is null, otherwise return this.mg.teamSpawnArea(team); keep method name getTeamSpawnArea and callers unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@src/core/game/TerrainMapLoader.ts`:
- Around line 41-42: The cache is currently keyed only by GameMapType
(loadedMaps) so calls to loadTerrainMap (or the loader function that reads `map`
and `mapSize`) can return stale data when the same map is requested with
different GameMapSize; change loadedMaps to use a composite key that includes
mapSize (e.g., Map<[GameMapType, GameMapSize], TerrainMapData> or a serialized
string key `${map}:${mapSize}`), update the lookup where `const cached =
loadedMaps.get(map)` to use the composite key, and update where results are
stored into loadedMaps to use the same composite key so scaled nations and
teamGameSpawnAreas are cached per map+size combination (ensure all references to
loadedMaps.get/set use the new key format).
---
Nitpick comments:
In `@src/core/execution/SpawnExecution.ts`:
- Around line 135-145: The hasPlayer() check in getTeamSpawnArea() is redundant
at the call site (randomSpawnLand() invoked from tick() after the player is
added), so remove the unnecessary guard and simplify the method: directly call
this.mg.player(this.playerInfo.id), retrieve team via player.team(), return
undefined if team is null, otherwise return this.mg.teamSpawnArea(team); keep
method name getTeamSpawnArea and callers unchanged.
In `@src/core/game/TerrainMapLoader.ts`:
- Around line 68-81: The scaling block for teamGameSpawnAreas can produce zero
width/height when halving odd small dimensions; update the scaling in
TerrainMapLoader where teamGameSpawnAreas is computed (the loop that builds
scaled) to clamp width and height to at least 1 (e.g., replace
Math.floor(a.width / 2) and Math.floor(a.height / 2) with
Math.max(Math.floor(...), 1)) so spawn areas never become zero-sized; keep x/y
as Math.floor(a.x/2) but ensure width/height use the Math.max guard to avoid
degenerate ranges later when randTile or similar functions consume these areas.
…ieval refactor(SpawnExecution): remove unnecessary player check in getTeamSpawnArea
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/core/execution/SpawnExecution.ts (1)
124-133:randTilelogic is correct; note implicit assumption aboutnextIntupper-bound exclusivity.
nextInt(area.x, area.x + area.width)produces exactlywidthdistinct x-values only ifnextIntis exclusive on the upper bound — which is consistent with the existingnextInt(0, mg.width())usage for the full-map path. IfPseudoRandom.nextIntsemantics ever change to inclusive, this would silently sample one column/row outside the area. A brief comment here would make the intent explicit.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/execution/SpawnExecution.ts` around lines 124 - 133, Add a short clarifying comment above the randTile method that documents the assumption that PseudoRandom.nextInt(a, b) treats b as an exclusive upper bound (so nextInt(area.x, area.x + area.width) yields exactly area.width distinct values), and note that if nextInt semantics change to inclusive the sampling could go out of the intended area; reference the randTile function and PseudoRandom.nextInt in the comment and, if desired, suggest clamping or using (width-1)/(height-1) to guard against out-of-range coordinates.src/core/game/TerrainMapLoader.ts (1)
41-42: Cache key fix is correct; consider a nestedMapto eliminate string-collision risk entirely.The old single-key lookup was buggy — loading the same map in two different sizes could return stale scaled/unscaled data from cache. The composite string key
${map}:${mapSize}fixes that correctly.Minor: when concatenating strings, the joiner must not appear in either key part.
GameMapTypevalues look like"WorldMap"/"BaikalNukeWars"so:is safe in practice, but a nestedMapremoves the concern completely:♻️ Nested map alternative
-const loadedMaps = new Map<string, TerrainMapData>(); +const loadedMaps = new Map<GameMapType, Map<GameMapSize, TerrainMapData>>();Then adjust lookup/insert:
- const cacheKey = `${map}:${mapSize}`; - const cached = loadedMaps.get(cacheKey); - if (cached !== undefined) return cached; + const cached = loadedMaps.get(map)?.get(mapSize); + if (cached !== undefined) return cached;- loadedMaps.set(cacheKey, result); + if (!loadedMaps.has(map)) loadedMaps.set(map, new Map()); + loadedMaps.get(map)!.set(mapSize, result);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/game/TerrainMapLoader.ts` around lines 41 - 42, Replace the composite string key approach in TerrainMapLoader (the cacheKey variable and its use with loadedMaps) with a nested Map keyed first by map (GameMapType) then by mapSize to avoid string-collision risks; change loadedMaps to Map<map, Map<mapSize, LoadedMapType>> (or equivalent), read via inner = loadedMaps.get(map) then cached = inner?.get(mapSize), and on insert ensure an inner map is created if missing (loadedMaps.set(map, new Map()) then inner.set(mapSize, value)); update all lookup/insert sites that currently use cacheKey to use this two-level map access pattern.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/core/execution/SpawnExecution.ts`:
- Around line 124-133: Add a short clarifying comment above the randTile method
that documents the assumption that PseudoRandom.nextInt(a, b) treats b as an
exclusive upper bound (so nextInt(area.x, area.x + area.width) yields exactly
area.width distinct values), and note that if nextInt semantics change to
inclusive the sampling could go out of the intended area; reference the randTile
function and PseudoRandom.nextInt in the comment and, if desired, suggest
clamping or using (width-1)/(height-1) to guard against out-of-range
coordinates.
In `@src/core/game/TerrainMapLoader.ts`:
- Around line 41-42: Replace the composite string key approach in
TerrainMapLoader (the cacheKey variable and its use with loadedMaps) with a
nested Map keyed first by map (GameMapType) then by mapSize to avoid
string-collision risks; change loadedMaps to Map<map, Map<mapSize,
LoadedMapType>> (or equivalent), read via inner = loadedMaps.get(map) then
cached = inner?.get(mapSize), and on insert ensure an inner map is created if
missing (loadedMaps.set(map, new Map()) then inner.set(mapSize, value)); update
all lookup/insert sites that currently use cacheKey to use this two-level map
access pattern.
# Conflicts: # src/core/execution/SpawnExecution.ts
…d layout consistency" This reverts commit a5f5562.
1 participant
Description:
Preparation for nuke wars, for v30.
Next PR will be adding the nuke wars modifier for public games, but Wonders #3224 needs to be merged first to avoid merge conflicts.
1. Disable boats setting
It's possible to disable
UnitType.TransportShipnow. Because they are not needed in nuke wars and can even be annoying.2. Team spawn zones for random spawn
Maps can have
teamGameSpawnAreasin their json file now.Spawn areas are currently active if
Please complete the following:
Please put your Discord username so you can be contacted if a bug or regression is found:
FloPinguin