feat: EN-062 - spatial audio v2: live voices, real distance/pan model, doppler#116
Merged
Merged
Conversation
Voice ids on every play; play_sound_3d_ex + voice_set_position/volume/ pitch/lowpass/stop make looping, moveable emitters possible (river, wind, creatures). Equal-power pan (0.85 width), inverse-clamped distance model (legacy 1/d preserved at ref=1/rolloff=1), distance air absorption, rear head-shadow cue, doppler with teleport guard, per-block gain ramps. Fixes on the way through: - Stereo was MIRRORED: listener right was cross(up,fwd) = screen left. - Windows/WASAPI never told the renderer the device rate; with the new fractional resampler assets also now play at their authored pitch (44.1k files on 48k endpoints ran ~9% sharp since forever). 10 new tests; watchOS stubs regenerated; web target exports the same API. Claude-Session: https://claude.ai/code/session_015wJLdkwm71jQCAqypaFeNE
📝 WalkthroughWalkthroughAdds EN-062 spatial audio v2 with stable controllable voices, looping and fades, distance attenuation, panning, occlusion, doppler, fractional resampling, gain ramping, expanded bindings, platform support, and tests. ChangesSpatial audio v2
Sequence Diagram(s)sequenceDiagram
participant GameAudio
participant AudioAPI
participant NativeFFI
participant AudioMixer
participant AudioRenderer
GameAudio->>AudioAPI: Play or control spatial voice
AudioAPI->>NativeFFI: Forward voice parameters
NativeFFI->>AudioMixer: Emit voice command
AudioMixer->>AudioRenderer: Send command through audio ring
AudioRenderer->>AudioRenderer: Spatialize, resample, filter, and mix voice
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
native/shared/src/audio/render.rs (1)
668-683: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winDownmix spatial stereo sources before applying point-source panning.
Left input currently receives only
g_land right input onlyg_r. A stereo asset with content in one channel therefore cannot move across the stereo field; it merely changes level. Downmix spatial voices to mono before panning, or apply a proper stereo panning matrix.Proposed fix
let mut sl = s0l + (s1l - s0l) * frac; let mut sr = s0r + (s1r - s0r) * frac; + + if v.spatial.is_some() { + let mono = (sl + sr) * 0.5; + sl = mono; + sr = mono; + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@native/shared/src/audio/render.rs` around lines 668 - 683, Update the spatial rendering path around the `sl`/`sr` gain calculations so stereo source samples are downmixed to a mono signal before applying point-source panning. Use that mono value as the input to the existing `v.g_l` and `v.g_r` gains, ensuring either source channel can be positioned across the stereo field rather than remaining isolated to its original output channel.
🧹 Nitpick comments (1)
native/shared/src/audio/mod.rs (1)
845-862: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTest per-voice isolation with two voices sharing one sound.
The current separate-mixer comparison only proves filtering works; a broken per-sound implementation would still pass. Play a muffled and unfiltered voice from the same handle and assert the latter remains open.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@native/shared/src/audio/mod.rs` around lines 845 - 862, Update per_voice_lowpass_muffles_one_emitter_only to use a single AudioMixer and shared sound handle for both muffled and unfiltered voices. Apply set_voice_lowpass only to the first voice, mix their output separately or otherwise isolate each voice’s result, and assert the unfiltered voice retains the open response so per-voice isolation is verified.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/tickets.md`:
- Line 2047: Update the first-consumer ticket reference in the “web target
exports the same six calls” record from SH-050 to SH-052, leaving the
surrounding text unchanged.
In `@native/shared/src/audio/mod.rs`:
- Around line 159-174: Update the voice lifecycle flow around send_play and the
Cmd::PlaySound/StopVoice paths so play returns failure when the command cannot
be enqueued, and stopping a voice cannot be permanently lost under ring
pressure. Guarantee delivery or coalesce lifecycle commands separately from
lossy updates, preserving the existing voice ID behavior only for successfully
enqueued plays.
In `@native/shared/src/audio/render.rs`:
- Around line 694-700: Update the looping branch in the frame-position
advancement logic around v.frame_pos so wrapping handles steps equal to or
larger than the full frames count, keeping looping positions within bounds
instead of subtracting frames only once. Preserve the existing ended-and-break
behavior for non-looping voices.
---
Outside diff comments:
In `@native/shared/src/audio/render.rs`:
- Around line 668-683: Update the spatial rendering path around the `sl`/`sr`
gain calculations so stereo source samples are downmixed to a mono signal before
applying point-source panning. Use that mono value as the input to the existing
`v.g_l` and `v.g_r` gains, ensuring either source channel can be positioned
across the stereo field rather than remaining isolated to its original output
channel.
---
Nitpick comments:
In `@native/shared/src/audio/mod.rs`:
- Around line 845-862: Update per_voice_lowpass_muffles_one_emitter_only to use
a single AudioMixer and shared sound handle for both muffled and unfiltered
voices. Apply set_voice_lowpass only to the first voice, mix their output
separately or otherwise isolate each voice’s result, and assert the unfiltered
voice retains the open response so per-voice isolation is verified.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: d51ed9ea-5e81-41dc-84c1-bc2aac2c7dae
📒 Files selected for processing (9)
docs/tickets.mdnative/shared/src/audio/mod.rsnative/shared/src/audio/render.rsnative/shared/src/ffi_core/audio_ffi.rsnative/watchos/src/ffi_stubs.rsnative/web/src/lib.rsnative/windows/src/lib.rspackage.jsonsrc/audio/index.ts
| Tests: 10 new (loop lifecycle, stereo crossing, legacy-curve equivalence, | ||
| equal-power, air absorption, rear cue, pitch rate, doppler zero-crossing | ||
| count, max-dist cull/return, per-voice occlusion). watchOS stubs regenerated; | ||
| web target exports the same six calls. First consumer: shooter SH-050. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Correct the first-consumer ticket reference.
The PR objective identifies the shooter integration as SH-052, but this record says SH-050.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/tickets.md` at line 2047, Update the first-consumer ticket reference in
the “web target exports the same six calls” record from SH-050 to SH-052,
leaving the surrounding text unchanged.
Comment on lines
+159
to
+174
| self.next_voice += 1; | ||
| let voice_id = self.next_voice; | ||
| self.send(Cmd::PlaySound { | ||
| sound_id: handle.to_bits(), | ||
| voice_id, | ||
| data, | ||
| volume, | ||
| spatial: Some([x, y, z]), | ||
| spatial, | ||
| looping, | ||
| ref_dist, | ||
| max_dist, | ||
| rolloff, | ||
| pitch: 1.0, | ||
| bus, send, lowpass, | ||
| }); | ||
| voice_id as f64 |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
Make voice creation and stopping reliable under ring pressure.
send_play returns a nonzero ID even when PlaySound is dropped, while a dropped StopVoice can leave a looping emitter audible indefinitely. Return failure when play cannot enqueue and guarantee or coalesce lifecycle commands rather than using the lossy update path.
Also applies to: 212-216
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@native/shared/src/audio/mod.rs` around lines 159 - 174, Update the voice
lifecycle flow around send_play and the Cmd::PlaySound/StopVoice paths so play
returns failure when the command cannot be enqueued, and stopping a voice cannot
be permanently lost under ring pressure. Guarantee delivery or coalesce
lifecycle commands separately from lossy updates, preserving the existing voice
ID behavior only for successfully enqueued plays.
Comment on lines
+694
to
+700
| v.frame_pos += step; | ||
| if v.frame_pos >= frames as f64 { | ||
| if v.looping { | ||
| v.frame_pos -= frames as f64; | ||
| } else { | ||
| ended = true; | ||
| break; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Wrap looping playback by the full frame count.
When step >= frames, subtracting frames once can leave frame_pos out of bounds. The next iteration exits without advancing, leaving the looping voice retained but permanently silent.
Proposed fix
if v.frame_pos >= frames as f64 {
if v.looping {
- v.frame_pos -= frames as f64;
+ v.frame_pos %= frames as f64;
} else {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| v.frame_pos += step; | |
| if v.frame_pos >= frames as f64 { | |
| if v.looping { | |
| v.frame_pos -= frames as f64; | |
| } else { | |
| ended = true; | |
| break; | |
| v.frame_pos += step; | |
| if v.frame_pos >= frames as f64 { | |
| if v.looping { | |
| v.frame_pos %= frames as f64; | |
| } else { | |
| ended = true; | |
| break; |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@native/shared/src/audio/render.rs` around lines 694 - 700, Update the looping
branch in the frame-position advancement logic around v.frame_pos so wrapping
handles steps equal to or larger than the full frames count, keeping looping
positions within bounds instead of subtracting frames only once. Preserve the
existing ended-and-break behavior for non-looping voices.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
The mixer could only fire-and-forget. This adds live spatial voices — looping, moveable emitters with per-voice volume/pitch/low-pass and a click-free stop — plus a real spatialization model, all engine-side:
play_sound_3d_ex+voice_set_position/volume/pitch/lowpass/stopBugs fixed on the way through
cross(up, fwd)— screen LEFT permat4_look_at's ownsvector. Every spatial sound panned to the wrong side. Now pinned by a test.Testing
23/23 audio tests (10 new: loop lifecycle, stereo crossing, legacy-curve equivalence, equal-power, air absorption, rear cue, pitch rate, doppler zero-crossing count, max-dist cull/return, per-voice occlusion). watchOS stubs regenerated; web target exports the same six calls. First consumer: shooter SH-052 (verified live in-game there).
https://claude.ai/code/session_015wJLdkwm71jQCAqypaFeNE
Summary by CodeRabbit