Skip to content

Remove lakes from the game 🌊#4214

Merged
evanpelle merged 1 commit into
mainfrom
remove-islake-and-terrain-lake
Jun 10, 2026
Merged

Remove lakes from the game 🌊#4214
evanpelle merged 1 commit into
mainfrom
remove-islake-and-terrain-lake

Conversation

@FloPinguin

@FloPinguin FloPinguin commented Jun 10, 2026

Copy link
Copy Markdown
Contributor

Description:

Nametags look weird here because on the left is a lake:

Screenshot 2026-06-10 170116

I removed isLake from the nametag position calculation

Because isLake was unused then, I removed it completely.

Full changelog:

  • Remove isLake() from GameMap interface, GameMapImpl, GameImpl, and GameView
  • Remove TerrainType.Lake enum value
  • terrainType() now returns Ocean for all water tiles (previously distinguished lake vs ocean, but nothing treated them differently)
  • Remove Lake case from PastelTheme and PastelThemeDark (already fell through to Ocean)
  • Exclude lakes from nametag placement grid in NameBoxCalculator

Maybe as a next step also remove lakes metadata from the map generator?

AI Model used: MiMo 2.5 Pro

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

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

FloPinguin

@FloPinguin FloPinguin added this to the v32 milestone Jun 10, 2026
@FloPinguin FloPinguin self-assigned this Jun 10, 2026
@FloPinguin FloPinguin requested a review from a team as a code owner June 10, 2026 18:42
@FloPinguin FloPinguin added the UI/UX UI/UX changes including assets, menus, QoL, etc. label Jun 10, 2026
@coderabbitai

coderabbitai Bot commented Jun 10, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

Walkthrough

This PR removes the TerrainType.Lake enum variant and consolidates all non-land water classification into TerrainType.Ocean. The GameMap interface removes the isLake() method; the GameMapImpl now returns Ocean for all non-land tiles. All client and game facades are updated to remove isLake() delegation, and rendering logic is adjusted to handle the removed terrain type.

Changes

Lake terrain type removal

Layer / File(s) Summary
Core terrain type and map contract
src/core/game/Game.ts, src/core/game/GameMap.ts
TerrainType enum removes Lake variant. GameMap interface removes isLake() method and maintains isWater() as the non-land predicate. GameMapImpl.terrainType() now returns TerrainType.Ocean unconditionally for all non-land tiles, and isWater() is simplified to !isLand(ref).
Game facade delegation cleanup
src/core/game/GameImpl.ts, src/client/view/GameView.ts
GameImpl retains isWater() delegation to the map and removes isLake(). GameView removes its isLake(ref) forwarding method.
Client rendering and grid updates
src/client/theme/PastelTheme.ts, src/client/theme/PastelThemeDark.ts, src/client/hud/NameBoxCalculator.ts
Theme switch cases separate Ocean handling from the removed Lake case. NameBoxCalculator removes the game.isLake(tile) condition from its grid blocked-cell check.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Suggested labels

approved

Suggested reviewers

  • evanpelle

Poem

Water once split, now flows as one,
Lake and Ocean, their duality done.
Unified terrain, simpler to see—
All non-land water flows to the sea. 🌊

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly summarizes the main change: removing the lake terrain type from the game.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Description check ✅ Passed The pull request description clearly explains the problem (nametags positioned incorrectly on lakes) and lists all changes made, directly relating to the changeset.

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


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.

@evanpelle evanpelle left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

thanks. proably in the future we should rename ocean => water.

@github-project-automation github-project-automation Bot moved this from Triage to Final Review in OpenFront Release Management Jun 10, 2026
@evanpelle evanpelle merged commit 3aaf0ea into main Jun 10, 2026
21 checks passed
@evanpelle evanpelle deleted the remove-islake-and-terrain-lake branch June 10, 2026 20:20
@github-project-automation github-project-automation Bot moved this from Final Review to Complete in OpenFront Release Management Jun 10, 2026
noahschmal added a commit to noahschmal/OpenFrontIO that referenced this pull request Jun 10, 2026
Upstream openfrontio#4214 removed TerrainType.Lake (terrainType() now returns Ocean
for all water). ColorblindTheme.terrainColor still had a `case
TerrainType.Lake`, which no longer compiles — fold it into the Ocean
case, matching the PastelTheme change.
noahschmal added a commit to noahschmal/OpenFrontIO that referenced this pull request Jun 11, 2026
Upstream openfrontio#4214 removed TerrainType.Lake (terrainType() now returns Ocean
for all water). ColorblindTheme.terrainColor still had a `case
TerrainType.Lake`, which no longer compiles — fold it into the Ocean
case, matching the PastelTheme change.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

UI/UX UI/UX changes including assets, menus, QoL, etc.

Projects

Status: Complete

Development

Successfully merging this pull request may close these issues.

2 participants