Fix bugs from the post-#72 review: arrow overdraw, duplicate signals, Pawns.tasks setter #124

New issue

Closed

opened 2026-09-22 21:05:41 +01:00 by liamjd · 0 comments

liamjd commented

2026-09-22 21:05:41 +01:00

Owner

These four bugs turned up in a whole-project review after #72 merged (2ad4d63). None of them breaks gameplay today. Each one is either a visible defect or a signal that doesn't mean what it says.

1. The entrance arrow is drawn twice (and is hard to see on sandy terrain)

PlacementController._build_entrance_arrow_mesh() (entities/buildings/placement_controller.gd:282-289) adds a second surface after the chevron. It is a plain triangle with its ratios hard-coded (0.35, 0.30, 0.20), not read from the arrow_*_ratio exports.

In addition, the default colour of the arrow closely matches the sandy terrain colour and can be hard to see. Choose a new colour which is distinct from all terrain colours.

With the default ratios, that triangle is exactly the chevron's outline without the notch:

it paints over the notch that the doc comment says makes the arrow read as a chevron;
where it overlaps the chevron, the 0.85 alpha is blended twice.

The ghost's arrow therefore shows as a triangle with a darker chevron inside it. The second surface came in with d85335f, and looks like scaffolding left over from building the chevron.

Fix: delete the second surface_begin … surface_end block.

Check: look at it headed. tools/screenshot_entrance_arrow.gd (#61) is the existing harness for this.

2. `TaskManager.add_subtask()` emits `task_added` twice

add_subtask() calls add(), which emits task_added (task_manager.gd:26). It then emits it again at line 150. test_add_subtask_emits_task_added_for_the_child uses assert_signal_emitted_with_parameters, which only checks that the signal fired at least once, so the duplicate gets through.

Fix: delete line 150.

Test: assert_signal_emit_count(manager, "task_added", 1) after a single add_subtask().

3. `next_for()` emits `task_unblocked` for tasks that were never blocked

task_manager.gd:122-124: every task that can start takes the else branch. That branch calls transition_to(PENDING), which is a no-op for a task already PENDING, and emits task_unblocked anyway. It does this once per idle pawn per tick, for every startable task. Buildings._task_unblocked has a guard, so nothing looks wrong, but any other listener would be told that tasks were unblocked when they never had been.

Fix: only transition and emit when task.status == Task.Status.BLOCKED.

Test: next_for() on a queue holding one ordinary PENDING task does not emit task_unblocked. A task that was blocked and can now start still does.

4. The `Pawns.tasks` setter never disconnects the previous manager

Buildings.tasks disconnects the old manager before connecting the new one (buildings.gd:43-45). Pawns.tasks (entities/pawns/pawns.gd:5-13) only connects. Reassigning it would leave the old manager's task_completed/task_cancelled/task_released still calling into the registry.

Fix: copy the Buildings pattern.

Test: assign manager A, then manager B. Cancelling a task on A must not clear the task from a pawn.

Minor: the debug panel's pawn list goes stale

DebugLayer.populate_pawns_list() only runs when debug mode is toggled (ui/debug_layer.gd:43), and _update_pawns_list() only updates labels that already exist. After an R-reroll, the old pawns' labels are removed and the new pawns never get one. Each of those removals also logs a push_error, because it detects removal by calling Pawns.get_pawn(), and that function errors on an unknown id.

Fix: repopulate on map.regenerated (or on pawn_spawned/pawn_removed), and check pawns.all() rather than calling get_pawn().

Acceptance criteria

Checked headed: the entrance arrow has a visible notch and one uniform alpha
task_added fires exactly once per add_subtask()
task_unblocked fires only on a BLOCKED → PENDING transition
Reassigning Pawns.tasks disconnects the previous manager
The debug pawn list is correct after a reroll, with no push_error
A test for each of 2-4 that fails before its fix

These four bugs turned up in a whole-project review after #72 merged (`2ad4d63`). None of them breaks gameplay today. Each one is either a visible defect or a signal that doesn't mean what it says. ## 1. The entrance arrow is drawn twice (and is hard to see on sandy terrain) `PlacementController._build_entrance_arrow_mesh()` (`entities/buildings/placement_controller.gd:282-289`) adds a second surface after the chevron. It is a plain triangle with its ratios hard-coded (`0.35`, `0.30`, `0.20`), not read from the `arrow_*_ratio` exports. In addition, the default colour of the arrow closely matches the sandy terrain colour and can be hard to see. Choose a new colour which is distinct from all terrain colours. With the default ratios, that triangle is exactly the chevron's outline without the notch: - it paints over the notch that the doc comment says makes the arrow read as a chevron; - where it overlaps the chevron, the 0.85 alpha is blended twice. The ghost's arrow therefore shows as a triangle with a darker chevron inside it. The second surface came in with `d85335f`, and looks like scaffolding left over from building the chevron. **Fix:** delete the second `surface_begin` … `surface_end` block. **Check:** look at it headed. `tools/screenshot_entrance_arrow.gd` (#61) is the existing harness for this. ## 2. `TaskManager.add_subtask()` emits `task_added` twice `add_subtask()` calls `add()`, which emits `task_added` (`task_manager.gd:26`). It then emits it again at line 150. `test_add_subtask_emits_task_added_for_the_child` uses `assert_signal_emitted_with_parameters`, which only checks that the signal fired at least once, so the duplicate gets through. **Fix:** delete line 150. **Test:** `assert_signal_emit_count(manager, "task_added", 1)` after a single `add_subtask()`. ## 3. `next_for()` emits `task_unblocked` for tasks that were never blocked `task_manager.gd:122-124`: every task that can start takes the `else` branch. That branch calls `transition_to(PENDING)`, which is a no-op for a task already PENDING, and emits `task_unblocked` anyway. It does this once per idle pawn per tick, for every startable task. `Buildings._task_unblocked` has a guard, so nothing looks wrong, but any other listener would be told that tasks were unblocked when they never had been. **Fix:** only transition and emit when `task.status == Task.Status.BLOCKED`. **Test:** `next_for()` on a queue holding one ordinary PENDING task does not emit `task_unblocked`. A task that was blocked and can now start still does. ## 4. The `Pawns.tasks` setter never disconnects the previous manager `Buildings.tasks` disconnects the old manager before connecting the new one (`buildings.gd:43-45`). `Pawns.tasks` (`entities/pawns/pawns.gd:5-13`) only connects. Reassigning it would leave the old manager's `task_completed`/`task_cancelled`/`task_released` still calling into the registry. **Fix:** copy the `Buildings` pattern. **Test:** assign manager A, then manager B. Cancelling a task on A must not clear the task from a pawn. ## Minor: the debug panel's pawn list goes stale `DebugLayer.populate_pawns_list()` only runs when debug mode is toggled (`ui/debug_layer.gd:43`), and `_update_pawns_list()` only updates labels that already exist. After an R-reroll, the old pawns' labels are removed and the new pawns never get one. Each of those removals also logs a `push_error`, because it detects removal by calling `Pawns.get_pawn()`, and that function errors on an unknown id. **Fix:** repopulate on `map.regenerated` (or on `pawn_spawned`/`pawn_removed`), and check `pawns.all()` rather than calling `get_pawn()`. ## Acceptance criteria - [ ] Checked headed: the entrance arrow has a visible notch and one uniform alpha - [ ] `task_added` fires exactly once per `add_subtask()` - [ ] `task_unblocked` fires only on a BLOCKED → PENDING transition - [ ] Reassigning `Pawns.tasks` disconnects the previous manager - [ ] The debug pawn list is correct after a reroll, with no `push_error` - [ ] A test for each of 2-4 that fails before its fix

liamjd added the

bug

label

2026-09-22 21:05:41 +01:00

liamjd referenced this issue

2026-09-22 21:05:54 +01:00

Remove dead code, tidy duplication, and index the main region #125

liamjd referenced this issue

2026-09-22 21:05:54 +01:00

Extract a BuildingTask base from ConstructionTask and DeconstructionTask #126

liamjd referenced this issue from a commit

2026-09-22 21:18:34 +01:00