Left-clicking a valid site places the building #58

Merged

liamjd merged 5 commits from 54-building-placement-and-model into main

2026-09-01 19:30:52 +01:00

liamjd commented

2026-09-01 19:28:54 +01:00

Owner

This is the last acceptance criterion of the Building placement milestone's core flow, and it introduces the first piece of persistent colony state — so most of the work below is in the data model rather than the click handling.

Closes #54

What landed

Building (record) and Buildings (registry). The record is the truth and the mesh is one way of drawing it. Building stores only type, anchor, rotation_steps and status; the cells it occupies are derived (offsets() → cells()) rather than stored, so the registry's _occupancy dictionary is a cache of that derivation rather than a second copy that can disagree with it. The record holds no node references, which is what keeps it serialisable for save/load later.

Buildings.place(), remove(), building_at(), is_occupied(), occupied_cells() and all(). remove() has no caller yet — demolition UI is a separate issue — but it is in the API because it is the thing that proves occupancy is maintained in one place; #56 also depends on it for unblocking A* cells.

OCCUPIED placement rule. Appended to the end of Reason so existing ordinals do not shift, and occupied is an optional Dictionary parameter so PlacementRules stays static and dependency-free and every prior call site still compiles. Checked after OUT_OF_BOUNDS, before WATER.

BuildingSite — a static helper owning the site position and rotation maths, called by both the ghost and the placed mesh. This exists specifically so the two cannot drift: if each computed the site independently, the bug would surface as a building that does not land where the ghost promised.

Input actions. place_building (LMB) and cancel_building_placement (RMB), both listed in the keybindings panel.

Verification

GUT: 131 tests, 131 passing, no orphans (up from 105 before this branch).

test/unit/test_buildings.gd (new, 19 tests) — record contents, distinct ids, occupancy under rotation, shape-not-bounding-box, building_at() identity across every cell of a multi-tile building, three overlap cases, four removal cases, record/view agreement, no-map operation, and both building_placed signal cases.
test/unit/test_building_site.gd (new, 10 tests) — tile centring, base-on-surface seating, seating on the highest cell of a straddling footprint, half- and quarter-turn behaviour, anchor translation, offsets-vs-absolute-cells, empty-footprint fallback, rotation sign.
test/unit/test_placement_rules.gd (extended, 7 tests) — occupied cell rejected, single-cell overlap rejected, adjacent-but-clear accepted, bounding-box hole ignored, default argument unchanged, and both check-order pins (OUT_OF_BOUNDS before OCCUPIED, OCCUPIED before WATER).

Mutation-tested, because a green suite only proves the code does something:

Mutation	Result
`place()` reduced to an anchor-only overlap guard	1 failure
`remove()` erases the record but not the occupancy	3 failures
`WATER` checked before `OCCUPIED`	1 failure

The first of those found a real gap: the original overlap tests all put the second building's anchor on an occupied cell, so they passed with or without the per-cell loop. test_an_overlap_that_misses_the_anchor_is_still_rejected was added to cover the discriminating case and is the one that fails under that mutant.

Rendering checked headed and by eye, since gl_compatibility draws nothing headless. Four warehouses (five_thick_c, an asymmetric C) at all four rotations on flat ground, plus one of each catalogue type: each lands squarely on its own cells, sits flush on the surface, is opaque, casts a shadow, and carries its own colour. Confirmed numerically as well — the mesh centre matches the centre of the cells the record claims, for every rotation.

Bugs found and fixed along the way

One shared material for every placed building. A single StandardMaterial3D was being reused as material_override on every mesh; since materials are reference-typed resources, mutating albedo_color repainted every building already on the map. Fixed by removing the override entirely — BuildingMeshes.build() already bakes placeholder_color onto the mesh's own material, and letting that through is how a building gets its colour.
The placed mesh was a duplicate() of the ghost, so it inherited the ghost's translucent override and its cast_shadow = OFF. Placed buildings now build a fresh MeshInstance3D.
position_for() was handed absolute cells instead of anchor-relative offsets, counting the anchor twice and landing a warehouse 5 world units from its footprint. It failed quietly rather than loudly because calculate_bounds() seeds its min/max at zero, which is only correct for an array containing the origin. Caught by the headed screenshot, not by the unit tests — every test at the time passed offsets. Two regression tests now pin it.
Left-click did not check _hover_valid, so a building could be placed on a site the ghost showed as invalid. The three early returns in _physics_process() also did not clear the flag despite a comment saying they must; they now share _clear_hover().
Right-click cancel vs. camera rotation. Right button is shared: drag free-rotates the camera, click-without-drag cancels placement. Converting to an input action needs both edges (is_action_pressed to record the press point, is_action_released to measure the drag distance), and neither branch may call set_input_as_handled() — PlacementController sits below CameraRig in the tree and so sees the event first, and consuming it would break free-rotate.
Buildings.place() only guarded the anchor cell, letting a rotated footprint silently steal cells from an existing building whose cells() still claimed them. Now guards every cell.

Known and deliberate

The placed mesh has no collider. Rejecting an occupied site is entirely the OCCUPIED rule's job; there is no physics fallback. This does not affect pawn navigation either — see #56, which covers making the A* graph aware of occupancy.
Placeholder meshes are boxes over the footprint's bounding rect, so two legally-placed buildings with concave footprints can visually intersect at the notch. Goes away with real meshes.
Regenerating the terrain does not clear the registry, so reroll() / next_shape() leave placed buildings floating over a new map. Known debug-only wart, deliberately not addressed here.
Buildings land BUILT; construction progression, unique enforcement, demolition UI and save/load are all out of scope per the issue.
level_at() is the sampled level, not the drawn one, so a placed mesh sinks on a shoreline exactly as the ghost does. That is #46's job.

Not covered by the suite

Rendering. Colour, material, opacity and seating are verified by headed screenshot only, as gl_compatibility renders nothing headless.

Left-clicking a valid site now commits the building: a solid mesh in the type's colour lands on the terrain, the cells it occupies are recorded so nothing else can be built on them, and placement mode ends. This is the last acceptance criterion of the Building placement milestone's core flow, and it introduces the first piece of persistent colony state — so most of the work below is in the data model rather than the click handling. Closes #54 ## What landed **`Building` (record) and `Buildings` (registry).** The record is the truth and the mesh is one way of drawing it. `Building` stores only `type`, `anchor`, `rotation_steps` and `status`; the cells it occupies are *derived* (`offsets()` → `cells()`) rather than stored, so the registry's `_occupancy` dictionary is a cache of that derivation rather than a second copy that can disagree with it. The record holds no node references, which is what keeps it serialisable for save/load later. `Buildings.place()`, `remove()`, `building_at()`, `is_occupied()`, `occupied_cells()` and `all()`. `remove()` has no caller yet — demolition UI is a separate issue — but it is in the API because it is the thing that proves occupancy is maintained in one place; #56 also depends on it for unblocking A\* cells. **`OCCUPIED` placement rule.** Appended to the end of `Reason` so existing ordinals do not shift, and `occupied` is an optional `Dictionary` parameter so `PlacementRules` stays static and dependency-free and every prior call site still compiles. Checked after `OUT_OF_BOUNDS`, before `WATER`. **`BuildingSite`** — a static helper owning the site position and rotation maths, called by both the ghost and the placed mesh. This exists specifically so the two cannot drift: if each computed the site independently, the bug would surface as a building that does not land where the ghost promised. **Input actions.** `place_building` (LMB) and `cancel_building_placement` (RMB), both listed in the keybindings panel. ## Verification **GUT: 131 tests, 131 passing, no orphans** (up from 105 before this branch). - `test/unit/test_buildings.gd` (new, 19 tests) — record contents, distinct ids, occupancy under rotation, shape-not-bounding-box, `building_at()` identity across every cell of a multi-tile building, three overlap cases, four removal cases, record/view agreement, no-map operation, and both `building_placed` signal cases. - `test/unit/test_building_site.gd` (new, 10 tests) — tile centring, base-on-surface seating, seating on the highest cell of a straddling footprint, half- and quarter-turn behaviour, anchor translation, offsets-vs-absolute-cells, empty-footprint fallback, rotation sign. - `test/unit/test_placement_rules.gd` (extended, 7 tests) — occupied cell rejected, single-cell overlap rejected, adjacent-but-clear accepted, bounding-box hole ignored, default argument unchanged, and both check-order pins (`OUT_OF_BOUNDS` before `OCCUPIED`, `OCCUPIED` before `WATER`). **Mutation-tested**, because a green suite only proves the code does something: | Mutation | Result | | --- | --- | | `place()` reduced to an anchor-only overlap guard | 1 failure | | `remove()` erases the record but not the occupancy | 3 failures | | `WATER` checked before `OCCUPIED` | 1 failure | The first of those found a real gap: the original overlap tests all put the second building's *anchor* on an occupied cell, so they passed with or without the per-cell loop. `test_an_overlap_that_misses_the_anchor_is_still_rejected` was added to cover the discriminating case and is the one that fails under that mutant. **Rendering checked headed and by eye**, since `gl_compatibility` draws nothing headless. Four warehouses (`five_thick_c`, an asymmetric C) at all four rotations on flat ground, plus one of each catalogue type: each lands squarely on its own cells, sits flush on the surface, is opaque, casts a shadow, and carries its own colour. Confirmed numerically as well — the mesh centre matches the centre of the cells the record claims, for every rotation. ## Bugs found and fixed along the way - **One shared material for every placed building.** A single `StandardMaterial3D` was being reused as `material_override` on every mesh; since materials are reference-typed resources, mutating `albedo_color` repainted every building already on the map. Fixed by removing the override entirely — `BuildingMeshes.build()` already bakes `placeholder_color` onto the mesh's own material, and letting that through *is* how a building gets its colour. - **The placed mesh was a `duplicate()` of the ghost**, so it inherited the ghost's translucent override *and* its `cast_shadow = OFF`. Placed buildings now build a fresh `MeshInstance3D`. - **`position_for()` was handed absolute cells instead of anchor-relative offsets**, counting the anchor twice and landing a warehouse 5 world units from its footprint. It failed quietly rather than loudly because `calculate_bounds()` seeds its min/max at zero, which is only correct for an array containing the origin. Caught by the headed screenshot, not by the unit tests — every test at the time passed offsets. Two regression tests now pin it. - **Left-click did not check `_hover_valid`**, so a building could be placed on a site the ghost showed as invalid. The three early returns in `_physics_process()` also did not clear the flag despite a comment saying they must; they now share `_clear_hover()`. - **Right-click cancel vs. camera rotation.** Right button is shared: drag free-rotates the camera, click-without-drag cancels placement. Converting to an input action needs *both* edges (`is_action_pressed` to record the press point, `is_action_released` to measure the drag distance), and neither branch may call `set_input_as_handled()` — `PlacementController` sits below `CameraRig` in the tree and so sees the event first, and consuming it would break free-rotate. - **`Buildings.place()` only guarded the anchor cell**, letting a rotated footprint silently steal cells from an existing building whose `cells()` still claimed them. Now guards every cell. ## Known and deliberate - **The placed mesh has no collider.** Rejecting an occupied site is entirely the `OCCUPIED` rule's job; there is no physics fallback. This does not affect pawn navigation either — see #56, which covers making the A\* graph aware of occupancy. - **Placeholder meshes are boxes over the footprint's bounding rect**, so two legally-placed buildings with concave footprints can visually intersect at the notch. Goes away with real meshes. - **Regenerating the terrain does not clear the registry**, so `reroll()` / `next_shape()` leave placed buildings floating over a new map. Known debug-only wart, deliberately not addressed here. - **Buildings land `BUILT`**; construction progression, `unique` enforcement, demolition UI and save/load are all out of scope per the issue. - **`level_at()` is the sampled level, not the drawn one**, so a placed mesh sinks on a shoreline exactly as the ghost does. That is #46's job. ## Not covered by the suite Rendering. Colour, material, opacity and seating are verified by headed screenshot only, as `gl_compatibility` renders nothing headless.

liamjd added 4 commits

2026-09-01 19:28:54 +01:00

Add Buildings node and core building placement logic 37e97c9f66

- Added `Buildings` node in the scene to manage building instances.
- Implemented `Buildings` script for handling building placement, occupancy checks, and management.
- Introduced a `Building` class to represent individual building instances.
- Integrated building placement into `PlacementController`.
- Minor variable naming update in `PlacementController`.

Add "OCCUPIED" reason to placement rules and improve building placement logic 739b1897ac

- Enhanced placement validation by introducing "OCCUPIED" as a new reason.
- Updated `Buildings` logic to track and validate occupancy for accurate placement.
- Adjusted `PlacementController` to handle new validation state and update visualization.
- Added `building_placed` signal for better event integration.

Refactor building placement mechanics and add BuildingSite utility class. 8155063a54

- Consolidated building placement logic with the new `BuildingSite` class for precise mesh positioning and rotation calculations.
- Enhanced error handling for footprint and occupancy validation in `Buildings`.
- Added `_spawn_view` to create placed building visuals with improved material handling.
- Updated `PlacementController` to align ghost and building placements using shared positioning logic.
- Introduced unit tests for `BuildingSite` to validate positioning and rotation calculations.
- Injected `map` dependency into `Buildings` for better testability and modularity.

Improve building placement and management logic across multiple components: 5c964958c6

- Added `remove()` method in `Buildings` to support removal of buildings, clearing occupancy and associated meshes.
- Introduced new tests for building placement, overlap, and removal validity.
- Enhanced placement validation to handle occupancy checks with tests for edge cases.
- Updated `KeybindingsPanel` and `PlacementController` for new input actions (`place_building` and `cancel_building_placement`).
- Refactored layout and flags in `KeybindingsPanel` for clarity and usability.

liamjd added 1 commit

2026-09-01 19:29:56 +01:00

Free placed-building views immediately rather than deferring 876473e1ae

_despawn_view() used remove_child() followed by queue_free(), which leaves
the mesh parentless but not yet freed until the end of the frame. That is
exactly the state GUT's orphan counter looks for, so every test calling
remove() reported an orphan - four warnings per suite run.

The view is a leaf MeshInstance3D with no script, no signal connections and
no processing, so there is no in-flight callback for an immediate free to
disturb, which is the case queue_free() exists to protect against. free()
also keeps get_child_count() honest the moment remove() returns.

Suite: 131 tests, 131 passing, no orphans.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01UKZD2iAUWjpqEijm2yK2f7

liamjd merged commit 3d18669183 into main

2026-09-01 19:30:52 +01:00

liamjd referenced this pull request from a commit

2026-09-01 19:30:53 +01:00

Merge pull request 'Left-clicking a valid site places the building' (#58) from 54-building-placement-and-model into main