237 lines
11 KiB
Markdown
237 lines
11 KiB
Markdown
# 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.
|
|
|
|
```gdscript
|
|
# 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:
|
|
1. Iterate all units in `_ready()`
|
|
2. Connect to `node_added` to catch late arrivals
|
|
3. 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 |
|