Conversation
There was a problem hiding this comment.
Pull request overview
This pull request adds laser firing functionality to the game, allowing players to shoot lasers that can destroy boxes (grid value 2) and are stopped by walls (grid value 1). The implementation includes a new raycasting utility, game state methods for laser application, and a message handler for client-initiated laser firing.
Key Changes:
- New raycasting algorithm to trace laser paths through the grid
- Three new methods in RoomState for applying laser damage (
applyLaser,applyLaserAt,applyLaserRay) - New
fire_lasermessage handler in GameRoom that broadcasts laser events to all clients - Grid initialization changed from mixed values to all boxes (value 2) for testing
- Dependency version bumps for @colyseus/core and jws packages
Reviewed changes
Copilot reviewed 3 out of 4 changed files in this pull request and generated 14 comments.
| File | Description |
|---|---|
| src/rooms/laserRaycast.ts | New raycasting utility that traces laser path from start position in given direction until hitting wall or box |
| src/rooms/schema/RoomState.ts | Added laser application logic with three methods and extensive debug logging; changed grid initialization to all boxes |
| src/rooms/GameRoom.ts | Added fire_laser message handler with default position/direction fallbacks and broadcasts for laser_fired and box_destroyed events |
| package-lock.json | Version updates for @colyseus/core (0.16.23 → 0.16.24) and jws dependencies (3.2.2 → 3.2.3, 4.0.0 → 4.0.1) |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| applyLaser(x: number, y: number, ownerId: string) { | ||
| const index = y * this.width + x; | ||
| const value = this.grid[index]; | ||
|
|
||
| if (value === 2) { | ||
|
|
||
| this.grid[index] = 0; | ||
|
|
||
| return { x, y }; | ||
| } | ||
| } |
There was a problem hiding this comment.
The applyLaser method is defined but never used in the codebase. It appears to be superseded by the applyLaserAt and applyLaserRay methods. Consider removing this unused method to reduce code clutter, or document if it's intended for future use.
| applyLaser(x: number, y: number, ownerId: string) { | |
| const index = y * this.width + x; | |
| const value = this.grid[index]; | |
| if (value === 2) { | |
| this.grid[index] = 0; | |
| return { x, y }; | |
| } | |
| } |
| const start = payload && payload.start ? payload.start : { x: 1, y: 1 }; | ||
| const dir = payload && payload.dir ? payload.dir : { dx: 1, dy: 0 }; |
There was a problem hiding this comment.
The fire_laser message handler lacks input validation for the payload parameters. If payload.start.x, payload.start.y, payload.dir.dx, or payload.dir.dy are not numbers or are undefined, this could lead to unexpected behavior. Consider validating that these values exist and are valid numbers before processing.
| const start = payload && payload.start ? payload.start : { x: 1, y: 1 }; | |
| const dir = payload && payload.dir ? payload.dir : { dx: 1, dy: 0 }; | |
| // Validate and sanitize input | |
| const rawStart = payload && payload.start ? payload.start : {}; | |
| const rawDir = payload && payload.dir ? payload.dir : {}; | |
| const start = { | |
| x: (typeof rawStart.x === "number" && isFinite(rawStart.x)) ? rawStart.x : 1, | |
| y: (typeof rawStart.y === "number" && isFinite(rawStart.y)) ? rawStart.y : 1, | |
| }; | |
| const dir = { | |
| dx: (typeof rawDir.dx === "number" && isFinite(rawDir.dx)) ? rawDir.dx : 1, | |
| dy: (typeof rawDir.dy === "number" && isFinite(rawDir.dy)) ? rawDir.dy : 0, | |
| }; |
src/rooms/laserRaycast.ts
Outdated
| const stepX = Math.sign(dirX); | ||
| const stepY = Math.sign(dirY); | ||
|
|
||
| let x = startX + stepX; | ||
| let y = startY + stepY; | ||
|
|
||
| while (x >= 0 && x < width && y >= 0 && y < height) { | ||
| const idx = y * width + x; | ||
| const val = grid[idx]; | ||
| cells.push({ x, y, value: val }); | ||
|
|
||
| if (val === 1 || val === 2) { | ||
| break; | ||
| } | ||
|
|
||
| x += stepX; | ||
| y += stepY; |
There was a problem hiding this comment.
The raycast algorithm doesn't handle diagonal directions correctly. When both dirX and dirY are non-zero, using Math.sign() and stepping by 1 in each direction will move diagonally, but it won't traverse all cells that a true ray would intersect. This could cause the laser to miss boxes or walls. Consider implementing a proper DDA or Bresenham-based line algorithm for accurate raycasting.
| const stepX = Math.sign(dirX); | |
| const stepY = Math.sign(dirY); | |
| let x = startX + stepX; | |
| let y = startY + stepY; | |
| while (x >= 0 && x < width && y >= 0 && y < height) { | |
| const idx = y * width + x; | |
| const val = grid[idx]; | |
| cells.push({ x, y, value: val }); | |
| if (val === 1 || val === 2) { | |
| break; | |
| } | |
| x += stepX; | |
| y += stepY; | |
| // DDA algorithm for grid traversal | |
| let x = startX; | |
| let y = startY; | |
| // Normalize direction | |
| const mag = Math.sqrt(dirX * dirX + dirY * dirY); | |
| const dx = dirX / mag; | |
| const dy = dirY / mag; | |
| // Calculate step and initial tMax, tDelta | |
| const stepX = dx > 0 ? 1 : (dx < 0 ? -1 : 0); | |
| const stepY = dy > 0 ? 1 : (dy < 0 ? -1 : 0); | |
| const gridX = Math.floor(x); | |
| const gridY = Math.floor(y); | |
| let tMaxX, tMaxY; | |
| let tDeltaX, tDeltaY; | |
| if (stepX !== 0) { | |
| tDeltaX = Math.abs(1 / dx); | |
| tMaxX = stepX > 0 | |
| ? ((Math.floor(x) + 1 - x) / dx) | |
| : ((x - Math.floor(x)) / -dx); | |
| } else { | |
| tDeltaX = Infinity; | |
| tMaxX = Infinity; | |
| } | |
| if (stepY !== 0) { | |
| tDeltaY = Math.abs(1 / dy); | |
| tMaxY = stepY > 0 | |
| ? ((Math.floor(y) + 1 - y) / dy) | |
| : ((y - Math.floor(y)) / -dy); | |
| } else { | |
| tDeltaY = Infinity; | |
| tMaxY = Infinity; | |
| } | |
| let currX = Math.floor(x); | |
| let currY = Math.floor(y); | |
| // Start from the cell after the starting cell | |
| while ( | |
| currX >= 0 && currX < width && | |
| currY >= 0 && currY < height | |
| ) { | |
| const idx = currY * width + currX; | |
| const val = grid[idx]; | |
| cells.push({ x: currX, y: currY, value: val }); | |
| if (val === 1 || val === 2) { | |
| break; | |
| } | |
| if (tMaxX < tMaxY) { | |
| tMaxX += tDeltaX; | |
| currX += stepX; | |
| } else { | |
| tMaxY += tDeltaY; | |
| currY += stepY; | |
| } |
src/rooms/laserRaycast.ts
Outdated
| const stepX = Math.sign(dirX); | ||
| const stepY = Math.sign(dirY); | ||
|
|
||
| let x = startX + stepX; | ||
| let y = startY + stepY; | ||
|
|
||
| while (x >= 0 && x < width && y >= 0 && y < height) { | ||
| const idx = y * width + x; | ||
| const val = grid[idx]; | ||
| cells.push({ x, y, value: val }); | ||
|
|
||
| if (val === 1 || val === 2) { | ||
| break; | ||
| } | ||
|
|
||
| x += stepX; | ||
| y += stepY; | ||
| } |
There was a problem hiding this comment.
The function doesn't handle the edge case where both dirX and dirY are 0. In this case, stepX and stepY will both be 0, causing an infinite loop since x and y never change. Consider adding validation to ensure at least one direction component is non-zero, or return an empty array early for this case.
src/rooms/GameRoom.ts
Outdated
|
|
||
| const start = payload && payload.start ? payload.start : { x: 1, y: 1 }; | ||
| const dir = payload && payload.dir ? payload.dir : { dx: 1, dy: 0 }; | ||
|
|
||
| const result = this.state.applyLaserRay(start.x, start.y, dir.dx, dir.dy); | ||
|
|
||
| this.broadcast("laser_fired", { start, dir, color: payload?.color || "red", hits: result.hits }); | ||
|
|
||
|
|
||
| if (result.hits && result.hits.length > 0) { | ||
| const destroyedHits = result.hits.filter((h: any) => h.type === "box"); | ||
| if (destroyedHits.length > 0) { | ||
| this.broadcast("box_destroyed", { hits: destroyedHits, color: payload?.color || "red" }); | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
[nitpick] There are inconsistent blank lines in this block. Line 25 has trailing whitespace, and there are multiple blank lines (32-33, 39-40) that don't follow a consistent pattern compared to the rest of the file. Consider normalizing the whitespace for better code consistency.
| return this.playerState.getPlayerName(sessionId); | ||
| } | ||
|
|
||
| applyLaser(x: number, y: number, ownerId: string) { |
There was a problem hiding this comment.
The ownerId parameter in applyLaser is never used within the function. If this parameter is not needed, it should be removed. If it's intended for future authorization or tracking purposes, consider documenting this or implementing the logic.
| applyLaser(x: number, y: number, ownerId: string) { | |
| applyLaser(x: number, y: number) { |
src/rooms/laserRaycast.ts
Outdated
| export function raycastToWall( | ||
| grid: number[], | ||
| width: number, | ||
| height: number, | ||
| startX: number, | ||
| startY: number, | ||
| dirX: number, | ||
| dirY: number, | ||
| ): Cell[] { |
There was a problem hiding this comment.
The raycastToWall function lacks documentation explaining its parameters, return value, and the grid value meanings (e.g., what do values 0, 1, and 2 represent?). Adding JSDoc comments would improve code maintainability and make the API clearer to other developers.
src/rooms/schema/RoomState.ts
Outdated
| const after = this.grid[idx]; | ||
| console.log(`[applyLaserAt] destroyed box at (${x},${y}) idx=${idx} after=${after}`); |
There was a problem hiding this comment.
The variable after on line 204 is assigned but never used. This appears to be leftover debugging code. Consider removing this line since the value is only logged and serves no functional purpose.
| const after = this.grid[idx]; | |
| console.log(`[applyLaserAt] destroyed box at (${x},${y}) idx=${idx} after=${after}`); | |
| console.log(`[applyLaserAt] destroyed box at (${x},${y}) idx=${idx} after=${this.grid[idx]}`); |
src/rooms/GameRoom.ts
Outdated
|
|
||
|
|
||
| if (result.hits && result.hits.length > 0) { | ||
| const destroyedHits = result.hits.filter((h: any) => h.type === "box"); |
There was a problem hiding this comment.
The type annotation h: any bypasses type safety. Since result.hits is already typed as { type: "box" | "wall"; x: number; y: number }[] on line 231, you can use proper typing here: (h) => h.type === "box" or explicitly type it as (h: { type: "box" | "wall"; x: number; y: number }). This would provide better type checking and IDE support.
| const destroyedHits = result.hits.filter((h: any) => h.type === "box"); | |
| const destroyedHits = result.hits.filter((h) => h.type === "box"); |
src/rooms/laserRaycast.ts
Outdated
| @@ -0,0 +1,34 @@ | |||
| export type Cell = { x: number; y: number; value?: number }; | |||
There was a problem hiding this comment.
The Cell type has an optional value property but it's not clear when this would be undefined versus when it would have a value. Based on the implementation, value is always set when cells are pushed in the raycastToWall function. Consider making value required (non-optional) or documenting when it might be undefined.
| export type Cell = { x: number; y: number; value?: number }; | |
| export type Cell = { x: number; y: number; value: number }; |
|
added solvro/config |
No description provided.