jpmschweitzer/settled-reach
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

fix(ui): address PR #49 review — 6 warnings + 2 suggestions (#122)

Browse Source 
Warnings fixed:
- BBCode injection (line 153): escape [ → [lb] in server text before interpolation
- sort_custom on silent-drop (line 125): sort now only runs on actual insertion/replacement
- clip_contents: add clip_contents=true to MonologueDisplay Control (overflow guard)
- confrontation tick guard (main.gd): _last_confrontation_tick deduplicates same-tick signals
- GameState decoupling: show_monologue() reads lattice_profile once and passes it through
  _show_line() → _build_line_node(); renderer no longer reaches into autoload (D-020)
- Equal-priority eviction: >= tiebreak (was >); FIFO for equal-priority queue overflow

Suggestions fixed:
- Minimum duration clamp: maxf(duration, FADE_IN_SEC + 0.1) — line survives own fade-in
- _label_text bounds check: guard against empty _visible before indexing [0]

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
This commit is contained in:
jpmschweitzer
2026-02-20 19:06:26 +01:00
co-authored by Claude Sonnet 4.6
parent c71ba8ebb3
commit 677ca9b59d
4 changed files with 69 additions and 30 deletions
Download Patch File Download Diff File Expand all files Collapse all files
client/scripts/main.gd
+9 -3
View File
@@ -19,8 +19,9 @@ extends Node2D
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
var _last_monologue_tick: int = -1 # Prevent re-consuming monologue when same tick polled twice
var _last_dialogue_tick: int = -1
var _last_confrontation_tick: int = -1 # Deduplicate confrontation_monologue signals within same tick
var _known_recognition_ids: Dictionary = {} # D-067: entity_ids that have already chimed
var _flash_rect: ColorRect = null # #502/#501: ephemeral screen flash overlay (shared: teleport preempts amber)
var _teleport_in_progress: bool = false # #501: defer smoothing re-enable by one frame after teleport
@@ -338,9 +339,14 @@ func _on_dialogue_option_selected(response_id: String, text: String) -> void:
# D-063: Handle confrontation beat monologue → show on monologue display (layer 7)
# Confrontation lines are high-priority (3) and urgent — full opacity, elevated colour.
# Tick guard deduplicates if dialogue box emits the signal multiple times in one tick.
func _on_confrontation_monologue(text: String, duration: float) -> void:
if monologue_display:
monologue_display.show_monologue(text, duration, 3, true)
if not monologue_display:
return
if GameState.current_tick == _last_confrontation_tick:
return
_last_confrontation_tick = GameState.current_tick
monologue_display.show_monologue(text, duration, 3, true)
# D-064: Handle walk-away → send WalkAway{npc_id} to server
client/tests/test_monologue_display.gd
+27 -2
View File
@@ -23,6 +23,8 @@ func _make_display() -> Node:
func _label_text(d: Node) -> String:
if d._visible.is_empty():
return ""
var slot_node: Node = d._visible[0].node
return (slot_node.get_child(0) as RichTextLabel).text
@@ -456,10 +458,33 @@ func test_gamestate_monologue_replaced_by_next_snapshot() -> void:
# ---------------------------------------------------------------------------
# D-049 Z-layer / canvas scope compliance
# Monologue display must be parented to CANVAS_UI (CanvasLayer 20), not the
# world layer. This constant check ensures it hasn't silently drifted.
# MonologueDisplay must be parented to UILayer (CanvasLayer, layer=20).
# Two tests: (1) constant sanity, (2) scene tree structural verification.
# ---------------------------------------------------------------------------
func test_canvas_ui_constant_is_20() -> void:
## D-049: CANVAS_UI = 20 is the agreed HUD layer for monologue display.
## Sanity check — constant must not drift from the spec.
assert_that(Constants.CANVAS_UI).is_equal(20)
func test_monologue_display_parented_to_canvas_layer_20_in_main_scene() -> void:
## D-049: Structural verification — MonologueDisplay must be a direct child of
## UILayer (CanvasLayer, layer=20) in the live scene tree, not the world layer.
## Catches regressions where the node gets accidentally moved to InsertOverlay
## (layer=10) or ModalLayer (layer=30), or dropped into the world z-stack.
##
## Scene path verified: Game/UILayer/MonologueDisplay (main.tscn line 141).
if not ResourceLoader.exists("res://scenes/main.tscn"):
push_warning("TestMonologueDisplay: main.tscn not found — D-049 scene tree test skipped")
return
var scene: Node = load("res://scenes/main.tscn").instantiate()
auto_free(scene)
add_child(scene)
var mono: Node = scene.get_node_or_null("UILayer/MonologueDisplay")
assert_that(mono != null).is_true()
var parent: Node = mono.get_parent()
assert_that(parent is CanvasLayer).is_true()
assert_that((parent as CanvasLayer).layer).is_equal(Constants.CANVAS_UI)
client/ui/monologue_display.gd
+32 -25
View File
@@ -5,20 +5,22 @@ extends Control
#
# Up to MAX_VISIBLE lines display simultaneously in a VBoxContainer.
# Additional arrivals queue up to MAX_QUEUE depth; lowest-priority entry is
# dropped when the queue is full and a higher-priority line arrives.
# dropped when the queue is full and an equal-or-higher-priority line arrives
# (>= tiebreak = FIFO: newest replaces oldest at same priority).
#
# Stagger: 0.15s minimum gap between consecutive fade-ins (spec §5.4).
# Colour: derived from GameState.lattice_profile at render time (D-032).
# Colour: lattice_profile passed in at call time — no autoload access in renderer.
# is_urgent=true → opacity 1.0 and elevated colour variant (bloom deferred).
const MAX_VISIBLE: int = 3
const MAX_QUEUE: int = 5
const STAGGER_SEC: float = 0.15
const FADE_IN_SEC: float = 0.3
const FADE_OUT_SEC: float = 0.5
const STAGGER_SEC: float = 0.15
const FADE_IN_SEC: float = 0.3
const FADE_OUT_SEC: float = 0.5
const MIN_DURATION: float = FADE_IN_SEC + 0.1 # clamp: line must survive its own fade-in
# Lattice colour palette — keyed by GameState.lattice_profile.
# Lattice colour palette — keyed by lattice_profile passed from GameState at show time.
# standard opacity = 0.85, urgent opacity = 1.0.
# Source: Tyre architecture review, Sprint 14.
const _LATTICE_COLORS: Dictionary = {
@@ -38,7 +40,7 @@ const _FALLBACK_URGENT: Color = Color("#e0e8f8")
# Visible slot: {node: Control, expire_timer: float, priority: int, tween: Tween}
var _visible: Array[Dictionary] = []
# Queue entry: {text: String, duration: float, priority: int, is_urgent: bool}
# Queue entry: {text, duration, priority, is_urgent, lattice_profile}
var _queue: Array[Dictionary] = []
# Msec timestamp when the next fade-in may begin (stagger enforcement)
var _next_fade_in_msec: float = 0.0
@@ -60,34 +62,37 @@ func _process(delta: float) -> void:
var now := float(Time.get_ticks_msec())
if now >= _next_fade_in_msec:
var next: Dictionary = _queue.pop_front()
_show_line(next.text, next.duration, next.priority, next.is_urgent)
_show_line(next.text, next.duration, next.priority, next.is_urgent, next.lattice_profile)
# Display a monologue line.
# priority: higher number = more important (default 2; urgent beats normal).
# is_urgent: visual flag — full opacity + elevated colour. Bloom deferred.
# Empty text is silently ignored — no slot created, no queue entry.
# lattice_profile is read from GameState here and passed down — renderer stays
# decoupled from the autoload (D-020 renderer contract).
func show_monologue(text: String, duration: float, priority: int = 2, is_urgent: bool = false) -> void:
if text.is_empty():
return
var profile := GameState.lattice_profile
var now := float(Time.get_ticks_msec())
if _visible.size() < MAX_VISIBLE and now >= _next_fade_in_msec:
_show_line(text, duration, priority, is_urgent)
_show_line(text, duration, priority, is_urgent, profile)
else:
_enqueue(text, duration, priority, is_urgent)
_enqueue(text, duration, priority, is_urgent, profile)
# ---------------------------------------------------------------------------
# Internal
# ---------------------------------------------------------------------------
func _show_line(text: String, duration: float, priority: int, is_urgent: bool) -> void:
var line_node := _build_line_node(text, is_urgent)
func _show_line(text: String, duration: float, priority: int, is_urgent: bool, lattice_profile: String) -> void:
var line_node := _build_line_node(text, is_urgent, lattice_profile)
_vbox.add_child(line_node)
var slot := {
node = line_node,
expire_timer = duration,
expire_timer = maxf(duration, MIN_DURATION), # clamp: survives own fade-in
priority = priority,
tween = null as Tween,
}
@@ -112,17 +117,17 @@ func _retire_slot(slot: Dictionary) -> void:
tween.tween_callback(node.queue_free)
func _enqueue(text: String, duration: float, priority: int, is_urgent: bool) -> void:
func _enqueue(text: String, duration: float, priority: int, is_urgent: bool, lattice_profile: String) -> void:
if _queue.size() < MAX_QUEUE:
_queue.append({text = text, duration = duration, priority = priority, is_urgent = is_urgent})
_queue.append({text = text, duration = duration, priority = priority, is_urgent = is_urgent, lattice_profile = lattice_profile})
_queue.sort_custom(func(a: Dictionary, b: Dictionary) -> bool: return a.priority > b.priority)
else:
# Replace the lowest-priority queued entry if new one outranks it
# >= tiebreak: newest replaces oldest at equal priority (FIFO for equal ranks)
var lowest := _lowest_priority_idx()
if priority > _queue[lowest].priority:
_queue[lowest] = {text = text, duration = duration, priority = priority, is_urgent = is_urgent}
# else: incoming line is lower/equal priority — silently drop
# Re-sort: highest priority at front (next to display)
_queue.sort_custom(func(a: Dictionary, b: Dictionary) -> bool: return a.priority > b.priority)
if priority >= _queue[lowest].priority:
_queue[lowest] = {text = text, duration = duration, priority = priority, is_urgent = is_urgent, lattice_profile = lattice_profile}
_queue.sort_custom(func(a: Dictionary, b: Dictionary) -> bool: return a.priority > b.priority)
# else: incoming line is strictly lower priority — silently drop; no sort needed
func _lowest_priority_idx() -> int:
@@ -133,9 +138,8 @@ func _lowest_priority_idx() -> int:
return idx
func _build_line_node(text: String, is_urgent: bool) -> Control:
var profile: String = GameState.lattice_profile
var palette: Dictionary = _LATTICE_COLORS.get(profile, {})
func _build_line_node(text: String, is_urgent: bool, lattice_profile: String) -> Control:
var palette: Dictionary = _LATTICE_COLORS.get(lattice_profile, {})
var color: Color = palette.get("urgent", _FALLBACK_URGENT) if is_urgent \
else palette.get("standard", _FALLBACK_STANDARD)
@@ -150,7 +154,10 @@ func _build_line_node(text: String, is_urgent: bool) -> Control:
label.fit_content = true
label.scroll_active = false
label.add_theme_font_size_override("normal_font_size", 13)
label.text = "[i][color=#%s]%s[/color][/i]" % [color.to_html(false), text]
# Escape [ to prevent BBCode injection from server-sourced text.
# [lb] is Godot's BBCode entity for a literal left bracket.
var safe_text := text.replace("[", "[lb]")
label.text = "[i][color=#%s]%s[/color][/i]" % [color.to_html(false), safe_text]
container.add_child(label)
return container
client/ui/monologue_display.tscn
+1
View File
@@ -13,6 +13,7 @@ anchor_right = 0.55
anchor_bottom = 0.98
grow_horizontal = 1
grow_vertical = 0
clip_contents = true
mouse_filter = 2
script = ExtResource("1_monologue")