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

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

Dead code, small duplications and one cheap optimisation, found in a whole-project review after #72 merged (2ad4d63). None of it changes behaviour. The bugs from the same review are in #124, and the BuildingTask refactor is #126.

Remove: nothing calls these

GameState.set_debug_mode() (globals/GameState.gd:75). It would also emit debug_mode_changed twice, because the setter already emits it.
GameState.contract_start_date: assigned in _ready(), never read.
GameState.run_day().
BuildingPanel's own signal building_type_selected (ui/building_panel/building_panel.gd:20). The panel emits GameState's signal of the same name instead.
Building.to_debug_string().
BuildingFootprint._print_cells().
Task.required_role, Task.remove_subtask(), Task.dump(). #74 will bring roles back properly, not as a string.
TerrainGrid.generate(): the unused walkable local (line 130) and the print below it, which is commented out. _build_navigation() can then stop returning a count.
TerrainGrid.is_sloped(): the unused threshold local (line 383).

Decide: only the tests call these

TerrainGrid.is_sloped() and neighbouring_cells(). Placement uses PlacementRules' level-spread rule instead. Either delete them along with their tests, or give them a real caller.

Tidy

entities/pawns/pawns.gd:168-171: one doc line appears twice, plus a stale TODO(#75): the body. The body exists now.
pawns.gd:184-186: if … == ERR_SKIP: continue followed by an unconditional continue. Only the second is needed.
pawns.gd:255-267: three identical signal handlers. Connect task_completed, task_cancelled and task_released directly to one handler that takes a Task.
Vector3(-1.0, -1.0, -1.0) is written out six times across pawn.gd and pawns.gd as "no destination". It should be one constant on Pawn.
placement_controller.gd:135-149: four match arms all assign INVALID_TINT. They can be one arm.
ui/debug_layer.gd: the text for each pawn label (lines 78, 88) and each task label (100, 105) is built in two places. One helper each.
buildings.gd:275: a line that's commented out. buildings.gd:267: existing_task != null and existing_task is …. is is already false on null.
entities/buildings/building.gd begins with a UTF-8 BOM.

Throwaway tools

Nine scripts in tools/ are labelled "THROWAWAY … Delete after use", and every issue they were written for is closed:

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

Two things to deal with first:

CLAUDE.md refers readers to screenshot_path_overlay.gd for why -s harnesses avoid project types. Move that explanation into CLAUDE.md before deleting it, or keep that file as the reference example.
Consider keeping screenshot_entrance_arrow.gd until the arrow fix in #124 has been checked headed.

Optimise: `is_in_main_region()`

TerrainGrid.is_in_main_region() (terrain_grid.gd:624) calls Array.has() on _main_region. On a 64×64 map that's a linear scan of up to about 4,000 cells. It sits on the hottest path in the game: Task.can_start() calls it for every task, for every idle pawn, on every tick.

Fix: keep a Dictionary[Vector2i, bool] alongside _main_region, rebuilt in _find_largest_region(). random_walkable_position() still needs the array for indexing.

While in pawns.gd: _check_path_collision() (line 219) rebuilds building.cells() for every waypoint. Compute it once before the loop.

Deliberately left alone

PawnRole, PawnLevel and the commented-out pawn fields: placeholders for #74.
The unused GameCalendar functions: a tested API, planned for forecasting.
count_of() copying values().
Construction allocating a new material each tick of progress.
PlacementRules looping over the cells four times.
The right-click cancel and screen-ray code shared by PlacementController and DemolishController: that belongs to #78's shared picker.
Water draw-level logic that is repeated in terrain_grid.gd: that belongs to #46.

Acceptance criteria

Everything under "Remove" is gone, and the suite is green
Decision on is_sloped()/neighbouring_cells() made and recorded in the PR
Items under "Tidy" are done
Throwaway tools deleted, with the -s explanation preserved
is_in_main_region() is a constant-time lookup; existing tests for main region and spawning still pass

Dead code, small duplications and one cheap optimisation, found in a whole-project review after #72 merged (`2ad4d63`). None of it changes behaviour. The bugs from the same review are in #124, and the `BuildingTask` refactor is #126. ## Remove: nothing calls these - `GameState.set_debug_mode()` (`globals/GameState.gd:75`). It would also emit `debug_mode_changed` twice, because the setter already emits it. - `GameState.contract_start_date`: assigned in `_ready()`, never read. - `GameState.run_day()`. - `BuildingPanel`'s own `signal building_type_selected` (`ui/building_panel/building_panel.gd:20`). The panel emits GameState's signal of the same name instead. - `Building.to_debug_string()`. - `BuildingFootprint._print_cells()`. - `Task.required_role`, `Task.remove_subtask()`, `Task.dump()`. #74 will bring roles back properly, not as a string. - `TerrainGrid.generate()`: the unused `walkable` local (line 130) and the `print` below it, which is commented out. `_build_navigation()` can then stop returning a count. - `TerrainGrid.is_sloped()`: the unused `threshold` local (line 383). ## Decide: only the tests call these - `TerrainGrid.is_sloped()` and `neighbouring_cells()`. Placement uses `PlacementRules`' level-spread rule instead. Either delete them along with their tests, or give them a real caller. ## Tidy - `entities/pawns/pawns.gd:168-171`: one doc line appears twice, plus a stale `TODO(#75): the body`. The body exists now. - `pawns.gd:184-186`: `if … == ERR_SKIP: continue` followed by an unconditional `continue`. Only the second is needed. - `pawns.gd:255-267`: three identical signal handlers. Connect `task_completed`, `task_cancelled` and `task_released` directly to one handler that takes a `Task`. - `Vector3(-1.0, -1.0, -1.0)` is written out six times across `pawn.gd` and `pawns.gd` as "no destination". It should be one constant on `Pawn`. - `placement_controller.gd:135-149`: four `match` arms all assign `INVALID_TINT`. They can be one arm. - `ui/debug_layer.gd`: the text for each pawn label (lines 78, 88) and each task label (100, 105) is built in two places. One helper each. - `buildings.gd:275`: a line that's commented out. `buildings.gd:267`: `existing_task != null and existing_task is …`. `is` is already false on null. - `entities/buildings/building.gd` begins with a UTF-8 BOM. ## Throwaway tools Nine scripts in `tools/` are labelled "THROWAWAY … Delete after use", and every issue they were written for is closed: `probe_menu_rects`, `probe_screen`, `probe_menu_clickthrough`, `probe_demolish_click`, `screenshot_buildings`, `screenshot_entrance_arrow`, `screenshot_building_blocks_path`, `screenshot_path_overlay`, `screenshot_window_resize`. Two things to deal with first: - CLAUDE.md refers readers to `screenshot_path_overlay.gd` for why `-s` harnesses avoid project types. Move that explanation into CLAUDE.md before deleting it, or keep that file as the reference example. - Consider keeping `screenshot_entrance_arrow.gd` until the arrow fix in #124 has been checked headed. ## Optimise: `is_in_main_region()` `TerrainGrid.is_in_main_region()` (`terrain_grid.gd:624`) calls `Array.has()` on `_main_region`. On a 64×64 map that's a linear scan of up to about 4,000 cells. It sits on the hottest path in the game: `Task.can_start()` calls it for every task, for every idle pawn, on every tick. **Fix:** keep a `Dictionary[Vector2i, bool]` alongside `_main_region`, rebuilt in `_find_largest_region()`. `random_walkable_position()` still needs the array for indexing. While in `pawns.gd`: `_check_path_collision()` (line 219) rebuilds `building.cells()` for every waypoint. Compute it once before the loop. ## Deliberately left alone - `PawnRole`, `PawnLevel` and the commented-out pawn fields: placeholders for #74. - The unused `GameCalendar` functions: a tested API, planned for forecasting. - `count_of()` copying `values()`. - Construction allocating a new material each tick of progress. - `PlacementRules` looping over the cells four times. - The right-click cancel and screen-ray code shared by `PlacementController` and `DemolishController`: that belongs to #78's shared picker. - Water draw-level logic that is repeated in `terrain_grid.gd`: that belongs to #46. ## Acceptance criteria - [ ] Everything under "Remove" is gone, and the suite is green - [ ] Decision on `is_sloped()`/`neighbouring_cells()` made and recorded in the PR - [ ] Items under "Tidy" are done - [ ] Throwaway tools deleted, with the `-s` explanation preserved - [ ] `is_in_main_region()` is a constant-time lookup; existing tests for main region and spawning still pass

liamjd added the

enhancement

label

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

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

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

liamjd referenced this issue from a commit

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