11 KiB
Architecture & Code Review — Dungeon Lords
Reviewed: 2026-04-02
Architecture Overview
The project is a Godot 4.6 grid-based tactics game using a signal-driven architecture with five major systems:
StrategyPhase (orchestrator)
├── CombatMap — grid, tiles, unit placement
├── PlayerController — input, selection, movement
├── CombatSystem — damage calculation
├── CombatUI — HUD, combat proposal panel
└── Camera2D — pan/zoom
Signal flow is clean: PlayerController -> StrategyPhase -> CombatUI -> CombatSystem. StrategyPhase acts as a mediator, which keeps systems decoupled. This is a solid foundation.
High Priority
1. No allegiance check on combat targeting
File: nodes/player_controller.gd:39
Clicking any other unit while one is selected triggers combat_requested, regardless of allegiance. A player unit can attack a friendly unit.
# Current — any two different alive units can fight
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)
Suggestion: Check clicked_unit.current_allegiance.type != _selected_unit.current_allegiance.type before emitting. Alternatively, handle this in StrategyPhase so the controller stays allegiance-agnostic and the orchestrator decides what's valid.
2. Movement has no pathfinding — can navigate around walls
File: nodes/player_controller.gd:68-83
The step-by-step mover picks one axis per frame. It only checks the immediate next tile, so the unit happily walks around an L-shaped wall rather than being blocked by it. It also means you can send a unit to the far corner of the map with a single click and it'll walk there uninterrupted, which may not be intended for a tactics game (most use action points / movement range).
Suggestion: Implement A* or BFS pathfinding on the grid. Even before that, consider adding a movement range limit — a unit probably shouldn't be able to walk 50 tiles in one go.
3. Combat proposal can go stale
Files: nodes/strategy_phase.gd:14, nodes/combat_system.gd:38
create_proposal snapshots stats at proposal time, but apply_proposal still reads unit.is_alive() and calls unit.take_damage() on the live unit objects. If a unit dies between proposal creation and fight confirmation (e.g. from a future AoE or status effect), the proposal holds a freed reference.
Currently harmless because nothing else can damage units while the proposal panel is open, but it will break as soon as you add any concurrent damage source (poison ticks, traps, etc.).
Suggestion: Validate unit references in apply_proposal with is_instance_valid() before using them. The process_combat function already does this check at the top — apply_proposal should too.
4. queue_free() in _die() creates dangling references
File: nodes/unit.gd:43
unit_died is emitted, then queue_free() is called. Listeners get the signal and store/use the unit reference, but the node will be freed at the end of the frame. CombatUI._on_unit_died compares _current_proposal.attacker.unit == unit — this works this frame, but CombatProposal continues holding the reference.
Suggestion: Either:
- Don't
queue_free()immediately; hide the unit and mark it dead, then free it explicitly when safe (e.g. after combat resolution). - Clear unit references in CombatProposal when a unit dies.
Medium Priority
5. Duplicated unit-tracking boilerplate
Files: nodes/player_controller.gd:16-24, scripts/combat_ui.gd:35-45
Both PlayerController and CombatUI independently:
- Iterate all units in
_ready() - Connect to
node_addedto catch late arrivals - Guard against duplicate connections
This pattern is repeated verbatim. If you add more systems that care about units, you'll copy it again.
Suggestion: Create a simple UnitRegistry (autoload or node) that emits unit_registered(unit) / unit_deregistered(unit). Systems subscribe to the registry instead of polling the scene tree.
6. Inline scripts in unit.tscn
File: prefabs/unit.tscn (SubResource GDScript_fhs1y, GDScript_on614)
The AllegianceIndicator and selection highlight have their scripts embedded as SubResource in the scene file. This makes them invisible to search/grep, hard to test, and easy to miss during refactors.
Suggestion: Extract to scripts/allegiance_indicator.gd and scripts/unit_selector_handler.gd. The scripts are small, but being discoverable matters more than being small.
7. CombatSystem has two code paths for combat
File: nodes/combat_system.gd
process_combat() (line 23) creates a proposal and immediately applies it with debug prints. apply_proposal() (line 38) does the actual dice rolls. The UI-driven flow uses create_proposal + apply_proposal, while process_combat is a direct shortcut that bypasses the UI.
Having two entry points that partially overlap invites bugs — a change to one may not be reflected in the other.
Suggestion: If process_combat is just for testing, move it to a test script or remove it. If it's for AI-controlled combat (enemies auto-fighting), it should still go through the same create_proposal -> apply_proposal pipeline and just skip the UI confirmation.
8. _get_unit_at() is O(n) per click
File: nodes/player_controller.gd:96-104
Iterates all units in the "units" group and snaps each position to the grid every time the player clicks. Fine for 10 units, sluggish for 100.
Suggestion: Maintain a Dictionary[Vector2i, Unit] in CombatMap that maps grid coords to occupying units. Update it on deploy/move/death. Lookup becomes O(1) and you also get collision detection for free (can't move into an occupied tile).
9. No movement range or turn system
Files: nodes/player_controller.gd, nodes/unit.gd
UnitStats has spd but nothing uses it. There's no concept of turns or movement range. The player can move any unit any distance at any time. This is the biggest gameplay gap — without it, combat is just "click two units together."
Suggestion: This is likely on your roadmap already, but architecturally: spd should translate to a movement budget per turn. The movement system should enforce it. Consider a TurnManager that tracks whose turn it is and how many action points remain.
10. atk_range is defined but never enforced
File: resources/resource_definitions/unit_stats.gd:11
atk_range defaults to 1 but nothing checks distance before allowing combat. A unit could attack from across the map.
Suggestion: Before emitting combat_requested, check that the Chebyshev or Manhattan distance between attacker and defender is within atk_range.
Low Priority
11. tile_highlight processes every frame when hidden
File: scripts/tile_highlight.gd:17
_process() runs unconditionally. CombatMap.set_highlight_enabled correctly calls set_process(false), but if the highlight is hidden via _notification(WM_MOUSE_EXIT) at line 30, processing continues.
Suggestion: Add set_process(false) in the hide() path and set_process(true) in show().
12. GridOverlay is wired but never used
Files: scripts/grid_overlay.gd, nodes/combat_map.gd:8
highlight_map is referenced and target_tile is wired via signal, but highlight_tile() and clear_tile() are never called. Dead code.
Suggestion: Either use it for movement range display (which would be valuable) or remove it to reduce confusion.
13. Jolt 3D Physics engine configured for a 2D game
File: project.godot:20
3d/physics_engine="Jolt Physics" is set but the game is entirely 2D and doesn't use physics. This is harmless but unnecessary.
Suggestion: Remove the line, or switch to Godot's default to avoid confusion.
14. current_hp default vs _init
File: resources/resource_definitions/unit_stats.gd:4,17
current_hp has no @export default value, but _init sets current_hp = max_hp. If a resource is created in the inspector and _init isn't called in the expected order, current_hp could be 0. The duplicate(true) in unit.gd:21 does trigger _init on the copy, but it's fragile.
Suggestion: Set @export var current_hp: int = 10 to match max_hp's default, or initialize it in Unit._ready() explicitly after duplication.
15. UnitInfo.name shadows Object.name
File: resources/resource_definitions/unit_info.gd:3
Resource inherits from Object, which already has a name property. Your @export var name shadows it. GDScript allows this but it can cause confusion — unit.current_info.name returns your custom name, but unit.name returns the node name.
Suggestion: Rename to display_name or unit_name to avoid ambiguity.
Architecture Suggestions
State Management
Right now game state is scattered across individual nodes. As the game grows (turns, phases, win conditions), consider a central GameState resource or node that tracks:
- Current phase (strategy, combat animation, enemy turn)
- Whose turn it is
- Remaining action points
- Win/loss conditions
Event Bus
The duplicated unit-tracking pattern (point #5) hints at a need for a global event bus or registry. An autoload EventBus with signals like unit_spawned, unit_died, turn_started would let systems subscribe without manually wiring to every unit.
Separation of Data and Presentation
Units mix data (current_stats, state) with presentation (Node2D position, queue_free). As complexity grows, separating the unit's logical state (grid position, alive/dead, stats) from its visual representation (sprite, animations, particles) will make things like replays, AI simulation, and networking much easier.
Testing
No automated tests exist. GdUnit4 or Gut can run unit tests for pure logic like CombatSystem calculations. Even a few tests on the combat math would catch regressions early.
Summary Table
| # | Issue | Priority | Effort |
|---|---|---|---|
| 1 | No allegiance check on combat | High | Small |
| 2 | No pathfinding | High | Medium |
| 3 | Stale combat proposal | High | Small |
| 4 | queue_free dangling refs |
High | Small |
| 5 | Duplicated unit-tracking | Medium | Medium |
| 6 | Inline scripts in tscn | Medium | Small |
| 7 | Two combat code paths | Medium | Small |
| 8 | O(n) unit lookup | Medium | Small |
| 9 | No turn/movement system | Medium | Large |
| 10 | atk_range not enforced | Medium | Small |
| 11 | tile_highlight always processing | Low | Trivial |
| 12 | Unused GridOverlay | Low | Trivial |
| 13 | Jolt 3D configured | Low | Trivial |
| 14 | current_hp default fragile | Low | Trivial |
| 15 | name shadows Object.name | Low | Trivial |