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

feat(simulation): Sprint 1 server — LocalBridge IPC + tile collision #3

Closed
jpmschweitzer wants to merge 0 commits from server into main
pull from: server
merge into: :main
Conversation 3 Commits - Files Changed -
jpmschweitzer commented 2026-02-11 19:08:21 +01:00
Owner
Copy Link
Copy Source

Summary

  • LocalBridge IPC over Unix domain sockets (#78) — length-prefixed MessagePack framing, SimBridge trait implementation, BridgeResource ECS wrapper
  • Tile collision system (#236) — TilePosition component, flat-storage WalkabilityMap with O(1) can_move_to(), MoveIntent + validate_movement system
  • 38 tests total (20 existing + 18 new), make ci-server green

Test plan

  • cargo nextest run — 38 tests passing
  • make ci-server — clippy, fmt, build, nextest all green
  • IPC integration: snapshot + input round-trips over real Unix sockets
  • Movement integration: full App with blocked/allowed tiles in one tick

Generated with Claude Code

## Summary - LocalBridge IPC over Unix domain sockets (#78) — length-prefixed MessagePack framing, SimBridge trait implementation, BridgeResource ECS wrapper - Tile collision system (#236) — TilePosition component, flat-storage WalkabilityMap with O(1) can_move_to(), MoveIntent + validate_movement system - 38 tests total (20 existing + 18 new), make ci-server green ## Test plan - [x] cargo nextest run — 38 tests passing - [x] make ci-server — clippy, fmt, build, nextest all green - [x] IPC integration: snapshot + input round-trips over real Unix sockets - [x] Movement integration: full App with blocked/allowed tiles in one tick Generated with [Claude Code](https://claude.com/claude-code)
jpmschweitzer added 3 commits 2026-02-11 19:08:22 +01:00
feat(simulation): add LocalBridge IPC over Unix socket (#78) 4ed15c1a38 
Length-prefixed MessagePack framing (4-byte BE length + payload),
LocalBridge struct implementing SimBridge trait over Unix domain
sockets, BridgeResource wrapper for ECS integration. Adds Io
error variant to BridgeError. Two integration tests verify
snapshot and input round-trips over real sockets.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
feat(simulation): add tile collision system (#236) 862ab9099f 
TilePosition component with discrete grid coordinates, flat-storage
WalkabilityMap resource with O(1) can_move_to() lookup, MoveIntent
component and validate_movement system. Movement validated against
walkability map each tick, blocking all NPC and player movement
through unwalkable tiles. 11 unit tests + 1 integration test.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
chore(meta): update changelog e44e90c2cb 
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
jpmschweitzer commented 2026-02-11 19:26:06 +01:00
Author
Owner
Copy Link
Copy Source

Dual-Agent Review: origin/server → main (PR #3)

Hoshe (Code Quality): APPROVE

All 38 tests pass. Clean error handling, comprehensive three-layer test coverage per D-030. Code quality is high.

# File Severity Issue
1 bridge/local.rs:94,110 suggestion .lock().unwrap() on Mutexes — use .expect("mutex poisoned") for clarity
2 simulation/movement.rs:157 warning No WalkabilityMap silently rejects all intents — should tracing::warn! not just trace
3 tests/bridge_ipc.rs suggestion Socket cleanup is best-effort — tests panicking may leak /tmp sockets

Tyre (Architecture): REQUEST_CHANGES

# File Severity Issue
1 simulation/movement.rs critical WalkabilityMap uses flat global indexing — incompatible with D-012 chunk architecture. Needs HashMap<ChunkCoord, ChunkData> or similar chunk-aware storage
2 bridge/types.rs + movement.rs critical Coordinate mismatch: VisibleEntity uses f32 x/y, TilePosition uses i32 x/y — no conversion layer, contract undocumented
3 simulation/movement.rs warning cardinal_neighbors() doesn't handle chunk boundary crossing
4 simulation/movement.rs warning No entity-entity collision — multiple entities can occupy same tile
5 bridge/framing.rs:8 warning 16MB MAX_MESSAGE_SIZE undocumented — no rationale or budget calculation
6 simulation/movement.rs suggestion No diagonal movement — 4-directional only, genre expects 8

Verdict: CHANGES REQUESTED

Tyre's D-012 chunk architecture concern is significant — flat WalkabilityMap indexing can't support chunk load/unload without rewrite. Coordinate system mismatch (f32 vs i32) needs resolution before IPC integration.

## Dual-Agent Review: origin/server → main (PR #3) ### Hoshe (Code Quality): APPROVE All 38 tests pass. Clean error handling, comprehensive three-layer test coverage per D-030. Code quality is high. | # | File | Severity | Issue | |---|------|----------|-------| | 1 | bridge/local.rs:94,110 | suggestion | `.lock().unwrap()` on Mutexes — use `.expect("mutex poisoned")` for clarity | | 2 | simulation/movement.rs:157 | warning | No WalkabilityMap silently rejects all intents — should `tracing::warn!` not just trace | | 3 | tests/bridge_ipc.rs | suggestion | Socket cleanup is best-effort — tests panicking may leak /tmp sockets | ### Tyre (Architecture): REQUEST_CHANGES | # | File | Severity | Issue | |---|------|----------|-------| | 1 | simulation/movement.rs | critical | WalkabilityMap uses flat global indexing — incompatible with D-012 chunk architecture. Needs HashMap<ChunkCoord, ChunkData> or similar chunk-aware storage | | 2 | bridge/types.rs + movement.rs | critical | Coordinate mismatch: VisibleEntity uses f32 x/y, TilePosition uses i32 x/y — no conversion layer, contract undocumented | | 3 | simulation/movement.rs | warning | cardinal_neighbors() doesn't handle chunk boundary crossing | | 4 | simulation/movement.rs | warning | No entity-entity collision — multiple entities can occupy same tile | | 5 | bridge/framing.rs:8 | warning | 16MB MAX_MESSAGE_SIZE undocumented — no rationale or budget calculation | | 6 | simulation/movement.rs | suggestion | No diagonal movement — 4-directional only, genre expects 8 | ### Verdict: CHANGES REQUESTED Tyre's D-012 chunk architecture concern is significant — flat WalkabilityMap indexing can't support chunk load/unload without rewrite. Coordinate system mismatch (f32 vs i32) needs resolution before IPC integration.
jpmschweitzer added 3 commits 2026-02-11 19:33:01 +01:00
refactor(simulation): chunk-based walkability map per D-012 78e0c71c6d 
Rewrites WalkabilityMap from flat Vec<bool> to HashMap<ChunkCoord, ChunkData>
with 32x32 tile chunks. Supports chunk load/unload for future borderless
generation. Unloaded chunks treated as unwalkable.

Adds TilePosition ↔ f32 render coordinate conversion (to_render_coords,
from_render_coords) bridging i32 simulation coords and f32 wire format.

Addresses Tyre PR review: D-012 chunk architecture compatibility and
VisibleEntity coordinate mismatch.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
fix(simulation): address bridge review feedback 9860fc0857 
Replace .lock().unwrap() with .expect("mutex poisoned") in LocalBridge
for clearer panic messages. Document 16MB MAX_MESSAGE_SIZE rationale
in framing.rs with entity count sizing analysis.

Addresses Hoshe PR review suggestions.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
chore(meta): update changelog 0f5f73a927 
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
jpmschweitzer commented 2026-02-11 20:11:25 +01:00
Author
Owner
Copy Link
Copy Source

Dual-Agent Review Round 2: origin/server → main (PR #3)

Round 1 Issue Resolution: ALL FIXED

Issue Status
Flat WalkabilityMap (CRITICAL) FIXED — chunk-based HashMap per D-012
f32/i32 coordinate mismatch (CRITICAL) FIXED — to_render_coords/from_render_coords
cardinal_neighbors() chunk boundary DOCUMENTED as TODO
MAX_MESSAGE_SIZE undocumented FIXED — sizing rationale added

Hoshe (Code Quality): REQUEST_CHANGES

Round 1 architectural feedback well addressed. Chunk-based refactor is clean with 16 unit + 2 integration tests. Two movement system gaps remain.

# File Severity Issue
1 movement.rs critical Entity-entity collision not implemented — validate_movement() only checks WalkabilityMap. Multiple entities can occupy same tile. Add spatial query (check existing TilePositions at target) before allowing move. The spatial patterns needed for collision ARE the patterns needed for performant entity queries — building movement without collision means building it twice.
2 movement.rs critical No diagonal movement — 4-directional only. Add 4 diagonal PlayerAction variants (MoveNortheast, etc.), update cardinal_neighbors() to all_neighbors() returning 8 positions. Consider diagonal cost (√2 vs 1) for future pathfinding.
3 movement.rs:41-42 suggestion Chunk boundary TODO comment is misleading — cardinal_neighbors() already works across chunks via independent chunk_coord() calculation. Can remove or clarify.

Tyre (Architecture): REQUEST_CHANGES

Revised from round 2 APPROVE after team lead review. D-012 chunk architecture and coordinate conversion are solid. Entity collision is architectural foundation, not feature scope — deferring it creates cascading rewrites when pathfinding and AI need occupied-tile awareness.

# File Severity Issue
1 movement.rs critical Entity-entity collision is architectural: spatial indexing needed for collision is the same pattern needed for D-026 simulation tiers (30-80 active NPCs). Without it, validate_movement + pathfinding + AI behavior all need rewriting later.
2 movement.rs critical 8-directional movement: genre-expected for immersive sim. Additive change but touches movement validation, input enum, and client input mapping. Better to establish the pattern now than retrofit diagonal cost calculations into existing 4-dir pathfinding.

Verdict: CHANGES REQUESTED

Both reviewers agree: entity-entity collision and 8-directional movement should be in before merge. These are foundational movement patterns — skipping them has a different cost than not shipping a feature.

## Dual-Agent Review Round 2: origin/server → main (PR #3) ### Round 1 Issue Resolution: ALL FIXED | Issue | Status | |-------|--------| | Flat WalkabilityMap (CRITICAL) | FIXED — chunk-based HashMap per D-012 | | f32/i32 coordinate mismatch (CRITICAL) | FIXED — to_render_coords/from_render_coords | | cardinal_neighbors() chunk boundary | DOCUMENTED as TODO | | MAX_MESSAGE_SIZE undocumented | FIXED — sizing rationale added | ### Hoshe (Code Quality): REQUEST_CHANGES Round 1 architectural feedback well addressed. Chunk-based refactor is clean with 16 unit + 2 integration tests. Two movement system gaps remain. | # | File | Severity | Issue | |---|------|----------|-------| | 1 | movement.rs | critical | Entity-entity collision not implemented — validate_movement() only checks WalkabilityMap. Multiple entities can occupy same tile. Add spatial query (check existing TilePositions at target) before allowing move. The spatial patterns needed for collision ARE the patterns needed for performant entity queries — building movement without collision means building it twice. | | 2 | movement.rs | critical | No diagonal movement — 4-directional only. Add 4 diagonal PlayerAction variants (MoveNortheast, etc.), update cardinal_neighbors() to all_neighbors() returning 8 positions. Consider diagonal cost (√2 vs 1) for future pathfinding. | | 3 | movement.rs:41-42 | suggestion | Chunk boundary TODO comment is misleading — cardinal_neighbors() already works across chunks via independent chunk_coord() calculation. Can remove or clarify. | ### Tyre (Architecture): REQUEST_CHANGES Revised from round 2 APPROVE after team lead review. D-012 chunk architecture and coordinate conversion are solid. Entity collision is architectural foundation, not feature scope — deferring it creates cascading rewrites when pathfinding and AI need occupied-tile awareness. | # | File | Severity | Issue | |---|------|----------|-------| | 1 | movement.rs | critical | Entity-entity collision is architectural: spatial indexing needed for collision is the same pattern needed for D-026 simulation tiers (30-80 active NPCs). Without it, validate_movement + pathfinding + AI behavior all need rewriting later. | | 2 | movement.rs | critical | 8-directional movement: genre-expected for immersive sim. Additive change but touches movement validation, input enum, and client input mapping. Better to establish the pattern now than retrofit diagonal cost calculations into existing 4-dir pathfinding. | ### Verdict: CHANGES REQUESTED Both reviewers agree: entity-entity collision and 8-directional movement should be in before merge. These are foundational movement patterns — skipping them has a different cost than not shipping a feature.
jpmschweitzer added 3 commits 2026-02-11 20:15:52 +01:00
feat(simulation): add 8-directional movement bef4383196 
Add diagonal PlayerAction variants (MoveNortheast, MoveNorthwest,
MoveSoutheast, MoveSouthwest) and TilePosition::all_neighbors()
returning all 8 surrounding tiles. Genre-expected for immersive sim.

Establishes the movement pattern before pathfinding is built on top.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
feat(simulation): add entity-entity collision f87bd2b0e5 
validate_movement now checks both terrain walkability AND tile
occupancy. Builds a spatial index of occupied tiles from stationary
entities, then resolves movers in order — first valid claim wins.

Same spatial pattern needed for D-026 simulation tiers (30-80 active
NPCs) and future pathfinding occupied-tile awareness.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
chore(meta): update changelog 90edf3fabc 
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
jpmschweitzer commented 2026-02-11 20:25:48 +01:00
Author
Owner
Copy Link
Copy Source

Dual-Agent Review Round 3: origin/server → main (PR #3)

Hoshe (Code Quality): APPROVE

Both requested changes verified. Entity-entity collision via HashMap<TilePosition, Entity> spatial tracking with first-claim-wins resolution. 8-directional movement via all_neighbors() and 4 diagonal PlayerAction variants. 3 new tests: all_neighbors, blocks_occupied_tile, two_movers_first_wins. 17 tests total, all passing.

Tyre (Architecture): APPROVE

D-010 determinism satisfied (first-claim-wins via stable query order). D-012 chunk architecture intact. HashMap rebuild at 10 tps × 30-80 Active NPCs is trivial. Diagonal wire format clean — input processing system (PlayerAction → MoveIntent) lands in future ticket. Ship it.

Verdict: APPROVED

## Dual-Agent Review Round 3: origin/server → main (PR #3) ### Hoshe (Code Quality): APPROVE Both requested changes verified. Entity-entity collision via HashMap<TilePosition, Entity> spatial tracking with first-claim-wins resolution. 8-directional movement via all_neighbors() and 4 diagonal PlayerAction variants. 3 new tests: all_neighbors, blocks_occupied_tile, two_movers_first_wins. 17 tests total, all passing. ### Tyre (Architecture): APPROVE D-010 determinism satisfied (first-claim-wins via stable query order). D-012 chunk architecture intact. HashMap rebuild at 10 tps × 30-80 Active NPCs is trivial. Diagonal wire format clean — input processing system (PlayerAction → MoveIntent) lands in future ticket. Ship it. ### Verdict: APPROVED
jpmschweitzer closed this pull request 2026-02-11 20:26:26 +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#3