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

Sprint 1: Game loop, wire format fix, TCP transport #6

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

Summary

  • Wire format fix: rmp_serde::to_vec() to to_vec_named() in LocalBridge and all tests — client Protocol.gd expects named maps, not positional arrays
  • TCP transport: New TcpBridge for Godot client connection (Godot has no Unix socket API). LocalBridge retained for Rust-to-Rust tests
  • Input processing: process_player_input system drains InputQueue, converts PlayerAction variants to MoveIntent components or pause/unpause toggles
  • Snapshot generation: generate_snapshot builds ObserverSnapshot from ECS state with render coordinate conversion
  • Bridge I/O: receive_bridge_inputs / send_bridge_snapshot systems wire the bridge to the ECS pipeline with graceful disconnect detection
  • Game loop: main.rs now accepts TCP connections, runs a proper tick loop with ServerRunning resource, configurable via CLI arg or SR_ADDR env var
  • New resources/types: PlayerCharacter marker, Player EntityKind variant, SnapshotBuffer, ServerRunning

Addresses server side of #81, #82, #83.

Test plan

  • make ci-server green — clippy, fmt, build, nextest (53 tests, 0 failures)
  • Wire format: all serialization tests use to_vec_named
  • TCP bridge: 3 integration tests (snapshot roundtrip, input roundtrip, EOF detection)
  • Input processing: 4 unit tests (move intent, pause toggle, no-player safety, future tick ignored)
  • E2E game loop: client connects via TCP, sends MoveNorth, receives snapshot with updated position
## Summary - **Wire format fix**: rmp_serde::to_vec() to to_vec_named() in LocalBridge and all tests — client Protocol.gd expects named maps, not positional arrays - **TCP transport**: New TcpBridge for Godot client connection (Godot has no Unix socket API). LocalBridge retained for Rust-to-Rust tests - **Input processing**: process_player_input system drains InputQueue, converts PlayerAction variants to MoveIntent components or pause/unpause toggles - **Snapshot generation**: generate_snapshot builds ObserverSnapshot from ECS state with render coordinate conversion - **Bridge I/O**: receive_bridge_inputs / send_bridge_snapshot systems wire the bridge to the ECS pipeline with graceful disconnect detection - **Game loop**: main.rs now accepts TCP connections, runs a proper tick loop with ServerRunning resource, configurable via CLI arg or SR_ADDR env var - **New resources/types**: PlayerCharacter marker, Player EntityKind variant, SnapshotBuffer, ServerRunning Addresses server side of #81, #82, #83. ## Test plan - make ci-server green — clippy, fmt, build, nextest (53 tests, 0 failures) - Wire format: all serialization tests use to_vec_named - TCP bridge: 3 integration tests (snapshot roundtrip, input roundtrip, EOF detection) - Input processing: 4 unit tests (move intent, pause toggle, no-player safety, future tick ignored) - E2E game loop: client connects via TCP, sends MoveNorth, receives snapshot with updated position
jpmschweitzer added 5 commits 2026-02-11 21:02:34 +01:00
fix(bridge): use named MessagePack format for wire compatibility f6ce7d4046 
Client-side Protocol.gd expects rmp_serde::to_vec_named() (maps with
string keys), but LocalBridge was using to_vec() (compact positional
arrays). Fix send_snapshot and update all test serialization calls to
match actual wire format. Also change EOF from Ok(vec![]) to
BridgeError::Transport so bridge systems can detect disconnects.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
feat(bridge): add TCP transport for Godot client connection 1b0e514560 
Godot has no Unix socket API, so TCP localhost is required for
client-server IPC. TcpBridge implements SimBridge with the same
framing protocol as LocalBridge. Includes accept/connect methods
and three integration tests over TCP.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
feat(simulation): add input processing, snapshot gen, and game loop 7864bfdf7d 
Implements the full server-side tick pipeline:
- process_player_input drains InputQueue, converts PlayerActions to
  MoveIntent components or pause/unpause toggles
- generate_snapshot builds ObserverSnapshot from ECS state with
  render coordinate conversion
- receive_bridge_inputs/send_bridge_snapshot handle bridge I/O with
  graceful disconnect detection via ServerRunning resource
- main.rs now accepts TCP connections and runs a proper game loop
- PlayerCharacter marker, Player EntityKind, SnapshotBuffer resource

Closes server side of #81, #82, #83.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
chore(bridge): format gen_fixtures.rs a5b88cb735 
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
chore(meta): update changelog 058d0352ab 
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
jpmschweitzer commented 2026-02-11 21:12:23 +01:00
Author
Owner
Copy Link
Copy Source

Dual-Agent Review: server -> main

Hoshe (Code Quality): REQUEST_CHANGES

Solid implementation with good test coverage (11 new tests including E2E). Two critical issues found.

# File Severity Issue
1 game_loop.rs:54 critical Tick synchronization: snapshot is generated after advance_tick, so snapshot.tick is the NEW tick (1), but the state reflects processing from tick 0. The snapshot for tick N should show the state at the END of tick N. Fix: move generate_snapshot before advance_tick, or label snapshots with previous_tick.
2 bridge_tcp.rs (all 3 tests) critical Race condition: ready signal sent BEFORE accept() completes. Client connects before server is listening. 100ms sleep masks the timing gap. Fix: refactor TcpBridge::accept() to accept an existing TcpListener so the test can bind once, pass the listener in, and the port is guaranteed ready — no sleep needed.
3 bridge/mod.rs:115-119 send_bridge_snapshot warning Send errors logged but don't set ServerRunning=false. Server continues for one tick after send failure. Consider matching receive_bridge_inputs behavior.
4 main.rs:23 warning expect() on accept — panics with no address/error context. Use unwrap_or_else with tracing.
5 bridge/mod.rs:108 receive_bridge_inputs suggestion Disconnect detection via msg.contains("disconnected") is string-matching. Consider a BridgeError::Disconnected variant for cleaner matching.
6 bridge/mod.rs generate_snapshot suggestion No tracing for snapshot entity count — hard to debug empty/missing entities from logs.

Tyre (Architecture): APPROVE

Production-quality server architecture. Clean separation of concerns, thorough three-layer testing (D-030), explicit system ordering, deterministic input processing (D-010 principle 4). Full D-020, D-010, D-030, D-012 compliance verified. No architectural issues.

# File Severity Issue
1 tcp.rs Mutex expect suggestion Acceptable for prototype; consider graceful degradation later.
2 bridge/mod.rs:108 suggestion String-matching for disconnect; consider explicit error variant.

Verdict: CHANGES REQUESTED

Fix 2 critical issues before merge: (1) tick synchronization semantics in snapshot generation, (2) test race condition — refactor TcpBridge::accept() to take an existing TcpListener instead of binding its own, eliminating the 100ms sleep hack.

## Dual-Agent Review: server -> main ### Hoshe (Code Quality): REQUEST_CHANGES Solid implementation with good test coverage (11 new tests including E2E). Two critical issues found. | # | File | Severity | Issue | |---|------|----------|-------| | 1 | `game_loop.rs:54` | critical | Tick synchronization: snapshot is generated after `advance_tick`, so `snapshot.tick` is the NEW tick (1), but the state reflects processing from tick 0. The snapshot for tick N should show the state at the END of tick N. Fix: move `generate_snapshot` before `advance_tick`, or label snapshots with `previous_tick`. | | 2 | `bridge_tcp.rs` (all 3 tests) | critical | Race condition: ready signal sent BEFORE `accept()` completes. Client connects before server is listening. 100ms sleep masks the timing gap. Fix: refactor `TcpBridge::accept()` to accept an existing `TcpListener` so the test can bind once, pass the listener in, and the port is guaranteed ready — no sleep needed. | | 3 | `bridge/mod.rs:115-119` send_bridge_snapshot | warning | Send errors logged but don't set `ServerRunning=false`. Server continues for one tick after send failure. Consider matching receive_bridge_inputs behavior. | | 4 | `main.rs:23` | warning | `expect()` on accept — panics with no address/error context. Use `unwrap_or_else` with tracing. | | 5 | `bridge/mod.rs:108` receive_bridge_inputs | suggestion | Disconnect detection via `msg.contains("disconnected")` is string-matching. Consider a `BridgeError::Disconnected` variant for cleaner matching. | | 6 | `bridge/mod.rs` generate_snapshot | suggestion | No tracing for snapshot entity count — hard to debug empty/missing entities from logs. | ### Tyre (Architecture): APPROVE Production-quality server architecture. Clean separation of concerns, thorough three-layer testing (D-030), explicit system ordering, deterministic input processing (D-010 principle 4). Full D-020, D-010, D-030, D-012 compliance verified. No architectural issues. | # | File | Severity | Issue | |---|------|----------|-------| | 1 | `tcp.rs` Mutex expect | suggestion | Acceptable for prototype; consider graceful degradation later. | | 2 | `bridge/mod.rs:108` | suggestion | String-matching for disconnect; consider explicit error variant. | ### Verdict: CHANGES REQUESTED Fix 2 critical issues before merge: (1) tick synchronization semantics in snapshot generation, (2) test race condition — refactor `TcpBridge::accept()` to take an existing `TcpListener` instead of binding its own, eliminating the 100ms sleep hack.
jpmschweitzer added 3 commits 2026-02-11 21:16:12 +01:00
fix(bridge): add Disconnected error variant, fix test race condition 2cd8786036 
Replace string-matching disconnect detection with explicit
BridgeError::Disconnected variant. Add TcpBridge::accept_on(listener)
that takes a pre-bound TcpListener, eliminating the 100ms sleep hack
in TCP tests. Send errors now also trigger ServerRunning=false.
Add trace logging to generate_snapshot for entity count visibility.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
fix(simulation): correct tick synchronization in snapshot generation b9af725b02 
Snapshot for tick N should show state at END of tick N. Reorder systems
so generate_snapshot runs after validate_movement but before
advance_tick. Previously snapshot.tick was the incremented tick,
not the tick whose inputs were processed. Also fix main.rs accept
error to log address context before exiting.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
fix(bridge): improve accept error reporting with address context 3f4c663242 
Replace expect() with unwrap_or_else that logs the bind address
and error via tracing before exiting.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
jpmschweitzer closed this pull request 2026-02-11 21:26:52 +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#6