liamjd/UntitledColonyBuilder
1
0
Fork 0
Code Issues 36 Pull requests Projects 1 Releases Packages Wiki Activity Actions

Demolish mode and DeconstructionTask #72

New issue
Closed
opened 2026-09-07 20:37:22 +01:00 by liamjd · 2 comments
liamjd commented 2026-09-07 20:37:22 +01:00
Owner
Copy link

Part of milestone #5. Depends on #70.

Buildings.remove() has no caller. Give it one via a demolish mode, driven by a task so a pawn has to go and do it.

AC:

  • A demolish mode (building panel button or key) where clicking a placed building marks it for removal; visual feedback that it is marked
  • A BUILT building enqueues a DeconstructionTask at its entrance cell; completion calls Buildings.remove() and emits building_removed
  • A PLANNED building whose construction has not started cancels the construction task and is removed immediately; no deconstruction task is created
  • An UNDER_CONSTRUCTION building: decide whether it cancels immediately or queues deconstruction, and write the decision in the PR
  • Marking then unmarking (click again, or cancel) cancels the deconstruction task
  • Unit tests for the three status branches through the registry
Part of milestone #5. Depends on #70. `Buildings.remove()` has no caller. Give it one via a demolish mode, driven by a task so a pawn has to go and do it. AC: - A demolish mode (building panel button or key) where clicking a placed building marks it for removal; visual feedback that it is marked - A BUILT building enqueues a `DeconstructionTask` at its entrance cell; completion calls `Buildings.remove()` and emits `building_removed` - A PLANNED building whose construction has not started cancels the construction task and is removed immediately; no deconstruction task is created - An UNDER_CONSTRUCTION building: decide whether it cancels immediately or queues deconstruction, and write the decision in the PR - Marking then unmarking (click again, or cancel) cancels the deconstruction task - Unit tests for the three status branches through the registry
liamjd commented 2026-09-20 20:43:11 +01:00
Author
Owner
Copy link

Spec written — tests are on 72-demolish-mode-and-deconstruction-task

test/unit/test_deconstruction_task.gd (new) and a MARKED section in
test/unit/test_building_view.gd. Both currently fail to parse, on
DeconstructionTask and BuildingView.State.MARKED — the same shape #70 and #71
started in.

Decision 1 — UNDER_CONSTRUCTION cancels immediately

The AC left this open. Specced as the same branch as PLANNED: marking a
half-built site cancels its construction task, removes the record on the spot,
and the labour already spent on it is lost.

The argument is that there is nothing yet to recover. Materials are #73; until a
building consumes them a part-built site holds nothing but spent pawn-ticks, and
no deconstruction task can hand those back — it would just spend more labour
reclaiming nothing. It would also create a fourth building state (under
construction and marked) that ConstructionTask._can_start(), BuildingView
and the work loop would each have to learn.

Worth revisiting when #73 lands: delivered materials are worth reclaiming, and
at that point a started site belongs on the BUILT branch. The tests for it are
grouped under one heading so that change is one section, not a scatter.

Decision 2 — marking is a flag, not a Building.Status

Building.marked_for_demolition: bool, alongside construction_blocked. A marked
building is still BUILT: it stands, it blocks navigation, it counts against its
type's build_limit. A fifth Status would also be reachable from PLANNED,
which Decision 1 says cannot exist.

Like construction_blocked it redraws the view and emits no signal —
building_status_changed is about the record's own lifecycle, and there is
nothing here yet worth committing to an event shape for.

Decision 3 — demolition costs work, derived from build_days

BuildingType.deconstruct_work_ticks(), mirroring build_work_ticks(): a
ratio of the build effort with a floor of one tick. The floor is what stops a
build_days = 0.0 building (the Colony Centre) being demolishable the instant a
pawn arrives. The tests assert the relations — non-zero, and less than the
build effort — not the ratio, so it stays tunable.

Decision 4 — invariant: a building carries at most one live task

It is what lets the registry find a building's task by scanning for its id
without first deciding which kind it wanted, and it is why the PLANNED and
UNDER_CONSTRUCTION branches cancel before doing anything else. Pinned by
test_a_building_never_carries_two_tasks_at_once.

Registry API the tests are written against

Buildings.mark_for_demolition(id: int) -> void
Buildings.unmark_for_demolition(id: int) -> void
Buildings.set_marked_for_demolition(id: int, marked: bool) -> void  # flag + redraw, no task

mark_for_demolition branches on status: BUILT/RUINED flags the record and
enqueues a DeconstructionTask at building.entrance; PLANNED and
UNDER_CONSTRUCTION cancel the construction task, which already removes the record
through ConstructionTask.on_cancel().

unmark_for_demolition cancels the deconstruction task;
DeconstructionTask.on_cancel() clears the flag via set_marked_for_demolition,
which does not touch tasks — that is what stops the two recursing into each
other. Same trap test_cancelling_does_not_recurse_through_removal covers one
level along.

UI: the ButtonGroup already enforces the arbitration

Rather than a new controller fighting PlacementController for the click: add a
Demolish toggle to BuildingPanel's existing button_group. Selecting it
unpresses any building card (and vice versa) for free, and the card's
toggled(false) already emits building_type_selected(null), which cancels
placement. A GameState.demolish_mode bool with a signal, mirroring
debug_mode, carries it to whatever handles the click.

One trap: BuildingPanel._cancel_placement() uses set_pressed_no_signal(false),
which will unpress the demolish toggle without clearing GameState.demolish_mode.
Right-click already routes through PlacementController.cancel_placement(), so
that path has to clear the flag explicitly.

Also in scope, small

place() sets build_progress = 1.0 on the build_days == 0.0 branch. It
currently leaves it at 0.0, so the Colony Centre claims to be 0% built for its
whole life — nothing read the field until #72 came to render a BUILT building
from it. Covered by test_a_building_that_lands_built_reads_as_fully_built;
drop it if you would rather keep it out of this branch.

Revised AC

  • A demolish mode (toggle on the building panel) where clicking a placed
    building marks it for removal, with visual feedback that it is marked
  • BUILT: enqueues a DeconstructionTask at the entrance cell; completion
    calls Buildings.remove() and emits building_removed
  • PLANNED: cancels the construction task, removed immediately, no
    deconstruction task
  • UNDER_CONSTRUCTION: as PLANNED (Decision 1), recorded in the PR
  • Marking then unmarking cancels the deconstruction task and restores the
    building's normal appearance
  • A marked building still counts toward its type's build_limit
  • The registry still works with no TaskManager at all
  • Unit tests for the three status branches through the registry — written,
    currently red
  • Headed screenshot of a marked building, since the suite cannot see it
## Spec written — tests are on `72-demolish-mode-and-deconstruction-task` `test/unit/test_deconstruction_task.gd` (new) and a `MARKED` section in `test/unit/test_building_view.gd`. Both currently fail to *parse*, on `DeconstructionTask` and `BuildingView.State.MARKED` — the same shape #70 and #71 started in. ### Decision 1 — UNDER_CONSTRUCTION cancels immediately The AC left this open. Specced as the **same branch as PLANNED**: marking a half-built site cancels its construction task, removes the record on the spot, and the labour already spent on it is lost. The argument is that there is nothing yet to recover. Materials are #73; until a building consumes them a part-built site holds nothing but spent pawn-ticks, and no deconstruction task can hand those back — it would just spend *more* labour reclaiming nothing. It would also create a fourth building state (under construction *and* marked) that `ConstructionTask._can_start()`, `BuildingView` and the work loop would each have to learn. Worth revisiting when #73 lands: delivered materials *are* worth reclaiming, and at that point a started site belongs on the BUILT branch. The tests for it are grouped under one heading so that change is one section, not a scatter. ### Decision 2 — marking is a flag, not a `Building.Status` `Building.marked_for_demolition: bool`, alongside `construction_blocked`. A marked building is still BUILT: it stands, it blocks navigation, it counts against its type's `build_limit`. A fifth `Status` would also be reachable from PLANNED, which Decision 1 says cannot exist. Like `construction_blocked` it redraws the view and emits **no signal** — `building_status_changed` is about the record's own lifecycle, and there is nothing here yet worth committing to an event shape for. ### Decision 3 — demolition costs work, derived from `build_days` `BuildingType.deconstruct_work_ticks()`, mirroring `build_work_ticks()`: a ratio of the build effort with a floor of one tick. The floor is what stops a `build_days = 0.0` building (the Colony Centre) being demolishable the instant a pawn arrives. The tests assert the *relations* — non-zero, and less than the build effort — not the ratio, so it stays tunable. ### Decision 4 — invariant: a building carries at most one live task It is what lets the registry find a building's task by scanning for its id without first deciding which kind it wanted, and it is why the PLANNED and UNDER_CONSTRUCTION branches cancel before doing anything else. Pinned by `test_a_building_never_carries_two_tasks_at_once`. ### Registry API the tests are written against ```gdscript Buildings.mark_for_demolition(id: int) -> void Buildings.unmark_for_demolition(id: int) -> void Buildings.set_marked_for_demolition(id: int, marked: bool) -> void # flag + redraw, no task ``` `mark_for_demolition` branches on status: BUILT/RUINED flags the record and enqueues a `DeconstructionTask` at `building.entrance`; PLANNED and UNDER_CONSTRUCTION cancel the construction task, which already removes the record through `ConstructionTask.on_cancel()`. `unmark_for_demolition` cancels the deconstruction task; `DeconstructionTask.on_cancel()` clears the flag via `set_marked_for_demolition`, which does **not** touch tasks — that is what stops the two recursing into each other. Same trap `test_cancelling_does_not_recurse_through_removal` covers one level along. ### UI: the ButtonGroup already enforces the arbitration Rather than a new controller fighting `PlacementController` for the click: add a Demolish toggle to `BuildingPanel`'s existing `button_group`. Selecting it unpresses any building card (and vice versa) for free, and the card's `toggled(false)` already emits `building_type_selected(null)`, which cancels placement. A `GameState.demolish_mode` bool with a signal, mirroring `debug_mode`, carries it to whatever handles the click. One trap: `BuildingPanel._cancel_placement()` uses `set_pressed_no_signal(false)`, which will unpress the demolish toggle without clearing `GameState.demolish_mode`. Right-click already routes through `PlacementController.cancel_placement()`, so that path has to clear the flag explicitly. ### Also in scope, small `place()` sets `build_progress = 1.0` on the `build_days == 0.0` branch. It currently leaves it at 0.0, so the Colony Centre claims to be 0% built for its whole life — nothing read the field until #72 came to render a BUILT building from it. Covered by `test_a_building_that_lands_built_reads_as_fully_built`; drop it if you would rather keep it out of this branch. ### Revised AC - [x] A demolish mode (toggle on the building panel) where clicking a placed building marks it for removal, with visual feedback that it is marked - [x] BUILT: enqueues a `DeconstructionTask` at the entrance cell; completion calls `Buildings.remove()` and emits `building_removed` - [x] PLANNED: cancels the construction task, removed immediately, no deconstruction task - [x] UNDER_CONSTRUCTION: as PLANNED (Decision 1), recorded in the PR - [ ] Marking then unmarking cancels the deconstruction task and restores the building's normal appearance - [ ] A marked building still counts toward its type's `build_limit` - [x] The registry still works with no `TaskManager` at all - [x] Unit tests for the three status branches through the registry — written, currently red - [ ] Headed screenshot of a marked building, since the suite cannot see it
liamjd commented 2026-09-22 20:38:23 +01:00
Author
Owner
Copy link

Clicking a building picked the wrong thing, and it wasn't a timing bug

Reported as "I can mark a building but I can almost never cancel it". The cause
is measurable and had nothing to do with pawns or task timing.

Building views had no collider, so DemolishController raycast the terrain
and asked building_at() which building stood on the cell that was hit. Under a
pitched camera the visible body of a building is drawn offset from the tile it
stands on, and the offset grows with height. Aiming at the middle of what the
player sees landed here, against the shipped catalogue:

type height click lands on result
colony_rocket 14 levels 3 cells away hits nothing
sensor_tower 8 levels 2 cells away hits nothing
ore_mine 3 levels the right cell works
warehouse 2 levels 1 cell away works, but only because its footprint is 2 cells wide

So the clickable region was the footprint on the ground, near the base, while
the thing the player aims at is the body. Marking a tall building was luck.
Unmarking one — where you aim at the body you just lit up orange — was close to
impossible.

Fix

  • Buildings._spawn_view() gives each view a StaticBody3D + BoxShape3D sized
    from the mesh AABB, on its own collision layer (2, now named buildings),
    carrying the building id as metadata. Parented to the view, so it rotates with
    it and is freed with it.
  • Buildings.pick(camera, screen_position) -> Building raycasts masked to that
    layer. The layer and the meta key stay private to the registry.
  • DemolishController calls it and no longer needs map at all.
  • PlacementController deliberately keeps picking the terrain cell — for
    placing, "the cell under the cursor" is the correct answer.

This also fixes clicking a building whose roof overhangs a neighbour, and it is
the same mechanism #78 wants for pawn picking, so that issue can call pick()
for buildings and add its own question for pawns rather than inventing a second
scheme.

Verification

test/unit/test_building_picking.gd — 6 tests, including one that loops every
type in the catalogue and aims at its own mesh centre, so a tall building added
later cannot quietly reintroduce the offset.

The unit tests compute a screen point directly; they cannot exercise the real
hover path because warp_mouse() is a no-op headless. So this was also checked
headed: tools/probe_demolish_click.gd puts the cursor on a 14-level colony
rocket's body, clicks twice, and reports marked then unmarked, saving a
screenshot either side. The marked shot shows the orange tint with the shadow
dropped; the unmarked shot shows it back to its own colour with the shadow
restored.

Also on this branch

GameState.interaction_mode replaced the per-mode booleans (InteractionMode
{NONE, PLACE, DEMOLISH, SELECT, TERRAIN}), and building_placement_canceled is
gone — it and the mode change always fired together. BuildingPanel now follows
the mode rather than keeping its own copy, so a mode ended by right-click no
longer leaves the button lit.

One Godot trap worth recording, pinned by test/unit/test_interaction_mode.gd:
pressing Demolish while a card is selected emits the outgoing button's
toggled(false) before the incoming toggled(true), so the mode is briefly
NONE between them. Writing to the buttons at that moment cancels the press
ButtonGroup is performing, and you end up with no button pressed and no mode set.

## Clicking a building picked the wrong thing, and it wasn't a timing bug Reported as "I can mark a building but I can almost never cancel it". The cause is measurable and had nothing to do with pawns or task timing. Building views had no collider, so `DemolishController` raycast the **terrain** and asked `building_at()` which building stood on the cell that was hit. Under a pitched camera the visible body of a building is drawn offset from the tile it stands on, and the offset grows with height. Aiming at the middle of what the player sees landed here, against the shipped catalogue: | type | height | click lands on | result | |---|---|---|---| | `colony_rocket` | 14 levels | 3 cells away | hits nothing | | `sensor_tower` | 8 levels | 2 cells away | hits nothing | | `ore_mine` | 3 levels | the right cell | works | | `warehouse` | 2 levels | 1 cell away | works, but only because its footprint is 2 cells wide | So the clickable region was the footprint **on the ground, near the base**, while the thing the player aims at is the body. Marking a tall building was luck. Unmarking one — where you aim at the body you just lit up orange — was close to impossible. ### Fix - `Buildings._spawn_view()` gives each view a `StaticBody3D` + `BoxShape3D` sized from the mesh AABB, on its own collision layer (2, now named `buildings`), carrying the building id as metadata. Parented to the view, so it rotates with it and is freed with it. - `Buildings.pick(camera, screen_position) -> Building` raycasts masked to that layer. The layer and the meta key stay private to the registry. - `DemolishController` calls it and no longer needs `map` at all. - `PlacementController` deliberately keeps picking the terrain cell — for *placing*, "the cell under the cursor" is the correct answer. This also fixes clicking a building whose roof overhangs a neighbour, and it is the same mechanism #78 wants for pawn picking, so that issue can call `pick()` for buildings and add its own question for pawns rather than inventing a second scheme. ### Verification `test/unit/test_building_picking.gd` — 6 tests, including one that loops every type in the catalogue and aims at its own mesh centre, so a tall building added later cannot quietly reintroduce the offset. The unit tests compute a screen point directly; they cannot exercise the real hover path because `warp_mouse()` is a no-op headless. So this was also checked headed: `tools/probe_demolish_click.gd` puts the cursor on a 14-level colony rocket's body, clicks twice, and reports **marked then unmarked**, saving a screenshot either side. The marked shot shows the orange tint with the shadow dropped; the unmarked shot shows it back to its own colour with the shadow restored. ### Also on this branch `GameState.interaction_mode` replaced the per-mode booleans (`InteractionMode` {NONE, PLACE, DEMOLISH, SELECT, TERRAIN}), and `building_placement_canceled` is gone — it and the mode change always fired together. `BuildingPanel` now follows the mode rather than keeping its own copy, so a mode ended by right-click no longer leaves the button lit. One Godot trap worth recording, pinned by `test/unit/test_interaction_mode.gd`: pressing Demolish while a card is selected emits the **outgoing** button's `toggled(false)` before the incoming `toggled(true)`, so the mode is briefly NONE between them. Writing to the buttons at that moment cancels the press ButtonGroup is performing, and you end up with no button pressed and no mode set.
Sign in to join this conversation.
No Branch/Tag specified
Branches Tags
main
159-terrain-stats-probe
143-pawn-selection-ring
140-pawn-list-on-hud
78-select-pawn-or-building
129-ticket-planning-skill
117-multi-pawn-work-slots
119-blocked-task-reason
47-highlight-perimeter-border
57-toast-notifications
125-general-code-cleanup
72-demolish-mode-and-deconstruction-task
71-render-planned-building-states
75-pawn-work-loop
112-pawn-knows-its-task
110-work-effort-float
113-game-speed-control
70-construction-task
88-build-time-unit
87-game-calendar-conversations
100-flash-pawn-on-failed-move
83-repath-when-placing-building
56-buildings-block-pathfinding
82-debug-path-overlay
95-debug-scatter-command
claude-md-git-credential-note
80-pawn-mover-walks-waypoints
77-pawn-record-and-registry
65-game-clock
68-parent-and-child-task-cascade
66-task-model-and-manager
60-building-count-limits
61-render-entrance-placement-arrow
55-footprint-entrance
41-cache-building-meshes
54-building-placement-and-model
44-rotate-when-placing-building
37-building-placement-validity-and-highlight
39-building-placement-ghost
36-building-meshes
29-resource-deposits-model
ms-1-building-placement
27-slope-query-in-terrain-grid
17-csgbox-building-placeholder
22-add-pawn-scene
19-render-map-in-editor
12-add-gut-test-framework
11-debug-ui-overlay
6-main-game-scene-grid
2-wire-seed-input
1-run-seed-default
No results found.
Labels
Clear labels   
assets
Art, 3D models, UI themes, sound, music

  
bug
Something is not working

  
duplicate
This issue or pull request already exists

  
enhancement
New feature

  
help wanted
Need some help

  
invalid
Something is wrong

  
question
More information is needed

  
testing
Unit and manual test activities

  
ui
User Interface

  
wontfix
This won't be fixed

No labels
assets
bug
duplicate
enhancement
help wanted
invalid
question
testing
ui
wontfix
Milestone
Clear milestone
No items
Projects
Clear projects
No items
No project
Assignees
Clear assignees
No assignees
1 participant
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference
liamjd/UntitledColonyBuilder#72
No description provided.