-
Notifications
You must be signed in to change notification settings - Fork 0
Restore orthogonal grid navigation #12
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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) }; |
Copilot
AI
Nov 8, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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; |
Copilot
AI
Nov 8, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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