Initial commit

code_review_findings.md

@@ -0,0 +1,77 @@
+# Code Review Findings
+
+## Fixed
+
+### 1. Camera and PlayerController input conflict
+**Files:** `scripts/camera_controller.gd`, `nodes/player_controller.gd`
+
+Both scripts used `_unhandled_input` with left mouse button. The camera used left click for dragging and never called `set_input_as_handled()`, so every left click also triggered unit selection/movement. Fixed with a drag threshold and click-on-release pattern.
+
+---
+
+## Outstanding
+
+### 2. Unit lookup is fragile
+**File:** `nodes/player_controller.gd:75`
+
+`_get_unit_at()` iterates `get_parent().get_children()` to find units. This couples the controller to a specific scene tree layout and will degrade as the scene grows.
+
+**Fix:** Use a group (`"units"`) or a dedicated unit registry so lookup doesn't depend on parent structure.
+
+### 3. No pathfinding — movement can skip walls diagonally
+**File:** `nodes/player_controller.gd:45-60`
+
+Step-by-step movement picks one axis per frame. If the goal is diagonally past an L-shaped wall, the unit can path around it depending on approach angle since only the next immediate tile is checked.
+
+**Fix:** Implement BFS/A* on the grid, or at minimum validate the full path before starting movement.
+
+### ~~4. CombatMap extends CanvasItem instead of Node2D~~ (fixed)
+**File:** `nodes/combat_map.gd:2`
+
+Changed to extend `Node2D`.
+
+### ~~5. Inconsistent naming convention~~ (fixed)
+**File:** `nodes/combat_map.gd:16`
+
+Renamed `tileCoords` to `tile_coords`.
+
+### ~~6. unit_template is a string path, not a PackedScene~~ (fixed)
+**File:** `scripts/test_map_generator.gd:7`
+
+Changed to `@export var unit_template: PackedScene` and removed the `load()` call. **Note:** The scene must be assigned in the Godot inspector on the TestMapGenerator node.
+
+### ~~7. No current_hp on units~~ (fixed)
+**File:** `nodes/unit.gd`
+
+Added `current_hp` variable, initialized to `max_hp` in `_ready()`.
+
+### ~~8. Debug print left in production code~~ (fixed)
+**File:** `scripts/test_map_generator.gd`
+
+Removed the debug `print()` call.
+
+### 9. tile_highlight.gd runs every frame unconditionally
+**File:** `scripts/tile_highlight.gd:12-17`
+
+`_process` updates position and alpha every frame even when the highlight is hidden. Could skip processing when invisible via `set_process(false)` on hide.
+
+### 10. GridOverlay is wired but unused
+**Files:** `scripts/grid_overlay.gd`, `nodes/combat_map.gd:6`
+
+`highlight_map` reference exists but is never called from any game logic. Dead code at the moment.
+
+---
+
+## Priority
+
+| Priority | Issue |
+|----------|-------|
+| ~~High~~ | ~~#1 Input conflict~~ (fixed) |
+| High | #3 Wall-skipping movement |
+| Medium | #2 Fragile unit lookup |
+| ~~Medium~~ | ~~#6 String path instead of PackedScene~~ (fixed) |
+| ~~Low~~ | ~~#4 CanvasItem base class~~ (fixed) |
+| ~~Low~~ | ~~#5 Inconsistent naming~~ (fixed) |
+| ~~Low~~ | ~~#7 No current_hp~~ (fixed) |
+| ~~Low~~ | ~~#8 Debug print~~ (fixed) |
+| Low | #9, #10 |