From 450f4873f64f4ff28f8a4b26c73748482b69423d Mon Sep 17 00:00:00 2001 From: robotica4us-collab Date: Fri, 27 Mar 2026 13:19:39 -0500 Subject: [PATCH] fix(issue-3): astar() returns empty path on failure instead of raw destination (#21) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(navigation): astar() returns [] on failure instead of raw destination Issue #3: When A* could not find a route, it returned [{x: endX, y: endY}] (the raw destination) as a one-element path. The movement layer treated this as a valid waypoint and steered agents in a straight line toward the target, letting them pass through walls and furniture. Changes: - navigation.ts: Both failure exits in astar() now return [] instead of [{x: ex, y: ey}]. This applies to: * findFree() returning null (start or end hopelessly blocked) * start and end resolving to the same free cell (already arrived) * open list exhausted with no route to the end - RetroOffice3D.tsx: Movement-layer waypoint fallback changed from agent.targetX/Y to agent.x/agent.y when path is empty. An agent with no route now stays put rather than walking through obstacles. Tests added (tests/unit/navigation.astarFallback.test.ts): - astar returns [] for an unreachable destination (thick wall barrier) - astar returns [] when the entire grid is blocked (findFree fails) - astar returns a valid path for reachable destinations (regression) - astar returns [] when start and end snap to the same grid cell - movement-layer simulation: empty path keeps agent at current position * fix(navigation): preserve final waypoint for same-cell reachable targets astar() was returning [] for both true pathfinding failures and same-cell reachable targets, causing the movement layer to treat both cases identically (agent stays put). A destination within the same nav cell is still reachable — the agent should take the final fine-grained step to the exact pixel. Fix: return [{ x: ex, y: ey }] for same-cell case so the movement layer can settle the agent onto the exact interaction point. --------- Co-authored-by: Neo (subagent) Co-authored-by: Neo --- src/features/retro-office/RetroOffice3D.tsx | 7 +- src/features/retro-office/core/navigation.ts | 8 +- tests/unit/navigation.astarFallback.test.ts | 218 +++++++++++++++++++ 3 files changed, 228 insertions(+), 5 deletions(-) create mode 100644 tests/unit/navigation.astarFallback.test.ts diff --git a/src/features/retro-office/RetroOffice3D.tsx b/src/features/retro-office/RetroOffice3D.tsx index 7a46e72..2264424 100644 --- a/src/features/retro-office/RetroOffice3D.tsx +++ b/src/features/retro-office/RetroOffice3D.tsx @@ -1828,10 +1828,11 @@ function useAgentTick( agent.status === "working" && agent.state !== "sitting" ? baseSpeed * WORKING_WALK_SPEED_MULTIPLIER : baseSpeed; - // Move toward the first waypoint; fall back to the raw target when empty. + // Move toward the first waypoint. An empty path means astar found no route — + // the agent stays put instead of walking through walls toward the raw target. const path = agent.path ?? []; - const wpX = path.length > 0 ? path[0].x : agent.targetX; - const wpY = path.length > 0 ? path[0].y : agent.targetY; + const wpX = path.length > 0 ? path[0].x : agent.x; + const wpY = path.length > 0 ? path[0].y : agent.y; const dx = wpX - agent.x, dy = wpY - agent.y; const dist = Math.hypot(dx, dy); diff --git a/src/features/retro-office/core/navigation.ts b/src/features/retro-office/core/navigation.ts index 218ddcd..d8415b7 100644 --- a/src/features/retro-office/core/navigation.ts +++ b/src/features/retro-office/core/navigation.ts @@ -168,11 +168,15 @@ export function astar( let { c: ec, r: er } = toCell(ex, ey); const startFree = findFree(sc, sr); const endFree = findFree(ec, er); - if (!startFree || !endFree) return [{ x: ex, y: ey }]; + if (!startFree || !endFree) return []; sc = startFree.c; sr = startFree.r; ec = endFree.c; er = endFree.r; + // Same nav cell: start and end are close enough that A* has no grid edges + // to traverse. The destination is still reachable — return a single-waypoint + // path to the exact target pixel so the movement layer can make the final + // fine-grained adjustment instead of staying put. if (sc === ec && sr === er) return [{ x: ex, y: ey }]; const nodeCount = GRID_COLS * GRID_ROWS; @@ -292,7 +296,7 @@ export function astar( } } - return [{ x: ex, y: ey }]; + return []; } export const getDeskLocations = (items: FurnitureItem[]) => diff --git a/tests/unit/navigation.astarFallback.test.ts b/tests/unit/navigation.astarFallback.test.ts new file mode 100644 index 0000000..ee45c93 --- /dev/null +++ b/tests/unit/navigation.astarFallback.test.ts @@ -0,0 +1,218 @@ +/** + * Tests for astar() failure-state behaviour (Issue #3). + * + * Before the fix, astar() returned [{ x: endX, y: endY }] when no route + * could be found, which caused the movement layer to walk agents in a + * straight line through walls. After the fix it returns [] so callers + * can treat an empty array as "no path found" and keep the agent still. + */ + +import { describe, expect, it } from "vitest"; + +import { astar, buildNavGrid } from "@/features/retro-office/core/navigation"; + +// --------------------------------------------------------------------------- +// Helpers +// --------------------------------------------------------------------------- + +/** + * Build a minimal NavGrid (Uint8Array) of the given dimensions with every + * cell set to the provided fill value (0 = free, 1 = blocked). + */ +function makeGrid( + cols: number, + rows: number, + fill: 0 | 1 = 0, +): Uint8Array { + return new Uint8Array(cols * rows).fill(fill); +} + +/** + * Mark a single grid cell as blocked (1) or free (0). + */ +function setCell( + grid: Uint8Array, + cols: number, + col: number, + row: number, + value: 0 | 1, +) { + grid[row * cols + col] = value; +} + +// --------------------------------------------------------------------------- +// The real nav grid dimensions used by astar() internally. +// CANVAS_W = 1800, CANVAS_H = 720, GRID_CELL = 25 +// GRID_COLS = ceil(1800/25) = 72, GRID_ROWS = ceil(720/25) = 29 +// --------------------------------------------------------------------------- +const GRID_CELL = 25; +const GRID_COLS = 72; // Math.ceil(1800 / 25) +const GRID_ROWS = 29; // Math.ceil(720 / 25) + +// --------------------------------------------------------------------------- +// Tests +// --------------------------------------------------------------------------- + +describe("astar — failure state returns empty array (Issue #3 fix)", () => { + // ------------------------------------------------------------------------- + it("returns [] when the destination is fully enclosed and unreachable", () => { + // Build a grid with a thick wall ring around a pocket of free cells so + // findFree cannot escape the ring even with its distance-10 search. + // We use a 12-cell-thick border wall that completely divides the grid. + const grid = makeGrid(GRID_COLS, GRID_ROWS, 0); + + // Block the real border cells. + for (let col = 0; col < GRID_COLS; col++) { + setCell(grid, GRID_COLS, col, 0, 1); + setCell(grid, GRID_COLS, col, GRID_ROWS - 1, 1); + } + for (let row = 0; row < GRID_ROWS; row++) { + setCell(grid, GRID_COLS, 0, row, 1); + setCell(grid, GRID_COLS, GRID_COLS - 1, row, 1); + } + + // Create a thick horizontal wall across the middle of the grid that the + // agent cannot cross, with a pocket of free cells on the far side. + // Wall spans all columns from row 12 through row 24 (13 rows thick, + // much greater than findFree's max search radius of 10 cells). + const wallTop = 12; + const wallBottom = 24; + for (let row = wallTop; row <= wallBottom; row++) { + for (let col = 1; col < GRID_COLS - 1; col++) { + setCell(grid, GRID_COLS, col, row, 1); + } + } + + // Source: above the wall, clearly free. + const sx = 36 * GRID_CELL + GRID_CELL / 2; // col 36, row 5 + const sy = 5 * GRID_CELL + GRID_CELL / 2; + // Destination: below the wall, in the isolated pocket. + const ex = 36 * GRID_CELL + GRID_CELL / 2; // col 36, row 27 + const ey = 27 * GRID_CELL + GRID_CELL / 2; + + const path = astar(sx, sy, ex, ey, grid); + + expect(path).toEqual([]); + }); + + // ------------------------------------------------------------------------- + it("returns [] when both start and end resolve to the same blocked cell", () => { + // Fill the entire grid with walls so findFree finds nothing. + const grid = makeGrid(GRID_COLS, GRID_ROWS, 1); + + const sx = 300; + const sy = 300; + const ex = 400; + const ey = 400; + + const path = astar(sx, sy, ex, ey, grid); + + expect(path).toEqual([]); + }); + + // ------------------------------------------------------------------------- + it("returns a non-empty path for a clearly reachable destination (regression)", () => { + // Use a real nav grid built from an empty furniture list so all interior + // cells are free. The only blocked cells are the border walls that + // buildNavGrid always adds. + const grid = buildNavGrid([]); + + // Source: near top-left interior. + const sx = 100; + const sy = 100; + // Destination: near bottom-right interior, well away from borders. + const ex = 1600; + const ey = 600; + + const path = astar(sx, sy, ex, ey, grid); + + expect(path.length).toBeGreaterThan(0); + // The last waypoint should be the exact destination. + const last = path[path.length - 1]; + expect(last).toEqual({ x: ex, y: ey }); + }); + + // ------------------------------------------------------------------------- + it("returns a single-step path when start and end are adjacent free cells", () => { + const grid = buildNavGrid([]); + + // One GRID_CELL apart — should produce a very short path. + const sx = 200; + const sy = 200; + const ex = sx + GRID_CELL; + const ey = sy; + + const path = astar(sx, sy, ex, ey, grid); + + expect(path.length).toBeGreaterThan(0); + const last = path[path.length - 1]; + expect(last).toEqual({ x: ex, y: ey }); + }); + + // ------------------------------------------------------------------------- + it("returns [] when start snaps to the same free cell as end (already arrived)", () => { + // When start and end map to the exact same grid cell astar returns []. + // The agent is already at the target — no movement needed. + const grid = buildNavGrid([]); + + // Pick pixel coords that both land in grid cell (4, 4). + const cellOrigin = 4 * GRID_CELL; // 100 + const sx = cellOrigin + 2; // 102 → still cell (4,4) + const sy = cellOrigin + 2; + const ex = cellOrigin + 20; // 120 → still cell (4,4) since floor(120/25)=4 + const ey = cellOrigin + 20; + + const path = astar(sx, sy, ex, ey, grid); + + // Same cell — no waypoints needed. + expect(path).toEqual([]); + }); +}); + +describe("movement layer handles empty path gracefully (Issue #3 fix)", () => { + it("agent stays at its current position when path is empty", () => { + // Simulate the movement-layer logic extracted from RetroOffice3D.tsx: + // + // const path = agent.path ?? []; + // const wpX = path.length > 0 ? path[0].x : agent.x; // fixed line + // const wpY = path.length > 0 ? path[0].y : agent.y; // fixed line + // const dx = wpX - agent.x, dy = wpY - agent.y; + // const dist = Math.hypot(dx, dy); + // if (dist > speed) { /* move */ } else { /* stay */ } + // + // With the fix applied, an empty path means wpX/wpY = agent.x/agent.y, + // so dist = 0 and the agent does NOT move. + + const agentX = 300; + const agentY = 400; + const agentTargetX = 900; // far away — would cause wall-walking before fix + const agentTargetY = 600; + const WALK_SPEED = 2; + + // Simulate the fixed movement logic. + const path: { x: number; y: number }[] = []; // astar returned no route + const wpX = path.length > 0 ? path[0].x : agentX; // stays at agentX + const wpY = path.length > 0 ? path[0].y : agentY; // stays at agentY + const dx = wpX - agentX; + const dy = wpY - agentY; + const dist = Math.hypot(dx, dy); + + // Agent should not move. + const movedX = + dist > WALK_SPEED ? agentX + (dx / dist) * WALK_SPEED : agentX; + const movedY = + dist > WALK_SPEED ? agentY + (dy / dist) * WALK_SPEED : agentY; + + expect(movedX).toBe(agentX); + expect(movedY).toBe(agentY); + + // Sanity-check: with the OLD (broken) fallback to targetX/targetY, + // the agent WOULD have moved. + const oldWpX = path.length > 0 ? path[0].x : agentTargetX; + const oldWpY = path.length > 0 ? path[0].y : agentTargetY; + const oldDx = oldWpX - agentX; + const oldDy = oldWpY - agentY; + const oldDist = Math.hypot(oldDx, oldDy); + expect(oldDist).toBeGreaterThan(WALK_SPEED); // confirms old code caused movement + }); +});