From eb51853b050a537888867188edc6387d2c6d48de Mon Sep 17 00:00:00 2001
From: VariableVince <24507472+VariableVince@users.noreply.github.com>
Date: Tue, 24 Mar 2026 00:23:49 +0100
Subject: [PATCH] Perf/Fix: spawn and other functions that need closest by unit
(#3243)
## Description:
Performance improvements.
- **PlayerImpl**: for _nukeSpawn_, cache config to const.
- **Other files**: for nukeSpawn and other functions doing the same,
introduce findClosestBy function.
- for **TradeShipExecution**, with the move from _distSortUnit_ to
_findClosestBy_, also add check if port isActive, !_isMarkedForDeletion_
and !_isUnderConstruction_. These checks should have been there already,
so now do it in one go to make use of the predicate isCandidate in
findClosestBy.
- for **TradeShipExectution.test.ts**, add mock functions for
_isMarkedForDeletion_ and _isUnderConstruction_ because of the above.
Also, set Unit tiles and Pathfinding node to actual valid TileRefs for
the testing map. This prevents NaN as return value from manhattanDist.
This problem was already present with the use of distSortUnit, but that
function just did NaN - NaN, returned the first and only port unit in
the array and called it a day. For findClosestBy we have to make sure
the predicate manhattanDist actually returns a number instead of NaN so
we need actually valid tiles. We now have a working test instead of a
test that actually silently failed like before.
- **PlayerImpl**: _warshipSpawn_ and _nukeSpawn_: Make use of the
isCandidate predicate of findClosestBy to have warshipSpawn not return
ports under construction or (smaller change) inactive. This fixes a bug
i have seen right away (where Warship spawns from under construction
Port).
Same for _nukeSpawn_ silos, don't return inactive silo just to be sure
now that we can easily add it to isCandidate predicate anyway. This
costs no performance in the _nukeSpawn_ benchmarks actually. This should
as a by-effecft fix an edge case bug i have seen, where a nuke is sent
from a phantom silo.
Some of this goes along with PR #3220 since playerImpl buildableUnits
makes use of the underlying spawn functions via canBuild. Just like
ConstructionExecution does. But i didn't want to add more to PR 3220
since there's already a lot in there.
The new function _findClosestBy_ could also be applied to some other
parts of code to benefit of it being faster, so i did that.
_findClosestBy_ uses _findMinimumBy_, which is a little more generic in
name. I think _findMinimumBy_ could be used by other parts of code,
while _findClosestBy_ is more clear naming for what it does now. But we
could ditch _findMinimumBy_ and just leave findClosestBy?
Examples of synthetic benchmarks (not included in this PR):
**BEFORE CHANGES (before Scamiv's PR #3241)**
**AFTER CHANGES (before Scamiv's PR #3241)**
**BEFORE CHANGES (after Scamiv's PR #3241)**
**AFTER CHANGES (after Scamiv's PR #3241)**
_Also see more **BEFORE** and **AFTER** in this comment:_
https://github.com/openfrontio/OpenFrontIO/pull/3243#issuecomment-3949060395
_And here a comparison in the flame charts:_
- based on the same replay and tried to get the performance recording
going at the same speed and length but always end up with small
differences
- because of a bug in replays currently, it puts you in with the same
clientID/persistantID currently. This means we can also record part of
what is normally only recordable with live human input (the
playerActions/playerBuildables).
**BEFORE** flame chart with nukeSpawn (human player) and maybeSendNuke
(Nation players, uses nukeSpawn via canBuild):
**AFTER** flame chart with nukeSpawn (human player) and maybeSendNuke
(Nation players, uses nukeSpawn via canBuild):
## 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
## Please put your Discord username so you can be contacted if a bug or
regression is found:
tryout33
---
src/client/ClientGameRunner.ts | 20 +++---
.../graphics/layers/RadialMenuElements.ts | 15 ++---
src/core/Util.ts | 54 +++++++++++++++
src/core/execution/TradeShipExecution.ts | 21 +++---
src/core/game/PlayerImpl.ts | 66 ++++++++++---------
.../executions/TradeShipExecution.test.ts | 31 ++++++---
6 files changed, 141 insertions(+), 66 deletions(-)
diff --git a/src/client/ClientGameRunner.ts b/src/client/ClientGameRunner.ts
index c8e092bdb..7b483373c 100644
--- a/src/client/ClientGameRunner.ts
+++ b/src/client/ClientGameRunner.ts
@@ -10,7 +10,7 @@ import {
PlayerRecord,
ServerMessage,
} from "../core/Schemas";
-import { createPartialGameRecord, replacer } from "../core/Util";
+import { createPartialGameRecord, findClosestBy, replacer } from "../core/Util";
import { ServerConfig } from "../core/configuration/Config";
import { getConfig } from "../core/configuration/ConfigLoader";
import { BuildableUnit, Structures, UnitType } from "../core/game/Game";
@@ -633,15 +633,15 @@ export class ClientGameRunner {
}
if (upgradeUnits.length > 0) {
- upgradeUnits.sort((a, b) => a.distance - b.distance);
- const bestUpgrade = upgradeUnits[0];
-
- this.eventBus.emit(
- new SendUpgradeStructureIntentEvent(
- bestUpgrade.unitId,
- bestUpgrade.unitType,
- ),
- );
+ const bestUpgrade = findClosestBy(upgradeUnits, (u) => u.distance);
+ if (bestUpgrade) {
+ this.eventBus.emit(
+ new SendUpgradeStructureIntentEvent(
+ bestUpgrade.unitId,
+ bestUpgrade.unitType,
+ ),
+ );
+ }
}
});
}
diff --git a/src/client/graphics/layers/RadialMenuElements.ts b/src/client/graphics/layers/RadialMenuElements.ts
index b24c24b03..1364bb41d 100644
--- a/src/client/graphics/layers/RadialMenuElements.ts
+++ b/src/client/graphics/layers/RadialMenuElements.ts
@@ -10,7 +10,7 @@ import {
} from "../../../core/game/Game";
import { TileRef } from "../../../core/game/GameMap";
import { GameView, PlayerView } from "../../../core/game/GameView";
-import { Emoji, flattenedEmojiTable } from "../../../core/Util";
+import { Emoji, findClosestBy, flattenedEmojiTable } from "../../../core/Util";
import { renderNumber, translateText } from "../../Utils";
import { UIState } from "../UIState";
import { BuildItemDisplay, BuildMenu, flattenedBuildTable } from "./BuildMenu";
@@ -555,14 +555,11 @@ export const deleteUnitElement: MenuElement = {
DELETE_SELECTION_RADIUS,
);
- if (myUnits.length > 0) {
- myUnits.sort(
- (a, b) =>
- params.game.manhattanDist(a.tile(), params.tile) -
- params.game.manhattanDist(b.tile(), params.tile),
- );
-
- params.playerActionHandler.handleDeleteUnit(myUnits[0].id());
+ const closestUnit = findClosestBy(myUnits, (unit) =>
+ params.game.manhattanDist(unit.tile(), params.tile),
+ );
+ if (closestUnit) {
+ params.playerActionHandler.handleDeleteUnit(closestUnit.id());
}
params.closeMenu();
diff --git a/src/core/Util.ts b/src/core/Util.ts
index d099e0197..f3dd45aad 100644
--- a/src/core/Util.ts
+++ b/src/core/Util.ts
@@ -61,6 +61,60 @@ export function distSortUnit(
};
}
+/**
+ * Finds minimum, by score, with single pass search
+ * Faster than array.reduce()
+ */
+export function findMinimumBy(
+ values: readonly T[],
+ score: (value: T) => number,
+ isCandidate?: (value: T) => boolean,
+): T | null {
+ let best: T | null = null;
+ let bestScore = Infinity;
+
+ if (isCandidate === undefined) {
+ for (let i = 0, len = values.length; i < len; i++) {
+ const value = values[i];
+ const currentScore = score(value);
+ if (currentScore < bestScore) {
+ bestScore = currentScore;
+ best = value;
+ }
+ }
+ return best;
+ }
+
+ for (let i = 0, len = values.length; i < len; i++) {
+ const value = values[i];
+ if (!isCandidate(value)) continue;
+
+ const currentScore = score(value);
+ if (currentScore < bestScore) {
+ bestScore = currentScore;
+ best = value;
+ }
+ }
+
+ return best;
+}
+
+/**
+ * Finds closest by fast. Example usage:
+ * findClosestBy(
+ * this.units(UnitType.MissileSilo),
+ * (silo) => mg.manhattanDist(silo.tile(), tile),
+ * (silo) => !silo.isInCooldown() && !silo.isUnderConstruction(),
+ * )
+ */
+export function findClosestBy(
+ values: readonly T[],
+ distance: (value: T) => number,
+ isCandidate?: (value: T) => boolean,
+): T | null {
+ return findMinimumBy(values, distance, isCandidate);
+}
+
export function simpleHash(str: string): number {
let hash = 0;
for (let i = 0; i < str.length; i++) {
diff --git a/src/core/execution/TradeShipExecution.ts b/src/core/execution/TradeShipExecution.ts
index 2c6b80e59..e1efba627 100644
--- a/src/core/execution/TradeShipExecution.ts
+++ b/src/core/execution/TradeShipExecution.ts
@@ -10,7 +10,7 @@ import {
import { TileRef } from "../game/GameMap";
import { PathFinding } from "../pathfinding/PathFinder";
import { PathStatus, SteppingPathFinder } from "../pathfinding/types";
-import { distSortUnit } from "../Util";
+import { findClosestBy } from "../Util";
export class TradeShipExecution implements Execution {
private active = true;
@@ -80,27 +80,32 @@ export class TradeShipExecution implements Execution {
return;
}
+ const curTile = this.tradeShip.tile();
+
if (
this.wasCaptured &&
(tradeShipOwner !== dstPortOwner || !this._dstPort.isActive())
) {
- const ports = this.tradeShip
- .owner()
- .units(UnitType.Port)
- .sort(distSortUnit(this.mg, this.tradeShip));
- if (ports.length === 0) {
+ const nearestPort = findClosestBy(
+ tradeShipOwner.units(UnitType.Port),
+ (port) => this.mg.manhattanDist(port.tile(), curTile),
+ (port) =>
+ port.isActive() &&
+ !port.isMarkedForDeletion() &&
+ !port.isUnderConstruction(),
+ );
+ if (nearestPort === null) {
this.tradeShip.delete(false);
this.active = false;
return;
} else {
- this._dstPort = ports[0];
+ this._dstPort = nearestPort;
this.tradeShip.setTargetUnit(this._dstPort);
// Plan-driven units don't emit per-tick unit updates, so force a sync for the new target.
this.tradeShip.touch();
}
}
- const curTile = this.tradeShip.tile();
if (curTile === this.dstPort()) {
this.complete();
return;
diff --git a/src/core/game/PlayerImpl.ts b/src/core/game/PlayerImpl.ts
index 3d915194c..b223b2f62 100644
--- a/src/core/game/PlayerImpl.ts
+++ b/src/core/game/PlayerImpl.ts
@@ -3,7 +3,7 @@ import { PseudoRandom } from "../PseudoRandom";
import { ClientID } from "../Schemas";
import {
assertNever,
- distSortUnit,
+ findClosestBy,
minInt,
simpleHash,
toInt,
@@ -994,14 +994,18 @@ export class PlayerImpl implements Player {
type: UnitType,
targetTile: TileRef,
): Unit | false {
- const range = this.mg.config().structureMinDist();
- const existing = this.mg
- .nearbyUnits(targetTile, range, type, undefined, true)
- .sort((a, b) => a.distSquared - b.distSquared);
- if (existing.length === 0) {
- return false;
- }
- return existing[0].unit;
+ const closest = findClosestBy(
+ this.mg.nearbyUnits(
+ targetTile,
+ this.mg.config().structureMinDist(),
+ type,
+ undefined,
+ true,
+ ),
+ (entry) => entry.distSquared,
+ );
+
+ return closest?.unit ?? false;
}
private canBuildUnitType(
@@ -1167,27 +1171,29 @@ export class PlayerImpl implements Player {
}
nukeSpawn(tile: TileRef, nukeType: UnitType): TileRef | false {
- if (this.mg.isSpawnImmunityActive()) {
+ const mg = this.mg;
+ if (mg.isSpawnImmunityActive()) {
return false;
}
const owner = this.mg.owner(tile);
// Allow nuking teammates after the game is over (aftergame fun)
- const gameOver = this.mg.getWinner() !== null;
+ const gameOver = mg.getWinner() !== null;
if (owner.isPlayer()) {
if (this.isOnSameTeam(owner) && !gameOver) {
return false;
}
}
+ const config = mg.config();
// Prevent launching nukes that would hit teammate structures (only in team games).
// Disabled after game-over so players can nuke teammates in the aftergame.
if (
- this.mg.config().gameConfig().gameMode === GameMode.Team &&
+ config.gameConfig().gameMode === GameMode.Team &&
nukeType !== UnitType.MIRV &&
!gameOver
) {
- const magnitude = this.mg.config().nukeMagnitudes(nukeType);
- const wouldHitTeammate = this.mg.anyUnitNearby(
+ const magnitude = config.nukeMagnitudes(nukeType);
+ const wouldHitTeammate = mg.anyUnitNearby(
tile,
magnitude.outer,
Structures.types,
@@ -1199,15 +1205,14 @@ export class PlayerImpl implements Player {
}
// only get missilesilos that are not on cooldown and not under construction
- const spawns = this.units(UnitType.MissileSilo)
- .filter((silo) => {
- return !silo.isInCooldown() && !silo.isUnderConstruction();
- })
- .sort(distSortUnit(this.mg, tile));
- if (spawns.length === 0) {
- return false;
- }
- return spawns[0].tile();
+ const bestSilo = findClosestBy(
+ this.units(UnitType.MissileSilo),
+ (silo) => mg.manhattanDist(silo.tile(), tile),
+ (silo) =>
+ silo.isActive() && !silo.isInCooldown() && !silo.isUnderConstruction(),
+ );
+
+ return bestSilo?.tile() ?? false;
}
portSpawn(tile: TileRef, validTiles: TileRef[] | null): TileRef | false {
@@ -1237,15 +1242,14 @@ export class PlayerImpl implements Player {
if (!this.mg.isOcean(tile)) {
return false;
}
- const spawns = this.units(UnitType.Port).sort(
- (a, b) =>
- this.mg.manhattanDist(a.tile(), tile) -
- this.mg.manhattanDist(b.tile(), tile),
+
+ const bestPort = findClosestBy(
+ this.units(UnitType.Port),
+ (port) => this.mg.manhattanDist(port.tile(), tile),
+ (port) => port.isActive() && !port.isUnderConstruction(),
);
- if (spawns.length === 0) {
- return false;
- }
- return spawns[0].tile();
+
+ return bestPort?.tile() ?? false;
}
landBasedUnitSpawn(tile: TileRef): TileRef | false {
diff --git a/tests/core/executions/TradeShipExecution.test.ts b/tests/core/executions/TradeShipExecution.test.ts
index 81cff0b02..9c91442b5 100644
--- a/tests/core/executions/TradeShipExecution.test.ts
+++ b/tests/core/executions/TradeShipExecution.test.ts
@@ -10,6 +10,7 @@ describe("TradeShipExecution", () => {
let pirate: Player;
let srcPort: Unit;
let piratePort: Unit;
+ let piratePort2: Unit;
let tradeShip: Unit;
let dstPort: Unit;
let tradeShipExecution: TradeShipExecution;
@@ -48,27 +49,41 @@ describe("TradeShipExecution", () => {
id: vi.fn(() => 3),
addGold: vi.fn(),
displayName: vi.fn(() => "Destination"),
- units: vi.fn(() => [piratePort]),
- unitCount: vi.fn(() => 1),
+ units: vi.fn(() => [piratePort, piratePort2]),
+ unitCount: vi.fn(() => 2),
canTrade: vi.fn(() => true),
} as any;
piratePort = {
- tile: vi.fn(() => 40011),
+ tile: vi.fn(() => 56),
owner: vi.fn(() => pirate),
isActive: vi.fn(() => true),
+ isUnderConstruction: vi.fn(() => false),
+ isMarkedForDeletion: vi.fn(() => false),
+ } as any;
+
+ piratePort2 = {
+ tile: vi.fn(() => 75),
+ owner: vi.fn(() => pirate),
+ isActive: vi.fn(() => true),
+ isUnderConstruction: vi.fn(() => false),
+ isMarkedForDeletion: vi.fn(() => false),
} as any;
srcPort = {
- tile: vi.fn(() => 20011),
+ tile: vi.fn(() => 10),
owner: vi.fn(() => origOwner),
isActive: vi.fn(() => true),
+ isUnderConstruction: vi.fn(() => false),
+ isMarkedForDeletion: vi.fn(() => false),
} as any;
dstPort = {
- tile: vi.fn(() => 30015), // 15x15
+ tile: vi.fn(() => 100),
owner: vi.fn(() => dstOwner),
isActive: vi.fn(() => true),
+ isUnderConstruction: vi.fn(() => false),
+ isMarkedForDeletion: vi.fn(() => false),
} as any;
tradeShip = {
@@ -80,13 +95,13 @@ describe("TradeShipExecution", () => {
setSafeFromPirates: vi.fn(),
touch: vi.fn(),
delete: vi.fn(),
- tile: vi.fn(() => 2001),
+ tile: vi.fn(() => 32),
} as any;
tradeShipExecution = new TradeShipExecution(origOwner, srcPort, dstPort);
tradeShipExecution.init(game, 0);
tradeShipExecution["pathFinder"] = {
- next: vi.fn(() => ({ status: PathStatus.NEXT, node: 2001 })),
+ next: vi.fn(() => ({ status: PathStatus.NEXT, node: 32 })),
findPath: vi.fn((from: number) => [from]),
} as any;
tradeShipExecution["tradeShip"] = tradeShip;
@@ -118,7 +133,7 @@ describe("TradeShipExecution", () => {
it("should complete trade and award gold", () => {
tradeShipExecution["pathFinder"] = {
- next: vi.fn(() => ({ status: PathStatus.COMPLETE, node: 2001 })),
+ next: vi.fn(() => ({ status: PathStatus.COMPLETE, node: 32 })),
findPath: vi.fn((from: number) => [from]),
} as any;
tradeShipExecution.tick(1);