WIP - Attenuation Models#3
Conversation
Cleaned up logs
Will use this to add the new incremental changes
📝 WalkthroughWalkthroughAdds camera terrain tracing and accessor; introduces a sound attenuation interface and an RTS attenuation model; integrates attenuation-based spatial audio across the OpenAL sound stack and sources (model allocation, per-source evaluation, refactor), plus new sound config options and build entries. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Source as CSoundSource
participant System as CSound
participant Model as RtsAttenuationModel
participant Camera as CCamera
participant AL as OpenAL
Note over Source,System: Play/Update flow for a 3D sound using attenuation model
Source->>System: GetAttenuationModel()
System-->>Source: attenuationModel (ptr)
Source->>Camera: Query camera (pos, forward, GetTerrainDistance)
Camera-->>Source: cameraData
Source->>Model: Evaluate(SoundAttenuationInput{soundPos})
Model->>Camera: Read camera state (FOV/terrain/etc.)
Model-->>Source: SoundAttenuationOutput (volume/filter/frustum metrics)
alt snd_useAttenuationModel = true
Source->>AL: Apply gain & filter from attenuation output
else
Source->>AL: Apply legacy distance-based attenuation
end
Note over AL: Sound plays with updated gain/filter
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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: 12
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
rts/System/Sound/OpenAL/SoundSource.cpp (1)
25-26: Duplicate definitions of ROLLOFF_FACTOR and REFERENCE_DIST.These constants are defined here as
static constexprand also declared asstatic constexprinSoundSource.h(lines 37-38). This creates duplicate definitions. Since the header already has the public declarations, remove them from the .cpp file.-static constexpr float ROLLOFF_FACTOR = 5.0f; -static constexpr float REFERENCE_DIST = 200.0f;
🧹 Nitpick comments (17)
rts/Game/Camera.h (1)
205-205: Fix indentation inconsistency and remove extra blank line.The new additions use 4-space indentation while the rest of the file appears to use tabs. Additionally, line 315 is an unnecessary blank line.
🔎 Proposed fix
- float GetTerrainDistance() const { return terrainDistance; } + float GetTerrainDistance() const { return terrainDistance; }- void TraceToTerrain(); + void TraceToTerrain();- float terrainDistance; - + float terrainDistance;Also applies to: 257-257, 314-315
rts/Game/Camera.cpp (1)
714-731: Consider refactoring for consistency and clarity.A few optional improvements:
- The magic number
30000could be a named constant (e.g.,MAX_TERRAIN_TRACE_DISTANCE)- Indentation uses 4 spaces instead of tabs, inconsistent with the rest of the file
🔎 Suggested improvements
Define a constant at the top of the file:
constexpr float MAX_TERRAIN_TRACE_DISTANCE = 30000.0f;Then use it in the call:
terrainDistance = TraceRay::GuiTraceRay( GetPos(), GetForward(), - 30000, + MAX_TERRAIN_TRACE_DISTANCE, nullptr, hitUnit, hitFeature, true, true, true );Also fix indentation to use tabs consistently with the rest of the file.
rts/System/Sound/CMakeLists.txt (1)
11-11: Minor: Inconsistent indentation style.The added lines use spaces for indentation, while the surrounding code uses tabs. Consider aligning with the existing style for consistency.
- AttenuationModels/RtsAttenuationModel.cpp + AttenuationModels/RtsAttenuationModel.cppAlso applies to: 46-46
rts/System/Sound/OpenAL/SoundSource.cpp (2)
142-144: Unused variable:gainis retrieved but never used.The
alGetSourcefcall reads the gain value intogain, but the variable is never used afterward. Either remove this dead code or use the value for validation/logging.- float gain; - - alGetSourcef(id, AL_GAIN, &gain);
79-88: Consider placing helper functions in an anonymous namespace.
SmoothTowardsandCurveare free functions at file scope. Using an anonymous namespace limits their visibility to this translation unit and avoids potential ODR issues.+namespace { + float SmoothTowards(float current, float target, float speed, float dt) { const float t = 1.0f - std::exp(-speed * dt); return current + (target - current) * t; } float Curve(float t, float min, float max, float k) { return min + (max - min) * std::pow(t, k); } + +} // namespacerts/System/Sound/OpenAL/SoundSource.h (1)
152-152: Potential performance concern: config lookup on every call.
UseAttenuationModel()reads fromconfigHandlereach time it's called, including duringUpdate()which runs frequently. Consider caching this value and updating it only when the config changes.+ bool useAttenuationModelCached = false; + - bool UseAttenuationModel() { return configHandler->GetBool("snd_useAttenuationModel"); } + bool UseAttenuationModel() const { return useAttenuationModelCached; }Then update the cached value in
CSound::ConfigNotifyor during initialization.rts/System/Sound/OpenAL/Sound.h (2)
13-13: Prefer including the interface header rather than the concrete implementation.Including
RtsAttenuationModel.hcreates tight coupling. SinceGetAttenuationModel()returnsISoundAttenuationModel*, the header only needs the interface. The concrete type can be forward-declared or included only in the .cpp file.-#include "System/Sound/AttenuationModels/RtsAttenuationModel.h" +#include "System/Sound/ISoundAttenuationModel.h"
29-30: Minor: Redundantpublic:section.There are two consecutive
public:sections. Consider mergingDrawDebug()into the existing public section below.rts/System/Sound/AttenuationModels/RtsAttenuationModel.h (2)
14-18: Performance concern: Config lookups on everyEvaluate()call.These getter methods read from
configHandlereach timeEvaluate()is called. SinceEvaluate()runs for every sound source during updates, this could become a performance bottleneck. Consider caching these values and updating them viaConfigNotify.+private: + // Cached config values - updated via ConfigNotify + float forwardAttenuationRange = 5000.0f; + float backwardAttenuationRange = 1000.0f; + float outerAttenuationRange = 2000.0f; + float minVolumeAttenuation = 0.0f; + float minFilterAttenuation = 0.0f; + +public: + void UpdateFromConfig(); + private: - float GetForwardAttenuationRange() const { return configHandler->GetFloat("snd_forwardAttenuationRange"); } - float GetBackwardAttenuationRange() const { return configHandler->GetFloat("snd_backwardAttenuationRange"); } - float GetOuterAttenuationRange() const { return configHandler->GetFloat("snd_outerAttenuationRange"); } - float GetMinVolumeAttenuation() const { return configHandler->GetFloat("snd_minVolumeAttenuation"); } - float GetMinFilterAttenuation() const { return configHandler->GetFloat("snd_minFilterAttenuation"); } + float GetForwardAttenuationRange() const { return forwardAttenuationRange; } + float GetBackwardAttenuationRange() const { return backwardAttenuationRange; } + float GetOuterAttenuationRange() const { return outerAttenuationRange; } + float GetMinVolumeAttenuation() const { return minVolumeAttenuation; } + float GetMinFilterAttenuation() const { return minFilterAttenuation; }
21-21: Move descriptive comment to class documentation.The trailing comment describing the RTS model would be more useful as a Doxygen-style comment above the class declaration.
+/** + * RTS Attenuation Model + * + * Designed for RTS games with viewport-based attenuation, forward/backward + * distance handling, off-screen attenuation, and zoom-aware center attenuation. + * Similar to sound systems in games like "Planetary Annihilation" or "Supreme Commander 2". + */ class RtsAttenuationModel : public ISoundAttenuationModel { ... }; - -// "RTS Model: Designed for RTS games with viewport-based attenuation..."rts/System/Sound/AttenuationModels/RtsAttenuationModel.cpp (3)
7-7: Unused include:TraceRay.h.The
TraceRay.hheader is included but no symbols from it appear to be used in this file.-#include "Game/TraceRay.h"
63-64: Simplifypow(forwardValue, 1).
std::pow(forwardValue, 1)is equivalent toforwardValue. Simplify for clarity.- out.volumeFactor = std::pow(forwardValue, 6) * outerValue; - out.filterFactor = std::pow(forwardValue, 1) * outerValue; + out.volumeFactor = std::pow(forwardValue, 6) * outerValue; + out.filterFactor = forwardValue * outerValue;
56-58: Complex expression is difficult to read and verify.This nested ternary with clamp and lerp is hard to follow. Consider breaking it into named intermediate values for clarity.
- float forwardValue = std::clamp(out.forwardDistance >= 0 ? - std::lerp(GetMinVolumeAttenuation(), 1.0f, 1.0f - out.forwardDistance / GetForwardAttenuationRange()) : - std::clamp(1.0f - (-out.forwardDistance) / GetBackwardAttenuationRange(), 0.0f, 1.0f), GetMinVolumeAttenuation(), 1.0f); + float forwardValue; + if (out.forwardDistance >= 0) { + // Sound is in front of camera + float t = 1.0f - out.forwardDistance / GetForwardAttenuationRange(); + forwardValue = std::lerp(GetMinVolumeAttenuation(), 1.0f, t); + } else { + // Sound is behind camera + float t = 1.0f - (-out.forwardDistance) / GetBackwardAttenuationRange(); + forwardValue = std::clamp(t, 0.0f, 1.0f); + } + forwardValue = std::clamp(forwardValue, GetMinVolumeAttenuation(), 1.0f);rts/System/Sound/ISoundAttenuationModel.h (3)
6-6: Address or remove the TODO-style comment.This comment suggests incomplete design. Either implement the additional fields now or create a tracked issue and remove the comment to avoid leaving unactionable TODOs in the codebase.
Do you want me to open an issue to track this potential enhancement?
9-9: Update the comment to match the actual struct fields.The comment mentions "totalFactor" but the struct contains
volumeFactorandfilterFactorinstead. Update the comment to accurately describe what the struct contains.
22-29: Consider adding documentation for the public API.The interface and its
Evaluatemethod lack documentation comments. Adding brief doc comments would help future developers understand the purpose of the attenuation model and the contract of theEvaluatemethod.📝 Example documentation
+/// Interface for sound attenuation models that calculate spatial audio parameters +/// based on listener position and orientation. class ISoundAttenuationModel { public: virtual ~ISoundAttenuationModel() = default; + /// Evaluates attenuation parameters for a sound source. + /// @param in Input containing the sound source position + /// @return Output containing attenuation factors and spatial data virtual SoundAttenuationOutput Evaluate( const SoundAttenuationInput& in ) const = 0; };rts/System/Sound/ISound.cpp (1)
44-46: Add validation constraints and specify units.These distance/range configs lack
minimumValue()constraints, unlike other configs in this file (see lines 30-38). Consider adding.minimumValue(0.0f)or appropriate bounds to prevent invalid negative distances.Additionally, specify the units in the descriptions (e.g., "Distance in game units" or "Distance in elmos").
🔎 Proposed constraints
-CONFIG(float, snd_forwardAttenuationRange).defaultValue(8000).description("Distance in front of the camera over which sound volume and clarity gradually fade."); -CONFIG(float, snd_backwardAttenuationRange).defaultValue(300).description("Distance behind the camera over which sounds are attenuated."); -CONFIG(float, snd_outerAttenuationRange).defaultValue(500).description("How far sounds can be outside the camera view before being strongly attenuated."); +CONFIG(float, snd_forwardAttenuationRange).defaultValue(8000.0f).minimumValue(0.0f).description("Distance (in game units) in front of the camera over which sound volume and clarity gradually fade."); +CONFIG(float, snd_backwardAttenuationRange).defaultValue(300.0f).minimumValue(0.0f).description("Distance (in game units) behind the camera over which sounds are attenuated."); +CONFIG(float, snd_outerAttenuationRange).defaultValue(500.0f).minimumValue(0.0f).description("How far sounds can be (in game units) outside the camera view before being strongly attenuated.");
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
rts/Game/Camera.cpprts/Game/Camera.hrts/System/Sound/AttenuationModels/RtsAttenuationModel.cpprts/System/Sound/AttenuationModels/RtsAttenuationModel.hrts/System/Sound/CMakeLists.txtrts/System/Sound/ISound.cpprts/System/Sound/ISound.hrts/System/Sound/ISoundAttenuationModel.hrts/System/Sound/OpenAL/Sound.cpprts/System/Sound/OpenAL/Sound.hrts/System/Sound/OpenAL/SoundSource.cpprts/System/Sound/OpenAL/SoundSource.h
🧰 Additional context used
🧬 Code graph analysis (7)
rts/System/Sound/AttenuationModels/RtsAttenuationModel.h (2)
rts/System/Sound/ISoundAttenuationModel.h (1)
ISoundAttenuationModel(22-29)rts/System/Sound/AttenuationModels/RtsAttenuationModel.cpp (2)
Evaluate(11-68)Evaluate(11-11)
rts/System/Sound/ISoundAttenuationModel.h (1)
rts/System/Sound/AttenuationModels/RtsAttenuationModel.cpp (2)
Evaluate(11-68)Evaluate(11-11)
rts/Game/Camera.cpp (1)
rts/Game/TraceRay.cpp (2)
GuiTraceRay(371-524)GuiTraceRay(371-381)
rts/System/Sound/ISound.h (2)
rts/System/Sound/ISoundAttenuationModel.h (1)
ISoundAttenuationModel(22-29)rts/System/Sound/OpenAL/Sound.h (1)
GetAttenuationModel(80-80)
rts/System/Sound/OpenAL/SoundSource.cpp (1)
rts/System/Sound/OpenAL/SoundSource.h (2)
UseAttenuationModel(152-152)GetCurrentPriority(48-62)
rts/System/Sound/AttenuationModels/RtsAttenuationModel.cpp (1)
rts/System/FastMath.h (1)
sqrt(226-226)
rts/System/Sound/OpenAL/SoundSource.h (1)
rts/System/Sound/OpenAL/SoundSource.cpp (6)
Update(152-197)Update(152-152)ApplyAttenuationModel(90-150)ApplyAttenuationModel(90-90)swap(34-49)swap(34-34)
🪛 Cppcheck (2.19.0)
rts/System/Sound/OpenAL/SoundSource.cpp
[warning] 317-317: The address of variable 'x' might be accessed at non-zero index.
(objectIndex)
[warning] 329-329: The address of variable 'x' might be accessed at non-zero index.
(objectIndex)
rts/System/Sound/AttenuationModels/RtsAttenuationModel.cpp
[error] 31-31: #error unknown CPU-architecture
(preprocessorErrorDirective)
🔇 Additional comments (6)
rts/Game/Camera.cpp (2)
7-7: LGTM!The include for TraceRay.h is necessary for the GuiTraceRay function call and is properly placed.
160-161: LGTM!The placement of TraceToTerrain() in the update flow is appropriate, ensuring terrain distance is computed after frustum updates and before matrices are loaded.
rts/System/Sound/OpenAL/SoundSource.cpp (1)
314-318: Questionable gain threshold warning.The warning triggers when
gain >= 0.9, but gains in the range [0.9, 1.0] are often legitimate for nearby or important sounds. Consider whether this threshold is too aggressive, or if this is meant to catch gains significantly over 1.0 instead.If the intent is to catch anomalous over-unity gains:
- if (gain >= 0.9) + if (gain > 1.0f) LOG_L(L_WARNING, "Gain was suspiciously high: %s, at %f", name.c_str(), gain);rts/System/Sound/ISound.h (1)
83-85: LGTM with minor style note.The new virtual method with a default
nullptrimplementation is appropriate for the interface pattern. Consider moving thispublic:section before theprivate:section for conventional ordering, though this is purely stylistic.rts/System/Sound/OpenAL/Sound.cpp (1)
1128-1128: Verify the debug sphere scale.The scale
CSoundSource::REFERENCE_DIST * ELMOS_TO_METERSresults in200 * 0.03125 = 6.25units. Verify this produces meaningful visualization at typical camera distances. The comment mentions "adjusted scale for better visibility" but the value seems small for an RTS view.rts/System/Sound/AttenuationModels/RtsAttenuationModel.cpp (1)
20-20: > Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
rts/System/Sound/OpenAL/SoundSource.h (1)
121-121: Remove unused member variablescameraZoomandlastUpdate.Based on past review analysis, these member variables are declared but never used in the implementation. They should be removed to avoid confusion.
🔎 Suggested removal
- float cameraZoom; - float3 currentPosition = float3(0.0f, 0.0f, 0.0f); ... spring_time loopStop {1e9}; - spring_time lastUpdate = spring_gettime(); bool in3D = false;Also applies to: 136-136
rts/System/Sound/OpenAL/Sound.cpp (1)
1118-1135: Thread safety concern:DrawDebug()accessessoundSourceswithout locking.This method iterates over
soundSourceswhich can be modified by the sound thread. Other methods likeUpdate()andGetNextBestSource()acquiresoundMutexbefore accessing this collection. Consider adding synchronization to prevent data races.🔎 Suggested fix
void CSound::DrawDebug() const { + std::lock_guard<spring::recursive_mutex> lck(soundMutex); + // Only draw 3D sound sources that are currently playing for (const CSoundSource& source: soundSources) {Note: This requires either making
soundMutexmutable or removing theconstqualifier fromDrawDebug().
🧹 Nitpick comments (10)
rts/Game/Camera.h (3)
205-205: LGTM! Accessor follows established pattern.The accessor is consistent with other distance getters like
GetNearPlaneDist()andGetFarPlaneDist(). Optionally, consider adding a brief documentation comment explaining the purpose (distance from camera to terrain intersection point).
257-257: Private method is appropriate for internal camera updates.The method is correctly declared as private, as it's called internally during the camera update cycle. Optionally, consider adding a documentation comment describing that this method traces from the camera to terrain and updates
terrainDistance.
314-315: Document the default terrain distance value.The default value
1000lacks a comment explaining its purpose. Consider adding inline documentation clarifying:
- This is the fallback distance before the first terrain trace completes
- The units are game units (consistent with attenuation range parameters like
snd_forwardAttenuationRange= 8000)- Why 1000 was chosen as the default (roughly 12.5% of the forward attenuation range)
This matters for the attenuation system in
RtsAttenuationModel.cpp, which usesGetTerrainDistance()to compute zoom-based volume attenuation.rts/System/Sound/OpenAL/SoundSource.h (2)
145-145: Consider movingUseAttenuationModel()to the .cpp file.Including
ConfigHandler.hin a header just for this inline helper adds compile-time dependencies to all files that includeSoundSource.h. Since this is a private helper, moving it to the implementation file would be cleaner.Additionally, calling
configHandler->GetBool()on every frame for every source may have minor overhead. Consider caching the value or querying it once per frame at a higher level.🔎 Suggested refactor
In
SoundSource.h, declare the helper without the body:- bool UseAttenuationModel() { return configHandler->GetBool("snd_useAttenuationModel"); } + bool UseAttenuationModel() const;In
SoundSource.cpp, add the implementation:bool CSoundSource::UseAttenuationModel() const { return configHandler->GetBool("snd_useAttenuationModel"); }Then remove the
#include "System/Config/ConfigHandler.h"from the header.
37-38: Inconsistent indentation style.The new code uses 4-space indentation while the existing code uses tabs. This creates visual inconsistency. Consider aligning with the existing tab-based indentation style of the file.
Also applies to: 43-44, 60-60, 65-70
rts/System/Sound/OpenAL/Sound.cpp (1)
83-85: Consider usingstd::unique_ptrfor automatic memory management.While the current implementation correctly manages the
attenuationModellifecycle, usingstd::unique_ptr<ISoundAttenuationModel>would provide automatic RAII cleanup and eliminate the need for explicitdeletecalls.rts/System/Sound/ISoundAttenuationModel.h (1)
10-19: Consider initializingSoundAttenuationOutputfields to prevent undefined behavior.The struct fields are default-initialized (uninitialized for primitive types). While
RtsAttenuationModel::Evaluate()currently sets all fields, future implementations or modifications could accidentally leave fields uninitialized, causing undefined behavior.🔎 Suggested initialization
struct SoundAttenuationOutput { - float forwardDistance; - float outerDistance; - float frustumHeight; - float frustumWidth; - float volumeFactor; - float filterFactor; - float tiltFactor; - float zoomFactor; + float forwardDistance = 0.0f; + float outerDistance = 0.0f; + float frustumHeight = 0.0f; + float frustumWidth = 0.0f; + float volumeFactor = 1.0f; // Safe default: full volume + float filterFactor = 1.0f; // Safe default: no filtering + float tiltFactor = 0.0f; + float zoomFactor = 0.0f; };rts/System/Sound/OpenAL/SoundSource.cpp (3)
75-84: Consider making helper functionsstaticor placing in anonymous namespace.
SmoothTowardsandCurveare free functions that don't access any class state. They should bestaticor placed in an anonymous namespace to limit their visibility to this translation unit.🔎 Suggested refactor
+namespace { + float SmoothTowards(float current, float target, float speed, float dt) { const float t = 1.0f - std::exp(-speed * dt); return current + (target - current) * t; } float Curve(float t, float min, float max, float k) { return min + (max - min) * std::pow(t, k); } +} // anonymous namespace
135-135: Add null check forcurrentChannelbefore dereferencing.While
currentChannelshould be set whencurrentSoundItem.id != 0, adding a defensive null check would prevent potential crashes if the state becomes inconsistent.🔎 Suggested fix
- alSourcef(id, AL_GAIN, std::clamp(attenuationOutput.volumeFactor * currentChannel->GetVolume(), 0.0f, 1.0f)); + const float channelVolume = (currentChannel != nullptr) ? currentChannel->GetVolume() : 1.0f; + alSourcef(id, AL_GAIN, std::clamp(attenuationOutput.volumeFactor * channelVolume, 0.0f, 1.0f));
81-84: Remove unusedCurvefunction.The
Curvefunction is defined but never used in this file. Remove it to avoid dead code.🔎 Suggested removal
-float Curve(float t, float min, float max, float k) -{ - return min + (max - min) * std::pow(t, k); -} -
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
rts/Game/Camera.hrts/System/Sound/ISound.cpprts/System/Sound/ISoundAttenuationModel.hrts/System/Sound/OpenAL/Sound.cpprts/System/Sound/OpenAL/Sound.hrts/System/Sound/OpenAL/SoundSource.cpprts/System/Sound/OpenAL/SoundSource.h
🚧 Files skipped from review as they are similar to previous changes (2)
- rts/System/Sound/ISound.cpp
- rts/System/Sound/OpenAL/Sound.h
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: Majavaa
Repo: Majavaa/RecoilEngine PR: 3
File: rts/System/Sound/ISound.cpp:43-43
Timestamp: 2025-12-28T10:25:36.188Z
Learning: In rts/System/Sound/ISound.cpp, the snd_useAttenuationModel config intentionally defaults to false while the description recommends "ON". This is by design to recommend the feature without forcing users to opt in.
📚 Learning: 2025-12-28T10:25:36.188Z
Learnt from: Majavaa
Repo: Majavaa/RecoilEngine PR: 3
File: rts/System/Sound/ISound.cpp:43-43
Timestamp: 2025-12-28T10:25:36.188Z
Learning: In rts/System/Sound/ISound.cpp, the snd_useAttenuationModel config intentionally defaults to false while the description recommends "ON". This is by design to recommend the feature without forcing users to opt in.
Applied to files:
rts/System/Sound/ISoundAttenuationModel.hrts/System/Sound/OpenAL/Sound.cpprts/System/Sound/OpenAL/SoundSource.hrts/System/Sound/OpenAL/SoundSource.cpp
🧬 Code graph analysis (4)
rts/System/Sound/ISoundAttenuationModel.h (1)
rts/System/Sound/AttenuationModels/RtsAttenuationModel.cpp (2)
Evaluate(11-68)Evaluate(11-11)
rts/Game/Camera.h (1)
rts/Game/Camera.cpp (2)
TraceToTerrain(714-731)TraceToTerrain(714-714)
rts/System/Sound/OpenAL/SoundSource.h (2)
rts/System/Sound/OpenAL/SoundSource.cpp (6)
Update(147-192)Update(147-147)ApplyAttenuationModel(86-145)ApplyAttenuationModel(86-86)swap(31-45)swap(31-31)rts/Rendering/Fonts/CFontTexture.cpp (1)
configHandler(271-271)
rts/System/Sound/OpenAL/SoundSource.cpp (1)
rts/System/Sound/OpenAL/SoundSource.h (3)
loopStop(135-146)UseAttenuationModel(145-145)GetCurrentPriority(48-62)
🪛 Cppcheck (2.19.0)
rts/System/Sound/OpenAL/SoundSource.cpp
[warning] 317-317: The address of variable 'x' might be accessed at non-zero index.
(objectIndex)
[warning] 329-329: The address of variable 'x' might be accessed at non-zero index.
(objectIndex)
🔇 Additional comments (4)
rts/System/Sound/ISoundAttenuationModel.h (1)
1-29: Clean interface design.The attenuation model abstraction is well-designed with clear input/output structures and a simple pure virtual interface that enables different attenuation implementations.
rts/System/Sound/OpenAL/SoundSource.cpp (3)
329-329: Static analysis false positive:alDeleteSources(1, &id)is correct.The static analysis warning about "address of variable might be accessed at non-zero index" is a false positive. Passing the address of a single
ALuintwith count=1 toalDeleteSourcesis the standard OpenAL pattern, matching thealGenSources(1, &id)call in the constructor.
252-283: Good separation of attenuation model path.The code correctly disables OpenAL's built-in distance attenuation (setting
AL_REFERENCE_DISTANCEandAL_ROLLOFF_FACTORto 0) when using the custom attenuation model, then applies attenuation throughAL_GAINviaApplyAttenuationModel. This is the right approach.
483-500: LGTM - UpdateVolume correctly handles both attenuation paths.The method properly checks for the attenuation model setting and routes to the appropriate volume calculation logic.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
rts/System/Sound/OpenAL/SoundSource.cpp (2)
124-133: Address TODO and FIXME comments.Multiple unresolved concerns are documented:
- Line 124-126: Random volume scaling needs redesign for normalized range
- Line 128: 2D events incorrectly affected by filters
- Line 130-131: Volume value conversion strategy unclear
- Line 133: Commented-out alternative gain calculation
These indicate uncertainty in the implementation approach.
Would you like me to open an issue to track these concerns, or help design a solution for the volume normalization and 2D/3D separation?
309-311: Remove unused gain retrieval.The gain value is retrieved but never used. This appears to be leftover debug code.
🔎 Proposed cleanup
alSourcePlay(id); - - float gain; - alGetSourcef(id, AL_GAIN, &gain); -
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
rts/System/Sound/OpenAL/SoundSource.cpp
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: Majavaa
Repo: Majavaa/RecoilEngine PR: 3
File: rts/System/Sound/ISound.cpp:43-43
Timestamp: 2025-12-28T10:25:36.188Z
Learning: In rts/System/Sound/ISound.cpp, the snd_useAttenuationModel config intentionally defaults to false while the description recommends "ON". This is by design to recommend the feature without forcing users to opt in.
📚 Learning: 2025-12-28T10:25:36.188Z
Learnt from: Majavaa
Repo: Majavaa/RecoilEngine PR: 3
File: rts/System/Sound/ISound.cpp:43-43
Timestamp: 2025-12-28T10:25:36.188Z
Learning: In rts/System/Sound/ISound.cpp, the snd_useAttenuationModel config intentionally defaults to false while the description recommends "ON". This is by design to recommend the feature without forcing users to opt in.
Applied to files:
rts/System/Sound/OpenAL/SoundSource.cpp
🧬 Code graph analysis (1)
rts/System/Sound/OpenAL/SoundSource.cpp (1)
rts/System/Sound/OpenAL/SoundSource.h (2)
UseAttenuationModel(145-145)GetCurrentPriority(48-62)
🪛 Cppcheck (2.19.0)
rts/System/Sound/OpenAL/SoundSource.cpp
[warning] 317-317: The address of variable 'x' might be accessed at non-zero index.
(objectIndex)
[warning] 329-329: The address of variable 'x' might be accessed at non-zero index.
(objectIndex)
🔇 Additional comments (5)
rts/System/Sound/OpenAL/SoundSource.cpp (5)
75-84: LGTM!The utility functions are well-implemented.
SmoothTowardsuses a correct exponential smoothing formula, andCurveprovides a clean power-curve mapping for attenuation calculations.
147-192: LGTM!The
Update()function cleanly integrates the new attenuation model path while preserving backward compatibility with the legacy audio system. The async playback handling and conditional logic are well-structured.
252-283: LGTM!The attenuation model integration in the 3D audio path is well-designed. The code correctly:
- Initializes filters on-demand
- Disables physics-based attenuation when using the custom model
- Preserves the legacy EFX path for backward compatibility
333-398: LGTM!The new public methods are well-implemented with appropriate state checks and cleanup logic. The
IsPlaying()optimization note about avoiding L2 cache misses is a good performance consideration.
483-500: LGTM!The
UpdateVolume()function properly integrates the attenuation model path while maintaining backward compatibility with the legacy volume calculation.
| if (attenuationFilter != 0) | ||
| alDeleteFilters(id, &attenuationFilter); |
There was a problem hiding this comment.
Critical: Wrong parameter in alDeleteFilters call.
Line 326 passes id (the source ID) as the first parameter to alDeleteFilters, but OpenAL expects the filter count. This causes undefined behavior.
🔎 Proposed fix
if (attenuationFilter != 0)
- alDeleteFilters(id, &attenuationFilter);
+ alDeleteFilters(1, &attenuationFilter);📝 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.
| if (attenuationFilter != 0) | |
| alDeleteFilters(id, &attenuationFilter); | |
| if (attenuationFilter != 0) | |
| alDeleteFilters(1, &attenuationFilter); |
🤖 Prompt for AI Agents
In rts/System/Sound/OpenAL/SoundSource.cpp around lines 325-326, the call to
alDeleteFilters incorrectly passes the source id as the first parameter; replace
that with the filter count (1) and pass the address of attenuationFilter: call
alDeleteFilters(1, &attenuationFilter); after deletion set attenuationFilter = 0
to avoid dangling state and keep the existing check attenuationFilter != 0.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
rts/System/Sound/OpenAL/SoundSource.cpp (4)
81-84: Dead code:Curvefunction is defined but never used.This function is not called anywhere in the file. If it's intended for future use, consider adding a comment or removing it until needed.
137-139: Unused variable:gainis fetched but never used.This appears to be leftover debug code. Either remove it or use it for logging/validation if intended.
- float gain; - - alGetSourcef(id, AL_GAIN, &gain); -
309-310: Unused variable:gainis fetched but never used.Same as in
ApplyAttenuationModel- this appears to be leftover debug code. Consider removing it.- float gain; - alGetSourcef(id, AL_GAIN, &gain); -
325-326: Consider resettingattenuationFilterto 0 after deletion.For defensive coding, reset the handle after deletion to prevent potential double-delete issues or stale handle usage.
if (attenuationFilter != 0) + { alDeleteFilters(1, &attenuationFilter); + attenuationFilter = 0; + }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
rts/System/Sound/OpenAL/SoundSource.cpp
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: Majavaa
Repo: Majavaa/RecoilEngine PR: 3
File: rts/System/Sound/ISound.cpp:43-43
Timestamp: 2025-12-28T10:25:36.188Z
Learning: In rts/System/Sound/ISound.cpp, the snd_useAttenuationModel config intentionally defaults to false while the description recommends "ON". This is by design to recommend the feature without forcing users to opt in.
📚 Learning: 2025-12-28T10:25:36.188Z
Learnt from: Majavaa
Repo: Majavaa/RecoilEngine PR: 3
File: rts/System/Sound/ISound.cpp:43-43
Timestamp: 2025-12-28T10:25:36.188Z
Learning: In rts/System/Sound/ISound.cpp, the snd_useAttenuationModel config intentionally defaults to false while the description recommends "ON". This is by design to recommend the feature without forcing users to opt in.
Applied to files:
rts/System/Sound/OpenAL/SoundSource.cpp
🧬 Code graph analysis (1)
rts/System/Sound/OpenAL/SoundSource.cpp (2)
rts/System/Sound/OpenAL/SoundSource.h (1)
GetCurrentPriority(48-62)rts/System/Sound/OpenAL/Sound.cpp (2)
Update(802-818)Update(802-802)
🪛 Cppcheck (2.19.0)
rts/System/Sound/OpenAL/SoundSource.cpp
[warning] 317-317: The address of variable 'x' might be accessed at non-zero index.
(objectIndex)
[warning] 329-329: The address of variable 'x' might be accessed at non-zero index.
(objectIndex)
🔇 Additional comments (1)
rts/System/Sound/OpenAL/SoundSource.cpp (1)
86-145: Attenuation model integration looks well-structured.The branching between the new attenuation model path and the legacy EFX path is clean. The smoothing logic for gradual transitions and proper null checks before accessing the attenuation model are good practices.
Note the TODO/FIXME comments (lines 124-131) for tracking remaining work items related to volume normalization and 2D event filtering.
Also applies to: 147-192, 194-316
1 participant
Description
Summary by CodeRabbit
New Features
Improvements
✏️ Tip: You can customize this high-level summary in your review settings.