motion plans: packed buffers + client-side movement#3267
motion plans: packed buffers + client-side movement#3267scamiv wants to merge 1 commit intoopenfrontio:mainfrom
Conversation
WalkthroughIntroduces a motion plan system for tracking and synchronizing unit movement. New MotionPlans module defines binary serialization format. Execution layers record motion plans; GameImpl manages plan-driven units; GameView applies plans for synchronized movement visualization; worker efficiently transfers packed data. Changes
Sequence DiagramsequenceDiagram
participant Exec as Execution Layer<br/>(Nuke/TradeShip/TransportShip)
participant GameImpl as GameImpl<br/>(recordMotionPlan)
participant Runner as GameRunner<br/>(drainPackedMotionPlans)
participant Worker as Worker<br/>(postMessage)
participant View as GameView<br/>(applyMotionPlans)
participant Layer as UnitLayer<br/>(sprite update)
Exec->>GameImpl: recordMotionPlan(GridPathPlan|ParabolaPlan)
Note over GameImpl: Store in motionPlanRecords<br/>Update planDrivenUnitIds
Runner->>GameImpl: drainPackedMotionPlans()
GameImpl-->>Runner: Uint32Array (packed)
Note over Runner: Include in callback<br/>packedMotionPlans field
Runner->>Worker: postMessage(gameUpdate)
Note over Worker: Add packedMotionPlans.buffer<br/>to transfer list
Worker-->>View: GameUpdate with<br/>packedMotionPlans
View->>View: unpackMotionPlans(packed)
View->>View: applyMotionPlans(records)
Note over View: Build unitMotionPlans map<br/>grid/parabola plan setup
View->>View: advanceMotionPlannedUnits(tick)
Note over View: Compute path position<br/>Update unit tile
Layer->>View: Query motion plan state
View-->>Layer: motionPlanUnitIds
Layer->>Layer: Update sprites<br/>motionPlanUnitIds ∪ updatedUnitIds
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 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.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/core/game/Game.ts (1)
1-1:⚠️ Potential issue | 🟡 MinorFix Prettier formatting before merge.
CI reports formatting violations in this file. Run
npx prettier --write src/core/game/Game.ts.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/game/Game.ts` at line 1, The file Game.ts fails Prettier formatting; run Prettier to reformat the file (e.g., execute npx prettier --write src/core/game/Game.ts) or apply Prettier rules to the import line and the rest of the file so the import { Config } from "../configuration/Config"; and surrounding code conform to the project's Prettier configuration; then re-run CI to confirm formatting violations are resolved.src/core/execution/TransportShipExecution.ts (1)
1-1:⚠️ Potential issue | 🟡 MinorFix Prettier formatting before merge.
CI reports formatting violations. Run
npx prettier --write src/core/execution/TransportShipExecution.ts.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/execution/TransportShipExecution.ts` at line 1, Prettier formatting violations are present in TransportShipExecution.ts; run the formatter and commit the changes: execute npx prettier --write on the file (or run your repo's pre-commit/CI formatting task) to fix spacing/linebreaks around imports and usages such as the import of renderTroops, then stage and push the updated file so CI passes.src/core/execution/NukeExecution.ts (1)
1-1:⚠️ Potential issue | 🟡 MinorFix Prettier formatting before merge.
CI reports formatting violations. Run
npx prettier --write src/core/execution/NukeExecution.ts.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/execution/NukeExecution.ts` at line 1, CI reports Prettier formatting violations in the NukeExecution code; fix by running your project's formatter on the file containing the NukeExecution class (apply Prettier formatting across src/core/execution/NukeExecution.ts or the repo), stage the resulting changes, and update the PR so the formatted file replaces the current unformatted version.
🧹 Nitpick comments (8)
src/core/execution/TransportShipExecution.ts (2)
16-16: Useimport typefor type-only import.Same pattern as
NukeExecution.ts–MotionPlanRecordis only used as a type annotation here.♻️ Proposed fix
-import { MotionPlanRecord } from "../game/MotionPlans"; +import type { MotionPlanRecord } from "../game/MotionPlans";🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/execution/TransportShipExecution.ts` at line 16, Change the value import of MotionPlanRecord to a type-only import: locate the import statement that brings in MotionPlanRecord in TransportShipExecution (symbol: MotionPlanRecord) and replace it with a type-only import so the module is not included at runtime (e.g. use "import type { MotionPlanRecord } ..."); ensure any usages remain as type annotations only and that no runtime references to MotionPlanRecord exist in functions or methods of TransportShipExecution.
280-289:startTickis hardcoded toticks + 1but should mirrorticks + this.ticksPerMove.
init()correctly usesstartTick: ticks + this.ticksPerMove. Here,ticks + 1works only becauseticksPerMoveis always1. IfticksPerMoveis ever increased, the updated plan will tell the client to start moving one tick before the boat actually moves.♻️ Proposed fix for consistency
this.mg.recordMotionPlan({ kind: "grid", unitId: this.boat.id(), planId: this.motionPlanId, - startTick: ticks + 1, + startTick: ticks + this.ticksPerMove, ticksPerStep: this.ticksPerMove, path: fullPath, });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/execution/TransportShipExecution.ts` around lines 280 - 289, The motion plan update is using startTick: ticks + 1 which should match the init() behavior; change the recordMotionPlan call in TransportShipExecution (where this.mg.recordMotionPlan is invoked and this.motionPlanId is set) to use startTick: ticks + this.ticksPerMove so the client start time stays consistent with ticksPerMove rather than assuming 1.src/core/execution/NukeExecution.ts (1)
19-19: Useimport typefor type-only import.
MotionPlanRecordis only used as a type annotation.Game.tsalready usesimport typefor the same import – keep it consistent.♻️ Proposed fix
-import { MotionPlanRecord } from "../game/MotionPlans"; +import type { MotionPlanRecord } from "../game/MotionPlans";🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/execution/NukeExecution.ts` at line 19, The import of MotionPlanRecord in NukeExecution.ts is type-only; change the declaration "import { MotionPlanRecord } from \"../game/MotionPlans\";" to use TypeScript's type-only import syntax (i.e., "import type { MotionPlanRecord } ...") so the compiler/tree-shaker knows it's a type import; update the import statement referencing MotionPlanRecord accordingly to match the pattern used in Game.ts.src/client/graphics/layers/UnitLayer.ts (1)
68-71: Redundant optional chaining onupdatesSinceLastTick().Per project convention,
updatesSinceLastTick()always returns an object where everyGameUpdateTypekey maps to an array (neverundefined). The?.on the result and on.map(...), plus the?? []fallback, are all unnecessary.♻️ Proposed simplification
- const updatedUnitIds = - this.game - .updatesSinceLastTick() - ?.[GameUpdateType.Unit]?.map((unit) => unit.id) ?? []; + const updatedUnitIds = this.game + .updatesSinceLastTick() + [GameUpdateType.Unit].map((unit) => unit.id);Based on learnings: "game.updatesSinceLastTick() returns a map/object where each GameUpdateType key always maps to at least an empty array, never undefined."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/client/graphics/layers/UnitLayer.ts` around lines 68 - 71, Remove the redundant optional chaining and nullish fallback when building updatedUnitIds in UnitLayer.ts: directly call this.game.updatesSinceLastTick()[GameUpdateType.Unit].map(unit => unit.id) since updatesSinceLastTick() always returns an object with arrays for each GameUpdateType; update the variable assignment for updatedUnitIds to rely on that contract and remove the "?.", the optional chaining on .map and the "?? []" fallback.src/core/execution/TradeShipExecution.ts (1)
56-65: Placeholder plan is immediately replaced on the same tick.The placeholder plan (planId = 1) emitted at lines 56–64 is superseded within the same
tick()call by the real plan (planId = 2) at lines 155–162, becausespawnedThisTickistrue. This means the placeholder is packed, transferred, and then immediately overwritten by the next record in the same batch.If the placeholder exists only as a safety net in case
findPathfails (returnsnull), consider skipping it and emitting only the real plan. That removes one unnecessary record per spawn.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/execution/TradeShipExecution.ts` around lines 56 - 65, The placeholder MotionPlanRecord (placeholderPlan with planId = this.motionPlanId) is being recorded via this.mg.recordMotionPlan(...) and immediately overwritten when a real plan is generated later in tick() (due to spawnedThisTick being true); remove the unnecessary placeholder emission by only calling this.mg.recordMotionPlan(...) when a real plan exists (i.e., after you call findPath and it returns a non-null path) or, alternatively, only emit the placeholder if findPath returns null as a true fallback; update references to motionPlanId/motionPlanDst accordingly and avoid writing the placeholderPlan block before the real-plan generation logic.src/core/game/MotionPlans.ts (1)
156-158:packed.slice()returns aUint32Array, not aTileRef[].
Uint32Array.prototype.slice()returns a newUint32Array. The castas unknown as TileRef[]hides this. While index access andforloops work the same on both, array methods likeArray.isArray(),.push(),.map(), or[Symbol.iterator]can behave differently in edge cases. If any downstream consumer treatspathas a plainnumber[], it could break.Consider converting to a real array:
Proposed fix
- const path = packed.slice(pathStart, pathEnd) as unknown as TileRef[]; + const path: TileRef[] = Array.from(packed.subarray(pathStart, pathEnd));Or, if you intentionally want
Uint32Arrayfor memory efficiency, change theGridPathPlan.pathtype toreadonly TileRef[] | Uint32Arrayso consumers know what to expect.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/game/MotionPlans.ts` around lines 156 - 158, The slice call assigns a Uint32Array to path but is force-cast to TileRef[]; replace the unsafe cast by converting the sliced buffer into a real array (e.g., use Array.from on the result of packed.slice(pathStart, pathEnd)) so GridPathPlan.path is a true TileRef[] at runtime, or alternatively update the GridPathPlan.path type to accept Uint32Array (e.g., readonly TileRef[] | Uint32Array) and update downstream consumers; locate the code around pathStart/pathEnd/path and the packed.slice(...) call to implement one of these fixes.src/core/game/GameImpl.ts (1)
387-389: Motion plan records are not cleared at the start ofexecuteNextTick, unlike tile updates.
tileUpdatePairsis reset to empty at line 389 insideexecuteNextTick(), so stale tile data never leaks across ticks. ButmotionPlanRecordsis only cleared insidedrainPackedMotionPlans(). If the caller ever skips a drain call, motion plan records from earlier ticks will silently accumulate and be sent late.Consider adding
this.motionPlanRecords.length = 0;at the top ofexecuteNextTickfor consistency, or document thatdrainPackedMotionPlansmust always be called after each tick.Also applies to: 462-470
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/game/GameImpl.ts` around lines 387 - 389, In executeNextTick(), clear stale motion plan state the same way tile updates are reset: add a reset for motionPlanRecords (e.g., this.motionPlanRecords.length = 0) at the start of executeNextTick so records cannot accumulate if drainPackedMotionPlans() is skipped; alternatively ensure drainPackedMotionPlans() is guaranteed to be called every tick and document that requirement — update executeNextTick() and any other tick-init code paths (see drainPackedMotionPlans and the region around the other tick handling at lines referenced) to keep motionPlanRecords lifecycle consistent with tileUpdatePairs.src/core/game/GameView.ts (1)
764-779:applyDerivedPositionruns every tick for every motion-planned unit, even when the tile is unchanged.When a unit is at the end of its path (or hasn't moved yet),
applyDerivedPositionstill allocates a newdataobject via spread (line 97–101), pushes tolastPos, and sets_wasUpdated = true. This can cause unnecessary rendering work for all layers that checkwasUpdated().Consider skipping the call when the position has not changed:
Proposed fix
const oldTile = unit.tile(); const newTile = this.motionTileAtTick(plan, currentTick); - unit.applyDerivedPosition(newTile); - if (newTile !== oldTile) { + if (newTile !== oldTile) { + unit.applyDerivedPosition(newTile); this.unitGrid.updateUnitCell(unit); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/game/GameView.ts` around lines 764 - 779, In advanceMotionPlannedUnits, avoid calling unit.applyDerivedPosition when the computed newTile equals the current tile to prevent allocating a new data object and setting _wasUpdated unnecessarily; change the logic to compute newTile via motionTileAtTick(plan, currentTick), compare it to oldTile (unit.tile()), and only call applyDerivedPosition and unitGrid.updateUnitCell when they differ — otherwise leave the unit state untouched so wasUpdated()/lastPos aren't modified for stationary units.
🤖 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/GameImpl.ts`:
- Around line 238-243: The code is using a double cast for the unit update
(update as any as { id: number; isActive: boolean }) which bypasses type safety;
change the cast to the proper UnitUpdate type after the type guard (i.e., when
update.type === GameUpdateType.Unit) so you directly treat update as UnitUpdate
and then call this.planDrivenUnitIds.delete(unitUpdate.id); ensure you reference
the UnitUpdate type and the GameUpdateType.Unit check in GameImpl.ts when
replacing the double cast.
In `@src/core/game/GameView.ts`:
- Around line 834-863: In the "parabola" branch of GameView (inside the switch
case "parabola") replace the hardcoded loop limit 20000 with a named constant
(e.g. const MAX_PARABOLA_STEPS = 1000) and use that constant in the for loop
that iterates pf.next(record.src, record.dst, record.increment); update the code
that builds tiles and the unitMotionPlans entry to rely on that constant so the
cap is explicit and adjustable (refer to symbols: the "parabola" case, pf
(UniversalPathFinding.Parabola), tiles array, and unitMotionPlans.set).
---
Outside diff comments:
In `@src/core/execution/NukeExecution.ts`:
- Line 1: CI reports Prettier formatting violations in the NukeExecution code;
fix by running your project's formatter on the file containing the NukeExecution
class (apply Prettier formatting across src/core/execution/NukeExecution.ts or
the repo), stage the resulting changes, and update the PR so the formatted file
replaces the current unformatted version.
In `@src/core/execution/TransportShipExecution.ts`:
- Line 1: Prettier formatting violations are present in
TransportShipExecution.ts; run the formatter and commit the changes: execute npx
prettier --write on the file (or run your repo's pre-commit/CI formatting task)
to fix spacing/linebreaks around imports and usages such as the import of
renderTroops, then stage and push the updated file so CI passes.
In `@src/core/game/Game.ts`:
- Line 1: The file Game.ts fails Prettier formatting; run Prettier to reformat
the file (e.g., execute npx prettier --write src/core/game/Game.ts) or apply
Prettier rules to the import line and the rest of the file so the import {
Config } from "../configuration/Config"; and surrounding code conform to the
project's Prettier configuration; then re-run CI to confirm formatting
violations are resolved.
---
Nitpick comments:
In `@src/client/graphics/layers/UnitLayer.ts`:
- Around line 68-71: Remove the redundant optional chaining and nullish fallback
when building updatedUnitIds in UnitLayer.ts: directly call
this.game.updatesSinceLastTick()[GameUpdateType.Unit].map(unit => unit.id) since
updatesSinceLastTick() always returns an object with arrays for each
GameUpdateType; update the variable assignment for updatedUnitIds to rely on
that contract and remove the "?.", the optional chaining on .map and the "?? []"
fallback.
In `@src/core/execution/NukeExecution.ts`:
- Line 19: The import of MotionPlanRecord in NukeExecution.ts is type-only;
change the declaration "import { MotionPlanRecord } from
\"../game/MotionPlans\";" to use TypeScript's type-only import syntax (i.e.,
"import type { MotionPlanRecord } ...") so the compiler/tree-shaker knows it's a
type import; update the import statement referencing MotionPlanRecord
accordingly to match the pattern used in Game.ts.
In `@src/core/execution/TradeShipExecution.ts`:
- Around line 56-65: The placeholder MotionPlanRecord (placeholderPlan with
planId = this.motionPlanId) is being recorded via this.mg.recordMotionPlan(...)
and immediately overwritten when a real plan is generated later in tick() (due
to spawnedThisTick being true); remove the unnecessary placeholder emission by
only calling this.mg.recordMotionPlan(...) when a real plan exists (i.e., after
you call findPath and it returns a non-null path) or, alternatively, only emit
the placeholder if findPath returns null as a true fallback; update references
to motionPlanId/motionPlanDst accordingly and avoid writing the placeholderPlan
block before the real-plan generation logic.
In `@src/core/execution/TransportShipExecution.ts`:
- Line 16: Change the value import of MotionPlanRecord to a type-only import:
locate the import statement that brings in MotionPlanRecord in
TransportShipExecution (symbol: MotionPlanRecord) and replace it with a
type-only import so the module is not included at runtime (e.g. use "import type
{ MotionPlanRecord } ..."); ensure any usages remain as type annotations only
and that no runtime references to MotionPlanRecord exist in functions or methods
of TransportShipExecution.
- Around line 280-289: The motion plan update is using startTick: ticks + 1
which should match the init() behavior; change the recordMotionPlan call in
TransportShipExecution (where this.mg.recordMotionPlan is invoked and
this.motionPlanId is set) to use startTick: ticks + this.ticksPerMove so the
client start time stays consistent with ticksPerMove rather than assuming 1.
In `@src/core/game/GameImpl.ts`:
- Around line 387-389: In executeNextTick(), clear stale motion plan state the
same way tile updates are reset: add a reset for motionPlanRecords (e.g.,
this.motionPlanRecords.length = 0) at the start of executeNextTick so records
cannot accumulate if drainPackedMotionPlans() is skipped; alternatively ensure
drainPackedMotionPlans() is guaranteed to be called every tick and document that
requirement — update executeNextTick() and any other tick-init code paths (see
drainPackedMotionPlans and the region around the other tick handling at lines
referenced) to keep motionPlanRecords lifecycle consistent with tileUpdatePairs.
In `@src/core/game/GameView.ts`:
- Around line 764-779: In advanceMotionPlannedUnits, avoid calling
unit.applyDerivedPosition when the computed newTile equals the current tile to
prevent allocating a new data object and setting _wasUpdated unnecessarily;
change the logic to compute newTile via motionTileAtTick(plan, currentTick),
compare it to oldTile (unit.tile()), and only call applyDerivedPosition and
unitGrid.updateUnitCell when they differ — otherwise leave the unit state
untouched so wasUpdated()/lastPos aren't modified for stationary units.
In `@src/core/game/MotionPlans.ts`:
- Around line 156-158: The slice call assigns a Uint32Array to path but is
force-cast to TileRef[]; replace the unsafe cast by converting the sliced buffer
into a real array (e.g., use Array.from on the result of packed.slice(pathStart,
pathEnd)) so GridPathPlan.path is a true TileRef[] at runtime, or alternatively
update the GridPathPlan.path type to accept Uint32Array (e.g., readonly
TileRef[] | Uint32Array) and update downstream consumers; locate the code around
pathStart/pathEnd/path and the packed.slice(...) call to implement one of these
fixes.
| if (update.type === GameUpdateType.Unit) { | ||
| const unitUpdate = update as any as { id: number; isActive: boolean }; | ||
| if (unitUpdate.isActive === false) { | ||
| this.planDrivenUnitIds.delete(unitUpdate.id); | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Find GameUpdate type definition
rg -n "type GameUpdate|interface GameUpdate" --type=ts -A15Repository: openfrontio/OpenFrontIO
Length of output: 4100
🏁 Script executed:
# Find GameUpdateType enum and Unit variant
rg -n "GameUpdateType" --type=ts -B2 -A8 | head -80Repository: openfrontio/OpenFrontIO
Length of output: 6254
🏁 Script executed:
# Check the actual update object structure for Unit type
rg -n "GameUpdateType.Unit" --type=ts -B5 -A5Repository: openfrontio/OpenFrontIO
Length of output: 12510
🏁 Script executed:
# Look at GameImpl.ts imports and context around line 238-243
sed -n '1,60p' src/core/game/GameImpl.ts
sed -n '230,250p' src/core/game/GameImpl.tsRepository: openfrontio/OpenFrontIO
Length of output: 2258
🏁 Script executed:
# Get full UnitUpdate definition
sed -n '128,145p' src/core/game/GameUpdates.tsRepository: openfrontio/OpenFrontIO
Length of output: 529
🏁 Script executed:
# Search for isActive field in UnitUpdate or related types
rg -n "isActive" --type=ts src/core/game/ | head -30Repository: openfrontio/OpenFrontIO
Length of output: 1958
🏁 Script executed:
# Check if UnitUpdate might have additional fields beyond what was shown
rg -n "interface UnitUpdate" --type=ts -A20Repository: openfrontio/OpenFrontIO
Length of output: 1301
Replace the double as any cast with a direct cast to UnitUpdate.
The cast update as any as { id: number; isActive: boolean } bypasses type safety unnecessarily. Since the type check update.type === GameUpdateType.Unit already narrows the union, you can cast directly to the UnitUpdate type without the as any intermediate step. This makes the intent clearer and preserves type safety.
Proposed fix
if (update.type === GameUpdateType.Unit) {
- const unitUpdate = update as any as { id: number; isActive: boolean };
+ const unitUpdate = update as UnitUpdate;
if (unitUpdate.isActive === false) {
this.planDrivenUnitIds.delete(unitUpdate.id);
}
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/core/game/GameImpl.ts` around lines 238 - 243, The code is using a double
cast for the unit update (update as any as { id: number; isActive: boolean })
which bypasses type safety; change the cast to the proper UnitUpdate type after
the type guard (i.e., when update.type === GameUpdateType.Unit) so you directly
treat update as UnitUpdate and then call
this.planDrivenUnitIds.delete(unitUpdate.id); ensure you reference the
UnitUpdate type and the GameUpdateType.Unit check in GameImpl.ts when replacing
the double cast.
| case "parabola": { | ||
| const existing = this.unitMotionPlans.get(record.unitId); | ||
| if (existing && record.planId <= existing.planId) { | ||
| break; | ||
| } | ||
|
|
||
| const pf = UniversalPathFinding.Parabola(this._map, { | ||
| increment: record.increment, | ||
| distanceBasedHeight: record.distanceBasedHeight, | ||
| directionUp: record.directionUp, | ||
| }); | ||
|
|
||
| const tiles: number[] = [record.src]; | ||
| for (let i = 0; i < 20000; i++) { | ||
| const step = pf.next(record.src, record.dst, record.increment); | ||
| if (step.status === PathStatus.NEXT) { | ||
| tiles.push(step.node); | ||
| continue; | ||
| } | ||
| break; | ||
| } | ||
|
|
||
| this.unitMotionPlans.set(record.unitId, { | ||
| planId: record.planId, | ||
| startTick: record.startTick, | ||
| ticksPerStep: 1, | ||
| path: Uint32Array.from(tiles), | ||
| }); | ||
| break; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# First, find where UniversalPathFinding and ParabolaUniversalPathFinder are defined
echo "=== Searching for UniversalPathFinding ==="
fd . -type f -name "*.ts" -o -name "*.js" | xargs rg -l "UniversalPathFinding|ParabolaUniversalPathFinder" 2>/dev/null | head -20Repository: openfrontio/OpenFrontIO
Length of output: 640
🏁 Script executed:
#!/bin/bash
# Find the ParabolaUniversalPathFinder class definition
echo "=== Finding ParabolaUniversalPathFinder definition ==="
rg "class ParabolaUniversalPathFinder" --type=ts -A 50 2>/dev/nullRepository: openfrontio/OpenFrontIO
Length of output: 4053
🏁 Script executed:
#!/bin/bash
# Search for how UniversalPathFinding.Parabola() is used elsewhere
echo "=== Searching for UniversalPathFinding.Parabola usage ==="
rg "UniversalPathFinding\.Parabola" --type=ts -B 2 -A 8 2>/dev/null | head -100Repository: openfrontio/OpenFrontIO
Length of output: 8914
🏁 Script executed:
#!/bin/bash
# Check for similar patterns in SteppingPathFinder for comparison
echo "=== Searching for SteppingPathFinder next() method ==="
rg "class SteppingPathFinder|\.next\(" --type=ts -A 3 2>/dev/null | grep -A 3 "SteppingPathFinder\|next(.*dst" | head -50Repository: openfrontio/OpenFrontIO
Length of output: 1390
🏁 Script executed:
#!/bin/bash
# Get the complete ParabolaUniversalPathFinder next() method
cat -n src/core/pathfinding/PathFinder.Parabola.ts | head -150Repository: openfrontio/OpenFrontIO
Length of output: 3393
🏁 Script executed:
#!/bin/bash
# Check SteppingPathFinder interface definition
rg -n "interface SteppingPathFinder|type SteppingPathFinder" -A 15 src/core/pathfinding/Repository: openfrontio/OpenFrontIO
Length of output: 339
🏁 Script executed:
#!/bin/bash
# Check the actual NukeExecution implementation to see if pattern is correct
cat -n src/core/execution/NukeExecution.ts | grep -A 20 "pathFinder.next"Repository: openfrontio/OpenFrontIO
Length of output: 968
🏁 Script executed:
#!/bin/bash
# Check if there's a constants file or if max iterations should be defined
rg -n "20000" src/core/game/GameView.ts -B 3 -A 3Repository: openfrontio/OpenFrontIO
Length of output: 365
🏁 Script executed:
#!/bin/bash
# Look at the full loop context to understand the iteration safety
sed -n '847,863p' src/core/game/GameView.ts | cat -nRepository: openfrontio/OpenFrontIO
Length of output: 704
🏁 Script executed:
#!/bin/bash
# Check if other pathfinders use similar patterns
rg "for.*let.*i.*0.*<" src/core/execution/ -B 2 -A 8 | head -60Repository: openfrontio/OpenFrontIO
Length of output: 4366
Replace hardcoded 20000 iteration limit with a named constant.
The loop uses 20000 as an arbitrary iteration cap with no explanation. Parabolic paths (for nuke trajectories, etc.) are typically 26–43 tiles even on large maps, so this limit is excessive and unclear. Define a named constant like const MAX_PARABOLA_STEPS = 1000; (or an appropriate value) to clarify intent and catch bugs if paths unexpectedly hit the limit.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/core/game/GameView.ts` around lines 834 - 863, In the "parabola" branch
of GameView (inside the switch case "parabola") replace the hardcoded loop limit
20000 with a named constant (e.g. const MAX_PARABOLA_STEPS = 1000) and use that
constant in the for loop that iterates pf.next(record.src, record.dst,
record.increment); update the code that builds tiles and the unitMotionPlans
entry to rely on that constant so the cap is explicit and adjustable (refer to
symbols: the "parabola" case, pf (UniversalPathFinding.Parabola), tiles array,
and unitMotionPlans.set).
github-project-automation
bot
moved this from Triage
to Development
in OpenFront Release Management
1 participant
If this PR fixes an issue, link it below. If not, delete these two lines.
Resolves #(issue number)
Description:
wip
Describe the PR.
Please complete the following:
Please put your Discord username so you can be contacted if a bug or regression is found:
DISCORD_USERNAME