Skip to content

Fix/warship freezing no path #4151

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
evanpelle merged 6 commits into from
Jun 5, 2026
+38 −0
Merged

Fix/warship freezing no path #4151

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
7692be2
Fix warship freezing when PatrolTile's proximity is unreachable from …
Katokoda Jun 3, 2026
a7f46ef
Add a test verifying that warship rejects new PatrolTile when in a di…
Katokoda Jun 3, 2026
bbfced6
Move the new warship test within the Warship-Description as pointed o…
Katokoda Jun 4, 2026
56f91de
Merge branch 'main' into fix/warship-freezing-no-path
Katokoda Jun 4, 2026
926675d
Removes redundant test of warship being on water as recommended by evan
Katokoda Jun 5, 2026
e889f7d
Merge branch 'fix/warship-freezing-no-path' of github.com:Katokoda/Op…
Katokoda Jun 5, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 10 additions & 0 deletions src/core/execution/MoveWarshipExecution.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,12 @@ export class MoveWarshipExecution implements Execution {
console.warn(`MoveWarshipExecution: position ${this.position} not valid`);
return;
}
// Get water component of new TargetTile for connectivity check
const newPatrolTileWaterComponent = mg.getWaterComponent(this.position);
if (newPatrolTileWaterComponent === null) {
console.warn(`MoveWarshipExecution: position ${this.position} not water`);
return;
}
Copy link
Copy Markdown
Collaborator

@evanpelle evanpelle Jun 4, 2026

Choose a reason for hiding this comment

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

do we need this check, since a warship should always be in a water component because it must always be on a water tile?

Copy link
Copy Markdown
Contributor Author

@Katokoda Katokoda Jun 4, 2026

Choose a reason for hiding this comment

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

Without it, the variable newPatrolTileWaterComponent is of type (number | null); which triggers an error in:
if (!mg.hasWaterComponent(warship.tile(), newPatrolTileWaterComponent)).
I could have a test within the loop as does src/core/execution/WarshipExecution.ts :

      // Check water component connectivity
      if (
        warshipComponent !== null &&
        !this.mg.hasWaterComponent(tile, warshipComponent)
      ) {

but it seems less clear, doesn't let me put a warning in there and may be less performant because the test is within each iteration.

However, I am entirely new to Typescript, maybe there is a better way to simply indicate "we know this can't be null".

Copy link
Copy Markdown
Collaborator

@evanpelle evanpelle Jun 4, 2026

Choose a reason for hiding this comment

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

You could do:

newPatrolTileWaterComponent! to force it to true.

Another option is to throw an error to crash the game if the watercomponent is null, since that should never happen.

Copy link
Copy Markdown
Contributor Author

@Katokoda Katokoda Jun 5, 2026

Choose a reason for hiding this comment

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

I did the !-trick. It seem to have the same effect (would crash the game) but I do not have the control over the error.

// Cache warship list and build a lookup map — avoids repeated iteration
const warshipMap = new Map(
this.owner.units(UnitType.Warship).map((u) => [u.id(), u]),
Expand All @@ -28,6 +34,10 @@ export class MoveWarshipExecution implements Execution {
console.warn(`MoveWarshipExecution: warship ${unitId} is not active`);
continue;
}
// Do not update the warship's patrolTile if it is in a different Water Component
if (!mg.hasWaterComponent(warship.tile(), newPatrolTileWaterComponent)) {
continue;
}
warship.updateWarshipState({
patrolTile: this.position,
});
Expand Down
32 changes: 32 additions & 0 deletions tests/Warship.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -915,4 +915,36 @@ describe("Warship", () => {
}
expect(tradeShip.owner()).toBe(player1);
});

test("Warship doesn't accept a new patrol tile if in a different water component", async () => {
const newPatrolTile = game.ref(coastX + 5, 15);

const warship = player1.buildUnit(
UnitType.Warship,
game.ref(coastX + 1, 10),
{
patrolTile: game.ref(coastX + 1, 10),
},
);

game.addExecution(new WarshipExecution(warship));

// Mock different water components
game.getWaterComponent = (tile: TileRef) => {
if (tile === newPatrolTile) return 1;
return 2;
};

game.hasWaterComponent = (tile: TileRef, component: number) => {
return game.getWaterComponent(tile) === component;
};

game.addExecution(
new MoveWarshipExecution(player1, [warship.id()], newPatrolTile),
);

executeTicks(game, 10);

expect(warship.warshipState().patrolTile).toBe(game.ref(coastX + 1, 10));
});
});
Loading