v30 nuke wars preparation: Disable boats & Team spawn zones#3263
v30 nuke wars preparation: Disable boats & Team spawn zones#3263
Conversation
FloPinguin
changed the title
WalkthroughThe PR adds team-based spawn area definitions to maps and introduces the TransportShip unit type. Changes include new teamGameSpawnAreas fields in map manifests, SpawnArea type definitions in game core, spawn location enforcement within designated team areas, and UI/localization support for the boat unit. Changes
Sequence DiagramsequenceDiagram
actor User
participant GameRunner
participant TerrainMapLoader
participant GameImpl
participant SpawnExecution
participant NationExecution
User->>GameRunner: createGame(config with map)
GameRunner->>TerrainMapLoader: loadTerrainMap(mapType)
TerrainMapLoader-->>GameRunner: TerrainMapData {gameMap, teamGameSpawnAreas}
GameRunner->>GameImpl: new GameImpl(..., teamGameSpawnAreas)
GameImpl->>GameImpl: store teamGameSpawnAreas
GameRunner-->>User: Game ready
User->>GameImpl: startGame()
GameImpl->>NationExecution: execute spawn phase
NationExecution->>GameImpl: teamSpawnArea(nation.team)
GameImpl-->>NationExecution: SpawnArea or undefined
alt Spawn outside team area
NationExecution->>SpawnExecution: schedule with team area constraint
else Spawn inside or no area
NationExecution->>SpawnExecution: proceed normally
end
SpawnExecution->>SpawnExecution: getTeamSpawnArea()
SpawnExecution->>SpawnExecution: randTile(spawnArea)
SpawnExecution-->>NationExecution: spawn tile
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~30 minutes Possibly related PRs
Suggested reviewers
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. Tip Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
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.
src/core/game/SpawnArea.ts
Outdated
There was a problem hiding this comment.
nit: mind keeping this in Game.ts for now. Eventually we'll need to figure out how to break it up.
github-project-automation
bot
moved this from Development
to Final Review
in OpenFront Release Management
github-project-automation
bot
moved this from Final Review
to Complete
in OpenFront Release Management
There was a problem hiding this comment.
🧹 Nitpick comments (3)
src/core/execution/SpawnExecution.ts (2)
159-166: Consider passing the player directly instead of re-fetching it.
getTeamSpawnArea()callsthis.mg.player(this.playerInfo.id)to get the player, but the caller (getSpawn) is itself called fromtick()which already has theplayerreference. Passing it as a parameter avoids a redundant map lookup and makes the data flow easier to follow.This is a minor style point — the lookup is safe because
tick()guarantees the player exists by this point.🤖 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 159 - 166, The getTeamSpawnArea method re-fetches the player via this.mg.player(this.playerInfo.id) causing a redundant lookup; change getTeamSpawnArea to accept a Player (or the exact player type) as a parameter and use that to obtain team and spawn area, then update callers (notably getSpawn and its caller tick) to pass the existing player reference instead of relying on playerInfo.id — ensure signature updates to getTeamSpawnArea(player: Player): SpawnArea | undefined and adjust any type imports/usages accordingly.
148-157: Clarify thatmg.ref()throws loudly on invalid coordinates.If a
SpawnAreahas coordinates extending beyond the map dimensions (e.g.,area.x + area.width > map.width()), callingnextInt(area.x, area.x + area.width)could generate a coordinate that exceeds the map bounds. When passed tothis.mg.ref(x, y), it throws an error:Invalid coordinates: ${x},${y}.Since map manifests are author-controlled and the code already fails loudly on corruption (rather than silently), the current behavior aligns with the codebase's established pattern for trusted data sources. Adding clamping remains a reasonable robustness improvement but is not necessary for correctness:
Suggested bounds clamping for extra safety
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); + const x = this.random.nextInt( + Math.max(0, area.x), + Math.min(this.mg.width(), area.x + area.width), + ); + const y = this.random.nextInt( + Math.max(0, area.y), + Math.min(this.mg.height(), area.y + area.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 148 - 157, randTile can generate coordinates outside the map if a SpawnArea extends beyond map bounds and this.mg.ref(x,y) will throw "Invalid coordinates: x,y" — make this explicit: in randTile (method randTile and using SpawnArea, random.nextInt, mg.width/height and this.mg.ref) add a pre-check that clamps or validates x/y against this.mg.width()/height() and either clamp to valid range or throw a clearer error message before calling this.mg.ref; include a short comment explaining that manifests are trusted so we fail loudly (or that clamping is applied for extra robustness).src/core/game/TerrainMapLoader.ts (1)
59-66: Pre-existing: nation scaling mutatesmanifest.nationsin place.Unlike the new spawn-area scaling (which correctly creates a fresh object), this block mutates
manifest.nations[i].coordinatesdirectly. Sincemanifestcomes from a loader and the result cachesnations: manifest.nations, this means the cached result holds already-mutated references. A second call with the same map but a different size would re-read the (now-mutated) manifest from a different source, but if any code path reuses the manifest object, coordinates would be wrong.This is not introduced by your PR — just flagging it since you did the right thing with spawn areas and may want to align this block later.
🤖 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 59 - 66, The nation scaling currently mutates manifest.nations in place (inside TerrainMapLoader where mapSize === GameMapSize.Compact), which contaminates the cached manifest; instead, create and return a new nations array with new nation objects and copied/scaled coordinate arrays (e.g. use manifest.nations.map to produce { ...nation, coordinates: [Math.floor(...), Math.floor(...)] }) so the original manifest remains unmodified—align this approach with the spawn-area scaling logic to ensure cached results hold fresh copies rather than mutated references.
🤖 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 159-166: The getTeamSpawnArea method re-fetches the player via
this.mg.player(this.playerInfo.id) causing a redundant lookup; change
getTeamSpawnArea to accept a Player (or the exact player type) as a parameter
and use that to obtain team and spawn area, then update callers (notably
getSpawn and its caller tick) to pass the existing player reference instead of
relying on playerInfo.id — ensure signature updates to getTeamSpawnArea(player:
Player): SpawnArea | undefined and adjust any type imports/usages accordingly.
- Around line 148-157: randTile can generate coordinates outside the map if a
SpawnArea extends beyond map bounds and this.mg.ref(x,y) will throw "Invalid
coordinates: x,y" — make this explicit: in randTile (method randTile and using
SpawnArea, random.nextInt, mg.width/height and this.mg.ref) add a pre-check that
clamps or validates x/y against this.mg.width()/height() and either clamp to
valid range or throw a clearer error message before calling this.mg.ref; include
a short comment explaining that manifests are trusted so we fail loudly (or that
clamping is applied for extra robustness).
In `@src/core/game/TerrainMapLoader.ts`:
- Around line 59-66: The nation scaling currently mutates manifest.nations in
place (inside TerrainMapLoader where mapSize === GameMapSize.Compact), which
contaminates the cached manifest; instead, create and return a new nations array
with new nation objects and copied/scaled coordinate arrays (e.g. use
manifest.nations.map to produce { ...nation, coordinates: [Math.floor(...),
Math.floor(...)] }) so the original manifest remains unmodified—align this
approach with the spawn-area scaling logic to ensure cached results hold fresh
copies rather than mutated references.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
resources/lang/en.jsonsrc/core/GameRunner.tssrc/core/configuration/DefaultConfig.tssrc/core/execution/SpawnExecution.tssrc/core/game/Game.tssrc/core/game/GameImpl.tssrc/core/game/TerrainMapLoader.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- src/core/GameRunner.ts
- src/core/game/GameImpl.ts
2 participants
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