fix(client): address PR #24 review — 5 critical bugs, 3 warnings, 8 suggestions
Critical: - Protocol test assertions updated v6→v7 (test_protocol_v6.gd) - Dialogue signal connections wired (option_selected→DialogueResponse, dialogue_dismissed→DialogueEnd sent to server via SimBridge) - Consume-once race fixed: _consume_dialogue() checks is_dialogue_active() before re-showing; dialogue_id tracking prevents re-trigger during fade - Dialogue box responsive: _update_layout() clamps width to MAX_WIDTH_PX (832px) or 65% viewport, height to 20% viewport (MAX_HEIGHT_RATIO) - Auto-pause added: SimBridge.send_input(PAUSE) on dialogue open/close Warnings: - Test coverage: 16 new tests in test_protocol_v7.gd (pending_recognitions decode, current_dialogue, GameState, SimBridge mock data, insert colors) - queue_redraw() optimization: early return when no entities and no pings - Fixed 200px height → responsive 20% viewport via _update_layout() Suggestions: - WASD detection refactored to _WALK_AWAY_ACTIONS array loop - Button colors reference Constants.INSERT_COLOR_TEXT/HOVER/ACTIVE - Named constants: COLOR_TRANSITION_START, SILHOUETTE_APPEAR_THRESHOLD, SILHOUETTE_SIZE with explanatory comments - Bounds check: MAX_PENDING_RECOGNITIONS=64 with truncation warning - Mock dialogue sustained across ticks (not 1-tick flash) - COLOR_PING coupling with cursor documented as intentional - Consume helpers extracted: _consume_monologue(), _consume_dialogue() Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
This commit is contained in:
+42
-14
@@ -4,6 +4,7 @@ extends Control
|
||||
# InsertOverlay (CanvasLayer 10, z-layer 6) — diegetic, insert-styled.
|
||||
# NPC speech top, player response options below, left-aligned.
|
||||
# Max 3 visible options. No close button — walk-away (WASD) or option select only.
|
||||
# Auto-pause in single-player when dialogue is open (D-061).
|
||||
# Sprint 7: UI skeleton with mock data. Server wiring deferred to #305.
|
||||
|
||||
signal option_selected(index: int, text: String)
|
||||
@@ -17,10 +18,19 @@ var _is_showing: bool = false
|
||||
var _active_tween: Tween = null
|
||||
var _option_buttons: Array[Button] = []
|
||||
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
|
||||
const MAX_OPTIONS: int = 3 # D-061: max 3 response options visible
|
||||
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)
|
||||
|
||||
# D-064: movement actions that trigger walk-away
|
||||
const _WALK_AWAY_ACTIONS: Array[StringName] = [
|
||||
&"move_north", &"move_south", &"move_east", &"move_west",
|
||||
&"move_northeast", &"move_southeast", &"move_southwest", &"move_northwest",
|
||||
]
|
||||
|
||||
|
||||
func _ready() -> void:
|
||||
@@ -28,6 +38,8 @@ func _ready() -> void:
|
||||
visible = false
|
||||
_is_showing = false
|
||||
mouse_filter = Control.MOUSE_FILTER_IGNORE
|
||||
_update_layout()
|
||||
get_viewport().size_changed.connect(_update_layout)
|
||||
|
||||
|
||||
func _unhandled_input(event: InputEvent) -> void:
|
||||
@@ -36,16 +48,21 @@ func _unhandled_input(event: InputEvent) -> void:
|
||||
|
||||
# D-064: WASD during dialogue → walk-away, 300ms fade
|
||||
if event is InputEventKey and event.pressed:
|
||||
if (event.is_action_pressed("move_north")
|
||||
or event.is_action_pressed("move_south")
|
||||
or event.is_action_pressed("move_east")
|
||||
or event.is_action_pressed("move_west")
|
||||
or event.is_action_pressed("move_northeast")
|
||||
or event.is_action_pressed("move_southeast")
|
||||
or event.is_action_pressed("move_southwest")
|
||||
or event.is_action_pressed("move_northwest")):
|
||||
hide_dialogue()
|
||||
dialogue_dismissed.emit()
|
||||
for action in _WALK_AWAY_ACTIONS:
|
||||
if event.is_action_pressed(action):
|
||||
hide_dialogue()
|
||||
dialogue_dismissed.emit()
|
||||
return
|
||||
|
||||
|
||||
# Responsive layout — clamps width to MAX_WIDTH_PX and height to 20% viewport.
|
||||
func _update_layout() -> void:
|
||||
var vp := get_viewport_rect().size
|
||||
var max_h := vp.y * MAX_HEIGHT_RATIO
|
||||
var w := minf(MAX_WIDTH_PX, vp.x * 0.65)
|
||||
panel.offset_left = -w / 2.0
|
||||
panel.offset_right = w / 2.0
|
||||
panel.offset_top = -max_h
|
||||
|
||||
|
||||
# Show dialogue with NPC speech and response options.
|
||||
@@ -54,6 +71,7 @@ func _unhandled_input(event: InputEvent) -> void:
|
||||
# options: Array of Strings — player response choices (max 3 shown)
|
||||
func show_dialogue(npc_name: String, speech: String, options: Array = []) -> void:
|
||||
_npc_name = npc_name
|
||||
_dialogue_id += 1
|
||||
|
||||
# NPC speech — name prefix in bold
|
||||
if npc_name.is_empty():
|
||||
@@ -72,10 +90,10 @@ func show_dialogue(npc_name: String, speech: String, options: Array = []) -> voi
|
||||
btn.alignment = HORIZONTAL_ALIGNMENT_LEFT
|
||||
btn.flat = true
|
||||
btn.mouse_default_cursor_shape = Control.CURSOR_POINTING_HAND
|
||||
# Insert-styled: geometric, minimal
|
||||
btn.add_theme_color_override("font_color", Color("#c8d0e0"))
|
||||
btn.add_theme_color_override("font_hover_color", Color("#e8c547"))
|
||||
btn.add_theme_color_override("font_pressed_color", Color("#6bc9a6"))
|
||||
# Insert-styled colors from shared palette (D-048/D-056)
|
||||
btn.add_theme_color_override("font_color", Constants.INSERT_COLOR_TEXT)
|
||||
btn.add_theme_color_override("font_hover_color", Constants.INSERT_COLOR_HOVER)
|
||||
btn.add_theme_color_override("font_pressed_color", Constants.INSERT_COLOR_ACTIVE)
|
||||
var idx := i
|
||||
btn.pressed.connect(func(): _on_option_pressed(idx))
|
||||
options_container.add_child(btn)
|
||||
@@ -86,6 +104,9 @@ func show_dialogue(npc_name: String, speech: String, options: Array = []) -> voi
|
||||
mouse_filter = Control.MOUSE_FILTER_STOP
|
||||
_is_showing = true
|
||||
|
||||
# D-061: auto-pause in single-player when dialogue opens
|
||||
SimBridge.send_input({"action": InputMapper.Action.PAUSE, "timestamp_msec": Time.get_ticks_msec()})
|
||||
|
||||
if _active_tween and _active_tween.is_valid():
|
||||
_active_tween.kill()
|
||||
_active_tween = create_tween()
|
||||
@@ -100,6 +121,9 @@ func hide_dialogue() -> void:
|
||||
_is_showing = false
|
||||
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()})
|
||||
|
||||
if _active_tween and _active_tween.is_valid():
|
||||
_active_tween.kill()
|
||||
_active_tween = create_tween()
|
||||
@@ -114,6 +138,10 @@ func is_dialogue_active() -> bool:
|
||||
return _is_showing
|
||||
|
||||
|
||||
func get_dialogue_id() -> int:
|
||||
return _dialogue_id
|
||||
|
||||
|
||||
func _on_option_pressed(index: int) -> void:
|
||||
if index < _option_buttons.size():
|
||||
option_selected.emit(index, _option_buttons[index].text)
|
||||
|
||||
@@ -8,7 +8,6 @@ anchors_preset = 12
|
||||
anchor_top = 1.0
|
||||
anchor_right = 1.0
|
||||
anchor_bottom = 1.0
|
||||
offset_top = -200.0
|
||||
grow_horizontal = 2
|
||||
grow_vertical = 0
|
||||
mouse_filter = 2
|
||||
@@ -22,7 +21,7 @@ anchor_top = 1.0
|
||||
anchor_right = 0.5
|
||||
anchor_bottom = 1.0
|
||||
offset_left = -416.0
|
||||
offset_top = -180.0
|
||||
offset_top = -200.0
|
||||
offset_right = 416.0
|
||||
grow_horizontal = 2
|
||||
grow_vertical = 0
|
||||
|
||||
Reference in New Issue
Block a user