Repository navigation
feat(audio): route SoundGraph voices through the 3D spatializer (#424) - #471
Conversation
SoundGraph voices previously played non-positional: SoundGraphSound's SetLocation/SetVelocity/SetOrientation only stored the value, never reaching a spatializer. Audio::DSP::Spatializer existed but was built around an ma_engine_node (it reads resampler.channels and drives the engine node's built-in spatializer.dopplerPitch) and was wired to nothing, whereas a SoundGraphSource attaches a bare ma_node straight to the engine endpoint. Teach the spatializer to host a bare node: - Spatializer: add InitSource(ma_node_base*) beside the existing InitSource(ma_engine_node*); both delegate to a shared InitSourceInternal. The Doppler sink (ma_engine_node*) is now nullable and the doppler-pitch writes are null-guarded, so a bare node still gets full VBAP panning + distance/cone attenuation (computed inside the spatializer node) while Doppler is skipped. ReleaseSource re-routes via a new attachedNode field. - SoundGraphSource: host a per-voice spatializer node — RegisterSpatializer (insert between the SoundGraph node and the endpoint, allocate a process-unique sourceID), UpdateSpatialPosition, UnregisterSpatializer (also from Shutdown, before ma_node_uninit). - SoundGraphSound: route SetLocation/SetVelocity/SetOrientation through Spatializer::UpdateSourcePosition; register the spatializer in InitializeAudioCallback. Spatialization-enabled is a runtime-only flag (default on) — deliberately not a serialized AudioSoundGraphComponent field, to avoid a SceneSerializer change. - Scene: feed the active listener pose into Spatializer::UpdateListener (init + per-frame) and drive per-frame SoundGraph voice positions. Test: AudioSoundGraphSpatializerTest (unit, device-free ma_engine) pins the bare-node hosting and position -> distance/attenuation via the spatializer's CPU-side getters. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughAdds per-voice 3D spatialization to ChangesSoundGraph 3D Spatialization
Possibly related issues
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
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 |
|
Spatializer::ReleaseSource uninitialized the spatializer node and VBAP but never freed the ma_channel_converter that InitSource allocates, leaking ~72 bytes per source. The leak was latent — nothing called InitSource/ReleaseSource before #424 — and surfaced as a LeakSanitizer failure on the new AudioSoundGraphSpatializerTest (Linux/Clang ASan+LSan; Windows/MSVC doesn't run LSan, so it passed locally). Add ma_channel_converter_uninit; it no-ops safely on a never-initialized (zeroed) converter. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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 `@OloEngine/src/OloEngine/Audio/SoundGraph/SoundGraphSound.cpp`:
- Around line 532-551: In SoundGraphSound::SetLocation, SetVelocity, and
SetOrientation, reject invalid spatial vectors before mutating member state or
calling SyncSpatialPositionToSource(). Add finite-value checks with
std::isfinite for the glm::vec3 inputs, and in SetOrientation also guard against
degenerate forward/up data before assigning m_Position, m_Velocity,
m_Orientation, or m_Up. Use these methods as the fix point so only valid vectors
are cached and forwarded to the spatializer.
In `@OloEngine/src/OloEngine/Audio/SoundGraph/SoundGraphSource.cpp`:
- Around line 388-392: UpdateSpatialPosition currently forwards the transform
directly into the spatializer, so add a guard in
SoundGraphSource::UpdateSpatialPosition before calling
m_Spatializer->UpdateSourcePosition. Validate the transform’s direction/up
vectors for finiteness and reject degenerate values (zero-length or invalid
vectors) using the project’s floating-point finite checks before forwarding.
Keep the existing m_SpatializerRegistered and m_Spatializer checks, then only
call UpdateSourcePosition when the spatial transform is valid.
In `@OloEngine/src/OloEngine/Scene/Scene.cpp`:
- Around line 592-598: The listener/source basis vectors used in Scene::Update
and related orientation paths are normalized without checking for finite values
or zero length. In the code that builds Audio::Transform and calls
Spatializer::UpdateListener / SetOrientation, validate the inverted transform
basis vectors (and any derived forward/right/up vectors) with std::isfinite and
length checks, then fall back to canonical axes if the transform is singular or
non-finite before normalizing. Apply the same guard in the other affected
orientation sites so AudioEngine::GetSpatializer() never receives NaN/invalid
basis vectors.
In `@OloEngine/tests/AudioSoundGraphSpatializerTest.cpp`:
- Around line 202-205: The re-registration check in
AudioSoundGraphSpatializerTest should verify that a new source ID is actually
different from the previously released one, not just non-zero. Capture the
original m_Source.GetSpatializerSourceID() before release, then after calling
m_Source.RegisterSpatializer(&m_Spatializer, config) assert the new
GetSpatializerSourceID() is not equal to the saved value and still not zero.
This should be done in the test around m_Source.RegisterSpatializer and
GetSpatializerSourceID so the allocator cannot recycle the same ID without being
detected.
🪄 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: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 621bf027-e3cd-4846-9a9a-47cfc0e53aa7
📒 Files selected for processing (9)
OloEngine/src/OloEngine/Audio/DSP/Spatializer/Spatializer.cppOloEngine/src/OloEngine/Audio/DSP/Spatializer/Spatializer.hOloEngine/src/OloEngine/Audio/SoundGraph/SoundGraphSound.cppOloEngine/src/OloEngine/Audio/SoundGraph/SoundGraphSound.hOloEngine/src/OloEngine/Audio/SoundGraph/SoundGraphSource.cppOloEngine/src/OloEngine/Audio/SoundGraph/SoundGraphSource.hOloEngine/src/OloEngine/Scene/Scene.cppOloEngine/tests/AudioSoundGraphSpatializerTest.cppOloEngine/tests/CMakeLists.txt



What & why
Closes the gap in #424: a SoundGraph voice was not 3D-positioned at all.
SoundGraphSound::SetLocation/SetVelocity/SetOrientationonly stored the value — nothing reached a spatializer.Audio::DSP::Spatializeralready did VBAP panning + distance/cone attenuation, but itsInitSourcewas built around anma_engine_node(it readsresampler.channelsand drives the engine node's built-inspatializer.dopplerPitch) and was wired to nothing in production. ASoundGraphSource, by contrast, attaches a barema_nodestraight to the engine endpoint — the crux mismatch this PR resolves.Changes
InitSource(ma_node_base*)beside the existingInitSource(ma_engine_node*); both delegate to a sharedInitSourceInternal. The Doppler sink (ma_engine_node*) is now nullable and the doppler-pitch writes (audio callback +UpdatePositionalData) are null-guarded, so a bare node still gets full VBAP panning + distance/cone attenuation (computed inside the spatializer node) while Doppler — which needs the engine node's resampler — is skipped.ReleaseSourcere-routes via a newattachedNodefield. The engine-node path is unchanged.RegisterSpatializer(inserts between the SoundGraph node and the endpoint, allocates a process-unique sourceID),UpdateSpatialPosition,UnregisterSpatializer(also fromShutdown, beforema_node_uninit).Spatializer::UpdateSourcePosition; registers the spatializer inInitializeAudioCallback; removes the deferral comment.Spatializer::UpdateListener(init + per-frame) and drives per-frame SoundGraph voice positions inOnUpdateRuntime.Spatialization-enabled ownership (scope item 3)
Owned at runtime by
SoundGraphSound::m_SpatializationEnabled(default on), not a serializedAudioSoundGraphComponentfield — deliberately, to avoid aSceneSerializer.cppchange that would collide with the in-flight #451 codegen work. A serialized per-voice toggle (to allow 2D SoundGraph voices) is the natural follow-up once #451 lands.Tests
AudioSoundGraphSpatializerTest(classifiedunit, device-freema_engineviaconfig.noDevice): pins that the bare SoundGraph node hosts a spatializer node, and that position drivesGetCurrentDistance/GetCurrentDistanceAttenuationcorrectly (geometry-exact distance, monotonic attenuation, full gain at the listener, listener-move recompute, clean release). All 5 pass; the surrounding 155 audio/SoundGraph/VBAP tests and theAudioRuntimeTicksViaSceneFunctional test still pass.Refs #424
🤖 Generated with Claude Code
Summary by CodeRabbit