From 2ab41a0f04b23af2ddb29a408ef7ff665f536fa0 Mon Sep 17 00:00:00 2001 From: Jeroen Schweitzer Date: Tue, 17 Feb 2026 17:33:56 +0100 Subject: [PATCH] =?UTF-8?q?fix(client):=20address=20PR=20#25=20review=20?= =?UTF-8?q?=E2=80=94=205=20critical=20bugs,=204=20warnings,=203=20suggesti?= =?UTF-8?q?ons?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Critical fixes: - hide_dialogue() sent PAUSE instead of UNPAUSE, permanently freezing simulation after every dialogue (both reviewers) - Confrontation monologue hardcoded in GDScript constant, violating D-042/D-020 — moved to ui-strings.yaml as dialogue.confrontation_beat - Walk-away WASD didn't call set_input_as_handled(), letting movement event propagate and potentially stepping on the same frame - is_dialogue_active() returned _is_showing only — interaction list could flash during 300ms fade gap. Now includes dialogue_active state - Removed dead _last_dialogue_id / get_dialogue_id() state (never read) Warnings addressed: - Audio registry now scans res://audio/ recursively (subdirs registered) - add_bus_effect guarded against duplicate calls in tests - int64 encoder dead code tagged KNOWN-DEFECT, filed as ticket #516 - D-073 zone crossfade stub comment clarifies Sprint 9+ deferral - listening_focus dip documents caller tick-gate responsibility (D-069/D-071) Co-Authored-By: Claude Opus 4.6 --- client/data/ui-strings.yaml | 8 +++++++ client/scripts/autoloads/audio_manager.gd | 26 +++++++++++++++++------ client/scripts/main.gd | 5 ----- client/tests/test_msgpack_boundaries.gd | 8 +++---- client/ui/dialogue_box.gd | 17 ++++++--------- 5 files changed, 37 insertions(+), 27 deletions(-) diff --git a/client/data/ui-strings.yaml b/client/data/ui-strings.yaml index 89a5e9f0c..79bac0ad3 100644 --- a/client/data/ui-strings.yaml +++ b/client/data/ui-strings.yaml @@ -144,6 +144,14 @@ tutorial_prompts: minimap_hint: "The overlay shows the area around you." knowledge_hint: "Open your notes to review what you know." +# ============================================================ +# DIALOGUE +# Text displayed during dialogue interactions. +# D-063: Confrontation beat monologue — character's internal hesitation. +# ============================================================ +dialogue: + confrontation_beat: "This changes things. No taking it back." + # ============================================================ # MENU AND SYSTEM TEXT # Non-diegetic. Standard game UI. Clean, no flavor text. diff --git a/client/scripts/autoloads/audio_manager.gd b/client/scripts/autoloads/audio_manager.gd index 5b871db76..1707d2e12 100644 --- a/client/scripts/autoloads/audio_manager.gd +++ b/client/scripts/autoloads/audio_manager.gd @@ -15,6 +15,8 @@ const BUSES := [BUS_MUSIC, BUS_AMBIENT, BUS_WORLD_SFX, BUS_PLAYER_ACTIONS, BUS_U # D-069: Audio dip profiles — dB offsets relative to player slider setting. # Mid-range of spec values used (e.g. -6 to -8 → -7). +# NOTE: listening_focus requires 30+ stationary ticks before activation (D-069/D-071). +# The tick gate is the caller's responsibility — AudioManager only manages bus volumes. const DIP_SPECS := { "dialogue": { "ease_in": 0.3, "ease_out": 0.5, @@ -70,8 +72,9 @@ func _setup_buses() -> void: _bus_volumes[bus_name] = 0.0 # Low-pass filter on Ambient bus — cutoff starts high (bypassed). # Confrontation dip sweeps it down to 800 Hz (D-069). + # Guard: skip if filter already exists (e.g. duplicate _ready in tests). var amb_idx := AudioServer.get_bus_index(BUS_AMBIENT) - if amb_idx >= 0: + if amb_idx >= 0 and _ambient_filter == null: _ambient_filter = AudioEffectLowPassFilter.new() _ambient_filter.cutoff_hz = FILTER_CUTOFF_DEFAULT AudioServer.add_bus_effect(amb_idx, _ambient_filter) @@ -80,20 +83,28 @@ func _setup_buses() -> void: # --- Asset registry (D-068 directory-scan pattern) --- func _scan_registry() -> void: - var dir := DirAccess.open("res://audio/") + _scan_dir("res://audio/") + print("AudioManager: %d assets registered" % _registry.size()) + + +func _scan_dir(path: String) -> void: + var dir := DirAccess.open(path) if dir == null: - print("AudioManager: res://audio/ not found — all play methods no-op") + if path == "res://audio/": + print("AudioManager: res://audio/ not found — all play methods no-op") return dir.list_dir_begin() var file_name := dir.get_next() while file_name != "": - if not dir.current_is_dir() and not file_name.ends_with(".import"): + var full_path := path.path_join(file_name) + if dir.current_is_dir(): + _scan_dir(full_path) + elif not file_name.ends_with(".import"): if file_name.get_extension().to_lower() in ["ogg", "wav", "mp3"]: - var stream := load("res://audio/" + file_name) as AudioStream + var stream := load(full_path) as AudioStream if stream: _registry[file_name.get_basename()] = stream file_name = dir.get_next() - print("AudioManager: %d assets registered" % _registry.size()) func has_asset(asset_key: String) -> bool: @@ -268,7 +279,8 @@ func get_volume(bus: String) -> float: # --- Zone crossfade (D-073 stub) --- ## Handle zone transition. Server sends zone_id per tile in ObserverSnapshot. -## Full crossfade implementation deferred — stub for integration point. +## Full crossfade implementation deferred to Sprint 9+ (D-073). +## Stub exists so server integration can call it without conditional checks. func set_zone(_zone_id: String) -> void: pass diff --git a/client/scripts/main.gd b/client/scripts/main.gd index 147d2c9ca..a6f56ae33 100644 --- a/client/scripts/main.gd +++ b/client/scripts/main.gd @@ -13,10 +13,6 @@ extends Node2D @onready var stance_indicator = $UILayer/StanceIndicator # D-053: z-layer 7 @onready var cursor_renderer = $UILayer/CursorRenderer # D-056: z-layer 7 -# Tracks the dialogue_id from dialogue_box to prevent consume-once race during fade-in. -# If a new snapshot arrives with null dialogue while fade-in is still running, -# we don't re-trigger show_dialogue because _last_dialogue_id still matches. -var _last_dialogue_id: int = 0 var _last_dialogue_npc_id: int = -1 # D-064: NPC entity_id for WalkAway input var _camera_anchored: bool = false var _last_monologue_tick: int = -1 # Prevent re-consuming monologue when same tick polled twice @@ -169,7 +165,6 @@ func _consume_dialogue() -> void: dlg.get("speech", ""), dlg.get("options", []) ) - _last_dialogue_id = dialogue_box.get_dialogue_id() GameState.current_dialogue = null diff --git a/client/tests/test_msgpack_boundaries.gd b/client/tests/test_msgpack_boundaries.gd index ed941e277..a2b9822d1 100644 --- a/client/tests/test_msgpack_boundaries.gd +++ b/client/tests/test_msgpack_boundaries.gd @@ -86,12 +86,12 @@ func test_encode_uint32() -> void: func test_encode_int64_positive() -> void: # BV-P24 to BV-P25: int 64 range (2^32 to 2^63-1) - # NOTE: The encoder's int_64 branch condition `-(1 << 63) <= v < (1 << 63)` + # KNOWN-DEFECT: The encoder's int_64 branch condition `-(1 << 63) <= v < (1 << 63)` # evaluates to `MIN_INT64 <= v < MIN_INT64` due to overflow, making it dead code. # Values that should be int_64 (0xd3) are instead encoded as uint_64 (0xcf). # Roundtrip still works because put_u64/get_u64 preserve the bit pattern. - # This test documents ACTUAL behavior. Fix the encoder condition to restore - # int_64 encoding (use explicit constant instead of `1 << 63`). + # This test documents ACTUAL behavior. Fix tracked in backlog. + # See: messagepack.gd int_64 branch — use explicit constant instead of `1 << 63`. _assert_encodes_to(4294967296, PackedByteArray([ 0xcf, 0x00, 0x00, 0x00, 0x01, 0x00, 0x00, 0x00, 0x00 ]), "BV-P24") @@ -138,7 +138,7 @@ func test_encode_int32_negative() -> void: func test_encode_int64_negative() -> void: # BV-N15 to BV-N16: int 64 range (< -2147483648) - # Same int_64 branch issue as positive int_64 — encoded as uint_64 (0xcf). + # KNOWN-DEFECT: Same int_64 branch issue as positive int_64 — encoded as uint_64 (0xcf). # Bit pattern is preserved: put_u64(negative) writes two's complement, # get_u64() reads it back and Variant stores as int64 with same bit pattern. _assert_encodes_to(-2147483649, PackedByteArray([ diff --git a/client/ui/dialogue_box.gd b/client/ui/dialogue_box.gd index fff191cb5..a0683983f 100644 --- a/client/ui/dialogue_box.gd +++ b/client/ui/dialogue_box.gd @@ -23,7 +23,6 @@ var _option_response_ids: Array[String] = [] # response_id per option, same ind var _option_texts: Array[String] = [] # raw display text per option var _option_is_confrontation: Array[bool] = [] # confrontation flag per option var _npc_name: String = "" -var _dialogue_id: int = 0 # Tracks current dialogue to prevent consume-once race const FADE_IN: float = 0.2 const FADE_OUT: float = 0.3 # D-064: 300ms fade on walk-away @@ -32,7 +31,7 @@ const MAX_HEIGHT_RATIO: float = 0.2 # D-061: max 20% viewport height const MAX_WIDTH_PX: float = 832.0 # D-061: max-width cap (~65% of 1280) const CONFRONTATION_BEAT_DURATION: float = 1.5 # D-063: pause before sending const CONFRONTATION_DIM_ALPHA: float = 0.7 # D-063: dialogue box dims during beat -const CONFRONTATION_MONOLOGUE: String = "This changes things. No taking it back." +const CONFRONTATION_MONOLOGUE_KEY: String = "dialogue.confrontation_beat" # D-064: movement actions that trigger walk-away const _WALK_AWAY_ACTIONS: Array[StringName] = [ @@ -58,6 +57,7 @@ func _unhandled_input(event: InputEvent) -> void: if event is InputEventKey and event.pressed: for action in _WALK_AWAY_ACTIONS: if event.is_action_pressed(action): + get_viewport().set_input_as_handled() _cancel_beat() hide_dialogue() dialogue_dismissed.emit() @@ -82,7 +82,6 @@ func _update_layout() -> void: # D-063: confrontation options render italic. func show_dialogue(npc_name: String, speech: String, options: Array = []) -> void: _npc_name = npc_name - _dialogue_id += 1 _cancel_beat() # NPC speech — name prefix in bold @@ -158,7 +157,7 @@ func hide_dialogue() -> void: mouse_filter = Control.MOUSE_FILTER_IGNORE # D-061: unpause when dialogue closes - SimBridge.send_input({"action": InputMapper.Action.PAUSE, "timestamp_msec": Time.get_ticks_msec()}) + SimBridge.send_input({"action": InputMapper.Action.UNPAUSE, "timestamp_msec": Time.get_ticks_msec()}) if _active_tween and _active_tween.is_valid(): _active_tween.kill() @@ -172,11 +171,7 @@ func hide_dialogue() -> void: func is_dialogue_active() -> bool: - return _is_showing - - -func get_dialogue_id() -> int: - return _dialogue_id + return _is_showing or GameState.dialogue_active func _on_option_pressed(index: int) -> void: @@ -209,8 +204,8 @@ func _start_confrontation_beat(response_id: String, text: String) -> void: _active_tween = create_tween() _active_tween.tween_property(panel, "modulate:a", CONFRONTATION_DIM_ALPHA, 0.2) - # D-063: monologue beat — hardcoded v0.1 line - confrontation_monologue.emit(CONFRONTATION_MONOLOGUE, CONFRONTATION_BEAT_DURATION) + # D-063: monologue beat — text from ui-strings.yaml (D-042) + confrontation_monologue.emit(UIStrings.get_text(CONFRONTATION_MONOLOGUE_KEY), CONFRONTATION_BEAT_DURATION) # D-063: audio dip via AudioManager AudioManager.apply_dip("confrontation")