Perf/Fix: spawn and other functions that need closest by unit#3243
Perf/Fix: spawn and other functions that need closest by unit#3243VariableVince wants to merge 11 commits intomainfrom
Conversation
WalkthroughAdds generic utilities Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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.
🧹 Nitpick comments (1)
src/core/Util.ts (1)
95-101:findClosestByis a thin alias overfindMinimumBy— consider whether both exports are needed long-term.Right now
findClosestByjust forwards tofindMinimumBywith zero added logic. The only difference is the parameter name (distancevsscore). This is fine for readability at call sites — "closest by distance" reads more naturally than "minimum by score" when dealing with spatial lookups. Just keep in mind that maintaining two nearly identical public exports adds surface area. IffindMinimumBynever gets used directly outside offindClosestBy, you could collapse them later.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/Util.ts` around lines 95 - 101, findClosestBy is a trivial wrapper around findMinimumBy with no added behavior (only a parameter rename), which risks duplicated public surface area; either remove the redundant export or make the intent explicit: search the repo for external usages of findClosestBy and findMinimumBy and if both are used keep both, otherwise collapse into a single export (prefer the clearer name for call sites, e.g., keep findClosestBy and remove or inline findMinimumBy), or add a short comment above findClosestBy documenting it's an intentional alias for readability; update any imports accordingly to use the chosen single function (references: findClosestBy, findMinimumBy).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/core/Util.ts`:
- Around line 95-101: findClosestBy is a trivial wrapper around findMinimumBy
with no added behavior (only a parameter rename), which risks duplicated public
surface area; either remove the redundant export or make the intent explicit:
search the repo for external usages of findClosestBy and findMinimumBy and if
both are used keep both, otherwise collapse into a single export (prefer the
clearer name for call sites, e.g., keep findClosestBy and remove or inline
findMinimumBy), or add a short comment above findClosestBy documenting it's an
intentional alias for readability; update any imports accordingly to use the
chosen single function (references: findClosestBy, findMinimumBy).
|
looks like a test is failing |
…redicate is provided
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/core/Util.ts (1)
110-116:findClosestByis a pure alias — consider whether it pulls its own weight.Right now
findClosestByadds no behaviour on top offindMinimumBy; they share the same signature and the body is a single delegation line. Two options worth considering:
- Keep as-is — the distinct name signals "this is a spatial distance query" at call sites, which is fine.
- Remove
findClosestByand have callers importfindMinimumBydirectly, reducing surface area.Either is valid; this is just a nudge to settle the "possible consolidation" point the PR already raises before more callers are added.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/Util.ts` around lines 110 - 116, findClosestBy is a no-op alias of findMinimumBy; decide whether to remove it or clarify intent: either delete the findClosestBy function and replace all its call sites/imports to use findMinimumBy (update tests and exports accordingly), or keep findClosestBy but add a brief doc comment above the function explaining it's a semantic alias for spatial/distance queries so callers understand the intent and keep the single-line delegation as-is; reference the symbols findClosestBy and findMinimumBy when making the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/core/Util.ts`:
- Around line 102-109: The JSDoc example for findClosestBy uses the incorrect
GameMap variable name "mg" — change it to "gm" to match the file's convention
and other helpers like distSort and distSortUnit; update the example line
containing mg.manhattanDist(...) to use gm.manhattanDist(...) so examples are
consistent and copy-pasteable for readers referencing findClosestBy.
---
Nitpick comments:
In `@src/core/Util.ts`:
- Around line 110-116: findClosestBy is a no-op alias of findMinimumBy; decide
whether to remove it or clarify intent: either delete the findClosestBy function
and replace all its call sites/imports to use findMinimumBy (update tests and
exports accordingly), or keep findClosestBy but add a brief doc comment above
the function explaining it's a semantic alias for spatial/distance queries so
callers understand the intent and keep the single-line delegation as-is;
reference the symbols findClosestBy and findMinimumBy when making the change.
github-project-automation
bot
moved this from Triage
to Development
in OpenFront Release Management
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/core/game/PlayerImpl.ts (1)
1183-1193:warshipSpawnstill usesthis.mginstead of a local cache.Not a bug, but
nukeSpawncachesthis.mginto a localmgfor speed, whilewarshipSpawndoes not. If this method is also on a hot path, the same local-cache pattern would be consistent. Low priority sincewarshipSpawndoes fewer lookups.Optional: cache mg locally for consistency
warshipSpawn(tile: TileRef): TileRef | false { - if (!this.mg.isOcean(tile)) { + const mg = this.mg; + if (!mg.isOcean(tile)) { return false; } - const bestPort = findClosestBy(this.units(UnitType.Port), (port) => - this.mg.manhattanDist(port.tile(), tile), + const bestPort = findClosestBy(this.units(UnitType.Port), (port) => + mg.manhattanDist(port.tile(), tile), ); return bestPort?.tile() ?? false; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/game/PlayerImpl.ts` around lines 1183 - 1193, warshipSpawn currently accesses this.mg repeatedly; mirror the nukeSpawn optimization by caching this.mg into a local const (e.g., const mg = this.mg) at the top of the warshipSpawn method and then use mg.isOcean and mg.manhattanDist in the findClosestBy callback (leave findClosestBy, units(UnitType.Port), and UnitType.Port unchanged) so lookups are consistent and slightly faster on hot paths.src/core/Util.ts (1)
102-116:findClosestByis a pure alias forfindMinimumBy— consider if both are needed.Right now
findClosestByjust callsfindMinimumBywith identical signature/behavior. The only difference is the parameter name (distancevsscore). This is fine for readability at call sites, but be aware the two could drift apart if someone later adds logic to only one of them.If the intent is purely a naming alias, a brief inline comment like
// Semantic alias – keep in sync with findMinimumBywould help future readers.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/Util.ts` around lines 102 - 116, findClosestBy currently just forwards to findMinimumBy and should be documented as an intentional semantic alias to avoid future drift; update the findClosestBy declaration to include a short inline comment like "// Semantic alias of findMinimumBy — keep behavior/signature in sync" (or alternatively remove the alias and replace call sites with findMinimumBy if duplication isn't desired), referencing the functions findClosestBy and findMinimumBy so readers know this is deliberate.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/core/game/PlayerImpl.ts`:
- Around line 1183-1193: warshipSpawn currently accesses this.mg repeatedly;
mirror the nukeSpawn optimization by caching this.mg into a local const (e.g.,
const mg = this.mg) at the top of the warshipSpawn method and then use
mg.isOcean and mg.manhattanDist in the findClosestBy callback (leave
findClosestBy, units(UnitType.Port), and UnitType.Port unchanged) so lookups are
consistent and slightly faster on hot paths.
In `@src/core/Util.ts`:
- Around line 102-116: findClosestBy currently just forwards to findMinimumBy
and should be documented as an intentional semantic alias to avoid future drift;
update the findClosestBy declaration to include a short inline comment like "//
Semantic alias of findMinimumBy — keep behavior/signature in sync" (or
alternatively remove the alias and replace call sites with findMinimumBy if
duplication isn't desired), referencing the functions findClosestBy and
findMinimumBy so readers know this is deliberate.
…or map "ocean_and_land" which lead to NaN as outcome of manhattanDist. Until now we used distSortUnit which just returned the first and only port it was given in the test, even though manhattanDist both returned NaN. Now with findClosestBy we need it to have actual existing tiles because we need actual comparable number to come out of manhattanDist.) Also for TradeShipExecution use some already cached values as input for findClosestBy.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
…eletion, still under construction (even if it might be finished when we arrive, we don't know that) or is inactive.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/core/execution/TradeShipExecution.ts`:
- Around line 87-103: The port mocks used in tests (dstPort, piratePort,
piratePort2, srcPort) are missing stubs for Unit methods isMarkedForDeletion()
and isUnderConstruction(), causing CI failures when TradeShipExecution calls
these; update the test beforeEach where those port mocks are created to add
vi.fn() stubs for both methods on each mock (e.g., isMarkedForDeletion: vi.fn(()
=> false), isUnderConstruction: vi.fn(() => false)) so findClosestBy/filtering
in TradeShipExecution works as expected.
There was a problem hiding this comment.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/core/execution/TradeShipExecution.ts`:
- Around line 87-103: Tests are failing because the port mocks used in
TradeShipExecution.test.ts lack the new filter-method stubs; in the beforeEach
add isMarkedForDeletion() and isUnderConstruction() stubs on each port mock
(dstPort, srcPort, piratePort, piratePort2) returning appropriate values (e.g.,
false) so findClosestBy's filter in TradeShipExecution (which calls
isMarkedForDeletion and isUnderConstruction) can run without error.
… make test happy...
…d unit, if it can be upgraded. So canUpgradeUnit should not be send as a predicate for isCandidate in findClosestBy, but only be checked afterwards for the one found unit. This makes findClosestBy faster and with that findUnitToUpgrade so it's only good.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
…pSpawn not return ports under construction or (smaller change) inactive. This fixes a bug right away. Same for nukeSpawn silos, don't return inactive silo just to be sure now that we can easily add it to isCandidate predicate anyway
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/core/game/PlayerImpl.ts (1)
992-1001: Consider usingisCandidatefor parity withnukeSpawn/warshipSpawn.Both
nukeSpawnandwarshipSpawnpass eligibility checks as theisCandidatepredicate so thatfindClosestByskips ineligible candidates early and returns the closest valid target. Here, the predicate is absent —findClosestByreturns the single closest unit regardless of eligibility, and if that unit failscanUpgradeUnit(e.g., it belongs to another player, is under construction, or is marked for deletion) the call returnsfalseeven if a slightly-farther eligible unit exists.In practice this is rare because
structureMinDistspacing makes it unlikely for two same-type units to coexist within the search range, but it creates a pattern inconsistency and a latent correctness gap.♻️ Proposed refactor using `isCandidate`
public findUnitToUpgrade(type: UnitType, targetTile: TileRef): Unit | false { const range = this.mg.config().structureMinDist(); - const closest = findClosestBy( + const closest = findClosestBy( this.mg.nearbyUnits(targetTile, range, type, undefined, true), (entry) => entry.distSquared, + (entry) => this.canUpgradeUnit(entry.unit), ); - - return closest === null - ? false - : this.canUpgradeUnit(closest.unit) - ? closest.unit - : false; + return closest?.unit ?? false; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/game/PlayerImpl.ts` around lines 992 - 1001, findClosestBy is currently called without a predicate so it returns the nearest unit even if ineligible; replace that with the same isCandidate-style predicate used by nukeSpawn/warshipSpawn so findClosestBy skips ineligible units early. Specifically, call findClosestBy(this.mg.nearbyUnits(targetTile, range, type, undefined, true), entry => ({ pass: this.canUpgradeUnit(entry.unit), key: entry.distSquared })) or use the existing isCandidate helper pattern so only units passing canUpgradeUnit(...) are considered, ensuring the returned closest is a valid upgradable unit instead of post-filtering.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/core/game/PlayerImpl.ts`:
- Around line 1155-1158: Prettier flagged formatting around the findClosestBy
call: run npx prettier --write on the file and reformat the call to
findClosestBy that uses this.units(UnitType.MissileSilo) and the two predicate
functions (the distance extractor using mg.manhattanDist(silo.tile(), tile) and
the boolean check using silo.isActive(), silo.isInCooldown(),
silo.isUnderConstruction()) so the long single-line predicate is
wrapped/indented per Prettier rules and the file passes CI.
---
Nitpick comments:
In `@src/core/game/PlayerImpl.ts`:
- Around line 992-1001: findClosestBy is currently called without a predicate so
it returns the nearest unit even if ineligible; replace that with the same
isCandidate-style predicate used by nukeSpawn/warshipSpawn so findClosestBy
skips ineligible units early. Specifically, call
findClosestBy(this.mg.nearbyUnits(targetTile, range, type, undefined, true),
entry => ({ pass: this.canUpgradeUnit(entry.unit), key: entry.distSquared })) or
use the existing isCandidate helper pattern so only units passing
canUpgradeUnit(...) are considered, ensuring the returned closest is a valid
upgradable unit instead of post-filtering.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/core/game/PlayerImpl.ts`:
- Around line 992-1001: findUnitToUpgrade currently calls findClosestBy over
this.mg.nearbyUnits without supplying an isCandidate predicate, so it picks the
nearest unit even if not upgradeable; change the findClosestBy invocation in
findUnitToUpgrade to pass an isCandidate function that returns true only when
this.canUpgradeUnit(entry.unit) is true (use the same entry.distSquared for the
comparator), so findClosestBy returns the closest eligible unit (or null) and
then return that unit or false as before.
| const closest = findClosestBy( | ||
| this.mg.nearbyUnits(targetTile, range, type, undefined, true), | ||
| (entry) => entry.distSquared, | ||
| ); | ||
|
|
||
| return closest === null | ||
| ? false | ||
| : this.canUpgradeUnit(closest.unit) | ||
| ? closest.unit | ||
| : false; |
There was a problem hiding this comment.
findUnitToUpgrade returns false for the wrong reason when the closest unit is not upgradable
findClosestBy has no isCandidate predicate here, so it always returns the nearest unit regardless of eligibility. canUpgradeUnit is then checked only on that single result. If the nearest unit is under construction (or marked for deletion, or otherwise ineligible), the function returns false — even when a slightly farther unit would have been eligible.
The isCandidate slot exists precisely for this: skip ineligible entries during the scan so the closest eligible unit is returned.
🐛 Proposed fix
public findUnitToUpgrade(type: UnitType, targetTile: TileRef): Unit | false {
const range = this.mg.config().structureMinDist();
const closest = findClosestBy(
this.mg.nearbyUnits(targetTile, range, type, undefined, true),
(entry) => entry.distSquared,
+ (entry) => this.canUpgradeUnit(entry.unit),
);
- return closest === null
- ? false
- : this.canUpgradeUnit(closest.unit)
- ? closest.unit
- : false;
+ return closest?.unit ?? false;
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/core/game/PlayerImpl.ts` around lines 992 - 1001, findUnitToUpgrade
currently calls findClosestBy over this.mg.nearbyUnits without supplying an
isCandidate predicate, so it picks the nearest unit even if not upgradeable;
change the findClosestBy invocation in findUnitToUpgrade to pass an isCandidate
function that returns true only when this.canUpgradeUnit(entry.unit) is true
(use the same entry.distSquared for the comparator), so findClosestBy returns
the closest eligible unit (or null) and then return that unit or false as
before.
VariableVince
changed the title
2 participants
Description:
Performance improvements.
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)
Please complete the following:
Please put your Discord username so you can be contacted if a bug or regression is found:
tryout33