Code review

docs/review/2026-04-07-architecture-review.md

@@ -0,0 +1,266 @@
+# Architecture & Code Quality Review — Dungeon Lords
+
+Reviewed: 2026-04-07
+Previous review: `architecture_review.md` (2026-04-02)
+
+This pass re-checks the 15 issues from the prior review and surfaces new findings introduced by recent work (fog of war, camera fixes, map sizing, corners, room/wall renderer).
+
+---
+
+## Status of Prior Review (2026-04-02)
+
+| # | Issue | Status | Notes |
+|---|-------|--------|-------|
+| 1  | No allegiance check on combat | **Open** | `nodes/player_controller.gd:135` |
+| 2  | No pathfinding | **Partially mitigated** | `is_tile_passable()` now blocks the next step (`player_controller.gd:122`), but units still walk around L-walls one axis at a time and have no movement budget |
+| 3  | Stale combat proposal | **Open** | `apply_proposal()` still has no `is_instance_valid()` guards (`combat_system.gd:129-148`) |
+| 4  | `queue_free()` dangling refs | **Open** | `nodes/unit.gd` still queue_frees immediately on death |
+| 5  | Duplicated unit-tracking | **Open** | `player_controller.gd:26-35` and `scripts/battle/combat_ui.gd:40-50` |
+| 6  | Inline scripts in unit.tscn | **Open** | Two `SubResource` GDScript blocks remain in `prefabs/unit.tscn` |
+| 7  | Two combat code paths | **Open** | `combat_system.gd:114` `process_combat()` is dead (no callers in code, only docs/plans) |
+| 8  | O(n) unit lookup | **Open** | `player_controller.gd:159-167` |
+| 9  | No turn / movement system | **Open** | `spd` still unused |
+| 10 | `atk_range` not enforced | **Open** | Tactic-range is checked when *building* a proposal (`combat_system.gd:25`), but `_handle_left_click` emits `combat_requested` without any pre-check |
+| 11 | tile_highlight always processing | **Resolved** | `tile_highlight.gd` is gone; replaced by `scripts/battle/grid_overlay.gd` which only updates on `target_tile()` calls |
+| 12 | Unused GridOverlay | **Resolved** | Now wired through `StrategyPhase._on_mouse_grid_changed → CombatMap.target_tile()` (`combat_map.gd:85`) |
+| 13 | Jolt 3D physics configured | **Open** | Still in `project.godot` |
+| 14 | `current_hp` default fragile | **Resolved-ish** | `UnitStats._init` sets it; works but `@export` still has no default |
+| 15 | `UnitInfo.name` shadows `Object.name` | **Open** | `resources/resource_definitions/unit_info.gd:3` |
+
+**Tally:** 11 still open, 1 partially mitigated, 3 resolved.
+
+---
+
+## High Priority
+
+### A. `apply_proposal` does not validate unit references
+
+**File:** `nodes/combat_system.gd:129-148`
+
+`process_combat()` was given an `is_instance_valid()` guard at the top, but the canonical path used by the UI — `apply_proposal()` — calls `def_unit.take_damage(...)` and `atk_unit.take_damage(...)` directly without checking either reference. Today this is harmless because `Unit._die()` only happens through this same code path, but the moment a second damage source exists (poison, AoE, traps), a unit can be freed mid-proposal and `apply_proposal` will hit a freed reference.
+
+**Fix:** Guard both unit accesses with `is_instance_valid()`. Better, snapshot the unit references into the proposal at creation time and treat them as nullable from then on.
+
+---
+
+### B. No allegiance check on combat targeting
+
+**File:** `nodes/player_controller.gd:135`
+
+```gdscript
+if _selected_unit and clicked_unit != _selected_unit \
+    and _selected_unit.is_alive() and clicked_unit.is_alive():
+    combat_requested.emit(_selected_unit, clicked_unit)
+```
+
+A player unit can attack a friendly unit. Carried over unchanged from the prior review.
+
+**Fix:** Compare `clicked_unit.current_allegiance.type` against the selected unit, ideally in `StrategyPhase` so `PlayerController` stays allegiance-agnostic.
+
+---
+
+### C. `Unit.queue_free()` on death leaves dangling references in proposals
+
+**File:** `nodes/unit.gd` (`_die`)
+
+When a unit dies it emits `unit_died` and immediately calls `queue_free()`. `CombatUI._on_unit_died` (`scripts/battle/combat_ui.gd:52-58`) compares the dying unit against `_current_proposal.attacker.unit` — that comparison happens *this* frame, so it's fine, but `CombatProposal` still holds the same reference and will be invalid for any code that runs after the deferred free.
+
+**Fix:** Either delay the `queue_free()` until after combat resolution finishes (hide-then-free), or null out unit references in any active proposal as part of the death handler.
+
+---
+
+### D. `attack_range` is never enforced before requesting combat
+
+**Files:** `nodes/player_controller.gd:135`, `nodes/combat_system.gd:22-27`
+
+Tactic-range filtering happens *inside* `create_proposal`, so a click that targets an unreachable enemy will produce a proposal with no valid tactics — and from there the behavior depends on `_find_default_attack` returning `null` and `_snapshot` happily writing zeros. The user-visible bug is that you can click any enemy on the map and a proposal opens regardless of distance.
+
+**Fix:** Check distance against the attacker's max tactic range (or `current_stats.atk_range`) inside `_handle_left_click` before emitting `combat_requested`. Reject the click otherwise.
+
+---
+
+## Medium Priority
+
+### E. Pathfinding still missing; movement is greedy axis-stepping
+
+**File:** `nodes/player_controller.gd:96-127`
+
+`is_tile_passable` now blocks the next single step, which is an improvement, but the mover is still a greedy "pick the larger axis" loop with no global plan. Going around an L-shaped wall succeeds; going around a U-shaped wall doesn't. There's also still no movement budget — a unit can walk to the far corner of the map with a single click.
+
+**Fix:** Compute the full path with BFS/A* over `MapLayout.is_passable` at click time, store it as a list of tiles, and step along it. Add a per-turn movement budget once a turn system exists.
+
+---
+
+### F. Duplicated unit-tracking boilerplate
+
+**Files:** `nodes/player_controller.gd:26-35`, `scripts/battle/combat_ui.gd:40-50`
+
+Both systems independently iterate `get_nodes_in_group("units")`, hook `node_added`, and guard against duplicate connections. Any third system that cares about units will copy this again.
+
+**Fix:** Promote a small `UnitRegistry` autoload that emits `unit_registered(unit)` / `unit_deregistered(unit)`. This ties into finding G below.
+
+---
+
+### G. `CombatMap.remove_unit` doesn't disconnect signals
+
+**File:** `nodes/combat_map.gd:80-82`
+
+`remove_unit` only calls `remove_child`. PlayerController and CombatUI both connected to `unit_died` (and `unit_selected_changed` for PlayerController via the AllegianceIndicator path). If a unit is removed and re-added — or destroyed without going through `_die` — the connections leak. Combined with finding F, an event-bus / registry would solve both.
+
+---
+
+### H. Two combat entry points; `process_combat` is dead
+
+**File:** `nodes/combat_system.gd:114-127`
+
+`process_combat()` has no callers in `nodes/` or `scripts/`; the only references are in plan/spec docs under `docs/superpowers/`. It mirrors the proposal-application path and is liable to drift.
+
+**Fix:** Delete `process_combat()` and let any future AI/auto-combat go through `create_proposal` → `apply_proposal` directly.
+
+---
+
+### I. O(n) unit lookup on every click
+
+**File:** `nodes/player_controller.gd:159-167`
+
+`_get_unit_at` snaps every unit in the group to the grid and compares. Fine for tens of units, slow for hundreds.
+
+**Fix:** Maintain a `Dictionary[Vector2i, Unit]` in `CombatMap` that updates on deploy/move/death. Lookups become O(1) and you get tile-occupancy collision detection for free.
+
+---
+
+### J. Sprite spam in `FogRenderer` and `WallRenderer`
+
+**Files:** `nodes/fog_renderer.gd:13-43`, `nodes/wall_renderer.gd:44-176`
+
+Both renderers create one `Sprite2D` per fog tile / per wall half-segment / per inner corner, with no pooling, and `_clear()` `queue_free`s every child on the next layout change. For a modest 30×30 map this is already in the high hundreds of nodes; for larger maps it scales linearly with `width × height` plus wall surface area, and every map reload thrashes the scene tree.
+
+**Fix:** Override `_draw()` and use `draw_texture_rect_region()` for both renderers. You get the same visual result with one draw call per region instead of N nodes, and `queue_redraw()` on layout change replaces the whole `_clear` + recreate cycle.
+
+---
+
+### K. `MapLayout` requires `initialize()` but exposes no guard
+
+**Files:** `nodes/combat_map.gd:89-101`, `resources/resource_definitions/map_layout.gd`
+
+`apply_layout()` is the only place that calls `map_layout.initialize()`. Anything that constructs a `MapLayout` and calls `is_passable()` / `is_tile_valid()` without going through `apply_layout` will silently see empty internal state.
+
+**Fix:** Either lazy-initialize inside `is_passable` / `is_tile_valid`, or assert that `initialize()` has been called.
+
+---
+
+### L. `target_tile` highlight updates while combat UI is open
+
+**File:** `nodes/player_controller.gd:44-52`
+
+`input_disabled` short-circuits `_unhandled_input` but `_process` still runs and emits `mouse_grid_changed`, so the cursor highlight keeps tracking the mouse during the combat proposal panel. May be intentional, but looks like a bug — the player just told the game "I'm interacting with the modal", not "keep showing me where my next click would land on the map."
+
+**Fix:** Bail out of `_process` (or at least suppress the emit) when `input_disabled` is true.
+
+---
+
+### M. Inline scripts in `prefabs/unit.tscn`
+
+The AllegianceIndicator and selection-highlight scripts are still embedded as `SubResource` GDScript blocks in the scene file. Invisible to grep, painful to refactor.
+
+**Fix:** Extract to `scripts/units/allegiance_indicator.gd` and `scripts/units/unit_selection_highlight.gd`.
+
+---
+
+## Low Priority
+
+### N. `WallRenderer._draw_outer_corners` is a stub
+
+**File:** `nodes/wall_renderer.gd:114-115, 179-183`
+
+The function is called but no-ops. The TODO is fine for now, but the matching atlas constants for outer corners aren't defined either, so when you implement it you'll need to add both. Worth a tracker entry.
+
+---
+
+### O. `WallRenderer._draw_tile_walls` match arms are identical
+
+**File:** `nodes/wall_renderer.gd:103-112`
+
+```gdscript
+match edge:
+    &"left":  _draw_edge_segments(tile_origin, edge)
+    &"right": _draw_edge_segments(tile_origin, edge)
+    &"top":   _draw_edge_segments(tile_origin, edge)
+    &"bottom":_draw_edge_segments(tile_origin, edge)
+```
+
+The whole `match` collapses to `for edge in edges: _draw_edge_segments(tile_origin, edge)`.
+
+---
+
+### P. `FogRenderer.FOG_RECT` is `(53, 53, 100, 100)` — tile-aligned magic constant
+
+**File:** `nodes/fog_renderer.gd:8`
+
+Hardcoded atlas region with no comment about why those coordinates. Same kind of magic numbers that the wall renderer uses, but at least the wall constants have section comments. Add a one-line comment naming the source asset.
+
+---
+
+### Q. `Jolt Physics` configured for a 2D project
+
+**File:** `project.godot`
+
+Carried over from prior review. `3d/physics_engine="Jolt Physics"` is harmless but unused — the game has no 3D nodes. Removing it removes a "huh, why?" for any new contributor.
+
+---
+
+### R. `UnitInfo.name` still shadows `Object.name`
+
+**File:** `resources/resource_definitions/unit_info.gd:3`
+
+Rename to `display_name` or `unit_name` to disambiguate from `Node.name` / `Object.name`.
+
+---
+
+### S. `CombatMap.is_wall` and `is_tile_valid` have legacy fallbacks
+
+**File:** `nodes/combat_map.gd:97-108`
+
+Both methods have a "no room system" fallback path. Now that every map goes through `MapLayout`, the fallback is dead — but it's also the kind of dead code that quietly hides bugs (a missing `map_layout` won't crash, it'll silently fall through to wall-tile checks). Either delete the fallbacks and assert, or document the contract.
+
+---
+
+## Architectural Notes
+
+### Renderer pattern is the right shape, wrong implementation
+`WallRenderer` and `FogRenderer` correctly isolate "draw the map's decoration layer" from `CombatMap`. The split is good. The execution — instantiating Sprite2D children — is the part to revisit. A `_draw()`-based implementation would let both classes keep the same public API (`draw_walls_for_layout(MapLayout)` / `draw_fog_for_layout(MapLayout)`) and their `_clear()` calls would become `queue_redraw()`.
+
+### Combat data flow is solid
+`PlayerController → StrategyPhase → CombatSystem ← CombatUI` is still clean. The remaining issues in this area (A, B, C, D) are all edge cases at the boundaries, not architectural problems.
+
+### The missing piece is still turn/movement
+Findings 2 and 9 from the prior review are the largest gameplay gap. Until that exists, "combat" is "pick two units anywhere on the map and click." Worth scoping a small turn manager (`turn_started(unit)`, `turn_ended(unit)`, `action_points_remaining`) before adding more combat features on top of the current freeform model.
+
+### No tests
+`CombatSystem` is pure logic that takes resources and returns a proposal — it's the easiest thing in the codebase to test, and the proposal/apply path is the most fragile. A few GdUnit4 tests on combat math would catch most of A/B/C/D as regressions.
+
+---
+
+## Summary
+
+| # | Finding | Priority | Effort |
+|---|---------|----------|--------|
+| A | `apply_proposal` no `is_instance_valid` | High | Small |
+| B | No allegiance check | High | Small |
+| C | `queue_free` dangling refs | High | Small |
+| D | `atk_range` not enforced pre-request | High | Small |
+| E | Greedy axis movement, no pathfinding | Medium | Medium |
+| F | Duplicated unit-tracking | Medium | Medium |
+| G | `remove_unit` leaks signal connections | Medium | Small |
+| H | Dead `process_combat` | Medium | Trivial |
+| I | O(n) unit lookup | Medium | Small |
+| J | Sprite-per-tile renderers | Medium | Medium |
+| K | `MapLayout.initialize` not guarded | Medium | Small |
+| L | Highlight updates while modal open | Medium | Trivial |
+| M | Inline scripts in `unit.tscn` | Medium | Small |
+| N | `_draw_outer_corners` stub | Low | Medium |
+| O | Redundant `match` in wall renderer | Low | Trivial |
+| P | Magic `FOG_RECT` constant | Low | Trivial |
+| Q | Jolt 3D in 2D project | Low | Trivial |
+| R | `UnitInfo.name` shadows | Low | Trivial |
+| S | Dead fallback paths in `CombatMap` | Low | Trivial |

docs/todo.txt

@@ -1,6 +1,17 @@
 * Reogranize files
 * Singletons named 'XXXServer'
 * Dialogue scene command system (ShowText, ShowSprite, MoveSprite, PlaySound, ChangeBackground, etc)
+  * Need to figure out more complicated systems like choices, script eval, conditionals, jumping, etc. Probably end up wanting a DialogueEditor or just making them gdscripts and being done with it
 * Finish Dialogue Scene (fast forward, auto, history functionality, etc)
-* Setup room system (everything is unpassable, carve out rooms, walls automatic, specify connections between rooms on tiles)
-* Basic map editor (test map data will be harder to craft the more we add)
+* Units have an AppearanceInfo component that has a dictionary of str, UnitAppearance. UnitAppearance stores idle sprite, walking sprite, back, front, portrait, etc. AppearanceInfo specifies the current UnitAppearance and has methods for getting that data
+* Finish main menu
+  * Game Start screen
+  * Load Data Screen
+  * Eushully room (CG viewer, scene viewer (after dialogue system), unit data screen (needs a unit registry))
+  * Options
+* Battle View
+  * Fog of war
+  * Basic map editor (test map data will be harder to craft the more we add)
+  * Walking animations
+  * Start plugging in the Himegari UI
+* Dialog boxes