AFK team mate: better ship handling + tests + bugfix #2203
Conversation
|
|
Pre-merge checks❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
🧰 Additional context used🧠 Learnings (8)📓 Common learnings📚 Learning: 2025-10-18T17:54:01.311ZApplied to files:
📚 Learning: 2025-10-20T20:15:28.858ZApplied to files:
📚 Learning: 2025-10-27T09:47:26.395ZApplied to files:
📚 Learning: 2025-10-20T11:02:16.969ZApplied to files:
📚 Learning: 2025-10-08T17:14:49.369ZApplied to files:
📚 Learning: 2025-08-29T16:16:11.309ZApplied to files:
📚 Learning: 2025-08-28T22:47:31.406ZApplied to files:
🧬 Code graph analysis (2)tests/Disconnected.test.ts (9)
tests/util/TestConfig.ts (3)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
src/core/execution/WarshipExecution.ts (1)
92-94: Targeting: isFriendly inversion looks rightThis should keep allied/disconnected owners from being targeted. Consider adding a test for a disconnected friendly owner to lock behavior. Also, for consistency, mirror this direction in the trade‑ship path (friend check) elsewhere in the file.
src/core/game/GameImpl.ts (1)
880-893: AFK teammate ship transfer: good; simplify unit selection and confirm ordering
- Logic matches the design: transfer ships only when conquered is disconnected and on the same team.
- Minor: use the typed
units(...types)helper.- const ships = conquered - .units() - .filter( - (u) => - u.type() === UnitType.Warship || - u.type() === UnitType.TransportShip, - ); + const ships = conquered.units(UnitType.Warship, UnitType.TransportShip);Please also confirm this executes before any routine that deletes conquered units, so transfers are not lost.
tests/Attack.test.ts (1)
24-31: Consider adding tests for the new ownership transfer logic.The PR introduces ownership transfer of transport ships when the original owner disconnects and a teammate conquers them. Consider adding test cases covering:
- Transport ship ownership transfer when original owner disconnects and same-team player conquers
- Transport ship deletion when non-teammate conquers disconnected player
- Proper updating of the
attackerfield after ownership transferExample test structure:
test("Transport ship transfers to conquering teammate when original owner disconnects", async () => { // Set up two players on same team // Player A sends transport ship // Player A disconnects // Player B (teammate) conquers Player A // Verify ship ownership transfers to Player B // Verify ship continues to destination under new owner });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
src/core/execution/ExecutionManager.ts(1 hunks)src/core/execution/FakeHumanExecution.ts(2 hunks)src/core/execution/TransportShipExecution.ts(2 hunks)src/core/execution/WarshipExecution.ts(1 hunks)src/core/game/GameImpl.ts(1 hunks)tests/Attack.test.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
src/core/execution/ExecutionManager.ts (2)
src/core/game/GameImpl.ts (1)
player(454-460)src/core/game/GameView.ts (1)
player(589-595)
tests/Attack.test.ts (1)
src/core/execution/TransportShipExecution.ts (1)
TransportShipExecution(17-254)
src/core/execution/TransportShipExecution.ts (1)
src/core/game/Game.ts (1)
Player(517-651)
🔇 Additional comments (3)
src/core/execution/FakeHumanExecution.ts (1)
394-401: LGTM on passing originalOwner to TransportShipExecutionBoth call sites correctly pass
this.playeras the new final arg; null-safety is ensured by earlier guards.Please confirm there are no remaining call sites still using the old 5‑arg signature.
Also applies to: 595-603
src/core/execution/ExecutionManager.ts (1)
72-79: Boat intent call site updated correctlyPassing
playerasoriginalOwneris correct here.Double‑check there are no other constructor invocations left unupdated (see script in TransportShipExecution comment).
tests/Attack.test.ts (1)
24-31: Constructor signature updated correctly.The additional
originalOwnerparameter is passed as expected. In the normal case, the player sending the boat (defender) is both the attacker and the original owner.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/core/execution/TransportShipExecution.ts(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/core/execution/TransportShipExecution.ts (1)
src/core/game/Game.ts (1)
Player(517-651)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Has Milestone
- GitHub Check: Validate Description
- GitHub Check: Deploy to openfront.dev
🔇 Additional comments (2)
src/core/execution/TransportShipExecution.ts (2)
42-42: LGTM!The new
originalOwnerparameter cleanly tracks the original owner for ownership transfer scenarios.
225-227: Verify stats attribution after ownership transfer.After ownership handoff,
boatArriveTroopsuses the newattacker(lines 227, 250), butboatSendTroopsused the originalattacker(line 157). This splits attribution between the original sender and the new owner.If intentional, document this behavior. Otherwise, consider:
- Emitting a separate transfer stat when ownership changes
- Preserving the original sender attribution in arrival stats
Based on past review comments.
Also applies to: 248-250
|
What if the conquering player already has three transports active? |
|
@TheGiraffe3 they keep their own transport ships too. Basically the situation for the team stays much more the same, which is more fair since they can't help if they team mate goes AFK |
|
So, in certain cases, you can have more than three transports at a time? |
|
@TheGiraffe3 You're asking the right questions. But yes in this specific case you can. The boatMaxNumber is only checked for canBuild via TransportShipUtils, and then in the init of TransportShipExecution. Not after that when the ownership changes. Transfering them to random team members who would have room left under boatMaxNumber, would be a bit expensive i think compared to what we're trying to solve. Also the new owner can't build new transport ships if he's above max and needs to wait until the boats land just like normal. |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/core/execution/TransportShipExecution.ts (1)
191-200: Use bestTransportShipSpawn for retreat src; avoid build-time limits.Retreat only needs “a valid shore for current owner”, not “can build now”. canBuild may include limits (max boats, gold), which could wrongly null src and delete the boat. Prefer bestTransportShipSpawn.
Apply this diff:
- // Ensure retreat source is valid for the new owner - if (this.mg.owner(this.src!) !== this.attacker) { - const newSrc = this.attacker.canBuild(UnitType.TransportShip, this.dst); - if (newSrc === false) { - this.src = null; - } else { - this.src = newSrc; - } - } + // Ensure retreat source is valid for the new owner + const newSrc = this.attacker.bestTransportShipSpawn(this.dst); + if (newSrc === false) { + this.src = null; + } else { + this.src = newSrc; + }Quick check to verify whether canBuild enforces boatMaxNumber or resource gates:
#!/bin/bash # Locate canBuild implementation and references to boatMaxNumber within it rg -n -C3 -P 'class\\s+.+\\bimplements\\b.+Player|interface\\s+Player\\b' --type=ts rg -n -C3 -P '\\bcanBuild\\s*\\(' --type=ts rg -n -C3 -P 'boatMaxNumber\\s*\\(' --type=ts
🧹 Nitpick comments (2)
src/core/execution/TransportShipExecution.ts (2)
36-37: Rename originalOwner to reflect rolling “handoff anchor”.You reassign originalOwner after a teammate capture, so it stops being the true “original” owner. Rename for clarity (e.g., ownerAtLastHandoff). No behavior change.
Apply this diff in this file:
- private originalOwner: Player; + private ownerAtLastHandoff: Player; @@ - ) { - this.originalOwner = this.attacker; - } + ) { + this.ownerAtLastHandoff = this.attacker; + } @@ - if ( - this.originalOwner.isDisconnected() && - this.boat.owner() !== this.originalOwner && - this.boat.owner().isOnSameTeam(this.originalOwner) - ) { - this.attacker = this.boat.owner(); - this.originalOwner = this.boat.owner(); // for when this owner disconnects too - } + if ( + this.ownerAtLastHandoff.isDisconnected() && + this.boat.owner() !== this.ownerAtLastHandoff && + this.boat.owner().isOnSameTeam(this.ownerAtLastHandoff) + ) { + this.attacker = this.boat.owner(); + this.ownerAtLastHandoff = this.boat.owner(); // for chained handoffs + }Also applies to: 44-46, 180-189
234-245: Align “friendly” check with WarshipExecution to avoid AFK edge cases.Warship moved off isFriendly due to AFK behavior. Mirror it here to prevent accidental attacks against disconnected teammates/allies.
- if (this.target.isPlayer() && this.attacker.isFriendly(this.target)) { + if ( + this.target.isPlayer() && + (this.attacker.isOnSameTeam(this.target) || + this.attacker.isAlliedWith(this.target)) + ) {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/core/execution/TransportShipExecution.ts(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/core/execution/TransportShipExecution.ts (2)
src/core/game/Game.ts (2)
Player(518-652)PlayerID(21-21)src/core/game/GameMap.ts (1)
TileRef(3-3)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Deploy to openfront.dev
🔇 Additional comments (2)
src/core/execution/TransportShipExecution.ts (2)
180-189: Confirm stats attribution and message consistency after handoff.Send is attributed to original attacker (init), arrival to the new attacker after handoff. If this split is intended, good; if not, consider emitting a transfer stat or re-attributing send. Also check if the “Naval invasion incoming from …” message should be updated/cancelled when ownership changes mid‑voyage.
202-215: Retreat flow reads well; side-effects are correct.Refund troops to current attacker, delete unit, deactivate, or retarget to src. Looks solid.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tests/util/TestConfig.ts (1)
80-102: Rename the subclass for PascalCase clarity.Please rename
useRealAttackLogictoUseRealAttackLogic(and update imports/exports) so type names stay PascalCase and easy to spot.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/core/execution/AttackExecution.ts(1 hunks)tests/Disconnected.test.ts(2 hunks)tests/util/Setup.ts(2 hunks)tests/util/TestConfig.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/core/execution/AttackExecution.ts
🧰 Additional context used
🧠 Learnings (7)
📓 Common learnings
Learnt from: Foorack
Repo: openfrontio/OpenFrontIO PR: 2141
File: src/client/ClientGameRunner.ts:228-234
Timestamp: 2025-10-08T17:14:49.369Z
Learning: For the window close confirmation feature in `ClientGameRunner.ts`, the troop count requirement (>10,000 troops) from issue #2137 was intentionally removed because it was arbitrary and troop count can be reported as low despite having significant land. The confirmation now triggers for any alive player regardless of troop count.
📚 Learning: 2025-10-26T15:37:07.732Z
Learnt from: GlacialDrift
Repo: openfrontio/OpenFrontIO PR: 2298
File: src/client/graphics/layers/TerritoryLayer.ts:200-210
Timestamp: 2025-10-26T15:37:07.732Z
Learning: In GameImpl.ts lines 124-139, team assignment logic varies by number of teams: when numPlayerTeams < 8, teams are assigned ColoredTeams values (Red, Blue, Yellow, Green, Purple, Orange, Teal); when numPlayerTeams >= 8, teams are assigned generic string identifiers like "Team 1", "Team 2", etc., which are not members of ColoredTeams.
Applied to files:
tests/Disconnected.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/Disconnected.test.tstests/util/TestConfig.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/Disconnected.test.tstests/util/TestConfig.ts
📚 Learning: 2025-08-28T22:47:31.406Z
Learnt from: BrewedCoffee
Repo: openfrontio/OpenFrontIO PR: 1957
File: tests/core/executions/PlayerExecution.test.ts:16-39
Timestamp: 2025-08-28T22:47:31.406Z
Learning: In test files, PlayerExecution instances must be manually registered with game.addExecution() because the setup utility doesn't automatically register SpawnExecution, which would normally handle this during the spawn phase in a real game.
Applied to files:
tests/Disconnected.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/Disconnected.test.tstests/util/TestConfig.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/util/TestConfig.ts
🧬 Code graph analysis (3)
tests/Disconnected.test.ts (9)
src/core/game/Game.ts (3)
Game(653-737)Player(515-651)PlayerInfo(399-413)tests/util/Setup.ts (1)
setup(23-81)tests/util/TestConfig.ts (1)
useRealAttackLogic(80-102)src/core/execution/SpawnExecution.ts (1)
SpawnExecution(7-61)src/core/execution/WarshipExecution.ts (1)
WarshipExecution(16-282)tests/util/utils.ts (1)
executeTicks(32-36)src/core/execution/AttackExecution.ts (1)
AttackExecution(18-375)src/core/Util.ts (1)
toInt(253-261)src/core/execution/TransportShipExecution.ts (1)
TransportShipExecution(17-278)
tests/util/Setup.ts (3)
tests/util/TestConfig.ts (1)
TestConfig(12-79)src/core/game/GameView.ts (1)
config(640-642)src/core/game/GameImpl.ts (1)
config(318-320)
tests/util/TestConfig.ts (3)
src/core/game/Game.ts (3)
Game(653-737)Player(515-651)TerraNullius(502-507)src/core/game/GameMap.ts (1)
TileRef(3-3)src/core/configuration/DefaultConfig.ts (1)
DefaultConfig(219-961)
🪛 GitHub Actions: 🧪 CI
tests/Disconnected.test.ts
[warning] 1-1: Code style issues found in the file. Run 'npx prettier --write' to fix.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Deploy to openfront.dev
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/Disconnected.test.ts(2 hunks)
🧰 Additional context used
🧠 Learnings (6)
📓 Common learnings
Learnt from: Foorack
Repo: openfrontio/OpenFrontIO PR: 2141
File: src/client/ClientGameRunner.ts:228-234
Timestamp: 2025-10-08T17:14:49.369Z
Learning: For the window close confirmation feature in `ClientGameRunner.ts`, the troop count requirement (>10,000 troops) from issue #2137 was intentionally removed because it was arbitrary and troop count can be reported as low despite having significant land. The confirmation now triggers for any alive player regardless of troop count.
📚 Learning: 2025-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:
tests/Disconnected.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/Disconnected.test.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/Disconnected.test.ts
📚 Learning: 2025-08-28T22:47:31.406Z
Learnt from: BrewedCoffee
Repo: openfrontio/OpenFrontIO PR: 1957
File: tests/core/executions/PlayerExecution.test.ts:16-39
Timestamp: 2025-08-28T22:47:31.406Z
Learning: In test files, PlayerExecution instances must be manually registered with game.addExecution() because the setup utility doesn't automatically register SpawnExecution, which would normally handle this during the spawn phase in a real game.
Applied to files:
tests/Disconnected.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/Disconnected.test.ts
🧬 Code graph analysis (1)
tests/Disconnected.test.ts (7)
tests/util/Setup.ts (1)
setup(23-81)tests/util/TestConfig.ts (1)
useRealAttackLogic(80-102)src/core/execution/WarshipExecution.ts (1)
WarshipExecution(16-282)tests/util/utils.ts (1)
executeTicks(32-36)src/core/execution/AttackExecution.ts (1)
AttackExecution(18-375)src/core/Util.ts (1)
toInt(253-261)src/core/execution/TransportShipExecution.ts (1)
TransportShipExecution(17-278)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Deploy to openfront.dev
VariableVince
changed the title
…/OpenFrontIO into warship-afk-handling
Description:
Have AFK player's Warships not attack team members ships, like Transport Ships boating in. If team mate conquers the AFK player, transfer over Warships and Transport Ships to conqueror. The transfered Transport ships attack in the name of the new owner when landing, and when they are retreated they move back to a new owner shore tile if they have any.
Added tests. Expectation is this PR will be merged in v26 as the real solution for the temporary workaround of deleting warships.
Currently:
(Left to think about: do we want to transfer part of the defender troops to the isOnSameTeam attacker? Defender looses less troops in the attack from their team mate. You'd expect troops to lay down their arms mostly, if the attacker is on the same team and doesn't loose troops themselves. Those troops that they loose less than normal, are then added to the attacker once they've been deemed conqueror. The enemy team can still attack and do normal damage, and can still also be the conqueror so the team members have to be fast.)
Changes in this PR:
GameImpl > conquerPlayer: Transfer ownership of the warships to the conquerer of their lands. If the conqueror is not a team member (other team can still attack, in their case with troop loss), they won't get the warships and the ships will be deleted like normal. If the conqueror is a team member, have them capture the warships.
(Captures need to happen in conquerPlayer since this is right before the last tiles are conquered and PlayerExecution finds out the player is dead and deletes its units. Captures will be recorded in the stats just like normal. Things like this add an extra incentive to be the fastest to conquer the AFK player, next to getting their gold which is also recorded in stats. The normal Event Box messages are also displayed.)
GameImpl > conquerPlayer: Also transfer Transport Ships. As a note: the limit of 3 transport ships concurrently out on water for one player, can be exceeded in this specific case (the boatMaxNumber is only checked for canBuild via TransportShipUtils, and in the init of a TransportShipExecution, not for an existing TransportShipExecution with a changing owner). This keeps the situation even for the team in terms of ships that are already out to attack, which is fair. captureUnit/setOwner won't do the full job here though, so changes in TransportShipExecution are needed.
TransportShipExecution: Added originalOwner. So we can check within the execution itself if the Original owner disconnected, and if so if the new owner is on the same team. Only in that case change private this.attacker. This.attacker can still not be changed from the outside in this way. Find new src of new owner to retreat boat to if needed, and if new owner has no shore set it to null so the boat will be deleted upon retreat. To find new src tile of new owner, use bestTransportShipSpawn instead of canBuild because canBuild checks for max boats = 3 etcera but the boat is already on its way so those checks don't apply (we could get false back from canBuild because there's 3 ships out, while we only need to find the source tile so use bestTransportShipSpawn).
TransportShipUtils: to use bestTransportShipSpawn to find new owner source tile to retreat to, we need to make sure it can handle a new owner without shore tiles. When the new owner has no shore tiles (candidates.length === 0), return false. This way it won't go on to call MiniAStar which would have SerialAStar error on an empty this.sources array.
WarshipExecution: Changed isFriendly. This makes sure we have the wanted behavior: allied/team ships should not attack each other once one of their owners goes AFK.
AttackExecution: added one more test specifically to check if attack on AFK teammate is still witthout troop loss "Player can attack disconnected team mate without troop loss". Also a bugfix that I left in after removing a related change from this PR: Add a check for removeTroops === false in the retreat() function, so at the end of the attack we don't add troops back to owner troops if they were never removed in the init. This check in retreat() is actually a bug fix because removeTroops is in the constructor and can be set to True, but in retreat() troops would then have been given back after not being removed at init.
DefaultConfig: small addition to comment.
Disconnected.test.ts: added tests. Added useRealAttackLogic because at least "Player can attack disconnected team mate without troop loss" needs to use the real attackLogic.
TestConfig: new class useRealAttackLogic extends TestConfig class, so a test setup can use the real attackLogic from DefaultConfig instead of the mock function in TestConfig.
Setup.ts: for the test setup, add parameter to accept useRealAttackLogic extension class. Defaults to TestConfig.
Please complete the following:
Please put your Discord username so you can be contacted if a bug or regression is found:
tryout33