15 KiB
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 — Resolved 2026-04-07
apply_proposal now early-returns if either unit reference is invalid, and re-checks both before the counterattack branch.
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
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 — Resolved 2026-04-07
process_combat() deleted from combat_system.gd. Only create_proposal → apply_proposal remains.
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 — Resolved 2026-04-07
Both renderers now override _draw() and use draw_texture_rect_region() directly. Zero child sprites; layout changes call queue_redraw() instead of thrashing the scene tree. Findings O (redundant match arms) and P (undocumented FOG_RECT) were folded into the same change.
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_frees 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 — Not a bug (2026-04-07)
On re-reading player_controller.gd:44-46, _process already early-returns when input_disabled is true. Original finding was based on a misread; no code change needed.
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 — Resolved 2026-04-07
Collapsed to a single for edge in edges: _build_edge_segments(...) loop as part of finding J's refactor.
File: nodes/wall_renderer.gd:103-112
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 — Resolved 2026-04-07
Fallback branches removed from is_tile_passable and is_tile_valid; both now assert(map_layout != null). The now-dead is_wall() helper was deleted.
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 |
Done | |
| 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 |
Done | |
| I | O(n) unit lookup | Medium | Small |
| J | Sprite-per-tile renderers | Done | |
| K | MapLayout.initialize not guarded |
Medium | Small |
| L | Highlight updates while modal open | Not a bug | |
| M | Inline scripts in unit.tscn |
Medium | Small |
| N | _draw_outer_corners stub |
Low | Medium |
| O | Redundant match in wall renderer |
Done | |
| 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 |
Done |