General code cleanup (#125) #127

Merged

liamjd merged 2 commits from 125-general-code-cleanup into main

2026-09-22 21:19:51 +01:00

liamjd commented

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

Owner

Closes #125

A general cleanup following the review after #72: dead code removed, small duplications tidied, one hot-path lookup made constant-time, and eight throwaway tools deleted. No behaviour changes. The bugs found in the same review are #124, and are deliberately left alone here, including the Pawns.tasks setter that never disconnects the old manager.

Removed (nothing called these)

In GameState: set_debug_mode(), which would also have emitted debug_mode_changed twice; contract_start_date, which was assigned but never read; and run_day().
BuildingPanel's own building_type_selected signal. The panel emits GameState's signal of the same name.
Building.to_debug_string(), and the UTF-8 BOM at the top of building.gd.
BuildingFootprint._print_cells().
Task.required_role, remove_subtask() and dump(). #74 will bring roles back properly, not as a string.
In TerrainGrid: the walkable count and the print that was commented out, so _build_navigation() now returns nothing; and the unused threshold local in is_sloped().

Tidied

Pawn.NO_DESTINATION replaces six copies of Vector3(-1, -1, -1).
Pawns: task_completed, task_cancelled and task_released now connect directly to a single _release_pawn_from_task(task), replacing three identical handlers. Also removed a doc line that appeared twice, a stale TODO(#75), and a redundant ERR_SKIP branch.
PlacementController: the five invalid placement reasons share one match arm.
Buildings: dropped a line that was commented out, and the != null checks in front of is, which is already false on null.
DebugLayer: the text for each pawn and task label is built by one helper each. The task list now looks up each label once instead of twice.

Optimised

TerrainGrid.is_in_main_region() did a linear Array.has() over the landmass, up to about 4,000 cells. Task.can_start() calls it for every task, for every idle pawn, on every tick. It now reads _main_region_set, a Dictionary. _set_main_region() is the only function that writes either the set or the array, so the two can't drift apart. The array stays, because random_walkable_position() indexes into it.
Pawns._check_path_collision() now works out the building's cells once, not once per waypoint.

Tools

Deleted eight of the scripts marked "THROWAWAY … Delete after use", all for closed issues:

probe_menu_rects, probe_screen, probe_menu_clickthrough, probe_demolish_click, screenshot_buildings, screenshot_building_blocks_path, screenshot_path_overlay, screenshot_window_resize.

CLAUDE.md referred readers to screenshot_path_overlay.gd for why -s scripts must not name project types. That pointer, and the ones in the two tools that stay, now name screenshot_building_states.gd, which already carries the full explanation.

Decisions

is_sloped() and neighbouring_cells() are kept, even though only tests call them. They have 7 tests of their own, and test_placement_rules.gd refers to is_sloped() by name to explain why placement uses a different rule. Deleting them would mean rewriting that reasoning for very little gain.
tools/screenshot_entrance_arrow.gd is kept so the arrow fix in #124 can be checked headed. Delete it with that fix.

Verification

Suite: 663 tests, 662 passing, 1 pending (#85's placeholder). Identical to main.
A headless run of the game logs no errors or warnings.
Played manually by Liam: all good.

🤖 Generated with Claude Code

Closes #125 A general cleanup following the review after #72: dead code removed, small duplications tidied, one hot-path lookup made constant-time, and eight throwaway tools deleted. **No behaviour changes.** The bugs found in the same review are #124, and are deliberately left alone here, including the `Pawns.tasks` setter that never disconnects the old manager. ## Removed (nothing called these) - In `GameState`: `set_debug_mode()`, which would also have emitted `debug_mode_changed` twice; `contract_start_date`, which was assigned but never read; and `run_day()`. - `BuildingPanel`'s own `building_type_selected` signal. The panel emits GameState's signal of the same name. - `Building.to_debug_string()`, and the UTF-8 BOM at the top of `building.gd`. - `BuildingFootprint._print_cells()`. - `Task.required_role`, `remove_subtask()` and `dump()`. #74 will bring roles back properly, not as a string. - In `TerrainGrid`: the `walkable` count and the print that was commented out, so `_build_navigation()` now returns nothing; and the unused `threshold` local in `is_sloped()`. ## Tidied - `Pawn.NO_DESTINATION` replaces six copies of `Vector3(-1, -1, -1)`. - `Pawns`: `task_completed`, `task_cancelled` and `task_released` now connect directly to a single `_release_pawn_from_task(task)`, replacing three identical handlers. Also removed a doc line that appeared twice, a stale `TODO(#75)`, and a redundant `ERR_SKIP` branch. - `PlacementController`: the five invalid placement reasons share one `match` arm. - `Buildings`: dropped a line that was commented out, and the `!= null` checks in front of `is`, which is already false on null. - `DebugLayer`: the text for each pawn and task label is built by one helper each. The task list now looks up each label once instead of twice. ## Optimised - **`TerrainGrid.is_in_main_region()`** did a linear `Array.has()` over the landmass, up to about 4,000 cells. `Task.can_start()` calls it for every task, for every idle pawn, on every tick. It now reads `_main_region_set`, a `Dictionary`. `_set_main_region()` is the only function that writes either the set or the array, so the two can't drift apart. The array stays, because `random_walkable_position()` indexes into it. - **`Pawns._check_path_collision()`** now works out the building's cells once, not once per waypoint. ## Tools Deleted eight of the scripts marked "THROWAWAY … Delete after use", all for closed issues: `probe_menu_rects`, `probe_screen`, `probe_menu_clickthrough`, `probe_demolish_click`, `screenshot_buildings`, `screenshot_building_blocks_path`, `screenshot_path_overlay`, `screenshot_window_resize`. CLAUDE.md referred readers to `screenshot_path_overlay.gd` for why `-s` scripts must not name project types. That pointer, and the ones in the two tools that stay, now name `screenshot_building_states.gd`, which already carries the full explanation. ## Decisions - **`is_sloped()` and `neighbouring_cells()` are kept**, even though only tests call them. They have 7 tests of their own, and `test_placement_rules.gd` refers to `is_sloped()` by name to explain why placement uses a different rule. Deleting them would mean rewriting that reasoning for very little gain. - **`tools/screenshot_entrance_arrow.gd` is kept** so the arrow fix in #124 can be checked headed. Delete it with that fix. ## Verification - **Suite: 663 tests, 662 passing, 1 pending** (#85's placeholder). Identical to `main`. - A headless run of the game logs no errors or warnings. - Played manually by Liam: all good. 🤖 Generated with [Claude Code](https://claude.com/claude-code)

liamjd added 2 commits

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

Delete the throwaway tools whose issues are closed (#125 ) c3948d88f3

Eight scripts in tools/ were labelled "THROWAWAY ... Delete after use",
and every issue they were written for is closed. CLAUDE.md pointed at
screenshot_path_overlay.gd for why -s harnesses avoid project types, so
that pointer, and the ones in the two tools that stay, now name
screenshot_building_states.gd, which carries the same explanation.

screenshot_entrance_arrow.gd stays until the arrow fix in #124 has been
checked headed.

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

Remove dead code, tidy duplication, index the main region (#125 ) 286edb3576

Removed, as nothing called them: GameState.set_debug_mode(),
contract_start_date and run_day(); BuildingPanel's own
building_type_selected signal; Building.to_debug_string();
BuildingFootprint._print_cells(); Task.required_role, remove_subtask()
and dump(); TerrainGrid's walkable count and the unused locals in
is_sloped(). Also drops the BOM from building.gd.

Tidied: Pawn.NO_DESTINATION replaces six copies of the sentinel; the
three identical task handlers in Pawns are one; the invalid placement
reasons share a match arm; the debug layer builds each label's text in
one place.

is_in_main_region() is on the hottest path in the game - every task, for
every idle pawn, every tick - and was a linear Array.has() over the
landmass. It now reads a set that _set_main_region() keeps in step with
the array. _check_path_collision() computes the building's cells once
rather than per waypoint.

No behaviour changes; the suite is unchanged at 663 / 662 / 1 pending.

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

liamjd merged commit 368f75a2ea into main

2026-09-22 21:19:51 +01:00

liamjd referenced this pull request from a commit

2026-09-22 21:19:53 +01:00

Merge pull request 'General code cleanup (#125)' (#127) from 125-general-code-cleanup into main

liamjd referenced this pull request

2026-09-28 20:21:34 +01:00

RANDOM THOUGHTS #90