From 952f994d59d683bb174f632bdf862ecf84fa6f3f Mon Sep 17 00:00:00 2001 From: Jeroen Schweitzer Date: Fri, 20 Feb 2026 20:39:12 +0100 Subject: [PATCH] =?UTF-8?q?fix(ui):=20address=20PR=20#52=20review=20?= =?UTF-8?q?=E2=80=94=2010=20items=20across=20Hoshe,=20Tyre,=20Araminta=20(?= =?UTF-8?q?#535)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Critical: bump PROTOCOL_VERSION 8→9 for conversation_events/ended fields. Hoshe review: - Dirty flag (_log_dirty) prevents per-frame O(n) BBCode rebuild - BBCode injection: _escape_bbcode() replaces [ with [lb] on server text - D-064 regression: dialogue_active cleared in fade callback, not before - YAML quoting: remove unnecessary quotes from numeric values Tyre review: - Carry-forward for dialogue_response, conversation_events, conversation_ended in receive_bytes() — arrays merge, scalar falls through - pause_requested/unpause_requested signals route through main.gd input recording (_pending_record_inputs) for #507 replay determinism - Fix version comments: dialogue_response is v8 (#305), not v9 - Remove dead _active_overheard dictionary Araminta review: - Passive lines: ┃ glyph prefix + _desaturate() for name colours - Active conversation entries pinned (no timeout), unpinned with timestamp reset on conversation end - _enforce_contrast(): minimum luminance floor for name colour readability - Simplified 1-on-1 attribution: "Speaker:" instead of "Speaker → You:" Co-Authored-By: Claude Opus 4.6 --- client/data/dialogue-theme.yaml | 6 +- client/scripts/autoloads/game_state.gd | 6 +- client/scripts/autoloads/sim_bridge.gd | 11 ++ client/scripts/main.gd | 16 +++ client/scripts/protocol/protocol.gd | 2 +- client/ui/dialogue_box.gd | 180 ++++++++++++++++++------- 6 files changed, 170 insertions(+), 51 deletions(-) diff --git a/client/data/dialogue-theme.yaml b/client/data/dialogue-theme.yaml index d45b91aec..bab788961 100644 --- a/client/data/dialogue-theme.yaml +++ b/client/data/dialogue-theme.yaml @@ -37,11 +37,11 @@ speech_color: "#c8d0e0" # Base opacity multiplier for non-player-centric lines. # 1.0 = full opacity, 0.0 = invisible. # ============================================================ -passive_opacity: "0.9" +passive_opacity: 0.9 # ============================================================ # ENTRY TIMING # All entry types share the same lifetime and fade duration. # ============================================================ -entry_lifetime_seconds: "15.0" -entry_fade_seconds: "3.0" +entry_lifetime_seconds: 15.0 +entry_fade_seconds: 3.0 diff --git a/client/scripts/autoloads/game_state.gd b/client/scripts/autoloads/game_state.gd index 1fd486ec3..9c1f82920 100644 --- a/client/scripts/autoloads/game_state.gd +++ b/client/scripts/autoloads/game_state.gd @@ -59,10 +59,12 @@ var rng_seed: Variant = null # v7 fields (#431, D-059/D-060) var pending_recognitions: Array = [] # [{entity_id, x, y, z, remaining_ticks, total_delay_ticks}] +# v8 fields (#305, D-028): NPC follow-up after player dialogue choice +var dialogue_response: Variant = null # {line_id, text, speaker_entity_id} + # v9 fields (#535, D-078): Overheard NPC-to-NPC conversations var conversation_events: Array = [] # [{speaker_id, target_id, speaker_name, target_name, occluded_line}] var conversation_ended: Array = [] # [{speaker_id, target_id}] -var dialogue_response: Variant = null # {line_id, text, speaker_entity_id} — NPC follow-up after player choice # #126, D-018: Medium-range sound events for fog-edge directional indicators. # Format: [{x, y, event_type, range_category}] — server sends current medium events per tick. @@ -186,7 +188,7 @@ func apply_snapshot(snapshot: Dictionary) -> void: else: conversation_ended = [] - # v9: dialogue_response (#535, D-028) — NPC follow-up after player choice + # v8: dialogue_response (#305, D-028) — NPC follow-up after player choice if snapshot.has("dialogue_response") and snapshot.dialogue_response is Dictionary: dialogue_response = snapshot.dialogue_response else: diff --git a/client/scripts/autoloads/sim_bridge.gd b/client/scripts/autoloads/sim_bridge.gd index 39df6d43a..fa91fb3c3 100644 --- a/client/scripts/autoloads/sim_bridge.gd +++ b/client/scripts/autoloads/sim_bridge.gd @@ -241,6 +241,17 @@ func receive_bytes(bytes: PackedByteArray) -> void: snapshot["current_monologue"] = _last_snapshot["current_monologue"] if snapshot.get("current_dialogue") == null and _last_snapshot.get("current_dialogue") != null: snapshot["current_dialogue"] = _last_snapshot["current_dialogue"] + # #535: Carry forward one-shot dialogue events (arrays merge, scalar falls through) + if snapshot.get("dialogue_response") == null and _last_snapshot.get("dialogue_response") != null: + snapshot["dialogue_response"] = _last_snapshot["dialogue_response"] + var old_conv_events: Array = _last_snapshot.get("conversation_events", []) + if old_conv_events.size() > 0: + var new_conv_events: Array = snapshot.get("conversation_events", []) + snapshot["conversation_events"] = old_conv_events + new_conv_events + var old_conv_ended: Array = _last_snapshot.get("conversation_ended", []) + if old_conv_ended.size() > 0: + var new_conv_ended: Array = snapshot.get("conversation_ended", []) + snapshot["conversation_ended"] = old_conv_ended + new_conv_ended _last_snapshot = snapshot # Drain the outbound buffer. Returns raw input entries for batch encoding. diff --git a/client/scripts/main.gd b/client/scripts/main.gd index 188404165..2255a47c9 100644 --- a/client/scripts/main.gd +++ b/client/scripts/main.gd @@ -62,6 +62,8 @@ func _ready() -> void: dialogue_box.option_selected.connect(_on_dialogue_option_selected) dialogue_box.dialogue_dismissed.connect(_on_dialogue_dismissed) dialogue_box.confrontation_monologue.connect(_on_confrontation_monologue) + dialogue_box.pause_requested.connect(_on_dialogue_pause_requested) + dialogue_box.unpause_requested.connect(_on_dialogue_unpause_requested) # #496: Print gauntlet session summary on disconnect if gauntlet_hud: @@ -389,6 +391,20 @@ func _on_confrontation_monologue(text: String, duration: float) -> void: monologue_display.show_monologue(text, duration, 3, true) +# D-061: Auto-pause on dialogue open — routed through input recording (#507, Tyre #3) +func _on_dialogue_pause_requested() -> void: + var input := {"action": InputMapper.Action.PAUSE, "timestamp_msec": Time.get_ticks_msec()} + SimBridge.send_input(input) + _pending_record_inputs.append(input) + + +# D-061: Auto-unpause on dialogue close — routed through input recording (#507, Tyre #3) +func _on_dialogue_unpause_requested() -> void: + var input := {"action": InputMapper.Action.UNPAUSE, "timestamp_msec": Time.get_ticks_msec()} + SimBridge.send_input(input) + _pending_record_inputs.append(input) + + # D-064: Handle walk-away → send WalkAway{npc_id} to server func _on_dialogue_dismissed() -> void: SimBridge.send_input({ diff --git a/client/scripts/protocol/protocol.gd b/client/scripts/protocol/protocol.gd index c21020776..65a440dec 100644 --- a/client/scripts/protocol/protocol.gd +++ b/client/scripts/protocol/protocol.gd @@ -11,7 +11,7 @@ class_name Protocol ## Protocol version — must match server PROTOCOL_VERSION in bridge/types.rs. ## Reject snapshots where version != this value. -const PROTOCOL_VERSION: int = 8 +const PROTOCOL_VERSION: int = 9 # -- Decode: bytes from server → GDScript types -------------------------------- diff --git a/client/ui/dialogue_box.gd b/client/ui/dialogue_box.gd index 619e106f7..6b7d1d7c0 100644 --- a/client/ui/dialogue_box.gd +++ b/client/ui/dialogue_box.gd @@ -7,20 +7,25 @@ extends Control # InsertOverlay (CanvasLayer 10, z-layer 6) — diegetic, insert-styled. # D-063: Confrontation options render italic, trigger monologue beat before send. # D-064: Walk-away via WASD dismisses active conversation (log entries persist). -# D-078: Overheard NPC-NPC lines shown at reduced opacity, no response options. +# dialogue_active held until fade completes (D-064 auto-pause spec). +# D-078: Overheard NPC-NPC lines prefixed with ┃ glyph + desaturated colours. signal option_selected(response_id: String, text: String) signal dialogue_dismissed # Walk-away or conversation end signal confrontation_monologue(text: String, duration: float) # D-063: beat monologue +signal pause_requested # D-061: auto-pause — main.gd routes through input recording (#507) +signal unpause_requested # D-061: auto-unpause @onready var panel: PanelContainer = $PanelContainer @onready var dialogue_log: RichTextLabel = $PanelContainer/MarginContainer/VBoxContainer/DialogueLog @onready var options_container: VBoxContainer = $PanelContainer/MarginContainer/VBoxContainer/OptionsContainer # -- Log state -- -var _log_entries: Array[Dictionary] = [] # {speaker, target, text, is_passive, timestamp_msec} -var _active_overheard: Dictionary = {} # "speaker_id:target_id" -> true +# Entry format: {speaker, target, text, is_passive, pinned, timestamp_msec} +# pinned entries do not expire (active conversation lines, Araminta review). +var _log_entries: Array[Dictionary] = [] var _in_player_conversation: bool = false +var _log_dirty: bool = false # Dirty flag — prevents per-frame O(n) BBCode rebuild (Hoshe #1) # -- Option state -- var _option_controls: Array[Control] = [] @@ -54,6 +59,9 @@ 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_KEY: String = "dialogue.confrontation_beat" const PLAYER_NAME: String = "You" +const PASSIVE_GLYPH: String = "\u2503 " # ┃ prefix for overheard lines (Araminta review) +const PASSIVE_DESATURATION: float = 0.4 # Desaturate passive name colours by this factor +const MIN_CONTRAST_LUMINANCE: float = 0.25 # Floor for name colour brightness against dark BG # D-064: movement actions that trigger walk-away const _WALK_AWAY_ACTIONS: Array[StringName] = [ @@ -74,6 +82,9 @@ func _ready() -> void: func _process(_delta: float) -> void: _expire_entries() + if _log_dirty: + _log_dirty = false + _rebuild_log() func _unhandled_input(event: InputEvent) -> void: @@ -152,16 +163,19 @@ func _update_layout() -> void: ## Append a dialogue line to the log. ## speaker/target: display names. text: the spoken line. -## is_passive: true for overheard NPC-NPC (renders at reduced opacity). +## is_passive: true for overheard NPC-NPC (renders with ┃ prefix + desaturated). +## Active conversation entries are pinned (no timeout) while _in_player_conversation. func append_line(speaker: String, target: String, text: String, is_passive: bool = false) -> void: + var pinned := not is_passive and _in_player_conversation _log_entries.append({ "speaker": speaker, "target": target, "text": text, "is_passive": is_passive, + "pinned": pinned, "timestamp_msec": Time.get_ticks_msec(), }) - _rebuild_log() + _log_dirty = true _ensure_visible() @@ -172,18 +186,12 @@ func append_conversation_event(event: Dictionary) -> void: var text: String = event.get("occluded_line", "") if text.is_empty(): return - var pair_key := "%s:%s" % [event.get("speaker_id", 0), event.get("target_id", 0)] - _active_overheard[pair_key] = true append_line(speaker, target, text, true) -## Handle conversation_ended — remove pair from active tracking. -func on_conversation_ended(event: Dictionary) -> void: - var pair_key := "%s:%s" % [event.get("speaker_id", 0), event.get("target_id", 0)] - _active_overheard.erase(pair_key) - # Also try the reverse pair - var reverse_key := "%s:%s" % [event.get("target_id", 0), event.get("speaker_id", 0)] - _active_overheard.erase(reverse_key) +## Handle conversation_ended — no-op currently (entries expire via timeout). +func on_conversation_ended(_event: Dictionary) -> void: + pass ## Append the player's chosen response to the log. @@ -221,23 +229,36 @@ func show_dialogue(npc_name: String, speech: String, options: Array = []) -> voi # D-069: Dialogue dip AudioManager.apply_dip("dialogue") - # D-061: auto-pause in single-player - SimBridge.send_input({"action": InputMapper.Action.PAUSE, "timestamp_msec": Time.get_ticks_msec()}) + # D-061: auto-pause — signal to main.gd for input recording (#507) + pause_requested.emit() ## End active player conversation — clears options but preserves log. +## D-064: dialogue_active held until fade completes. func _end_player_conversation() -> void: _in_player_conversation = false _clear_options() mouse_filter = Control.MOUSE_FILTER_IGNORE + # Unpin all entries and reset their timestamps so timeout starts now (Araminta review) + var now := Time.get_ticks_msec() + for entry in _log_entries: + if entry.pinned: + entry.pinned = false + entry.timestamp_msec = now + _log_dirty = true + # D-069: Clear dialogue/confrontation dip AudioManager.clear_dip() - # D-061: unpause - SimBridge.send_input({"action": InputMapper.Action.UNPAUSE, "timestamp_msec": Time.get_ticks_msec()}) + # D-061: unpause — signal to main.gd for input recording (#507) + unpause_requested.emit() - GameState.dialogue_active = false + # D-064: dialogue_active stays true — cleared after fade completes in hide callback. + # If log entries still exist, panel stays visible and entries expire via timeout. + # If no entries, hide immediately with fade. + if _log_entries.is_empty(): + hide_dialogue() ## Hide the entire panel with fade. @@ -247,6 +268,7 @@ func hide_dialogue() -> void: if _in_player_conversation: _end_player_conversation() + return # _end_player_conversation may call hide_dialogue if log is empty _is_showing = false @@ -256,6 +278,7 @@ func hide_dialogue() -> void: _active_tween.tween_property(panel, "modulate:a", 0.0, FADE_OUT) _active_tween.tween_callback(func(): visible = false + GameState.dialogue_active = false # D-064: unblock movement after fade completes ) @@ -364,10 +387,11 @@ func _rebuild_log() -> void: var now := Time.get_ticks_msec() var bbcode := "" for entry in _log_entries: - var age_sec: float = (now - entry.timestamp_msec) / 1000.0 var alpha: float = 1.0 - if age_sec > _entry_lifetime: - alpha = clampf(1.0 - (age_sec - _entry_lifetime) / _entry_fade, 0.0, 1.0) + if not entry.pinned: + var age_sec: float = (now - entry.timestamp_msec) / 1000.0 + if age_sec > _entry_lifetime: + alpha = clampf(1.0 - (age_sec - _entry_lifetime) / _entry_fade, 0.0, 1.0) if entry.is_passive: alpha *= _passive_opacity if alpha <= 0.0: @@ -380,32 +404,83 @@ func _rebuild_log() -> void: ## Format a single log entry as BBCode. +## Hoshe #2: escape BBCode brackets in server-sourced strings. +## Araminta: passive lines get ┃ prefix + desaturated colours. +## Non-blocking: 1-on-1 player dialogue simplifies to "Speaker:" (no → You). func _format_entry(entry: Dictionary, alpha: float) -> String: + var speaker: String = _escape_bbcode(entry.speaker) + var target: String = _escape_bbcode(entry.target) + var text: String = _escape_bbcode(entry.text) + var is_passive: bool = entry.is_passive + var speaker_color := _color_for_name(entry.speaker) var target_color := _color_for_name(entry.target) - # Apply alpha to all colors in this line + # Desaturate passive name colours (Araminta review) + if is_passive: + speaker_color = _desaturate(speaker_color, PASSIVE_DESATURATION) + target_color = _desaturate(target_color, PASSIVE_DESATURATION) + var sc := _color_with_alpha(speaker_color, alpha) - var tc := _color_with_alpha(target_color, alpha) var ac := _color_with_alpha(_arrow_color, alpha) + var tc := _color_with_alpha(target_color, alpha) var txc := _color_with_alpha(_speech_color, alpha) - return "[color=%s][b]%s[/b][/color][color=%s] \u2192 [/color][color=%s][b]%s[/b][/color][color=%s]: %s[/color]" % [ - sc, entry.speaker, ac, tc, entry.target, txc, entry.text - ] + var prefix := PASSIVE_GLYPH if is_passive else "" + + # Non-blocking: simplify 1-on-1 player dialogue — no arrow for Speaker → You or You → Speaker + var involves_player := entry.speaker == PLAYER_NAME or entry.target == PLAYER_NAME + if involves_player and not is_passive: + # Just "Speaker: text" or "You: text" + return "%s[color=%s][b]%s[/b][/color][color=%s]: %s[/color]" % [ + prefix, sc, speaker, txc, text + ] + else: + # Full "Speaker → Target: text" for overheard + return "%s[color=%s][b]%s[/b][/color][color=%s] \u2192 [/color][color=%s][b]%s[/b][/color][color=%s]: %s[/color]" % [ + prefix, sc, speaker, ac, tc, target, txc, text + ] -## Get a stable color for a character name. +## Escape BBCode bracket characters in server-sourced text (Hoshe #2). +static func _escape_bbcode(text: String) -> String: + return text.replace("[", "[lb]") + + +## Get a stable color for a character name, with contrast floor enforcement. func _color_for_name(char_name: String) -> Color: if char_name == PLAYER_NAME: return _player_color if _npc_colors.is_empty(): return _speech_color - var idx := char_name.hash() % _npc_colors.size() - # hash() can return negative in GDScript — use abs - if idx < 0: - idx = -idx - return _npc_colors[idx] + var idx := absi(char_name.hash()) % _npc_colors.size() + var color := _npc_colors[idx] + return _enforce_contrast(color) + + +## Enforce minimum luminance so name colours remain readable against dark BG (Araminta #3). +static func _enforce_contrast(color: Color) -> Color: + var lum := color.r * 0.299 + color.g * 0.587 + color.b * 0.114 + if lum < MIN_CONTRAST_LUMINANCE: + var boost := MIN_CONTRAST_LUMINANCE / maxf(lum, 0.001) + return Color( + minf(color.r * boost, 1.0), + minf(color.g * boost, 1.0), + minf(color.b * boost, 1.0), + color.a + ) + return color + + +## Desaturate a colour by a factor (0.0 = no change, 1.0 = full greyscale). +static func _desaturate(color: Color, amount: float) -> Color: + var grey := color.r * 0.299 + color.g * 0.587 + color.b * 0.114 + return Color( + lerpf(color.r, grey, amount), + lerpf(color.g, grey, amount), + lerpf(color.b, grey, amount), + color.a + ) ## Convert a Color to a hex string with alpha baked in. @@ -415,29 +490,44 @@ static func _color_with_alpha(base: Color, alpha: float) -> String: # -- Entry expiry -- -## Remove fully expired entries. Hide panel if nothing remains. +## Remove fully expired entries. Mark dirty if any fading entries exist. +## Pinned entries (active conversation) skip expiry entirely (Araminta #2). func _expire_entries() -> void: if _log_entries.is_empty(): return var now := Time.get_ticks_msec() var total_lifetime_msec: int = int((_entry_lifetime + _entry_fade) * 1000.0) - var had_entries := _log_entries.size() > 0 + var removed := false + var has_fading := false - # Remove expired entries from the front (oldest first) + # Remove expired non-pinned entries from the front (oldest first) while _log_entries.size() > 0: - var age: int = now - _log_entries[0].timestamp_msec + var entry: Dictionary = _log_entries[0] + if entry.pinned: + break # Pinned entries never expire + var age: int = now - entry.timestamp_msec if age < total_lifetime_msec: + # Check if this entry is in the fading phase + if age > int(_entry_lifetime * 1000.0): + has_fading = true break _log_entries.remove_at(0) + removed = true - # Rebuild if entries were removed - if had_entries and _log_entries.size() == 0: - _rebuild_log() - if not _in_player_conversation: - hide_dialogue() - elif had_entries: - # Rebuild to update fading entries - _rebuild_log() + # Check remaining entries for fading state + if not has_fading: + for entry in _log_entries: + if not entry.pinned: + var age: int = now - entry.timestamp_msec + if age > int(_entry_lifetime * 1000.0): + has_fading = true + break + + if removed or has_fading: + _log_dirty = true + + if removed and _log_entries.is_empty() and not _in_player_conversation: + hide_dialogue() # -- Visibility --