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

feat(audio): Sprint 13 client — AudioManager, crossfade, dip profiles (#528-#533) #47

Closed
jpmschweitzer wants to merge 0 commits from client into main
pull from: client
merge into: :main
Conversation 1 Commits - Files Changed -
jpmschweitzer commented 2026-02-19 18:20:04 +01:00
Owner
Copy Link
Copy Source

Summary

Sprint 13 client team — full audio architecture implementation.

  • #528 AudioManager: Volume slider persistence (ConfigFile), settings UI with 5 teal-bordered sliders, default_bus_layout.tres for editor. 5-bus setup: Music, Ambient, WorldSFX, PlayerActions, UISounds (D-068).
  • #529 Zone crossfade: Defensive zone_id read from snapshot tiles, 1.5-2s ambient tween. Auto-activates when server ships OQ-09 (#523). No client changes needed (D-073).
  • #530 Audio dip profiles: Dialogue dip in show/hide_dialogue(), confrontation override, ListeningFocus 30-tick gate via stationary_ticks in game_state.gd (D-069, D-070, D-071).
  • #531 Recognition chime: Verified already complete from prior sprint — fires at ONSET per D-067.
  • #533 NPC murmur: Client plumbing for event-driven World SFX playback, no-ops until audio asset arrives (#532).

Test plan

  • 57 new tests in test_audio_sprint13.gd — bus setup, dip profiles, zone crossfade, recognition chime, murmur wiring
  • 3 lambda capture fixes in test_audio_bus_routing.gd (String → Array wrapper for Godot 4 closure reliability)
  • before_test() bus volume reset for test isolation
  • Verify make ci passes
  • Manual: open settings, adjust 5 volume sliders, confirm persistence across restart
  • Manual: walk between zones, confirm 1.5-2s ambient crossfade (requires server zone_id)
  • Manual: open dialogue, confirm ambient dip; stand still 30+ ticks, confirm WorldSFX boost

External dependencies

  • Server OQ-09 (#523): zone_id in snapshot → zone crossfades activate
  • Audio #532: murmur asset → NPC conversation sounds activate
## Summary Sprint 13 client team — full audio architecture implementation. - **#528 AudioManager:** Volume slider persistence (ConfigFile), settings UI with 5 teal-bordered sliders, `default_bus_layout.tres` for editor. 5-bus setup: Music, Ambient, WorldSFX, PlayerActions, UISounds (D-068). - **#529 Zone crossfade:** Defensive `zone_id` read from snapshot tiles, 1.5-2s ambient tween. Auto-activates when server ships OQ-09 (#523). No client changes needed (D-073). - **#530 Audio dip profiles:** Dialogue dip in `show/hide_dialogue()`, confrontation override, ListeningFocus 30-tick gate via `stationary_ticks` in `game_state.gd` (D-069, D-070, D-071). - **#531 Recognition chime:** Verified already complete from prior sprint — fires at ONSET per D-067. - **#533 NPC murmur:** Client plumbing for event-driven World SFX playback, no-ops until audio asset arrives (#532). ## Test plan - [x] 57 new tests in `test_audio_sprint13.gd` — bus setup, dip profiles, zone crossfade, recognition chime, murmur wiring - [x] 3 lambda capture fixes in `test_audio_bus_routing.gd` (String → Array wrapper for Godot 4 closure reliability) - [x] `before_test()` bus volume reset for test isolation - [ ] Verify `make ci` passes - [ ] Manual: open settings, adjust 5 volume sliders, confirm persistence across restart - [ ] Manual: walk between zones, confirm 1.5-2s ambient crossfade (requires server zone_id) - [ ] Manual: open dialogue, confirm ambient dip; stand still 30+ ticks, confirm WorldSFX boost ## External dependencies - Server OQ-09 (#523): `zone_id` in snapshot → zone crossfades activate - Audio #532: murmur asset → NPC conversation sounds activate
jpmschweitzer added 2 commits 2026-02-19 18:20:05 +01:00
feat(audio): Sprint 13 — AudioManager, zone crossfade, dip profiles, murmur wiring (#528, #529, #530, #533) 0e6cc4eef8 
AudioManager: volume slider persistence (ConfigFile), settings UI with
5 teal-bordered sliders, default_bus_layout.tres for editor. Zone
crossfade: defensive zone_id read from snapshot tiles, 1.5-2s ambient
tween, auto-activates when server ships OQ-09. Dip profiles: dialogue
dip in show/hide_dialogue, ListeningFocus 30-tick gate via
stationary_ticks in game_state.gd. NPC murmur: client plumbing for
event-driven World SFX playback, no-ops until audio asset arrives.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
test(audio): Sprint 13 test suite + lambda capture fix (#528-#533) 513682522f 
57 new tests in test_audio_sprint13.gd covering bus setup, dip profiles,
zone crossfade, recognition chime, and murmur wiring. Fix lambda capture
bug in 3 signal tests (String → Array wrapper for reliable closure
capture in Godot 4). Add bus volume reset to before_test() for test
isolation against prefs-loaded state.

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

Review: client -> main (PR #47)

Hoshe (Code Quality): REQUEST_CHANGES

Solid Sprint 13 audio implementation — 5-bus architecture correctly wired, dip profiles implemented, zone crossfade functional, and comprehensive test suite (560 lines). Issues with premature murmur wiring and untested edge cases.

# File Severity Issue
1 audio_manager.gd SOUND_EVENT_ASSETS warning Voice/VoiceConversation → sfx_npc_murmur entries added but D-072 explicitly defers NPC proximity murmur. Asset key sfx_npc_murmur is unconfirmed — if audio team delivers as sfx_npc_murmur_01.ogg, this becomes a silent permanent miss. Remove until #533 PR lands with confirmed asset
2 audio_manager.gd set_zone() warning Rapid back-and-forth zone crossing (D-073 spec concern) is untested. When _kill_zone_tweens() fires mid-fade, old player stays alive at intermediate volume — behavior is correct but the test suite never verifies this interruptible crossfade path
3 audio_manager.gd _load_prefs() warning Persistence load path is entirely untested. No test verifies _load_prefs() restores previously saved values on startup, or handles corrupted/missing config gracefully
4 main.gd _update_zone() warning O(N) linear scan through visible_tiles to find player tile. Existing code uses visible_positions Dictionary for O(1) lookups — zone lookup should follow the same pattern
5 audio_manager.gd _scan_dir() suggestion dir.list_dir_end() never called after scan completes. Godot 4 handles cleanup via refcounting but docs recommend explicit end
6 settings_dialog.gd _build_ui() suggestion Lambda capture of bus_name in slider loop — commit message says "lambda capture fix" but full file not verifiable from diff excerpt. Confirm bus_name is bound to a local before closure
7 test_audio_sprint13.gd Layer 4 suggestion Dip call site tests verify AudioManager in isolation but don't test actual dialogue_box.gd wiring. A scene-level test would catch regressions if the apply_dip("dialogue") call is removed
8 audio_manager.gd ZONE_ASSETS suggestion "workplace" → "amb_hub_layer" diverges from D-038 which names the asset amb_workplace_layer. Intentional consolidation per sprint brief but deserves a comment. Also amb_station_base (D-038 global base hum) has no entry — unclear if it plays globally elsewhere

Tyre (Architecture): REQUEST_CHANGES

Architecture follows D-068/D-069/D-071/D-073 correctly. Dip state machine is clean, proportional volume during active dip is correct, ListeningFocus gate is caller-responsibility as spec'd. Two architectural issues need fixing.

# File Severity Issue
1 audio_manager.gd SOUND_EVENT_ASSETS warning Voice/VoiceConversation entries violate D-072 deferral. These will fire before the eavesdropping information quality model (D-071: distance-dependent, stance-modified, ListeningFocus-boosted) is wired server-side. Remove — they belong in the sprint that implements D-072 event-driven murmur end-to-end
2 main.gd _update_zone() warning Runs in _process() unconditionally (60fps+), not inside if snapshot != null: block. Zone only changes on new snapshots — move inside snapshot block. Better: extract zone_id from player tile in GameState.apply_snapshot() as GameState.current_zone_id (like player_facing, player_stance), avoiding client re-derivation of server-authoritative state per D-020
3 audio_manager.gd set_zone() hub/workplace suggestion hub and workplace map to same asset amb_hub_layer — the old_asset != new_asset guard correctly skips fade-out on hub→workplace transition. Add a comment noting this intentional same-asset pattern
4 dialogue_box.gd _start_confrontation_beat suggestion Confrontation dip replaces (not nests) the dialogue dip. hide_dialogue's clear_dip restores to base volumes regardless of which profile was last active. Add a comment noting the intentional replacement semantics

Verdict: CHANGES REQUESTED

Key fixes before merge:

  1. Remove Voice/VoiceConversation from SOUND_EVENT_ASSETS — D-072 defers NPC proximity murmur to a future sprint
  2. Move _update_zone() inside the if snapshot != null: block — avoid per-frame tile scanning; ideally extract zone_id in GameState.apply_snapshot() as a first-class field
  3. Add rapid zone-crossing test coverage for D-073 interruptible crossfade path
  4. Add _load_prefs() persistence test (load after save roundtrip)
## Review: client -> main (PR #47) ### Hoshe (Code Quality): REQUEST_CHANGES Solid Sprint 13 audio implementation — 5-bus architecture correctly wired, dip profiles implemented, zone crossfade functional, and comprehensive test suite (560 lines). Issues with premature murmur wiring and untested edge cases. | # | File | Severity | Issue | |---|------|----------|-------| | 1 | `audio_manager.gd` SOUND_EVENT_ASSETS | warning | `Voice`/`VoiceConversation` → `sfx_npc_murmur` entries added but D-072 explicitly defers NPC proximity murmur. Asset key `sfx_npc_murmur` is unconfirmed — if audio team delivers as `sfx_npc_murmur_01.ogg`, this becomes a silent permanent miss. Remove until #533 PR lands with confirmed asset | | 2 | `audio_manager.gd` `set_zone()` | warning | Rapid back-and-forth zone crossing (D-073 spec concern) is untested. When `_kill_zone_tweens()` fires mid-fade, old player stays alive at intermediate volume — behavior is correct but the test suite never verifies this interruptible crossfade path | | 3 | `audio_manager.gd` `_load_prefs()` | warning | Persistence load path is entirely untested. No test verifies `_load_prefs()` restores previously saved values on startup, or handles corrupted/missing config gracefully | | 4 | `main.gd` `_update_zone()` | warning | O(N) linear scan through `visible_tiles` to find player tile. Existing code uses `visible_positions` Dictionary for O(1) lookups — zone lookup should follow the same pattern | | 5 | `audio_manager.gd` `_scan_dir()` | suggestion | `dir.list_dir_end()` never called after scan completes. Godot 4 handles cleanup via refcounting but docs recommend explicit end | | 6 | `settings_dialog.gd` `_build_ui()` | suggestion | Lambda capture of `bus_name` in slider loop — commit message says "lambda capture fix" but full file not verifiable from diff excerpt. Confirm `bus_name` is bound to a local before closure | | 7 | `test_audio_sprint13.gd` Layer 4 | suggestion | Dip call site tests verify AudioManager in isolation but don't test actual `dialogue_box.gd` wiring. A scene-level test would catch regressions if the `apply_dip("dialogue")` call is removed | | 8 | `audio_manager.gd` ZONE_ASSETS | suggestion | `"workplace"` → `"amb_hub_layer"` diverges from D-038 which names the asset `amb_workplace_layer`. Intentional consolidation per sprint brief but deserves a comment. Also `amb_station_base` (D-038 global base hum) has no entry — unclear if it plays globally elsewhere | ### Tyre (Architecture): REQUEST_CHANGES Architecture follows D-068/D-069/D-071/D-073 correctly. Dip state machine is clean, proportional volume during active dip is correct, ListeningFocus gate is caller-responsibility as spec'd. Two architectural issues need fixing. | # | File | Severity | Issue | |---|------|----------|-------| | 1 | `audio_manager.gd` SOUND_EVENT_ASSETS | warning | `Voice`/`VoiceConversation` entries violate D-072 deferral. These will fire before the eavesdropping information quality model (D-071: distance-dependent, stance-modified, ListeningFocus-boosted) is wired server-side. Remove — they belong in the sprint that implements D-072 event-driven murmur end-to-end | | 2 | `main.gd` `_update_zone()` | warning | Runs in `_process()` unconditionally (60fps+), not inside `if snapshot != null:` block. Zone only changes on new snapshots — move inside snapshot block. Better: extract `zone_id` from player tile in `GameState.apply_snapshot()` as `GameState.current_zone_id` (like `player_facing`, `player_stance`), avoiding client re-derivation of server-authoritative state per D-020 | | 3 | `audio_manager.gd` `set_zone()` hub/workplace | suggestion | `hub` and `workplace` map to same asset `amb_hub_layer` — the `old_asset != new_asset` guard correctly skips fade-out on hub→workplace transition. Add a comment noting this intentional same-asset pattern | | 4 | `dialogue_box.gd` `_start_confrontation_beat` | suggestion | Confrontation dip replaces (not nests) the dialogue dip. `hide_dialogue`'s `clear_dip` restores to base volumes regardless of which profile was last active. Add a comment noting the intentional replacement semantics | ### Verdict: CHANGES REQUESTED **Key fixes before merge:** 1. Remove `Voice`/`VoiceConversation` from `SOUND_EVENT_ASSETS` — D-072 defers NPC proximity murmur to a future sprint 2. Move `_update_zone()` inside the `if snapshot != null:` block — avoid per-frame tile scanning; ideally extract `zone_id` in `GameState.apply_snapshot()` as a first-class field 3. Add rapid zone-crossing test coverage for D-073 interruptible crossfade path 4. Add `_load_prefs()` persistence test (load after save roundtrip)
jpmschweitzer added 1 commit 2026-02-19 18:37:28 +01:00
fix(client): address PR #47 review — murmur deferral, zone extraction, 6 tests 8985340bdd 
Remove premature Voice/VoiceConversation from SOUND_EVENT_ASSETS (D-072
defers murmur to end-to-end sprint). Extract current_zone_id in
GameState.apply_snapshot() as first-class field, eliminating O(N) tile
scan in main.gd (D-020 server-authoritative). Add dir.list_dir_end()
after registry scan. Add rapid zone-crossing + _load_prefs() roundtrip
tests. Enhance comments on hub/workplace same-asset pattern, station
base hum, and confrontation dip replacement semantics.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
jpmschweitzer closed this pull request 2026-02-19 18:46:02 +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#47