[peppy]

• Builds off Add setting for the volume of ruleset elements (hitsounds) #28733
• Addresses Auto-SFX / Separate Hitsound Mute #37901, Why are hitsounds so loud in lazer? #38410, Map-agnostic, constant volume hitsound option #18654, Option to turn hitsounds volume off only #25068, Separate SFX and Hit Sounds volume sliders. #34798
• Depends on Adjust sample store retrieval API to allow more flexibility osu-framework#6834

Caveats:

• Intentionally called "hitsound volume", not "gameplay volume" because the intention is to specifically target hitsounds. Gameplay effects should still be adjusted by the effect volume, as far as I'm concerned. (alternative take: call this gameplay and include everything non-UI, then rename "effect" to "UI sounds" or something)
• Affects everything in the playfield and overlay layers. This seems like a pretty good separation point, which includes things like the metronome from muted mod. It's about what players will expect from this.
• Does not affect storyboards. I'm sure we're going to get people complaining about that meaning that "storyboarded hitsounds" are not affected, but I'm not sure whether this is a correct take or not.
• The following now reads bad. It looks as if the "beatmap hitsounds" would be a toggle turning them off, not enabling/disabling beatmap skinned hitsounds. Open to suggestion:

There may be exceptions that come up which we need to handle separately. I may do a 180 based on how this is received and adjust more than one of the caveats above. Tachyon feedback will be important here.

I also have started on a redesign of the volume overlay to better house this. Consider the one in this PR a temporary change.

---

This brings a long-sought feature online. I'm not 100% sure on what this should or shouldn't cover, so any feedback is appreciated on how this works for you!

[cloudrac3r]

Thank you for reintroducing this!

I think renaming the "beatmap hitsounds" toggle to something like "custom hitsounds" or "beatmap samples" would be adequate at clearing up that long-standing confusion.

[peppy]

It would have to be "Custom beatmap hitsounds" at very least, which doesn't fit well in the UI.

[bdach]

This diff is so quaint that it has me... suspicious? Like I'm wondering whether I should be measuring whether this isn't impacting hitsound latency weirdly due to the new mixer being present or something. I forgot or wasn't even aware that just plopping a mixer like this was even possible, so I'm shocked at this being such a simple implementation.

One major thing in a quick run of this implementation of the feature that I'm not sure how to feel about, wasn't (explicitly?) signposted in the OP, and possibly explains why I didn't see this feature as simple to execute: the "effects" volume still affects hitsounds, and in fact the hitsounds volume seems to be the product (as in multiplication) of the effects volume and the hitsounds volume. Which feels weird because prior to this, aside from the master volume, the other gauges were mutually exclusive, but now they are not.

(alternative take: call this gameplay and include everything non-UI, then rename "effect" to "UI sounds" or something)

This doesn't sound doable due to the above.

It looks as if the "beatmap hitsounds" would be a toggle turning them off, not enabling/disabling beatmap skinned hitsounds. Open to suggestion:

Maybe literally just "use hitsounds from beatmap (skin)" or something along those lines.

[peppy]

Like I'm wondering whether I should be measuring whether this isn't impacting hitsound latency weirdly due to the new mixer being present or something

Given we are already using mixers at a top level (migrated as part of the other audio subsystem changes a while back) on most platforms, I don't think this is something which needs to be checked on. We aren't further nesting mixers, as they are still only added top level, so this biggest consideration may be the added CPU from running an extra mixer, but this does seem negligible in the grand scheme.

One major thing in a quick run of this implementation of the feature that I'm not sure how to feel about, wasn't (explicitly?) signposted in the OP, and possibly explains why I didn't see this feature as simple to execute: the "effects" volume still affects hitsounds, and in fact the hitsounds volume seems to be the product (as in multiplication) of the effects volume and the hitsounds volume. Which feels weird because prior to this, aside from the master volume, the other gauges were mutually exclusive, but now they are not.

I thought I checked this multiple times but maybe my ears perceived me. This is a definite blocker, will check on what is causing this and figure out how badly this throws a cog in the works.

[bdach]

will check on what is causing this and figure out how badly this throws a cog in the works.

When you mentioned wanting this feature to be prioritised in non-public spaces I looked into the current state of things a bit and can thus probably answer this one.

The effects volume adjustment is implemented via AudioManager.VolumeSample, and AudioManager.VolumeSample works by hooking onto the global sample store's adjustments. When a sample is retrieved from a SampleStore, the sample is AddItem()'d to the store, which also has the effect of binding adjustments from the store. Thus every sample ever retrieved from AudioManager.Samples will have the volume adjustment applied.

As far as I can tell, there's little wiggle room except for doing the zig of e.g. using a completely separate sample store for gameplay sample retrieval so this adjustment never gets bound, or doing the zag of somehow rewriting effects volume adjustment such that it no longer works by attaching to the global sample store. Both looked not that simple to execute and therefore I passed on that task.

[bdach]

One idea that maybe salvages this in a simple-enough manner is to leave AudioManager.VolumeSample well alone and never use it, and instead add a second mixer for samples at OsuGame level such that MusicController is somehow exempted from it (or use a third mixer for MusicController?). In that setup, because the mixer choice works by upwards hierarchy traversal towards the closest DrawableAudioMixer, the gameplay mixer will still control the gameplay samples, but the game-global mixer will control everything else except the track. I didn't think about this in the past because I wasn't aware mixers worked like this. Very untested though.

The only downside would be that it would be a bit of a smell that the framework already exposes separated mixers for track and sample and this setup would almost completely stop using that setup.

[peppy]

This is my alternative proposal:

diff --git a/osu.Game/Skinning/Skin.cs b/osu.Game/Skinning/Skin.cs
index 9f5cc47d44..2391644997 100644
--- a/osu.Game/Skinning/Skin.cs
+++ b/osu.Game/Skinning/Skin.cs
@@ -155,7 +155,7 @@ public void RecycleSamples()
         {
             Samples?.Dispose();
 
-            var samples = resources?.AudioManager?.GetSampleStore(store);
+            var samples = resources?.AudioManager?.GetSampleStore(store, applyGlobalSampleVolume: false);
 
             if (samples != null)
             {

diff --git a/osu.Framework/Audio/AudioManager.cs b/osu.Framework/Audio/AudioManager.cs
index 824e6c10d..38ab22ce5 100644
--- a/osu.Framework/Audio/AudioManager.cs
+++ b/osu.Framework/Audio/AudioManager.cs
@@ -317,12 +317,16 @@ public ITrackStore GetTrackStore(IResourceStore<byte[]> store = null, AudioMixer
         /// </remarks>
         /// <param name="store">The <see cref="IResourceStore{T}"/> of which to retrieve the <see cref="SampleStore"/>.</param>
         /// <param name="mixer">The <see cref="AudioMixer"/> to use for samples created by this store. Defaults to the global <see cref="SampleMixer"/>.</param>
-        public ISampleStore GetSampleStore(IResourceStore<byte[]> store = null, AudioMixer mixer = null)
+        public ISampleStore GetSampleStore(IResourceStore<byte[]> store = null, AudioMixer mixer = null, bool applyGlobalSampleVolume = true)
         {
             if (store == null) return globalSampleStore.Value;
 
             SampleStore sm = new SampleStore(store, mixer ?? SampleMixer);
-            globalSampleStore.Value.AddItem(sm);
+            if (applyGlobalSampleVolume)
+                globalSampleStore.Value.AddItem(sm);
+            else
+                AddItem(sm);
+
             return sm;
         }
 

Could you get behind it? I honestly think this should have been the default for this method, but there was probably valid reason for nesting it at the time.

[bdach]

This is my alternative proposal:

Doing things at Skin level is a different proposition and doesn't work for "hitsound volume" adjustment specifically because we have lookups of skin samples on screens that are not gameplay / hitsounds. Examples: results screen applause.wav, pause screen background loop.

[peppy]

Could make a method route specifically for hitsounds?

[bdach]

Maybe you could, I'm not sure. Sample lookups are mired in several intermediate classes like PoolableSkinnableSample and SkinnableSound and they are used randomly and interchangeably and in contexts not limited to gameplay. I foresee this devolving into chasing down individual usages and peppering them with boolean flags to determine whether it's a hitsound lookup or not. I may be wrong, haven't tried, but in my head the proposition looks a bit grim.

[peppy]

I foresee this devolving into chasing down individual usages and peppering them with boolean flags to determine whether it's a hitsound lookup or not. I may be wrong, haven't tried, but in my head the proposition looks a bit grim.

I think it will be something like this, although probably in reverse (adding somethign specifically in HitSampleInfo denoting as such).

I'm giving it a try and will report back.

[peppy]

It looks to work...

https://github.com/peppy/osu-framework/compare/sample-store-changes?expand=1

The sketchiest part is the API change framework side, but I don't think it's the end of the world.

[bdach]

4b55c5f shouldn't be here.

Otherwise I guess this is kind of ugly but someone has already posted this to reddit so if I push back on code quality concerns I'll probably get hate mail.