fix(client): address PR #25 review — 5 critical bugs, 4 warnings, 3 suggestions
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 <noreply@anthropic.com>
This commit is contained in:
@@ -144,6 +144,14 @@ tutorial_prompts:
|
|||||||
minimap_hint: "The overlay shows the area around you."
|
minimap_hint: "The overlay shows the area around you."
|
||||||
knowledge_hint: "Open your notes to review what you know."
|
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
|
# MENU AND SYSTEM TEXT
|
||||||
# Non-diegetic. Standard game UI. Clean, no flavor text.
|
# Non-diegetic. Standard game UI. Clean, no flavor text.
|
||||||
|
|||||||
@@ -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.
|
# D-069: Audio dip profiles — dB offsets relative to player slider setting.
|
||||||
# Mid-range of spec values used (e.g. -6 to -8 → -7).
|
# 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 := {
|
const DIP_SPECS := {
|
||||||
"dialogue": {
|
"dialogue": {
|
||||||
"ease_in": 0.3, "ease_out": 0.5,
|
"ease_in": 0.3, "ease_out": 0.5,
|
||||||
@@ -70,8 +72,9 @@ func _setup_buses() -> void:
|
|||||||
_bus_volumes[bus_name] = 0.0
|
_bus_volumes[bus_name] = 0.0
|
||||||
# Low-pass filter on Ambient bus — cutoff starts high (bypassed).
|
# Low-pass filter on Ambient bus — cutoff starts high (bypassed).
|
||||||
# Confrontation dip sweeps it down to 800 Hz (D-069).
|
# 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)
|
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 = AudioEffectLowPassFilter.new()
|
||||||
_ambient_filter.cutoff_hz = FILTER_CUTOFF_DEFAULT
|
_ambient_filter.cutoff_hz = FILTER_CUTOFF_DEFAULT
|
||||||
AudioServer.add_bus_effect(amb_idx, _ambient_filter)
|
AudioServer.add_bus_effect(amb_idx, _ambient_filter)
|
||||||
@@ -80,20 +83,28 @@ func _setup_buses() -> void:
|
|||||||
# --- Asset registry (D-068 directory-scan pattern) ---
|
# --- Asset registry (D-068 directory-scan pattern) ---
|
||||||
|
|
||||||
func _scan_registry() -> void:
|
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:
|
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
|
return
|
||||||
dir.list_dir_begin()
|
dir.list_dir_begin()
|
||||||
var file_name := dir.get_next()
|
var file_name := dir.get_next()
|
||||||
while file_name != "":
|
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"]:
|
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:
|
if stream:
|
||||||
_registry[file_name.get_basename()] = stream
|
_registry[file_name.get_basename()] = stream
|
||||||
file_name = dir.get_next()
|
file_name = dir.get_next()
|
||||||
print("AudioManager: %d assets registered" % _registry.size())
|
|
||||||
|
|
||||||
|
|
||||||
func has_asset(asset_key: String) -> bool:
|
func has_asset(asset_key: String) -> bool:
|
||||||
@@ -268,7 +279,8 @@ func get_volume(bus: String) -> float:
|
|||||||
# --- Zone crossfade (D-073 stub) ---
|
# --- Zone crossfade (D-073 stub) ---
|
||||||
|
|
||||||
## Handle zone transition. Server sends zone_id per tile in ObserverSnapshot.
|
## 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:
|
func set_zone(_zone_id: String) -> void:
|
||||||
pass
|
pass
|
||||||
|
|
||||||
|
|||||||
@@ -13,10 +13,6 @@ extends Node2D
|
|||||||
@onready var stance_indicator = $UILayer/StanceIndicator # D-053: z-layer 7
|
@onready var stance_indicator = $UILayer/StanceIndicator # D-053: z-layer 7
|
||||||
@onready var cursor_renderer = $UILayer/CursorRenderer # D-056: 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 _last_dialogue_npc_id: int = -1 # D-064: NPC entity_id for WalkAway input
|
||||||
var _camera_anchored: bool = false
|
var _camera_anchored: bool = false
|
||||||
var _last_monologue_tick: int = -1 # Prevent re-consuming monologue when same tick polled twice
|
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("speech", ""),
|
||||||
dlg.get("options", [])
|
dlg.get("options", [])
|
||||||
)
|
)
|
||||||
_last_dialogue_id = dialogue_box.get_dialogue_id()
|
|
||||||
GameState.current_dialogue = null
|
GameState.current_dialogue = null
|
||||||
|
|
||||||
|
|
||||||
|
|||||||
@@ -86,12 +86,12 @@ func test_encode_uint32() -> void:
|
|||||||
|
|
||||||
func test_encode_int64_positive() -> void:
|
func test_encode_int64_positive() -> void:
|
||||||
# BV-P24 to BV-P25: int 64 range (2^32 to 2^63-1)
|
# 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.
|
# 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).
|
# 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.
|
# Roundtrip still works because put_u64/get_u64 preserve the bit pattern.
|
||||||
# This test documents ACTUAL behavior. Fix the encoder condition to restore
|
# This test documents ACTUAL behavior. Fix tracked in backlog.
|
||||||
# int_64 encoding (use explicit constant instead of `1 << 63`).
|
# See: messagepack.gd int_64 branch — use explicit constant instead of `1 << 63`.
|
||||||
_assert_encodes_to(4294967296, PackedByteArray([
|
_assert_encodes_to(4294967296, PackedByteArray([
|
||||||
0xcf, 0x00, 0x00, 0x00, 0x01, 0x00, 0x00, 0x00, 0x00
|
0xcf, 0x00, 0x00, 0x00, 0x01, 0x00, 0x00, 0x00, 0x00
|
||||||
]), "BV-P24")
|
]), "BV-P24")
|
||||||
@@ -138,7 +138,7 @@ func test_encode_int32_negative() -> void:
|
|||||||
|
|
||||||
func test_encode_int64_negative() -> void:
|
func test_encode_int64_negative() -> void:
|
||||||
# BV-N15 to BV-N16: int 64 range (< -2147483648)
|
# 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,
|
# 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.
|
# get_u64() reads it back and Variant stores as int64 with same bit pattern.
|
||||||
_assert_encodes_to(-2147483649, PackedByteArray([
|
_assert_encodes_to(-2147483649, PackedByteArray([
|
||||||
|
|||||||
@@ -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_texts: Array[String] = [] # raw display text per option
|
||||||
var _option_is_confrontation: Array[bool] = [] # confrontation flag per option
|
var _option_is_confrontation: Array[bool] = [] # confrontation flag per option
|
||||||
var _npc_name: String = ""
|
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_IN: float = 0.2
|
||||||
const FADE_OUT: float = 0.3 # D-064: 300ms fade on walk-away
|
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 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_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_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
|
# D-064: movement actions that trigger walk-away
|
||||||
const _WALK_AWAY_ACTIONS: Array[StringName] = [
|
const _WALK_AWAY_ACTIONS: Array[StringName] = [
|
||||||
@@ -58,6 +57,7 @@ func _unhandled_input(event: InputEvent) -> void:
|
|||||||
if event is InputEventKey and event.pressed:
|
if event is InputEventKey and event.pressed:
|
||||||
for action in _WALK_AWAY_ACTIONS:
|
for action in _WALK_AWAY_ACTIONS:
|
||||||
if event.is_action_pressed(action):
|
if event.is_action_pressed(action):
|
||||||
|
get_viewport().set_input_as_handled()
|
||||||
_cancel_beat()
|
_cancel_beat()
|
||||||
hide_dialogue()
|
hide_dialogue()
|
||||||
dialogue_dismissed.emit()
|
dialogue_dismissed.emit()
|
||||||
@@ -82,7 +82,6 @@ func _update_layout() -> void:
|
|||||||
# D-063: confrontation options render italic.
|
# D-063: confrontation options render italic.
|
||||||
func show_dialogue(npc_name: String, speech: String, options: Array = []) -> void:
|
func show_dialogue(npc_name: String, speech: String, options: Array = []) -> void:
|
||||||
_npc_name = npc_name
|
_npc_name = npc_name
|
||||||
_dialogue_id += 1
|
|
||||||
_cancel_beat()
|
_cancel_beat()
|
||||||
|
|
||||||
# NPC speech — name prefix in bold
|
# NPC speech — name prefix in bold
|
||||||
@@ -158,7 +157,7 @@ func hide_dialogue() -> void:
|
|||||||
mouse_filter = Control.MOUSE_FILTER_IGNORE
|
mouse_filter = Control.MOUSE_FILTER_IGNORE
|
||||||
|
|
||||||
# D-061: unpause when dialogue closes
|
# 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():
|
if _active_tween and _active_tween.is_valid():
|
||||||
_active_tween.kill()
|
_active_tween.kill()
|
||||||
@@ -172,11 +171,7 @@ func hide_dialogue() -> void:
|
|||||||
|
|
||||||
|
|
||||||
func is_dialogue_active() -> bool:
|
func is_dialogue_active() -> bool:
|
||||||
return _is_showing
|
return _is_showing or GameState.dialogue_active
|
||||||
|
|
||||||
|
|
||||||
func get_dialogue_id() -> int:
|
|
||||||
return _dialogue_id
|
|
||||||
|
|
||||||
|
|
||||||
func _on_option_pressed(index: int) -> void:
|
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 = create_tween()
|
||||||
_active_tween.tween_property(panel, "modulate:a", CONFRONTATION_DIM_ALPHA, 0.2)
|
_active_tween.tween_property(panel, "modulate:a", CONFRONTATION_DIM_ALPHA, 0.2)
|
||||||
|
|
||||||
# D-063: monologue beat — hardcoded v0.1 line
|
# D-063: monologue beat — text from ui-strings.yaml (D-042)
|
||||||
confrontation_monologue.emit(CONFRONTATION_MONOLOGUE, CONFRONTATION_BEAT_DURATION)
|
confrontation_monologue.emit(UIStrings.get_text(CONFRONTATION_MONOLOGUE_KEY), CONFRONTATION_BEAT_DURATION)
|
||||||
|
|
||||||
# D-063: audio dip via AudioManager
|
# D-063: audio dip via AudioManager
|
||||||
AudioManager.apply_dip("confrontation")
|
AudioManager.apply_dip("confrontation")
|
||||||
|
|||||||
Reference in New Issue
Block a user