Skip to content

Conversation

@FloPinguin
Copy link
Contributor

Description:

The calculation is based on: 50 players per 1_000_000 land tiles, limited at 125 players because of performance
Second number is 75% of that, third one 50%
That way, the player counts are staying mostly the same
Look at the "Dynamic Config" column, these are the new player counts:
(The 125 players limit is missing in that column, only relevant for the twolakes map)

Screenshot_2026-01-12_152758 Screenshot_2026-01-12_152814

This PR also removes MapDescription from Maps.ts because its unused.
And this PR updates the map-generator README.md to reflect the changes

Please complete the following:

  • I have added screenshots for all UI updates
  • I process any text displayed to the user through translateText() and I've added it to the en.json file
  • I have added relevant tests to the test directory
  • I confirm I have thoroughly tested these changes and take full responsibility for any bugs introduced

Please put your Discord username so you can be contacted if a bug or regression is found:

FloPinguin

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Jan 12, 2026

Walkthrough

Player-count and map-eligibility logic was moved from ServerConfig/DefaultConfig into MapPlaylist and made asynchronous via map land-tile lookups. gameConfig() now returns a Promise; callers were updated. MapDescription and related per-map constants were removed; a new MapLandTiles utility was added.

Changes

Cohort / File(s) Summary
Config API Removals
src/core/configuration/Config.ts, src/core/configuration/DefaultConfig.ts
Removed public APIs and data: lobbyMaxPlayers(), getRandomPublicGameModifiers(), supportsCompactMapForTeams(), and the numPlayersConfig mapping; removed related imports (GameMapType, PublicGameModifiers, etc.).
MapPlaylist Async Refactor
src/server/MapPlaylist.ts
gameConfig()async Promise<GameConfig>; moved lobby sizing and compact-map checks into async/private methods (lobbyMaxPlayers, supportsCompactMapForTeams, calculateMapPlayerCounts, getRandomPublicGameModifiers); uses land-tile data for player calculations.
Map Land-Tile Utility
src/server/MapLandTiles.ts
New module getMapLandTiles(map): Promise<number> with lazy FetchGameMapLoader initialization, manifest() usage, caching, and error fallback.
Async Call Sites
src/server/Master.ts, src/server/Worker.ts
Updated call sites to await playlist.gameConfig() before serializing or passing to game creation logic.
Client & Test Updates
src/client/components/Maps.ts, tests/util/TestServerConfig.ts
Removed exported MapDescription; TestServerConfig methods changed to async signatures and removed map parameters.
Docs
map-generator/README.md
Updated instructions: add maps to GameMapType and MapPlaylist (reflects removal of MapDescription/DefaultConfig mapping guidance).

Sequence Diagram(s)

sequenceDiagram
    participant Worker as Worker/Master
    participant Playlist as MapPlaylist
    participant LandTiles as MapLandTiles
    participant Loader as FetchGameMapLoader

    Worker->>Playlist: await gameConfig()
    Playlist->>Playlist: lobbyMaxPlayers(map, mode, teams, isCompact)
    Playlist->>LandTiles: await getMapLandTiles(map)
    LandTiles->>Loader: getMapLoader() / load(map)
    Loader-->>LandTiles: Promise<manifest>
    LandTiles->>Loader: await manifest()
    Loader-->>LandTiles: manifest (num_land_tiles)
    LandTiles-->>Playlist: land tile count
    Playlist->>Playlist: calculateMapPlayerCounts(landTiles) / apply team adjustments
    Playlist-->>Worker: Promise<GameConfig> (resolved)
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Possibly related PRs

Suggested labels

Balance Tweak, Maps

Suggested reviewers

  • evanpelle
  • scottanderson

Poem

🗺️ Config folded its wings to roam,
Async maps now fetch their home,
Tile counts whisper player tunes,
Playlists hum beneath bright moons,
Lobbies sized where map light shone. ✨

🚥 Pre-merge checks | ✅ 3
✅ Passed checks (3 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly and specifically describes the main change: removing hardcoded player configuration and implementing dynamic calculation based on map land tiles.
Description check ✅ Passed The description is well-related to the changeset, explaining the calculation logic, including screenshots of results, and noting additional changes like MapDescription removal and README updates.
Docstring Coverage ✅ Passed Docstring coverage is 83.33% which is sufficient. The required threshold is 80.00%.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.


📜 Recent review details

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 3145a33 and 6a10525.

📒 Files selected for processing (1)
  • src/server/MapLandTiles.ts
🚧 Files skipped from review as they are similar to previous changes (1)
  • src/server/MapLandTiles.ts

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 5

🤖 Fix all issues with AI agents
In @src/core/configuration/DefaultConfig.ts:
- Around line 1-3: DefaultConfig currently imports getMapLandTiles from server
code, creating a hard core->server dependency; remove the direct import and
instead accept a map-tile provider as an injected dependency (e.g., a
fetchMapTiles or getMapLandTiles function parameter on the DefaultConfig
constructor or init function) or lazily load it only when needed; update call
sites to pass the server implementation (or swap in a core implementation if you
decide to move the logic into src/core/), and ensure DefaultConfig has a safe
no-op/default provider for environments without a server (tests/clients) so core
remains independent.

In @src/server/MapLandTiles.ts:
- Around line 13-14: The top-of-file comment incorrectly refers to
BinaryLoaderGameMapLoader; update the comment to mention the actual loader in
use, FetchGameMapLoader, so it reads something like "FetchGameMapLoader already
caches maps, so no need for additional caching here." Locate the header comment
in MapLandTiles (around the function/class handling land tile counts) and
replace the wrong class name with FetchGameMapLoader to keep documentation
accurate.
- Around line 7-11: getMapLoader currently hardcodes
"http://localhost:3000/maps"; change it to read the base URL from an environment
variable (e.g. process.env.GAME_MAPS_URL or MAPS_SERVICE_URL) and pass that into
the FetchGameMapLoader constructor (fall back to the existing localhost value
only for local dev if desired). Update the getMapLoader function so it uses the
env var when creating new FetchGameMapLoader and keep the mapLoader caching
logic intact.

In @tests/util/TestServerConfig.ts:
- Around line 52-54: The lobbyMaxPlayers stub's signature doesn't match the
ServerConfig interface; update the async method lobbyMaxPlayers(map:
GameMapType) to accept the full parameter list async lobbyMaxPlayers(map:
GameMapType, mode: GameMode, numPlayerTeams: TeamCountConfig, isCompactMap:
boolean): Promise<number>, add the missing imports for GameMode and
TeamCountConfig, and either implement the logic or keep the body throwing an
Error("Method not implemented.") so the type matches the interface.
🧹 Nitpick comments (1)
src/server/MapLandTiles.ts (1)

21-24: Silent fallback may mask production issues.

Returning 1_000_000 on error hides failures. The fallback is reasonable for resilience, but logging alone may not surface issues quickly in production.

Consider whether this should throw in certain environments (e.g., during startup/preload) or emit a metric/alert when the fallback is used.

📜 Review details

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between a7714cd and 3b095f9.

📒 Files selected for processing (7)
  • src/core/configuration/Config.ts
  • src/core/configuration/DefaultConfig.ts
  • src/server/MapLandTiles.ts
  • src/server/MapPlaylist.ts
  • src/server/Master.ts
  • src/server/Worker.ts
  • tests/util/TestServerConfig.ts
🧰 Additional context used
🧠 Learnings (10)
📓 Common learnings
Learnt from: Foorack
Repo: openfrontio/OpenFrontIO PR: 2141
File: src/client/ClientGameRunner.ts:228-234
Timestamp: 2025-10-08T17:14:49.369Z
Learning: For the window close confirmation feature in `ClientGameRunner.ts`, the troop count requirement (>10,000 troops) from issue #2137 was intentionally removed because it was arbitrary and troop count can be reported as low despite having significant land. The confirmation now triggers for any alive player regardless of troop count.
📚 Learning: 2025-12-26T22:21:21.904Z
Learnt from: FloPinguin
Repo: openfrontio/OpenFrontIO PR: 2689
File: src/client/PublicLobby.ts:245-245
Timestamp: 2025-12-26T22:21:21.904Z
Learning: In public lobbies with HumansVsNations mode in src/client/PublicLobby.ts, maxPlayers represents only human player slots (already halved in DefaultConfig.ts). The nation NPCs are added automatically server-side and don't count toward maxPlayers. Therefore, getTeamSize correctly returns maxPlayers directly for HumansVsNations to display the proper team size (e.g., maxPlayers=5 yields "5 Humans vs 5 Nations").

Applied to files:

  • src/core/configuration/Config.ts
  • src/server/MapPlaylist.ts
  • tests/util/TestServerConfig.ts
  • src/core/configuration/DefaultConfig.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/core/configuration/Config.ts
  • src/server/Master.ts
  • tests/util/TestServerConfig.ts
  • src/core/configuration/DefaultConfig.ts
📚 Learning: 2025-10-26T15:37:07.732Z
Learnt from: GlacialDrift
Repo: openfrontio/OpenFrontIO PR: 2298
File: src/client/graphics/layers/TerritoryLayer.ts:200-210
Timestamp: 2025-10-26T15:37:07.732Z
Learning: In GameImpl.ts lines 124-139, team assignment logic varies by number of teams: when numPlayerTeams < 8, teams are assigned ColoredTeams values (Red, Blue, Yellow, Green, Purple, Orange, Teal); when numPlayerTeams >= 8, teams are assigned generic string identifiers like "Team 1", "Team 2", etc., which are not members of ColoredTeams.

Applied to files:

  • src/core/configuration/Config.ts
  • src/server/MapPlaylist.ts
  • tests/util/TestServerConfig.ts
  • src/core/configuration/DefaultConfig.ts
📚 Learning: 2025-10-20T20:15:28.858Z
Learnt from: sambokai
Repo: openfrontio/OpenFrontIO PR: 2225
File: src/core/execution/FakeHumanExecution.ts:51-51
Timestamp: 2025-10-20T20:15:28.858Z
Learning: In src/core/execution/FakeHumanExecution.ts, game balance constants like MIRV_COOLDOWN_TICKS, MIRV_HESITATION_ODDS, VICTORY_DENIAL_TEAM_THRESHOLD, VICTORY_DENIAL_INDIVIDUAL_THRESHOLD, and STEAMROLL_CITY_GAP_MULTIPLIER are experimental tuning parameters subject to frequent change during balance testing. Do not flag changes to these values as issues or compare them against previous values.

Applied to files:

  • src/core/configuration/Config.ts
  • src/core/configuration/DefaultConfig.ts
📚 Learning: 2025-10-21T20:06:04.823Z
Learnt from: Saphereye
Repo: openfrontio/OpenFrontIO PR: 2233
File: src/client/HostLobbyModal.ts:891-891
Timestamp: 2025-10-21T20:06:04.823Z
Learning: For the HumansVsNations game mode in `src/client/HostLobbyModal.ts` and related files, the implementation strategy is to generate all nations and adjust their strength for balancing, rather than limiting lobby size based on the number of available nations on the map.

Applied to files:

  • src/server/MapPlaylist.ts
  • tests/util/TestServerConfig.ts
  • src/core/configuration/DefaultConfig.ts
📚 Learning: 2025-11-12T23:11:34.445Z
Learnt from: MaxHT0x
Repo: openfrontio/OpenFrontIO PR: 2262
File: src/core/configuration/DefaultConfig.ts:806-832
Timestamp: 2025-11-12T23:11:34.445Z
Learning: In src/core/configuration/DefaultConfig.ts, JSDoc documentation for configuration methods should not be added inline, as it was requested that documentation be placed elsewhere in the codebase.

Applied to files:

  • src/core/configuration/DefaultConfig.ts
📚 Learning: 2025-10-08T17:14:49.369Z
Learnt from: Foorack
Repo: openfrontio/OpenFrontIO PR: 2141
File: src/client/ClientGameRunner.ts:228-234
Timestamp: 2025-10-08T17:14:49.369Z
Learning: For the window close confirmation feature in `ClientGameRunner.ts`, the troop count requirement (>10,000 troops) from issue #2137 was intentionally removed because it was arbitrary and troop count can be reported as low despite having significant land. The confirmation now triggers for any alive player regardless of troop count.

Applied to files:

  • src/core/configuration/DefaultConfig.ts
📚 Learning: 2025-06-01T04:56:16.870Z
Learnt from: scottanderson
Repo: openfrontio/OpenFrontIO PR: 992
File: src/core/configuration/DefaultConfig.ts:0-0
Timestamp: 2025-06-01T04:56:16.870Z
Learning: In the OpenFrontIO codebase, the number of player teams (`numPlayerTeams`) will never be higher than the minimum team size, so the modulo operation in `lobbyMaxPlayers()` will not result in zero players.

Applied to files:

  • src/core/configuration/DefaultConfig.ts
📚 Learning: 2025-11-26T20:49:29.140Z
Learnt from: scamiv
Repo: openfrontio/OpenFrontIO PR: 2519
File: src/core/game/GameView.ts:516-525
Timestamp: 2025-11-26T20:49:29.140Z
Learning: In GameView.ts, when usesSharedTileState is true (SAB mode), packedTileUpdates contains unpacked tile references as BigInt(tileRef) only, because all tile state lives in the shared Uint16Array. In non-SAB mode, packedTileUpdates contains packed TileUpdate bigints in the format (tileRef << 16n | state), which must be decoded via updateTile(tu). Therefore, Number(tu) is correct in SAB mode and shifting right by 16 bits would be wrong.

Applied to files:

  • src/core/configuration/DefaultConfig.ts
🧬 Code graph analysis (3)
src/server/MapLandTiles.ts (3)
src/core/game/GameMapLoader.ts (1)
  • GameMapLoader (4-6)
src/core/game/FetchGameMapLoader.ts (1)
  • FetchGameMapLoader (4-73)
src/core/game/GameImpl.ts (1)
  • map (206-208)
src/server/MapPlaylist.ts (1)
src/core/Schemas.ts (1)
  • GameConfig (90-90)
src/core/configuration/DefaultConfig.ts (1)
src/server/MapLandTiles.ts (1)
  • getMapLandTiles (15-25)
🔇 Additional comments (10)
src/server/Master.ts (1)

310-310: LGTM!

The await is correct here since gameConfig() now returns a Promise<GameConfig>. The surrounding try/catch block handles any rejection properly.

src/server/Worker.ts (1)

499-501: LGTM!

The await is properly placed, and mutating the resolved gameConfig object before passing to createGame works as expected.

src/core/configuration/Config.ts (2)

34-39: Interface signature update looks good.

The return type Promise<number> makes sense for dynamic calculations based on map data.


62-62: LGTM!

Return type Promise<boolean> aligns with the async pattern for map-based computations.

src/server/MapPlaylist.ts (3)

91-91: LGTM!

Method signature correctly updated to async, returning Promise<GameConfig>.


110-115: Await in conditional is correct.

Using await inside the if condition works fine here. The logic remains clear: disable compact maps for team games when the map does not support it.


122-127: LGTM!

The await for lobbyMaxPlayers is properly placed, and all required arguments are passed through.

tests/util/TestServerConfig.ts (1)

85-87: LGTM!

The async signature matches the updated ServerConfig interface for supportsCompactMapForTeams.

src/core/configuration/DefaultConfig.ts (2)

131-166: Async conversion looks correct.

The method now awaits getMapLandTiles and uses calculateMapPlayerCounts for dynamic player counts. The logic for team modes, compact maps, and random tier selection is preserved.


209-215: Async conversion for supportsCompactMapForTeams is consistent.

The method correctly awaits getMapLandTiles and uses the third element (smallest count) from calculateMapPlayerCounts to determine support. The threshold of 50 is reasonable.

Duwibi
Duwibi previously approved these changes Jan 12, 2026
Copy link
Contributor

@Duwibi Duwibi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very nice change. This part of mapmaking finally seems balanced!

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Nitpick comments (2)
src/server/MapLandTiles.ts (2)

15-25: Consider adding type safety for manifest access.

The manifest.map.num_land_tiles path assumes a specific shape from response.json() (which returns any). If the manifest structure ever changes or the property is missing, this silently falls back to 1M players worth of tiles.

For robustness, validate the manifest shape or add a typed interface.

♻️ Suggested type guard
+interface MapManifest {
+  map: {
+    num_land_tiles: number;
+  };
+}
+
+function isValidManifest(obj: unknown): obj is MapManifest {
+  return (
+    typeof obj === "object" &&
+    obj !== null &&
+    "map" in obj &&
+    typeof (obj as MapManifest).map?.num_land_tiles === "number"
+  );
+}
+
 export async function getMapLandTiles(map: GameMapType): Promise<number> {
   try {
     const loader = getMapLoader();
     const mapData = loader.getMapData(map);
     const manifest = await mapData.manifest();
+    if (!isValidManifest(manifest)) {
+      console.warn(`Invalid manifest structure for ${map}, using fallback`);
+      return 1_000_000;
+    }
     return manifest.map.num_land_tiles;
   } catch (error) {
     console.error(`Failed to load manifest for ${map}:`, error);
     return 1_000_000; // Default fallback
   }
 }

21-24: Consider tracking repeated fallback usage.

The fallback silently returns 1M tiles when manifest loading fails. In production, repeated failures could indicate network issues or misconfiguration that might go unnoticed with only console.error.

If the project has metrics or structured logging, consider incrementing a counter on fallback so operations can alert on elevated failure rates.

📜 Review details

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 3b095f9 and 8cea3f8.

📒 Files selected for processing (5)
  • map-generator/README.md
  • src/client/components/Maps.ts
  • src/core/configuration/DefaultConfig.ts
  • src/server/MapLandTiles.ts
  • tests/util/TestServerConfig.ts
💤 Files with no reviewable changes (1)
  • src/client/components/Maps.ts
✅ Files skipped from review due to trivial changes (1)
  • map-generator/README.md
🚧 Files skipped from review as they are similar to previous changes (1)
  • src/core/configuration/DefaultConfig.ts
🧰 Additional context used
🧠 Learnings (9)
📓 Common learnings
Learnt from: Foorack
Repo: openfrontio/OpenFrontIO PR: 2141
File: src/client/ClientGameRunner.ts:228-234
Timestamp: 2025-10-08T17:14:49.369Z
Learning: For the window close confirmation feature in `ClientGameRunner.ts`, the troop count requirement (>10,000 troops) from issue #2137 was intentionally removed because it was arbitrary and troop count can be reported as low despite having significant land. The confirmation now triggers for any alive player regardless of troop count.
📚 Learning: 2025-07-12T06:35:10.457Z
Learnt from: Aotumuri
Repo: openfrontio/OpenFrontIO PR: 1357
File: resources/lang/ja.json:0-0
Timestamp: 2025-07-12T06:35:10.457Z
Learning: In OpenFrontIO project, "giantworldmap" is the correct localization key name for the giant world map, used consistently across all language files and TypeScript code. Do not suggest renaming this key.

Applied to files:

  • src/server/MapLandTiles.ts
📚 Learning: 2025-11-26T20:49:29.140Z
Learnt from: scamiv
Repo: openfrontio/OpenFrontIO PR: 2519
File: src/core/game/GameView.ts:516-525
Timestamp: 2025-11-26T20:49:29.140Z
Learning: In GameView.ts, when usesSharedTileState is true (SAB mode), packedTileUpdates contains unpacked tile references as BigInt(tileRef) only, because all tile state lives in the shared Uint16Array. In non-SAB mode, packedTileUpdates contains packed TileUpdate bigints in the format (tileRef << 16n | state), which must be decoded via updateTile(tu). Therefore, Number(tu) is correct in SAB mode and shifting right by 16 bits would be wrong.

Applied to files:

  • src/server/MapLandTiles.ts
📚 Learning: 2026-01-12T21:37:01.156Z
Learnt from: FloPinguin
Repo: openfrontio/OpenFrontIO PR: 2874
File: src/server/MapLandTiles.ts:7-11
Timestamp: 2026-01-12T21:37:01.156Z
Learning: In this repository's OpenFrontIO deployment, inter-service HTTP calls to the master should target http://localhost:3000 (master at port 3000) as the canonical address. Apply this as the standard for all server-side TypeScript code that communicates with the master. Avoid hardcoding non-master URLs; centralize the master address (e.g., via config or env) when possible, and ensure internal service communication uses localhost:3000 in this architecture.

Applied to files:

  • src/server/MapLandTiles.ts
  • tests/util/TestServerConfig.ts
📚 Learning: 2025-12-26T22:21:21.904Z
Learnt from: FloPinguin
Repo: openfrontio/OpenFrontIO PR: 2689
File: src/client/PublicLobby.ts:245-245
Timestamp: 2025-12-26T22:21:21.904Z
Learning: In public lobbies with HumansVsNations mode in src/client/PublicLobby.ts, maxPlayers represents only human player slots (already halved in DefaultConfig.ts). The nation NPCs are added automatically server-side and don't count toward maxPlayers. Therefore, getTeamSize correctly returns maxPlayers directly for HumansVsNations to display the proper team size (e.g., maxPlayers=5 yields "5 Humans vs 5 Nations").

Applied to files:

  • tests/util/TestServerConfig.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:

  • tests/util/TestServerConfig.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:

  • tests/util/TestServerConfig.ts
📚 Learning: 2025-06-01T04:56:16.870Z
Learnt from: scottanderson
Repo: openfrontio/OpenFrontIO PR: 992
File: src/core/configuration/DefaultConfig.ts:0-0
Timestamp: 2025-06-01T04:56:16.870Z
Learning: In the OpenFrontIO codebase, the number of player teams (`numPlayerTeams`) will never be higher than the minimum team size, so the modulo operation in `lobbyMaxPlayers()` will not result in zero players.

Applied to files:

  • tests/util/TestServerConfig.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:

  • tests/util/TestServerConfig.ts
🧬 Code graph analysis (1)
src/server/MapLandTiles.ts (3)
src/core/game/GameMapLoader.ts (1)
  • GameMapLoader (4-6)
src/core/game/FetchGameMapLoader.ts (1)
  • FetchGameMapLoader (4-73)
src/core/game/GameImpl.ts (1)
  • map (206-208)
🔇 Additional comments (2)
tests/util/TestServerConfig.ts (1)

52-54: LGTM!

The async signatures and removal of the map parameter match the updated ServerConfig interface. Stub implementations throw as expected for test doubles.

Also applies to: 85-87

src/server/MapLandTiles.ts (1)

5-11: LGTM on singleton loader setup.

Lazy initialization with ??= is clean. Per learnings, localhost:3000 is the correct internal master address for this deployment.

coderabbitai[bot]
coderabbitai bot previously approved these changes Jan 12, 2026
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Nitpick comments (2)
src/core/configuration/MapLandTiles.ts (1)

15-24: Add type guard for manifest data to avoid silent failures.

The manifest is fetched as untyped JSON. If manifest.map or num_land_tiles is missing or malformed, you could get undefined or NaN flowing into calculations downstream.

Consider adding a simple check:

♻️ Suggested improvement
 export async function getMapLandTiles(map: GameMapType): Promise<number> {
   try {
     const loader = getMapLoader();
     const mapData = loader.getMapData(map);
     const manifest = await mapData.manifest();
-    return manifest.map.num_land_tiles;
+    const tiles = manifest?.map?.num_land_tiles;
+    if (typeof tiles !== "number" || !Number.isFinite(tiles)) {
+      console.warn(`Invalid num_land_tiles for ${map}, using fallback`);
+      return 1_000_000;
+    }
+    return tiles;
   } catch (error) {
     console.error(`Failed to load manifest for ${map}:`, error);
     return 1_000_000; // Default fallback
   }
 }
src/core/configuration/DefaultConfig.ts (1)

209-215: Consider caching or combining calls to reduce duplicate fetches.

Both lobbyMaxPlayers and supportsCompactMapForTeams call getMapLandTiles(map) independently. If both methods are called for the same map in sequence (e.g., during lobby setup), this results in two async lookups.

FetchGameMapLoader caches map data internally, so this is not a correctness issue, but you could extract a shared helper or have callers pass pre-fetched tile counts if performance matters.

📜 Review details

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 8cea3f8 and a9cf666.

📒 Files selected for processing (2)
  • src/core/configuration/DefaultConfig.ts
  • src/core/configuration/MapLandTiles.ts
🧰 Additional context used
🧠 Learnings (17)
📓 Common learnings
Learnt from: Foorack
Repo: openfrontio/OpenFrontIO PR: 2141
File: src/client/ClientGameRunner.ts:228-234
Timestamp: 2025-10-08T17:14:49.369Z
Learning: For the window close confirmation feature in `ClientGameRunner.ts`, the troop count requirement (>10,000 troops) from issue #2137 was intentionally removed because it was arbitrary and troop count can be reported as low despite having significant land. The confirmation now triggers for any alive player regardless of troop count.
📚 Learning: 2025-07-12T06:35:10.457Z
Learnt from: Aotumuri
Repo: openfrontio/OpenFrontIO PR: 1357
File: resources/lang/ja.json:0-0
Timestamp: 2025-07-12T06:35:10.457Z
Learning: In OpenFrontIO project, "giantworldmap" is the correct localization key name for the giant world map, used consistently across all language files and TypeScript code. Do not suggest renaming this key.

Applied to files:

  • src/core/configuration/MapLandTiles.ts
📚 Learning: 2026-01-12T21:37:01.156Z
Learnt from: FloPinguin
Repo: openfrontio/OpenFrontIO PR: 2874
File: src/server/MapLandTiles.ts:7-11
Timestamp: 2026-01-12T21:37:01.156Z
Learning: In this repository's OpenFrontIO deployment, inter-service HTTP calls to the master should target http://localhost:3000 (master at port 3000) as the canonical address. Apply this as the standard for all server-side TypeScript code that communicates with the master. Avoid hardcoding non-master URLs; centralize the master address (e.g., via config or env) when possible, and ensure internal service communication uses localhost:3000 in this architecture.

Applied to files:

  • src/core/configuration/MapLandTiles.ts
  • src/core/configuration/DefaultConfig.ts
📚 Learning: 2025-10-08T17:14:49.369Z
Learnt from: Foorack
Repo: openfrontio/OpenFrontIO PR: 2141
File: src/client/ClientGameRunner.ts:228-234
Timestamp: 2025-10-08T17:14:49.369Z
Learning: For the window close confirmation feature in `ClientGameRunner.ts`, the troop count requirement (>10,000 troops) from issue #2137 was intentionally removed because it was arbitrary and troop count can be reported as low despite having significant land. The confirmation now triggers for any alive player regardless of troop count.

Applied to files:

  • src/core/configuration/MapLandTiles.ts
  • src/core/configuration/DefaultConfig.ts
📚 Learning: 2025-12-26T22:21:21.904Z
Learnt from: FloPinguin
Repo: openfrontio/OpenFrontIO PR: 2689
File: src/client/PublicLobby.ts:245-245
Timestamp: 2025-12-26T22:21:21.904Z
Learning: In public lobbies with HumansVsNations mode in src/client/PublicLobby.ts, maxPlayers represents only human player slots (already halved in DefaultConfig.ts). The nation NPCs are added automatically server-side and don't count toward maxPlayers. Therefore, getTeamSize correctly returns maxPlayers directly for HumansVsNations to display the proper team size (e.g., maxPlayers=5 yields "5 Humans vs 5 Nations").

Applied to files:

  • src/core/configuration/DefaultConfig.ts
📚 Learning: 2025-10-21T20:06:04.823Z
Learnt from: Saphereye
Repo: openfrontio/OpenFrontIO PR: 2233
File: src/client/HostLobbyModal.ts:891-891
Timestamp: 2025-10-21T20:06:04.823Z
Learning: For the HumansVsNations game mode in `src/client/HostLobbyModal.ts` and related files, the implementation strategy is to generate all nations and adjust their strength for balancing, rather than limiting lobby size based on the number of available nations on the map.

Applied to files:

  • src/core/configuration/DefaultConfig.ts
📚 Learning: 2025-10-26T15:37:07.732Z
Learnt from: GlacialDrift
Repo: openfrontio/OpenFrontIO PR: 2298
File: src/client/graphics/layers/TerritoryLayer.ts:200-210
Timestamp: 2025-10-26T15:37:07.732Z
Learning: In GameImpl.ts lines 124-139, team assignment logic varies by number of teams: when numPlayerTeams < 8, teams are assigned ColoredTeams values (Red, Blue, Yellow, Green, Purple, Orange, Teal); when numPlayerTeams >= 8, teams are assigned generic string identifiers like "Team 1", "Team 2", etc., which are not members of ColoredTeams.

Applied to files:

  • src/core/configuration/DefaultConfig.ts
📚 Learning: 2025-11-26T20:49:29.140Z
Learnt from: scamiv
Repo: openfrontio/OpenFrontIO PR: 2519
File: src/core/game/GameView.ts:516-525
Timestamp: 2025-11-26T20:49:29.140Z
Learning: In GameView.ts, when usesSharedTileState is true (SAB mode), packedTileUpdates contains unpacked tile references as BigInt(tileRef) only, because all tile state lives in the shared Uint16Array. In non-SAB mode, packedTileUpdates contains packed TileUpdate bigints in the format (tileRef << 16n | state), which must be decoded via updateTile(tu). Therefore, Number(tu) is correct in SAB mode and shifting right by 16 bits would be wrong.

Applied to files:

  • src/core/configuration/DefaultConfig.ts
📚 Learning: 2025-11-12T23:11:34.445Z
Learnt from: MaxHT0x
Repo: openfrontio/OpenFrontIO PR: 2262
File: src/core/configuration/DefaultConfig.ts:806-832
Timestamp: 2025-11-12T23:11:34.445Z
Learning: In src/core/configuration/DefaultConfig.ts, JSDoc documentation for configuration methods should not be added inline, as it was requested that documentation be placed elsewhere in the codebase.

Applied to files:

  • src/core/configuration/DefaultConfig.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/core/configuration/DefaultConfig.ts
📚 Learning: 2025-10-20T11:02:16.969Z
Learnt from: sambokai
Repo: openfrontio/OpenFrontIO PR: 2225
File: src/core/execution/FakeHumanExecution.ts:57-57
Timestamp: 2025-10-20T11:02:16.969Z
Learning: In src/core/execution/FakeHumanExecution.ts, the correct MIRV victory denial thresholds are VICTORY_DENIAL_TEAM_THRESHOLD = 0.8 (80% for team games) and VICTORY_DENIAL_INDIVIDUAL_THRESHOLD = 0.65 (65% for individual players), not 0.85 and 0.7 as might be mentioned in some documentation.

Applied to files:

  • src/core/configuration/DefaultConfig.ts
📚 Learning: 2025-12-13T14:58:29.645Z
Learnt from: scamiv
Repo: openfrontio/OpenFrontIO PR: 2607
File: src/core/execution/PlayerExecution.ts:271-295
Timestamp: 2025-12-13T14:58:29.645Z
Learning: In src/core/execution/PlayerExecution.ts surroundedBySamePlayer(), the `as Player` cast on `mg.playerBySmallID(scan.enemyId)` is intentional. Since scan.enemyId comes from ownerID() on an owned tile and playerBySmallID() only returns Player or undefined, the cast expresses a known invariant. The maintainers prefer loud failures (runtime errors) over silent masking (early returns with guards) for corrupted game state scenarios at trusted call sites.

Applied to files:

  • src/core/configuration/DefaultConfig.ts
📚 Learning: 2025-06-22T05:48:19.241Z
Learnt from: scottanderson
Repo: openfrontio/OpenFrontIO PR: 786
File: src/client/TerritoryPatternsModal.ts:337-338
Timestamp: 2025-06-22T05:48:19.241Z
Learning: In src/client/TerritoryPatternsModal.ts, the bit shifting operators (<<) used in coordinate calculations with decoder.getScale() are intentional and should not be changed to multiplication. The user scottanderson confirmed this is functioning as intended.

Applied to files:

  • src/core/configuration/DefaultConfig.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/configuration/DefaultConfig.ts
📚 Learning: 2025-06-01T04:56:16.870Z
Learnt from: scottanderson
Repo: openfrontio/OpenFrontIO PR: 992
File: src/core/configuration/DefaultConfig.ts:0-0
Timestamp: 2025-06-01T04:56:16.870Z
Learning: In the OpenFrontIO codebase, the number of player teams (`numPlayerTeams`) will never be higher than the minimum team size, so the modulo operation in `lobbyMaxPlayers()` will not result in zero players.

Applied to files:

  • src/core/configuration/DefaultConfig.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/configuration/DefaultConfig.ts
📚 Learning: 2025-10-20T20:15:28.858Z
Learnt from: sambokai
Repo: openfrontio/OpenFrontIO PR: 2225
File: src/core/execution/FakeHumanExecution.ts:51-51
Timestamp: 2025-10-20T20:15:28.858Z
Learning: In src/core/execution/FakeHumanExecution.ts, game balance constants like MIRV_COOLDOWN_TICKS, MIRV_HESITATION_ODDS, VICTORY_DENIAL_TEAM_THRESHOLD, VICTORY_DENIAL_INDIVIDUAL_THRESHOLD, and STEAMROLL_CITY_GAP_MULTIPLIER are experimental tuning parameters subject to frequent change during balance testing. Do not flag changes to these values as issues or compare them against previous values.

Applied to files:

  • src/core/configuration/DefaultConfig.ts
🧬 Code graph analysis (2)
src/core/configuration/MapLandTiles.ts (2)
src/core/game/GameMapLoader.ts (1)
  • GameMapLoader (4-6)
src/core/game/FetchGameMapLoader.ts (1)
  • FetchGameMapLoader (4-73)
src/core/configuration/DefaultConfig.ts (1)
src/core/configuration/MapLandTiles.ts (1)
  • getMapLandTiles (15-25)
🔇 Additional comments (3)
src/core/configuration/MapLandTiles.ts (1)

7-11: LGTM on loader singleton and localhost URL.

The singleton pattern avoids repeated instantiation. Based on learnings, http://localhost:3000/maps is the correct canonical address for inter-service calls to the master in this architecture.

src/core/configuration/DefaultConfig.ts (2)

131-166: LGTM on async lobbyMaxPlayers implementation.

The logic is clear:

  • Fetch land tiles once, calculate tiers, apply mode/team adjustments.
  • The random distribution (30%/30%/40%) adds variety to lobby sizes.
  • Team mode 1.5x multiplier and compact map 25% reduction are well-bounded.

175-187: Consider edge case for very small maps.

When limitedBase is 5 (the minimum):

  • 75% of 5 → roundToNearest5(3.75) = 5
  • 50% of 5 → roundToNearest5(2.5) = 5

All three tiers become identical (5). This is probably fine since tiny maps should have uniform small player counts, but worth confirming this is the intended behavior.

@evanpelle evanpelle added this to the v29 milestone Jan 12, 2026
return 60 * 1000;
}

lobbyMaxPlayers(
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

could we move this logic from Config to MapPlaylist? Since i think that's the only place it's used anyways

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok did that 👍

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should this be moved to server/ because that's the only place it's used?

Copy link
Contributor Author

@FloPinguin FloPinguin Jan 13, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did that 👍
Previously it was a problem because in DefaultConfig we would have imported server stuff, while DefaultConfig is core.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Nitpick comments (4)
src/server/MapLandTiles.ts (2)

15-24: Consider caching the manifest result to avoid repeated fetches.

FetchGameMapLoader.getMapData() caches the MapData object, but calling manifest() creates a new HTTP request each time. In MapPlaylist.ts, getMapLandTiles is called twice per gameConfig() invocation (once in supportsCompactMapForTeams, once in lobbyMaxPlayers), resulting in duplicate network calls for the same map.

♻️ Proposed fix: add manifest result caching
+const manifestCache = new Map<GameMapType, number>();
+
 export async function getMapLandTiles(map: GameMapType): Promise<number> {
+  const cached = manifestCache.get(map);
+  if (cached !== undefined) {
+    return cached;
+  }
+
   try {
     const loader = getMapLoader();
     const mapData = loader.getMapData(map);
     const manifest = await mapData.manifest();
-    return manifest.map.num_land_tiles;
+    const landTiles = manifest.map.num_land_tiles;
+    manifestCache.set(map, landTiles);
+    return landTiles;
   } catch (error) {
     console.error(`Failed to load manifest for ${map}:`, error);
     return 1_000_000; // Default fallback
   }
 }

21-23: Fallback value silently hides failures.

Returning 1,000,000 on error produces ~50 players (reasonable), but console.error may get lost in logs. Consider using the structured logger from ./Logger for consistency with the rest of the server code.

♻️ Proposed fix
+import { logger } from "./Logger";
+
+const log = logger.child({});
+
 // ...
   } catch (error) {
-    console.error(`Failed to load manifest for ${map}:`, error);
+    log.error({ error, map }, `Failed to load manifest for map`);
     return 1_000_000; // Default fallback
   }
src/server/MapPlaylist.ts (2)

169-175: Duplicate async calls to getMapLandTiles for the same map.

Both supportsCompactMapForTeams (line 172) and lobbyMaxPlayers (line 183) fetch land tiles independently. If manifest caching isn't added in MapLandTiles.ts, consider passing the land tiles as a parameter:

♻️ Alternative: pass landTiles to avoid duplicate fetches
-  private async supportsCompactMapForTeams(map: GameMapType): Promise<boolean> {
-    const landTiles = await getMapLandTiles(map);
+  private supportsCompactMapForTeams(landTiles: number): boolean {
     const [, , smallest] = this.calculateMapPlayerCounts(landTiles);
     return smallest >= 50;
   }

   private async lobbyMaxPlayers(
     map: GameMapType,
     mode: GameMode,
     numPlayerTeams: TeamCountConfig | undefined,
     isCompactMap?: boolean,
   ): Promise<number> {
     const landTiles = await getMapLandTiles(map);
+    // Pass landTiles to supportsCompactMapForTeams if needed upstream
     // ...
   }

Then in gameConfig():

+    const landTiles = await getMapLandTiles(map);
+
     if (
       mode === GameMode.Team &&
-      !(await this.supportsCompactMapForTeams(map))
+      !this.supportsCompactMapForTeams(landTiles)
     ) {
       isCompact = false;
     }

Also applies to: 177-184


226-227: Consider extracting magic numbers as named constants.

Values like 1_000_000, 50, 125, and 5 define the player count formula. Naming them improves readability:

♻️ Proposed constants
const TILES_PER_PLAYER_UNIT = 1_000_000;
const PLAYERS_PER_UNIT = 50;
const MAX_PLAYERS = 125;
const MIN_PLAYERS = 5;

// In calculateMapPlayerCounts:
const base = roundToNearest5((landTiles / TILES_PER_PLAYER_UNIT) * PLAYERS_PER_UNIT);
const limitedBase = Math.min(Math.max(base, MIN_PLAYERS), MAX_PLAYERS);
📜 Review details

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between a9cf666 and 3145a33.

📒 Files selected for processing (4)
  • src/core/configuration/Config.ts
  • src/core/configuration/DefaultConfig.ts
  • src/server/MapLandTiles.ts
  • src/server/MapPlaylist.ts
💤 Files with no reviewable changes (2)
  • src/core/configuration/Config.ts
  • src/core/configuration/DefaultConfig.ts
🧰 Additional context used
🧠 Learnings (11)
📓 Common learnings
Learnt from: Foorack
Repo: openfrontio/OpenFrontIO PR: 2141
File: src/client/ClientGameRunner.ts:228-234
Timestamp: 2025-10-08T17:14:49.369Z
Learning: For the window close confirmation feature in `ClientGameRunner.ts`, the troop count requirement (>10,000 troops) from issue #2137 was intentionally removed because it was arbitrary and troop count can be reported as low despite having significant land. The confirmation now triggers for any alive player regardless of troop count.
Learnt from: FloPinguin
Repo: openfrontio/OpenFrontIO PR: 2689
File: src/client/PublicLobby.ts:245-245
Timestamp: 2025-12-26T22:21:21.904Z
Learning: In public lobbies with HumansVsNations mode in src/client/PublicLobby.ts, maxPlayers represents only human player slots (already halved in DefaultConfig.ts). The nation NPCs are added automatically server-side and don't count toward maxPlayers. Therefore, getTeamSize correctly returns maxPlayers directly for HumansVsNations to display the proper team size (e.g., maxPlayers=5 yields "5 Humans vs 5 Nations").
📚 Learning: 2025-11-26T20:49:29.140Z
Learnt from: scamiv
Repo: openfrontio/OpenFrontIO PR: 2519
File: src/core/game/GameView.ts:516-525
Timestamp: 2025-11-26T20:49:29.140Z
Learning: In GameView.ts, when usesSharedTileState is true (SAB mode), packedTileUpdates contains unpacked tile references as BigInt(tileRef) only, because all tile state lives in the shared Uint16Array. In non-SAB mode, packedTileUpdates contains packed TileUpdate bigints in the format (tileRef << 16n | state), which must be decoded via updateTile(tu). Therefore, Number(tu) is correct in SAB mode and shifting right by 16 bits would be wrong.

Applied to files:

  • src/server/MapLandTiles.ts
📚 Learning: 2025-07-12T06:35:10.457Z
Learnt from: Aotumuri
Repo: openfrontio/OpenFrontIO PR: 1357
File: resources/lang/ja.json:0-0
Timestamp: 2025-07-12T06:35:10.457Z
Learning: In OpenFrontIO project, "giantworldmap" is the correct localization key name for the giant world map, used consistently across all language files and TypeScript code. Do not suggest renaming this key.

Applied to files:

  • src/server/MapLandTiles.ts
📚 Learning: 2026-01-12T21:37:01.156Z
Learnt from: FloPinguin
Repo: openfrontio/OpenFrontIO PR: 2874
File: src/server/MapLandTiles.ts:7-11
Timestamp: 2026-01-12T21:37:01.156Z
Learning: In this repository's OpenFrontIO deployment, inter-service HTTP calls to the master should target http://localhost:3000 (master at port 3000) as the canonical address. Apply this as the standard for all server-side TypeScript code that communicates with the master. Avoid hardcoding non-master URLs; centralize the master address (e.g., via config or env) when possible, and ensure internal service communication uses localhost:3000 in this architecture.

Applied to files:

  • src/server/MapLandTiles.ts
  • src/server/MapPlaylist.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/server/MapLandTiles.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/server/MapPlaylist.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/MapPlaylist.ts
📚 Learning: 2025-12-26T22:21:21.904Z
Learnt from: FloPinguin
Repo: openfrontio/OpenFrontIO PR: 2689
File: src/client/PublicLobby.ts:245-245
Timestamp: 2025-12-26T22:21:21.904Z
Learning: In public lobbies with HumansVsNations mode in src/client/PublicLobby.ts, maxPlayers represents only human player slots (already halved in DefaultConfig.ts). The nation NPCs are added automatically server-side and don't count toward maxPlayers. Therefore, getTeamSize correctly returns maxPlayers directly for HumansVsNations to display the proper team size (e.g., maxPlayers=5 yields "5 Humans vs 5 Nations").

Applied to files:

  • src/server/MapPlaylist.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/server/MapPlaylist.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/server/MapPlaylist.ts
📚 Learning: 2025-06-01T04:56:16.870Z
Learnt from: scottanderson
Repo: openfrontio/OpenFrontIO PR: 992
File: src/core/configuration/DefaultConfig.ts:0-0
Timestamp: 2025-06-01T04:56:16.870Z
Learning: In the OpenFrontIO codebase, the number of player teams (`numPlayerTeams`) will never be higher than the minimum team size, so the modulo operation in `lobbyMaxPlayers()` will not result in zero players.

Applied to files:

  • src/server/MapPlaylist.ts
🧬 Code graph analysis (2)
src/server/MapLandTiles.ts (2)
src/core/game/GameMapLoader.ts (1)
  • GameMapLoader (4-6)
src/core/game/FetchGameMapLoader.ts (1)
  • FetchGameMapLoader (4-73)
src/server/MapPlaylist.ts (4)
src/core/Schemas.ts (2)
  • GameConfig (90-90)
  • TeamCountConfig (164-164)
src/core/configuration/DefaultConfig.ts (1)
  • playerTeams (212-214)
src/core/game/Game.ts (5)
  • PublicGameModifiers (206-209)
  • Duos (55-55)
  • Trios (56-56)
  • Quads (57-57)
  • HumansVsNations (58-58)
src/server/MapLandTiles.ts (1)
  • getMapLandTiles (15-25)
🔇 Additional comments (7)
src/server/MapLandTiles.ts (1)

1-11: Singleton pattern and imports look good.

Clean setup using nullish coalescing assignment for lazy initialization. The hardcoded http://localhost:3000/maps aligns with the project's architecture for inter-service calls to master. Based on learnings, this is the canonical address.

src/server/MapPlaylist.ts (6)

90-141: Async gameConfig looks well-structured.

The flow is clear: get map, determine team count, apply modifiers, check compact support, then build config. Good use of satisfies GameConfig for type safety.


107-114: Verify compact map logic with the awaited check.

The comment on lines 107-108 explains the intent well. The async check supportsCompactMapForTeams(map) correctly guards compact mode for team games.


158-167: Simple helpers, clean implementation.

getTeamCount and getRandomPublicGameModifiers are straightforward. The 10%/5% probabilities for random spawn and compact mode are reasonable.


185-191: Random tier selection and compact reduction look correct.

The weighted distribution (30% large, 30% medium, 40% small) and 75% reduction for compact maps (keeping 25%) are clear. Math.max(3, ...) ensures at least 3 players for compact games.


193-211: Team divisibility adjustments are correct.

Each team type correctly adjusts player count to be divisible by the team size. The HumansVsNations case halving the count aligns with the retrieved learning that nation NPCs are added server-side.


221-233: Edge case: 75% and 50% tiers could round to same value for small maps.

When limitedBase is 5 (the minimum):

  • 75% → 5 * 0.75 = 3.75 → rounds to 5
  • 50% → 5 * 0.5 = 2.5 → rounds to 5

All three tiers would be [5, 5, 5]. This is likely fine since small maps need consistent sizing, but worth noting.

Is this expected behavior for very small maps, or should the lower tiers have explicit minimums (e.g., 75% tier min 4, 50% tier min 3)?

Copy link
Collaborator

@evanpelle evanpelle left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks!

@evanpelle evanpelle merged commit 464a4a8 into openfrontio:main Jan 13, 2026
7 checks passed
ryanbarlow97 pushed a commit that referenced this pull request Jan 13, 2026
…d tiles 🔧 (#2874)

## Description:

The calculation is based on: 50 players per 1_000_000 land tiles,
limited at 125 players because of performance
Second number is 75% of that, third one 50%
That way, the player counts are staying mostly the same
Look at the "Dynamic Config" column, these are the new player counts:
(The 125 players limit is missing in that column, only relevant for the
twolakes map)

<img width="930" height="1033" alt="Screenshot_2026-01-12_152758"
src="https://github.com/user-attachments/assets/e1791740-e263-47b3-8b27-4f9aa358d381"
/>
<img width="926" height="324" alt="Screenshot_2026-01-12_152814"
src="https://github.com/user-attachments/assets/78d6789b-374f-4f8b-b50f-f6f08395572b"
/>

This PR also removes `MapDescription` from `Maps.ts` because its unused.
And this PR updates the map-generator `README.md` to reflect the changes

## Please complete the following:

- [X] I have added screenshots for all UI updates
- [X] I process any text displayed to the user through translateText()
and I've added it to the en.json file
- [X] I have added relevant tests to the test directory
- [X] I confirm I have thoroughly tested these changes and take full
responsibility for any bugs introduced

## Please put your Discord username so you can be contacted if a bug or
regression is found:

FloPinguin
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants