diff --git a/docs/review/2026-04-07-architecture-review.md b/docs/review/2026-04-07-architecture-review.md new file mode 100644 index 0000000..3d061f1 --- /dev/null +++ b/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 |