From bc9a691bc1db9b55cb01dc0f3459b89d95bb5351 Mon Sep 17 00:00:00 2001 From: Jeroen Schweitzer Date: Wed, 11 Feb 2026 19:32:49 +0100 Subject: [PATCH] fix(client): address PR #4 review feedback MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Hoshe critical fixes: - _action_enum_to_wire uses InputMapper.Action constants instead of fragile integer literals; OPEN_MENU explicitly handled as client-only - Remove int() coercion on tick/entity_id — use direct assignment since GDScript int is signed 64-bit (safe for realistic tick values) - Check encode result before buffering in send_input() — reject empty bytes instead of corrupting the outbound stream - Test snapshot now uses Protocol format {tick, entities} instead of legacy schema; GameState updated to derive player position from entity data; main.gd and world_renderer.gd updated accordingly Hoshe warnings: - 5 negative tests added (truncated bytes, wrong type, missing fields, empty bytes, encode validation) — 20/20 tests pass - receive_bytes signal is emitted at consume time in poll_snapshot by design (documented in code) Tyre suggestions: - Remove duplicated root-level fixtures — single source of truth in client/tests/fixtures/msgpack/ - gen_fixtures.rs writes directly to client/ directory - Add `make fixtures` target for regeneration Co-Authored-By: Claude Opus 4.6 --- Makefile | 3 ++ client/scripts/autoloads/game_state.gd | 33 +++++------- client/scripts/autoloads/sim_bridge.gd | 48 ++++++++--------- client/scripts/main.gd | 10 ---- client/scripts/protocol/protocol.gd | 11 ++-- client/scripts/protocol/protocol.gd.uid | 1 + client/scripts/rendering/world_renderer.gd | 4 +- client/tests/test_protocol.gd | 33 ++++++++++++ client/tests/test_snapshot_parsing.gd | 50 ++++++++---------- server/tests/gen_fixtures.rs | 3 +- .../fixtures/msgpack/input_move_north.msgpack | 1 - .../msgpack/input_perception_mode.msgpack | 1 - tests/fixtures/msgpack/snapshot_empty.msgpack | Bin 17 -> 0 bytes .../msgpack/snapshot_multi_entity.msgpack | Bin 140 -> 0 bytes .../fixtures/msgpack/snapshot_one_npc.msgpack | Bin 55 -> 0 bytes 15 files changed, 109 insertions(+), 89 deletions(-) create mode 100644 client/scripts/protocol/protocol.gd.uid delete mode 100644 tests/fixtures/msgpack/input_move_north.msgpack delete mode 100644 tests/fixtures/msgpack/input_perception_mode.msgpack delete mode 100644 tests/fixtures/msgpack/snapshot_empty.msgpack delete mode 100644 tests/fixtures/msgpack/snapshot_multi_entity.msgpack delete mode 100644 tests/fixtures/msgpack/snapshot_one_npc.msgpack diff --git a/Makefile b/Makefile index 5daf2e895..d799989bc 100644 --- a/Makefile +++ b/Makefile @@ -73,6 +73,9 @@ test: test-server test-client test-server: cd server && cargo nextest run +fixtures: + cd server && cargo test --test gen_fixtures -- --ignored + test-client: @test -n "$(GODOT)" || { echo "Godot not found. Run 'make setup' first."; exit 1; } $(GODOT) --headless --path client -s res://addons/gdUnit4/bin/GdUnitCmdTool.gd --ignoreHeadlessMode -a res://tests/ diff --git a/client/scripts/autoloads/game_state.gd b/client/scripts/autoloads/game_state.gd index badb8800a..ad08a89ad 100644 --- a/client/scripts/autoloads/game_state.gd +++ b/client/scripts/autoloads/game_state.gd @@ -1,31 +1,26 @@ extends Node -# Updated each frame from ObserverSnapshot data +# Updated each frame from ObserverSnapshot data (Protocol format: {tick, entities}). +# Entities use Protocol decoded format: {entity_id, x, y, z, kind: {variant, data}}. var current_snapshot: Dictionary = {} +var current_tick: int = 0 var player_position: Vector2 = Vector2.ZERO var visible_entities: Array = [] -var fog_state: Dictionary = {} -var hud_data: Dictionary = {} + +# Player entity ID — the first entity is assumed to be the player (will be +# refined when the server assigns explicit player entity IDs). +var player_entity_id: int = 1 func apply_snapshot(snapshot: Dictionary) -> void: current_snapshot = snapshot - # Parse player data - if snapshot.has("player") and snapshot.player.has("position"): - var pos = snapshot.player.position - if pos is Array and pos.size() >= 2: - player_position = Vector2(pos[0], pos[1]) - else: - push_warning("GameState: malformed player position in snapshot") + if snapshot.has("tick"): + current_tick = snapshot.tick - # Parse entities if snapshot.has("entities"): visible_entities = snapshot.entities - - # Parse fog state - if snapshot.has("fog"): - fog_state = snapshot.fog - - # Parse HUD data - if snapshot.has("hud"): - hud_data = snapshot.hud + # Derive player position from the player entity + for entity in visible_entities: + if entity.has("entity_id") and entity.entity_id == player_entity_id: + player_position = Vector2(entity.x, entity.y) + break diff --git a/client/scripts/autoloads/sim_bridge.gd b/client/scripts/autoloads/sim_bridge.gd index 7095e919a..0154ac915 100644 --- a/client/scripts/autoloads/sim_bridge.gd +++ b/client/scripts/autoloads/sim_bridge.gd @@ -49,7 +49,11 @@ func send_input(player_input: Dictionary) -> void: if action_name.is_empty(): return var tick: int = player_input.get("timestamp_msec", 0) - _outbound_buffer.append(Protocol.encode_player_input(tick, action_name)) + var encoded := Protocol.encode_player_input(tick, action_name) + if encoded.size() == 0: + push_error("SimBridge: failed to encode player input (action=%s)" % action_name) + return + _outbound_buffer.append(encoded) # Poll for snapshot from simulation. # In test mode returns hardcoded data. In live mode, returns the last decoded snapshot (if any). @@ -83,41 +87,37 @@ func drain_outbound() -> Array[PackedByteArray]: return messages # Map InputMapper.Action enum values to wire-format action names (matching Rust PlayerAction). +# OPEN_MENU is client-only — no Rust equivalent, not sent over the wire. static func _action_enum_to_wire(action: int) -> String: match action: - 0: return "MoveNorth" # InputMapper.Action.MOVE_NORTH - 1: return "MoveSouth" # InputMapper.Action.MOVE_SOUTH - 2: return "MoveEast" # InputMapper.Action.MOVE_EAST - 3: return "MoveWest" # InputMapper.Action.MOVE_WEST - 4: return "Interact" # InputMapper.Action.INTERACT - 5: return "UsePerceptionMode" # InputMapper.Action.USE_PERCEPTION_MODE - 7: return "Pause" # InputMapper.Action.PAUSE + InputMapper.Action.MOVE_NORTH: return "MoveNorth" + InputMapper.Action.MOVE_SOUTH: return "MoveSouth" + InputMapper.Action.MOVE_EAST: return "MoveEast" + InputMapper.Action.MOVE_WEST: return "MoveWest" + InputMapper.Action.INTERACT: return "Interact" + InputMapper.Action.USE_PERCEPTION_MODE: return "UsePerceptionMode" + InputMapper.Action.PAUSE: return "Pause" + InputMapper.Action.OPEN_MENU: + # Client-only action, not part of wire protocol + push_warning("SimBridge: OPEN_MENU is client-only, not sent to server") + return "" _: push_warning("SimBridge: unknown action enum %s" % action) return "" -# Hardcoded test snapshot for development (deterministic per D-010 principle 4) +# Hardcoded test snapshot matching Protocol format (deterministic per D-010 principle 4). +# Uses the same {tick, entities} schema as Protocol.decode_snapshot() returns. func _test_snapshot() -> Dictionary: _test_tick += 1 return { "tick": _test_tick, - "player": { - "position": [10, 10], - "health": 100 - }, "entities": [ { - "id": 1, - "type": "npc", - "position": [12, 8], - "name": "Test NPC" + "entity_id": 1, + "x": 10.0, + "y": 10.0, + "z": 0, + "kind": { "variant": "Npc", "data": null }, }, ], - "fog": { - "radius": 8 - }, - "hud": { - "perception_mode": "baseline", - "time": "08:00" - } } diff --git a/client/scripts/main.gd b/client/scripts/main.gd index 31f803d95..d23871ed1 100644 --- a/client/scripts/main.gd +++ b/client/scripts/main.gd @@ -24,16 +24,6 @@ func _process(_delta: float) -> void: # Track camera to player position (D-015) camera.position = GameState.player_position * 32 # tile-space to pixel-space - # Update HUD - if hud and hud.has_method("update_from_hud_data"): - hud.update_from_hud_data(GameState.hud_data) - if snapshot.has("player") and snapshot.player.has("health"): - hud.update_health(snapshot.player.health) - - # Wire monologue display (D-016 perception data path) - if snapshot.has("monologue") and monologue_display: - monologue_display.show_monologue(snapshot.monologue) - # Send queued input to simulation var inputs = InputMapper.flush_queue() for input in inputs: diff --git a/client/scripts/protocol/protocol.gd b/client/scripts/protocol/protocol.gd index 3f86e733f..8bfdfd8e2 100644 --- a/client/scripts/protocol/protocol.gd +++ b/client/scripts/protocol/protocol.gd @@ -31,8 +31,11 @@ static func decode_snapshot(bytes: PackedByteArray) -> Variant: if entity != null: entities.append(entity) + # GDScript int is signed 64-bit. Rust tick is u64 but will not exceed 2^63 + # in any realistic scenario (would require ~29 billion years at 10 ticks/min). + var tick: int = raw["tick"] return { - "tick": int(raw["tick"]), + "tick": tick, "entities": entities, } @@ -44,8 +47,9 @@ static func _decode_entity(raw: Dictionary) -> Variant: push_warning("Protocol: entity missing required fields: %s" % str(raw.keys())) return null + var entity_id: int = raw["entity_id"] return { - "entity_id": int(raw["entity_id"]), + "entity_id": entity_id, "x": float(raw["x"]), "y": float(raw["y"]), "z": int(raw["z"]), @@ -108,7 +112,8 @@ static func decode_player_input(bytes: PackedByteArray) -> Variant: push_error("Protocol: player_input missing required fields") return null + var tick: int = raw["tick"] return { - "tick": int(raw["tick"]), + "tick": tick, "action": _decode_enum_variant(raw["action"]), } diff --git a/client/scripts/protocol/protocol.gd.uid b/client/scripts/protocol/protocol.gd.uid new file mode 100644 index 000000000..f4c3493eb --- /dev/null +++ b/client/scripts/protocol/protocol.gd.uid @@ -0,0 +1 @@ +uid://fkmd537xvwxe diff --git a/client/scripts/rendering/world_renderer.gd b/client/scripts/rendering/world_renderer.gd index 419090e40..12155ead8 100644 --- a/client/scripts/rendering/world_renderer.gd +++ b/client/scripts/rendering/world_renderer.gd @@ -15,6 +15,6 @@ func update_from_state() -> void: if entity_renderer and entity_renderer.has_method("update_entities"): entity_renderer.update_entities(GameState.visible_entities) - # Update fog overlay + # Update fog overlay (fog data will come in D-020 expansion) if fog_renderer and fog_renderer.has_method("update_fog"): - fog_renderer.update_fog(GameState.fog_state, GameState.player_position) + fog_renderer.update_fog({}, GameState.player_position) diff --git a/client/tests/test_protocol.gd b/client/tests/test_protocol.gd index b9e396b21..1a87fbe5c 100644 --- a/client/tests/test_protocol.gd +++ b/client/tests/test_protocol.gd @@ -135,3 +135,36 @@ func test_gdscript_encode_matches_rust_fixture() -> void: assert_that(from_gd.tick).is_equal(from_rust.tick) assert_that(from_gd.action.variant).is_equal(from_rust.action.variant) assert_that(from_gd.action.data).is_equal(from_rust.action.data) + + +# -- Negative tests: malformed/truncated input ----------------------------------- + +func test_decode_snapshot_truncated_bytes() -> void: + var truncated := PackedByteArray([0x82, 0xa4]) # Incomplete msgpack map + var result = Protocol.decode_snapshot(truncated) + assert_that(result).is_null() + + +func test_decode_snapshot_wrong_type() -> void: + # Encode an array instead of a map — should fail validation + var encoded = Messagepack.encode([1, 2, 3]) + var result = Protocol.decode_snapshot(encoded.value) + assert_that(result).is_null() + + +func test_decode_snapshot_missing_fields() -> void: + # Map with wrong keys + var encoded = Messagepack.encode({"foo": "bar"}) + var result = Protocol.decode_snapshot(encoded.value) + assert_that(result).is_null() + + +func test_decode_player_input_empty_bytes() -> void: + var result = Protocol.decode_player_input(PackedByteArray()) + assert_that(result).is_null() + + +func test_encode_returns_empty_on_failure() -> void: + # Verify that a valid encode produces non-empty bytes + var bytes = Protocol.encode_player_input(1, "MoveNorth") + assert_that(bytes.size()).is_greater(0) diff --git a/client/tests/test_snapshot_parsing.gd b/client/tests/test_snapshot_parsing.gd index dcc5c49d5..af025e14c 100644 --- a/client/tests/test_snapshot_parsing.gd +++ b/client/tests/test_snapshot_parsing.gd @@ -1,30 +1,24 @@ ## D-030 Layer 1: Fixture-based tests for snapshot parsing -## Validates that GameState correctly parses ObserverSnapshot data +## Validates that GameState correctly parses Protocol-format ObserverSnapshot data class_name TestSnapshotParsing extends GdUnitTestSuite -# Valid snapshot fixture +# Valid snapshot in Protocol format (matches Protocol.decode_snapshot output) var _valid_snapshot: Dictionary = { "tick": 1, - "player": { - "position": [10, 15], - "health": 85 - }, "entities": [ - {"id": 1, "type": "npc", "position": [12, 8], "name": "Test NPC"}, - {"id": 2, "type": "npc", "position": [5, 20], "name": "Second NPC"}, + {"entity_id": 1, "x": 10.0, "y": 15.0, "z": 0, "kind": {"variant": "Npc", "data": null}}, + {"entity_id": 2, "x": 5.0, "y": 20.0, "z": 0, "kind": {"variant": "Npc", "data": null}}, ], - "fog": {"radius": 8}, - "hud": {"perception_mode": "baseline", "time": "14:30"} } func test_apply_valid_snapshot() -> void: + GameState.player_entity_id = 1 GameState.apply_snapshot(_valid_snapshot) + assert_that(GameState.current_tick).is_equal(1) assert_that(GameState.player_position).is_equal(Vector2(10, 15)) assert_that(GameState.visible_entities.size()).is_equal(2) - assert_that(GameState.fog_state).is_equal({"radius": 8}) - assert_that(GameState.hud_data).is_equal({"perception_mode": "baseline", "time": "14:30"}) func test_empty_snapshot_no_crash() -> void: # Reset state @@ -37,26 +31,25 @@ func test_empty_snapshot_no_crash() -> void: assert_that(GameState.player_position).is_equal(Vector2.ZERO) assert_that(GameState.visible_entities.size()).is_equal(0) -func test_malformed_position_no_crash() -> void: - var bad_snapshot: Dictionary = { - "player": {"position": "not_an_array"}, - } - # Reset - GameState.player_position = Vector2.ZERO +func test_no_player_entity_position_unchanged() -> void: + GameState.player_entity_id = 999 # No entity with this ID + GameState.player_position = Vector2(5, 5) - # Should not crash — logs warning instead - GameState.apply_snapshot(bad_snapshot) - assert_that(GameState.player_position).is_equal(Vector2.ZERO) + GameState.apply_snapshot(_valid_snapshot) + + # Position stays at previous value since no matching entity + assert_that(GameState.player_position).is_equal(Vector2(5, 5)) func test_missing_fields_partial_update() -> void: # First apply valid snapshot + GameState.player_entity_id = 1 GameState.apply_snapshot(_valid_snapshot) assert_that(GameState.player_position).is_equal(Vector2(10, 15)) - # Apply snapshot with only HUD data — player position unchanged - GameState.apply_snapshot({"hud": {"perception_mode": "thermal", "time": "22:00"}}) + # Apply snapshot with no entities — player position unchanged (no matching entity) + GameState.apply_snapshot({"tick": 2}) assert_that(GameState.player_position).is_equal(Vector2(10, 15)) - assert_that(GameState.hud_data.perception_mode).is_equal("thermal") + assert_that(GameState.current_tick).is_equal(2) func test_sim_bridge_test_snapshot_deterministic() -> void: SimBridge._test_tick = 0 @@ -65,8 +58,9 @@ func test_sim_bridge_test_snapshot_deterministic() -> void: assert_that(snap1.tick).is_equal(1) assert_that(snap2.tick).is_equal(2) - # Snapshot structure is stable - assert_that(snap1.has("player")).is_true() + # Snapshot matches Protocol format + assert_that(snap1.has("tick")).is_true() assert_that(snap1.has("entities")).is_true() - assert_that(snap1.has("fog")).is_true() - assert_that(snap1.has("hud")).is_true() + assert_that(snap1.entities.size()).is_greater(0) + assert_that(snap1.entities[0].has("entity_id")).is_true() + assert_that(snap1.entities[0].has("kind")).is_true() diff --git a/server/tests/gen_fixtures.rs b/server/tests/gen_fixtures.rs index 2f14af3a4..4cd63aff2 100644 --- a/server/tests/gen_fixtures.rs +++ b/server/tests/gen_fixtures.rs @@ -6,7 +6,8 @@ use std::fs; use std::path::Path; fn write_fixture(name: &str, bytes: &[u8]) { - let dir = Path::new("../tests/fixtures/msgpack"); + // Write directly into the Godot project's test fixtures (single source of truth) + let dir = Path::new("../client/tests/fixtures/msgpack"); fs::create_dir_all(dir).expect("create fixture dir"); let path = dir.join(format!("{}.msgpack", name)); fs::write(&path, bytes).expect("write fixture"); diff --git a/tests/fixtures/msgpack/input_move_north.msgpack b/tests/fixtures/msgpack/input_move_north.msgpack deleted file mode 100644 index 937f385e1..000000000 --- a/tests/fixtures/msgpack/input_move_north.msgpack +++ /dev/null @@ -1 +0,0 @@ -tickdactionMoveNorth \ No newline at end of file diff --git a/tests/fixtures/msgpack/input_perception_mode.msgpack b/tests/fixtures/msgpack/input_perception_mode.msgpack deleted file mode 100644 index 3f922c7e3..000000000 --- a/tests/fixtures/msgpack/input_perception_mode.msgpack +++ /dev/null @@ -1 +0,0 @@ -tickȦactionUsePerceptionModethermal \ No newline at end of file diff --git a/tests/fixtures/msgpack/snapshot_empty.msgpack b/tests/fixtures/msgpack/snapshot_empty.msgpack deleted file mode 100644 index 5435b2917bd5e065ec0af149fc763eb6025eb1a4..0000000000000000000000000000000000000000 GIT binary patch literal 0 HcmV?d00001 literal 17 YcmZo#Qj(dR&9EXhuOzc1GqrdE07FCvZvX%Q diff --git a/tests/fixtures/msgpack/snapshot_multi_entity.msgpack b/tests/fixtures/msgpack/snapshot_multi_entity.msgpack deleted file mode 100644 index 2c0217a4547eadf70ccbe33d72e3023431b87d0d..0000000000000000000000000000000000000000 GIT binary patch literal 0 HcmV?d00001 literal 140 zcmZo#Qj(dReU|z8iqyQ4%#zI1;>oQm!OY6|%oN6j6{j2)Ffc5vJmshWq^cN}WM}53 zEcPo%MpDHDROMI!R^{LTQpE^Xwah;$D>b(Ap;NqjRR9v|G^5Eholx2C1&OU E06`QvPXGV_ diff --git a/tests/fixtures/msgpack/snapshot_one_npc.msgpack b/tests/fixtures/msgpack/snapshot_one_npc.msgpack deleted file mode 100644 index f98303206327ef637ef23d4bada61349a76e1511..0000000000000000000000000000000000000000 GIT binary patch literal 0 HcmV?d00001 literal 55 zcmZo#Qj(dRt+gUGuOzc1GqreP>q;=QGCnhfabd+NM+F9kg_Wlq7XYa$h9%jVc`1wi G3X%bojuylK