Conversation
…nability Refactor main orchestration into modular helpers
…, repoint ecs-lib
…n-simulation-iwvzyn Render analytic demo dungeon with FOV overlays
…y-items Show pickup tooltip within reach
…-mechanics Improve overlay interactions and movement
…iptref Refactor rules scripting to use shared ScriptRef
…th-emissive-torches Add emissive torch lighting with occlusion
…add-lightning-spell-mechanic Add lightning gesture casting and improve movement
wooden bow + projectile
…hanics Fix touch targeting and ranged ammo handling
There was a problem hiding this comment.
Pull Request Overview
This PR refactors the movement system from continuous analytic geometry to grid-aligned tile-based movement. The system now uses a tile map for collision detection and pathfinding, replacing the previous geometry kernel sweep-based movement.
Key changes:
- Introduced a new tile map system with walkability and opacity flags
- Replaced sweep-based continuous movement with cardinal-direction grid steps
- Updated positions to use integer grid coordinates throughout
Reviewed Changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/rules/environment/tileMap.js | New module providing tile map data structure and utilities for grid-based collision |
| src/rules/systems/movementSystem.js | Refactored to use cardinal steps on tile grid instead of analytic geometry sweeps |
| src/rules/systems/aiChaseSystem.js | Updated AI to chase using grid-aligned cardinal directions |
| src/rules/systems/interactionSystem.js | Added tile map updates when doors open/close |
| src/rules/environment/dungeonGenerator.js | Modified to generate tile-based dungeons and sync with geometry kernel |
| src/rules/environment/index.js | Added exports for tile map functions |
| src/main/scene/demoScene.js | Adjusted entity positions to grid-aligned integer coordinates |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
src/rules/systems/movementSystem.js
Outdated
| if (absX !== 0) return { x: Math.sign(dx), y: 0 }; | ||
| return { x: 0, y: Math.sign(dy) }; |
There was a problem hiding this comment.
Lines 22-23 are unreachable. When absX === absY (and both non-zero), line 20 evaluates to false and line 21 evaluates to false, but then line 22 checks if absX !== 0 which will always be true in this case. This means line 22 will always execute for the absX === absY case, making line 23 unreachable. Consider simplifying to: return { x: Math.sign(dx), y: 0 }; on line 22 without the condition, or if you want Y-preference when equal, change line 22's condition to if (absY === 0).
| if (absX !== 0) return { x: Math.sign(dx), y: 0 }; | |
| return { x: 0, y: Math.sign(dy) }; | |
| // If absX === absY and both non-zero, prefer X direction | |
| return { x: Math.sign(dx), y: 0 }; |
src/rules/systems/movementSystem.js
Outdated
| const key = tileKey(nextX, nextY); | ||
| const occ = tileOccupants.get(key) || null; | ||
| if (!walkable) { | ||
| blockedBy = occ; |
There was a problem hiding this comment.
When a tile is not walkable but has no occupant (occ is null), blockedBy is set to null. This could cause issues in the subsequent blockedBy check at line 138 where blockedBy?.interactable is accessed. If the intent is to only set blockedBy when there's an actual occupant, the assignment should be conditional: if (occ) blockedBy = occ;. If unwalkable tiles should always block regardless of occupants, the logic is correct but could be clearer.
| blockedBy = occ; | |
| if (occ) blockedBy = occ; |
Summary
Testing
npm test(fails: repository is missing src/lib/ecs-js/index.js so the test runner cannot resolve the ECS library)Codex Task