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

feat(client): Sprint 12 — sound pipeline, indicators, renderer fix #43

Closed
jpmschweitzer wants to merge 0 commits from client into main
pull from: client
merge into: :main
Conversation 2 Commits - Files Changed -
+1022 -5
jpmschweitzer commented 2026-02-19 15:06:39 +01:00
Owner
Copy Link
Copy Source

Summary

Sprint 12 client delivery — sound event pipeline, medium-range indicators, renderer fix, and dialogue width resolution.

  • #345: Fix entity_renderer.gd field name bug (id → entity_id) with 5 regression tests
  • #447 / D-076: Resolve OQ-29 — dialogue max-width set to 640px, wired to dialogue_box.gd, .tscn offsets updated
  • #125: Close-range stereo audio — snapshot partitioning, AudioManager.play_sound_event(), D-067 recognition chime on UISounds bus, SOUND_EVENT_ASSETS registry
  • #126: Medium-range visual indicators — fog-edge directional arrows with D-018 color coding, 3.5s lifetime with fade, append+dedup pattern

Bug fixes found in pre-PR review

  • draw_colored_polygon → draw_polygon (runtime crash in indicator renderer)
  • Indicator _indicators.clear() on every tick killed indicators after ~100ms — changed to append+dedup so 3.5s lifetime actually works
  • dialogue_box.gd hardcoded MAX_WIDTH_PX = 832 instead of referencing Constants.DIALOGUE_MAX_WIDTH
  • Missing has("entity_id") guard in _play_recognition_chimes()
  • _color_for_type renamed to public color_for_type (testable API)
  • decisions/perception.md footer typo (1920px → 640px)

Test coverage

  • test_audio_bus_routing.gd (293 lines): D-068 bus constants, D-069 dip profiles, D-067 chime, snapshot integration
  • test_rendering.gd (+199 lines): #345 regression suite, #126 indicator data layer, GameState sound event partitioning
  • test_sound_indicators.gd (236 lines): D-018 color spec, range routing, direction geometry, indicator script API

Review

  • Tyre (architecture): APPROVE — clean pipeline, all decisions compliant
  • Hoshe (QA): APPROVE after fixes — 2 warnings resolved, 5 suggestions noted
## Summary Sprint 12 client delivery — sound event pipeline, medium-range indicators, renderer fix, and dialogue width resolution. - **#345**: Fix entity_renderer.gd field name bug (`id` → `entity_id`) with 5 regression tests - **#447 / D-076**: Resolve OQ-29 — dialogue max-width set to 640px, wired to dialogue_box.gd, .tscn offsets updated - **#125**: Close-range stereo audio — snapshot partitioning, AudioManager.play_sound_event(), D-067 recognition chime on UISounds bus, SOUND_EVENT_ASSETS registry - **#126**: Medium-range visual indicators — fog-edge directional arrows with D-018 color coding, 3.5s lifetime with fade, append+dedup pattern ### Bug fixes found in pre-PR review - `draw_colored_polygon` → `draw_polygon` (runtime crash in indicator renderer) - Indicator `_indicators.clear()` on every tick killed indicators after ~100ms — changed to append+dedup so 3.5s lifetime actually works - `dialogue_box.gd` hardcoded `MAX_WIDTH_PX = 832` instead of referencing `Constants.DIALOGUE_MAX_WIDTH` - Missing `has("entity_id")` guard in `_play_recognition_chimes()` - `_color_for_type` renamed to public `color_for_type` (testable API) - decisions/perception.md footer typo (1920px → 640px) ### Test coverage - `test_audio_bus_routing.gd` (293 lines): D-068 bus constants, D-069 dip profiles, D-067 chime, snapshot integration - `test_rendering.gd` (+199 lines): #345 regression suite, #126 indicator data layer, GameState sound event partitioning - `test_sound_indicators.gd` (236 lines): D-018 color spec, range routing, direction geometry, indicator script API ### Review - Tyre (architecture): APPROVE — clean pipeline, all decisions compliant - Hoshe (QA): APPROVE after fixes — 2 warnings resolved, 5 suggestions noted
jpmschweitzer added 4 commits 2026-02-19 15:06:39 +01:00
feat(client): Sprint 12 — renderer fix, sound pipeline, medium-range indicators c6bd7c2db2 
#345: entity_renderer.gd already used entity_id; added regression tests
  confirming old "id" field is rejected and "entity_id" is accepted.

#447 (OQ-29): DIALOGUE_MAX_WIDTH = 1920 added to constants.gd. Full
  viewport width at target resolution (60 × TILE_SIZE), per D-061 Lead
  directive "max-width". Recorded as D-076 in decisions/perception.md.

#126: SoundIndicatorRenderer — fog-edge directional arrows for medium-range
  sound events (D-018). Node2D at z:951 in World scene. Color-coded per
  D-018/D-069 (neutral/voice/danger). GameState.medium_sound_events
  partitions Medium events from snapshot sound_events field. Tests added
  to test_rendering.gd; Hoshe's test_sound_indicators.gd stubs updated.

#125: Close-range stereo audio pipeline wired. AudioManager.play_sound_event()
  maps event_type to D-038 asset key (Footstep/FootstepSprint → sfx_footstep_*).
  GameState.close_sound_events partitions Close events. main.gd calls
  _play_close_sound_events() each snapshot tick. test_audio_bus_routing.gd
  Layer 4 stubs upgraded to real tests.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
fix(client): apply arch review corrections — 640px dialogue width, D-067 chime 719721cd4c 
DIALOGUE_MAX_WIDTH: correct to 640px (20 × TILE_SIZE) per Tyre architecture
  review. D-076 updated in decisions/perception.md with amendment note.
  Initial 1920px was D-061 "max-width" but readability wins at 640px.

D-067 recognition chime: wire sfx_monologue_chime to fog entity recognition
  onset. AudioManager.CHIME_RECOGNITION constant added. main.gd tracks seen
  entity IDs in _known_recognition_ids; fires chime on first appearance in
  pending_recognitions, expires when entity leaves the queue. UISounds bus
  (not WorldSFX) per D-038 "monologue chime is a UI sound." Tests added
  to test_audio_bus_routing.gd (Layer 2b).

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
fix(client): correct 5 bugs found in pre-PR review f337bb3ca8 
- sound_indicator_renderer: draw_colored_polygon → draw_polygon (runtime crash)
- sound_indicator_renderer: append+dedup instead of clear — indicators now
  survive 3.5s instead of dying after one server tick
- dialogue_box: wire Constants.DIALOGUE_MAX_WIDTH (640px) instead of
  hardcoded 832px MAX_WIDTH_PX
- dialogue_box.tscn: update default offsets to ±320 (was ±416)
- decisions/perception.md: fix footer typo (1920px → 640px)
- tests updated for new append/dedup indicator behavior

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
fix(client): address PR review warnings — field guard + public API rename e1a1a9cfc2 
- main.gd: add has("entity_id") guard to _play_recognition_chimes()
  (matches defensive pattern in _play_close_sound_events and update_sound_events)
- sound_indicator_renderer.gd: rename _color_for_type → color_for_type
  (public testable API, not an internal-only method)
- test_rendering.gd: update test calls to match rename

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
jpmschweitzer commented 2026-02-19 15:28:52 +01:00
Author
Owner
Copy Link
Copy Source

Review: client -> main (type: code) — PR #43

Verdict: CHANGES REQUESTED

Hoshe (Code Quality): REQUEST_CHANGES

The PR delivers four coherent feature tickets with good structural decisions — D-069 dip values are correct, recognition chime architecture matches D-067, indicator z-ordering is correctly above fog. One warning-level issue requires a fix before merge.

# File Severity Issue
1 audio_manager.gd ~line 149 warning SOUND_EVENT_ASSETS key mismatch: maps "Footstep" / "FootstepWalk" etc. to "sfx_footstep_metal", but the actual audio file is sfx_footstep_metal_walk.ogg. Registry scan uses file_name.get_basename() producing key "sfx_footstep_metal_walk". Lookup in play_sound_event will always miss. The _run variant (sfx_footstep_metal_run) is correct. Fix: update walk-speed keys to "sfx_footstep_metal_walk".
2 sound_indicator_renderer.gd ~line 74 suggestion get_camera_2d() null fallback uses magic Vector2(2.0, 2.0). Works correctly now but not sourced from any constant. Consider adding a Constants.CAMERA_DEFAULT_ZOOM or documenting the assumption.
3 audio_manager.gd ~line 149 suggestion SOUND_EVENT_ASSETS maps all walk/careful/crouch stances to the same asset key. D-053 implies distinct sound profiles per stance. Worth tracking as a future audio differentiation ticket.
4 main.gd ~line 212 suggestion _play_close_sound_events() clears GameState.close_sound_events = [] after consuming. Consistent with existing consume-once patterns (current_monologue, current_dialogue). A brief comment noting consume-once semantics would help future readers.

Tyre (Architecture): APPROVE

Well-structured sprint landing four tickets cleanly. Audio pipeline (D-068/D-069), close-range positional audio (#125), and medium-range indicators (#126) all conform to their respective decisions. D-076 dialogue width fix is correctly derived and properly documented as a decision amendment. Test coverage is substantive.

# File Severity Issue
1 sound_indicator_renderer.gd:30-32 suggestion Color constants COLOR_NEUTRAL, COLOR_VOICE, COLOR_DANGER duplicate values from Constants.INSERT_COLOR_TEXT, Constants.ENTITY_COLOR_POI, Constants.ENTITY_COLOR_HOSTILE. Source from Constants to prevent palette drift on future adjustments.
2 audio_manager.gd:139-150 suggestion play_loop() casts stream.duplicate() as AudioStream — if duplicate fails, null propagates silently to a mute player. Add null guard: if loop_stream == null: return null.
3 main.gd:212 suggestion GameState.close_sound_events = [] is consume-once by design. Consistent with existing pattern, but worth a comment noting D-009 multiplayer implications if a second observer reads the same state.

Deduplicated action items

  1. WARNING — SOUND_EVENT_ASSETS walk-speed key mismatch: "sfx_footstep_metal" should be "sfx_footstep_metal_walk" to match the actual audio filename.
  2. Suggestion — Indicator colors should reference Constants instead of duplicating hex literals.
  3. Suggestion — play_loop() null guard on stream.duplicate().
  4. Suggestion — Camera zoom fallback magic number.
  5. Suggestion — Document consume-once semantics on close_sound_events.
  6. Suggestion — Track stance-differentiated audio as future work.
## Review: client -> main (type: code) — PR #43 **Verdict: CHANGES REQUESTED** --- ### Hoshe (Code Quality): REQUEST_CHANGES The PR delivers four coherent feature tickets with good structural decisions — D-069 dip values are correct, recognition chime architecture matches D-067, indicator z-ordering is correctly above fog. One warning-level issue requires a fix before merge. | # | File | Severity | Issue | |---|------|----------|-------| | 1 | `audio_manager.gd` ~line 149 | warning | `SOUND_EVENT_ASSETS` key mismatch: maps `"Footstep"` / `"FootstepWalk"` etc. to `"sfx_footstep_metal"`, but the actual audio file is `sfx_footstep_metal_walk.ogg`. Registry scan uses `file_name.get_basename()` producing key `"sfx_footstep_metal_walk"`. Lookup in `play_sound_event` will always miss. The `_run` variant (`sfx_footstep_metal_run`) is correct. Fix: update walk-speed keys to `"sfx_footstep_metal_walk"`. | | 2 | `sound_indicator_renderer.gd` ~line 74 | suggestion | `get_camera_2d()` null fallback uses magic `Vector2(2.0, 2.0)`. Works correctly now but not sourced from any constant. Consider adding a `Constants.CAMERA_DEFAULT_ZOOM` or documenting the assumption. | | 3 | `audio_manager.gd` ~line 149 | suggestion | `SOUND_EVENT_ASSETS` maps all walk/careful/crouch stances to the same asset key. D-053 implies distinct sound profiles per stance. Worth tracking as a future audio differentiation ticket. | | 4 | `main.gd` ~line 212 | suggestion | `_play_close_sound_events()` clears `GameState.close_sound_events = []` after consuming. Consistent with existing consume-once patterns (`current_monologue`, `current_dialogue`). A brief comment noting consume-once semantics would help future readers. | ### Tyre (Architecture): APPROVE Well-structured sprint landing four tickets cleanly. Audio pipeline (D-068/D-069), close-range positional audio (#125), and medium-range indicators (#126) all conform to their respective decisions. D-076 dialogue width fix is correctly derived and properly documented as a decision amendment. Test coverage is substantive. | # | File | Severity | Issue | |---|------|----------|-------| | 1 | `sound_indicator_renderer.gd:30-32` | suggestion | Color constants `COLOR_NEUTRAL`, `COLOR_VOICE`, `COLOR_DANGER` duplicate values from `Constants.INSERT_COLOR_TEXT`, `Constants.ENTITY_COLOR_POI`, `Constants.ENTITY_COLOR_HOSTILE`. Source from Constants to prevent palette drift on future adjustments. | | 2 | `audio_manager.gd:139-150` | suggestion | `play_loop()` casts `stream.duplicate() as AudioStream` — if duplicate fails, null propagates silently to a mute player. Add null guard: `if loop_stream == null: return null`. | | 3 | `main.gd:212` | suggestion | `GameState.close_sound_events = []` is consume-once by design. Consistent with existing pattern, but worth a comment noting D-009 multiplayer implications if a second observer reads the same state. | ### Deduplicated action items 1. **WARNING** — `SOUND_EVENT_ASSETS` walk-speed key mismatch: `"sfx_footstep_metal"` should be `"sfx_footstep_metal_walk"` to match the actual audio filename. 2. Suggestion — Indicator colors should reference `Constants` instead of duplicating hex literals. 3. Suggestion — `play_loop()` null guard on `stream.duplicate()`. 4. Suggestion — Camera zoom fallback magic number. 5. Suggestion — Document consume-once semantics on `close_sound_events`. 6. Suggestion — Track stance-differentiated audio as future work.
jpmschweitzer added 1 commit 2026-02-19 15:41:12 +01:00
fix(client): address PR #43 review — walk key mismatch + 5 suggestions dfcafee3ee 
Fix SOUND_EVENT_ASSETS walk-speed keys to match actual filename
(sfx_footstep_metal_walk), add play_loop null guard, source indicator
colors from Constants, extract CAMERA_DEFAULT_ZOOM, document
consume-once semantics on close_sound_events.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
jpmschweitzer commented 2026-02-19 15:45:12 +01:00
Author
Owner
Copy Link
Copy Source

Re-Review: client -> main (type: code) — PR #43

Verdict: APPROVED

All 6 items from the previous review are verified fixed. No new critical or warning issues introduced.

Hoshe (Code Quality): APPROVE

All 6 items properly addressed. Walk key mismatch corrected, indicator colors sourced from Constants, play_loop() null guard added, camera zoom fallback uses Constants.CAMERA_DEFAULT_ZOOM, consume-once documented, stance audio gap flagged with D-053 inline comments.

# File Severity Issue
1 audio_manager.gd SOUND_EVENT_ASSETS suggestion FootstepCareful and FootstepCrouch keys are defensive additions — confirm server actually emits these event type strings. If not, they are harmless dead entries.

Tyre (Architecture): APPROVE

Pipeline is D-018/D-038/D-067/D-068/D-069/D-076 compliant. 5-bus architecture correct, recognition chime fires at onset per D-067, dip profiles match spec mid-range values, consume-once pattern documented with D-009 rationale.

# File Severity Issue
1 audio_manager.gd:180 suggestion D-038 lists asset as sfx_footstep_metal.ogg but code maps to sfx_footstep_metal_walk. Confirm audio branch filename matches. Safe either way (no-op fallback).
2 constants.gd suggestion D-076 referenced in code but not yet recorded in decisions/ domain files. Process gap, not a code defect.
3 audio_manager.gd:26 suggestion listening_focus tick gate is caller's responsibility per comment. No runtime enforcement — consider push_warning() in debug builds.

Previous review items — all verified

  1. WARNING — Walk key mismatch: FIXED. "sfx_footstep_metal" → "sfx_footstep_metal_walk".
  2. Suggestion — Indicator colors: FIXED. Now references Constants.INSERT_COLOR_TEXT, ENTITY_COLOR_POI, ENTITY_COLOR_HOSTILE.
  3. Suggestion — play_loop() null guard: FIXED. Returns null if stream.duplicate() fails.
  4. Suggestion — Camera zoom fallback: FIXED. Uses Constants.CAMERA_DEFAULT_ZOOM.
  5. Suggestion — Consume-once docs: FIXED. Full comment block on _play_close_sound_events().
  6. Suggestion — Stance audio tracking: FIXED. D-053 inline comments on FootstepCareful/FootstepCrouch entries.
## Re-Review: client -> main (type: code) — PR #43 **Verdict: APPROVED** All 6 items from the previous review are verified fixed. No new critical or warning issues introduced. --- ### Hoshe (Code Quality): APPROVE All 6 items properly addressed. Walk key mismatch corrected, indicator colors sourced from Constants, play_loop() null guard added, camera zoom fallback uses Constants.CAMERA_DEFAULT_ZOOM, consume-once documented, stance audio gap flagged with D-053 inline comments. | # | File | Severity | Issue | |---|------|----------|-------| | 1 | `audio_manager.gd` SOUND_EVENT_ASSETS | suggestion | `FootstepCareful` and `FootstepCrouch` keys are defensive additions — confirm server actually emits these event type strings. If not, they are harmless dead entries. | ### Tyre (Architecture): APPROVE Pipeline is D-018/D-038/D-067/D-068/D-069/D-076 compliant. 5-bus architecture correct, recognition chime fires at onset per D-067, dip profiles match spec mid-range values, consume-once pattern documented with D-009 rationale. | # | File | Severity | Issue | |---|------|----------|-------| | 1 | `audio_manager.gd:180` | suggestion | D-038 lists asset as `sfx_footstep_metal.ogg` but code maps to `sfx_footstep_metal_walk`. Confirm audio branch filename matches. Safe either way (no-op fallback). | | 2 | `constants.gd` | suggestion | D-076 referenced in code but not yet recorded in `decisions/` domain files. Process gap, not a code defect. | | 3 | `audio_manager.gd:26` | suggestion | `listening_focus` tick gate is caller's responsibility per comment. No runtime enforcement — consider `push_warning()` in debug builds. | ### Previous review items — all verified 1. **WARNING — Walk key mismatch**: FIXED. `"sfx_footstep_metal"` → `"sfx_footstep_metal_walk"`. 2. **Suggestion — Indicator colors**: FIXED. Now references `Constants.INSERT_COLOR_TEXT`, `ENTITY_COLOR_POI`, `ENTITY_COLOR_HOSTILE`. 3. **Suggestion — play_loop() null guard**: FIXED. Returns `null` if `stream.duplicate()` fails. 4. **Suggestion — Camera zoom fallback**: FIXED. Uses `Constants.CAMERA_DEFAULT_ZOOM`. 5. **Suggestion — Consume-once docs**: FIXED. Full comment block on `_play_close_sound_events()`. 6. **Suggestion — Stance audio tracking**: FIXED. D-053 inline comments on FootstepCareful/FootstepCrouch entries.
jpmschweitzer closed this pull request 2026-02-19 15:57:10 +01:00

Pull request closed

This pull request cannot be reopened because the branch was deleted.
Sign in to join this conversation.
Reviewers
No Reviewers
Labels
No items
No labels
Milestone
No items
No Milestone
Projects
Clear projects
No projects
Assignees
Clear assignees
jpmschweitzer (Jeroen Schweitzer)
No Assignees
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: jpmschweitzer/settled-reach#43