From 336823139634a78af71fb709609ac6347764810f Mon Sep 17 00:00:00 2001 From: Jeroen Schweitzer Date: Wed, 11 Feb 2026 17:53:24 +0100 Subject: [PATCH] =?UTF-8?q?fix(client):=20address=20PR=20review=20?= =?UTF-8?q?=E2=80=94=20input=20bug,=20bounds=20checks,=20fixture=20tests?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Fix input_mapper double-check bug (redundant InputEventKey + pressed filter) - Add bounds checking for position arrays in entity_renderer and game_state - Make test snapshot deterministic (incrementing counter, not wall clock) - Fix tween overlap in monologue_display (cancel active tween before new one) - Extract TILE_SIZE constant from magic number 32 - Add 5 D-030 Layer 1 fixture tests for snapshot parsing (7/7 total passing) Co-Authored-By: Claude Opus 4.6 --- client/project.godot | 16 +++-- client/scripts/autoloads/game_state.gd | 5 +- client/scripts/autoloads/input_mapper.gd | 16 +---- client/scripts/autoloads/sim_bridge.gd | 6 +- client/scripts/rendering/entity_renderer.gd | 7 +- client/tests/test_snapshot_parsing.gd | 72 +++++++++++++++++++++ client/ui/monologue_display.gd | 15 +++-- 7 files changed, 106 insertions(+), 31 deletions(-) create mode 100644 client/tests/test_snapshot_parsing.gd diff --git a/client/project.godot b/client/project.godot index f13a795fd..74765696f 100644 --- a/client/project.godot +++ b/client/project.godot @@ -1,7 +1,10 @@ ; Engine configuration file. ; It's best edited using the editor UI and not directly, -; but you can still edit the file manually. -; This file is specific to Godot 4.x format. +; since the parameters that go here are not all obvious. +; +; Format: +; [section] ; section goes between [] +; param=value ; assign values to parameters config_version=5 @@ -22,9 +25,12 @@ InputMapper="*res://scripts/autoloads/input_mapper.gd" window/size/viewport_width=1920 window/size/viewport_height=1080 -window/size/resizable=true window/stretch/mode="canvas_items" +[editor_plugins] + +enabled=PackedStringArray("res://addons/gdUnit4/plugin.cfg") + [input] move_north={ @@ -72,10 +78,6 @@ pause={ ] } -[editor_plugins] - -enabled=PackedStringArray("res://addons/gdUnit4/plugin.cfg") - [rendering] renderer/rendering_method="gl_compatibility" diff --git a/client/scripts/autoloads/game_state.gd b/client/scripts/autoloads/game_state.gd index a54ab0cbd..badb8800a 100644 --- a/client/scripts/autoloads/game_state.gd +++ b/client/scripts/autoloads/game_state.gd @@ -13,7 +13,10 @@ func apply_snapshot(snapshot: Dictionary) -> void: # Parse player data if snapshot.has("player") and snapshot.player.has("position"): var pos = snapshot.player.position - player_position = Vector2(pos[0], pos[1]) + if pos is Array and pos.size() >= 2: + player_position = Vector2(pos[0], pos[1]) + else: + push_warning("GameState: malformed player position in snapshot") # Parse entities if snapshot.has("entities"): diff --git a/client/scripts/autoloads/input_mapper.gd b/client/scripts/autoloads/input_mapper.gd index 79bf2e931..d53412797 100644 --- a/client/scripts/autoloads/input_mapper.gd +++ b/client/scripts/autoloads/input_mapper.gd @@ -9,40 +9,26 @@ enum Action { var input_queue: Array[Dictionary] = [] func _unhandled_input(event: InputEvent) -> void: - # Only process key press events (not releases or repeats) - if not event is InputEventKey: - return - if not event.pressed or event.echo: - return - var action: Action = -1 - var action_name: String = "" # Map input actions to semantic Action enum + # is_action_pressed handles press detection for all input types (key, gamepad, etc.) if event.is_action_pressed("move_north"): action = Action.MOVE_NORTH - action_name = "move_north" elif event.is_action_pressed("move_south"): action = Action.MOVE_SOUTH - action_name = "move_south" elif event.is_action_pressed("move_east"): action = Action.MOVE_EAST - action_name = "move_east" elif event.is_action_pressed("move_west"): action = Action.MOVE_WEST - action_name = "move_west" elif event.is_action_pressed("interact"): action = Action.INTERACT - action_name = "interact" elif event.is_action_pressed("perception_mode"): action = Action.USE_PERCEPTION_MODE - action_name = "perception_mode" elif event.is_action_pressed("open_menu"): action = Action.OPEN_MENU - action_name = "open_menu" elif event.is_action_pressed("pause"): action = Action.PAUSE - action_name = "pause" # Queue the action if valid if action != -1: diff --git a/client/scripts/autoloads/sim_bridge.gd b/client/scripts/autoloads/sim_bridge.gd index 66c032db7..d3dca48f5 100644 --- a/client/scripts/autoloads/sim_bridge.gd +++ b/client/scripts/autoloads/sim_bridge.gd @@ -5,6 +5,7 @@ enum ConnectionState { DISCONNECTED, CONNECTING, CONNECTED, ERROR } var state: ConnectionState = ConnectionState.DISCONNECTED var test_mode: bool = true # Enable test mode for development without Rust server +var _test_tick: int = 0 # Signals signal connection_state_changed(old_state: ConnectionState, new_state: ConnectionState) @@ -54,10 +55,11 @@ func poll_snapshot() -> Variant: # TODO: Actual polling logic when IPC/MessagePack is implemented return null -# Hardcoded test snapshot for development +# Hardcoded test snapshot for development (deterministic per D-010 principle 4) func _test_snapshot() -> Dictionary: + _test_tick += 1 return { - "tick": Time.get_ticks_msec() / 100, # Increment over time for testing + "tick": _test_tick, "player": { "position": [10, 10], "health": 100 diff --git a/client/scripts/rendering/entity_renderer.gd b/client/scripts/rendering/entity_renderer.gd index 808517e81..21e40b559 100644 --- a/client/scripts/rendering/entity_renderer.gd +++ b/client/scripts/rendering/entity_renderer.gd @@ -3,6 +3,8 @@ extends Node2D # Entity renderer — manages entity sprites under the Entities node # Creates/updates/removes Sprite2D children based on entity data +const TILE_SIZE: int = 32 + var entity_nodes: Dictionary = {} # id -> Node2D mapping func _ready() -> void: @@ -63,7 +65,10 @@ func _update_entity_node(entity_id: int, entity_data: Dictionary) -> void: # Update position if entity_data.has("position"): var pos = entity_data.position - entity_node.position = Vector2(pos[0] * 32, pos[1] * 32) # 32px grid + if pos is Array and pos.size() >= 2: + entity_node.position = Vector2(pos[0] * TILE_SIZE, pos[1] * TILE_SIZE) + else: + push_warning("EntityRenderer: malformed position for entity %s" % entity_id) # Remove an entity node func _remove_entity_node(entity_id: int) -> void: diff --git a/client/tests/test_snapshot_parsing.gd b/client/tests/test_snapshot_parsing.gd new file mode 100644 index 000000000..dcc5c49d5 --- /dev/null +++ b/client/tests/test_snapshot_parsing.gd @@ -0,0 +1,72 @@ +## D-030 Layer 1: Fixture-based tests for snapshot parsing +## Validates that GameState correctly parses ObserverSnapshot data +class_name TestSnapshotParsing +extends GdUnitTestSuite + +# Valid snapshot fixture +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"}, + ], + "fog": {"radius": 8}, + "hud": {"perception_mode": "baseline", "time": "14:30"} +} + +func test_apply_valid_snapshot() -> void: + GameState.apply_snapshot(_valid_snapshot) + + 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 + GameState.player_position = Vector2.ZERO + GameState.visible_entities = [] + + GameState.apply_snapshot({}) + + # State should remain at defaults + 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 + + # Should not crash — logs warning instead + GameState.apply_snapshot(bad_snapshot) + assert_that(GameState.player_position).is_equal(Vector2.ZERO) + +func test_missing_fields_partial_update() -> void: + # First apply valid snapshot + 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"}}) + assert_that(GameState.player_position).is_equal(Vector2(10, 15)) + assert_that(GameState.hud_data.perception_mode).is_equal("thermal") + +func test_sim_bridge_test_snapshot_deterministic() -> void: + SimBridge._test_tick = 0 + var snap1 = SimBridge._test_snapshot() + var snap2 = SimBridge._test_snapshot() + + 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() + assert_that(snap1.has("entities")).is_true() + assert_that(snap1.has("fog")).is_true() + assert_that(snap1.has("hud")).is_true() diff --git a/client/ui/monologue_display.gd b/client/ui/monologue_display.gd index 429a28b24..860c6d58a 100644 --- a/client/ui/monologue_display.gd +++ b/client/ui/monologue_display.gd @@ -9,6 +9,7 @@ extends Control var fade_timer: float = 0.0 var fade_duration: float = 5.0 # Display duration before fade var is_visible: bool = false +var _active_tween: Tween = null func _ready() -> void: print("MonologueDisplay: Initialized") @@ -29,9 +30,11 @@ func show_monologue(text: String, duration: float = 5.0) -> void: fade_timer = 0.0 is_visible = true - # Fade in - var tween = create_tween() - tween.tween_property(text_panel, "modulate:a", 1.0, 0.3) + # Cancel any active tween before starting a new one + if _active_tween and _active_tween.is_valid(): + _active_tween.kill() + _active_tween = create_tween() + _active_tween.tween_property(text_panel, "modulate:a", 1.0, 0.3) # Fade out the monologue func _fade_out() -> void: @@ -39,5 +42,7 @@ func _fade_out() -> void: return is_visible = false - var tween = create_tween() - tween.tween_property(text_panel, "modulate:a", 0.0, 0.5) + if _active_tween and _active_tween.is_valid(): + _active_tween.kill() + _active_tween = create_tween() + _active_tween.tween_property(text_panel, "modulate:a", 0.0, 0.5)