Skip to content

perf: optimize hot loops, Set lookups, and O(n) path reconstruction#444

Open
devin-ai-integration[bot] wants to merge 1 commit into
mainfrom
devin/1775062033-perf-improvements
Open

perf: optimize hot loops, Set lookups, and O(n) path reconstruction#444
devin-ai-integration[bot] wants to merge 1 commit into
mainfrom
devin/1775062033-perf-improvements

Conversation

@devin-ai-integration

@devin-ai-integration devin-ai-integration Bot commented Apr 1, 2026

Copy link
Copy Markdown
Contributor

Summary

Incremental performance improvements targeting hot paths in the vehicle/entity systems and pathfinding:

  • gridFinders.ts: Convert SPORTS_TYPES, RELAXATION_TYPES, ACTIVE_RECREATION_TYPES, and ENTERABLE_BUILDING_TYPES from arrays to Sets for O(1) .has() lookups instead of O(n) .includes(). Backward-compatible array exports are preserved.
  • utils.ts: Fix O(n²) BFS path reconstruction in findPathOnRoads — previously stored parent coordinates and did a linear search to find the parent's queue index on each step. Now stores the parent queue index directly (BFS_PARENT_IDX), making reconstruction O(path length).
  • vehicleSystems.ts:
    • Replace forEach with for loops in drawCars, drawBuses, drawEmergencyVehicles to avoid closure allocation per entity per frame.
    • Remove [...array] spread copies in updateCars and updateBuses — iterate the original ref array directly.
    • Add viewport culling to drawCars (was missing; buses/emergency vehicles already had it).
    • Hoist skipTypes array in isVehicleBehindBuilding to a module-level VEHICLE_SKIP_BUILDING_TYPES Set to avoid re-allocation every call.

Review & Testing Checklist for Human

  • Verify drawCars viewport culling margin (20px) is sufficient. Buses use 60/80px and emergency vehicles use 40/60px margins. Cars are smaller, but check that cars near the viewport edge don't visibly pop in/out during panning.
  • Verify updateCars/updateBuses direct iteration is safe. The spread copy was removed — confirm no code path during the loop body modifies carsRef.current or busesRef.current itself (adding/removing elements). Mutating individual car/bus properties is fine.
  • Test pathfinding still works correctly — place buildings far apart, verify emergency vehicles and buses can still navigate to them. The BFS reconstruction change is the most algorithmically significant edit.
  • Visual smoke test — play the game for a few minutes at various zoom levels. Confirm all vehicle types (cars, buses, emergency vehicles) render correctly and no entities disappear unexpectedly.

Notes

  • All pre-existing lint errors/warnings are unchanged (19 errors, 12 warnings — none introduced by this PR).
  • The SPORTS_TYPES, RELAXATION_TYPES, ACTIVE_RECREATION_TYPES array exports are kept for backward compatibility in case external consumers iterate them.

Link to Devin session: https://app.devin.ai/sessions/0ac3bedf903a4d1b8a552966e05e346a
Requested by: @amilich


Summary by cubic

Improves simulation performance by speeding up hot paths in pathfinding and vehicle rendering. Uses Set lookups, reduces per-frame allocations, and makes BFS path reconstruction O(path length).

  • Performance
    • Converted recreation/enterable building arrays to Sets for O(1) .has() checks; kept array exports for iteration.
    • Made BFS path reconstruction O(path length) by storing the parent queue index instead of coordinates.
    • Reduced per-frame allocations in vehicle systems by replacing forEach with for loops and iterating original arrays (no spread copies).
    • Added viewport culling to drawCars (20px margin) and hoisted occlusion skip types to a module-level Set.

Written for commit 26557e5. Summary will update on new commits.

- vehicleSystems.ts: Replace forEach with for loops to avoid closure allocation per iteration
- vehicleSystems.ts: Remove array spread copies in updateCars/updateBuses (iterate directly)
- vehicleSystems.ts: Add viewport culling to drawCars to skip off-screen vehicles
- vehicleSystems.ts: Use module-level VEHICLE_SKIP_BUILDING_TYPES Set for occlusion checks
- gridFinders.ts: Convert SPORTS_TYPES, RELAXATION_TYPES, ACTIVE_RECREATION_TYPES, ENTERABLE_BUILDING_TYPES to Sets for O(1) lookups
- utils.ts: Fix O(n²) BFS path reconstruction by storing parent queue index directly instead of parent coordinates

Co-Authored-By: Andrew Milich <milichab@gmail.com>
@devin-ai-integration devin-ai-integration Bot requested a review from amilich April 1, 2026 16:51
@devin-ai-integration

Copy link
Copy Markdown
Contributor Author

🤖 Devin AI Engineer

I'll be helping with this pull request! Here's what you should know:

✅ I will automatically:

  • Address comments on this PR. Add '(aside)' to your comment to have me ignore it.
  • Look at CI failures and help fix them

Note: I can only respond to comments from users who have write access to this repository.

⚙️ Control Options:

  • Disable automatic comment and CI monitoring

@vercel

vercel Bot commented Apr 1, 2026

Copy link
Copy Markdown
Contributor

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated (UTC)
isometric-city Ready Ready Preview, Comment Apr 1, 2026 4:52pm

Request Review

@cubic-dev-ai cubic-dev-ai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

No issues found across 3 files

@deepsource-io

deepsource-io Bot commented Apr 1, 2026

Copy link
Copy Markdown

DeepSource Code Review

We reviewed changes in 50f5e1d...26557e5 on this pull request. Below is the summary for the review, and you can see the individual issues we found as inline review comments.

See full review on DeepSource ↗

PR Report Card

Overall Grade   Security  

Reliability  

Complexity  

Hygiene  

Code Review Summary

Analyzer Status Updated (UTC) Details
JavaScript Apr 1, 2026 10:57p.m. Review ↗
Shell Apr 1, 2026 10:57p.m. Review ↗
Secrets Apr 1, 2026 10:57p.m. Review ↗

Comment on lines 210 to 212
export function isSportsFacility(buildingType: BuildingType): boolean {
return SPORTS_TYPES.includes(buildingType);
return SPORTS_TYPES_SET.has(buildingType);
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Unexpected function declaration in the global scope, wrap in an IIFE for a local variable, assign as global property for a global variable


It is considered a best practice to avoid 'polluting' the global scope with variables that are intended to be local to the script. Global variables created from a script can produce name collisions with global variables created from another script, which will usually lead to runtime errors or unexpected behavior. It is mostly useful for browser scripts.

Comment on lines 217 to 219
export function isRelaxationArea(buildingType: BuildingType): boolean {
return RELAXATION_TYPES.includes(buildingType);
return RELAXATION_TYPES_SET.has(buildingType);
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Unexpected function declaration in the global scope, wrap in an IIFE for a local variable, assign as global property for a global variable


It is considered a best practice to avoid 'polluting' the global scope with variables that are intended to be local to the script. Global variables created from a script can produce name collisions with global variables created from another script, which will usually lead to runtime errors or unexpected behavior. It is mostly useful for browser scripts.

Comment on lines 224 to 226
export function isEnterableBuilding(buildingType: BuildingType): boolean {
return ENTERABLE_BUILDING_TYPES.includes(buildingType);
return ENTERABLE_BUILDING_TYPES_SET.has(buildingType);
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Unexpected function declaration in the global scope, wrap in an IIFE for a local variable, assign as global property for a global variable


It is considered a best practice to avoid 'polluting' the global scope with variables that are intended to be local to the script. Global variables created from a script can produce name collisions with global variables created from another script, which will usually lead to runtime errors or unexpected behavior. It is mostly useful for browser scripts.

Comment on lines +978 to 984
for (let ci = 0; ci < carsRef.current.length; ci++) {
const c = carsRef.current[ci];
const key = c.tileY * currentGridSize + c.tileX;
const bucket = carsByTile.get(key);
if (bucket) bucket.push(c);
else carsByTile.set(key, [c]);
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Expected a `for-of` loop instead of a `for` loop with this simple iteration


A for-of loop is recommended when the loop index is only used to read from the collection.

if (!carsByTile.has(key)) carsByTile.set(key, []);
carsByTile.get(key)!.push(car);
for (let ci = 0; ci < carsRef.current.length; ci++) {
const c = carsRef.current[ci];

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Variable name is too small


Short variable names affect code readability and complicate code refactoring, because of the difficulty in searching and replacing such short characters.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant