From 44d9941f045e7b48a971d6a3d736b7a25dce6269 Mon Sep 17 00:00:00 2001 From: Jeroen Schweitzer Date: Thu, 19 Feb 2026 12:19:41 +0100 Subject: [PATCH] =?UTF-8?q?fix(client):=20address=20PR=20#40=20review=20?= =?UTF-8?q?=E2=80=94=2013=20suggestions?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Rename _insert to insert_state in main.gd (Hoshe #1) - Add after_each() to test_bug_report_ring_buffer.gd for GameState cleanup on assertion failure (Hoshe #2) - Add after_each() to test_insert_off_behavior.gd for stance/interaction restore on assertion failure (Hoshe #3) - Fix assertion message: "unknown" → "unavailable" (Hoshe #4) - Document memory ceiling of 60 JSON snapshots in ring buffer (Hoshe #5) - Add precision warning for u64 rng_seed via JSON float (Hoshe #6) - Promote _action_enum_to_wire to public action_enum_to_wire (Tyre #1) - Add @warning_ignore for unused _tick parameter (Tyre #5) - Document insert_active assumption for future no-insert characters (Tyre #4) - Restructure OQ-07 decision amendments as bullet points (Tyre #7) Co-Authored-By: Claude Opus 4.6 --- client/scripts/autoloads/game_state.gd | 4 +++- client/scripts/autoloads/sim_bridge.gd | 8 ++++---- client/scripts/main.gd | 8 ++++---- client/tests/test_anti_tedium.gd | 2 +- client/tests/test_bug_report_ring_buffer.gd | 17 ++++++++++++++++- client/tests/test_hub_teleport.gd | 4 ++-- client/tests/test_insert_off_behavior.gd | 7 +++++++ client/tests/test_interaction_list.gd | 6 +++--- client/tests/test_local_bridge.gd | 12 ++++++------ client/ui/bug_report_dialog.gd | 11 ++++++++--- decisions/perception.md | 11 +++++++++-- 11 files changed, 63 insertions(+), 27 deletions(-) diff --git a/client/scripts/autoloads/game_state.gd b/client/scripts/autoloads/game_state.gd index 5b852d539..6be09bc9f 100644 --- a/client/scripts/autoloads/game_state.gd +++ b/client/scripts/autoloads/game_state.gd @@ -42,7 +42,9 @@ var gauntlet_mode: bool = false # true when snapshot includes gauntlet_mode fla # OQ-07 (#522): Insert active state — false suppresses verb labels (z-layer 6). # Cursor shape changes still fire when false (D-056 option a). -# v0.1: always true (both detective and smuggler have inserts). Server may send false in future. +# v0.1 assumption: always true — both playable characters (detective and smuggler) +# have neural inserts. Future characters without inserts would receive false from +# the server's "insert_active" snapshot field, disabling all z-layer-6 UI. var insert_active: bool = true # #507: RNG seed for replay determinism — populated from snapshot "rng_seed" field. diff --git a/client/scripts/autoloads/sim_bridge.gd b/client/scripts/autoloads/sim_bridge.gd index 4c7c8b98b..df6724770 100644 --- a/client/scripts/autoloads/sim_bridge.gd +++ b/client/scripts/autoloads/sim_bridge.gd @@ -179,7 +179,7 @@ func send_input(player_input: Dictionary) -> Error: return ERR_CONNECTION_ERROR if test_mode: var action: int = player_input.get("action", -1) - var wire_name: String = _action_enum_to_wire(action) + var wire_name: String = action_enum_to_wire(action) if not wire_name.is_empty(): if wire_name == "SetFacing": # D-054: Use action_data.facing from the input dict, not InputMapper global @@ -192,9 +192,9 @@ func send_input(player_input: Dictionary) -> Error: else: _test_input_queue.append(wire_name) return OK - var action_name := _action_enum_to_wire(player_input.get("action", -1)) + var action_name := action_enum_to_wire(player_input.get("action", -1)) if action_name.is_empty(): - # _action_enum_to_wire already emits push_warning for invalid actions + # action_enum_to_wire already emits push_warning for invalid actions return ERR_INVALID_PARAMETER # Use the server's current tick so drain_for_tick processes this input immediately. # The client-side timestamp_msec is only useful for ordering within a frame. @@ -251,7 +251,7 @@ func drain_outbound() -> Array[Dictionary]: # 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: +static func action_enum_to_wire(action: int) -> String: match action: InputMapper.Action.MOVE_NORTH: return "MoveNorth" InputMapper.Action.MOVE_NORTHEAST: return "MoveNortheast" diff --git a/client/scripts/main.gd b/client/scripts/main.gd index 6223d1b1f..ed72d567b 100644 --- a/client/scripts/main.gd +++ b/client/scripts/main.gd @@ -84,13 +84,13 @@ func _process(_delta: float) -> void: # OQ-07 (#522): propagate insert state to all z-layer-6 display nodes. # Cursor shape still fires (D-056 option a) — only verb labels suppressed. - var _insert := GameState.insert_active + var insert_state := GameState.insert_active if cursor_renderer and cursor_renderer.has_method("set_insert_active"): - cursor_renderer.set_insert_active(_insert) + cursor_renderer.set_insert_active(insert_state) if interaction_list and interaction_list.has_method("set_insert_active"): - interaction_list.set_insert_active(_insert) + interaction_list.set_insert_active(insert_state) if interaction_prompt and interaction_prompt.has_method("set_insert_active"): - interaction_prompt.set_insert_active(_insert) + interaction_prompt.set_insert_active(insert_state) # D-057: Update interaction list from game state # Suppress during dialogue — player is in conversation, verb list is noise diff --git a/client/tests/test_anti_tedium.gd b/client/tests/test_anti_tedium.gd index f9c493d09..0f7791add 100644 --- a/client/tests/test_anti_tedium.gd +++ b/client/tests/test_anti_tedium.gd @@ -518,7 +518,7 @@ func test_bug_report_not_double_activatable() -> void: func test_bug_report_action_not_on_wire() -> void: # BUG_REPORT is client-only — must not produce a wire-format action name. - var wire_name := SimBridge._action_enum_to_wire(InputMapper.Action.BUG_REPORT) + var wire_name := SimBridge.action_enum_to_wire(InputMapper.Action.BUG_REPORT) assert_that(wire_name).override_failure_message( "BUG_REPORT must not produce a wire action name (client-only)" ).is_equal("") diff --git a/client/tests/test_bug_report_ring_buffer.gd b/client/tests/test_bug_report_ring_buffer.gd index 61350f8ac..f3f3afb5e 100644 --- a/client/tests/test_bug_report_ring_buffer.gd +++ b/client/tests/test_bug_report_ring_buffer.gd @@ -18,6 +18,21 @@ var BugReportDialogScript = load("res://ui/bug_report_dialog.gd") const EXPECTED_CAPACITY := 60 +func after_each() -> void: + # Reset GameState fields mutated by tests to prevent cross-test leakage. + GameState.current_tick = 0 + GameState.room_id = null + GameState.player_stance = "Walk" + GameState.player_facing = "South" + GameState.player_position = Vector2.ZERO + GameState.visible_entities = [] + GameState.visible_tiles = [] + GameState.game_time = {} + GameState.current_monologue = null + GameState.current_dialogue = null + GameState.rng_seed = null + + # -- Helpers ------------------------------------------------------------------- func _make_dialog() -> Control: @@ -285,7 +300,7 @@ func test_seed_written_on_capture() -> void: # Should return some Variant (int or String) without crashing var seed_val = dialog._get_current_seed() assert_that(seed_val != null).override_failure_message( - "_get_current_seed() must not return null (use 0 or 'unknown' if seed unavailable)" + "_get_current_seed() must not return null (use 0 or 'unavailable' if seed unavailable)" ).is_true() diff --git a/client/tests/test_hub_teleport.gd b/client/tests/test_hub_teleport.gd index 12ce5f24b..fe727e930 100644 --- a/client/tests/test_hub_teleport.gd +++ b/client/tests/test_hub_teleport.gd @@ -129,13 +129,13 @@ func test_gauntlet_mode_transitions_off() -> void: func test_teleport_hub_wire_name() -> void: # TELEPORT_HUB must encode to "TeleportToHub" on the wire (matching Rust PlayerAction) - var wire_name := SimBridge._action_enum_to_wire(InputMapper.Action.TELEPORT_HUB) + var wire_name := SimBridge.action_enum_to_wire(InputMapper.Action.TELEPORT_HUB) assert_that(wire_name).is_equal("TeleportToHub") func test_teleport_hub_wire_not_empty() -> void: # Wire name must not be empty (empty = client-only, not sent to server) - var wire_name := SimBridge._action_enum_to_wire(InputMapper.Action.TELEPORT_HUB) + var wire_name := SimBridge.action_enum_to_wire(InputMapper.Action.TELEPORT_HUB) assert_that(wire_name.is_empty()).is_false() diff --git a/client/tests/test_insert_off_behavior.gd b/client/tests/test_insert_off_behavior.gd index 77b8a1500..bfd4ffe89 100644 --- a/client/tests/test_insert_off_behavior.gd +++ b/client/tests/test_insert_off_behavior.gd @@ -16,6 +16,13 @@ class_name TestInsertOffBehavior extends GdUnitTestSuite +func after_each() -> void: + # Reset GameState fields mutated by tests to prevent cross-test leakage. + GameState.nearby_interactions = [] + GameState.player_stance = "Walk" + GameState.insert_active = true + + # -- Helpers ------------------------------------------------------------------- func _make_cursor() -> Node: diff --git a/client/tests/test_interaction_list.gd b/client/tests/test_interaction_list.gd index 404d7adeb..f122d1295 100644 --- a/client/tests/test_interaction_list.gd +++ b/client/tests/test_interaction_list.gd @@ -268,12 +268,12 @@ func test_insert_off_hides_interaction_list() -> void: func test_stance_up_wire_mapping() -> void: # Verify InputMapper.Action.TOGGLE_STANCE_UP maps to "ToggleStanceUp" wire name - var wire = SimBridge._action_enum_to_wire(InputMapper.Action.TOGGLE_STANCE_UP) + var wire = SimBridge.action_enum_to_wire(InputMapper.Action.TOGGLE_STANCE_UP) assert_that(wire).is_equal("ToggleStanceUp") func test_stance_down_wire_mapping() -> void: - var wire = SimBridge._action_enum_to_wire(InputMapper.Action.TOGGLE_STANCE_DOWN) + var wire = SimBridge.action_enum_to_wire(InputMapper.Action.TOGGLE_STANCE_DOWN) assert_that(wire).is_equal("ToggleStanceDown") @@ -295,7 +295,7 @@ func test_all_movement_actions_have_wire_mapping() -> void: InputMapper.Action.TOGGLE_STANCE_DOWN, ] for action in actions_with_mapping: - var wire = SimBridge._action_enum_to_wire(action) + var wire = SimBridge.action_enum_to_wire(action) assert_that(wire.length()).is_greater(0) diff --git a/client/tests/test_local_bridge.gd b/client/tests/test_local_bridge.gd index 880e12ab1..9bc822498 100644 --- a/client/tests/test_local_bridge.gd +++ b/client/tests/test_local_bridge.gd @@ -163,23 +163,23 @@ func test_action_enum_to_wire_all_directions_clockwise() -> void: ] for pair in expected: - var wire_name := SimBridge._action_enum_to_wire(pair[0]) + var wire_name := SimBridge.action_enum_to_wire(pair[0]) assert_that(wire_name).is_equal(pair[1]) func test_action_enum_to_wire_non_movement() -> void: - assert_that(SimBridge._action_enum_to_wire(InputMapper.Action.INTERACT)).is_equal("Interact") - assert_that(SimBridge._action_enum_to_wire(InputMapper.Action.USE_PERCEPTION_MODE)).is_equal("UsePerceptionMode") - assert_that(SimBridge._action_enum_to_wire(InputMapper.Action.PAUSE)).is_equal("Pause") + assert_that(SimBridge.action_enum_to_wire(InputMapper.Action.INTERACT)).is_equal("Interact") + assert_that(SimBridge.action_enum_to_wire(InputMapper.Action.USE_PERCEPTION_MODE)).is_equal("UsePerceptionMode") + assert_that(SimBridge.action_enum_to_wire(InputMapper.Action.PAUSE)).is_equal("Pause") func test_action_enum_to_wire_open_menu_returns_empty() -> void: - var wire_name := SimBridge._action_enum_to_wire(InputMapper.Action.OPEN_MENU) + var wire_name := SimBridge.action_enum_to_wire(InputMapper.Action.OPEN_MENU) assert_that(wire_name).is_equal("") func test_action_enum_to_wire_unknown_returns_empty() -> void: - var wire_name := SimBridge._action_enum_to_wire(9999) + var wire_name := SimBridge.action_enum_to_wire(9999) assert_that(wire_name).is_equal("") diff --git a/client/ui/bug_report_dialog.gd b/client/ui/bug_report_dialog.gd index 383795217..a2a5efdfe 100644 --- a/client/ui/bug_report_dialog.gd +++ b/client/ui/bug_report_dialog.gd @@ -41,6 +41,9 @@ var _active: bool = false # Input ring: replay-format PlayerInput arrays, one per tick. # Snapshot ring: ObserverSnapshot JSON strings, one per tick. # Separate heads and counts so each buffer can be tested independently. +# Memory ceiling: 60 snapshot JSON strings (each ~2-8KB depending on entity count) +# + 60 input arrays (negligible). Worst case ~480KB resident. Acceptable for a +# debug tool that is always active during Gauntlet play. var _input_ring: Array = [] # Array[Array] — each slot: Array of {tick, action} dicts var _input_head: int = 0 # Next write index (0..RING_SIZE-1) var _input_count: int = 0 # Filled slot count (0..RING_SIZE) @@ -91,8 +94,7 @@ func record_tick(tick: int, snapshot_json: String, mapper_inputs: Array) -> void ## Push replay-format inputs for one tick. inputs is Array of {tick, action} dicts. ## Overwrites oldest entry when buffer is full (circular eviction). -## tick param is unused here — each input dict carries its own tick field. -## Kept in the signature so tests and callers can pass tick for symmetry with record_tick(). +@warning_ignore("unused_parameter") func _push_tick_inputs(_tick: int, inputs: Array) -> void: _input_ring[_input_head] = inputs _input_head = (_input_head + 1) % RING_SIZE @@ -133,6 +135,9 @@ func _format_snapshots_jsonl() -> String: ## Return the RNG seed for seed.txt. Never returns null. ## Uses GameState.rng_seed if available; falls back to "unavailable" string. +## Note: rng_seed is u64 on the server. JSON encodes u64 as a number, which +## loses precision above 2^53 via float intermediary. When the server field +## lands, consider string-encoding the seed to preserve all 64 bits. func _get_current_seed() -> Variant: if GameState.rng_seed != null: return GameState.rng_seed @@ -145,7 +150,7 @@ func _get_current_seed() -> Variant: ## {"tick": N, "action": {"Interact": {"target_entity_id": ..., "verb": ...}}} func _to_replay_format(input: Dictionary, tick: int) -> Dictionary: var action_enum: int = input.get("action", -1) - var wire: String = SimBridge._action_enum_to_wire(action_enum) + var wire: String = SimBridge.action_enum_to_wire(action_enum) if wire.is_empty(): return {} # Client-only action (BUG_REPORT, OPEN_MENU) diff --git a/decisions/perception.md b/decisions/perception.md index 10af01f29..fdace2f21 100644 --- a/decisions/perception.md +++ b/decisions/perception.md @@ -212,7 +212,11 @@ How the player observes and interacts with the world: camera, fog, line-of-sight - **Source:** Control & Interaction Workshop (2026-02-13) - **Raised by:** Araminta (visual spec), Stig (UX rules + diegetic test), Ozzie (weapon suppression) - **Dissent:** None. -- **OQ-07 resolution (2026-02-19, #522):** Insert-off behavior is **option (a): cursor shape still changes, verb labels suppressed.** The cursor state machine fires normally (entity hover → bracket shape, object hover → X-shape) — the character's body physically orients toward targets as a subconscious/spatial response. But the insert does not process these into actionable data: `should_show_interactions()` returns false when `insert_active == false`, and interaction labels (z-layer 6) are hidden via `set_insert_active(false)` on `InteractionList` and `InteractionPrompt`. `GameState.insert_active` is the source of truth (defaults true in v0.1; wired from snapshot field `insert_active`). Rationale: diegetically consistent — the body reacts to proximity; the insert reacts to commands. +- **OQ-07 resolution (2026-02-19, #522):** Insert-off behavior is **option (a): cursor shape still changes, verb labels suppressed.** + - Cursor state machine fires normally (entity hover → bracket shape, object hover → X-shape) — the character's body physically orients toward targets as a subconscious/spatial response. + - Insert does not process targets into actionable data: `should_show_interactions()` returns false when `insert_active == false`, and interaction labels (z-layer 6) are hidden via `set_insert_active(false)` on `InteractionList` and `InteractionPrompt`. + - `GameState.insert_active` is the source of truth (defaults true in v0.1; wired from snapshot field `insert_active`). + - Rationale: diegetically consistent — the body reacts to proximity; the insert reacts to commands. ### D-057: Entity interaction — vertical list, insert-styled - **Date:** 2026-02-13 @@ -225,7 +229,10 @@ How the player observes and interacts with the world: camera, fog, line-of-sight - **Source:** Control & Interaction Workshop (2026-02-13) - **Raised by:** Stig (vertical list structure + diegetic test), Araminta (insert aesthetic), Dudley (two-phase verb computation), Nigel (character-archetype verb sets). Lead resolved: Stig's structure, Araminta's styling. - **Dissent:** Araminta argued for spoke radial (geometry transformation signals qualitative knowledge change — new spoke growing). Lead rejected: items moving under cursor when knowledge changes is a moving goalpost (bad UX while aiming at an option). -- **OQ-07 resolution (2026-02-19, #522):** When `insert_active == false`, the interaction list hides completely (`set_insert_active(false)` → `_hide()`). Cursor shape changes still occur per D-056 OQ-07 — the list suppression is independent of cursor state. `GameState.insert_active` drives this at runtime, wired via `main.gd` on each snapshot. +- **OQ-07 resolution (2026-02-19, #522):** + - When `insert_active == false`, the interaction list hides completely (`set_insert_active(false)` → `_hide()`). + - Cursor shape changes still occur per D-056 OQ-07 — list suppression is independent of cursor state. + - `GameState.insert_active` drives this at runtime, wired via `main.gd` on each snapshot. ### D-058: World menu — radial, 4 spokes - **Date:** 2026-02-13