From 64897a40b3c8347e9fa595d430580c14b01c15ea Mon Sep 17 00:00:00 2001 From: Jeroen Schweitzer Date: Thu, 12 Feb 2026 01:07:02 +0100 Subject: [PATCH] =?UTF-8?q?fix(client):=20address=20PR=20#11=20review=20?= =?UTF-8?q?=E2=80=94=20type=20safety,=20perf,=20tests?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Add Dictionary validation in game_state visible_tiles loop to prevent crash on malformed data (Hoshe critical). Skip redundant modulate.a writes (Hoshe suggestion). Add 3 tests: malformed visible_tiles, facing rotation accuracy, v1 backward compatibility. Clarify D-033 Phase 1/Phase 2 comments. Co-Authored-By: Claude Opus 4.6 --- client/scripts/autoloads/game_state.gd | 2 ++ client/scripts/constants.gd | 3 +- client/scripts/rendering/entity_renderer.gd | 8 ++--- client/tests/test_protocol.gd | 23 +++++++++++++ client/tests/test_rendering.gd | 37 +++++++++++++++++++++ 5 files changed, 68 insertions(+), 5 deletions(-) diff --git a/client/scripts/autoloads/game_state.gd b/client/scripts/autoloads/game_state.gd index 797422773..c695e08e1 100644 --- a/client/scripts/autoloads/game_state.gd +++ b/client/scripts/autoloads/game_state.gd @@ -62,6 +62,8 @@ func apply_snapshot(snapshot: Dictionary) -> void: if not has_explicit_positions: visible_positions.clear() for vtile in snapshot.visible_tiles: + if not vtile is Dictionary or not vtile.has("x") or not vtile.has("y"): + continue var pos := Vector2i(vtile.x, vtile.y) if vtile.has("visibility"): visibility_sectors[pos] = vtile.visibility diff --git a/client/scripts/constants.gd b/client/scripts/constants.gd index 4940cb390..f7df9800f 100644 --- a/client/scripts/constants.gd +++ b/client/scripts/constants.gd @@ -7,7 +7,8 @@ const TILE_SIZE: int = 32 # D-033: Entity relationship color palette # Color represents the player's RELATIONSHIP to the entity, not an objective property. -# Phase 1: default colors by entity kind. Phase 2 (#361): driven by RelationshipState. +# Phase 1: default colors mapped by entity kind (Player/Npc/Object/Terrain). +# Phase 2 (#361): colors derived from RelationshipState via the knowledge graph. const ENTITY_COLOR_UNKNOWN: Color = Color("#4a9ebb") # Unknown/Neutral — cool teal const ENTITY_COLOR_FRIENDLY: Color = Color("#6bc9a6") # Known/Friendly — soft green const ENTITY_COLOR_POI: Color = Color("#e8c547") # Person of Interest — warm amber diff --git a/client/scripts/rendering/entity_renderer.gd b/client/scripts/rendering/entity_renderer.gd index 8cb01224e..cd6a439a5 100644 --- a/client/scripts/rendering/entity_renderer.gd +++ b/client/scripts/rendering/entity_renderer.gd @@ -79,11 +79,11 @@ func _update_entity_node(entity_id: int, entity_data: Dictionary) -> void: ) # v2: Peripheral vision dimming (D-015) + # null visibility (v1 backward compat) defaults to full alpha var visibility: Variant = entity_data.get("visibility") - if visibility == "Peripheral": - entity_node.modulate.a = Constants.PERIPHERAL_ALPHA - else: - entity_node.modulate.a = 1.0 + var target_alpha := Constants.PERIPHERAL_ALPHA if visibility == "Peripheral" else 1.0 + if not is_equal_approx(entity_node.modulate.a, target_alpha): + entity_node.modulate.a = target_alpha # v2: Update facing indicator rotation (player entity only) if entity_id == GameState.player_entity_id: diff --git a/client/tests/test_protocol.gd b/client/tests/test_protocol.gd index b1eaa3ed7..818f7659c 100644 --- a/client/tests/test_protocol.gd +++ b/client/tests/test_protocol.gd @@ -322,6 +322,29 @@ func test_multi_entity_visibility_sectors() -> void: assert_that(snapshot.entities[3].visibility).is_equal("Forward") +# -- v1 backward compatibility (no v2 fields → graceful null defaults) -------- + +func test_decode_v1_snapshot_graceful_defaults() -> void: + # Minimal v1 snapshot — only tick + entities, no v2 fields + var v1_raw := {"tick": 10, "entities": [ + {"entity_id": 1, "x": 5.0, "y": 5.0, "z": 0, "kind": "Player"}, + ]} + var encoded: Variant = Messagepack.encode(v1_raw) + assert_that(encoded.status).is_null() + + var snapshot: Variant = Protocol.decode_snapshot(encoded.value) + assert_that(snapshot).is_not_null() + assert_that(snapshot.tick).is_equal(10) + assert_that(snapshot.entities.size()).is_equal(1) + # v2 fields should be null/empty, not crash + assert_that(snapshot.version).is_null() + assert_that(snapshot.game_time).is_null() + assert_that(snapshot.player_facing).is_null() + assert_that(snapshot.visible_tiles.size()).is_equal(0) + # Entity should have null visibility + assert_that(snapshot.entities[0].visibility).is_null() + + # -- Batch input fixture (D-030 Layer 1 bidirectional symmetry) ---------------- func test_decode_batch_input_fixture() -> void: diff --git a/client/tests/test_rendering.gd b/client/tests/test_rendering.gd index 4f0eecbd4..21b662395 100644 --- a/client/tests/test_rendering.gd +++ b/client/tests/test_rendering.gd @@ -98,6 +98,21 @@ func test_game_state_derives_visible_positions_from_visible_tiles() -> void: assert_that(GameState.visibility_sectors[Vector2i(5, 5)]).is_equal("Forward") assert_that(GameState.visibility_sectors[Vector2i(6, 5)]).is_equal("Peripheral") +func test_game_state_skips_malformed_visible_tiles() -> void: + var vtiles := [ + {"x": 5, "y": 5, "z": 0, "visibility": "Forward"}, + null, + "not_a_dict", + {"z": 0}, # missing x, y + {"x": 6, "y": 6, "z": 0, "visibility": "Peripheral"}, + ] + GameState.apply_snapshot({"tick": 1, "visible_tiles": vtiles}) + # Only the 2 valid entries should be stored + assert_that(GameState.visible_positions.size()).is_equal(2) + assert_that(GameState.visible_positions.has(Vector2i(5, 5))).is_true() + assert_that(GameState.visible_positions.has(Vector2i(6, 6))).is_true() + assert_that(GameState.visibility_sectors.size()).is_equal(2) + func test_game_state_warns_on_missing_player() -> void: GameState.player_entity_id = 999 GameState.player_position = Vector2(5, 5) @@ -283,6 +298,28 @@ func test_entity_renderer_player_has_facing_indicator() -> void: assert_that(indicator is Polygon2D).is_true() renderer.queue_free() +func test_entity_renderer_facing_indicator_rotation_accuracy() -> void: + GameState.player_entity_id = 1 + var renderer := _make_entity_renderer() + var directions := { + "North": 0.0, + "Northeast": PI / 4.0, + "East": PI / 2.0, + "Southeast": 3.0 * PI / 4.0, + "South": PI, + "Southwest": 5.0 * PI / 4.0, + "West": 3.0 * PI / 2.0, + "Northwest": 7.0 * PI / 4.0, + } + renderer.update_entities(_test_entities_v2) + var player_node = renderer.entity_nodes[1] + var indicator = player_node.get_node_or_null("FacingIndicator") + for dir_name in directions: + GameState.player_facing = dir_name + renderer.update_entities(_test_entities_v2) + assert_that(indicator.rotation).is_equal_approx(directions[dir_name], 0.001) + renderer.queue_free() + func test_entity_renderer_npc_has_no_facing_indicator() -> void: GameState.player_entity_id = 1 var renderer := _make_entity_renderer()