Player response appears in log after option selection
Overheard NPC conversation lines appear in test mode (Mira/Soren)
Walk-away clears options but log entries persist until timeout
Entry fade and removal after timeout
gdUnit4 tests pass: test_monologue_display.gd
## Summary
Sprint 14 client work — three tickets across monologue display, debug tooling, and dialogue system.
- **#122 Monologue display**: Multi-line queue architecture with character colours, italic BBCode, priority system, and full gdUnit4 test suite
- **#511 F3 debug overlay**: Real-time game state display toggled with F3 — tick, fps, position, entity counts, dialogue/monologue status, zone, and more
- **#535 Unified dialogue log**: Refactored dialogue box into a scrolling conversation log. Player-NPC and overheard NPC-NPC conversations flow chronologically. Per-character name colours from `dialogue-theme.yaml`, timeout-based entry expiry, walk-away preserves log entries, overheard lines at 90% opacity (D-078)
## Key design decisions
- Dialogue panel is a single unified log (not separate panels for active vs passive dialogue)
- Walk-away clears response options but preserves log entries — earned information is fair game
- NPC name colours loaded from `data/dialogue-theme.yaml` (configurable palette, hash-indexed)
- All entry types share the same timeout for expiry (15s + 3s fade, configurable)
## Test plan
- [ ] F3 toggle: overlay appears/disappears, data updates per tick
- [ ] Monologue display: queue ordering, character colours, italic formatting
- [ ] Dialogue log: NPC speech lines accumulate chronologically
- [ ] Player response appears in log after option selection
- [ ] Overheard NPC conversation lines appear in test mode (Mira/Soren)
- [ ] Walk-away clears options but log entries persist until timeout
- [ ] Entry fade and removal after timeout
- [ ] gdUnit4 tests pass: `test_monologue_display.gd`
Refactors dialogue box into a scrolling conversation log. All dialogue
(player-NPC and overheard NPC-NPC) flows chronologically, oldest at top.
Player response options at the bottom during active conversations.
- Entries expire after configurable timeout (equal for all message types)
- Walk-away clears options but preserves log entries (fair information)
- Per-character name colors from dialogue-theme.yaml (hash-indexed palette)
- Overheard lines render at 90% opacity (D-078)
- Protocol decode for conversation_events + conversation_ended
- GameState fields for conversation_events, conversation_ended, dialogue_response
- Mock Mira/Soren NPC-NPC conversation in test snapshot
- Also wires #511 debug overlay into main.gd and main.tscn
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Reviewers: Hoshe (Code Quality), Tyre (Architecture) Verdict: CHANGES REQUESTED
Hoshe (Code Quality): REQUEST_CHANGES
Well-structured PR overall. Monologue display redesign is correct, test suite is thorough, protocol v9 additions are clean. Three issues need addressing.
#
File
Severity
Issue
1
dialogue_box.gd:_expire_entries
warning
_rebuild_log() runs every frame (~60fps) for entire entry lifetime. had_entries always true past early-return, so elif always fires. O(n) BBCode rebuild per frame for 15+ seconds. Add a dirty flag.
2
dialogue_box.gd:_format_entry
warning
BBCode injection: server-sourced entry.speaker, entry.target, entry.text interpolated into BBCode without [ escaping. monologue_display.gd does this correctly with [lb] — same pattern needed here.
3
dialogue_box.gd:_end_player_conversation
warning
GameState.dialogue_active = false and UNPAUSE sent immediately when options clear, but panel still visible. Old code held pause until fade-out. Behavioral change vs D-064 auto-pause spec.
4
dialogue_box.gd:_unhandled_input
suggestion
No test for passive-only walk-away: overheard entries visible, WASD pressed, verify walk-away does NOT fire.
5
dialogue_box.gd:_color_for_name
suggestion
Comment "hash() can return negative in GDScript" misleading for String.hash() (always non-negative). Harmless guard.
6
main.gd:_consume_dialogue_response
suggestion
_last_dialogue_npc_name used instead of decoded speaker_entity_id. Safe now, silent misattribution risk later.
7
tests
suggestion
No test coverage for dialogue_box.gd expiry/hide lifecycle.
8
dialogue-theme.yaml
suggestion
Inconsistent quoting: numeric values quoted ("0.9"), colors unquoted. Works but confusing.
Tyre (Architecture): REQUEST_CHANGES
Unified conversation log architecture is fundamentally sound — reusing D-061 dialogue panel for D-078 overheard content is elegant. D-010/D-020 client-server boundary respected (occlusion stays server-side, client renders verbatim). Protocol version mismatch, carry-forward gaps, and input path bypass need fixing.
#
File
Severity
Issue
1
protocol.gd:14
critical
PROTOCOL_VERSION still 8 but PR adds v9 fields. Hard == check in decode_snapshot() rejects snapshots on mismatch. Must be bumped.
2
sim_bridge.gd:receive_bytes
warning
dialogue_response, conversation_events, conversation_ended have no carry-forward logic. NPC response after player choice can be silently dropped under real server load.
3
dialogue_box.gd:show_dialogue/end
warning
SimBridge.send_input() called directly for PAUSE/UNPAUSE, bypassing main.gd's _pending_record_inputs. Bug report replay (#507) misses pause events. Should emit signals.
4
dialogue_box.gd D-078
warning
Passive lines use 0.9 opacity only — perceptually negligible. D-078 spec says "dimmed border or header treatment." No visual marker distinguishes overheard from player dialogue. Needs Araminta sign-off.
5
dialogue_box.gd:_active_overheard
warning
Dictionary populated/cleared but drives zero behavior. Dead state. Either connect to functionality or remove.
6
dialogue_box.gd:_process
suggestion
Per-frame _rebuild_log() — dirty flag would be a 5-line fix.
7
main.gd:_consume_dialogue_response
suggestion
Use speaker_entity_id from protocol decode instead of cached name.
8
dialogue_box.gd:_load_theme
suggestion
Calls UIStrings._parse_yaml() (private method). Promote to public utility.
9
tests
suggestion
No test_protocol_v9.gd — v7 precedent makes decode-layer tests expected.
10
protocol.gd:172 vs game_state.gd:189
suggestion
dialogue_response labeled "v8" in protocol, "v9" in game_state. Inconsistent version comments.
Overall Verdict: CHANGES REQUESTED
Critical (1): Protocol version constant must be bumped for v9 fields.
Warnings (7 unique):
BBCode injection in dialogue log formatting (escape [ with [lb])
Per-frame _rebuild_log() without dirty flag (perf)
dialogue_active cleared before panel fade completes (D-064 regression)
v9 one-shot events missing carry-forward in receive_bytes()
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 <noreply@anthropic.com>
Reviewers: Hoshe (Code Quality), Tyre (Architecture) Context: Fix commit e19887c addressed 10 items from original review (1 critical, 7 warnings, 8 suggestions). Verdict: APPROVED
Hoshe (Code Quality): APPROVE
All 3 original warnings are correctly fixed: dirty flag prevents per-frame rebuild, _escape_bbcode() sanitizes server strings in _format_entry(), and dialogue_active clears in tween callback after fade. YAML quoting and hash comment addressed.
Original items — verified fixed:
H1 (dirty flag): _log_dirty flag added, _rebuild_log() only called when dirty. Fixed.
H2 (BBCode injection in log): _escape_bbcode() applied to speaker, target, text in _format_entry(). Fixed.
H3 (dialogue_active lifecycle): Cleared in hide_dialogue() tween callback after 300ms fade. Fixed.
H8 (YAML quoting): Numeric values unquoted, color strings quoted. Fixed.
#
File
Severity
Issue
1
dialogue_box.gd:_show_options
suggestion
Options path doesn't call _escape_bbcode() on raw_text before "[i]%s[/i]" interpolation. Lower risk than log entries (standalone RichTextLabels, no BBCode concatenation, curated server content per D-010), but inconsistent with the log escaping pattern.
2
dialogue_box.gd:_expire_entries
suggestion
Front-anchored expiry loop breaks on the first pinned entry. Expired unpinned entries at higher indices are not removed until the pin clears. Acceptable given D-061 entry lifetime bounds.
3
dialogue_box.gd:_enforce_contrast
suggestion
For near-black NPC name colors (lum < 0.02), the boost multiplier can produce heavily saturated tints. Low risk given the curated palette in theme YAML.
4
tests
suggestion
No test_dialogue_box.gd for the unified log refactor. Original H4 (passive walk-away) and H7 (expiry/hide lifecycle) remain unaddressed.
5
main.gd:_consume_dialogue_response
suggestion
_last_dialogue_npc_name still used instead of speaker_entity_id. Carry-over from original H6 — acceptable for v0.1.
Tyre (Architecture): APPROVE
All original items verified: PROTOCOL_VERSION bumped to 9, carry-forward logic added for v9 arrays, PAUSE/UNPAUSE routed through _pending_record_inputs via signals, passive visual distinction implemented (┃ glyph + desaturated colours), dead _active_overheard removed, version comments consistent. Architecture is clean — unified log pattern respects D-010/D-020 boundaries.
T10 (version comments): dialogue_response labeled v8 in both protocol.gd and game_state.gd. Fixed.
#
File
Severity
Issue
1
sim_bridge.gd:receive_bytes
suggestion
conversation_events carry-forward merges without an upper bound cap. Under OS focus loss, events could accumulate. Precedent: MAX_PENDING_RECOGNITIONS = 64. Consider a parallel cap.
2
dialogue_box.gd:_expire_entries
suggestion
Front-anchored expiry loop — same note as Hoshe #2. Acceptable given D-061 entry lifetime bounds.
3
tests
suggestion
No test_protocol_v9.gd — v7 precedent makes decode-layer tests expected.
4
dialogue_box.gd:_load_theme
suggestion
UIStrings._parse_yaml() coupling — narrow, static, same codebase. Not a blocker.
5
main.gd:_consume_dialogue_response
suggestion
speaker_entity_id unused — acknowledged for v0.1.
Overall Verdict: APPROVED
All original issues resolved:
Critical: PROTOCOL_VERSION bumped to 9 ✓
7 warnings: All fixed (dirty flag, BBCode in log, dialogue_active lifecycle, carry-forward, PAUSE/UNPAUSE routing, passive visual, dead state) ✓
Araminta items: Pinning, contrast floor, player dialogue simplification ✓
Suggestions (9): BBCode escaping consistency in options, missing tests (protocol v9, dialogue box lifecycle, passive walk-away), speaker_entity_id unused, carry-forward unbounded, expiry loop fragility, contrast boost for dark colors, _parse_yaml coupling.
## Re-Review: client -> main (type: code) — PR #52
**Reviewers:** Hoshe (Code Quality), Tyre (Architecture)
**Context:** Fix commit `e19887c` addressed 10 items from original review (1 critical, 7 warnings, 8 suggestions).
**Verdict: APPROVED**
---
### Hoshe (Code Quality): APPROVE
All 3 original warnings are correctly fixed: dirty flag prevents per-frame rebuild, `_escape_bbcode()` sanitizes server strings in `_format_entry()`, and `dialogue_active` clears in tween callback after fade. YAML quoting and hash comment addressed.
**Original items — verified fixed:**
- H1 (dirty flag): `_log_dirty` flag added, `_rebuild_log()` only called when dirty. **Fixed.**
- H2 (BBCode injection in log): `_escape_bbcode()` applied to speaker, target, text in `_format_entry()`. **Fixed.**
- H3 (dialogue_active lifecycle): Cleared in `hide_dialogue()` tween callback after 300ms fade. **Fixed.**
- H8 (YAML quoting): Numeric values unquoted, color strings quoted. **Fixed.**
| # | File | Severity | Issue |
|---|------|----------|-------|
| 1 | `dialogue_box.gd:_show_options` | suggestion | Options path doesn't call `_escape_bbcode()` on `raw_text` before `"[i]%s[/i]"` interpolation. Lower risk than log entries (standalone RichTextLabels, no BBCode concatenation, curated server content per D-010), but inconsistent with the log escaping pattern. |
| 2 | `dialogue_box.gd:_expire_entries` | suggestion | Front-anchored expiry loop breaks on the first pinned entry. Expired unpinned entries at higher indices are not removed until the pin clears. Acceptable given D-061 entry lifetime bounds. |
| 3 | `dialogue_box.gd:_enforce_contrast` | suggestion | For near-black NPC name colors (lum < 0.02), the boost multiplier can produce heavily saturated tints. Low risk given the curated palette in theme YAML. |
| 4 | tests | suggestion | No `test_dialogue_box.gd` for the unified log refactor. Original H4 (passive walk-away) and H7 (expiry/hide lifecycle) remain unaddressed. |
| 5 | `main.gd:_consume_dialogue_response` | suggestion | `_last_dialogue_npc_name` still used instead of `speaker_entity_id`. Carry-over from original H6 — acceptable for v0.1. |
---
### Tyre (Architecture): APPROVE
All original items verified: PROTOCOL_VERSION bumped to 9, carry-forward logic added for v9 arrays, PAUSE/UNPAUSE routed through `_pending_record_inputs` via signals, passive visual distinction implemented (┃ glyph + desaturated colours), dead `_active_overheard` removed, version comments consistent. Architecture is clean — unified log pattern respects D-010/D-020 boundaries.
**Original items — verified fixed:**
- T1 (critical, protocol version): `PROTOCOL_VERSION = 9`. **Fixed.**
- T2 (carry-forward): `conversation_events` and `conversation_ended` arrays merged in `receive_bytes()`. `dialogue_response` scalar carried forward on null. **Fixed.** (No duplicate delivery risk — `poll_snapshot()` nulls `_last_snapshot` on consumption.)
- T3 (PAUSE/UNPAUSE bypass): `pause_requested`/`unpause_requested` signals connected in `main.gd._ready()`, handlers append to `_pending_record_inputs`. **Fixed.**
- T4 (passive visual): ┃ glyph prefix + 40% desaturation + 0.9 opacity + contrast floor. **Fixed.**
- T5 (dead state): `_active_overheard` removed entirely. **Fixed.**
- T10 (version comments): `dialogue_response` labeled v8 in both protocol.gd and game_state.gd. **Fixed.**
| # | File | Severity | Issue |
|---|------|----------|-------|
| 1 | `sim_bridge.gd:receive_bytes` | suggestion | `conversation_events` carry-forward merges without an upper bound cap. Under OS focus loss, events could accumulate. Precedent: `MAX_PENDING_RECOGNITIONS = 64`. Consider a parallel cap. |
| 2 | `dialogue_box.gd:_expire_entries` | suggestion | Front-anchored expiry loop — same note as Hoshe #2. Acceptable given D-061 entry lifetime bounds. |
| 3 | tests | suggestion | No `test_protocol_v9.gd` — v7 precedent makes decode-layer tests expected. |
| 4 | `dialogue_box.gd:_load_theme` | suggestion | `UIStrings._parse_yaml()` coupling — narrow, static, same codebase. Not a blocker. |
| 5 | `main.gd:_consume_dialogue_response` | suggestion | `speaker_entity_id` unused — acknowledged for v0.1. |
---
### Overall Verdict: APPROVED
**All original issues resolved:**
- Critical: PROTOCOL_VERSION bumped to 9 ✓
- 7 warnings: All fixed (dirty flag, BBCode in log, dialogue_active lifecycle, carry-forward, PAUSE/UNPAUSE routing, passive visual, dead state) ✓
- Araminta items: Pinning, contrast floor, player dialogue simplification ✓
**Suggestions (9):** BBCode escaping consistency in options, missing tests (protocol v9, dialogue box lifecycle, passive walk-away), `speaker_entity_id` unused, carry-forward unbounded, expiry loop fragility, contrast boost for dark colors, `_parse_yaml` coupling.
Blocking a user prevents them from interacting with repositories, such as opening or commenting on pull requests or issues. Learn more about blocking a user.
Summary
Sprint 14 client work — three tickets across monologue display, debug tooling, and dialogue system.
dialogue-theme.yaml, timeout-based entry expiry, walk-away preserves log entries, overheard lines at 90% opacity (D-078)Key design decisions
data/dialogue-theme.yaml(configurable palette, hash-indexed)Test plan
test_monologue_display.gdPR Review: client -> main (type: code) — PR #52
Reviewers: Hoshe (Code Quality), Tyre (Architecture)
Verdict: CHANGES REQUESTED
Hoshe (Code Quality): REQUEST_CHANGES
Well-structured PR overall. Monologue display redesign is correct, test suite is thorough, protocol v9 additions are clean. Three issues need addressing.
dialogue_box.gd:_expire_entries_rebuild_log()runs every frame (~60fps) for entire entry lifetime.had_entriesalways true past early-return, soelifalways fires. O(n) BBCode rebuild per frame for 15+ seconds. Add a dirty flag.dialogue_box.gd:_format_entryentry.speaker,entry.target,entry.textinterpolated into BBCode without[escaping.monologue_display.gddoes this correctly with[lb]— same pattern needed here.dialogue_box.gd:_end_player_conversationGameState.dialogue_active = falseand UNPAUSE sent immediately when options clear, but panel still visible. Old code held pause until fade-out. Behavioral change vs D-064 auto-pause spec.dialogue_box.gd:_unhandled_inputdialogue_box.gd:_color_for_nameString.hash()(always non-negative). Harmless guard.main.gd:_consume_dialogue_response_last_dialogue_npc_nameused instead of decodedspeaker_entity_id. Safe now, silent misattribution risk later.dialogue_box.gdexpiry/hide lifecycle.dialogue-theme.yaml"0.9"), colors unquoted. Works but confusing.Tyre (Architecture): REQUEST_CHANGES
Unified conversation log architecture is fundamentally sound — reusing D-061 dialogue panel for D-078 overheard content is elegant. D-010/D-020 client-server boundary respected (occlusion stays server-side, client renders verbatim). Protocol version mismatch, carry-forward gaps, and input path bypass need fixing.
protocol.gd:14PROTOCOL_VERSIONstill 8 but PR adds v9 fields. Hard==check indecode_snapshot()rejects snapshots on mismatch. Must be bumped.sim_bridge.gd:receive_bytesdialogue_response,conversation_events,conversation_endedhave no carry-forward logic. NPC response after player choice can be silently dropped under real server load.dialogue_box.gd:show_dialogue/endSimBridge.send_input()called directly for PAUSE/UNPAUSE, bypassingmain.gd's_pending_record_inputs. Bug report replay (#507) misses pause events. Should emit signals.dialogue_box.gdD-078dialogue_box.gd:_active_overhearddialogue_box.gd:_process_rebuild_log()— dirty flag would be a 5-line fix.main.gd:_consume_dialogue_responsespeaker_entity_idfrom protocol decode instead of cached name.dialogue_box.gd:_load_themeUIStrings._parse_yaml()(private method). Promote to public utility.test_protocol_v9.gd— v7 precedent makes decode-layer tests expected.protocol.gd:172vsgame_state.gd:189dialogue_responselabeled "v8" in protocol, "v9" in game_state. Inconsistent version comments.Overall Verdict: CHANGES REQUESTED
Critical (1): Protocol version constant must be bumped for v9 fields.
Warnings (7 unique):
[with[lb])_rebuild_log()without dirty flag (perf)dialogue_activecleared before panel fade completes (D-064 regression)receive_bytes()main.gdinput recording (#507)_active_overhearddead infrastructureSuggestions (8): Missing tests (protocol v9, dialogue expiry lifecycle, passive walk-away),
speaker_entity_idunused, version comment mismatch, YAML quoting, hash comment, private method coupling.Re-Review: client -> main (type: code) — PR #52
Reviewers: Hoshe (Code Quality), Tyre (Architecture)
Context: Fix commit
e19887caddressed 10 items from original review (1 critical, 7 warnings, 8 suggestions).Verdict: APPROVED
Hoshe (Code Quality): APPROVE
All 3 original warnings are correctly fixed: dirty flag prevents per-frame rebuild,
_escape_bbcode()sanitizes server strings in_format_entry(), anddialogue_activeclears in tween callback after fade. YAML quoting and hash comment addressed.Original items — verified fixed:
_log_dirtyflag added,_rebuild_log()only called when dirty. Fixed._escape_bbcode()applied to speaker, target, text in_format_entry(). Fixed.hide_dialogue()tween callback after 300ms fade. Fixed.dialogue_box.gd:_show_options_escape_bbcode()onraw_textbefore"[i]%s[/i]"interpolation. Lower risk than log entries (standalone RichTextLabels, no BBCode concatenation, curated server content per D-010), but inconsistent with the log escaping pattern.dialogue_box.gd:_expire_entriesdialogue_box.gd:_enforce_contrasttest_dialogue_box.gdfor the unified log refactor. Original H4 (passive walk-away) and H7 (expiry/hide lifecycle) remain unaddressed.main.gd:_consume_dialogue_response_last_dialogue_npc_namestill used instead ofspeaker_entity_id. Carry-over from original H6 — acceptable for v0.1.Tyre (Architecture): APPROVE
All original items verified: PROTOCOL_VERSION bumped to 9, carry-forward logic added for v9 arrays, PAUSE/UNPAUSE routed through
_pending_record_inputsvia signals, passive visual distinction implemented (┃ glyph + desaturated colours), dead_active_overheardremoved, version comments consistent. Architecture is clean — unified log pattern respects D-010/D-020 boundaries.Original items — verified fixed:
PROTOCOL_VERSION = 9. Fixed.conversation_eventsandconversation_endedarrays merged inreceive_bytes().dialogue_responsescalar carried forward on null. Fixed. (No duplicate delivery risk —poll_snapshot()nulls_last_snapshoton consumption.)pause_requested/unpause_requestedsignals connected inmain.gd._ready(), handlers append to_pending_record_inputs. Fixed._active_overheardremoved entirely. Fixed.dialogue_responselabeled v8 in both protocol.gd and game_state.gd. Fixed.sim_bridge.gd:receive_bytesconversation_eventscarry-forward merges without an upper bound cap. Under OS focus loss, events could accumulate. Precedent:MAX_PENDING_RECOGNITIONS = 64. Consider a parallel cap.dialogue_box.gd:_expire_entriestest_protocol_v9.gd— v7 precedent makes decode-layer tests expected.dialogue_box.gd:_load_themeUIStrings._parse_yaml()coupling — narrow, static, same codebase. Not a blocker.main.gd:_consume_dialogue_responsespeaker_entity_idunused — acknowledged for v0.1.Overall Verdict: APPROVED
All original issues resolved:
Suggestions (9): BBCode escaping consistency in options, missing tests (protocol v9, dialogue box lifecycle, passive walk-away),
speaker_entity_idunused, carry-forward unbounded, expiry loop fragility, contrast boost for dark colors,_parse_yamlcoupling.Pull request closed