Merge pull request #32593 from peppy/less-sample-overhead-during-silly-sliders

Fix stutters when sliders with hundreds of repeats display for the first time

osu.Game.Rulesets.Osu.Tests/TestSceneSpinner.cs

@@ -86,9 +86,12 @@ namespace osu.Game.Rulesets.Osu.Tests
         [Test]
         public void TestSpinningSamplePitchShift()
         {
+            PausableSkinnableSound spinSample = null;
+
             AddStep("Add spinner", () => SetContents(_ => testSingle(5, true, 4000)));
-            AddUntilStep("Pitch starts low", () => getSpinningSample().Frequency.Value < 0.8);
-            AddUntilStep("Pitch increases", () => getSpinningSample().Frequency.Value > 0.8);
+            AddUntilStep("wait for spin sample", () => (spinSample = getSpinningSample()) != null);
+            AddUntilStep("Pitch starts low", () => spinSample.Frequency.Value < 0.8);
+            AddUntilStep("Pitch increases", () => spinSample.Frequency.Value > 0.8);
 
             PausableSkinnableSound getSpinningSample() =>
                 drawableSpinner.ChildrenOfType<PausableSkinnableSound>().FirstOrDefault(s => s.Samples.Any(i => i.LookupNames.Any(l => l.Contains("spinnerspin"))));

osu.Game.Tests/Visual/Editing/TestSceneEditorSamplePlayback.cs

@@ -24,12 +24,7 @@ namespace osu.Game.Tests.Visual.Editing
             PoolableSkinnableSample[] loopingSamples = null;
             PoolableSkinnableSample[] onceOffSamples = null;
 
-            AddStep("get first slider", () =>
-            {
-                slider = Editor.ChildrenOfType<DrawableSlider>().OrderBy(s => s.HitObject.StartTime).First();
-                onceOffSamples = slider.ChildrenOfType<PoolableSkinnableSample>().Where(s => !s.Looping).ToArray();
-                loopingSamples = slider.ChildrenOfType<PoolableSkinnableSample>().Where(s => s.Looping).ToArray();
-            });
+            AddStep("get first slider", () => slider = Editor.ChildrenOfType<DrawableSlider>().OrderBy(s => s.HitObject.StartTime).First());
 
             AddStep("start playback", () => EditorClock.Start());
 
@@ -38,6 +33,9 @@ namespace osu.Game.Tests.Visual.Editing
                 if (!slider.Tracking.Value)
                     return false;
 
+                onceOffSamples = slider.ChildrenOfType<PoolableSkinnableSample>().Where(s => !s.Looping).ToArray();
+                loopingSamples = slider.ChildrenOfType<PoolableSkinnableSample>().Where(s => s.Looping).ToArray();
+
                 if (!loopingSamples.Any(s => s.Playing))
                     return false;
 

osu.Game/Rulesets/Objects/Drawables/DrawableHitObject.cs

@@ -5,7 +5,6 @@
 
 using System;
 using System.Collections.Generic;
-using System.Collections.Specialized;
 using System.Diagnostics;
 using System.Linq;
 using JetBrains.Annotations;
@@ -16,7 +15,6 @@ using osu.Framework.Extensions.ObjectExtensions;
 using osu.Framework.Extensions.TypeExtensions;
 using osu.Framework.Graphics;
 using osu.Framework.Lists;
-using osu.Framework.Threading;
 using osu.Framework.Utils;
 using osu.Game.Audio;
 using osu.Game.Configuration;
@@ -63,6 +61,8 @@ namespace osu.Game.Rulesets.Objects.Drawables
 
         protected PausableSkinnableSound Samples { get; private set; }
 
+        private bool samplesLoaded;
+
         public virtual IEnumerable<HitSampleInfo> GetSamples() => HitObject.Samples;
 
         private readonly List<DrawableHitObject> nestedHitObjects = new List<DrawableHitObject>();
@@ -227,6 +227,12 @@ namespace osu.Game.Rulesets.Objects.Drawables
 
             comboColourBrightness.BindValueChanged(_ => UpdateComboColour());
 
+            samplesBindable.BindCollectionChanged((_, _) =>
+            {
+                if (samplesLoaded)
+                    LoadSamples();
+            });
+
             // Apply transforms
             updateStateFromResult();
         }
@@ -293,8 +299,6 @@ namespace osu.Game.Rulesets.Objects.Drawables
             }
 
             samplesBindable.BindTo(HitObject.SamplesBindable);
-            samplesBindable.BindCollectionChanged(onSamplesChanged, true);
-
             HitObject.DefaultsApplied += onDefaultsApplied;
 
             OnApply();
@@ -335,11 +339,8 @@ namespace osu.Game.Rulesets.Objects.Drawables
 
             samplesBindable.UnbindFrom(HitObject.SamplesBindable);
 
-            // When a new hitobject is applied, the samples will be cleared before re-populating.
-            // In order to stop this needless update, the event is unbound and re-bound as late as possible in Apply().
-            samplesBindable.CollectionChanged -= onSamplesChanged;
-
             // Release the samples for other hitobjects to use.
+            samplesLoaded = false;
             Samples?.ClearSamples();
 
             foreach (var obj in nestedHitObjects)
@@ -396,8 +397,6 @@ namespace osu.Game.Rulesets.Objects.Drawables
             Samples.Samples = samples.Cast<ISampleInfo>().ToArray();
         }
 
-        private void onSamplesChanged(object sender, NotifyCollectionChangedEventArgs e) => LoadSamples();
-
         private void onNewResult(DrawableHitObject drawableHitObject, JudgementResult result) => OnNewResult?.Invoke(drawableHitObject, result);
 
         private void onRevertResult()
@@ -631,6 +630,33 @@ namespace osu.Game.Rulesets.Objects.Drawables
 
         #endregion
 
+        protected override void Update()
+        {
+            // We use a flag here to load samples only when they are required to be played.
+            // Why in Update and not PlaySamples? Because some hit object implementations may expect LoadSamples to be called to load custom samples
+            // (slider slide sound as an example).
+            //
+            // This is best effort optimisation (over previous method of loading and de-pooling in `OnApply`) due to requiring knowledge of
+            // hitobjects' metadata. For cases like sliders with many repeats, there can be a sudden request to de-pool (ie slider with many repeats)
+            // hundreds of samples, causing a gameplay stutter.
+            //
+            // Note that we already have optimisations in OsuPlayfield for this but it applies to DrawableHitObjects and not samples.
+            //
+            // This is definitely not the end of optimisation of sample loading, but the structure of gameplay samples is going to take some
+            // time to dismantle and optimise. Optimally:
+            //
+            // - we would want to remove as much of the drawable overheads from samples as possible (currently two drawables per sample worst case)
+            // - pool the rawest representation of samples possible (if required at that point).
+            // - infer metadata at beatmap load to asynchronously preload the samples (into memory / bass).
+            if (!samplesLoaded)
+            {
+                samplesLoaded = true;
+                LoadSamples();
+            }
+
+            base.Update();
+        }
+
         public override bool UpdateSubTreeMasking() => false;
 
         protected override void UpdateAfterChildren()
@@ -640,14 +666,6 @@ namespace osu.Game.Rulesets.Objects.Drawables
             UpdateResult(false);
         }
 
-        /// <summary>
-        /// Schedules an <see cref="Action"/> to this <see cref="DrawableHitObject"/>.
-        /// </summary>
-        /// <remarks>
-        /// Only provided temporarily until hitobject pooling is implemented.
-        /// </remarks>
-        protected internal new ScheduledDelegate Schedule(Action action) => base.Schedule(action);
-
         /// <summary>
         /// An offset prior to the start time of <see cref="HitObject"/> at which this <see cref="DrawableHitObject"/> may begin displaying contents.
         /// By default, <see cref="DrawableHitObject"/>s are assumed to display their contents within 10 seconds prior to the start time of <see cref="HitObject"/>.