Extract a BuildingTask base from ConstructionTask and DeconstructionTask #126

New issue

Open

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

liamjd commented

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

Owner

ConstructionTask and DeconstructionTask are about 70% the same code. Pull the shared part into an abstract BuildingTask before #73 adds a third task type that targets a building.

The duplication

In entities/tasks/construction_task.gd and entities/tasks/deconstruction_task.gd:

Both classes have a building_id: int = -1.
_can_start, _work and on_complete each open with the same ctx == null or ctx.buildings == null guard, then fetch the building and check it isn't null. That is six copies of the same lookup between the two classes.
_work computes progress the same way in both: 1.0 if total <= 0.0 else 1.0 - work_remaining / total, then set_build_progress(). Only the source of total differs (build_work_ticks() against deconstruct_work_ticks()).

Knock-on duplication in entities/buildings/buildings.gd:

place() (lines 91-97) and mark_for_demolition() (lines 276-283) set up a task the same way: building_id, target_cell = building.entrance, work_remaining, label.
_task_for() (lines 449-461) needs a two-branch match var c when c is … only because the two classes have no common parent.

Proposed shape

@abstract
class_name BuildingTask
extends Task

var building_id: int = -1

## The building this task acts on, or null if the registry or the record is gone.
func _building(ctx: TaskContext) -> Building

## Total work for this task type, in ticks. The denominator for progress.
@abstract
func _total_work_ticks(building: Building) -> int

Once that exists:

_task_for() becomes a single if task is BuildingTask and task.building_id == building_id.
The progress write moves into the base class, or into a helper that subclasses call.
The task setup in Buildings can become a factory or an _init(building). Whether that is worth doing is a call to make during implementation.

Decisions for Liam

Where progress is written. At the moment DeconstructionTask writes demolition progress into build_progress, which is a known gap from PR #122. The refactor is the natural place to decide between:
- a separate demolish_progress field;
- build_progress counting down;
- not recording demolition progress at all.
on_cancel. DeconstructionTask.on_cancel() is pass, so any cancel route other than unmark_for_demolition() leaves a building marked with no task behind it (another known gap from #122). Clearing marked_for_demolition there would make it hold whichever route cancels the task, #85 included.
Whether _task_blocked / _task_unblocked in Buildings should widen to BuildingTask. Today only construction can be blocked in a way the view shows.

Acceptance criteria

BuildingTask exists, and both task types extend it
Neither subclass repeats the ctx/building guard
Buildings._task_for() has no per-type branches
Decisions 1-3 made and recorded in the PR
Existing test_construction_task.gd and test_deconstruction_task.gd pass unchanged, apart from whatever decisions 1 and 2 change
If decision 2 is taken: a test that cancelling a DeconstructionTask through TaskManager.cancel() clears the mark

Related: #72, #73, #85. The bugs and cleanup from the same review are #124 and #125.

`ConstructionTask` and `DeconstructionTask` are about 70% the same code. Pull the shared part into an abstract `BuildingTask` before #73 adds a third task type that targets a building. ## The duplication In `entities/tasks/construction_task.gd` and `entities/tasks/deconstruction_task.gd`: - Both classes have a `building_id: int = -1`. - `_can_start`, `_work` and `on_complete` each open with the same `ctx == null or ctx.buildings == null` guard, then fetch the building and check it isn't null. That is six copies of the same lookup between the two classes. - `_work` computes progress the same way in both: `1.0 if total <= 0.0 else 1.0 - work_remaining / total`, then `set_build_progress()`. Only the source of `total` differs (`build_work_ticks()` against `deconstruct_work_ticks()`). Knock-on duplication in `entities/buildings/buildings.gd`: - `place()` (lines 91-97) and `mark_for_demolition()` (lines 276-283) set up a task the same way: `building_id`, `target_cell = building.entrance`, `work_remaining`, `label`. - `_task_for()` (lines 449-461) needs a two-branch `match var c when c is …` only because the two classes have no common parent. ## Proposed shape ```gdscript @abstract class_name BuildingTask extends Task var building_id: int = -1 ## The building this task acts on, or null if the registry or the record is gone. func _building(ctx: TaskContext) -> Building ## Total work for this task type, in ticks. The denominator for progress. @abstract func _total_work_ticks(building: Building) -> int ``` Once that exists: - `_task_for()` becomes a single `if task is BuildingTask and task.building_id == building_id`. - The progress write moves into the base class, or into a helper that subclasses call. - The task setup in `Buildings` can become a factory or an `_init(building)`. Whether that is worth doing is a call to make during implementation. ## Decisions for Liam 1. **Where progress is written.** At the moment `DeconstructionTask` writes demolition progress into `build_progress`, which is a known gap from PR #122. The refactor is the natural place to decide between: - a separate `demolish_progress` field; - `build_progress` counting down; - not recording demolition progress at all. 2. **`on_cancel`.** `DeconstructionTask.on_cancel()` is `pass`, so any cancel route other than `unmark_for_demolition()` leaves a building marked with no task behind it (another known gap from #122). Clearing `marked_for_demolition` there would make it hold whichever route cancels the task, #85 included. 3. **Whether `_task_blocked` / `_task_unblocked` in `Buildings` should widen to `BuildingTask`.** Today only construction can be blocked in a way the view shows. ## Acceptance criteria - [ ] `BuildingTask` exists, and both task types extend it - [ ] Neither subclass repeats the `ctx`/building guard - [ ] `Buildings._task_for()` has no per-type branches - [ ] Decisions 1-3 made and recorded in the PR - [ ] Existing `test_construction_task.gd` and `test_deconstruction_task.gd` pass unchanged, apart from whatever decisions 1 and 2 change - [ ] If decision 2 is taken: a test that cancelling a `DeconstructionTask` through `TaskManager.cancel()` clears the mark Related: #72, #73, #85. The bugs and cleanup from the same review are #124 and #125.

liamjd added the

enhancement

label

2026-09-22 21:05:42 +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-28 06:06:09 +01:00

Multi-pawn work slots, opted into per site #117

liamjd referenced this issue

2026-09-30 21:50:56 +01:00