diff --git a/src/app/game/agents/Person.ts b/src/app/game/agents/Person.ts index c340720..2ba66d2 100644 --- a/src/app/game/agents/Person.ts +++ b/src/app/game/agents/Person.ts @@ -120,9 +120,14 @@ export default class Person { this.social.setGender(gender); } - walk(currentTile: Tile, timeDelta: number): void { + // Moves the person one axis-step toward the current target. Returns true ONLY on the frame it detects + // final arrival at the destination (path exhausted + target reached). Callers must observe arrival through + // this return value: walk() clears currentTarget/currentDestination the instant it arrives, so a caller + // re-querying isDestinationReached() afterwards would read the already-wiped state and see a false negative + // (the bug that stalled the WalkingToCar/WalkingToDestination travel steps). + walk(currentTile: Tile, timeDelta: number): boolean { if (this.insideBuilding || !this.asset || !this.currentTarget || !this.currentDestination /*|| !(currentTile instanceof Road)*/) { - return; + return false; } const speedX = this.speed * Math.sign(this.currentTarget.x - this.x) * timeDelta; @@ -150,13 +155,15 @@ export default class Person { if (this.isDestinationReached()) { this.currentTarget = null; this.currentDestination = null; - return; + return true; } if (this.isCurrentTargetReached()) { this.setNextTarget(currentTile); - return; + return false; } + + return false; } setNextTarget(currentTile: Tile): void { @@ -294,8 +301,9 @@ export default class Person { } break; case TravelStep.WalkingToCar: - this.walk(currentTile, timeDelta); - if (this.isDestinationReached()) { + // walk() reports arrival directly: re-querying isDestinationReached() here would read the + // state walk() just cleared on arrival and stall this step forever (the arrival false-negative). + if (this.walk(currentTile, timeDelta)) { this.travelStep = TravelStep.EnteringCar; } break; @@ -333,8 +341,9 @@ export default class Person { this.travelStep = TravelStep.WalkingToDestination; break; case TravelStep.WalkingToDestination: - this.walk(currentTile, timeDelta); - if (this.isDestinationReached()) { + // Same arrival contract as WalkingToCar: trust walk()'s return, not a post-hoc + // isDestinationReached() read of the state it wiped on arrival. + if (this.walk(currentTile, timeDelta)) { this.travelStep = TravelStep.Arrived; } break; diff --git a/test/agents/person.test.ts b/test/agents/person.test.ts index cd0c3c1..c839a58 100644 --- a/test/agents/person.test.ts +++ b/test/agents/person.test.ts @@ -230,39 +230,81 @@ describe('walk() guard clauses skip movement entirely', () => { }); describe('walk(): real per-frame movement', () => { - test('converges on the target curb over successive calls, switching axis and updating depth along the way', () => { - const road = new Road(2, 2, 'road'); - road.calculateCurb({ width: 48, height: 48 }, { x: 120, y: 120 }); // curb corners at 100/140 + test('real per-frame movement drives a WalkingToDestination step past arrival to Arrived (no manual travelStep poking)', () => { + // A walking commute (no vehicle): ExitingBuilding -> WalkingToDestination, then real per-frame walk() + // movement alone must carry the step to Arrived. This is the regression guard: walk() clears its + // target/destination the instant it arrives, so if processTravel re-queried isDestinationReached() + // afterwards it would read the wiped state, never see arrival, and stall in WalkingToDestination. + const currentTile = new Road(2, 2, 'road'); + const destRoad = new Road(3, 3, null); + destRoad.calculateCurb({ width: 48, height: 48 }, { x: 168, y: 168 }); // curb corners near 148/188 + const destBuilding = new Building(5, 5, null); + + const gameStub = { + pixelToTilePosition: () => ({ row: 3, col: 3 }), + field: { getTile: () => destRoad, removeVehicle: () => undefined }, + } as unknown as GameManager; + const pathFinder = { findPath: () => [destRoad] } as unknown as PathFinder; + const p = new Person(0, 0); + p.setGameManager(gameStub); p.setAsset({} as any); - const pathFinder = { findPath: () => [road] } as unknown as PathFinder; + p.setDestination(destBuilding); // no vehicle -> walking commute - p.setDestinationTile(road, { row: 2, col: 2 }, pathFinder); + p.update(currentTile, 0, new Set(), pathFinder); // ExitingBuilding -> WalkingToDestination + expect((p as any).travelStep).toBe(TravelStep.WalkingToDestination); + expect((p as any).currentTarget).not.toBeNull(); const target = { ...(p as any).currentTarget }; - expect(target).not.toBeNull(); + // Drive real movement only — no manual travelStep assignment — until the step advances or we time out. let iterations = 0; - while ((p as any).currentTarget !== null && iterations < 500) { - p.walk(road, 20); + while ((p as any).travelStep === TravelStep.WalkingToDestination && iterations < 2000) { + p.update(currentTile, 50, new Set(), pathFinder); iterations++; } - expect(iterations).toBeLessThan(500); // actually converged, not just timed out - // walk() never snaps to the exact target (unlike Vehicle.drive()), it only stops advancing once - // within the < 1px "reached" threshold, so the final position lands close to, not exactly on, it. + // The observable win: arrival detected during real walk() movement actually advanced the travel step. + expect(iterations).toBeLessThan(2000); // advanced, not stalled/timed out + expect((p as any).travelStep).toBe(TravelStep.Arrived); + // And it genuinely walked onto the target curb (within the < 1px "reached" threshold) to get there. const finalPosition = p.getPosition()!; expect(Math.abs(finalPosition.x - target.x)).toBeLessThan(1); expect(Math.abs(finalPosition.y - target.y)).toBeLessThan(1); - expect(p.getDepth()).toBe((road.getRow() + 1) * 10 + 1); - - // walk() clears currentTarget/currentDestination the instant it detects arrival internally. A - // caller-side isDestinationReached() check made right after — exactly what processTravel's - // WalkingToCar/WalkingToDestination cases do immediately after calling walk() — therefore reads the - // already-cleared state and sees a false negative. This documents real, current behavior (existing - // travel-flow tests, e.g. test/agents/personTravel.test.ts, work around it by forcing travelStep - // directly instead of driving arrival through real per-frame walk() calls). - expect((p as any).currentDestination).toBeNull(); - expect(p.isDestinationReached()).toBe(false); + expect(p.getDepth()).toBe((currentTile.getRow() + 1) * 10 + 1); + }); + + test('real per-frame movement drives a WalkingToCar step past arrival to EnteringCar (no manual travelStep poking)', () => { + // Same regression guard on the vehicle commute path: WalkingToCar must advance to EnteringCar off real + // walk() movement alone. + const homeTile = new Road(2, 2, 'road'); + const vehicleRoad = new Road(3, 3, null); + vehicleRoad.calculateCurb({ width: 48, height: 48 }, { x: 168, y: 168 }); + const vehicle = new Vehicle(168, 168); + const destBuilding = new Building(5, 5, null); + + const gameStub = { + pixelToTilePosition: () => ({ row: 3, col: 3 }), + field: { getTile: () => vehicleRoad, removeVehicle: () => undefined }, + } as unknown as GameManager; + const pathFinder = { findPath: () => [vehicleRoad] } as unknown as PathFinder; + + const p = new Person(0, 0); + p.setGameManager(gameStub); + p.setVehicle(vehicle); + p.setAsset({} as any); + p.setDestination(destBuilding); + + p.update(homeTile, 0, new Set(), pathFinder); // ExitingBuilding -> WalkingToCar + expect((p as any).travelStep).toBe(TravelStep.WalkingToCar); + + let iterations = 0; + while ((p as any).travelStep === TravelStep.WalkingToCar && iterations < 2000) { + p.update(homeTile, 50, new Set(), pathFinder); + iterations++; + } + + expect(iterations).toBeLessThan(2000); // advanced, not stalled + expect((p as any).travelStep).toBe(TravelStep.EnteringCar); }); test('moves along X first, then Y, when the target is diagonal (matches the constructor default Axis.X)', () => {