Tower defense game#424
Conversation
Co-authored-by: Andrew Milich <milichab@gmail.com>
|
Cursor Agent can help with this pull request. Just |
|
Your pull request is now ready for review with Assert. Stop waiting for your code to break. Ship with confidence using Assert. |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| const TILE_PATH: Set<string> = new Set( | ||
| PATH_CELLS.map((cell) => `${cell.x},${cell.y}`) | ||
| ); |
There was a problem hiding this comment.
🔴 TILE_PATH only contains waypoint cells, not intermediate path cells — towers can be placed on the path
TILE_PATH is built from PATH_CELLS which only lists the 7 waypoint corners of the path (e.g. (0,2), (3,2), (3,7), etc.). The isOnPath() function checks against this set, but the actual enemy path includes all intermediate cells between waypoints.
Root Cause and Impact
At line 70-72, TILE_PATH is constructed by mapping only the waypoint cells:
const TILE_PATH: Set<string> = new Set(
PATH_CELLS.map((cell) => `${cell.x},${cell.y}`)
);The path goes from (0,2) → (3,2) horizontally, meaning cells (1,2) and (2,2) are on the enemy route. But TILE_PATH only has 7 entries (the waypoints), while the actual path covers 25 cells. The 18 intermediate cells are missing.
This causes two problems:
- Towers can be placed on the path (line 436):
isOnPath(col, row)returnsfalsefor intermediate cells like(1,2), so the placement check passes and a tower is placed directly on the enemy route. - Visual mismatch (line 183): Intermediate path cells are not highlighted with the yellow background, making the path appear as disconnected dots rather than a continuous route.
Placing towers on the path doesn't block enemies (they move by distance along the path line, not by grid cells), so the tower just wastes coins while enemies walk right through it.
Prompt for agents
In src/app/tower-defense/page.tsx, lines 70-72, TILE_PATH is built from only the waypoint cells in PATH_CELLS. It needs to include ALL cells along the path segments between waypoints. Replace the TILE_PATH construction with code that iterates over consecutive pairs of PATH_CELLS and adds every intermediate cell. For each pair of consecutive waypoints, if they share the same x coordinate, iterate over all y values between them; if they share the same y coordinate, iterate over all x values between them. Add each intermediate cell as a string 'x,y' to the Set. This will fix both the tower placement check in isOnPath() at line 133 and the visual path highlighting at line 183.
Was this helpful? React with 👍 or 👎 to provide feedback.
| requestAnimationFrame(gameLoop); | ||
| }; | ||
|
|
||
| const handle = requestAnimationFrame(gameLoop); | ||
| return () => { | ||
| cancelAnimationFrame(handle); |
There was a problem hiding this comment.
🔴 Game loop continues running after component unmount due to uncancelled recursive requestAnimationFrame
The cleanup function in the game loop useEffect only cancels the initial requestAnimationFrame handle, but once the callback fires it schedules a new frame whose handle is never stored or cancelled.
Root Cause and Impact
At lines 492-508, the game loop is set up as:
const handle = requestAnimationFrame(gameLoop);
return () => {
cancelAnimationFrame(handle);
};Inside gameLoop (line 500), requestAnimationFrame(gameLoop) is called recursively, returning a new handle that is never stored. When the cleanup runs, cancelAnimationFrame(handle) cancels the original handle (which has already fired and is a no-op), while the most recently scheduled frame continues to fire. This causes the game loop to keep running after the component unmounts, calling setGameState and setHudTick on an unmounted component indefinitely.
Impact: Memory leak and wasted CPU cycles. On every frame after unmount, simulate() runs and React state setters are called on a stale component. If the user navigates away and back, multiple game loops will stack up.
The standard fix is to use a ref or a mutable cancelled flag that the cleanup sets to true, and check it at the top of gameLoop before scheduling the next frame.
| requestAnimationFrame(gameLoop); | |
| }; | |
| const handle = requestAnimationFrame(gameLoop); | |
| return () => { | |
| cancelAnimationFrame(handle); | |
| frameHandle = requestAnimationFrame(gameLoop); | |
| }; | |
| let frameHandle = requestAnimationFrame(gameLoop); | |
| return () => { | |
| cancelAnimationFrame(frameHandle); |
Was this helpful? React with 👍 or 👎 to provide feedback.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
3 issues found across 1 file
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="src/app/tower-defense/page.tsx">
<violation number="1" location="src/app/tower-defense/page.tsx:70">
P1: `TILE_PATH` only contains the 7 waypoint cells, not the intermediate cells the path actually traverses. Towers can be placed on the visible path at any non-waypoint tile (e.g., cells (1,2), (2,2) between the first two waypoints). You need to interpolate all cells between consecutive waypoints when building the set.</violation>
<violation number="2" location="src/app/tower-defense/page.tsx:500">
P1: The `requestAnimationFrame` chain leaks after unmount. Only the initial frame ID is cancelled, but once the first callback fires, each recursive `requestAnimationFrame(gameLoop)` produces a new untracked ID. Use a `running` flag to break the chain on cleanup.</violation>
<violation number="3" location="src/app/tower-defense/page.tsx:515">
P2: Setting `canvas.width` / `canvas.height` every frame (driven by `hudTick`) forces the browser to reallocate the canvas buffer ~60 times/sec. Move the DPR/sizing setup into a separate one-time effect, and only call `renderFrame` in this effect.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review, or fix all with cubic.
|
|
||
| setGameState((state) => simulate(state, delta)); | ||
| setHudTick((tick) => tick + 1); | ||
| requestAnimationFrame(gameLoop); |
There was a problem hiding this comment.
P1: The requestAnimationFrame chain leaks after unmount. Only the initial frame ID is cancelled, but once the first callback fires, each recursive requestAnimationFrame(gameLoop) produces a new untracked ID. Use a running flag to break the chain on cleanup.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/app/tower-defense/page.tsx, line 500:
<comment>The `requestAnimationFrame` chain leaks after unmount. Only the initial frame ID is cancelled, but once the first callback fires, each recursive `requestAnimationFrame(gameLoop)` produces a new untracked ID. Use a `running` flag to break the chain on cleanup.</comment>
<file context>
@@ -0,0 +1,634 @@
+
+ setGameState((state) => simulate(state, delta));
+ setHudTick((tick) => tick + 1);
+ requestAnimationFrame(gameLoop);
+ };
+
</file context>
| y: cell.y * TILE + TILE * 0.5, | ||
| })); | ||
|
|
||
| const TILE_PATH: Set<string> = new Set( |
There was a problem hiding this comment.
P1: TILE_PATH only contains the 7 waypoint cells, not the intermediate cells the path actually traverses. Towers can be placed on the visible path at any non-waypoint tile (e.g., cells (1,2), (2,2) between the first two waypoints). You need to interpolate all cells between consecutive waypoints when building the set.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/app/tower-defense/page.tsx, line 70:
<comment>`TILE_PATH` only contains the 7 waypoint cells, not the intermediate cells the path actually traverses. Towers can be placed on the visible path at any non-waypoint tile (e.g., cells (1,2), (2,2) between the first two waypoints). You need to interpolate all cells between consecutive waypoints when building the set.</comment>
<file context>
@@ -0,0 +1,634 @@
+ y: cell.y * TILE + TILE * 0.5,
+}));
+
+const TILE_PATH: Set<string> = new Set(
+ PATH_CELLS.map((cell) => `${cell.x},${cell.y}`)
+);
</file context>
| const ctx = canvas?.getContext('2d'); | ||
| if (!canvas || !ctx) return; | ||
| const dpr = window.devicePixelRatio || 1; | ||
| canvas.width = BOARD_WIDTH * dpr; |
There was a problem hiding this comment.
P2: Setting canvas.width / canvas.height every frame (driven by hudTick) forces the browser to reallocate the canvas buffer ~60 times/sec. Move the DPR/sizing setup into a separate one-time effect, and only call renderFrame in this effect.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/app/tower-defense/page.tsx, line 515:
<comment>Setting `canvas.width` / `canvas.height` every frame (driven by `hudTick`) forces the browser to reallocate the canvas buffer ~60 times/sec. Move the DPR/sizing setup into a separate one-time effect, and only call `renderFrame` in this effect.</comment>
<file context>
@@ -0,0 +1,634 @@
+ const ctx = canvas?.getContext('2d');
+ if (!canvas || !ctx) return;
+ const dpr = window.devicePixelRatio || 1;
+ canvas.width = BOARD_WIDTH * dpr;
+ canvas.height = BOARD_HEIGHT * dpr;
+ canvas.style.width = `${BOARD_WIDTH}px`;
</file context>
2 participants
Implement a new standalone tower defense game accessible at
/tower-defense.