Tradeship performance (#1448)

## Description:

Small performance improvements for tradeship execution. With the new cap
of 100 tradeships in total, the impact is smaller but still there.

Also added jlrouillard's (Vivacious Box) test with permission from PR
#1449. Tests 100% passed.

- Save tradeship owner, destination port owner and tradeship current
tile in constants for re-use.
- Added check if wasCaptured was already set to true, before comparing
origOwner and tradeship owner and setting wasCaptured to true.
Currently, wasCaptured is set to true once and never back to false
again. This PR keeps it that way but a but faster.
- One less call to pathfinder needed: before calling pathfinder, check
if current tile is destination port tile, and if so call complete. This
is basically the same as pathfinder would do, but less complex without
need for e.g. manhattan distance.
- case PathFindResultType.Completed: isn't needed anymore because of the
above. Kept as fallback. But moved it down in the switch statement, as
there's a low(er) chance the case will ever be true.
- Test if we need to find a new destination port for captured ship:
currently, if this.wasCaptured is true, it will look for a new
destination port each tick again and again. With this PR, only do this
if destination port isn't owned by capturer, or if that port has become
inactive. In tests with thousands of tradeships, went down from 300K+
times setting 'new' port, to only 1250 calls.

**Q:** Will this code work for ships that are captured again, after
being captured already?
**A:** Yes. After wasCaptured is set to true once (after first capture),
we only need to check each tick if the destination port is owned by the
current tradeship owner, to know if we need to assign a new destination
port again.

## Please complete the following:

- [x] I have added screenshots for all UI updates
- [x] I process any text displayed to the user through translateText()
and I've added it to the en.json file
- [x] I have added relevant tests to the test directory
- [x] I confirm I have thoroughly tested these changes and take full
responsibility for any bugs introduced
- [x] I understand that submitting code with bugs that could have been
caught through manual testing blocks releases and new features for all
contributors

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

tryout33

src/core/execution/TradeShipExecution.ts

@@ -54,14 +54,16 @@ export class TradeShipExecution implements Execution {
       return;
     }
 
-    if (this.origOwner !== this.tradeShip.owner()) {
+    const tradeShipOwner = this.tradeShip.owner();
+    const dstPortOwner = this._dstPort.owner();
+    if (this.wasCaptured !== true && this.origOwner !== tradeShipOwner) {
       // Store as variable in case ship is recaptured by previous owner
       this.wasCaptured = true;
     }
 
     // If a player captures another player's port while trading we should delete
     // the ship.
-    if (this._dstPort.owner().id() === this.srcPort.owner().id()) {
+    if (dstPortOwner.id() === this.srcPort.owner().id()) {
       this.tradeShip.delete(false);
       this.active = false;
       return;
@@ -69,15 +71,17 @@ export class TradeShipExecution implements Execution {
 
     if (
       !this.wasCaptured &&
-      (!this._dstPort.isActive() ||
-        !this.tradeShip.owner().canTrade(this._dstPort.owner()))
+      (!this._dstPort.isActive() || !tradeShipOwner.canTrade(dstPortOwner))
     ) {
       this.tradeShip.delete(false);
       this.active = false;
       return;
     }
 
-    if (this.wasCaptured) {
+    if (
+      this.wasCaptured &&
+      (tradeShipOwner !== dstPortOwner || !this._dstPort.isActive())
+    ) {
       const ports = this.tradeShip
         .owner()
         .units(UnitType.Port)
@@ -92,18 +96,18 @@ export class TradeShipExecution implements Execution {
       }
     }
 
-    const result = this.pathFinder.nextTile(
-      this.tradeShip.tile(),
-      this._dstPort.tile(),
-    );
+    const curTile = this.tradeShip.tile();
+    if (curTile === this.dstPort()) {
+      this.complete();
+      return;
+    }
+
+    const result = this.pathFinder.nextTile(curTile, this._dstPort.tile());
 
     switch (result.type) {
-      case PathFindResultType.Completed:
-        this.complete();
-        break;
       case PathFindResultType.Pending:
         // Fire unit event to rerender.
-        this.tradeShip.move(this.tradeShip.tile());
+        this.tradeShip.move(curTile);
         break;
       case PathFindResultType.NextTile:
         // Update safeFromPirates status
@@ -113,6 +117,9 @@ export class TradeShipExecution implements Execution {
         this.tradeShip.move(result.node);
         this.tilesTraveled++;
         break;
+      case PathFindResultType.Completed:
+        this.complete();
+        break;
       case PathFindResultType.PathNotFound:
         console.warn("captured trade ship cannot find route");
         if (this.tradeShip.isActive()) {

tests/core/executions/TradeShipExecution.test.ts

@@ -0,0 +1,122 @@
+import { TradeShipExecution } from "../../../src/core/execution/TradeShipExecution";
+import { Game, Player, Unit } from "../../../src/core/game/Game";
+import { setup } from "../../util/Setup";
+
+describe("TradeShipExecution", () => {
+  let game: Game;
+  let origOwner: Player;
+  let dstOwner: Player;
+  let pirate: Player;
+  let srcPort: Unit;
+  let piratePort: Unit;
+  let tradeShip: Unit;
+  let dstPort: Unit;
+  let tradeShipExecution: TradeShipExecution;
+
+  beforeEach(async () => {
+    // Mock Game, Player, Unit, and required methods
+
+    game = await setup("ocean_and_land", {
+      infiniteGold: true,
+      instantBuild: true,
+    });
+    game.displayMessage = jest.fn();
+    origOwner = {
+      canBuild: jest.fn(() => true),
+      buildUnit: jest.fn((type, spawn, opts) => tradeShip),
+      displayName: jest.fn(() => "Origin"),
+      addGold: jest.fn(),
+      units: jest.fn(() => [dstPort]),
+      unitCount: jest.fn(() => 1),
+      id: jest.fn(() => 1),
+      canTrade: jest.fn(() => true),
+    } as any;
+
+    dstOwner = {
+      id: jest.fn(() => 2),
+      addGold: jest.fn(),
+      displayName: jest.fn(() => "Destination"),
+      units: jest.fn(() => [dstPort]),
+      unitCount: jest.fn(() => 1),
+      canTrade: jest.fn(() => true),
+    } as any;
+
+    pirate = {
+      id: jest.fn(() => 3),
+      addGold: jest.fn(),
+      displayName: jest.fn(() => "Destination"),
+      units: jest.fn(() => [piratePort]),
+      unitCount: jest.fn(() => 1),
+      canTrade: jest.fn(() => true),
+    } as any;
+
+    piratePort = {
+      tile: jest.fn(() => 40011),
+      owner: jest.fn(() => pirate),
+      isActive: jest.fn(() => true),
+    } as any;
+
+    srcPort = {
+      tile: jest.fn(() => 20011),
+      owner: jest.fn(() => origOwner),
+      isActive: jest.fn(() => true),
+    } as any;
+
+    dstPort = {
+      tile: jest.fn(() => 30015), // 15x15
+      owner: jest.fn(() => dstOwner),
+      isActive: jest.fn(() => true),
+    } as any;
+
+    tradeShip = {
+      isActive: jest.fn(() => true),
+      owner: jest.fn(() => origOwner),
+      move: jest.fn(),
+      setTargetUnit: jest.fn(),
+      setSafeFromPirates: jest.fn(),
+      delete: jest.fn(),
+      tile: jest.fn(() => 2001),
+    } as any;
+
+    tradeShipExecution = new TradeShipExecution(origOwner, srcPort, dstPort);
+    tradeShipExecution.init(game, 0);
+    tradeShipExecution["pathFinder"] = {
+      nextTile: jest.fn(() => ({ type: 0, node: 2001 })),
+    } as any;
+    tradeShipExecution["tradeShip"] = tradeShip;
+  });
+
+  it("should initialize and tick without errors", () => {
+    tradeShipExecution.tick(1);
+    expect(tradeShipExecution.isActive()).toBe(true);
+  });
+
+  it("should deactivate if tradeShip is not active", () => {
+    tradeShip.isActive = jest.fn(() => false);
+    tradeShipExecution.tick(1);
+    expect(tradeShipExecution.isActive()).toBe(false);
+  });
+
+  it("should delete ship if port owner changes to current owner", () => {
+    dstPort.owner = jest.fn(() => origOwner);
+    tradeShipExecution.tick(1);
+    expect(tradeShip.delete).toHaveBeenCalledWith(false);
+    expect(tradeShipExecution.isActive()).toBe(false);
+  });
+
+  it("should pick another port if ship is captured", () => {
+    tradeShip.owner = jest.fn(() => pirate);
+    tradeShipExecution.tick(1);
+    expect(tradeShip.setTargetUnit).toHaveBeenCalledWith(piratePort);
+  });
+
+  it("should complete trade and award gold", () => {
+    tradeShipExecution["pathFinder"] = {
+      nextTile: jest.fn(() => ({ type: 2, node: 2001 })),
+    } as any;
+    tradeShipExecution.tick(1);
+    expect(tradeShip.delete).toHaveBeenCalledWith(false);
+    expect(tradeShipExecution.isActive()).toBe(false);
+    expect(game.displayMessage).toHaveBeenCalled();
+  });
+});