-
Notifications
You must be signed in to change notification settings - Fork 420
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add the ability to adjust a mixer's volume (and decouple custom audio stores from global volumes) #6326
base: master
Are you sure you want to change the base?
Conversation
I've pushed a change with proposed changes as it's easier than trying to drop in a review. |
Note now this is a slightly breaking change – |
I fixed a bug where AudioMixer was being added to the ActiveMixers collection before it was initialized (Audio Thread race condition) |
Could you describe how / where this bug was showing up? |
When creating a new mixer, in the debug visualizer, sometimes the mixer output level bar would not show any activity (sound would still play fine). |
Hi! I'd like to keep the ball rolling on this. Is there something missing I can help with? Or is it just a matter of waiting a bit more for people to review it? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've previously voiced my concerns IRL that I don't have faith in this way of going about things by loosely tying components externally rather than via the well established recursive hierarchy.
But my warning will not be heeded, so I guess we'll have to see how this goes.
if (channel is IAdjustableAudioComponent adj) | ||
adj.BindAdjustments(this); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is the Remove()
one enqueued to the channel, but this one isn't?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
peppy made this change, by my local testing, it doesn't matter if the adjustment bind is executed inside or outside of the audio thread... I can at least make it consistent across both methods, is it preferable that both are enqueue'd or not?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Comparing with AudioCollectionManager<T>
, binding adjustments there is enqueued to the audio thread, so might as well do the same here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done, both are now enqueue'd
By this, you mean the hierarchy of drawables, or something else? Because if you are referring to the drawable hierarchy I can conjure up several cases wherein tying audio interactions to drawables had undesirable results due to idiosyncrasies of the drawable hierarchy (masking, presence...) causing audio components tied to drawables to behave in an unexpected manner. I generally have lost most of belief that tying audio and drawables is correct. |
I mean the hierarchy of audio components, which is admittedly a bit different to the drawable one. |
Now that I think about it even more, with the context that this PR doesn't touch the drawable hierarchy at all ( Anyone want to take a shot at guessing where its |
To be truthful I have personally not been wanting to look at this PR as I kinda don't like the game-side feature that this is attempting to facilitate, I think it's not necessary / incurs too much complexity. If I'm alone on this / there's a consensus that we want that feature in some form, then I might try reading through this, but otherwise I kinda personally don't really care, am fine ignoring this, and would like to spend my time elsewhere. |
Thank you all for your comments. I believe I understand well enough of how the audio stack works, so here's my personal response to your concerns (please correct me if I'm wrong):
@smoogipoo By default, the volume of everything is 1. Previous to this PR, an adjustment was added to every track/sample, to respond to the music/effects settings volume sliders
In the case of
@bdach I believe this PR makes the whole system more flexible, meaning new mixers can be used whenever necessary in the game repository and be effective.
This is desired by some that are annoyed by the hitsounds but still want to hear the new UI sfx, myself included. This discussion and 1, 2 forum posts pushed me over the edge to open this PR |
Hit sound volume adjust is in my eyes one of the top 20 feature requests from the game over the years. The most common request is "I want to hear UI sounds but not hitsounds" which is something I personally disagree with, but the voice for this is present so I do want to provide it as an option. |
At face value, I'm seeing that the audio adjustments have transferred from the stores to the mixers, and that the mixers only accept I'm not sure if there are issues beyond that the I personally feel that |
Does this help explaining why I'm confused? I don't even know if this is the full view of things. What I mean is not "how does it get its volume" - I can answer that myself because I wrote half this thing, my concern is that it's difficult to follow and now (I believe) there are multiple branches because this isn't using the already tried and tested hierarchical relationship of components. It's injecting the mixer's volume directly into components willy-nilly. What happens when we support nested mixers? I think it'll look something like this: Unless I'm missing something. |
@smoogipoo I believe thinking about it in a more traditional sound mixing routing way should help. I will use |
The theory is fine, but that's not how o!f works to my understanding. Am I to understand that the diagram I've put above is not how o!f currently (not in the most perfect scenario) interacts as a result of this PR? I haven't tested it so it's possible I'm totally wrong. In my scenario, it means if you have a parent I don't disagree with the perfect scenario of a traditional DAW, but for that to work in o!f, things would need to be redesigned. The current structure is more like a DAW where everything's recursive. |
In your diagram, Is the |
There's actually two issues here...
diff --git a/osu.Framework/Graphics/Audio/DrawableAudioMixer.cs b/osu.Framework/Graphics/Audio/DrawableAudioMixer.cs
index 4e3c696ea..d558f73bb 100644
--- a/osu.Framework/Graphics/Audio/DrawableAudioMixer.cs
+++ b/osu.Framework/Graphics/Audio/DrawableAudioMixer.cs
@@ -20,6 +20,7 @@ public partial class DrawableAudioMixer : AudioContainer, IAudioMixer
private void load(AudioManager audio)
{
mixer = audio.CreateAudioMixer(Name);
+ mixer.BindAdjustments(this);
}
public void Add(IAudioChannel channel)
using NUnit.Framework;
using osu.Framework.Allocation;
using osu.Framework.Audio.Sample;
using osu.Framework.Graphics.Audio;
using osu.Framework.Tests.Visual;
namespace osu.Framework.Tests
{
public partial class TestSceneTesting : FrameworkTestScene
{
private DrawableSample sample = null!;
[BackgroundDependencyLoader]
private void load(ISampleStore samples)
{
Add(new DrawableAudioMixer
{
Volume = { Value = 0.5f },
Child = sample = new DrawableSample(samples.Get("long.mp3"))
});
}
[Test]
public void RunTest()
{
SampleChannel channel = null!;
AddStep("play channel", () =>
{
channel = sample.GetChannel();
channel.Looping = true;
channel.Play();
});
AddAssert("sample volume is 0.5", () => sample.AggregateVolume.Value, () => Is.EqualTo(0.5));
AddAssert("channel volume is 0.5", () => channel.AggregateVolume.Value, () => Is.EqualTo(0.5));
}
}
} That second assertion should pass, but the volume is actually 0.25 as I originally stated. |
No, that's the |
Working through this with @smoogipoo, here's a partial diff of a direction that might work better, but it's incomplete and by no means guaranteed to be correct / usable. Posting for further consideration diff --git a/osu.Framework.Tests/Audio/BassTestComponents.cs b/osu.Framework.Tests/Audio/BassTestComponents.cs
index 416b0ccbf..ea0387e07 100644
--- a/osu.Framework.Tests/Audio/BassTestComponents.cs
+++ b/osu.Framework.Tests/Audio/BassTestComponents.cs
@@ -34,8 +34,10 @@ public BassTestComponents(bool init = true)
Mixer = CreateMixer();
Resources = new DllResourceStore(typeof(TrackBassTest).Assembly);
- TrackStore = new TrackStore(Resources, Mixer);
- SampleStore = new SampleStore(Resources, Mixer);
+ TrackStore = new TrackStore(Resources);
+ SampleStore = new SampleStore(Resources);
+ Mixer.AddItem(TrackStore);
+ Mixer.AddItem(SampleStore);
Add(TrackStore, SampleStore);
}
diff --git a/osu.Framework/Audio/AudioManager.cs b/osu.Framework/Audio/AudioManager.cs
index 89e27b862..d87764f82 100644
--- a/osu.Framework/Audio/AudioManager.cs
+++ b/osu.Framework/Audio/AudioManager.cs
@@ -184,15 +184,15 @@ public AudioManager(AudioThread audioThread, ResourceStore<byte[]> trackStore, R
globalTrackStore = new Lazy<TrackStore>(() =>
{
- var store = new TrackStore(trackStore, TrackMixer);
- AddItem(store);
+ var store = new TrackStore(trackStore);
+ TrackMixer.AddItem(store);
return store;
});
globalSampleStore = new Lazy<SampleStore>(() =>
{
- var store = new SampleStore(sampleStore, SampleMixer);
- AddItem(store);
+ var store = new SampleStore(sampleStore);
+ SampleMixer.AddItem(store);
return store;
});
@@ -293,8 +293,8 @@ public ITrackStore GetTrackStore(IResourceStore<byte[]> store = null, AudioMixer
{
if (store == null) return globalTrackStore.Value;
- TrackStore tm = new TrackStore(store, mixer ?? TrackMixer);
- globalTrackStore.Value.AddItem(tm);
+ TrackStore tm = new TrackStore(store);
+ (mixer ?? TrackMixer).AddItem(tm);
return tm;
}
@@ -313,8 +313,8 @@ public ISampleStore GetSampleStore(IResourceStore<byte[]> store = null, AudioMix
{
if (store == null) return globalSampleStore.Value;
- SampleStore sm = new SampleStore(store, mixer ?? SampleMixer);
- globalSampleStore.Value.AddItem(sm);
+ SampleStore sm = new SampleStore(store);
+ (mixer ?? SampleMixer).AddItem(sm);
return sm;
}
diff --git a/osu.Framework/Audio/Mixing/AudioMixer.cs b/osu.Framework/Audio/Mixing/AudioMixer.cs
index da8a3c765..8ae388453 100644
--- a/osu.Framework/Audio/Mixing/AudioMixer.cs
+++ b/osu.Framework/Audio/Mixing/AudioMixer.cs
@@ -9,7 +9,7 @@ namespace osu.Framework.Audio.Mixing
/// <summary>
/// Mixes together multiple <see cref="IAudioChannel"/>s into one output.
/// </summary>
- public abstract class AudioMixer : AdjustableAudioComponent, IAudioMixer
+ public abstract class AudioMixer : AudioCollectionManager<AudioComponent>, IAudioMixer
{
public readonly string Identifier;
@@ -27,9 +27,11 @@ protected AudioMixer(AudioMixer? fallbackMixer, string identifier)
Identifier = identifier;
}
- public void Add(IAudioChannel channel)
+ protected override void ItemAdded(AudioComponent item)
{
- channel.EnqueueAction(() =>
+ base.ItemAdded(item);
+
+ if (item is IAudioChannel channel)
{
if (channel.Mixer == this)
return;
@@ -43,10 +45,15 @@ public void Add(IAudioChannel channel)
AddInternal(channel);
channel.Mixer = this;
- });
+ }
}
- public void Remove(IAudioChannel channel) => Remove(channel, true);
+ protected override void ItemRemoved(AudioComponent item)
+ {
+ base.ItemRemoved(item);
+ if (item is IAudioChannel channel)
+ Remove(channel, true);
+ }
public abstract void AddEffect(IEffectParameter effect, int priority = 0);
@@ -77,8 +84,8 @@ protected void Remove(IAudioChannel channel, bool returnToDefault)
adj.UnbindAdjustments(this);
// Add the channel back to the default mixer so audio can always be played.
- if (returnToDefault)
- fallbackMixer.AsNonNull().Add(channel);
+ if (returnToDefault && channel is AudioComponent comp)
+ fallbackMixer.AsNonNull().AddItem(comp);
});
}
diff --git a/osu.Framework/Audio/Sample/SampleBass.cs b/osu.Framework/Audio/Sample/SampleBass.cs
index 5052a2cd5..9487b6270 100644
--- a/osu.Framework/Audio/Sample/SampleBass.cs
+++ b/osu.Framework/Audio/Sample/SampleBass.cs
@@ -1,8 +1,6 @@
// Copyright (c) ppy Pty Ltd <[email protected]>. Licensed under the MIT Licence.
// See the LICENCE file in the repository root for full licence text.
-using osu.Framework.Audio.Mixing.Bass;
-
namespace osu.Framework.Audio.Sample
{
internal sealed class SampleBass : Sample
@@ -12,13 +10,11 @@ internal sealed class SampleBass : Sample
public override bool IsLoaded => factory.IsLoaded;
private readonly SampleBassFactory factory;
- private readonly BassAudioMixer mixer;
- internal SampleBass(SampleBassFactory factory, BassAudioMixer mixer)
+ internal SampleBass(SampleBassFactory factory)
: base(factory.Name)
{
this.factory = factory;
- this.mixer = mixer;
PlaybackConcurrency.BindTo(factory.PlaybackConcurrency);
}
@@ -26,7 +22,7 @@ internal SampleBass(SampleBassFactory factory, BassAudioMixer mixer)
protected override SampleChannel CreateChannel()
{
var channel = new SampleChannelBass(this);
- mixer.Add(channel);
+ AddItem(channel);
return channel;
}
}
diff --git a/osu.Framework/Audio/Sample/SampleBassFactory.cs b/osu.Framework/Audio/Sample/SampleBassFactory.cs
index 3b863a68b..695975c49 100644
--- a/osu.Framework/Audio/Sample/SampleBassFactory.cs
+++ b/osu.Framework/Audio/Sample/SampleBassFactory.cs
@@ -5,7 +5,6 @@
using System.Runtime.InteropServices;
using ManagedBass;
using osu.Framework.Allocation;
-using osu.Framework.Audio.Mixing.Bass;
using osu.Framework.Bindables;
using osu.Framework.Platform;
@@ -32,15 +31,12 @@ internal class SampleBassFactory : AudioCollectionManager<AdjustableAudioCompone
/// </summary>
internal readonly Bindable<int> PlaybackConcurrency = new Bindable<int>(Sample.DEFAULT_CONCURRENCY);
- private readonly BassAudioMixer mixer;
-
private NativeMemoryTracker.NativeMemoryLease? memoryLease;
private byte[]? data;
- public SampleBassFactory(byte[] data, string name, BassAudioMixer mixer)
+ public SampleBassFactory(byte[] data, string name)
{
this.data = data;
- this.mixer = mixer;
Name = name;
@@ -97,7 +93,7 @@ private void loadSample()
memoryLease = NativeMemoryTracker.AddMemory(this, dataLength);
}
- public Sample CreateSample() => new SampleBass(this, mixer) { OnPlay = onPlay };
+ public Sample CreateSample() => new SampleBass(this) { OnPlay = onPlay };
private void onPlay(Sample sample)
{
diff --git a/osu.Framework/Audio/Sample/SampleStore.cs b/osu.Framework/Audio/Sample/SampleStore.cs
index a47a9f15a..298fd03bc 100644
--- a/osu.Framework/Audio/Sample/SampleStore.cs
+++ b/osu.Framework/Audio/Sample/SampleStore.cs
@@ -9,8 +9,6 @@
using System.Threading;
using System.Threading.Tasks;
using JetBrains.Annotations;
-using osu.Framework.Audio.Mixing;
-using osu.Framework.Audio.Mixing.Bass;
using osu.Framework.IO.Stores;
using osu.Framework.Statistics;
@@ -19,16 +17,14 @@ namespace osu.Framework.Audio.Sample
internal class SampleStore : AudioCollectionManager<AdjustableAudioComponent>, ISampleStore
{
private readonly ResourceStore<byte[]> store;
- private readonly AudioMixer mixer;
private readonly Dictionary<string, SampleBassFactory> factories = new Dictionary<string, SampleBassFactory>();
public int PlaybackConcurrency { get; set; } = Sample.DEFAULT_CONCURRENCY;
- internal SampleStore([NotNull] IResourceStore<byte[]> store, [NotNull] AudioMixer mixer)
+ internal SampleStore([NotNull] IResourceStore<byte[]> store)
{
this.store = new ResourceStore<byte[]>(store);
- this.mixer = mixer;
AddExtension(@"wav");
AddExtension(@"mp3");
@@ -49,7 +45,7 @@ public Sample Get(string name)
this.LogIfNonBackgroundThread(name);
byte[] data = store.Get(name);
- factory = factories[name] = data == null ? null : new SampleBassFactory(data, name, (BassAudioMixer)mixer) { PlaybackConcurrency = { Value = PlaybackConcurrency } };
+ factory = factories[name] = data == null ? null : new SampleBassFactory(data, name) { PlaybackConcurrency = { Value = PlaybackConcurrency } };
if (factory != null)
AddItem(factory);
diff --git a/osu.Framework/Audio/Track/TrackStore.cs b/osu.Framework/Audio/Track/TrackStore.cs
index 495e3f071..6666c6e9d 100644
--- a/osu.Framework/Audio/Track/TrackStore.cs
+++ b/osu.Framework/Audio/Track/TrackStore.cs
@@ -9,7 +9,6 @@
using System.Threading;
using System.Threading.Tasks;
using JetBrains.Annotations;
-using osu.Framework.Audio.Mixing;
using osu.Framework.IO.Stores;
namespace osu.Framework.Audio.Track
@@ -17,12 +16,10 @@ namespace osu.Framework.Audio.Track
internal class TrackStore : AudioCollectionManager<AdjustableAudioComponent>, ITrackStore
{
private readonly IResourceStore<byte[]> store;
- private readonly AudioMixer mixer;
- internal TrackStore([NotNull] IResourceStore<byte[]> store, [NotNull] AudioMixer mixer)
+ internal TrackStore([NotNull] IResourceStore<byte[]> store)
{
this.store = store;
- this.mixer = mixer;
(store as ResourceStore<byte[]>)?.AddExtension(@"mp3");
}
@@ -49,7 +46,6 @@ public Track Get(string name)
TrackBass trackBass = new TrackBass(dataStream, name);
- mixer.Add(trackBass);
AddItem(trackBass);
return trackBass; |
I think I'd be alright with this PR if we removed We were discussing just using BDL for audio, however manual that is... [Resolved]
private IAudioMixer mixer { get; set; } = null!;
[Resolved]
private ISampleStore samples { get; set; } = null!;
[Resolved]
private AudioManager audio { get; set; } = null!;
private void doSomething()
{
// Play a sample...
var sample = samples.Get(...);
mixer.Add(sample);
sample.GetChannel().Play();
// Create a mixer...
var ourMixer = audio.CreateMixer();
mixer.Add(ourMixer); // Nested mixers, or you can play samples on ourMixer directly...
// We can cache the mixer and use BDL to handle overrides.
// How do we handle the track vs sample mixer with this structure? We'll need to figure that one out!
} Maybe.... Or perhaps something like The other way is to remove the audio hierarchy and force using the drawable components, which was the path I first thought of. My reasoning being is historically we've had issues in osu!stable where samples were kept active in the background because some component somewhere didn't stop a sample, and having them be automatically managed might resolve some of those issues (though it's also been brought up that drawable disposal is async sooooooo...). |
Hello everyone! Messing with the Audio stack is beyond what I can do and I believe it's better left to you, more experienced devs. That said, I really want to have the hitsound mute/volume feature merged. I want to kindly ask the reviewers if any of the following options would be acceptable right now:
As always, thank you! |
Please be patient and wait for an outcome. While you may want this, it's just a drop in the bucket of things we're aiming to get done. I don't believe we need to take shortcuts or make temporary hacks to expedite this feature as people have lived without it for decades. |
Pair of ppy/osu#28733
This change decouples the volume settings from the track/sample store and instead link it with the specific mixer, allowing other mixes to control their own volume.