unblock_cells()'s early-out is nested inside its own loop and can never fire #99

New issue

Closed

opened 2026-09-15 08:45:38 +01:00 by liamjd · 0 comments

liamjd commented

2026-09-15 08:45:38 +01:00

Owner

Noticed while reviewing #83. Not a live bug — the affected line cannot execute — but the guard it was meant to be is missing, and the code reads as though it is there.

TerrainGrid.unblock_cells() (entities/terrain/terrain_grid.gd:573):

for cell: Vector2i in changed_cells:
    if is_walkable(cell):  # now answers on terrain alone: water stays out
        _astar.add_point(_point_id(cell), Vector2(cell.x, cell.y))
        if changed_cells.is_empty():
            return
_refresh_connections_around(changed_cells)
_main_region = _find_largest_region()

The if changed_cells.is_empty(): return is nested inside the loop and inside the is_walkable() branch, so it is unreachable by construction: the loop only runs when changed_cells is non-empty, and the body only runs for a cell that is in it.

Its sibling block_cells() (line 556) has the same early-out in the right place — between the loop and the refresh — where it does real work:

if changed_cells.is_empty():
    return
_refresh_connections_around(changed_cells)
_main_region = _find_largest_region()

So unblock_cells() currently runs _refresh_connections_around([]) and a full _find_largest_region() sweep even when nothing changed — i.e. when every cell handed to it was already unblocked. _find_largest_region() flood-fills the whole grid, so an inert call is not free, and unblock_cells() is on the building_removed path where a demolition UI will call it repeatedly.

Fix: dedent the check out of the loop, to match block_cells().

for cell: Vector2i in changed_cells:
    if is_walkable(cell):
        _astar.add_point(_point_id(cell), Vector2(cell.x, cell.y))
if changed_cells.is_empty():
    return
_refresh_connections_around(changed_cells)
_main_region = _find_largest_region()

AC:

The early-out sits between the loop and the refresh, mirroring block_cells().
test_unblocking_a_cell_that_was_never_blocked_is_inert (test/unit/test_terrain_navigation.gd:308) still passes — it already covers the behaviour, which is why nothing caught the misplacement.

Noticed while reviewing #83. Not a live bug — the affected line cannot execute — but the guard it was meant to be is missing, and the code reads as though it is there. `TerrainGrid.unblock_cells()` (entities/terrain/terrain_grid.gd:573): ```gdscript for cell: Vector2i in changed_cells: if is_walkable(cell): # now answers on terrain alone: water stays out _astar.add_point(_point_id(cell), Vector2(cell.x, cell.y)) if changed_cells.is_empty(): return _refresh_connections_around(changed_cells) _main_region = _find_largest_region() ``` The `if changed_cells.is_empty(): return` is nested inside the loop **and** inside the `is_walkable()` branch, so it is unreachable by construction: the loop only runs when `changed_cells` is non-empty, and the body only runs for a cell that is in it. Its sibling `block_cells()` (line 556) has the same early-out in the right place — between the loop and the refresh — where it does real work: ```gdscript if changed_cells.is_empty(): return _refresh_connections_around(changed_cells) _main_region = _find_largest_region() ``` So `unblock_cells()` currently runs `_refresh_connections_around([])` and a full `_find_largest_region()` sweep even when nothing changed — i.e. when every cell handed to it was already unblocked. `_find_largest_region()` flood-fills the whole grid, so an inert call is not free, and `unblock_cells()` is on the `building_removed` path where a demolition UI will call it repeatedly. Fix: dedent the check out of the loop, to match `block_cells()`. ```gdscript for cell: Vector2i in changed_cells: if is_walkable(cell): _astar.add_point(_point_id(cell), Vector2(cell.x, cell.y)) if changed_cells.is_empty(): return _refresh_connections_around(changed_cells) _main_region = _find_largest_region() ``` AC: - The early-out sits between the loop and the refresh, mirroring `block_cells()`. - `test_unblocking_a_cell_that_was_never_blocked_is_inert` (test/unit/test_terrain_navigation.gd:308) still passes — it already covers the behaviour, which is why nothing caught the misplacement.

liamjd added this to the Pawns and pathfinding milestone

2026-09-15 08:45:38 +01:00

liamjd referenced this issue from a commit

2026-09-17 07:57:18 +01:00

liamjd referenced this issue from a commit

2026-09-17 07:57:29 +01:00

Dedent unblock_cells()'s early-out so it can actually fire

liamjd referenced this issue from a pull request that will close it,

2026-09-17 07:57:45 +01:00

Dedent unblock_cells()'s unreachable early-out #105

liamjd closed this issue

2026-09-17 07:59:25 +01:00