feat: land mines #2806
feat: land mines #2806
Conversation
|
|
WalkthroughAdds a new Land Mine unit type across the game, including keybinds, UI components, graphics rendering, construction mechanics, and detonation behavior when captured by enemies. Also implements client-side visibility filtering in GameRunner to hide units with restricted visibility from non-owners. Changes
Sequence DiagramssequenceDiagram
participant Player as Player / Client
participant GameRunner as GameRunner
participant PlayerExec as PlayerExecution
participant LandMineExec as LandMineExecution
participant Config as Config
Player->>GameRunner: Place Land Mine (construction)
Note over GameRunner: Tick 1: Mine under construction
GameRunner->>GameRunner: tick()
Player->>GameRunner: Enemy captures mine tile (Tick N)
GameRunner->>PlayerExec: executeNextTick()
PlayerExec->>PlayerExec: Check mine is under construction?
alt Under Construction
PlayerExec->>GameRunner: Delete mine
Note over GameRunner: Mine deleted, no detonation
else Completed
PlayerExec->>LandMineExec: Mine activated
LandMineExec->>LandMineExec: tile captured by enemy?
alt Enemy Capture
LandMineExec->>Config: Get nuke magnitudes
Config-->>LandMineExec: inner: 6, outer: 15
LandMineExec->>LandMineExec: Calculate blast radius
LandMineExec->>LandMineExec: Remove enemy troops in blast
LandMineExec->>LandMineExec: Apply fallout on land
LandMineExec->>LandMineExec: Damage enemy outgoing attacks
LandMineExec->>GameRunner: Display messages
LandMineExec->>GameRunner: Delete mine, deactivate
Note over GameRunner: Mine detonates
else Ally Capture
LandMineExec->>LandMineExec: Transfer ownership to ally
Note over GameRunner: Mine transfers, no detonation
end
end
GameRunner->>GameRunner: filterHiddenUnits()
Note over GameRunner: Remove mine updates from enemy clients
sequenceDiagram
participant ServerTick as Server tick()
participant GameRunner as GameRunner
participant Updates as GameUpdates
participant ClientA as Client (Ally)
participant ClientE as Client (Enemy)
ServerTick->>GameRunner: executeNextTick()
Note over GameRunner: Compute all unit updates
GameRunner->>Updates: Add Land Mine update<br/>(visibleToEnemies: false)
GameRunner->>GameRunner: filterHiddenUnits(updates)
GameRunner->>GameRunner: For each update:<br/>Check owner & visibility
alt Mine owner or allied
GameRunner->>ClientA: Send mine update
Note over ClientA: Ally sees mine state
else Enemy or spectator
GameRunner->>GameRunner: Remove from updates
Note over ClientE: Enemy does not see mine
end
GameRunner->>ClientA: Broadcast updates
GameRunner->>ClientE: Broadcast updates (filtered)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
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. 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: 14
🤖 Fix all issues with AI agents
In @resources/lang/ar.json:
- Around line 205-206: Remove the Arabic translation entry for the key
"land_mine" from this PR (i.e., delete the "land_mine": "لغم أرضي" line) so
non-English translation files remain unchanged; leave other keys like "mirv"
untouched and ensure future addition of the Arabic "land_mine" translation is
done in a dedicated "mls" translation PR authored by Aotumori against the
translations workflow.
In @resources/lang/bg.json:
- Around line 327-328: Remove the non-English translation entry "land_mine" from
resources/lang/bg.json (and any other non-en locale files touched in this PR
such as the entries at the other noted locations), leaving only existing
legitimate keys like "factory"; ensure that new translation keys introduced in
this PR exist only in resources/lang/en.json and revert any additions of those
keys from other locale files so that only en.json contains the new keys in this
PR.
In @resources/lang/eo.json:
- Around line 278-279: The PR incorrectly modifies non-English locale keys
"factory" and "land_mine" in resources/lang/eo.json; revert those changes so
only resources/lang/en.json is modified in this feature PR by removing the
added/changed Esperanto entries for "factory" and "land_mine" from the diff
(restore the file to its pre-PR state) and leave translation updates to the MLS
workflow handled by Aotumori.
In @resources/lang/gl.json:
- Around line 261-262: Remove the non-English key(s) from this locale file:
delete the "land_mine" entry (and any other newly added non-en keys) from
resources/lang/gl.json so this PR only changes en.json; ensure the corresponding
new translation key(s) remain added to resources/lang/en.json (e.g.,
"land_mine") and leave other locale files untouched for the dedicated
translation PR by Aotumori.
In @resources/lang/he.json:
- Around line 250-251: Remove the Hebrew translations added in this PR by
reverting the changes to the keys "factory" and "land_mine" in
resources/lang/he.json so that non-English locale files are untouched; only keep
translation edits in resources/lang/en.json and leave Aotumori/MLS team to add
Hebrew entries in a dedicated "mls" PR.
In @resources/lang/ko.json:
- Around line 254-255: Remove the Korean translations added in this PR by
reverting the changes that add the "factory" and "land_mine" keys to the ko
locale (undo the edits that set "factory": "공장" and "land_mine": "지뢰"); ensure
only resources/lang/en.json contains new/changed keys in this PR and leave
non-English files untouched so the translation team can add them in their MLS
PR.
In @resources/lang/nl.json:
- Around line 327-328: Remove the Dutch translations for the land mine keys from
this PR: delete the entries corresponding to unit_type.land_mine,
user_setting.build_land_mine_desc, and build_menu.desc.land_mine (and any
duplicate lines 372-373, 523-524) from resources/lang/nl.json so only the
English changes remain in this feature PR; those Dutch translations should be
added later in a dedicated "mls" translation PR by Aotumori.
In @resources/lang/ru.json:
- Around line 327-328: Remove the Russian translations for the land_mine-related
keys from this PR: delete the entries for unit_type.land_mine,
user_setting.build_land_mine_desc, and build_menu.desc.land_mine so only the
English translation remains changed in this feature PR; revert those ru.json
additions (or remove the three keys) and commit the change so translations are
added later in a dedicated "mls" translation PR by Aotumori.
In @resources/lang/sl.json:
- Around line 278-279: This PR accidentally includes Slovenian translations for
the key unit_type.land_mine and build_menu.desc.land_mine; remove those two
"land_mine" entries from the Slovenian translations in this diff so only the
English source remains changed in this feature PR, and leave adding Slovenian
translations to a dedicated "mls" translation PR by Aotumori.
In @resources/lang/tp.json:
- Around line 205-206: The PR added a Toki Pona translation for the key
"land_mine" which should not be changed in this feature PR; remove the
"land_mine" entry from the Toki Pona locale (tp.json) in this branch so only
English translations are modified in this PR, leaving "mirv" untouched, and add
the Toki Pona translation later in a dedicated "mls" translation PR by Aotumori.
In @src/client/InputHandler.ts:
- Line 214: The "Minus" key is conflicting between buildLandMine and the zoomOut
fallback causing simultaneous ZoomEvent during build; update the buildLandMine
mapping from "Minus" to an unused key like "KeyM" (or
"BracketLeft"/"BracketRight") wherever the buildLandMine key is defined and
referenced, and ensure any checks that compare pressed keys (e.g., in activeKeys
handling, the build action trigger in the method that fires the land-mine
action, and any key-to-action maps) are updated to the new key constant so
holding the new key no longer triggers the zoom loop via the zoomOut fallback.
In @src/core/execution/LandMineExecution.ts:
- Around line 57-62: When an ally captures the mine tile the code only sets
this.originalOwner but doesn't transfer the unit, causing ownership mismatch;
update the handler in LandMineExecution (the block that checks
currentOwner.isFriendly(this.originalOwner)) to call
currentOwner.captureUnit(this.mine) before assigning this.originalOwner =
currentOwner so the Unit object ownership is transferred the same way Defense
Posts do with captureUnit and decreaseLevel.
In @tests/LandMine.test.ts:
- Line 314: There is a stray empty template literal (``) in
tests/LandMine.test.ts (the ``; expression on line 314) that should be removed;
locate the empty template literal token `` (and the trailing semicolon if
present) inside the test file and delete it so the file contains no no-op
template literal and ESLint no-empty-template-string error is resolved, then run
tests/lint to verify no syntax or formatting regressions.
- Line 116: The variable defenderInitialTiles assigned from
defender.numTilesOwned() is unused; either remove the assignment line "const
defenderInitialTiles = defender.numTilesOwned();" or use it in a meaningful
assertion (e.g., compare defenderInitialTiles to defender.numTilesOwned() after
the operation to verify tile changes). Update the test by deleting the unused
variable or by turning it into an assertion referencing defenderInitialTiles and
the appropriate post-operation value to validate expected behavior.
🧹 Nitpick comments (7)
resources/lang/pt-PT.json (1)
261-262: Non-English locale file should not be updated in this PR.Based on learnings, translation files in
resources/lang/*.json(excepten.json) should only be updated in dedicated translation PRs titled "mls" and made by Aotumori. Regular feature PRs should only updateen.jsonwhen adding translation keys.Consider removing these changes and letting the translation workflow handle non-English locales.
src/core/configuration/DefaultConfig.ts (1)
899-900: Consider renaming or documentingnukeMagnitudesfor clarity.The method
nukeMagnitudesnow handlesLandMine, which is not a nuke. While reusing this method works, the name is slightly misleading. A comment or renaming to something likeexplosionMagnitudescould help future maintainability.resources/lang/zh-CN.json (1)
327-328: Non-English locale file should not be updated in this PR.Based on learnings, translation files in
resources/lang/*.json(excepten.json) should only be updated in dedicated translation PRs titled "mls" and made by Aotumori. Regular feature PRs should only updateen.json.Consider removing these changes from zh-CN.json.
resources/lang/ja.json (1)
327-328: Non-English locale file should not be updated in this PR.Based on learnings, translation files in
resources/lang/*.json(excepten.json) should only be updated in dedicated translation PRs titled "mls" and made by Aotumori. Regular feature PRs should only updateen.json.Consider removing these changes from ja.json.
tests/LandMine.test.ts (1)
401-427: Test duplicates production filtering logic.The filter logic here (lines 408-426) mirrors the implementation in
GameRunner.filterHiddenUnits(). This works for validation, but if the production logic changes, this test may give false positives. Consider calling the actual filtering method or extracting shared logic to avoid drift.src/core/GameRunner.ts (1)
193-196: Use nullish coalescing assignment operator.ESLint suggests using
??=for cleaner syntax.Proposed fix
private filterHiddenUnits(updates: GameUpdates): void { - // Lazy-load our player reference - if (this.myPlayer === null) { - this.myPlayer = this.game.playerByClientID(this.clientID) ?? null; - } + // Lazy-load our player reference + this.myPlayer ??= this.game.playerByClientID(this.clientID) ?? null;src/core/execution/LandMineExecution.ts (1)
111-122: Damaging all outgoing attacks regardless of location.This applies damage to all of the attacker's outgoing attacks, even those far from the blast. While this creates interesting gameplay, it differs from the localized physical damage to tiles and units. If this is intentional game design, consider adding a comment explaining the rationale.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
resources/images/buildings/landMine1.pngis excluded by!**/*.png
📒 Files selected for processing (44)
resources/lang/ar.jsonresources/lang/bg.jsonresources/lang/cs.jsonresources/lang/da.jsonresources/lang/de.jsonresources/lang/el.jsonresources/lang/en.jsonresources/lang/eo.jsonresources/lang/fa.jsonresources/lang/fi.jsonresources/lang/fr.jsonresources/lang/gl.jsonresources/lang/he.jsonresources/lang/hu.jsonresources/lang/ja.jsonresources/lang/ko.jsonresources/lang/mk.jsonresources/lang/nl.jsonresources/lang/pl.jsonresources/lang/pt-PT.jsonresources/lang/ru.jsonresources/lang/sk.jsonresources/lang/sl.jsonresources/lang/sv-SE.jsonresources/lang/tp.jsonresources/lang/tr.jsonresources/lang/uk.jsonresources/lang/zh-CN.jsonsrc/client/InputHandler.tssrc/client/UserSettingModal.tssrc/client/graphics/layers/BuildMenu.tssrc/client/graphics/layers/StructureDrawingUtils.tssrc/client/graphics/layers/StructureIconsLayer.tssrc/client/graphics/layers/StructureLayer.tssrc/client/graphics/layers/UnitDisplay.tssrc/core/GameRunner.tssrc/core/configuration/DefaultConfig.tssrc/core/execution/ConstructionExecution.tssrc/core/execution/LandMineExecution.tssrc/core/execution/PlayerExecution.tssrc/core/execution/nation/structureSpawnTileValue.tssrc/core/game/Game.tssrc/core/game/PlayerImpl.tstests/LandMine.test.ts
🧰 Additional context used
🧠 Learnings (23)
📚 Learning: 2026-01-03T19:37:22.111Z
Learnt from: ryanbarlow97
Repo: openfrontio/OpenFrontIO PR: 2782
File: src/client/graphics/layers/StructureIconsLayer.ts:616-620
Timestamp: 2026-01-03T19:37:22.111Z
Learning: In `src/client/graphics/layers/StructureIconsLayer.ts`, the confirm button in `ensureGhostControls()` for locked ghost nukes (AtomBomb/HydrogenBomb) intentionally does NOT call `removeGhostStructure()` after emitting the build intent. This allows mobile players to rapidly place multiple nukes by "spamming" the confirm button without needing to reopen the radial menu.
Applied to files:
src/client/graphics/layers/StructureDrawingUtils.tssrc/client/InputHandler.tssrc/client/graphics/layers/StructureLayer.tssrc/client/graphics/layers/BuildMenu.ts
📚 Learning: 2025-05-18T23:36:12.847Z
Learnt from: scottanderson
Repo: openfrontio/OpenFrontIO PR: 784
File: src/core/game/StatsImpl.ts:143-159
Timestamp: 2025-05-18T23:36:12.847Z
Learning: In this codebase, NukeType is a union type derived from UnitType values (specifically bomb-related values like AtomBomb, HydrogenBomb, MIRV, and MIRVWarhead) rather than a separate enum. This means comparing NukeType values against UnitType values in switch statements is valid and intentional.
Applied to files:
src/client/graphics/layers/StructureDrawingUtils.tssrc/core/execution/ConstructionExecution.tssrc/core/game/PlayerImpl.tssrc/core/game/Game.tssrc/core/configuration/DefaultConfig.ts
📚 Learning: 2025-05-18T23:36:12.847Z
Learnt from: scottanderson
Repo: openfrontio/OpenFrontIO PR: 784
File: src/core/game/StatsImpl.ts:143-159
Timestamp: 2025-05-18T23:36:12.847Z
Learning: In this codebase, NukeType is a union type derived from specific UnitType enum values (AtomBomb, HydrogenBomb, MIRV, MIRVWarhead). This means that comparisons in switch statements between a NukeType parameter and UnitType enum values are valid and will work correctly at runtime.
Applied to files:
src/client/graphics/layers/StructureDrawingUtils.tssrc/core/execution/ConstructionExecution.tssrc/core/game/PlayerImpl.tssrc/core/game/Game.tssrc/core/configuration/DefaultConfig.ts
📚 Learning: 2025-08-16T10:52:08.292Z
Learnt from: TheGiraffe3
Repo: openfrontio/OpenFrontIO PR: 884
File: resources/lang/en.json:456-461
Timestamp: 2025-08-16T10:52:08.292Z
Learning: In OpenFrontIO, translation files in resources/lang/*.json (except en.json) should not be updated in regular PRs. Only dedicated translation PRs titled "mls" and made by Aotumori should update non-English locale files. Regular PRs should only update en.json when adding or modifying translation keys.
Applied to files:
resources/lang/ko.jsonresources/lang/sl.jsonresources/lang/gl.jsonresources/lang/tp.jsonresources/lang/eo.jsonresources/lang/ja.json
📚 Learning: 2025-06-02T14:27:37.609Z
Learnt from: andrewNiziolek
Repo: openfrontio/OpenFrontIO PR: 1007
File: resources/lang/de.json:115-115
Timestamp: 2025-06-02T14:27:37.609Z
Learning: For OpenFrontIO project: When localization keys are renamed in language JSON files, the maintainers separate technical changes from translation content updates. They wait for community translators to update the actual translation values rather than attempting to translate in the same PR. This allows technical changes to proceed while ensuring accurate translations from native speakers.
Applied to files:
resources/lang/ko.jsonresources/lang/eo.json
📚 Learning: 2026-01-03T19:37:22.111Z
Learnt from: ryanbarlow97
Repo: openfrontio/OpenFrontIO PR: 2782
File: src/client/graphics/layers/StructureIconsLayer.ts:616-620
Timestamp: 2026-01-03T19:37:22.111Z
Learning: In StructureIconsLayer.ts, within ensureGhostControls(), when handling locked ghost nukes (AtomBomb/HydrogenBomb), do not call removeGhostStructure() after emitting the build intent. This enables rapid repeated placement by mobile players. Reviewers should verify that the confirm button path intentionally bypasses ghost removal in this scenario and that this design aligns with UX goals and does not introduce inconsistent state.
Applied to files:
src/client/graphics/layers/StructureIconsLayer.ts
📚 Learning: 2025-06-22T21:51:14.990Z
Learnt from: devalnor
Repo: openfrontio/OpenFrontIO PR: 1248
File: src/client/graphics/layers/TerritoryInfoLayer.ts:20-20
Timestamp: 2025-06-22T21:51:14.990Z
Learning: In TerritoryInfoLayer.ts, the highlightedTerritory field uses both null and undefined intentionally: undefined represents initial state or inactive layer (Ctrl released), while null represents active layer with no territory being highlighted at cursor position. This distinction is important for proper state change detection.
Applied to files:
src/client/graphics/layers/StructureLayer.ts
📚 Learning: 2025-10-18T11:00:57.142Z
Learnt from: NewYearNewPhil
Repo: openfrontio/OpenFrontIO PR: 2230
File: src/client/graphics/GameRenderer.ts:269-277
Timestamp: 2025-10-18T11:00:57.142Z
Learning: In src/client/graphics/GameRenderer.ts, the GameRecapCapture implementation does not use setCaptureRenderEnabled on layers. Instead, it uses RecapCaptureSurface.capture() to render capture layers (TerrainLayer, TerritoryLayer, RailroadLayer, StructureIconsLayer, UnitLayer) directly to an off-screen canvas without requiring layer-level capture mode methods.
Applied to files:
src/client/graphics/layers/StructureLayer.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:
tests/LandMine.test.tssrc/core/execution/ConstructionExecution.tssrc/core/execution/nation/structureSpawnTileValue.tssrc/client/graphics/layers/UnitDisplay.tssrc/core/execution/PlayerExecution.tssrc/core/game/Game.tssrc/core/execution/LandMineExecution.ts
📚 Learning: 2025-10-18T17:54:01.311Z
Learnt from: sambokai
Repo: openfrontio/OpenFrontIO PR: 2225
File: src/core/execution/FakeHumanExecution.ts:172-173
Timestamp: 2025-10-18T17:54:01.311Z
Learning: In src/core/execution/FakeHumanExecution.ts, MIRVs and ground attacks should not be mutually exclusive. The considerMIRV() method should not short-circuit maybeAttack() - both actions can occur in the same tick.
Applied to files:
tests/LandMine.test.tssrc/core/execution/ConstructionExecution.tssrc/core/execution/PlayerExecution.tssrc/core/execution/LandMineExecution.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:
tests/LandMine.test.tssrc/core/execution/nation/structureSpawnTileValue.tssrc/core/GameRunner.tssrc/core/execution/PlayerExecution.tssrc/core/game/PlayerImpl.tssrc/core/execution/LandMineExecution.ts
📚 Learning: 2025-10-27T09:47:26.395Z
Learnt from: sambokai
Repo: openfrontio/OpenFrontIO PR: 2225
File: src/core/execution/FakeHumanExecution.ts:770-795
Timestamp: 2025-10-27T09:47:26.395Z
Learning: In src/core/execution/FakeHumanExecution.ts, the selectSteamrollStopTarget() method intentionally allows MIRV targeting when secondHighest city count is 0 (e.g., nuclear endgame scenarios where structures are destroyed). This is valid game design—"if you can afford it, you're good to go"—and should not be flagged as requiring a guard condition.
Applied to files:
tests/LandMine.test.tssrc/core/execution/ConstructionExecution.tssrc/core/execution/nation/structureSpawnTileValue.tssrc/core/execution/PlayerExecution.tssrc/core/execution/LandMineExecution.ts
📚 Learning: 2025-08-27T08:12:19.610Z
Learnt from: mokizzz
Repo: openfrontio/OpenFrontIO PR: 1940
File: resources/lang/en.json:763-766
Timestamp: 2025-08-27T08:12:19.610Z
Learning: In OpenFrontIO, some country entries in src/client/data/countries.json may have similar names but different codes (e.g., "Empire of Japan" vs "Empire of Japan1"). Each unique code requires its own translation key in resources/lang/en.json after normalization. Always verify against countries.json before suggesting removal of translation keys.
Applied to files:
resources/lang/tp.jsonresources/lang/eo.json
📚 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/execution/nation/structureSpawnTileValue.tssrc/core/execution/LandMineExecution.ts
📚 Learning: 2025-08-18T03:11:18.617Z
Learnt from: scottanderson
Repo: openfrontio/OpenFrontIO PR: 1851
File: src/core/execution/FakeHumanExecution.ts:433-441
Timestamp: 2025-08-18T03:11:18.617Z
Learning: In FakeHumanExecution.maybeSpawnStructure, the perceived cost multiplier (owned + 1, capped at 5) applied to structure costs is intentional. This makes AI nations build fewer structures than players could, saving gold for bombs/nukes rather than maxing out structures.
Applied to files:
src/core/execution/nation/structureSpawnTileValue.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/execution/nation/structureSpawnTileValue.tssrc/core/GameRunner.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: In `ClientGameRunner.ts`, the `myPlayer` field is always set when `shouldPreventWindowClose()` is called, so the null check in that method is sufficient without needing to fetch it again from `gameView.playerByClientID()`.
Applied to files:
src/core/GameRunner.ts
📚 Learning: 2025-06-09T02:20:43.637Z
Learnt from: VariableVince
Repo: openfrontio/OpenFrontIO PR: 1110
File: src/client/Main.ts:293-295
Timestamp: 2025-06-09T02:20:43.637Z
Learning: In src/client/Main.ts, during game start in the handleJoinLobby callback, UI elements are hidden using direct DOM manipulation with classList.add("hidden") for consistency. This includes modals, buttons, and error divs. The codebase follows this pattern rather than using component APIs for hiding elements during game transitions.
Applied to files:
src/core/GameRunner.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/GameRunner.tssrc/core/execution/PlayerExecution.ts
📚 Learning: 2025-08-29T16:16:11.309Z
Learnt from: BrewedCoffee
Repo: openfrontio/OpenFrontIO PR: 1957
File: src/core/execution/PlayerExecution.ts:40-52
Timestamp: 2025-08-29T16:16:11.309Z
Learning: In OpenFrontIO PlayerExecution.ts, when Defense Posts are captured due to tile ownership changes, the intended behavior is to first call u.decreaseLevel() to downgrade them, then still transfer them to the capturing player via captureUnit(). This is not a bug - Defense Posts should be both downgraded and captured, not destroyed outright.
Applied to files:
src/core/execution/PlayerExecution.ts
📚 Learning: 2025-10-20T11:02:16.969Z
Learnt from: sambokai
Repo: openfrontio/OpenFrontIO PR: 2225
File: src/core/execution/FakeHumanExecution.ts:57-57
Timestamp: 2025-10-20T11:02:16.969Z
Learning: In src/core/execution/FakeHumanExecution.ts, the correct MIRV victory denial thresholds are VICTORY_DENIAL_TEAM_THRESHOLD = 0.8 (80% for team games) and VICTORY_DENIAL_INDIVIDUAL_THRESHOLD = 0.65 (65% for individual players), not 0.85 and 0.7 as might be mentioned in some documentation.
Applied to files:
src/core/execution/PlayerExecution.tssrc/core/execution/LandMineExecution.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/game/Game.ts
📚 Learning: 2025-08-14T14:05:00.867Z
Learnt from: Dimitrije-V
Repo: openfrontio/OpenFrontIO PR: 1814
File: src/client/graphics/layers/PlayerActionHandler.ts:0-0
Timestamp: 2025-08-14T14:05:00.867Z
Learning: In TypeScript, when dealing with union types like NukeType (derived from UnitType), it's better to accept the broader type (UnitType) as a parameter and use runtime type checking rather than trying to narrow the parameter type at compile time. The approach of keeping flexible input parameters with runtime validation followed by properly typed state/storage is more practical than over-constraining function signatures.
Applied to files:
src/core/game/Game.tssrc/core/configuration/DefaultConfig.ts
🧬 Code graph analysis (6)
tests/LandMine.test.ts (2)
src/core/execution/ConstructionExecution.ts (1)
ConstructionExecution(14-181)src/core/configuration/DefaultConfig.ts (1)
unitInfo(404-546)
src/core/execution/ConstructionExecution.ts (1)
src/core/execution/LandMineExecution.ts (1)
LandMineExecution(15-181)
src/core/GameRunner.ts (10)
src/core/game/Game.ts (4)
Player(548-688)NameViewData(908-912)Game(690-789)GameUpdates(33-35)src/core/Schemas.ts (2)
Player(126-126)ClientID(26-26)src/core/execution/ExecutionManager.ts (1)
Executor(32-154)src/core/game/GameUpdates.ts (2)
GameUpdateViewData(17-23)ErrorUpdate(25-28)src/core/configuration/DefaultConfig.ts (1)
unitInfo(404-546)src/core/execution/AttackExecution.ts (1)
owner(382-384)src/core/execution/TransportShipExecution.ts (1)
owner(287-289)src/core/execution/MIRVExecution.ts (1)
owner(194-196)src/core/game/UnitImpl.ts (1)
owner(181-183)src/core/execution/NukeExecution.ts (1)
owner(340-342)
src/client/graphics/layers/UnitDisplay.ts (1)
src/core/game/GameView.ts (2)
config(783-785)player(732-738)
src/client/UserSettingModal.ts (1)
src/client/LangSelector.ts (1)
translateText(224-244)
src/core/execution/LandMineExecution.ts (4)
src/core/game/Game.ts (4)
Execution(372-377)Game(690-789)Player(548-688)Unit(453-533)src/core/game/GameMap.ts (1)
TileRef(3-3)src/core/PseudoRandom.ts (1)
PseudoRandom(3-76)src/core/configuration/DefaultConfig.ts (1)
maxTroops(817-848)
🪛 GitHub Check: 🔍 ESLint
tests/LandMine.test.ts
[failure] 314-314:
Expected an assignment or function call and instead saw an expression.
[failure] 116-116:
'defenderInitialTiles' is assigned a value but never used.
src/core/GameRunner.ts
[failure] 194-194:
Prefer using nullish coalescing operator (??=) instead of an assignment expression, as it is simpler to read.
🔇 Additional comments (47)
resources/lang/fi.json (2)
275-276: LGTM! Finnish translation for land mine added correctly.The translation "Maamiina" is accurate and appropriate. The JSON structure is properly maintained with the trailing comma, and the entry placement is consistent with the file structure.
460-461: LGTM! Build menu description accurately conveys the mechanic.The Finnish description "Räjähtää kun vihollinen valtaa tämän ruudun" (Explodes when an enemy captures this tile) is clear, grammatically correct, and aligns with the PR objectives. The description style is consistent with other brief, action-focused entries in the build menu.
resources/lang/sv-SE.json (2)
256-257: LGTM! Translation entry added correctly.The JSON structure is valid, and the new "land_mine" translation entry follows the same pattern as existing unit types. The trailing comma on line 256 properly accommodates the new entry.
419-420: LGTM! Build menu description added correctly.The description translation is consistent with the land mine feature behavior described in the PR objectives. JSON structure is valid with proper comma placement.
resources/lang/el.json (3)
307-308: LGTM! Clean localization entry for land mines.The
unit_typeentries are properly structured with correct JSON syntax. The comma on line 307 ensures valid formatting, and the newland_minekey follows the existing naming pattern.
352-353: LGTM! Keybind descriptions follow established patterns.The
build_land_mineentries in the user settings section are consistent with other build command entries. The description follows the same format as other unit build descriptions (e.g., comparing with lines 346-353).
503-504: LGTM! Description accurately reflects mine behavior.The
build_menu.descentries are well-structured. The land mine description on line 504 correctly describes the explosion mechanic when an enemy captures the tile, which aligns with the PR objectives.While the structure looks good, consider verifying that this Greek translation is consistent with the English source (
en.json) mentioned in the PR checklist. Run the following to compare the key structure across language files:#!/bin/bash # Verify land_mine keys exist consistently across all language files echo "=== Checking for land_mine keys in all language files ===" fd -e json . resources/lang/ --exec sh -c 'echo "File: {}"; grep -o "\"land_mine\"" {} | wc -l' echo -e "\n=== Checking specific key patterns ===" rg -n "land_mine" resources/lang/en.jsonresources/lang/da.json (1)
261-262: LGTM! Land Mine translations added correctly.The Danish translations for the Land Mine unit type and build menu description are properly formatted with correct JSON syntax.
Also applies to: 431-432
resources/lang/hu.json (1)
278-279: LGTM! Hungarian translations added correctly.The Land Mine translations are properly integrated with correct JSON structure.
Also applies to: 465-466
resources/lang/pl.json (1)
307-308: LGTM! Polish translations with keybind support added.The Polish translations include both unit type labels and user setting keybind entries, providing complete localization coverage for the Land Mine feature.
Also applies to: 352-353, 503-504
resources/lang/fr.json (1)
327-328: LGTM! French translations with keybind support added.The French localization includes unit type, build menu description, and keybind labels, providing comprehensive Land Mine feature support.
Also applies to: 372-373, 523-524
src/core/game/PlayerImpl.ts (1)
1006-1007: LGTM! Land Mine build routing added correctly.The
UnitType.LandMinecase correctly routes tolandBasedStructureSpawn, which handles territory ownership validation and minimum distance checks between structures. This is consistent with how other land-based structures (City, Factory, DefensePost, etc.) are validated.src/client/graphics/layers/StructureIconsLayer.ts (1)
94-94: LGTM!The addition of
LandMineto the structures map follows the existing pattern consistently. All structure types are initialized withvisible: true, which is appropriate for the new unit type.src/core/game/Game.ts (1)
199-199: LGTM! Clean type definitions for Land Mine feature.The changes correctly introduce the Land Mine unit type with proper type-level integration:
visibleToEnemiesoptional flag onUnitInfoenables per-unit visibility controlUnitType.LandMinefollows naming conventions- Correctly classified as a structure type
- Parameter map entry matches other zero-parameter structures (City, Port, Factory)
Also applies to: 219-219, 235-235, 304-305
src/client/UserSettingModal.ts (1)
487-494: LGTM! Keybind integration follows established patterns.The Land Mine keybind is properly wired:
- Follows the same structure as other build keybinds
- Uses "Minus" key to avoid conflicts with existing binds (as noted in PR description)
- Translation keys follow naming conventions
- Positioned logically between Defense Post and Missile Silo
src/client/graphics/layers/StructureLayer.ts (1)
14-14: LGTM! Rendering configuration is consistent.The Land Mine rendering setup follows the same pattern as other structures:
- Icon import matches existing convention
- Border and territory radius calculations identical to other structures (Port, City, Factory, etc.)
- Reuses existing rendering infrastructure without special cases
Also applies to: 73-77
src/core/execution/PlayerExecution.ts (1)
57-60: Clean implementation of under-construction mine cleanup.The logic correctly deletes land mines that are still being built when the tile is captured. This prevents incomplete mines from persisting or triggering unintended behavior. The check is properly scoped (type check + construction status) and follows the existing pattern established by the DefensePost handling above.
resources/lang/cs.json (1)
219-220: Czech translations added correctly.Both required translation keys are present with appropriate values. The unit type label and build menu description accurately convey the land mine feature in Czech.
Also applies to: 349-350
resources/lang/sk.json (1)
261-262: Slovak translations added correctly.Both required translation keys are present with appropriate values. The unit type label and build menu description accurately convey the land mine feature in Slovak.
Also applies to: 431-432
resources/lang/de.json (1)
250-251: German translations added correctly.Both required translation keys are present with appropriate values. The unit type label "Landmine" and build menu description accurately convey the land mine feature in German.
Also applies to: 394-395
resources/lang/tr.json (1)
275-276: Turkish translations added correctly.Both required translation keys are present with appropriate values. The unit type label "Kara Mayını" and build menu description accurately convey the land mine feature in Turkish.
Also applies to: 460-461
resources/lang/uk.json (3)
327-328: LGTM! Ukrainian translations added correctly.The land_mine unit type and description translations follow the established pattern and are consistent with other unit type entries in this file.
523-524: LGTM! Build menu description added correctly.The Ukrainian description accurately conveys the Land Mine's behavior and is consistent with the existing translation style.
372-373: LGTM! Keybind translations added correctly.The buildLandMine keybind entries are consistent with other build action translations in the user settings section.
resources/lang/mk.json (2)
275-276: LGTM! Macedonian translations added correctly.The land_mine unit type translation follows the established pattern and is consistent with other unit type entries.
460-461: LGTM! Build menu description added correctly.The Macedonian description accurately conveys the Land Mine's detonation behavior when captured by an enemy.
src/core/execution/ConstructionExecution.ts (3)
6-6: LGTM! Import added correctly.The LandMineExecution import follows the existing pattern and alphabetical ordering.
148-150: LGTM! LandMine construction case added correctly.The LandMine case follows the established pattern for structure-type units and properly instantiates LandMineExecution.
167-167: LGTM! Structure classification added correctly.LandMine is properly classified as a structure, ensuring it follows the standard construction lifecycle.
src/client/graphics/layers/UnitDisplay.ts (5)
15-15: LGTM! Icon import added correctly.Using the explosion icon for Land Mine is thematically appropriate and follows the existing import pattern.
38-38: LGTM! Field added consistently.The _landMine field follows the established pattern for tracking unit counts.
64-64: LGTM! Disabled check updated correctly.LandMine is properly included in the allDisabled calculation, ensuring correct UI visibility.
114-114: LGTM! Unit tracking added correctly.The _landMine counter is properly updated in the tick method, consistent with other unit types.
169-175: LGTM! Land Mine UI item rendered correctly.The Land Mine display follows the established pattern, uses the correct icon, counter, and default hotkey ("-" as specified in the PR objectives).
src/client/graphics/layers/BuildMenu.ts (2)
29-29: LGTM! Icon import added correctly.The landMineIcon import is consistent with UnitDisplay.ts and follows the established pattern.
107-113: LGTM! Build menu entry added correctly.The Land Mine entry is complete with all required fields, uses the correct translation keys, and is logically placed among defensive structures.
src/core/execution/nation/structureSpawnTileValue.ts (1)
151-191: LGTM!The land mine placement logic follows established patterns in the codebase. The approach of placing mines near borders where hostile neighbors exist, while maintaining spacing between mines, is sensible for the intended gameplay.
src/client/graphics/layers/StructureDrawingUtils.ts (1)
7-7: LGTM!The land mine icon import and structure configuration follow existing patterns. Using the "circle" shape keeps visual consistency with similar structures.
Also applies to: 21-21, 67-67
src/core/configuration/DefaultConfig.ts (1)
533-542: LGTM!The land mine configuration looks correct:
- Cost scaling ($75k base, +$50k per unit, capped at $250k) matches the PR description.
- 5-second construction time is reasonable.
visibleToEnemies: falseenables the stealth mechanic.resources/lang/en.json (1)
378-379: LGTM!The English translation keys for land mine are correctly added following existing naming conventions. The descriptions are clear and concise.
Also applies to: 424-425, 575-576
resources/lang/fa.json (1)
327-328: LGTM!The Persian translations for land mine are properly added and follow the existing file structure. Keys are placed in the correct sections:
unit_type,user_setting, andbuild_menu.desc.Also applies to: 372-373, 523-524
tests/LandMine.test.ts (1)
25-82: Good test setup and structure.The
beforeEachhook properly initializes the game with infinite resources and spawns both players. The spawn phase completion and initial expansion provide a solid foundation for the test scenarios.src/core/GameRunner.ts (2)
188-229: Clean visibility filtering implementation.The filtering logic properly handles all cases: visible units pass through, owners see their own hidden units, allies see each other's hidden units, and spectators cannot see any hidden units. The
isPlayer()guard at line 221 prevents callingisAlliedWith()on TerraNullius.
74-82: LGTM!The
clientIDparameter is correctly passed to enable per-client visibility filtering.src/core/execution/LandMineExecution.ts (3)
68-84: Clean tile destruction calculation.The
tilesToDestroymethod properly filters to only attacker-owned tiles and uses squared distance to avoid sqrt calls. The random chance for outer ring adds visual variety.
32-66: Well-structured tick logic.The tick method handles all ownership states cleanly: inactive mine, under construction, terra nullius capture, original owner, ally capture, and enemy capture. The early returns make the flow easy to follow.
95-109: The per-tile troop removal is intentional and matches the behavior in NukeExecution. As tiles are destroyed,attacker.numTilesOwned()decreases, causing cumulative damage to scale with destruction. This cascading effect is the intended design.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
tests/LandMine.test.ts (2)
134-315: Consider extracting slow-build game setup.Tests 4 and 5 have substantial setup duplication (lines 134-186 and 224-276). Consider extracting a helper function like:
async function setupSlowBuildGame() { const slowBuildGame = await setup("plains", { infiniteGold: true, instantBuild: false, infiniteTroops: true, }); // ... player setup and spawning return { slowBuildGame, defender2, attacker2 }; }This would improve maintainability and reduce duplication.
384-458: Consider simplifying update filtering tests.These tests duplicate the filtering logic from
GameRunner.filterHiddenUnits(lines 405-424, 503-520). If the filtering implementation changes, both the code and tests need updates.Consider testing the behavior more directly:
// Instead of reimplementing the filter logic: const attackerLandMineUpdates = allUnitUpdates.filter( (u: UnitUpdate) => u.unitType === UnitType.LandMine ); expect(attackerLandMineUpdates).toHaveLength(0);This approach verifies the correct outcome without coupling tests to implementation details.
Also applies to: 460-526
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/core/GameRunner.tstests/LandMine.test.ts
🧰 Additional context used
🧠 Learnings (13)
📚 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: In `ClientGameRunner.ts`, the `myPlayer` field is always set when `shouldPreventWindowClose()` is called, so the null check in that method is sufficient without needing to fetch it again from `gameView.playerByClientID()`.
Applied to files:
src/core/GameRunner.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/GameRunner.tstests/LandMine.test.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/GameRunner.tstests/LandMine.test.ts
📚 Learning: 2025-06-09T02:20:43.637Z
Learnt from: VariableVince
Repo: openfrontio/OpenFrontIO PR: 1110
File: src/client/Main.ts:293-295
Timestamp: 2025-06-09T02:20:43.637Z
Learning: In src/client/Main.ts, during game start in the handleJoinLobby callback, UI elements are hidden using direct DOM manipulation with classList.add("hidden") for consistency. This includes modals, buttons, and error divs. The codebase follows this pattern rather than using component APIs for hiding elements during game transitions.
Applied to files:
src/core/GameRunner.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/GameRunner.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:
tests/LandMine.test.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:
tests/LandMine.test.ts
📚 Learning: 2025-10-18T17:54:01.311Z
Learnt from: sambokai
Repo: openfrontio/OpenFrontIO PR: 2225
File: src/core/execution/FakeHumanExecution.ts:172-173
Timestamp: 2025-10-18T17:54:01.311Z
Learning: In src/core/execution/FakeHumanExecution.ts, MIRVs and ground attacks should not be mutually exclusive. The considerMIRV() method should not short-circuit maybeAttack() - both actions can occur in the same tick.
Applied to files:
tests/LandMine.test.ts
📚 Learning: 2025-10-27T09:47:26.395Z
Learnt from: sambokai
Repo: openfrontio/OpenFrontIO PR: 2225
File: src/core/execution/FakeHumanExecution.ts:770-795
Timestamp: 2025-10-27T09:47:26.395Z
Learning: In src/core/execution/FakeHumanExecution.ts, the selectSteamrollStopTarget() method intentionally allows MIRV targeting when secondHighest city count is 0 (e.g., nuclear endgame scenarios where structures are destroyed). This is valid game design—"if you can afford it, you're good to go"—and should not be flagged as requiring a guard condition.
Applied to files:
tests/LandMine.test.ts
📚 Learning: 2025-08-29T16:16:11.309Z
Learnt from: BrewedCoffee
Repo: openfrontio/OpenFrontIO PR: 1957
File: src/core/execution/PlayerExecution.ts:40-52
Timestamp: 2025-08-29T16:16:11.309Z
Learning: In OpenFrontIO PlayerExecution.ts, when Defense Posts are captured due to tile ownership changes, the intended behavior is to first call u.decreaseLevel() to downgrade them, then still transfer them to the capturing player via captureUnit(). This is not a bug - Defense Posts should be both downgraded and captured, not destroyed outright.
Applied to files:
tests/LandMine.test.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:
tests/LandMine.test.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:
tests/LandMine.test.ts
📚 Learning: 2025-09-06T07:10:42.293Z
Learnt from: Michal-Martinek
Repo: openfrontio/OpenFrontIO PR: 2014
File: eslint.config.js:43-43
Timestamp: 2025-09-06T07:10:42.293Z
Learning: When analyzing potential no-unused-expressions violations, always examine the full context. Expressions used in template literals, filter predicates, assignments, or other consuming contexts are not violations even if they use short-circuit operators like &&.
Applied to files:
tests/LandMine.test.ts
🧬 Code graph analysis (1)
src/core/GameRunner.ts (4)
src/core/game/PlayerImpl.ts (1)
clientID(197-199)src/core/game/Game.ts (4)
Player(548-688)NameViewData(908-912)Game(690-789)GameUpdates(33-35)src/core/game/GameUpdates.ts (1)
GameUpdateViewData(17-23)src/core/configuration/DefaultConfig.ts (1)
unitInfo(404-546)
🔇 Additional comments (6)
tests/LandMine.test.ts (3)
18-82: LGTM: Solid test setup.The test setup properly isolates each test with
beforeEach, configures the game with appropriate test flags (instant build, infinite resources), and establishes initial territories for both players.
84-132: LGTM: Core explosion behavior well tested.The tests verify buildability, explosion on enemy capture, and that explosions only damage the attacker. The defensive
whileloop withmaxTicksprevents infinite loops if capture fails.
317-382: LGTM: Ally interactions and unit properties verified.The tests cover ally capture behavior (using direct
conquer()for precise control), mine destruction on detonation, and verify the unit's configuration flags. All assertions are appropriate.src/core/GameRunner.ts (3)
35-97: LGTM: Clean parameter threading for client identification.The
clientIDparameter flows cleanly fromcreateGameRunnerthrough to theGameRunnerconstructor. The lazy-initializedmyPlayerfield uses a typed union (Player | null) appropriately for the spectator case.
175-176: LGTM: Filtering correctly positioned in tick pipeline.The visibility filtering is applied after tile updates are processed and before results are sent to the client via
callBack, ensuring enemies never receive updates for hidden units.
188-227: LGTM: Visibility filtering is correctly implemented.The method properly handles all cases:
- Lazy-loads
myPlayerusing the??=operator for efficiency- Defaults to visible when
visibleToEnemies !== false(handles undefined/true)- Correctly handles spectators (no
myPlayer)- Uses type narrowing with
owner.isPlayer()before alliance checks- Filters in-place by mutating the updates array
The logic is sound and type-safe.
|
@coderabbitai generate docstrings |
|
Warning Docstrings generation is disabled for your repository or organization. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 4945 🏁 Script executed: Length of output: 9079 🏁 Script executed: Length of output: 6663 🏁 Script executed: Length of output: 84 🏁 Script executed: Length of output: 4420 🏁 Script executed: Length of output: 3250 🏁 Script executed: Length of output: 1620 @ryanhaticus I'll generate comprehensive docstrings for the land mines feature. Here are the updated files: LandMineExecution.tsimport {
Execution,
Game,
isStructureType,
MessageType,
Player,
Unit,
UnitType,
} from "../game/Game";
import { TileRef } from "../game/GameMap";
import { PseudoRandom } from "../PseudoRandom";
const SPRITE_RADIUS = 16;
/**
* Execution handler for Land Mine units.
*
* Land mines are hidden defensive structures that detonate when an enemy captures
* their tile. They do not explode if captured by allies or if still under construction.
*
* Key behaviors:
* - Monitors tile ownership changes
* - Transfers ownership to allies without detonating
* - Detonates on enemy capture, damaging only the attacker
* - Destroys the mine after detonation
*/
export class LandMineExecution implements Execution {
private mg: Game;
private active: boolean = true;
private originalOwner: Player;
/**
* Creates a new LandMineExecution instance.
*
* @param mine - The land mine unit to manage
*/
constructor(private mine: Unit) {
this.originalOwner = mine.owner();
}
/**
* Initializes the execution with game context.
*
* @param mg - The game instance
* @param ticks - Current tick count (unused)
*/
init(mg: Game, ticks: number): void {
this.mg = mg;
}
/**
* Determines if this execution should run during the spawn phase.
*
* @returns Always false, land mines do not activate during spawn phase
*/
activeDuringSpawnPhase(): boolean {
return false;
}
/**
* Executes one tick of land mine logic.
*
* Checks tile ownership and handles:
* - Deactivation if mine is destroyed
* - No-op while under construction
* - Deletion if tile becomes terra nullius
* - No-op if owned by original owner
* - Ownership transfer to allies
* - Detonation on enemy capture
*
* @param ticks - Current tick count (unused)
*/
tick(ticks: number): void {
if (!this.mine.isActive()) {
this.active = false;
return;
}
// Do nothing while the structure is under construction
if (this.mine.isUnderConstruction()) {
return;
}
// Check if the mine's tile has been captured by an enemy
const currentOwner = this.mg.owner(this.mine.tile());
if (!currentOwner.isPlayer()) {
// Tile is now terra nullius, delete the mine
this.mine.delete();
this.active = false;
return;
}
// If the tile is still owned by the original owner, do nothing
if (currentOwner === this.originalOwner) {
return;
}
// If captured by an ally of the original owner, transfer ownership
if (currentOwner.isFriendly(this.originalOwner)) {
currentOwner.captureUnit(this.mine);
this.originalOwner = currentOwner;
return;
}
// An enemy has captured the tile - DETONATE!
this.detonate(currentOwner);
}
/**
* Collects all tiles within blast radius that are owned by the attacker.
*
* Uses breadth-first search with randomized outer blast radius damage.
* Tiles in the inner radius always included; outer radius has 50% chance.
*
* @param attacker - The player who triggered the mine
* @returns Set of tiles owned by attacker within blast radius
*/
private tilesToDestroy(attacker: Player): Set<TileRef> {
const magnitude = this.mg.config().nukeMagnitudes(UnitType.LandMine);
const rand = new PseudoRandom(this.mg.ticks());
const inner2 = magnitude.inner * magnitude.inner;
const outer2 = magnitude.outer * magnitude.outer;
const tile = this.mine.tile();
// Only include tiles owned by the attacker
return this.mg.bfs(tile, (_, n: TileRef) => {
const owner = this.mg.owner(n);
if (!owner.isPlayer() || owner !== attacker) {
return false;
}
const d2 = this.mg.euclideanDistSquared(tile, n);
return d2 <= outer2 && (d2 <= inner2 || rand.chance(2));
});
}
/**
* Detonates the land mine, damaging only the attacker.
*
* Explosion effects:
* - Attacker loses control of tiles in blast radius
* - Attacker loses troops based on nuke death factor
* - Land tiles are marked with fallout
* - Attacker's outgoing attacks are damaged
* - Attacker's units in blast radius are destroyed (except nukes in flight)
* - Nearby buildings are redrawn for visual update
* - Notification messages sent to both attacker and mine owner
* - Mine is deleted and execution deactivated
*
* @param attacker - The player who triggered the mine
*/
private detonate(attacker: Player) {
const magnitude = this.mg.config().nukeMagnitudes(UnitType.LandMine);
const tile = this.mine.tile();
const toDestroy = this.tilesToDestroy(attacker);
// Calculate max troops for death factor
const maxTroops = this.mg.config().maxTroops(attacker);
// Only damage the attacker's territory and troops
for (const t of toDestroy) {
attacker.relinquish(t);
attacker.removeTroops(
this.mg.config().nukeDeathFactor(
UnitType.AtomBomb, // Use atom bomb death factor calculation
attacker.troops(),
attacker.numTilesOwned(),
maxTroops,
),
);
if (this.mg.isLand(t)) {
this.mg.setFallout(t, true);
}
}
// Also damage attacker's outgoing attacks
attacker.outgoingAttacks().forEach((attack) => {
const deaths = this.mg
.config()
.nukeDeathFactor(
UnitType.AtomBomb,
attack.troops(),
attacker.numTilesOwned(),
maxTroops,
);
attack.setTroops(attack.troops() - deaths);
});
// Destroy attacker's units in blast radius (excluding nukes in flight)
const outer2 = magnitude.outer * magnitude.outer;
for (const unit of this.mg.units()) {
// Skip units not owned by the attacker
if (unit.owner() !== attacker) {
continue;
}
if (
unit.type() !== UnitType.AtomBomb &&
unit.type() !== UnitType.HydrogenBomb &&
unit.type() !== UnitType.MIRVWarhead &&
unit.type() !== UnitType.MIRV
) {
if (this.mg.euclideanDistSquared(tile, unit.tile()) < outer2) {
unit.delete(true, this.originalOwner);
}
}
}
// Notify the attacker
this.mg.displayMessage(
`You triggered a land mine placed by ${this.originalOwner.displayName()}!`,
MessageType.NUKE_INBOUND,
attacker.id(),
);
// Notify the mine owner
this.mg.displayMessage(
`Your land mine was triggered by ${attacker.displayName()}!`,
MessageType.CAPTURED_ENEMY_UNIT,
this.originalOwner.id(),
);
// Redraw buildings in the area
this.redrawBuildings(magnitude.outer + SPRITE_RADIUS);
// Delete the mine
this.mine.delete(false);
this.active = false;
}
/**
* Forces a visual update for buildings near the explosion.
*
* Ensures that structures within the visual range of the explosion
* are redrawn to reflect the changed game state.
*
* @param range - The radius in which to redraw buildings
*/
private redrawBuildings(range: number) {
const tile = this.mine.tile();
const rangeSquared = range * range;
for (const unit of this.mg.units()) {
if (isStructureType(unit.type())) {
if (this.mg.euclideanDistSquared(tile, unit.tile()) < rangeSquared) {
unit.touch();
}
}
}
}
/**
* Checks if this execution is still active.
*
* @returns True if the mine is still monitoring, false if detonated or destroyed
*/
isActive(): boolean {
return this.active;
}
}GameRunner.ts (filterHiddenUnits method) /**
* Filters out unit updates for units that should be hidden from this client.
*
* Units with visibleToEnemies explicitly set to false are only visible to:
* - Their owner
* - Allied players of their owner
*
* Enemies and spectators will not receive updates for these hidden units,
* preventing cheating through client-side inspection (e.g., land mines).
*
* @param updates - The game updates to filter
*/
private filterHiddenUnits(updates: GameUpdates): void {
// Lazy-load our player reference
this.myPlayer ??= this.game.playerByClientID(this.clientID) ?? null;
updates[GameUpdateType.Unit] = updates[GameUpdateType.Unit].filter(
(unitUpdate) => {
const unitInfo = this.game.config().unitInfo(unitUpdate.unitType);
// If unit is visible to enemies (default), keep it
if (unitInfo.visibleToEnemies !== false) {
return true;
}
// Unit is hidden from enemies - check if we should see it
if (this.myPlayer === null) {
// Spectator - can't see hidden enemy units
return false;
}
// Check if we own or are allied with the owner
const ownerSmallID = unitUpdate.ownerID;
if (this.myPlayer.smallID() === ownerSmallID) {
return true; // We own it
}
// Check if owner is our ally
const owner = this.game.playerBySmallID(ownerSmallID);
if (owner.isPlayer() && this.myPlayer.isAlliedWith(owner)) {
return true; // Allied with owner
}
// Enemy unit that should be hidden
return false;
},
);
}structureSpawnTileValue.ts (LandMine case) case UnitType.LandMine: {
/**
* Land mine placement strategy:
*
* Prioritizes tiles where enemies are likely to attack:
* - Close to borders (where combat happens)
* - Adjacent to hostile players
* - Spaced apart from other mines to maximize coverage
*
* Weighting factors:
* - Distance from border: closer is better (within borderSpacing)
* - Hostile neighbors: more hostile = higher weight
* - Spacing from other mines: maintains minimum structureSpacing
*/
return (tile) => {
let w = 0;
// Prefer to be close to the border (where enemies will attack)
const [closest, closestBorderDist] = closestTile(mg, borderTiles, tile);
if (closest !== null) {
// Prefer tiles close to the border but not right on it
w += Math.max(0, borderSpacing - closestBorderDist);
// Prefer adjacent players who are hostile
const neighbors: Set<Player> = new Set();
for (const neighborTile of mg.neighbors(closest)) {
if (!mg.isLand(neighborTile)) continue;
const id = mg.ownerID(neighborTile);
if (id === player.smallID()) continue;
const neighbor = mg.playerBySmallID(id);
if (!neighbor.isPlayer()) continue;
neighbors.add(neighbor);
}
for (const neighbor of neighbors) {
w +=
borderSpacing * (Relation.Friendly - player.relation(neighbor));
}
}
// Prefer to be away from other land mines
const otherTiles: Set<TileRef> = new Set(
otherUnits.map((u) => u.tile()),
);
otherTiles.delete(tile);
const closestOther = closestTwoTiles(mg, otherTiles, [tile]);
if (closestOther !== null) {
const d = mg.manhattanDist(closestOther.x, tile);
w += Math.min(d, structureSpacing);
}
return w;
};
}These docstrings follow the existing patterns in your codebase and provide clear explanations of the land mine functionality using simple, readable language. |
ryanhaticus
force-pushed
the
ryanhaticus/land-mines
branch
from
26ec53e to
e2b5e4f
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/core/GameRunner.ts (1)
188-227: Implementation is clean and correct.The visibility filtering logic correctly handles all cases:
- Visible units pass through immediately (line 201)
- Spectators see no hidden units (line 206-209)
- Owners and allies see their hidden units (lines 213-221)
The lazy-load pattern for
myPlayerand early return for visible units keep performance reasonable.Optional: Cache config reference if many land mines are expected
If games scale to hundreds of land mines, consider caching the config reference to avoid repeated
this.game.config()calls:private filterHiddenUnits(updates: GameUpdates): void { // Lazy-load our player reference this.myPlayer ??= this.game.playerByClientID(this.clientID) ?? null; + const config = this.game.config(); updates[GameUpdateType.Unit] = updates[GameUpdateType.Unit].filter( (unitUpdate) => { - const unitInfo = this.game.config().unitInfo(unitUpdate.unitType); + const unitInfo = config.unitInfo(unitUpdate.unitType);This is purely an optimization and not required given the early return at line 201.
src/core/execution/nation/structureSpawnTileValue.ts (1)
151-191: Consider extracting the hostile neighbor and clustering logic.The LandMine placement logic duplicates the hostile neighbor evaluation (lines 162-175) and anti-clustering logic (lines 178-187) from the DefensePost case (lines 74-87 and 90-99). Both blocks are nearly identical.
Extract these into reusable helper functions to reduce duplication and make future adjustments easier.
🔎 Example refactor
// Helper to evaluate hostile neighbors at a border tile function evaluateHostileNeighbors( mg: Game, player: Player, borderTile: TileRef, borderSpacing: number, ): number { let weight = 0; const neighbors: Set<Player> = new Set(); for (const tile of mg.neighbors(borderTile)) { if (!mg.isLand(tile)) continue; const id = mg.ownerID(tile); if (id === player.smallID()) continue; const neighbor = mg.playerBySmallID(id); if (!neighbor.isPlayer()) continue; neighbors.add(neighbor); } for (const neighbor of neighbors) { weight += borderSpacing * (Relation.Friendly - player.relation(neighbor)); } return weight; } // Helper to penalize clustering function penalizeClusteringWith( mg: Game, tile: TileRef, otherUnits: Unit[], structureSpacing: number, ): number { const otherTiles: Set<TileRef> = new Set(otherUnits.map((u) => u.tile())); otherTiles.delete(tile); const closestOther = closestTwoTiles(mg, otherTiles, [tile]); if (closestOther !== null) { const d = mg.manhattanDist(closestOther.x, tile); return Math.min(d, structureSpacing); } return 0; }Then use these helpers in both DefensePost and LandMine cases.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
resources/images/buildings/landMine1.pngis excluded by!**/*.png
📒 Files selected for processing (17)
resources/lang/en.jsonsrc/client/InputHandler.tssrc/client/UserSettingModal.tssrc/client/graphics/layers/BuildMenu.tssrc/client/graphics/layers/StructureDrawingUtils.tssrc/client/graphics/layers/StructureIconsLayer.tssrc/client/graphics/layers/StructureLayer.tssrc/client/graphics/layers/UnitDisplay.tssrc/core/GameRunner.tssrc/core/configuration/DefaultConfig.tssrc/core/execution/ConstructionExecution.tssrc/core/execution/LandMineExecution.tssrc/core/execution/PlayerExecution.tssrc/core/execution/nation/structureSpawnTileValue.tssrc/core/game/Game.tssrc/core/game/PlayerImpl.tstests/LandMine.test.ts
🚧 Files skipped from review as they are similar to previous changes (11)
- src/core/game/PlayerImpl.ts
- src/client/graphics/layers/StructureIconsLayer.ts
- src/client/UserSettingModal.ts
- src/core/execution/ConstructionExecution.ts
- resources/lang/en.json
- src/client/graphics/layers/StructureLayer.ts
- src/client/graphics/layers/BuildMenu.ts
- src/core/execution/PlayerExecution.ts
- src/core/configuration/DefaultConfig.ts
- src/core/game/Game.ts
- src/client/InputHandler.ts
🧰 Additional context used
🧠 Learnings (17)
📚 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: In `ClientGameRunner.ts`, the `myPlayer` field is always set when `shouldPreventWindowClose()` is called, so the null check in that method is sufficient without needing to fetch it again from `gameView.playerByClientID()`.
Applied to files:
src/core/GameRunner.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/GameRunner.tssrc/core/execution/LandMineExecution.tstests/LandMine.test.tssrc/core/execution/nation/structureSpawnTileValue.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/GameRunner.tstests/LandMine.test.ts
📚 Learning: 2025-06-09T02:20:43.637Z
Learnt from: VariableVince
Repo: openfrontio/OpenFrontIO PR: 1110
File: src/client/Main.ts:293-295
Timestamp: 2025-06-09T02:20:43.637Z
Learning: In src/client/Main.ts, during game start in the handleJoinLobby callback, UI elements are hidden using direct DOM manipulation with classList.add("hidden") for consistency. This includes modals, buttons, and error divs. The codebase follows this pattern rather than using component APIs for hiding elements during game transitions.
Applied to files:
src/core/GameRunner.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/GameRunner.tssrc/core/execution/nation/structureSpawnTileValue.ts
📚 Learning: 2026-01-03T19:37:22.111Z
Learnt from: ryanbarlow97
Repo: openfrontio/OpenFrontIO PR: 2782
File: src/client/graphics/layers/StructureIconsLayer.ts:616-620
Timestamp: 2026-01-03T19:37:22.111Z
Learning: In `src/client/graphics/layers/StructureIconsLayer.ts`, the confirm button in `ensureGhostControls()` for locked ghost nukes (AtomBomb/HydrogenBomb) intentionally does NOT call `removeGhostStructure()` after emitting the build intent. This allows mobile players to rapidly place multiple nukes by "spamming" the confirm button without needing to reopen the radial menu.
Applied to files:
src/client/graphics/layers/StructureDrawingUtils.tssrc/client/graphics/layers/UnitDisplay.ts
📚 Learning: 2025-05-18T23:36:12.847Z
Learnt from: scottanderson
Repo: openfrontio/OpenFrontIO PR: 784
File: src/core/game/StatsImpl.ts:143-159
Timestamp: 2025-05-18T23:36:12.847Z
Learning: In this codebase, NukeType is a union type derived from UnitType values (specifically bomb-related values like AtomBomb, HydrogenBomb, MIRV, and MIRVWarhead) rather than a separate enum. This means comparing NukeType values against UnitType values in switch statements is valid and intentional.
Applied to files:
src/client/graphics/layers/StructureDrawingUtils.ts
📚 Learning: 2025-05-18T23:36:12.847Z
Learnt from: scottanderson
Repo: openfrontio/OpenFrontIO PR: 784
File: src/core/game/StatsImpl.ts:143-159
Timestamp: 2025-05-18T23:36:12.847Z
Learning: In this codebase, NukeType is a union type derived from specific UnitType enum values (AtomBomb, HydrogenBomb, MIRV, MIRVWarhead). This means that comparisons in switch statements between a NukeType parameter and UnitType enum values are valid and will work correctly at runtime.
Applied to files:
src/client/graphics/layers/StructureDrawingUtils.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/execution/LandMineExecution.tstests/LandMine.test.tssrc/client/graphics/layers/UnitDisplay.tssrc/core/execution/nation/structureSpawnTileValue.ts
📚 Learning: 2025-10-18T17:54:01.311Z
Learnt from: sambokai
Repo: openfrontio/OpenFrontIO PR: 2225
File: src/core/execution/FakeHumanExecution.ts:172-173
Timestamp: 2025-10-18T17:54:01.311Z
Learning: In src/core/execution/FakeHumanExecution.ts, MIRVs and ground attacks should not be mutually exclusive. The considerMIRV() method should not short-circuit maybeAttack() - both actions can occur in the same tick.
Applied to files:
src/core/execution/LandMineExecution.tstests/LandMine.test.ts
📚 Learning: 2025-10-20T11:02:16.969Z
Learnt from: sambokai
Repo: openfrontio/OpenFrontIO PR: 2225
File: src/core/execution/FakeHumanExecution.ts:57-57
Timestamp: 2025-10-20T11:02:16.969Z
Learning: In src/core/execution/FakeHumanExecution.ts, the correct MIRV victory denial thresholds are VICTORY_DENIAL_TEAM_THRESHOLD = 0.8 (80% for team games) and VICTORY_DENIAL_INDIVIDUAL_THRESHOLD = 0.65 (65% for individual players), not 0.85 and 0.7 as might be mentioned in some documentation.
Applied to files:
src/core/execution/LandMineExecution.tstests/LandMine.test.ts
📚 Learning: 2025-10-27T09:47:26.395Z
Learnt from: sambokai
Repo: openfrontio/OpenFrontIO PR: 2225
File: src/core/execution/FakeHumanExecution.ts:770-795
Timestamp: 2025-10-27T09:47:26.395Z
Learning: In src/core/execution/FakeHumanExecution.ts, the selectSteamrollStopTarget() method intentionally allows MIRV targeting when secondHighest city count is 0 (e.g., nuclear endgame scenarios where structures are destroyed). This is valid game design—"if you can afford it, you're good to go"—and should not be flagged as requiring a guard condition.
Applied to files:
src/core/execution/LandMineExecution.tstests/LandMine.test.tssrc/core/execution/nation/structureSpawnTileValue.ts
📚 Learning: 2025-08-29T16:16:11.309Z
Learnt from: BrewedCoffee
Repo: openfrontio/OpenFrontIO PR: 1957
File: src/core/execution/PlayerExecution.ts:40-52
Timestamp: 2025-08-29T16:16:11.309Z
Learning: In OpenFrontIO PlayerExecution.ts, when Defense Posts are captured due to tile ownership changes, the intended behavior is to first call u.decreaseLevel() to downgrade them, then still transfer them to the capturing player via captureUnit(). This is not a bug - Defense Posts should be both downgraded and captured, not destroyed outright.
Applied to files:
src/core/execution/LandMineExecution.tstests/LandMine.test.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/execution/LandMineExecution.tstests/LandMine.test.tssrc/core/execution/nation/structureSpawnTileValue.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:
tests/LandMine.test.ts
📚 Learning: 2025-09-06T07:10:42.293Z
Learnt from: Michal-Martinek
Repo: openfrontio/OpenFrontIO PR: 2014
File: eslint.config.js:43-43
Timestamp: 2025-09-06T07:10:42.293Z
Learning: When analyzing potential no-unused-expressions violations, always examine the full context. Expressions used in template literals, filter predicates, assignments, or other consuming contexts are not violations even if they use short-circuit operators like &&.
Applied to files:
tests/LandMine.test.ts
📚 Learning: 2025-08-18T03:11:18.617Z
Learnt from: scottanderson
Repo: openfrontio/OpenFrontIO PR: 1851
File: src/core/execution/FakeHumanExecution.ts:433-441
Timestamp: 2025-08-18T03:11:18.617Z
Learning: In FakeHumanExecution.maybeSpawnStructure, the perceived cost multiplier (owned + 1, capped at 5) applied to structure costs is intentional. This makes AI nations build fewer structures than players could, saving gold for bombs/nukes rather than maxing out structures.
Applied to files:
src/core/execution/nation/structureSpawnTileValue.ts
🧬 Code graph analysis (2)
src/core/GameRunner.ts (2)
src/core/game/Game.ts (4)
Player(560-700)PlayerID(22-22)Game(702-801)GameUpdates(33-35)src/core/configuration/DefaultConfig.ts (1)
unitInfo(435-577)
src/core/execution/LandMineExecution.ts (4)
src/core/game/Game.ts (4)
Execution(384-389)Game(702-801)Player(560-700)Unit(465-545)src/core/game/GameMap.ts (1)
TileRef(3-3)src/core/PseudoRandom.ts (1)
PseudoRandom(3-76)src/core/configuration/DefaultConfig.ts (1)
maxTroops(848-879)
🔇 Additional comments (8)
src/core/GameRunner.ts (2)
78-78: Constructor changes look good.The addition of
clientIDparameter andmyPlayercaching field properly support the per-client visibility filtering feature. The lazy initialization pattern formyPlayer(used infilterHiddenUnits) avoids repeated lookups.Also applies to: 88-88, 96-96
175-176: Correct placement for visibility filtering.Filtering unit updates after tick execution and before the callback ensures hidden units are suppressed on the server side before reaching the client.
src/client/graphics/layers/UnitDisplay.ts (1)
15-15: LGTM!The Land Mine UI integration follows the established pattern for other units. Icon import, counter initialization, allDisabled check, tick() population, and render() display are all consistent with existing units.
Also applies to: 38-38, 64-64, 114-114, 169-175
src/client/graphics/layers/StructureDrawingUtils.ts (1)
7-7: LGTM!Land Mine icon and shape registration follows the established pattern. The "circle" shape choice (line 21) matches City and Factory, which may be intentional for visual consistency.
Also applies to: 21-21, 67-67
tests/LandMine.test.ts (1)
1-527: LGTM! Excellent test coverage.The test suite comprehensively validates Land Mine behavior:
- Basic buildability and explosion mechanics
- Construction timing and under-construction behavior
- Ally vs. enemy interactions
- Territory ownership and visibility rules
- Server-side update filtering for enemies and allies
The tests follow consistent patterns and thoroughly exercise edge cases. Well done!
src/core/execution/LandMineExecution.ts (3)
32-66: LGTM! Ownership transfer and detonation logic is correct.The tick() method properly handles all ownership scenarios:
- Inactive mine cleanup
- Construction delay
- Terra nullius deletion
- Ally ownership transfer (with captureUnit() call)
- Enemy detonation
The ally transfer at lines 57-62 correctly calls
captureUnit()to maintain unit/tile ownership consistency.
68-84: LGTM! Blast radius calculation is correct.The
tilesToDestroy()method correctly:
- Uses configured Land Mine magnitudes
- Limits damage to attacker-owned tiles only
- Applies inner/outer radius with random exemption
- Uses BFS for efficient tile traversal
86-122: Code is correct—no changes needed.LandMine cannot be passed to
nukeDeathFactor()because it is not part of theNukeTypeunion (which only includes AtomBomb, HydrogenBomb, MIRV, and MIRVWarhead). UsingUnitType.AtomBombas a fallback is intentional and necessary, as confirmed by the inline comment. The death factor formula(5 * humans) / Math.max(1, tilesOwned)is appropriate for both weapons.
|
Hello! I believe this currently adds a little more in complexity and adds another unit i dont feel we need at the moment. For the forseable future we are going to close this, but thanks for the work! |
3 participants
Description:
Adds land mines to the game which cause a small explosion when captured by an enemy. I know this is a rather large change, but some friends wanted to play together with land mines on our own server so I was writing this code anyway.
visibleToEnemiesonUnitInfo. In addition, we don't send unit updates from the server to enemy clients to prevent cheating.Mas I did not want to change existing binds.Video (skip to end to see me agro a captor): https://youtu.be/JrMJPf_3t1Q
Please complete the following:
Please put your Discord username so you can be contacted if a bug or regression is found:
haticus