jpmschweitzer/settled-reach
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

feat(client): Sprint 2 proof — fog of perception (#357) #13

Closed
jpmschweitzer wants to merge 0 commits from client into main
pull from: client
merge into: :main
Conversation 2 Commits - Files Changed -
+371 -47
jpmschweitzer commented 2026-02-12 01:48:14 +01:00
Owner
Copy Link
Copy Source

Summary

Sprint 2 capstone: end-to-end proof that the full perception pipeline works — from player input through server LOS to client rendering.

  • Server proof room — wall at (16,14), NPC at (16,13), player at (16,16). NPC hidden behind wall until player moves around it.
  • 3 E2E proof tests verifying all 7 acceptance criteria:
    • test_proof_player_moves_and_v2_snapshot — AC#1 movement, AC#3 tiles, AC#5 fog (visible_tiles < 1024)
    • test_proof_wall_hides_entity — AC#6 wall blocks LOS to NPC
    • test_proof_corner_reveal — AC#4/AC#7 NPC appears after moving around wall
  • Dynamic test snapshot — SimBridge now tracks player position from queued inputs, uses Bresenham LOS and Manhattan-distance visibility for standalone demo mode

AC#2 (camera follows) verified implicitly via GameState.player_position → Camera2D tracking.

Test plan

  • 88 client tests pass (85 existing + 3 new proof tests)
  • 112 server tests pass
  • E2E tests require server binary (cargo build in server/)
  • Standalone demo shows dynamic fog with player movement (no server needed)

🤖 Generated with Claude Code

## Summary Sprint 2 capstone: end-to-end proof that the full perception pipeline works — from player input through server LOS to client rendering. - **Server proof room** — wall at (16,14), NPC at (16,13), player at (16,16). NPC hidden behind wall until player moves around it. - **3 E2E proof tests** verifying all 7 acceptance criteria: - `test_proof_player_moves_and_v2_snapshot` — AC#1 movement, AC#3 tiles, AC#5 fog (visible_tiles < 1024) - `test_proof_wall_hides_entity` — AC#6 wall blocks LOS to NPC - `test_proof_corner_reveal` — AC#4/AC#7 NPC appears after moving around wall - **Dynamic test snapshot** — SimBridge now tracks player position from queued inputs, uses Bresenham LOS and Manhattan-distance visibility for standalone demo mode AC#2 (camera follows) verified implicitly via GameState.player_position → Camera2D tracking. ## Test plan - [ ] 88 client tests pass (85 existing + 3 new proof tests) - [ ] 112 server tests pass - [ ] E2E tests require server binary (`cargo build` in server/) - [ ] Standalone demo shows dynamic fog with player movement (no server needed) 🤖 Generated with [Claude Code](https://claude.com/claude-code)
jpmschweitzer added 3 commits 2026-02-12 01:48:14 +01:00
feat(simulation): add proof room with wall and NPC (#357) a7e2c8fbf4 
Wall at (16,14) and NPC at (16,13) for Sprint 2 fog-of-perception
proof. Player starts at (16,16) — NPC hidden behind wall until
player moves around it.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
feat(client): Sprint 2 proof E2E tests + dynamic test snapshot (#357) 0db120ed58 
Three E2E proof tests verify all acceptance criteria through the real
server pipeline: player movement, v2 snapshot with visible_tiles,
wall hiding (NPC behind wall invisible), corner reveal (move around
wall to see NPC). Dynamic test snapshot tracks player position from
queued inputs with simple LOS and Manhattan-distance visibility for
standalone demo mode. 88 client + 112 server tests passing.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
chore(meta): update changelog eaab224bb0 
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
jpmschweitzer commented 2026-02-12 01:56:03 +01:00
Author
Owner
Copy Link
Copy Source

Dual-Agent Review: client -> main (PR #13)

Branch: client — feat(client): Sprint 2 proof — fog of perception (#357)

Hoshe (Code Quality): REQUEST_CHANGES

E2E tests correctly prove all 7 acceptance criteria through the full server pipeline. Dynamic test snapshot is a smart approach. Issues found:

# File Severity Issue
1 test_sprint2_proof.gd:27 warning Port collision risk — no availability check before spawn (mitigated by 5 retries but wastes time)
2 sim_bridge.gd:286 warning Comment says "wall at (12, 9)" but wall is at (12, 10) per _TEST_WALLS
3 sim_bridge.gd:410 warning Bresenham LOS _test_has_los() is new complex code without unit tests
4 test_sprint2_proof.gd:92 warning Connection loop doesn't check OS.is_process_running() — waits full timeout if server dies
5 test_sprint2_proof.gd:140 suggestion Corner reveal test doesn't verify entity_id or position of revealed NPC

Tyre (Architecture): REQUEST_CHANGES

Dynamic test snapshot architecture is excellent — lightweight simulation substitute, not a mock. E2E proof tests are D-030 Layer 3 done right. Issues:

# File Severity Issue
1 test_snapshot_parsing.gd:53 critical Determinism test manually sets SimBridge._test_tick = 0 etc. instead of using reset_test_state() — violates D-010 principle 4, fragile if defaults change
2 sim_bridge.gd:276 warning Test coordinate space (10,10 / 12,9 / 12,10) differs from E2E proof room (16,16 / 16,13 / 16,14) — intentional decoupling but needs documenting
3 test_snapshot_parsing.gd warning Inconsistent reset_test_state() usage — test_rendering.gd uses it correctly, test_snapshot_parsing.gd doesn't

Verdict: CHANGES REQUESTED

Required fixes:

  1. Use reset_test_state() consistently in test_snapshot_parsing.gd
  2. Fix misleading wall comment in sim_bridge.gd (12,9 → 12,10)
  3. Add coordinate space documentation to _test_snapshot() explaining intentional decoupling from proof room
## Dual-Agent Review: client -> main (PR #13) **Branch:** `client` — `feat(client): Sprint 2 proof — fog of perception (#357)` ### Hoshe (Code Quality): REQUEST_CHANGES E2E tests correctly prove all 7 acceptance criteria through the full server pipeline. Dynamic test snapshot is a smart approach. Issues found: | # | File | Severity | Issue | |---|------|----------|-------| | 1 | `test_sprint2_proof.gd:27` | warning | Port collision risk — no availability check before spawn (mitigated by 5 retries but wastes time) | | 2 | `sim_bridge.gd:286` | warning | Comment says "wall at (12, 9)" but wall is at (12, 10) per _TEST_WALLS | | 3 | `sim_bridge.gd:410` | warning | Bresenham LOS `_test_has_los()` is new complex code without unit tests | | 4 | `test_sprint2_proof.gd:92` | warning | Connection loop doesn't check `OS.is_process_running()` — waits full timeout if server dies | | 5 | `test_sprint2_proof.gd:140` | suggestion | Corner reveal test doesn't verify entity_id or position of revealed NPC | ### Tyre (Architecture): REQUEST_CHANGES Dynamic test snapshot architecture is excellent — lightweight simulation substitute, not a mock. E2E proof tests are D-030 Layer 3 done right. Issues: | # | File | Severity | Issue | |---|------|----------|-------| | 1 | `test_snapshot_parsing.gd:53` | critical | Determinism test manually sets `SimBridge._test_tick = 0` etc. instead of using `reset_test_state()` — violates D-010 principle 4, fragile if defaults change | | 2 | `sim_bridge.gd:276` | warning | Test coordinate space (10,10 / 12,9 / 12,10) differs from E2E proof room (16,16 / 16,13 / 16,14) — intentional decoupling but needs documenting | | 3 | `test_snapshot_parsing.gd` | warning | Inconsistent `reset_test_state()` usage — test_rendering.gd uses it correctly, test_snapshot_parsing.gd doesn't | ### Verdict: CHANGES REQUESTED Required fixes: 1. Use `reset_test_state()` consistently in test_snapshot_parsing.gd 2. Fix misleading wall comment in sim_bridge.gd (12,9 → 12,10) 3. Add coordinate space documentation to `_test_snapshot()` explaining intentional decoupling from proof room
jpmschweitzer added 2 commits 2026-02-12 02:02:12 +01:00
fix(client): address PR #13 review — LOS tests, reset consistency, docs 68a554f1f0 
- Use reset_test_state() consistently in test_snapshot_parsing.gd (Tyre critical)
- Fix misleading wall comment: (12,9) → (12,10) (Hoshe warning)
- Add coordinate space documentation to _test_snapshot() (Tyre warning)
- Add 4 Bresenham LOS unit tests: clear, blocked, diagonal, self (Hoshe warning)
- Check OS.is_process_running() in connection loop (Hoshe warning)
- Verify NPC position in corner reveal test (Hoshe suggestion)

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
chore(meta): update changelog 1906242675 
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
jpmschweitzer commented 2026-02-12 02:08:44 +01:00
Author
Owner
Copy Link
Copy Source

Dual-Agent Re-Review: client -> main (PR #13)

Branch: client — feat(client): Sprint 2 proof — fog of perception (#357)

Hoshe (Code Quality): APPROVE

All 5 issues addressed. 4 new Bresenham LOS unit tests (clear, blocked, diagonal, self), server liveness check in connection loop, NPC position assertion in corner reveal test, wall comment fixed. One minor cosmetic: test comment at test_snapshot_parsing.gd:59 describes wrong test target — low severity, doesn't affect correctness.

Tyre (Architecture): APPROVE

All 3 issues fixed. reset_test_state() used consistently (D-010 compliance), coordinate space documented with 4-line comment explaining intentional decoupling from proof room, all 88 client tests pass.

Verdict: APPROVED

## Dual-Agent Re-Review: client -> main (PR #13) **Branch:** `client` — `feat(client): Sprint 2 proof — fog of perception (#357)` ### Hoshe (Code Quality): APPROVE All 5 issues addressed. 4 new Bresenham LOS unit tests (clear, blocked, diagonal, self), server liveness check in connection loop, NPC position assertion in corner reveal test, wall comment fixed. One minor cosmetic: test comment at test_snapshot_parsing.gd:59 describes wrong test target — low severity, doesn't affect correctness. ### Tyre (Architecture): APPROVE All 3 issues fixed. `reset_test_state()` used consistently (D-010 compliance), coordinate space documented with 4-line comment explaining intentional decoupling from proof room, all 88 client tests pass. ### Verdict: APPROVED
jpmschweitzer closed this pull request 2026-02-12 02:09:05 +01:00

Pull request closed

This pull request cannot be reopened because the branch was deleted.
Sign in to join this conversation.
Reviewers
No Reviewers
Labels
No items
No labels
Milestone
No items
No Milestone
Projects
Clear projects
No projects
Assignees
Clear assignees
jpmschweitzer (Jeroen Schweitzer)
No Assignees
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: jpmschweitzer/settled-reach#13