Fix post-#72 review bugs: arrow overdraw, spurious signals, Pawns.tasks setter (#124) #135

Merged

liamjd merged 2 commits from 124-post-72-review-bug-fixes into main

2026-09-28 05:47:59 +01:00

liamjd commented

2026-09-28 05:44:29 +01:00

Owner

Closes #124

Changes

Entrance arrow: deleted the leftover second triangle that covered the chevron's notch and doubled its alpha. The colour changes from pale yellow, which disappeared on sand, to a saturated orange (Color(1.0, 0.4, 0.0, 0.85)). Orange is distinct from every terrain kind and from the ghost's validity tints.
add_subtask() double emit: already fixed on main in d7df02a. This PR adds the missing assert_signal_emit_count(..., 1) test. The test could not fail before its fix, because the fix predates it.
Spurious task_unblocked: next_for() now emits only on a BLOCKED -> PENDING transition. Beyond the ticket: complete() also moves a blocked parent to PENDING, and it did so silently. The spurious emit from next_for() had been covering for that, so without it a building marked unreachable could stay marked. complete() now emits task_unblocked and clears block_reason itself.
Pawns.tasks setter: disconnects the previous manager before connecting the new one, the same pattern as Buildings.tasks.
Debug pawn list (minor): syncs its labels with pawns.all() on pawn_spawned/pawn_removed and on each tick. It no longer calls get_pawn(), so there is no push_error. It also fixes a separate bug: turning debug mode off and on reused label names before the old labels were freed, so Godot renamed the new labels and they stopped updating.

Verification

The tests were committed ahead of the fixes. Before the fixes, three of the new tests failed for the right reasons: task_unblocked fired for a task that was never blocked, task_unblocked fired twice, and the old manager released a pawn. With the fixes, the suite passes: 773 tests, 772 passing, and 1 Pending test that was already there (#85).
Headed harness (throwaway, not committed): the controller's own arrow mesh and material were placed on water, sand, grass, forest and rock tiles. The arrow has one surface, a visible notch and uniform alpha, and it reads clearly on each terrain. Snow did not appear on any preset for that seed, so it was not photographed.
Headed harness: with debug mode on, the pawn labels matched pawns.all() after two rerolls and a debug toggle, with no errors.
Reviewed and game-tested by Liam.

🤖 Generated with Claude Code

Closes #124 ## Changes 1. **Entrance arrow**: deleted the leftover second triangle that covered the chevron's notch and doubled its alpha. The colour changes from pale yellow, which disappeared on sand, to a saturated orange (`Color(1.0, 0.4, 0.0, 0.85)`). Orange is distinct from every terrain kind and from the ghost's validity tints. 2. **`add_subtask()` double emit**: already fixed on main in `d7df02a`. This PR adds the missing `assert_signal_emit_count(..., 1)` test. The test could not fail before its fix, because the fix predates it. 3. **Spurious `task_unblocked`**: `next_for()` now emits only on a BLOCKED -> PENDING transition. **Beyond the ticket:** `complete()` also moves a blocked parent to PENDING, and it did so silently. The spurious emit from `next_for()` had been covering for that, so without it a building marked unreachable could stay marked. `complete()` now emits `task_unblocked` and clears `block_reason` itself. 4. **`Pawns.tasks` setter**: disconnects the previous manager before connecting the new one, the same pattern as `Buildings.tasks`. 5. **Debug pawn list (minor)**: syncs its labels with `pawns.all()` on `pawn_spawned`/`pawn_removed` and on each tick. It no longer calls `get_pawn()`, so there is no `push_error`. It also fixes a separate bug: turning debug mode off and on reused label names before the old labels were freed, so Godot renamed the new labels and they stopped updating. ## Verification - The tests were committed ahead of the fixes. Before the fixes, three of the new tests failed for the right reasons: `task_unblocked` fired for a task that was never blocked, `task_unblocked` fired twice, and the old manager released a pawn. With the fixes, the suite passes: 773 tests, 772 passing, and 1 Pending test that was already there (#85). - Headed harness (throwaway, not committed): the controller's own arrow mesh and material were placed on water, sand, grass, forest and rock tiles. The arrow has one surface, a visible notch and uniform alpha, and it reads clearly on each terrain. Snow did not appear on any preset for that seed, so it was not photographed. - Headed harness: with debug mode on, the pawn labels matched `pawns.all()` after two rerolls and a debug toggle, with no errors. - Reviewed and game-tested by Liam. 🤖 Generated with [Claude Code](https://claude.com/claude-code)

liamjd added 2 commits

2026-09-28 05:44:29 +01:00

Spec the post-#72 review fixes ahead of implementation (#124 ) 94c8278e04

- task_added fires exactly once per add_subtask()
- task_unblocked fires only on a BLOCKED -> PENDING transition, including
  the one complete() makes when a parent's last child finishes
- reassigning Pawns.tasks stops the old manager releasing pawns

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

Fix arrow overdraw, spurious signals, Pawns.tasks setter and debug list (#124 ) b5f212a6f6

- Drop the entrance arrow's leftover second triangle, which hid the notch
  and doubled the alpha; recolour it orange so it reads on sand
- next_for() emits task_unblocked only for a task that was BLOCKED;
  complete() now announces the parent it unblocks, which the spurious
  emit used to cover for
- Pawns.tasks disconnects the previous manager, as Buildings.tasks does
- The debug pawn list syncs with pawns.all() on spawn/remove and each
  tick, so it survives a reroll without push_errors from get_pawn()

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

liamjd merged commit af510efb98 into main

2026-09-28 05:47:59 +01:00

liamjd referenced this pull request from a commit

2026-09-28 05:48:00 +01:00

Merge pull request 'Fix post-#72 review bugs: arrow overdraw, spurious signals, Pawns.tasks setter (#124)' (#135) from 124-post-72-review-bug-fixes into main