Draft
Conversation
feat: sound Qt
📝 WalkthroughWalkthroughThis pull request introduces a comprehensive sound management system overhaul, adding granular audio channel controls, item ambience tracking, protocol-based sound playback, a dedicated debug UI, and UI sound integration. New sound manager implementations handle debug snapshots, item-based ambient sounds, and protocol-driven audio routing alongside expanded sound channels and per-widget sound capabilities. Changes
Sequence Diagram(s)sequenceDiagram
participant Game
participant SoundManager
participant ProtocolParser
participant SoundChannel
participant AudioDevice
participant Lua/UI
Note over Game,Lua/UI: Protocol-Based Sound Playback Flow
Game->>ProtocolParser: onMagicEffect(MAGIC_EFFECTS_CREATE_SOUND_MAIN_EFFECT)
ProtocolParser->>SoundManager: playProtocolSoundMain(soundSource, soundEffectId, pos)
SoundManager->>SoundManager: shouldPlayProtocolSound(checks: enabled, volume, cooldown)
alt Playback Eligible
SoundManager->>SoundManager: chooseProtocolAudioFileId(soundEffectId)
SoundManager->>SoundManager: playProtocolAudioFileId(audioFileId, pos, channelId)
SoundManager->>SoundManager: buildProtocolSoundPath(fileName)
SoundManager->>SoundChannel: playChannelSound(filename, gain, pitch)
SoundChannel->>AudioDevice: Play audio with spatial positioning
SoundManager->>SoundManager: recordProtocolDebugEvent(if debug enabled)
SoundManager->>Lua/UI: Update debug snapshot on poll
else Ineligible
SoundManager->>SoundManager: Skip playback (log reason if debug)
end
sequenceDiagram
participant Game
participant Map
participant SoundManager
participant ItemAmbienceTracker
participant SoundChannel
participant AudioDevice
Note over Game,AudioDevice: Item Ambience Lifecycle
Map->>SoundManager: onItemTileChanged(position)
SoundManager->>ItemAmbienceTracker: Mark tile dirty
Game->>Map: setCentralPosition(newPos)
Map->>SoundManager: onListenerPositionChanged(newPos, oldPos)
SoundManager->>ItemAmbienceTracker: Rescan tiles if distance/z changed
SoundManager->>SoundManager: poll() - updateItemAmbienceSources()
SoundManager->>ItemAmbienceTracker: Rebuild/update tracked tiles if dirty
loop Per Active Effect
ItemAmbienceTracker->>SoundManager: Get current itemCount from tiles
SoundManager->>SoundManager: resolveItemAmbientAudioFileId(itemCount)
SoundManager->>SoundManager: Compute gain from nearest distance
SoundManager->>SoundChannel: playChannelSound(looping, gain, pan)
SoundChannel->>AudioDevice: Stream ambient audio with spatial audio
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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: 11
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/framework/sound/combinedsoundsource.cpp (1)
62-108:⚠️ Potential issue | 🟡 MinorFix state persistence in
setFading()for consistency.The changes correctly persist values to member variables before propagating to child sources. However,
setFading()does not persist to the inherited member variables (m_fadeState,m_fadeTime,m_fadeStartTime,m_fadeGain), unlike other setters in this class. To maintain consistency with the established pattern (setLooping,setGain,setPitch, etc.),setFading()should set these inherited members in addition to propagating to child sources.(Note:
setReferenceDistance()has no corresponding member variable to persist, so it correctly only propagates to children.)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/framework/sound/combinedsoundsource.cpp` around lines 62 - 108, CombinedSoundSource::setFading currently propagates fading to child sources but doesn't persist the values to the inherited member variables; update setFading to assign the incoming values to m_fadeState, m_fadeTime, m_fadeStartTime, and m_fadeGain before iterating m_sources so the object's fade state is consistent with other setters (e.g., setLooping, setGain) and then continue propagating to each child source.src/framework/sound/soundmanager.h (1)
45-50:⚠️ Potential issue | 🟠 MajorDefault-initialize
maxSoundDistance.The new field is read as a sentinel in the item-ambience path, so leaving it uninitialized makes that branch undefined if any parse path or default construction misses the assignment.
🧯 Proposed fix
struct ClientItemAmbient { uint32_t id; std::vector<uint32_t> clientIds; - uint32_t maxSoundDistance; + uint32_t maxSoundDistance{ 0 };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/framework/sound/soundmanager.h` around lines 45 - 50, The new uint32_t field maxSoundDistance in struct ClientItemAmbient is left uninitialized; make it default-initialized to a safe sentinel (e.g., 0 or other agreed sentinel) in the member declaration so default-constructed or partially-parsed ClientItemAmbient instances have a defined value; also update any constructors or aggregate initializations that might rely on an implicit value to ensure they don't overwrite the default unintentionally (refer to ClientItemAmbient and maxSoundDistance).
🧹 Nitpick comments (5)
modules/corelib/ui/uiminiwindow.lua (1)
7-7: Verify sound ID selection.Sound ID
2785is used here for click sounds, which is the same ID used fordisplay-soundin10-windows.otui. Meanwhile, the baseButtonstyle uses2774for click sounds.Is using the same sound for miniwindow clicks and window display intentional? Different sounds typically help users distinguish between clicking/interacting vs. windows opening.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@modules/corelib/ui/uiminiwindow.lua` at line 7, The miniwindow currently registers the click sound with miniwindow:addSound(ESoundUI.SoundTypeClick, 2785), which duplicates the display-sound ID used elsewhere and differs from the base Button click ID 2774; decide whether miniwindow clicks should use the distinct Button click sound (2774) or a unique new ID, then update the call to miniwindow:addSound(ESoundUI.SoundTypeClick, <chosen-id>) accordingly and ensure any corresponding OTUI/10-windows.otui entries are adjusted to keep click vs display sounds distinct (reference miniwindow:addSound and ESoundUI.SoundTypeClick).modules/corelib/ui/uibutton.lua (1)
7-7: Prefer style-driven default for click sound instead of hardcoding it here.This duplicates theme/style responsibility and makes future sound-theme overrides harder. Consider keeping the default in OTUI only (or via a shared constant).
♻️ Suggested adjustment
function UIButton.create() local button = UIButton.internalCreate() button:setFocusable(false) - button:setClickSound(2774) button.cursorPushed = false return button end🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@modules/corelib/ui/uibutton.lua` at line 7, Remove the hardcoded sound id set by button:setClickSound(2774) and instead apply the theme/style-driven default: read the click sound from the button's style/theme (or from a shared constant like DEFAULT_CLICK_SOUND exposed by the OTUI theme module) and call button:setClickSound(...) with that value; update any initialization around the UI button creation in uibutton.lua to fall back to the shared/default value when the style does not specify one so the click sound can be overridden by theme changes.src/framework/ui/uiwidget.h (1)
449-451: Consider usingUISoundTypeenum for type safety.The
addSoundandremoveSoundmethods useintfor the sound type parameter. UsingUISoundTypewould provide compile-time type safety and better self-documentation.♻️ Proposed refactor for type safety
- void setClickSound(int soundId); - void addSound(int soundType, int soundId); - void removeSound(int soundType); + void setClickSound(int soundId); + void addSound(UISoundType soundType, int soundId); + void removeSound(UISoundType soundType);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/framework/ui/uiwidget.h` around lines 449 - 451, Change the sound-type parameters from int to the UISoundType enum to enforce type safety: update the declarations of addSound(int soundType, int soundId) and removeSound(int soundType) to addSound(UISoundType soundType, int soundId) and removeSound(UISoundType soundType), and propagate the type change through corresponding implementations and all call sites (including any overloads or tests) to use UISoundType instead of raw ints; ensure any switch/case or map keys that previously used ints are updated to accept UISoundType and include necessary header or forward declaration for UISoundType.src/client/luafunctions.cpp (1)
1178-1181: Consider relocatingg_soundsbinding to a more logical position.The
g_soundssingleton binding is placed betweenUIMapclass bindings andUIMinimapregistration. For better code organization, consider moving this binding to be with other singleton registrations (near the top of the function whereg_things,g_map, etc. are registered), or at least add a comment explaining why it's placed here.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/client/luafunctions.cpp` around lines 1178 - 1181, The g_sounds singleton binding (g_lua.bindSingletonFunction("g_sounds", "getDebugSnapshot", &SoundManager::getDebugSnapshot, &g_sounds)) is placed amid UIMap/UIMinimap class bindings and should be moved or documented: relocate this bind call to the block where other singletons (e.g., g_things, g_map) are registered near the top of the function to keep singleton registrations together, or if there is a specific reason for its current placement, add a clear comment above the binding explaining why it must remain here; update references to SoundManager::getDebugSnapshot and g_sounds accordingly.modules/client_options/data_options.lua (1)
810-834: Consider simplifying theauxstate tracking logic.The
auxfield is used to track whether the enabled/disabled state has changed, but the double-negation and comparison logic (shouldBeDisabled ~= (not options.soundMusic.aux)) is hard to follow.Consider renaming
auxto something descriptive likewasEnabledorpreviouslyDisabled, and simplifying the condition:local wasDisabled = not options.soundMusic.aux if shouldBeDisabled ~= wasDisabled then ... end🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@modules/client_options/data_options.lua` around lines 810 - 834, The aux flag in the soundMusic table (inside the action function) is confusing; rename options.soundMusic.aux to a clear boolean like options.soundMusic.wasDisabled (or wasEnabled) and simplify the comparison by computing a local wasDisabled = not options.soundMusic.wasDisabled (or local wasEnabled = options.soundMusic.wasEnabled) and then use if shouldBeDisabled ~= wasDisabled then (or if shouldBeDisabled ~= (not options.soundMusic.wasEnabled) accordingly). Update the assignment that flips the flag (currently options.soundMusic.aux = not shouldBeDisabled) to set the new descriptive flag consistently (e.g. options.soundMusic.wasDisabled = shouldBeDisabled or options.soundMusic.wasEnabled = not shouldBeDisabled) and leave the removeEvent/scheduleEvent/g_sounds calls unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@modules/client_options/data_options.lua`:
- Around line 767-808: The master volume handler (soundMaster.action) currently
writes channel gains directly via g_sounds.getChannel(...):setGain(value / 100),
which overwrites per-channel sliders (soundMusic, soundAmbience, soundItems,
soundEventVolume) leading to last-write-wins; change to multiplicative behavior
by applying effectiveGain = (masterValue/100) * (channelOptionValue/100) for
each channel instead of just masterValue/100, and add/update a small helper
(e.g., applyChannelGain or use the existing channel setters) so both
soundMaster.action and each individual channel setter (soundMusic,
soundAmbience, soundItems, soundEventVolume) compute and set
g_sounds.getChannel(channelId):setGain(master * channel) so updates from either
control preserve the multiplicative relationship.
In `@modules/client_options/styles/sound/battleSounds.otui`:
- Around line 32-55: Widget IDs for the battle sound sub-channel controls are
inconsistent (e.g., battleSoundOwnBattlesubChannelsSpells,
battleSoundOwnBattleSubChannelsAttack,
battleSoundOwnBattleSoundSubChannelsHealing/Support) which will break lookups;
rename IDs to a single consistent pattern (for example:
battleSoundOwnBattleSubChannelsSpells, battleSoundOwnBattleSubChannelsAttack,
battleSoundOwnBattleSubChannelsHealing, battleSoundOwnBattleSubChannelsSupport)
and update any places that reference the old IDs (including the QtCheckBox
`@onCheckChange` callbacks and any Lua code in modules.client_options that
reads/writes options via setOption/getId). Ensure you remove the duplicated
"Sound" and fix the lowercase "s" in "subChannels" so all IDs match exactly.
In `@modules/client_options/styles/sound/sound.otui`:
- Around line 15-17: The sliders are incorrectly clamped to 1–100; update each
slider block to allow 0 by changing &minimumScrollValue from 1 to 0 (e.g.,
update the &minimumScrollValue in the blocks containing
&minimumScrollValue/&maximumScrollValue/&scrollSize at the shown occurrences —
lines 15-17, 35-37, 61-63, 82-84, 113-115) so the range becomes 0–100 and the
panel can represent a true muted (0) volume.
In `@modules/client_options/styles/sound/uiSounds.otui`:
- Around line 66-68: The option id "soundNotificationUIInteractions" must be
renamed to follow the existing soundNotificationsubChannels* pattern so it
round-trips with stored settings; change the id to
"soundNotificationsubChannelsUIInteractions" (or the exact existing subchannel
key used elsewhere) in the OptionCheckBox definition and update any references
that read/write subchannel keys (e.g., code handling
soundNotificationsubChannels*) so the UI toggle maps to the correct stored
subchannel.
- Around line 108-110: The tooltip for the setting identified by id
"soundNotificationsubChannelsSystemAnnouncements" is incorrect (it references
equipped-item timers); update the !tooltip value to a relevant, localized string
via tr(), e.g. tr('Play a sound when system announcements are posted') or
similar, so that the tooltip matches the feature (system announcement sound
notifications) and replace the existing incorrect text in the UI resource for
soundNotificationsubChannelsSystemAnnouncements.
In `@modules/corelib/const.lua`:
- Around line 332-342: Rename the misspelled key 'secundaryChannel' to
'secondaryChannel' in the constant table (replace the identifier
secundaryChannel = 12 with secondaryChannel = 12) and search the codebase for
any references to 'secundaryChannel' to update them to 'secondaryChannel' so
usages (lookups, assignments, comparisons) remain consistent.
In `@modules/game_soundDebug/sounddebug.lua`:
- Around line 188-195: The resetSummaries()/clearVisuals() functions call
setText() on UI label objects that may be nil if setupWidgets() hasn't run;
guard these calls by first checking the window/widget initialization (e.g., if
headerLabel and mixSummaryLabel and sourceSummaryLabel etc. are non-nil) or add
an isInitialized flag set in setupWidgets(), and only call setText() when
initialized; update resetSummaries(), clearVisuals(), and the other similar
blocks (the other summary-reset functions mentioned) to perform these nil checks
or gate the entire function behind the isInitialized boolean before mutating any
labels.
In `@src/client/uimap.cpp`:
- Around line 162-177: getTilePoint and getTileRect currently return physical
pixels because they add m_mapviewRect (already scaled by
g_window.getDisplayDensity()); change them to return logical/UI coordinates by
removing that added physical offset and instead account for display density: in
getTilePoint (and any use of transformPositionTo2D) stop adding
m_mapviewRect.topLeft() directly (or divide that topLeft by
g_window.getDisplayDensity()), so the returned point is in UI/widget
coordinates, and in getTileRect compute width/height in logical pixels (divide
m_mapView->m_tileSize * horizontal/verticalStretchFactor by
g_window.getDisplayDensity() before rounding and clamping). Update references to
m_mapviewRect.topLeft(), getTilePoint, and getTileRect accordingly so the
Lua-facing API returns logical UI coords consistent with the rest of UIMap.
In `@src/framework/sound/soundmanager_protocol.cpp`:
- Around line 172-186: The current per-category volume
(getProtocolVolumeSetting) is used only as an on/off gate in
SoundManager::shouldPlayProtocolSound so gain later ignores category volume;
change callers to compute a scaledCategoryVolume =
std::clamp(getProtocolVolumeSetting(soundSource), 0, 100) / 100.0f and pass that
scaled float into the playback/gain calculation instead of assuming full volume,
then use that same scaledCategoryVolume when calling recordProtocolDebugEvent()
/ logProtocolDebug() so the debug gain reflects category attenuation; keep
existing gating via
shouldPlayProtocolSound/isProtocolSubChannelEnabled/getSoundEffectType but plumb
the scaledCategoryVolume through the playback path and apply it multiplicatively
to the distance-derived gain.
- Around line 110-113: The option keys used when initializing
m_protocolSoundSettings are inconsistent: m_protocolSoundSettings.ownHealing,
.ownSupport and .ownWeapons use
"battleSoundOwnBattleSoundSubChannels{Healing,Support,Weapons}" which includes
an extra "Sound" and will miss the intended setting; update these keys to match
the correct pattern "battleSoundOwnBattleSubChannelsHealing",
"battleSoundOwnBattleSubChannelsSupport" and
"battleSoundOwnBattleSubChannelsWeapons" (the same pattern used by ownAttack) in
the soundmanager_protocol initialization, and scan for other occurrences of the
misspelled keys to correct them as well.
In `@src/framework/sound/soundmanager.cpp`:
- Around line 290-306: playUiSoundById currently ignores randomness by always
using the first ID from getRandomSoundIds; update playUiSoundById to select a
random element from the vector returned by getRandomSoundIds instead of using
.front(). Use a proper random engine (e.g., std::mt19937 seeded once) and
std::uniform_int_distribution to pick an index into the soundIds vector, then
call getAudioFileNameById(selectedId) and proceed to
playChannelSound(CHANNEL_UI, buildProtocolSoundPath(fileName)); ensure you
handle empty vectors as currently done.
---
Outside diff comments:
In `@src/framework/sound/combinedsoundsource.cpp`:
- Around line 62-108: CombinedSoundSource::setFading currently propagates fading
to child sources but doesn't persist the values to the inherited member
variables; update setFading to assign the incoming values to m_fadeState,
m_fadeTime, m_fadeStartTime, and m_fadeGain before iterating m_sources so the
object's fade state is consistent with other setters (e.g., setLooping, setGain)
and then continue propagating to each child source.
In `@src/framework/sound/soundmanager.h`:
- Around line 45-50: The new uint32_t field maxSoundDistance in struct
ClientItemAmbient is left uninitialized; make it default-initialized to a safe
sentinel (e.g., 0 or other agreed sentinel) in the member declaration so
default-constructed or partially-parsed ClientItemAmbient instances have a
defined value; also update any constructors or aggregate initializations that
might rely on an implicit value to ensure they don't overwrite the default
unintentionally (refer to ClientItemAmbient and maxSoundDistance).
---
Nitpick comments:
In `@modules/client_options/data_options.lua`:
- Around line 810-834: The aux flag in the soundMusic table (inside the action
function) is confusing; rename options.soundMusic.aux to a clear boolean like
options.soundMusic.wasDisabled (or wasEnabled) and simplify the comparison by
computing a local wasDisabled = not options.soundMusic.wasDisabled (or local
wasEnabled = options.soundMusic.wasEnabled) and then use if shouldBeDisabled ~=
wasDisabled then (or if shouldBeDisabled ~= (not options.soundMusic.wasEnabled)
accordingly). Update the assignment that flips the flag (currently
options.soundMusic.aux = not shouldBeDisabled) to set the new descriptive flag
consistently (e.g. options.soundMusic.wasDisabled = shouldBeDisabled or
options.soundMusic.wasEnabled = not shouldBeDisabled) and leave the
removeEvent/scheduleEvent/g_sounds calls unchanged.
In `@modules/corelib/ui/uibutton.lua`:
- Line 7: Remove the hardcoded sound id set by button:setClickSound(2774) and
instead apply the theme/style-driven default: read the click sound from the
button's style/theme (or from a shared constant like DEFAULT_CLICK_SOUND exposed
by the OTUI theme module) and call button:setClickSound(...) with that value;
update any initialization around the UI button creation in uibutton.lua to fall
back to the shared/default value when the style does not specify one so the
click sound can be overridden by theme changes.
In `@modules/corelib/ui/uiminiwindow.lua`:
- Line 7: The miniwindow currently registers the click sound with
miniwindow:addSound(ESoundUI.SoundTypeClick, 2785), which duplicates the
display-sound ID used elsewhere and differs from the base Button click ID 2774;
decide whether miniwindow clicks should use the distinct Button click sound
(2774) or a unique new ID, then update the call to
miniwindow:addSound(ESoundUI.SoundTypeClick, <chosen-id>) accordingly and ensure
any corresponding OTUI/10-windows.otui entries are adjusted to keep click vs
display sounds distinct (reference miniwindow:addSound and
ESoundUI.SoundTypeClick).
In `@src/client/luafunctions.cpp`:
- Around line 1178-1181: The g_sounds singleton binding
(g_lua.bindSingletonFunction("g_sounds", "getDebugSnapshot",
&SoundManager::getDebugSnapshot, &g_sounds)) is placed amid UIMap/UIMinimap
class bindings and should be moved or documented: relocate this bind call to the
block where other singletons (e.g., g_things, g_map) are registered near the top
of the function to keep singleton registrations together, or if there is a
specific reason for its current placement, add a clear comment above the binding
explaining why it must remain here; update references to
SoundManager::getDebugSnapshot and g_sounds accordingly.
In `@src/framework/ui/uiwidget.h`:
- Around line 449-451: Change the sound-type parameters from int to the
UISoundType enum to enforce type safety: update the declarations of addSound(int
soundType, int soundId) and removeSound(int soundType) to addSound(UISoundType
soundType, int soundId) and removeSound(UISoundType soundType), and propagate
the type change through corresponding implementations and all call sites
(including any overloads or tests) to use UISoundType instead of raw ints;
ensure any switch/case or map keys that previously used ints are updated to
accept UISoundType and include necessary header or forward declaration for
UISoundType.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f3849e65-2b11-4ebb-bdc2-02b6d4bd755c
📒 Files selected for processing (45)
data/styles/10-buttons.otuidata/styles/10-windows.otuimodules/client/client.luamodules/client_options/data_options.luamodules/client_options/options.luamodules/client_options/styles/sound/audio.otuimodules/client_options/styles/sound/battleSounds.otuimodules/client_options/styles/sound/sound.otuimodules/client_options/styles/sound/uiSounds.otuimodules/corelib/const.luamodules/corelib/ui/uibutton.luamodules/corelib/ui/uiminiwindow.luamodules/game_interface/interface.otmodmodules/game_screenshot/game_screenshot.luamodules/game_soundDebug/sounddebug.luamodules/game_soundDebug/sounddebug.otmodmodules/game_soundDebug/sounddebug.otuimodules/game_things/things.luasrc/CMakeLists.txtsrc/client/creature.cppsrc/client/game.cppsrc/client/luafunctions.cppsrc/client/luavaluecasts_client.cppsrc/client/luavaluecasts_client.hsrc/client/map.cppsrc/client/protocolgameparse.cppsrc/client/uimap.cppsrc/client/uimap.hsrc/framework/luafunctions.cppsrc/framework/sound/combinedsoundsource.cppsrc/framework/sound/soundchannel.cppsrc/framework/sound/soundmanager.cppsrc/framework/sound/soundmanager.hsrc/framework/sound/soundmanager_debug.cppsrc/framework/sound/soundmanager_itemambience.cppsrc/framework/sound/soundmanager_protocol.cppsrc/framework/sound/soundmanager_types.hsrc/framework/sound/soundsource.cppsrc/framework/sound/soundsource.hsrc/framework/sound/streamsoundsource.cppsrc/framework/sound/streamsoundsource.hsrc/framework/ui/uiwidget.cppsrc/framework/ui/uiwidget.hsrc/framework/ui/uiwidgetbasestyle.cppvc18/otclient.vcxproj
💤 Files with no reviewable changes (1)
- modules/client_options/styles/sound/audio.otui
Comment on lines
+767
to
+808
| soundMaster = { | ||
| value = 100, | ||
| aux = true, | ||
| action = function(value, options, controller, panels, extraWidgets) | ||
| if not g_sounds then | ||
| return | ||
| end | ||
| local soundMasterWidget = panels.soundPanel:recursiveGetChildById('soundMaster') | ||
| soundMasterWidget:setText(string.format('Master Volume: %d %%', value)) | ||
| for channelName, channelId in pairs(SoundChannels) do | ||
| g_sounds.getChannel(channelId):setGain(value / 100) | ||
| end | ||
| local shouldDisable = value <= 1 | ||
| local hasChanged = shouldDisable ~= (options.soundMaster.aux or false) | ||
| if not hasChanged then | ||
| return | ||
| end | ||
| options.soundMaster.aux = shouldDisable | ||
| if shouldDisable then | ||
| g_sounds.setAudioEnabled(false) | ||
| extraWidgets.audioButton:setIcon('/images/topbuttons/button_mute_pressed') | ||
| else | ||
| g_sounds.setAudioEnabled(true) | ||
| extraWidgets.audioButton:setIcon('/images/topbuttons/button_mute_up') | ||
| end | ||
| local function togglePanel(panel) | ||
| if not panel then | ||
| return | ||
| end | ||
| local infoPanel = panel:recursiveGetChildById('info') | ||
| local children = panel:getChildren() | ||
| for _, widget in ipairs(children) do | ||
| if widget:getStyle().__class ~= "UILabel" then | ||
| widget:setEnabled(not shouldDisable) | ||
| end | ||
| end | ||
| infoPanel:setVisible(shouldDisable) | ||
| infoPanel:setHeight(shouldDisable and 30 or 0) | ||
| end | ||
| togglePanel(panels.battleSoundsPanel) | ||
| togglePanel(panels.iuSoundPanel) | ||
| end |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if there's any other place that implements master volume as a multiplier
rg -n -C3 'soundMaster|masterVolume|setGain' --type=lua --iglob '*option*'Repository: opentibiabr/otclient
Length of output: 5540
🏁 Script executed:
#!/bin/bash
# Get the full implementation of individual sound channels
rg -n -A 15 'soundMusic\s*=|soundAmbience\s*=|soundItems\s*=|soundEventVolume\s*=' modules/client_options/data_options.lua | head -100Repository: opentibiabr/otclient
Length of output: 2291
🏁 Script executed:
#!/bin/bash
# Search for g_sounds definition and setGain implementation
rg -n -A 10 'function.*setGain|def.*setGain' --type=luaRepository: opentibiabr/otclient
Length of output: 896
🏁 Script executed:
#!/bin/bash
# Search for g_sounds initialization and how it handles volume
rg -n -B 5 -A 10 'g_sounds\s*=' --type=lua | head -80Repository: opentibiabr/otclient
Length of output: 671
🏁 Script executed:
#!/bin/bash
# Look for comments around the soundMaster definition
sed -n '750,810p' modules/client_options/data_options.lua | cat -nRepository: opentibiabr/otclient
Length of output: 2864
🏁 Script executed:
#!/bin/bash
# Look for README or docs mentioning audio or sound
find . -type f \( -name "README*" -o -name "*.md" \) | xargs grep -l -i "audio\|sound\|volume" 2>/dev/null | head -5Repository: opentibiabr/otclient
Length of output: 99
🏁 Script executed:
#!/bin/bash
# Check both README files for audio/sound information
echo "=== README.md ===" && grep -n -i -B 2 -A 5 "audio\|sound\|volume" README.md && echo -e "\n=== data/sounds/README.md ===" && cat data/sounds/README.mdRepository: opentibiabr/otclient
Length of output: 3141
🏁 Script executed:
#!/bin/bash
# Look for how options are loaded/applied
rg -n -B 3 -A 8 'onLoad|loadOption|applyOption' modules/client_options/data_options.lua | head -50Repository: opentibiabr/otclient
Length of output: 46
🏁 Script executed:
#!/bin/bash
# Look at the complete soundMaster action to see if it triggers updates to individual channels
sed -n '767,808p' modules/client_options/data_options.lua | cat -nRepository: opentibiabr/otclient
Length of output: 2199
Master volume overwrites individual channel gains—needs multiplicative behavior or clarifying comment.
Lines 776-778 set all channel gains to the master volume value directly:
for channelName, channelId in pairs(SoundChannels) do
g_sounds.getChannel(channelId):setGain(value / 100)
endThe individual channel sliders (soundMusic, soundAmbience, soundItems, soundEventVolume) each set their own gain independently without considering the master volume, creating a "last-write-wins" behavior. For example: set Master to 50%, then adjust Music to 100%—the Music channel ends up at 100%, not 50% of 100%.
Standard audio systems multiply master with individual channel volumes. Either implement multiplicative behavior (individual gain × master gain) or add a comment explaining why this simpler approach is intentional.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@modules/client_options/data_options.lua` around lines 767 - 808, The master
volume handler (soundMaster.action) currently writes channel gains directly via
g_sounds.getChannel(...):setGain(value / 100), which overwrites per-channel
sliders (soundMusic, soundAmbience, soundItems, soundEventVolume) leading to
last-write-wins; change to multiplicative behavior by applying effectiveGain =
(masterValue/100) * (channelOptionValue/100) for each channel instead of just
masterValue/100, and add/update a small helper (e.g., applyChannelGain or use
the existing channel setters) so both soundMaster.action and each individual
channel setter (soundMusic, soundAmbience, soundItems, soundEventVolume) compute
and set g_sounds.getChannel(channelId):setGain(master * channel) so updates from
either control preserve the multiplicative relationship.
Comment on lines
+32
to
+55
| OptionCheckBox | ||
| id: battleSoundOwnBattlesubChannelsSpells | ||
| !text: tr('Spells') | ||
| anchors.top: prev.bottom | ||
| margin-top:5 | ||
|
|
||
| MiniWindowContents | ||
| id: panelOwnBattleSubChannels | ||
| padding-left: 10 | ||
| padding-right: 5 | ||
| padding-top: 10 | ||
| layout: verticalBox | ||
| QtCheckBox | ||
| id: battleSoundOwnBattleSubChannelsAttack | ||
| !text: tr('Attack') | ||
| @onCheckChange: modules.client_options.setOption(self:getId(), self:isChecked()) | ||
| QtCheckBox | ||
| id: battleSoundOwnBattleSoundSubChannelsHealing | ||
| !text: tr('Healing') | ||
| @onCheckChange: modules.client_options.setOption(self:getId(), self:isChecked()) | ||
| QtCheckBox | ||
| id: battleSoundOwnBattleSoundSubChannelsSupport | ||
| !text: tr('Support') | ||
| @onCheckChange: modules.client_options.setOption(self:getId(), self:isChecked()) |
There was a problem hiding this comment.
Inconsistent ID naming may cause bugs.
The widget IDs have inconsistent naming patterns:
- Line 33:
battleSoundOwnBattlesubChannelsSpells(lowercase 's' in "subChannels") - Line 45:
battleSoundOwnBattleSubChannelsAttack(uppercase 'S') - Line 49:
battleSoundOwnBattleSoundSubChannelsHealing(extra "Sound" word) - Line 53:
battleSoundOwnBattleSoundSubChannelsSupport(extra "Sound" word)
This inconsistency could cause issues when accessing these widgets from Lua code or when the options system tries to save/load values by ID.
✏️ Proposed fix for consistent naming
OptionCheckBox
- id: battleSoundOwnBattlesubChannelsSpells
+ id: battleSoundOwnBattleSubChannelsSpells
!text: tr('Spells')
anchors.top: prev.bottom
margin-top:5
...
QtCheckBox
id: battleSoundOwnBattleSubChannelsAttack
!text: tr('Attack')
`@onCheckChange`: modules.client_options.setOption(self:getId(), self:isChecked())
QtCheckBox
- id: battleSoundOwnBattleSoundSubChannelsHealing
+ id: battleSoundOwnBattleSubChannelsHealing
!text: tr('Healing')
`@onCheckChange`: modules.client_options.setOption(self:getId(), self:isChecked())
QtCheckBox
- id: battleSoundOwnBattleSoundSubChannelsSupport
+ id: battleSoundOwnBattleSubChannelsSupport
!text: tr('Support')
`@onCheckChange`: modules.client_options.setOption(self:getId(), self:isChecked())
OptionCheckBox
- id: battleSoundOwnBattleSoundSubChannelsWeapons
+ id: battleSoundOwnBattleSubChannelsWeapons
anchors.top: prev.bottom
!text: tr('Weapons')
margin-top: -10🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@modules/client_options/styles/sound/battleSounds.otui` around lines 32 - 55,
Widget IDs for the battle sound sub-channel controls are inconsistent (e.g.,
battleSoundOwnBattlesubChannelsSpells, battleSoundOwnBattleSubChannelsAttack,
battleSoundOwnBattleSoundSubChannelsHealing/Support) which will break lookups;
rename IDs to a single consistent pattern (for example:
battleSoundOwnBattleSubChannelsSpells, battleSoundOwnBattleSubChannelsAttack,
battleSoundOwnBattleSubChannelsHealing, battleSoundOwnBattleSubChannelsSupport)
and update any places that reference the old IDs (including the QtCheckBox
`@onCheckChange` callbacks and any Lua code in modules.client_options that
reads/writes options via setOption/getId). Ensure you remove the duplicated
"Sound" and fix the lowercase "s" in "subChannels" so all IDs match exactly.
Comment on lines
+15
to
+17
| &minimumScrollValue: 1 | ||
| &maximumScrollValue: 100 | ||
| &scrollSize: 21 |
There was a problem hiding this comment.
Allow the new volume sliders to reach 0%.
All five sliders now clamp to 1-100, so the new panel can no longer mute an individual channel or faithfully represent a persisted 0 volume value.
🎚️ Suggested fix
- &minimumScrollValue: 1
+ &minimumScrollValue: 0
...
- &minimumScrollValue: 1
+ &minimumScrollValue: 0
...
- &minimumScrollValue: 1
+ &minimumScrollValue: 0
...
- &minimumScrollValue: 1
+ &minimumScrollValue: 0
...
- &minimumScrollValue: 1
+ &minimumScrollValue: 0Also applies to: 35-37, 61-63, 82-84, 113-115
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@modules/client_options/styles/sound/sound.otui` around lines 15 - 17, The
sliders are incorrectly clamped to 1–100; update each slider block to allow 0 by
changing &minimumScrollValue from 1 to 0 (e.g., update the &minimumScrollValue
in the blocks containing &minimumScrollValue/&maximumScrollValue/&scrollSize at
the shown occurrences — lines 15-17, 35-37, 61-63, 82-84, 113-115) so the range
becomes 0–100 and the panel can represent a true muted (0) volume.
Comment on lines
+66
to
+68
| OptionCheckBox | ||
| id: soundNotificationUIInteractions | ||
| !text: tr('UI Interactions') |
There was a problem hiding this comment.
Rename this option to the notification subchannel key.
id: soundNotificationUIInteractions breaks the soundNotificationsubChannels* naming used by the rest of this section. As written, the "UI Interactions" notification toggle is unlikely to round-trip with the stored notification setting or hit the expected subchannel gate.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@modules/client_options/styles/sound/uiSounds.otui` around lines 66 - 68, The
option id "soundNotificationUIInteractions" must be renamed to follow the
existing soundNotificationsubChannels* pattern so it round-trips with stored
settings; change the id to "soundNotificationsubChannelsUIInteractions" (or the
exact existing subchannel key used elsewhere) in the OptionCheckBox definition
and update any references that read/write subchannel keys (e.g., code handling
soundNotificationsubChannels*) so the UI toggle maps to the correct stored
subchannel.
Comment on lines
+108
to
+110
| id: soundNotificationsubChannelsSystemAnnouncements | ||
| !text: tr('System Announcements') | ||
| !tooltip: tr('Check this box to see how much time or how many charges are left \non your equipped items') |
There was a problem hiding this comment.
Tooltip text is copied from an unrelated setting.
This tooltip describes equipped-item timers, not system-announcement sounds, so it will send users to the wrong feature.
📝 Suggested text
- !tooltip: tr('Check this box to see how much time or how many charges are left \non your equipped items')
+ !tooltip: tr('Play a sound when a system announcement is received.')📝 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
| id: soundNotificationsubChannelsSystemAnnouncements | |
| !text: tr('System Announcements') | |
| !tooltip: tr('Check this box to see how much time or how many charges are left \non your equipped items') | |
| id: soundNotificationsubChannelsSystemAnnouncements | |
| !text: tr('System Announcements') | |
| !tooltip: tr('Play a sound when a system announcement is received.') |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@modules/client_options/styles/sound/uiSounds.otui` around lines 108 - 110,
The tooltip for the setting identified by id
"soundNotificationsubChannelsSystemAnnouncements" is incorrect (it references
equipped-item timers); update the !tooltip value to a relevant, localized string
via tr(), e.g. tr('Play a sound when system announcements are posted') or
similar, so that the tooltip matches the feature (system announcement sound
notifications) and replace the existing incorrect text in the UI resource for
soundNotificationsubChannelsSystemAnnouncements.
Comment on lines
+188
to
+195
| local function resetSummaries() | ||
| headerLabel:setText('Sound debug is idle.') | ||
| mixSummaryLabel:setText('Waiting for active sources.') | ||
| sourceSummaryLabel:setText('No sources.') | ||
| itemSummaryLabel:setText('No ambient items active.') | ||
| eventSummaryLabel:setText('No positional sound events yet.') | ||
| spatialSummaryLabel:setText('Map and radar markers follow positional audio.') | ||
| end |
There was a problem hiding this comment.
Guard clearVisuals() until the window is initialized.
init() wires this function to g_game.onGameEnd, but these labels are only assigned in setupWidgets(). Logging out before ever opening the debug window will call setText() on nil here.
🛡️ Proposed fix
local function clearVisuals()
+ if not widgetsInitialized then
+ return
+ end
+
for _, state in pairs(meterStates) do
state.value = 0
state.peak = 0
state.base = 0
endAlso applies to: 222-237, 487-490
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@modules/game_soundDebug/sounddebug.lua` around lines 188 - 195, The
resetSummaries()/clearVisuals() functions call setText() on UI label objects
that may be nil if setupWidgets() hasn't run; guard these calls by first
checking the window/widget initialization (e.g., if headerLabel and
mixSummaryLabel and sourceSummaryLabel etc. are non-nil) or add an isInitialized
flag set in setupWidgets(), and only call setText() when initialized; update
resetSummaries(), clearVisuals(), and the other similar blocks (the other
summary-reset functions mentioned) to perform these nil checks or gate the
entire function behind the isInitialized boolean before mutating any labels.
Comment on lines
+162
to
+177
| auto point = m_mapView->transformPositionTo2D(pos) - m_mapView->m_posInfo.drawOffset; | ||
| point.x *= m_mapView->m_posInfo.horizontalStretchFactor; | ||
| point.y *= m_mapView->m_posInfo.verticalStretchFactor; | ||
| point += m_mapviewRect.topLeft(); | ||
| return point; | ||
| } | ||
|
|
||
| Rect UIMap::getTileRect(const Position& pos) | ||
| { | ||
| if (!pos.isMapPosition() || !m_mapView || !m_mapView->isInRange(pos)) | ||
| return {}; | ||
|
|
||
| const auto point = getTilePoint(pos); | ||
| const auto width = std::max<int>(1, std::lround(static_cast<float>(m_mapView->m_tileSize) * m_mapView->m_posInfo.horizontalStretchFactor)); | ||
| const auto height = std::max<int>(1, std::lround(static_cast<float>(m_mapView->m_tileSize) * m_mapView->m_posInfo.verticalStretchFactor)); | ||
| return { point.x, point.y, width, height }; |
There was a problem hiding this comment.
Return logical UI coordinates from these helpers.
getTilePoint()/getTileRect() add m_mapviewRect, which is already multiplied by g_window.getDisplayDensity(). That makes this new Lua-facing API report physical pixels instead of widget/UI pixels, so overlays anchored with these values will be offset on HiDPI displays and won't round-trip cleanly with the rest of UIMap's point-based API.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/client/uimap.cpp` around lines 162 - 177, getTilePoint and getTileRect
currently return physical pixels because they add m_mapviewRect (already scaled
by g_window.getDisplayDensity()); change them to return logical/UI coordinates
by removing that added physical offset and instead account for display density:
in getTilePoint (and any use of transformPositionTo2D) stop adding
m_mapviewRect.topLeft() directly (or divide that topLeft by
g_window.getDisplayDensity()), so the returned point is in UI/widget
coordinates, and in getTileRect compute width/height in logical pixels (divide
m_mapView->m_tileSize * horizontal/verticalStretchFactor by
g_window.getDisplayDensity() before rounding and clamping). Update references to
m_mapviewRect.topLeft(), getTilePoint, and getTileRect accordingly so the
Lua-facing API returns logical UI coords consistent with the rest of UIMap.
Comment on lines
+110
to
+113
| m_protocolSoundSettings.ownAttack = getBooleanSetting("battleSoundOwnBattleSubChannelsAttack", true); | ||
| m_protocolSoundSettings.ownHealing = getBooleanSetting("battleSoundOwnBattleSoundSubChannelsHealing", true); | ||
| m_protocolSoundSettings.ownSupport = getBooleanSetting("battleSoundOwnBattleSoundSubChannelsSupport", true); | ||
| m_protocolSoundSettings.ownWeapons = getBooleanSetting("battleSoundOwnBattleSoundSubChannelsWeapons", true); |
There was a problem hiding this comment.
These option keys look misspelled.
battleSoundOwnBattleSoundSubChannels{Healing,Support,Weapons} has an extra Sound compared with ...OwnBattleSubChannelsAttack and the other groups. Unless the options layer uses the same typo, those three toggles will always fall back to the default value.
🔧 Proposed fix
- m_protocolSoundSettings.ownHealing = getBooleanSetting("battleSoundOwnBattleSoundSubChannelsHealing", true);
- m_protocolSoundSettings.ownSupport = getBooleanSetting("battleSoundOwnBattleSoundSubChannelsSupport", true);
- m_protocolSoundSettings.ownWeapons = getBooleanSetting("battleSoundOwnBattleSoundSubChannelsWeapons", true);
+ m_protocolSoundSettings.ownHealing = getBooleanSetting("battleSoundOwnBattleSubChannelsHealing", true);
+ m_protocolSoundSettings.ownSupport = getBooleanSetting("battleSoundOwnBattleSubChannelsSupport", true);
+ m_protocolSoundSettings.ownWeapons = getBooleanSetting("battleSoundOwnBattleSubChannelsWeapons", true);📝 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
| m_protocolSoundSettings.ownAttack = getBooleanSetting("battleSoundOwnBattleSubChannelsAttack", true); | |
| m_protocolSoundSettings.ownHealing = getBooleanSetting("battleSoundOwnBattleSoundSubChannelsHealing", true); | |
| m_protocolSoundSettings.ownSupport = getBooleanSetting("battleSoundOwnBattleSoundSubChannelsSupport", true); | |
| m_protocolSoundSettings.ownWeapons = getBooleanSetting("battleSoundOwnBattleSoundSubChannelsWeapons", true); | |
| m_protocolSoundSettings.ownAttack = getBooleanSetting("battleSoundOwnBattleSubChannelsAttack", true); | |
| m_protocolSoundSettings.ownHealing = getBooleanSetting("battleSoundOwnBattleSubChannelsHealing", true); | |
| m_protocolSoundSettings.ownSupport = getBooleanSetting("battleSoundOwnBattleSubChannelsSupport", true); | |
| m_protocolSoundSettings.ownWeapons = getBooleanSetting("battleSoundOwnBattleSubChannelsWeapons", true); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/framework/sound/soundmanager_protocol.cpp` around lines 110 - 113, The
option keys used when initializing m_protocolSoundSettings are inconsistent:
m_protocolSoundSettings.ownHealing, .ownSupport and .ownWeapons use
"battleSoundOwnBattleSoundSubChannels{Healing,Support,Weapons}" which includes
an extra "Sound" and will miss the intended setting; update these keys to match
the correct pattern "battleSoundOwnBattleSubChannelsHealing",
"battleSoundOwnBattleSubChannelsSupport" and
"battleSoundOwnBattleSubChannelsWeapons" (the same pattern used by ownAttack) in
the soundmanager_protocol initialization, and scan for other occurrences of the
misspelled keys to correct them as well.
Comment on lines
+172
to
+186
| bool SoundManager::shouldPlayProtocolSound(const uint8_t soundSource, const uint16_t soundEffectId) | ||
| { | ||
| if (!isAudioEnabled()) | ||
| return false; | ||
|
|
||
| const auto volume = getProtocolVolumeSetting(soundSource); | ||
| if (volume <= 1) | ||
| return false; | ||
|
|
||
| if (soundSource >= 1) { | ||
| return isProtocolSubChannelEnabled(soundSource, getSoundEffectType(soundEffectId)); | ||
| } | ||
|
|
||
| return soundSource == 0; | ||
| } |
There was a problem hiding this comment.
Apply the configured category volume to the gain.
The own/others/creature volume settings are only used as an on/off gate today. Once playback reaches this helper, gain is derived only from distance, so 10% and 100% sound identical, and the debug gain you record later is overstated too.
🔊 Suggested direction
-bool SoundManager::playProtocolAudioFileId(const uint32_t audioFileId, const Position& pos, const int channelId)
+bool SoundManager::playProtocolAudioFileId(const uint32_t audioFileId, const Position& pos, const int channelId, const float volumeScale)
{
const auto& localPlayer = g_game.getLocalPlayer();
if (!localPlayer || !pos.isMapPosition())
return false;
@@
- const float gain = 1.0f - (distance / PROTOCOL_MAX_DISTANCE);
+ const float gain = (1.0f - (distance / PROTOCOL_MAX_DISTANCE)) * volumeScale;
const float stereoBalance = std::clamp(static_cast<float>(pos.x - playerPos.x) / PROTOCOL_MAX_DISTANCE, -1.0f, 1.0f);
@@
}Then pass std::clamp(getProtocolVolumeSetting(soundSource), 0, 100) / 100.0f from the call sites and use that same scaled gain for recordProtocolDebugEvent() / logProtocolDebug().
Also applies to: 230-257
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/framework/sound/soundmanager_protocol.cpp` around lines 172 - 186, The
current per-category volume (getProtocolVolumeSetting) is used only as an on/off
gate in SoundManager::shouldPlayProtocolSound so gain later ignores category
volume; change callers to compute a scaledCategoryVolume =
std::clamp(getProtocolVolumeSetting(soundSource), 0, 100) / 100.0f and pass that
scaled float into the playback/gain calculation instead of assuming full volume,
then use that same scaledCategoryVolume when calling recordProtocolDebugEvent()
/ logProtocolDebug() so the debug gain reflects category attenuation; keep
existing gating via
shouldPlayProtocolSound/isProtocolSubChannelEnabled/getSoundEffectType but plumb
the scaledCategoryVolume through the playback path and apply it multiplicatively
to the distance-derived gain.
Comment on lines
+290
to
+306
| void SoundManager::playUiSoundById(const int soundId) | ||
| { | ||
| if (soundId <= 0) | ||
| return; | ||
|
|
||
| std::string fileName = getAudioFileNameById(soundId); | ||
| if (fileName.empty()) { | ||
| const auto soundIds = getRandomSoundIds(soundId); | ||
| if (!soundIds.empty()) | ||
| fileName = getAudioFileNameById(soundIds.front()); | ||
| } | ||
|
|
||
| if (fileName.empty()) | ||
| return; | ||
|
|
||
| playChannelSound(CHANNEL_UI, buildProtocolSoundPath(fileName)); | ||
| } |
There was a problem hiding this comment.
Random sound selection not implemented — always picks first element.
getRandomSoundIds() returns a list of sound IDs intended for random selection, but this function only uses .front(). This defeats the "random sound effect" feature described in the protobuf schema.
🐛 Proposed fix to randomize sound selection
void SoundManager::playUiSoundById(const int soundId)
{
if (soundId <= 0)
return;
std::string fileName = getAudioFileNameById(soundId);
if (fileName.empty()) {
const auto soundIds = getRandomSoundIds(soundId);
- if (!soundIds.empty())
- fileName = getAudioFileNameById(soundIds.front());
+ if (!soundIds.empty()) {
+ const auto randomIndex = std::rand() % soundIds.size();
+ fileName = getAudioFileNameById(soundIds[randomIndex]);
+ }
}
if (fileName.empty())
return;
playChannelSound(CHANNEL_UI, buildProtocolSoundPath(fileName));
}Note: Consider using a proper random engine (e.g., std::mt19937 with std::uniform_int_distribution) for better randomness if this is called frequently.
📝 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
| void SoundManager::playUiSoundById(const int soundId) | |
| { | |
| if (soundId <= 0) | |
| return; | |
| std::string fileName = getAudioFileNameById(soundId); | |
| if (fileName.empty()) { | |
| const auto soundIds = getRandomSoundIds(soundId); | |
| if (!soundIds.empty()) | |
| fileName = getAudioFileNameById(soundIds.front()); | |
| } | |
| if (fileName.empty()) | |
| return; | |
| playChannelSound(CHANNEL_UI, buildProtocolSoundPath(fileName)); | |
| } | |
| void SoundManager::playUiSoundById(const int soundId) | |
| { | |
| if (soundId <= 0) | |
| return; | |
| std::string fileName = getAudioFileNameById(soundId); | |
| if (fileName.empty()) { | |
| const auto soundIds = getRandomSoundIds(soundId); | |
| if (!soundIds.empty()) { | |
| const auto randomIndex = std::rand() % soundIds.size(); | |
| fileName = getAudioFileNameById(soundIds[randomIndex]); | |
| } | |
| } | |
| if (fileName.empty()) | |
| return; | |
| playChannelSound(CHANNEL_UI, buildProtocolSoundPath(fileName)); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/framework/sound/soundmanager.cpp` around lines 290 - 306, playUiSoundById
currently ignores randomness by always using the first ID from
getRandomSoundIds; update playUiSoundById to select a random element from the
vector returned by getRandomSoundIds instead of using .front(). Use a proper
random engine (e.g., std::mt19937 seeded once) and std::uniform_int_distribution
to pick an index into the soundIds vector, then call
getAudioFileNameById(selectedId) and proceed to playChannelSound(CHANNEL_UI,
buildProtocolSoundPath(fileName)); ensure you handle empty vectors as currently
done.
Contributor
Author
|
Honestly, I’m not sure if it’s a good idea to merge this or not. I don’t know what the impact of this PR on performance would be (I’m a noob in C++ and don’t know how to use a profiler). I’m just leaving it here for reference in case someone might want it. Obviously, the module, the soundmanager_debug.cpp, and the uimap are used to verify that everything is working properly. |
59 tasks
Contributor
@dudantas can u help us? |
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
2 participants
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.
Make sure the video isn't muted
2026-03-28.19-00-44.mp4
#1098
#1074
Summary by CodeRabbit
New Features
Chores