Replace audio effect BindableList by Add/Remove methods

[smoogipoo]

The BindableList adds complexity when you want to update the filter that I don't think is justified.

We've also recently disabled effect re-prioritisation due to it causing segfaults, so I don't foresee ever really needing that ability either, at least not in a way that it's a common operation. Therefore I think it's best to just move to a simpler state of being.

osu!-side diff (note that I've also removed the invalidation because I don't think it's necessary):

diff --git a/osu.Game/Audio/Effects/AudioFilter.cs b/osu.Game/Audio/Effects/AudioFilter.cs
index bfa9b31242..8db457ae67 100644
--- a/osu.Game/Audio/Effects/AudioFilter.cs
+++ b/osu.Game/Audio/Effects/AudioFilter.cs
@@ -1,10 +1,8 @@
 // Copyright (c) ppy Pty Ltd <contact@ppy.sh>. Licensed under the MIT Licence.
 // See the LICENCE file in the repository root for full licence text.
 
-using System.Diagnostics;
 using ManagedBass.Fx;
 using osu.Framework.Audio.Mixing;
-using osu.Framework.Caching;
 using osu.Framework.Graphics;
 
 namespace osu.Game.Audio.Effects
@@ -26,8 +24,6 @@ public partial class AudioFilter : Component, ITransformableFilter
         private readonly BQFParameters filter;
         private readonly BQFType type;
 
-        private readonly Cached filterApplication = new Cached();
-
         private int cutoff;
 
         /// <summary>
@@ -42,7 +38,7 @@ public int Cutoff
                     return;
 
                 cutoff = value;
-                filterApplication.Invalidate();
+                updateFilter();
             }
         }
 
@@ -64,18 +60,9 @@ public AudioFilter(AudioMixer mixer, BQFType type = BQFType.LowPass)
                 fQ = 0.7f
             };
 
-            Cutoff = getInitialCutoff(type);
-        }
-
-        protected override void Update()
-        {
-            base.Update();
+            cutoff = getInitialCutoff(type);
 
-            if (!filterApplication.IsValid)
-            {
-                updateFilter(cutoff);
-                filterApplication.Validate();
-            }
+            updateFilter();
         }
 
         private int getInitialCutoff(BQFType type)
@@ -93,13 +80,13 @@ private int getInitialCutoff(BQFType type)
             }
         }
 
-        private void updateFilter(int newValue)
+        private void updateFilter()
         {
             switch (type)
             {
                 case BQFType.LowPass:
                     // Workaround for weird behaviour when rapidly setting fCenter of a low-pass filter to nyquist - 1hz.
-                    if (newValue >= MAX_LOWPASS_CUTOFF)
+                    if (Cutoff >= MAX_LOWPASS_CUTOFF)
                     {
                         ensureDetached();
                         return;
@@ -109,7 +96,7 @@ private void updateFilter(int newValue)
 
                 // Workaround for weird behaviour when rapidly setting fCenter of a high-pass filter to 1hz.
                 case BQFType.HighPass:
-                    if (newValue <= 1)
+                    if (Cutoff <= 1)
                     {
                         ensureDetached();
                         return;
@@ -120,17 +107,8 @@ private void updateFilter(int newValue)
 
             ensureAttached();
 
-            int filterIndex = mixer.Effects.IndexOf(filter);
-
-            if (filterIndex < 0) return;
-
-            if (mixer.Effects[filterIndex] is BQFParameters existingFilter)
-            {
-                existingFilter.fCenter = newValue;
-
-                // required to update effect with new parameters.
-                mixer.Effects[filterIndex] = existingFilter;
-            }
+            filter.fCenter = Cutoff;
+            mixer.UpdateEffect(filter);
         }
 
         private void ensureAttached()
@@ -138,8 +116,7 @@ private void ensureAttached()
             if (IsAttached)
                 return;
 
-            Debug.Assert(!mixer.Effects.Contains(filter));
-            mixer.Effects.Add(filter);
+            mixer.AddEffect(filter);
             IsAttached = true;
         }
 
@@ -148,8 +125,7 @@ private void ensureDetached()
             if (!IsAttached)
                 return;
 
-            Debug.Assert(mixer.Effects.Contains(filter));
-            mixer.Effects.Remove(filter);
+            mixer.RemoveEffect(filter);
             IsAttached = false;
         }
 

[bdach]

Why does the new code not attempt to handle UpdateEffect() calls that change the effect type entirely in the same way? Does it not need to? Was this tested to work in such a scenario? (I only checked docs, but I can't see any mention of whether this is going to work or not).

[smoogipoo]

It doesn't support changing the FX type anymore. You can't even do that because UpdateEffect() uses an IEffectParameters reference.

In my first commit, the one that had a wrapping AudioEffect class, the implementation also threw an exception if that was attempted, so I'm very much of the mindset that this shouldn't be a thing and may only be supported by the user doing this themselves.

Fwiw, changing the FX type is a "non-atomic" operation anyway - it needs to remove the previous filter and add the new one which means there's a small amount of time during which sound could be playing with no filter attached. As well, doing this means that priority gets kind of funky.

[peppy]

I'm OK with this.