feat(ui): entity-bound speaker colors + entity renderer test fixes (#573, #574) #82

2026-03-04T02:06:04+01:00

jpmschweitzer commented

2026-03-04 02:06:04 +01:00

Sprint 23 client deliverables

#573 — Bind dialogue speaker colors to entity identity

Speaker colors in player dialogue were assigned by hashing the NPC's display name, which meant colors could shift before the name resolved. This PR binds speaker colors to entity ID instead.

Changes in dialogue_box.gd:

_npc_entity_colors: Dictionary — maps entity_id → Color for dialogue participants
_npc_entity_id: int — tracks current NPC entity ID across the conversation
_next_npc_color: int — round-robin palette index for client-side assignment
_assign_npc_color(entity_id) — registers palette color on first encounter, returns same color on repeat calls
show_dialogue — accepts npc_entity_id param, calls _assign_npc_color
append_line — optional speaker_entity_id/target_entity_id stored in log entries
append_player_line — passes _npc_entity_id as target_entity_id
append_dialogue_response — accepts entity_id, registers color, passes to append_line
_format_entry — looks up _npc_entity_colors before name-hash fallback

Changes in main.gd:

_consume_dialogue: passes _last_dialogue_npc_id to show_dialogue
_consume_dialogue_response: passes speaker_entity_id to append_dialogue_response

All changes are backward-compatible (new params are optional with -1 defaults).

#574 — Fix pre-existing test failures in test_entity_renderer

Seven tests in test_rendering.gd were failing due to stale assertions from the ColorRect → Sprite2D renderer migration:

5 tests: as ColorRect/.color → as Sprite2D/.self_modulate
2 tests: position offset (TILE_SIZE-24)/2 = 4.0 → EntityRenderer.ENTITY_OFFSET_X/Y = 0.0
1 test: rotation accuracy expected values assumed Godot normalises to (-π, π]; Godot 4 returns raw values
Also added SoundIndicatorRenderer to .godot/global_script_class_cache.cfg (was causing parse errors on every test in test_rendering.gd)

All 52 test_rendering tests now pass.

## Sprint 23 client deliverables ### #573 — Bind dialogue speaker colors to entity identity Speaker colors in player dialogue were assigned by hashing the NPC's display name, which meant colors could shift before the name resolved. This PR binds speaker colors to entity ID instead. **Changes in `dialogue_box.gd`:** - `_npc_entity_colors: Dictionary` — maps entity_id → Color for dialogue participants - `_npc_entity_id: int` — tracks current NPC entity ID across the conversation - `_next_npc_color: int` — round-robin palette index for client-side assignment - `_assign_npc_color(entity_id)` — registers palette color on first encounter, returns same color on repeat calls - `show_dialogue` — accepts `npc_entity_id` param, calls `_assign_npc_color` - `append_line` — optional `speaker_entity_id`/`target_entity_id` stored in log entries - `append_player_line` — passes `_npc_entity_id` as target_entity_id - `append_dialogue_response` — accepts `entity_id`, registers color, passes to `append_line` - `_format_entry` — looks up `_npc_entity_colors` before name-hash fallback **Changes in `main.gd`:** - `_consume_dialogue`: passes `_last_dialogue_npc_id` to `show_dialogue` - `_consume_dialogue_response`: passes `speaker_entity_id` to `append_dialogue_response` All changes are backward-compatible (new params are optional with -1 defaults). ### #574 — Fix pre-existing test failures in test_entity_renderer Seven tests in `test_rendering.gd` were failing due to stale assertions from the ColorRect → Sprite2D renderer migration: - 5 tests: `as ColorRect`/`.color` → `as Sprite2D`/`.self_modulate` - 2 tests: position offset `(TILE_SIZE-24)/2 = 4.0` → `EntityRenderer.ENTITY_OFFSET_X/Y = 0.0` - 1 test: rotation accuracy expected values assumed Godot normalises to (-π, π]; Godot 4 returns raw values - Also added `SoundIndicatorRenderer` to `.godot/global_script_class_cache.cfg` (was causing parse errors on every test in test_rendering.gd) All 52 test_rendering tests now pass.

jpmschweitzer added 2 commits 2026-03-04 02:08:36 +01:00

fix(ui): correct entity renderer test assertions for Sprite2D and zero offsets (#574 ) 87b5cbb6c2

Fixes 7 pre-existing failures in test_entity_renderer tests:
- ColorRect → Sprite2D cast; .color → .self_modulate for D-033 color checks
- Position offset: (TILE_SIZE-24)/2 → EntityRenderer.ENTITY_OFFSET_{X,Y} (0.0)
- Rotation accuracy: expected values updated for raw un-normalised Godot rotation
Also adds SoundIndicatorRenderer to global_script_class_cache.cfg so test_rendering.gd parses.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

feat(ui): bind dialogue speaker colors to entity identity (#573 ) 974bb2b28e

Maintains Dict[entity_id → Color] in dialogue_box for player conversations.
On first encounter, assigns a round-robin palette color; reuses on subsequent lines.
Eliminates position-based name-hash coloring for player dialogue.

Changes:
- Add _npc_entity_colors dict, _npc_entity_id, _next_npc_color fields
- Add _assign_npc_color(entity_id) — registers palette color on first encounter
- show_dialogue: accept npc_entity_id param, register entity color
- append_line: optional speaker_entity_id/target_entity_id stored in log entries
- append_player_line: pass _npc_entity_id as target_entity_id
- append_dialogue_response: accept entity_id, register, pass to append_line
- _format_entry else branch: look up _npc_entity_colors before name-hash fallback
- main.gd: pass _last_dialogue_npc_id to show_dialogue, speaker_entity_id to append_dialogue_response

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

jpmschweitzer commented

2026-03-04 02:11:41 +01:00

Review: `client` → `main` (type: code)

Hoshe (Code Quality): REQUEST_CHANGES

Summary: Entity renderer test fixes (#574) are correct — Sprite2D/self_modulate assertions match the implementation. However, the new speaker color system (#573) has zero test coverage and a state management bug: _npc_entity_colors and _next_npc_color are never cleared on room change/teleport, causing color drift and palette exhaustion across rooms.

#	File:line	Severity	Issue
1	`dialogue_box.gd:48-53`	warning	`_npc_entity_colors`, `_npc_entity_id`, `_next_npc_color` never reset on room change. After teleport, entity IDs may be reused and palette slots are consumed permanently. Color registry should be cleared in `hide_dialogue()` or via a `reset_session_colors()` call.
2	`dialogue_box.gd:531-541`	warning	Player line target color depends on `_assign_npc_color` having been called first via `show_dialogue`. Ordering dependency is implicit — if response arrives before initial dialogue on same tick, falls back to name-hash silently.
3	All test files	warning	Zero test coverage for the entire #573 color system — `_assign_npc_color`, `_npc_entity_colors`, entity-ID parameters on `show_dialogue`/`append_line`/`append_dialogue_response`.
4	`dialogue_box.gd:579-589`	suggestion	`_enforce_contrast` applied at assignment time but not re-checked after `_desaturate()` for passive lines — desaturated version could fall below readability floor.
5	`test_rendering.gd:325-328`	suggestion	Southwest/West rotation values updated but Northwest wrap not annotated — could be "fixed" by mistake.

Tyre (Architecture): REQUEST_CHANGES

Summary: Core implementation pattern is clean — entity-ID keyed color registry with round-robin palette and name-hash fallback is the right approach. D-033 compliance maintained (dialogue colors are UI-layer, separate from world entity tinting). However, unbounded _next_npc_color guarantees palette collisions in longer sessions (8 palette entries, 9+ NPCs = collision), and there's no explicit design decision on whether colors persist across conversations.

#	File:line	Severity	Issue
1	`dialogue_box.gd:51-53`	warning	`_npc_entity_colors` and `_next_npc_color` never reset — palette exhaustion guaranteed with 9+ NPCs in a session. Need an explicit decision: clear on conversation end, conflict-detect on assignment, or document as known v0.1 limitation.
2	`dialogue_box.gd:197`	suggestion	`append_line()` now has 6 positional params with 2 optional ints at tail — heading toward call-site confusion. Flag for dictionary-options overload in Phase 2.
3	`main.gd:385`	suggestion	`_last_dialogue_npc_id` fallback is brittle — fast re-engagement with a different NPC could misattribute color. Low probability in v0.1 but worth a comment.
4	`dialogue_box.gd:_assign_npc_color()`	suggestion	Add `# TODO D-033 Phase 2: derive from relationship color` — current independent palette will need alignment when relationship-based colors arrive.

Verdict: CHANGES REQUESTED

Issues to fix before merge:

Color registry lifecycle — decide and implement: clear on room change/conversation end, or document the persistence + palette-exhaustion as intentional v0.1 behavior
Test coverage — at least basic tests for _assign_npc_color (round-robin assignment, same entity returns same color, fallback when no entity ID)
Ordering dependency — add comment or guard for the implicit dependency between show_dialogue and append_player_line color paths

## Review: `client` → `main` (type: code) ### Hoshe (Code Quality): REQUEST_CHANGES **Summary:** Entity renderer test fixes (#574) are correct — Sprite2D/self_modulate assertions match the implementation. However, the new speaker color system (#573) has zero test coverage and a state management bug: `_npc_entity_colors` and `_next_npc_color` are never cleared on room change/teleport, causing color drift and palette exhaustion across rooms. | # | File:line | Severity | Issue | |---|-----------|----------|-------| | 1 | `dialogue_box.gd:48-53` | warning | `_npc_entity_colors`, `_npc_entity_id`, `_next_npc_color` never reset on room change. After teleport, entity IDs may be reused and palette slots are consumed permanently. Color registry should be cleared in `hide_dialogue()` or via a `reset_session_colors()` call. | | 2 | `dialogue_box.gd:531-541` | warning | Player line target color depends on `_assign_npc_color` having been called first via `show_dialogue`. Ordering dependency is implicit — if response arrives before initial dialogue on same tick, falls back to name-hash silently. | | 3 | All test files | warning | Zero test coverage for the entire #573 color system — `_assign_npc_color`, `_npc_entity_colors`, entity-ID parameters on `show_dialogue`/`append_line`/`append_dialogue_response`. | | 4 | `dialogue_box.gd:579-589` | suggestion | `_enforce_contrast` applied at assignment time but not re-checked after `_desaturate()` for passive lines — desaturated version could fall below readability floor. | | 5 | `test_rendering.gd:325-328` | suggestion | Southwest/West rotation values updated but Northwest wrap not annotated — could be "fixed" by mistake. | --- ### Tyre (Architecture): REQUEST_CHANGES **Summary:** Core implementation pattern is clean — entity-ID keyed color registry with round-robin palette and name-hash fallback is the right approach. D-033 compliance maintained (dialogue colors are UI-layer, separate from world entity tinting). However, unbounded `_next_npc_color` guarantees palette collisions in longer sessions (8 palette entries, 9+ NPCs = collision), and there's no explicit design decision on whether colors persist across conversations. | # | File:line | Severity | Issue | |---|-----------|----------|-------| | 1 | `dialogue_box.gd:51-53` | warning | `_npc_entity_colors` and `_next_npc_color` never reset — palette exhaustion guaranteed with 9+ NPCs in a session. Need an explicit decision: clear on conversation end, conflict-detect on assignment, or document as known v0.1 limitation. | | 2 | `dialogue_box.gd:197` | suggestion | `append_line()` now has 6 positional params with 2 optional ints at tail — heading toward call-site confusion. Flag for dictionary-options overload in Phase 2. | | 3 | `main.gd:385` | suggestion | `_last_dialogue_npc_id` fallback is brittle — fast re-engagement with a different NPC could misattribute color. Low probability in v0.1 but worth a comment. | | 4 | `dialogue_box.gd:_assign_npc_color()` | suggestion | Add `# TODO D-033 Phase 2: derive from relationship color` — current independent palette will need alignment when relationship-based colors arrive. | --- ### Verdict: CHANGES REQUESTED **Issues to fix before merge:** 1. **Color registry lifecycle** — decide and implement: clear on room change/conversation end, or document the persistence + palette-exhaustion as intentional v0.1 behavior 2. **Test coverage** — at least basic tests for `_assign_npc_color` (round-robin assignment, same entity returns same color, fallback when no entity ID) 3. **Ordering dependency** — add comment or guard for the implicit dependency between `show_dialogue` and `append_player_line` color paths

jpmschweitzer added 3 commits 2026-03-04 09:50:40 +01:00

fix(ui): clear color registry on conversation end, add speaker color tests (#573 ) d30ab62bc0

- Reset _npc_entity_colors/_npc_entity_id/_next_npc_color in _end_player_conversation()
  to prevent palette exhaustion across long sessions with many unique NPCs
- Re-enforce contrast floor after passive desaturation (_enforce_contrast after _desaturate)
- Add TestDialogueSpeakerColors suite: palette allocation, entity reuse, reset, fallback

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

fix(ui): correct Sprite2D/self_modulate assertions in P2 client tests (#574 ) 60d2c5bb90

Same Sprite2D correction applied to test_client_p2.gd entity color tests
(Terrain, Player) — ColorRect was replaced with Sprite2D in entity_renderer.gd.
Minor comment clarification in test_rendering.gd rotation test.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

feat(ui): add in-game debug console with tilde toggle and command dispatch (#581 ) c3abf32185

- debug_console.gd: new ModalLayer Control — tilde key toggles bottom-40% panel,
  command history (up/down), SimBridge dispatch for all DebugCommandKind variants:
  ticks, contaminate, tp, activate, triangle, npc, triangles, pop, status, help
- debug_console.tscn: minimal scene node; UI built programmatically in _ready()
- input_mapper.gd: DEBUG_COMMAND action added to Action enum
- sim_bridge.gd: DEBUG_COMMAND → "DebugCommand" wire mapping
- protocol.gd: v18 debug_response decode (command, text, success fields)
- game_state.gd: debug_response field + apply_snapshot one-shot handling
- main.gd: @onready ref, router registration, _consume_debug_response(), settings signal
- settings_dialog.gd: debug_console_toggled signal + CheckButton toggle row (+36px height),
  reads initial state from user://settings.cfg; CheckButton state loaded from PREFS_PATH
- main.tscn: DebugConsole node on ModalLayer, load_steps 27→28

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

jpmschweitzer commented

2026-03-04 09:52:36 +01:00

Update: PR now also includes #581 (debug console).

Additional commits since opening:

fix(ui): clear color registry on conversation end, add speaker color tests (#573)
fix(ui): correct Sprite2D/self_modulate assertions in P2 client tests (#574)
feat(ui): in-game debug console with tilde toggle and command dispatch (#581)

#581 summary:

New DebugConsole Control on ModalLayer — tilde key toggles bottom-40% panel
All DebugCommandKind variants dispatched through SimBridge: ticks, contaminate, tp, activate, triangle, npc, triangles, pop, status, help
Protocol v18 debug_response decode + game_state.gd one-shot field
Settings dialog CheckButton toggle, persists via user://settings.cfg
Command history (up/down, max 20 entries), keyboard swallowed while open

**Update: PR now also includes #581 (debug console).** Additional commits since opening: - `fix(ui)`: clear color registry on conversation end, add speaker color tests (#573) - `fix(ui)`: correct Sprite2D/self_modulate assertions in P2 client tests (#574) - `feat(ui)`: in-game debug console with tilde toggle and command dispatch (#581) **#581 summary:** - New `DebugConsole` Control on `ModalLayer` — tilde key toggles bottom-40% panel - All `DebugCommandKind` variants dispatched through `SimBridge`: ticks, contaminate, tp, activate, triangle, npc, triangles, pop, status, help - Protocol v18 `debug_response` decode + `game_state.gd` one-shot field - Settings dialog CheckButton toggle, persists via `user://settings.cfg` - Command history (up/down, max 20 entries), keyboard swallowed while open

jpmschweitzer added 1 commit 2026-03-04 20:32:40 +01:00

fix(ui): render LOS boundary wall tiles through fog without marking explored (#585 ) 475280191c

- game_state.gd: add boundary_positions Dictionary field; BoundaryWall tiles from
  visible_tiles go to boundary_positions instead of visible_positions — rendered by
  tile_renderer but not tracked as explored fog memory
- fog_state.gd: update_from_state() writes VIS_FORWARD for boundary_positions so fog
  lifts over margin wall content; boundary tiles excluded from exploration step so they
  don't persist as EXP_EXPLORED when player turns away
- tile_renderer.gd: no changes needed — renders all visible_tiles by type, sector-agnostic
- test_fog_shader.gd: 4 new tests — boundary excluded from visible_positions, tracked in
  boundary_positions, cleared each snapshot, fog lifts to VIS_FORWARD

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

jpmschweitzer added 2 commits 2026-03-04 20:33:41 +01:00

test(client): add BoundaryWall fog tests and document tile_renderer behavior (#585 ) 33b26c1a15

4 new tests in test_fog_sprint22.gd verify BoundaryWall tiles populate
boundary_positions (not visible_positions), get VIS_FORWARD without
EXP_VISIBLE, stay EXP_UNEXPLORED after leaving LOS, and clear on new
snapshot. Comment in tile_renderer.gd documents implicit rendering path.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

chore(meta): update changelog e9dba609c9

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

jpmschweitzer commented

2026-03-04 22:21:48 +01:00

Re-review: `client` → `main` — Sprint 23 full client PR

Hoshe (Code Quality): APPROVE (conditional on critical #1)

Summary: All 3 warnings from the previous review are resolved. The 4 tickets are well-implemented with good test coverage. One operational issue: PROTOCOL_VERSION still 17.

Previous review fixes verified: color registry clear, test coverage, ordering dependency — all resolved.

#	File:line	Severity	Issue
1	`protocol.gd:14`	critical	`PROTOCOL_VERSION = 17` but server is 18. Client will reject every snapshot — game unplayable. One-line fix: change to 18.
2	`debug_console.gd:189-191`	suggestion	`tp` with invalid z silently defaults to 0 — harmless but unhelpful feedback.
3	`debug_console.gd:236-237`	suggestion	`append_response` auto-opens console even if user disabled it via settings.
4	`settings_dialog.gd:127-130`	suggestion	Reads prefs from ConfigFile directly instead of querying live DebugConsole state.

Tyre (Architecture): REQUEST_CHANGES

Summary: Three tickets (#573, #574, #581) are solid. Debug console is well-encapsulated with correct D-020 flow. BoundaryWall implementation correctly implements "seen now only, no memory trace" per D-059. Two must-fix issues: protocol version mismatch and missing pause state.

#	File:line	Severity	Issue
1	`protocol.gd:14`	critical	`PROTOCOL_VERSION = 17` — server is 18. Snapshot decode will push_error and return null on every tick.
2	`debug_console.gd:96-109`	warning	Console swallows all keyboard input when open — this is correct (commands like `tp the-last-shift` contain WASD). But the console does not trigger a D-088 pause state transition. The sim keeps advancing while you type debug commands, which defeats the purpose of `AdvanceTicks` and `SkipToContamination`. Must add D-088 Overlay pause when console opens, unpause when closed — consistent with settings/journal.
3	`settings_dialog.gd:127-130`	warning	Double-read pattern — reads ConfigFile directly instead of live DebugConsole state. Checkbox and actual state could diverge.

Verdict: CHANGES REQUESTED

Must fix:

PROTOCOL_VERSION = 18 in protocol.gd:14 — one-line fix, blocks the entire client from functioning
Debug console must trigger D-088 Overlay pause state when opened — sim advancing during debug input breaks time-manipulation commands and is inconsistent with other overlays

## Re-review: `client` → `main` — Sprint 23 full client PR ### Hoshe (Code Quality): APPROVE (conditional on critical #1) **Summary:** All 3 warnings from the previous review are resolved. The 4 tickets are well-implemented with good test coverage. One operational issue: PROTOCOL_VERSION still 17. Previous review fixes verified: color registry clear, test coverage, ordering dependency — all resolved. | # | File:line | Severity | Issue | |---|-----------|----------|-------| | 1 | `protocol.gd:14` | **critical** | `PROTOCOL_VERSION = 17` but server is 18. Client will reject every snapshot — game unplayable. One-line fix: change to 18. | | 2 | `debug_console.gd:189-191` | suggestion | `tp` with invalid z silently defaults to 0 — harmless but unhelpful feedback. | | 3 | `debug_console.gd:236-237` | suggestion | `append_response` auto-opens console even if user disabled it via settings. | | 4 | `settings_dialog.gd:127-130` | suggestion | Reads prefs from ConfigFile directly instead of querying live DebugConsole state. | ### Tyre (Architecture): REQUEST_CHANGES **Summary:** Three tickets (#573, #574, #581) are solid. Debug console is well-encapsulated with correct D-020 flow. BoundaryWall implementation correctly implements "seen now only, no memory trace" per D-059. Two must-fix issues: protocol version mismatch and missing pause state. | # | File:line | Severity | Issue | |---|-----------|----------|-------| | 1 | `protocol.gd:14` | **critical** | `PROTOCOL_VERSION = 17` — server is 18. Snapshot decode will push_error and return null on every tick. | | 2 | `debug_console.gd:96-109` | **warning** | Console swallows all keyboard input when open — this is correct (commands like `tp the-last-shift` contain WASD). But the console does not trigger a D-088 pause state transition. The sim keeps advancing while you type debug commands, which defeats the purpose of `AdvanceTicks` and `SkipToContamination`. Must add D-088 Overlay pause when console opens, unpause when closed — consistent with settings/journal. | | 3 | `settings_dialog.gd:127-130` | warning | Double-read pattern — reads ConfigFile directly instead of live DebugConsole state. Checkbox and actual state could diverge. | ### Verdict: CHANGES REQUESTED **Must fix:** 1. `PROTOCOL_VERSION = 18` in `protocol.gd:14` — one-line fix, blocks the entire client from functioning 2. Debug console must trigger D-088 Overlay pause state when opened — sim advancing during debug input breaks time-manipulation commands and is inconsistent with other overlays

jpmschweitzer added 3 commits 2026-03-04 22:32:12 +01:00

fix(client): bump PROTOCOL_VERSION 17 → 18 to match server 1feec914b7

Server #580 bumped to 18 for debug_response field. Client was still
at 17, causing every snapshot to be rejected — game unplayable.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

fix(client): debug console review fixes — D-088 pause, settings state, response guard 388df8000a

- Add D-088 Overlay pause/unpause signals to DebugConsole, wire in main.gd
  so sim does not advance while typing debug commands
- Settings dialog reads live DebugConsole.is_enabled() instead of ConfigFile
  directly, preventing checkbox/state divergence
- append_response respects disabled state — no auto-open when user disabled
  console via settings
- tp command warns on invalid z value instead of silently defaulting to 0

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

chore(meta): update changelog 1f52f0b00d

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

jpmschweitzer commented

2026-03-04 22:42:56 +01:00

Re-review: `client` → `main` — after fix commits `1feec91`, `388df80`

Hoshe (Code Quality): APPROVE

Summary: All 4 items from the previous review are confirmed fixed. PROTOCOL_VERSION is 18, D-088 pause signals are correctly wired, settings dialog reads live console state, and invalid z gives user feedback. No new critical or warning issues found.

Previous review fixes verified: protocol version, D-088 pause, settings live state, tp invalid z — all resolved.

#	File:line	Severity	Issue
1	`debug_console.gd:242-248`	suggestion	`append_response` auto-open emits `pause_requested` — if dialogue overlay is already paused, the subsequent `unpause_requested` on console close could unbalance the pause state.
2	`settings_dialog.gd:128`	suggestion	Hardcoded path `/root/Main/ModalLayer/DebugConsole` — silently falls back to prefs if scene tree changes.
3	`fog_state.gd:166-171`	suggestion	BoundaryWall tiles not included in `_grow_bounds_from_positions` — correctness relies on implicit 8-tile margin; worth a comment.

Tyre (Architecture): APPROVE

Summary: All 3 critical/warning items fixed. D-088 pause correctly wired through server-authoritative path (same handlers as dialogue). BoundaryWall implementation is architecturally clean — correct split of concerns across game_state/fog_state/tile_renderer. Five suggestion-level observations, none blocking.

#	File:line	Severity	Issue
1	`debug_console.gd:247`	suggestion	`append_response` auto-open emits `pause_requested` regardless of whether another overlay pause is already live — risk of unbalanced unpause during active dialogue.
2	`settings_dialog.gd:128`	suggestion	Hardcoded absolute node path — fragile to scene restructuring; better to inject state via `main.gd`.
3	`fog_state.gd:~165`	suggestion	BoundaryWall tiles rely on implicit 8-tile margin contract for bounds inclusion — worth a comment.
4	`test_fog_sprint22.gd:650`	suggestion	2ms headless performance budget may be inconsistent with known ~30ms headless baseline.
5	`tile_renderer.gd:82`	suggestion	`clear()` on every update — confirm intentional (project notes mention removing it for fog compositing).

Verdict: APPROVED

## Re-review: `client` → `main` — after fix commits 1feec91, 388df80 ### Hoshe (Code Quality): APPROVE **Summary:** All 4 items from the previous review are confirmed fixed. PROTOCOL_VERSION is 18, D-088 pause signals are correctly wired, settings dialog reads live console state, and invalid z gives user feedback. No new critical or warning issues found. Previous review fixes verified: protocol version, D-088 pause, settings live state, tp invalid z — all resolved. | # | File:line | Severity | Issue | |---|-----------|----------|-------| | 1 | `debug_console.gd:242-248` | suggestion | `append_response` auto-open emits `pause_requested` — if dialogue overlay is already paused, the subsequent `unpause_requested` on console close could unbalance the pause state. | | 2 | `settings_dialog.gd:128` | suggestion | Hardcoded path `/root/Main/ModalLayer/DebugConsole` — silently falls back to prefs if scene tree changes. | | 3 | `fog_state.gd:166-171` | suggestion | BoundaryWall tiles not included in `_grow_bounds_from_positions` — correctness relies on implicit 8-tile margin; worth a comment. | ### Tyre (Architecture): APPROVE **Summary:** All 3 critical/warning items fixed. D-088 pause correctly wired through server-authoritative path (same handlers as dialogue). BoundaryWall implementation is architecturally clean — correct split of concerns across game_state/fog_state/tile_renderer. Five suggestion-level observations, none blocking. | # | File:line | Severity | Issue | |---|-----------|----------|-------| | 1 | `debug_console.gd:247` | suggestion | `append_response` auto-open emits `pause_requested` regardless of whether another overlay pause is already live — risk of unbalanced unpause during active dialogue. | | 2 | `settings_dialog.gd:128` | suggestion | Hardcoded absolute node path — fragile to scene restructuring; better to inject state via `main.gd`. | | 3 | `fog_state.gd:~165` | suggestion | BoundaryWall tiles rely on implicit 8-tile margin contract for bounds inclusion — worth a comment. | | 4 | `test_fog_sprint22.gd:650` | suggestion | 2ms headless performance budget may be inconsistent with known ~30ms headless baseline. | | 5 | `tile_renderer.gd:82` | suggestion | `clear()` on every update — confirm intentional (project notes mention removing it for fog compositing). | ### Verdict: APPROVED

jpmschweitzer closed this pull request

2026-03-04 22:43:29 +01:00

Pull request closed

This pull request cannot be reopened because the branch was deleted.

Sign in to join this conversation.

1 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: jpmschweitzer/settled-reach#82

feat(ui): entity-bound speaker colors + entity renderer test fixes (#573, #574) #82

Sprint 23 client deliverables

#573 — Bind dialogue speaker colors to entity identity

#574 — Fix pre-existing test failures in test_entity_renderer

Review: client → main (type: code)

Hoshe (Code Quality): REQUEST_CHANGES

Tyre (Architecture): REQUEST_CHANGES

Verdict: CHANGES REQUESTED

Re-review: client → main — Sprint 23 full client PR

Hoshe (Code Quality): APPROVE (conditional on critical #1)

Tyre (Architecture): REQUEST_CHANGES

Verdict: CHANGES REQUESTED

Re-review: client → main — after fix commits 1feec91, 388df80

Hoshe (Code Quality): APPROVE

Tyre (Architecture): APPROVE

Verdict: APPROVED

Pull request closed

Review: `client` → `main` (type: code)

Re-review: `client` → `main` — Sprint 23 full client PR

Re-review: `client` → `main` — after fix commits `1feec91`, `388df80`