Improve transport ship spawn tile selection #2804
Conversation
Refactor transport ship spawning logic to find the best source tile based on fastest route to target.
|
|
WalkthroughTransportShipExecution now selects spawn tiles using cost-based optimization. Instead of checking feasibility directly, it gathers candidate spawn tiles, computes travel cost to destination via pathfinding for each, and selects the tile with minimum cost as the source. Changes
Sequence DiagramsequenceDiagram
participant ES as TransportShipExecution
participant TST as transportShipSpawnTiles()
participant PF as Pathfinder
participant SL as Tile Selection
ES->>TST: Get candidate spawn tiles
TST-->>ES: Return tile candidates
loop For each candidate tile
ES->>PF: Calculate travel cost<br/>(tile → destination)
PF-->>ES: Return cost estimate
end
ES->>SL: Find tile with min cost
SL-->>ES: Return best source tile
alt Valid source found
ES->>ES: Set this.src to best tile
ES->>ES: Continue execution
else No valid source
ES->>ES: Log warning
ES->>ES: Deactivate execution
end
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks✅ Passed checks (3 passed)
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: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/core/execution/TransportShipExecution.ts (1)
136-150: Critical: Remove dead code with undefined variable reference.This entire block has multiple issues:
- Undefined variable:
closestTileSrcis never defined (lines 139, 148), which will cause runtime crashes if reached.- Dead code: After line 134 sets
this.src = bestSrc, and lines 128-132 ensurebestSrcis not null (or deactivate execution), the conditionthis.src === nullat line 136 should never be true.- Leftover logic: This appears to be remnant code from the old implementation that should have been removed.
Since the new logic (lines 116-134) already handles spawn tile selection and validation, this fallback block is unnecessary and broken.
🔎 Remove the broken fallback logic
- if (this.src === null) { - // Only update the src if it's not already set - // because we assume that the src is set to the best spawn tile - this.src = closestTileSrc; - } else { - if ( - this.mg.owner(this.src) !== this.attacker || - !this.mg.isShore(this.src) - ) { - console.warn( - `src is not a shore tile or not owned by: ${this.attacker.name()}`, - ); - this.src = closestTileSrc; - } - }Note: If you need to handle the case where
this.srcis pre-set externally via the constructor, add validation before line 116 to check if the pre-setthis.srcis valid, and only run the best-tile search if it's null.
🤖 Fix all issues with AI agents
In @src/core/execution/TransportShipExecution.ts:
- Around line 116-134: The block that finds a spawn tile (using candidateTiles
from attacker.transportShipSpawnTiles(), the loop over tile,
pathFinder.estimateCost, and setting this.src) is indented with tabs instead of
the project's 2-space style; replace all tabs in this block with consistent
2-space indentation so the for loop, if blocks, and assignments align with the
rest of the file and TypeScript conventions, ensuring candidateTiles, bestSrc,
lowestCost, the for (const tile ...) loop, the if (!bestSrc) branch, and the
final this.src = bestSrc; lines all use 2 spaces per indent level.
- Line 116: The call to this.attacker.transportShipSpawnTiles() fails because
Player lacks transportShipSpawnTiles(); add a method transportShipSpawnTiles():
TileRef[] to the Player class that returns all shore tiles controlled by the
player which are valid spawn locations for a transport ship (i.e., filter the
player's tile collection for tiles owned/controlled by this player that meet the
"shore" and "spawnable" criteria). Implement the method with the exact signature
transportShipSpawnTiles(): TileRef[], reuse any existing tile-accessor or helper
predicates (e.g., controlled/owned tiles iterator and any isShore/canSpawn
helpers) if present, and ensure it is public so TransportShipExecution.ts can
call this.attacker.transportShipSpawnTiles() without compilation errors.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/core/execution/TransportShipExecution.ts
🧰 Additional context used
🧬 Code graph analysis (1)
src/core/execution/TransportShipExecution.ts (5)
src/core/game/GameMap.ts (2)
TileRef(3-3)cost(253-255)src/core/game/UnitImpl.ts (1)
tile(178-180)src/core/game/TrainStation.ts (2)
tile(141-143)cost(184-186)src/core/game/GameImpl.ts (1)
cost(917-919)src/core/pathfinding/MiniAStar.ts (1)
cost(17-24)
🪛 GitHub Actions: 🧪 CI
src/core/execution/TransportShipExecution.ts
[error] 116-116: TS2551: Property 'transportShipSpawnTiles' does not exist on type 'Player'. Did you mean 'bestTransportShipSpawn'?
🪛 GitHub Check: 🔬 Test
src/core/execution/TransportShipExecution.ts
[failure] 116-116: tests/Disconnected.test.ts > Disconnected > Disconnected team member interactions > Retreating transport ship is deleted if new owner has no shore tiles
TypeError: this.attacker.transportShipSpawnTiles is not a function
❯ TransportShipExecution.init src/core/execution/TransportShipExecution.ts:116:41
❯ src/core/game/GameImpl.ts:373:11
❯ GameImpl.executeNextTick src/core/game/GameImpl.ts:371:22
❯ executeTicks tests/util/utils.ts:34:10
❯ tests/Disconnected.test.ts:435:7
[failure] 116-116: tests/Disconnected.test.ts > Disconnected > Disconnected team member interactions > Captured transport ship should retreat to owner's shore tile
TypeError: this.attacker.transportShipSpawnTiles is not a function
❯ TransportShipExecution.init src/core/execution/TransportShipExecution.ts:116:41
❯ src/core/game/GameImpl.ts:373:11
❯ GameImpl.executeNextTick src/core/game/GameImpl.ts:371:22
❯ executeTicks tests/util/utils.ts:34:10
❯ tests/Disconnected.test.ts:397:7
[failure] 116-116: tests/Disconnected.test.ts > Disconnected > Disconnected team member interactions > Captured transport ship landing attack should be in name of new owner
TypeError: this.attacker.transportShipSpawnTiles is not a function
❯ TransportShipExecution.init src/core/execution/TransportShipExecution.ts:116:41
❯ src/core/game/GameImpl.ts:373:11
❯ GameImpl.executeNextTick src/core/game/GameImpl.ts:371:22
❯ executeTicks tests/util/utils.ts:34:10
❯ tests/Disconnected.test.ts:362:7
[failure] 116-116: tests/Attack.test.ts > Attack immunity > Should be able to send a boat after immunity phase
TypeError: this.attacker.transportShipSpawnTiles is not a function
❯ TransportShipExecution.init src/core/execution/TransportShipExecution.ts:116:41
❯ src/core/game/GameImpl.ts:373:11
❯ GameImpl.executeNextTick src/core/game/GameImpl.ts:371:22
❯ tests/Attack.test.ts:440:10
[failure] 116-116: tests/Attack.test.ts > Attack > Boat penalty on retreat Transport Ship arrival
TypeError: this.attacker.transportShipSpawnTiles is not a function
❯ TransportShipExecution.init src/core/execution/TransportShipExecution.ts:116:41
❯ src/core/game/GameImpl.ts:373:11
❯ GameImpl.executeNextTick src/core/game/GameImpl.ts:371:22
❯ tests/Attack.test.ts:138:10
[failure] 116-116: tests/Attack.test.ts > Attack > Nuke reduce attacking boat troop count
TypeError: this.attacker.transportShipSpawnTiles is not a function
❯ TransportShipExecution.init src/core/execution/TransportShipExecution.ts:116:41
❯ src/core/game/GameImpl.ts:373:11
❯ GameImpl.executeNextTick src/core/game/GameImpl.ts:371:22
❯ constructionExecution tests/util/utils.ts:28:10
❯ tests/Attack.test.ts:119:5
| const candidateTiles = this.attacker.transportShipSpawnTiles(); | ||
| let bestSrc: TileRef | null = null; | ||
| let lowestCost = Infinity; | ||
|
|
||
| for (const tile of candidateTiles) { | ||
| const cost = this.pathFinder.estimateCost(tile, this.dst); | ||
| if (cost < lowestCost) { | ||
| lowestCost = cost; | ||
| bestSrc = tile; | ||
| } | ||
| } | ||
|
|
||
| if (!bestSrc) { | ||
| console.warn(`can't find valid transport ship spawn`); | ||
| this.active = false; | ||
| return; | ||
| } | ||
|
|
||
| this.src = bestSrc; |
There was a problem hiding this comment.
Fix indentation: Use spaces consistently.
This code block uses tab indentation while the rest of the file uses 2-space indentation. This violates TypeScript conventions and hurts readability for the team.
🔎 Fix indentation to use 2 spaces
- const candidateTiles = this.attacker.transportShipSpawnTiles();
- let bestSrc: TileRef | null = null;
- let lowestCost = Infinity;
-
- for (const tile of candidateTiles) {
- const cost = this.pathFinder.estimateCost(tile, this.dst);
- if (cost < lowestCost) {
- lowestCost = cost;
- bestSrc = tile;
- }
- }
-
- if (!bestSrc) {
- console.warn(`can't find valid transport ship spawn`);
- this.active = false;
- return;
- }
-
- this.src = bestSrc;
+ const candidateTiles = this.attacker.transportShipSpawnTiles();
+ let bestSrc: TileRef | null = null;
+ let lowestCost = Infinity;
+
+ for (const tile of candidateTiles) {
+ const cost = this.pathFinder.estimateCost(tile, this.dst);
+ if (cost < lowestCost) {
+ lowestCost = cost;
+ bestSrc = tile;
+ }
+ }
+
+ if (!bestSrc) {
+ console.warn(`can't find valid transport ship spawn`);
+ this.active = false;
+ return;
+ }
+
+ this.src = bestSrc;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const candidateTiles = this.attacker.transportShipSpawnTiles(); | |
| let bestSrc: TileRef | null = null; | |
| let lowestCost = Infinity; | |
| for (const tile of candidateTiles) { | |
| const cost = this.pathFinder.estimateCost(tile, this.dst); | |
| if (cost < lowestCost) { | |
| lowestCost = cost; | |
| bestSrc = tile; | |
| } | |
| } | |
| if (!bestSrc) { | |
| console.warn(`can't find valid transport ship spawn`); | |
| this.active = false; | |
| return; | |
| } | |
| this.src = bestSrc; | |
| const candidateTiles = this.attacker.transportShipSpawnTiles(); | |
| let bestSrc: TileRef | null = null; | |
| let lowestCost = Infinity; | |
| for (const tile of candidateTiles) { | |
| const cost = this.pathFinder.estimateCost(tile, this.dst); | |
| if (cost < lowestCost) { | |
| lowestCost = cost; | |
| bestSrc = tile; | |
| } | |
| } | |
| if (!bestSrc) { | |
| console.warn(`can't find valid transport ship spawn`); | |
| this.active = false; | |
| return; | |
| } | |
| this.src = bestSrc; |
🧰 Tools
🪛 GitHub Actions: 🧪 CI
[error] 116-116: TS2551: Property 'transportShipSpawnTiles' does not exist on type 'Player'. Did you mean 'bestTransportShipSpawn'?
🪛 GitHub Check: 🔬 Test
[failure] 116-116: tests/Disconnected.test.ts > Disconnected > Disconnected team member interactions > Retreating transport ship is deleted if new owner has no shore tiles
TypeError: this.attacker.transportShipSpawnTiles is not a function
❯ TransportShipExecution.init src/core/execution/TransportShipExecution.ts:116:41
❯ src/core/game/GameImpl.ts:373:11
❯ GameImpl.executeNextTick src/core/game/GameImpl.ts:371:22
❯ executeTicks tests/util/utils.ts:34:10
❯ tests/Disconnected.test.ts:435:7
[failure] 116-116: tests/Disconnected.test.ts > Disconnected > Disconnected team member interactions > Captured transport ship should retreat to owner's shore tile
TypeError: this.attacker.transportShipSpawnTiles is not a function
❯ TransportShipExecution.init src/core/execution/TransportShipExecution.ts:116:41
❯ src/core/game/GameImpl.ts:373:11
❯ GameImpl.executeNextTick src/core/game/GameImpl.ts:371:22
❯ executeTicks tests/util/utils.ts:34:10
❯ tests/Disconnected.test.ts:397:7
[failure] 116-116: tests/Disconnected.test.ts > Disconnected > Disconnected team member interactions > Captured transport ship landing attack should be in name of new owner
TypeError: this.attacker.transportShipSpawnTiles is not a function
❯ TransportShipExecution.init src/core/execution/TransportShipExecution.ts:116:41
❯ src/core/game/GameImpl.ts:373:11
❯ GameImpl.executeNextTick src/core/game/GameImpl.ts:371:22
❯ executeTicks tests/util/utils.ts:34:10
❯ tests/Disconnected.test.ts:362:7
[failure] 116-116: tests/Attack.test.ts > Attack immunity > Should be able to send a boat after immunity phase
TypeError: this.attacker.transportShipSpawnTiles is not a function
❯ TransportShipExecution.init src/core/execution/TransportShipExecution.ts:116:41
❯ src/core/game/GameImpl.ts:373:11
❯ GameImpl.executeNextTick src/core/game/GameImpl.ts:371:22
❯ tests/Attack.test.ts:440:10
[failure] 116-116: tests/Attack.test.ts > Attack > Boat penalty on retreat Transport Ship arrival
TypeError: this.attacker.transportShipSpawnTiles is not a function
❯ TransportShipExecution.init src/core/execution/TransportShipExecution.ts:116:41
❯ src/core/game/GameImpl.ts:373:11
❯ GameImpl.executeNextTick src/core/game/GameImpl.ts:371:22
❯ tests/Attack.test.ts:138:10
[failure] 116-116: tests/Attack.test.ts > Attack > Nuke reduce attacking boat troop count
TypeError: this.attacker.transportShipSpawnTiles is not a function
❯ TransportShipExecution.init src/core/execution/TransportShipExecution.ts:116:41
❯ src/core/game/GameImpl.ts:373:11
❯ GameImpl.executeNextTick src/core/game/GameImpl.ts:371:22
❯ constructionExecution tests/util/utils.ts:28:10
❯ tests/Attack.test.ts:119:5
🤖 Prompt for AI Agents
In @src/core/execution/TransportShipExecution.ts around lines 116 - 134, The
block that finds a spawn tile (using candidateTiles from
attacker.transportShipSpawnTiles(), the loop over tile, pathFinder.estimateCost,
and setting this.src) is indented with tabs instead of the project's 2-space
style; replace all tabs in this block with consistent 2-space indentation so the
for loop, if blocks, and assignments align with the rest of the file and
TypeScript conventions, ensuring candidateTiles, bestSrc, lowestCost, the for
(const tile ...) loop, the if (!bestSrc) branch, and the final this.src =
bestSrc; lines all use 2 spaces per indent level.
| this.active = false; | ||
| return; | ||
| } | ||
| const candidateTiles = this.attacker.transportShipSpawnTiles(); |
There was a problem hiding this comment.
Critical: Method transportShipSpawnTiles() does not exist.
The call to this.attacker.transportShipSpawnTiles() fails compilation and causes all tests to crash. The Player type does not have this method defined.
You need to implement transportShipSpawnTiles(): TileRef[] on the Player class to return all valid shore tiles controlled by the player where a transport ship can spawn.
Would you like me to generate a shell script to search for similar methods on the Player class that could serve as a reference for implementing this?
🧰 Tools
🪛 GitHub Actions: 🧪 CI
[error] 116-116: TS2551: Property 'transportShipSpawnTiles' does not exist on type 'Player'. Did you mean 'bestTransportShipSpawn'?
🪛 GitHub Check: 🔬 Test
[failure] 116-116: tests/Disconnected.test.ts > Disconnected > Disconnected team member interactions > Retreating transport ship is deleted if new owner has no shore tiles
TypeError: this.attacker.transportShipSpawnTiles is not a function
❯ TransportShipExecution.init src/core/execution/TransportShipExecution.ts:116:41
❯ src/core/game/GameImpl.ts:373:11
❯ GameImpl.executeNextTick src/core/game/GameImpl.ts:371:22
❯ executeTicks tests/util/utils.ts:34:10
❯ tests/Disconnected.test.ts:435:7
[failure] 116-116: tests/Disconnected.test.ts > Disconnected > Disconnected team member interactions > Captured transport ship should retreat to owner's shore tile
TypeError: this.attacker.transportShipSpawnTiles is not a function
❯ TransportShipExecution.init src/core/execution/TransportShipExecution.ts:116:41
❯ src/core/game/GameImpl.ts:373:11
❯ GameImpl.executeNextTick src/core/game/GameImpl.ts:371:22
❯ executeTicks tests/util/utils.ts:34:10
❯ tests/Disconnected.test.ts:397:7
[failure] 116-116: tests/Disconnected.test.ts > Disconnected > Disconnected team member interactions > Captured transport ship landing attack should be in name of new owner
TypeError: this.attacker.transportShipSpawnTiles is not a function
❯ TransportShipExecution.init src/core/execution/TransportShipExecution.ts:116:41
❯ src/core/game/GameImpl.ts:373:11
❯ GameImpl.executeNextTick src/core/game/GameImpl.ts:371:22
❯ executeTicks tests/util/utils.ts:34:10
❯ tests/Disconnected.test.ts:362:7
[failure] 116-116: tests/Attack.test.ts > Attack immunity > Should be able to send a boat after immunity phase
TypeError: this.attacker.transportShipSpawnTiles is not a function
❯ TransportShipExecution.init src/core/execution/TransportShipExecution.ts:116:41
❯ src/core/game/GameImpl.ts:373:11
❯ GameImpl.executeNextTick src/core/game/GameImpl.ts:371:22
❯ tests/Attack.test.ts:440:10
[failure] 116-116: tests/Attack.test.ts > Attack > Boat penalty on retreat Transport Ship arrival
TypeError: this.attacker.transportShipSpawnTiles is not a function
❯ TransportShipExecution.init src/core/execution/TransportShipExecution.ts:116:41
❯ src/core/game/GameImpl.ts:373:11
❯ GameImpl.executeNextTick src/core/game/GameImpl.ts:371:22
❯ tests/Attack.test.ts:138:10
[failure] 116-116: tests/Attack.test.ts > Attack > Nuke reduce attacking boat troop count
TypeError: this.attacker.transportShipSpawnTiles is not a function
❯ TransportShipExecution.init src/core/execution/TransportShipExecution.ts:116:41
❯ src/core/game/GameImpl.ts:373:11
❯ GameImpl.executeNextTick src/core/game/GameImpl.ts:371:22
❯ constructionExecution tests/util/utils.ts:28:10
❯ tests/Attack.test.ts:119:5
🤖 Prompt for AI Agents
In @src/core/execution/TransportShipExecution.ts at line 116, The call to
this.attacker.transportShipSpawnTiles() fails because Player lacks
transportShipSpawnTiles(); add a method transportShipSpawnTiles(): TileRef[] to
the Player class that returns all shore tiles controlled by the player which are
valid spawn locations for a transport ship (i.e., filter the player's tile
collection for tiles owned/controlled by this player that meet the "shore" and
"spawnable" criteria). Implement the method with the exact signature
transportShipSpawnTiles(): TileRef[], reuse any existing tile-accessor or helper
predicates (e.g., controlled/owned tiles iterator and any isShore/canSpawn
helpers) if present, and ensure it is public so TransportShipExecution.ts can
call this.attacker.transportShipSpawnTiles() without compilation errors.
github-project-automation
bot
moved this from Triage
to Development
in OpenFront Release Management
DevelopingTom
left a comment
DevelopingTom
left a comment
There was a problem hiding this comment.
It looks like it's missing a few important files. Could you update your PR with those?
Thanks!
4 participants
Currently, when sending a transport ship to a target territory, the game selects the spawn tile that is closest to the target in straight-line distance from the player's territory. This can result in transport ships taking longer water routes, causing troops to arrive later than necessary.
This PR refactors the transport ship spawning logic to select the source tile that allows the ship to reach the target in the shortest estimated travel time over water, using the pathfinder to estimate routes.
Key points:
Evaluates all eligible shore tiles controlled by the player.
Estimates the path length or travel time from each tile to the destination.
Chooses the tile that minimizes the expected arrival time of the transport ship.
This ensures transport ships are sent efficiently and troops arrive at the target faster, even if the chosen tile is not the geographically closest territory.
Checklist:
I have added screenshots for all UI updates (not applicable here)
I process any text displayed to the user through translateText() and I've added it to the en.json file (not applicable)
I have added relevant tests to the test directory (No)
I confirm I have thoroughly tested these changes and take full responsibility for any bugs introduced
Discord contact: exudiz