From c8de1a06294eb63e7f780a176d326f017c25fe74 Mon Sep 17 00:00:00 2001 From: Jeroen Schweitzer Date: Wed, 25 Feb 2026 21:59:38 +0100 Subject: [PATCH] refactor(client): make stationary_ticks and zone_id server-authoritative (#557) apply_snapshot() now reads stationary_ticks and zone_id directly from the server snapshot when present (D-020 compliance). Client-side accumulation and tile lookup retained as deprecated fallbacks until the server populates these fields. Protocol.gd extended with decode paths and TODO markers for the server team. Co-Authored-By: Claude Opus 4.6 --- client/scripts/autoloads/game_state.gd | 54 +++++--- client/scripts/protocol/protocol.gd | 20 +++ client/tests/test_game_state.gd | 67 ++++++++- client/tests/test_game_state_sprint20.gd | 164 +++++++++++++++++++++++ 4 files changed, 283 insertions(+), 22 deletions(-) create mode 100644 client/tests/test_game_state_sprint20.gd diff --git a/client/scripts/autoloads/game_state.gd b/client/scripts/autoloads/game_state.gd index a59174bfd..305e6cf69 100644 --- a/client/scripts/autoloads/game_state.gd +++ b/client/scripts/autoloads/game_state.gd @@ -109,14 +109,17 @@ var medium_sound_events: Array = [] var close_sound_events: Array = [] # D-071 (#530): Consecutive ticks without player position change. -# Incremented per snapshot in apply_snapshot(). Reset to 0 on movement. +# D-020: Server-authoritative — read from snapshot "stationary_ticks" field. +# Fallback: client-side accumulation (deprecated, remove when server populates field). # ListeningFocus boost activates at 30+ ticks (main.gd manages the dip). var stationary_ticks: int = 0 +# DEPRECATED: Only used by client-side accumulation fallback. Remove with fallback. var _prev_player_position: Vector2 = Vector2(-1e9, -1e9) # sentinel: no previous position # D-073 (#529): Server-authoritative zone_id from the player's current tile. -# Extracted in apply_snapshot() — avoids O(N) tile scan in main.gd per Tyre review. -# Empty string when zone_id field absent (server hasn't shipped OQ-09 yet). +# D-020: Read directly from snapshot "zone_id" field. +# Fallback: client-side tile lookup (deprecated, remove when server populates field). +# Empty string when zone_id field absent. var current_zone_id: String = "" func apply_snapshot(snapshot: Dictionary) -> void: @@ -140,12 +143,19 @@ func apply_snapshot(snapshot: Dictionary) -> void: push_warning("GameState: no Player entity found in %d entities" % [ visible_entities.size()]) - # D-071 (#530): Track consecutive stationary ticks for ListeningFocus boost. - # Compares current player_position against previous snapshot's position. - if player_position == _prev_player_position: - stationary_ticks += 1 + # D-020/D-071 (#530): Server-authoritative stationary_ticks for ListeningFocus boost. + # Prefer server-sent value; fall back to client-side accumulation until server populates. + if snapshot.has("stationary_ticks") and snapshot.stationary_ticks is int: + # D-020: direct field assignment from server-authoritative snapshot. + stationary_ticks = snapshot.stationary_ticks else: - stationary_ticks = 0 + # DEPRECATED fallback — client-side accumulation. Remove when server sends + # "stationary_ticks" in ObserverSnapshot (D-020 violation: derives behavior- + # driving state on the client). Server tracks this in ListeningFocus component. + if player_position == _prev_player_position: + stationary_ticks += 1 + else: + stationary_ticks = 0 _prev_player_position = player_position # Tiles for rendering: test mode sends "tiles", live server sends tile data in "visible_tiles" @@ -295,16 +305,24 @@ func apply_snapshot(snapshot: Dictionary) -> void: if snapshot.has("player_knowledge") and snapshot.player_knowledge is Dictionary: player_knowledge = snapshot.player_knowledge - # D-073 (#529): O(1) zone_id lookup. Build coord→tile dict from member visible_tiles - # (populated above from either "tiles" test-mode key or "visible_tiles" live key). - # Must use the member var, not snapshot.visible_tiles, so test mode is covered. - var _tile_by_coord: Dictionary = {} - for vtile in visible_tiles: - if vtile is Dictionary and vtile.has("x") and vtile.has("y"): - _tile_by_coord[Vector2i(vtile.x, vtile.y)] = vtile - var player_pos_key := Vector2i(int(player_position.x), int(player_position.y)) - var player_tile = _tile_by_coord.get(player_pos_key, null) - current_zone_id = player_tile.get("zone_id", "") if player_tile else "" + # D-020/D-073 (#529): Server-authoritative zone_id for zone ambient crossfade. + # Prefer server-sent top-level value; fall back to client-side tile lookup until + # server populates top-level "zone_id" in ObserverSnapshot. + if snapshot.has("zone_id") and snapshot.zone_id is String: + # D-020: direct field assignment from server-authoritative snapshot. + current_zone_id = snapshot.zone_id + else: + # DEPRECATED fallback — client-side tile lookup. Remove when server sends + # top-level "zone_id" in ObserverSnapshot (D-020 violation: derives zone + # identity on the client via tile iteration). Server sends zone_id per + # VisibleTile but not as a top-level snapshot field. + var _tile_by_coord: Dictionary = {} + for vtile in visible_tiles: + if vtile is Dictionary and vtile.has("x") and vtile.has("y"): + _tile_by_coord[Vector2i(vtile.x, vtile.y)] = vtile + var player_pos_key := Vector2i(int(player_position.x), int(player_position.y)) + var player_tile = _tile_by_coord.get(player_pos_key, null) + current_zone_id = player_tile.get("zone_id", "") if player_tile else "" # v2: visible_tiles with visibility sectors # Derives visible_positions when not explicitly provided (real server mode) diff --git a/client/scripts/protocol/protocol.gd b/client/scripts/protocol/protocol.gd index 0f00caf74..c7166c479 100644 --- a/client/scripts/protocol/protocol.gd +++ b/client/scripts/protocol/protocol.gd @@ -244,6 +244,24 @@ static func decode_snapshot(bytes: PackedByteArray) -> Variant: "error": raw_save.get("error"), } + # TODO(server): Send stationary_ticks in ObserverSnapshot (D-071, D-020). + # Server already tracks this in ListeningFocus component (server/src/simulation/listening.rs). + # When server populates this field, client-side accumulation fallback in game_state.gd + # can be removed — apply_snapshot() should contain only direct field assignments. + var stationary_ticks: Variant = null + var raw_st: Variant = raw.get("stationary_ticks") + if raw_st != null: + stationary_ticks = int(raw_st) + + # TODO(server): Send top-level zone_id string in ObserverSnapshot (D-073, D-020). + # Server sends zone_id per VisibleTile but not as a top-level snapshot field. + # When server populates this, client-side tile iteration fallback in game_state.gd + # can be removed — apply_snapshot() should contain only direct field assignments. + var zone_id: Variant = null + var raw_zid: Variant = raw.get("zone_id") + if raw_zid is String: + zone_id = raw_zid + # v14: player_knowledge (#264, D-041) — partial KG dump for journal panel. # {entities: [{entity_id, name, confidence, source, state, relationship, last_observed_tick}], # facts: [{fact_id, confidence, source, state, acquired_tick}]} @@ -302,6 +320,8 @@ static func decode_snapshot(bytes: PackedByteArray) -> Variant: "examine_result": examine_result, "player_knowledge": player_knowledge, "save_result": save_result, + "stationary_ticks": stationary_ticks, + "zone_id": zone_id, } diff --git a/client/tests/test_game_state.gd b/client/tests/test_game_state.gd index 48b3c69cf..aebd84461 100644 --- a/client/tests/test_game_state.gd +++ b/client/tests/test_game_state.gd @@ -19,6 +19,8 @@ func before_each() -> void: GameState.player_stance = "Walk" GameState.player_inventory = [] GameState.stationary_ticks = 0 + GameState._prev_player_position = Vector2(-1e9, -1e9) + GameState.current_zone_id = "" GameState.insert_active = true @@ -115,19 +117,49 @@ func test_apply_snapshot_inventory_absent_clears_list() -> void: assert_that(GameState.player_inventory.size()).is_equal(0) -# -- Stationary tick counter (D-071) ------------------------------------- +# -- Stationary ticks: server-authoritative (D-020/D-071) ----------------- -func test_stationary_ticks_increments_when_player_position_unchanged() -> void: +func test_stationary_ticks_from_server_snapshot() -> void: + ## D-020: When server sends stationary_ticks, client reads it directly. + GameState.apply_snapshot({"tick": 1, "entities": [], "stationary_ticks": 42}) + assert_int(GameState.stationary_ticks).is_equal(42) + + +func test_stationary_ticks_server_value_overrides_client_accumulation() -> void: + ## D-020: Server value takes priority — client must not accumulate on top of it. + var snapshot := { + "tick": 1, + "entities": [{"entity_id": 1, "x": 10.0, "y": 10.0, "z": 0, "kind": {"variant": "Player", "data": null}}], + "stationary_ticks": 5, + } + GameState.apply_snapshot(snapshot) + GameState.apply_snapshot(snapshot) # same position, but server sends 5 again + assert_int(GameState.stationary_ticks).is_equal(5) # server value, not 6 + + +func test_stationary_ticks_missing_field_degrades_gracefully() -> void: + ## D-020 fallback: when server omits stationary_ticks, no crash, defaults to 0. + GameState.stationary_ticks = 0 + GameState.apply_snapshot({"tick": 1, "entities": []}) + # No crash; field retains a valid integer value. + assert_int(GameState.stationary_ticks).is_greater_equal(0) + + +# -- Stationary ticks: deprecated client-side fallback (D-071) ----------- + +func test_stationary_ticks_fallback_increments_when_position_unchanged() -> void: + ## Deprecated fallback: client accumulates when server omits the field. var snapshot := { "tick": 1, "entities": [{"entity_id": 1, "x": 10.0, "y": 10.0, "z": 0, "kind": {"variant": "Player", "data": null}}], } - GameState.apply_snapshot(snapshot) # first call: position changes from ZERO + GameState.apply_snapshot(snapshot) # first call: position changes from sentinel GameState.apply_snapshot(snapshot) # second call: position unchanged → +1 assert_int(GameState.stationary_ticks).is_greater(0) -func test_stationary_ticks_resets_on_player_movement() -> void: +func test_stationary_ticks_fallback_resets_on_movement() -> void: + ## Deprecated fallback: client resets on movement when server omits the field. var s1 := { "tick": 1, "entities": [{"entity_id": 1, "x": 10.0, "y": 10.0, "z": 0, "kind": {"variant": "Player", "data": null}}], @@ -143,6 +175,33 @@ func test_stationary_ticks_resets_on_player_movement() -> void: assert_int(GameState.stationary_ticks).is_equal(0) +# -- Zone ID: server-authoritative (D-020/D-073) ------------------------- + +func test_zone_id_from_server_snapshot() -> void: + ## D-020: When server sends top-level zone_id, client reads it directly. + GameState.apply_snapshot({"tick": 1, "entities": [], "zone_id": "zone_alpha"}) + assert_that(GameState.current_zone_id).is_equal("zone_alpha") + + +func test_zone_id_server_value_overrides_tile_derivation() -> void: + ## D-020: Server top-level zone_id takes priority over tile-derived zone_id. + var snapshot := { + "tick": 1, + "entities": [{"entity_id": 1, "x": 5.0, "y": 5.0, "z": 0, "kind": {"variant": "Player", "data": null}}], + "tiles": [{"x": 5, "y": 5, "z": 0, "type": "floor", "zone_id": "zone_from_tile"}], + "zone_id": "zone_from_server", + } + GameState.apply_snapshot(snapshot) + assert_that(GameState.current_zone_id).is_equal("zone_from_server") + + +func test_zone_id_missing_field_degrades_gracefully() -> void: + ## D-020 fallback: when server omits zone_id and no tiles match, defaults to "". + GameState.current_zone_id = "" + GameState.apply_snapshot({"tick": 1, "entities": []}) + assert_that(GameState.current_zone_id).is_equal("") + + # -- insert_active (OQ-07, #522) ----------------------------------------- func test_apply_snapshot_insert_active_false() -> void: diff --git a/client/tests/test_game_state_sprint20.gd b/client/tests/test_game_state_sprint20.gd new file mode 100644 index 000000000..7c302a03d --- /dev/null +++ b/client/tests/test_game_state_sprint20.gd @@ -0,0 +1,164 @@ +## Sprint 20 #557: GameState.apply_snapshot() refactor tests. +## +## Verifies D-020 compliance: apply_snapshot() reads server-authoritative values +## for stationary_ticks and zone_id directly from the snapshot when present, +## and degrades gracefully when the server has not yet added these fields. +## +## Does NOT replace test_game_state.gd or test_snapshot_zone_id.gd — those cover +## existing behaviour. This file covers the new code paths added in Sprint 20. +## +## D-030: fixture-based, server-free, no subprocess required. +class_name TestGameStateSprint20 +extends GdUnitTestSuite + + +func before_each() -> void: + GameState.stationary_ticks = 0 + GameState._prev_player_position = Vector2(-1e9, -1e9) + GameState.current_zone_id = "" + GameState.player_position = Vector2.ZERO + GameState.visible_tiles = [] + GameState.visible_positions = {} + GameState.visibility_sectors = {} + + +# -- stationary_ticks: server-authoritative path (D-020) ---------------------- + +func test_stationary_ticks_reads_server_value_when_present() -> void: + # When snapshot includes stationary_ticks, apply_snapshot() must use the + # server value directly without client-side accumulation (D-020). + var snapshot := { + "tick": 5, + "entities": [ + {"entity_id": 1, "x": 10.0, "y": 10.0, "z": 0, "kind": {"variant": "Player", "data": null}}, + ], + "stationary_ticks": 42, + } + GameState.apply_snapshot(snapshot) + assert_int(GameState.stationary_ticks).override_failure_message( + "apply_snapshot() must read stationary_ticks=42 from snapshot (D-020)" + ).is_equal(42) + + +func test_stationary_ticks_server_value_does_not_accumulate() -> void: + # Server-sent value must be assigned directly — NOT added to existing value. + # Two calls with stationary_ticks=10 must yield 10, not 20. + var snapshot := { + "tick": 1, + "entities": [ + {"entity_id": 1, "x": 5.0, "y": 5.0, "z": 0, "kind": {"variant": "Player", "data": null}}, + ], + "stationary_ticks": 10, + } + GameState.apply_snapshot(snapshot) + GameState.apply_snapshot(snapshot) + assert_int(GameState.stationary_ticks).override_failure_message( + "Server value must be assigned directly, not accumulated (D-020)" + ).is_equal(10) + + +func test_stationary_ticks_server_can_reset_to_zero() -> void: + # Server sends 0 when player moves — client must accept this reset. + GameState.stationary_ticks = 50 + var snapshot := { + "tick": 2, + "entities": [ + {"entity_id": 1, "x": 5.0, "y": 5.0, "z": 0, "kind": {"variant": "Player", "data": null}}, + ], + "stationary_ticks": 0, + } + GameState.apply_snapshot(snapshot) + assert_int(GameState.stationary_ticks).override_failure_message( + "Server reset to 0 must override client-held value" + ).is_equal(0) + + +# -- stationary_ticks: DEPRECATED fallback (graceful degradation) -------------- + +func test_stationary_ticks_fallback_when_field_absent() -> void: + # When snapshot has no stationary_ticks, client-side accumulation must still + # run (backward compat until server ships field). No crash. + var snapshot := { + "tick": 1, + "entities": [ + {"entity_id": 1, "x": 10.0, "y": 10.0, "z": 0, "kind": {"variant": "Player", "data": null}}, + ], + } + GameState.apply_snapshot(snapshot) # position changes from ZERO → resets to 0 + GameState.apply_snapshot(snapshot) # position unchanged → increments + assert_int(GameState.stationary_ticks).override_failure_message( + "Client-side fallback must increment stationary_ticks when field absent" + ).is_greater(0) + + +func test_apply_snapshot_missing_stationary_ticks_no_crash() -> void: + # Snapshots lacking stationary_ticks must not crash apply_snapshot(). + var snapshot := {"tick": 1, "entities": []} + # No assertion needed beyond confirming no exception is raised. + GameState.apply_snapshot(snapshot) + assert_bool(true).is_true() + + +# -- zone_id: server-authoritative path (D-020/D-073) ------------------------- + +func test_current_zone_id_reads_server_value_when_present() -> void: + # When snapshot includes top-level zone_id, apply_snapshot() must use it + # directly without tile lookup (D-020). + var snapshot := { + "tick": 1, + "entities": [ + {"entity_id": 1, "x": 5.0, "y": 5.0, "z": 0, "kind": {"variant": "Player", "data": null}}, + ], + "zone_id": "zone_server_direct", + } + GameState.apply_snapshot(snapshot) + assert_str(GameState.current_zone_id).override_failure_message( + "apply_snapshot() must read zone_id='zone_server_direct' from snapshot (D-020)" + ).is_equal("zone_server_direct") + + +func test_current_zone_id_server_value_overrides_tile_data() -> void: + # When snapshot has both zone_id and visible_tiles with a different zone, + # the top-level zone_id field takes priority. + var snapshot := { + "tick": 1, + "entities": [ + {"entity_id": 1, "x": 5.0, "y": 5.0, "z": 0, "kind": {"variant": "Player", "data": null}}, + ], + "zone_id": "zone_from_server", + "visible_tiles": [ + {"x": 5, "y": 5, "z": 0, "type": "floor", "zone_id": "zone_from_tile"}, + ], + } + GameState.apply_snapshot(snapshot) + assert_str(GameState.current_zone_id).override_failure_message( + "Top-level zone_id must override tile-derived zone when both present" + ).is_equal("zone_from_server") + + +# -- zone_id: DEPRECATED fallback (graceful degradation) ---------------------- + +func test_current_zone_id_fallback_to_tile_lookup_when_absent() -> void: + # When snapshot lacks top-level zone_id, tile lookup fallback must run. + # Existing test_snapshot_zone_id.gd covers detailed scenarios; this is a + # smoke test confirming the fallback path still works after Sprint 20 refactor. + var snapshot := { + "tick": 1, + "entities": [ + {"entity_id": 1, "x": 3.0, "y": 3.0, "z": 0, "kind": {"variant": "Player", "data": null}}, + ], + "visible_tiles": [ + {"x": 3, "y": 3, "z": 0, "type": "floor", "zone_id": "zone_tile_fallback"}, + ], + } + GameState.apply_snapshot(snapshot) + assert_str(GameState.current_zone_id).override_failure_message( + "Tile lookup fallback must populate zone_id when top-level field absent" + ).is_equal("zone_tile_fallback") + + +func test_apply_snapshot_missing_zone_id_no_crash() -> void: + # Snapshots lacking both zone_id and visible_tiles must not crash. + var snapshot := {"tick": 1, "entities": []} + GameState.apply_snapshot(snapshot) + assert_str(GameState.current_zone_id).is_equal("")