Code review
This commit is contained in:
gamer147
266
docs/review/2026-04-07-architecture-review.md
Normal file
266
docs/review/2026-04-07-architecture-review.md
Normal file
@@ -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 |
|
||||
Reference in New Issue
Block a user