Merge pull request #22659 from peppy/fix-taiko-drum-nested-sample-detection

Fix `GameplaySampleTriggerSource` not considering nested objects when determining the best sample to play

osu.Game.Tests/Visual/Gameplay/TestSceneGameplaySampleTriggerSource.cs

@@ -7,16 +7,20 @@ using NUnit.Framework;
 using osu.Framework.Allocation;
 using osu.Framework.Audio;
 using osu.Framework.Timing;
+using osu.Framework.Utils;
 using osu.Game.Audio;
 using osu.Game.Beatmaps;
 using osu.Game.Beatmaps.ControlPoints;
+using osu.Game.Beatmaps.Legacy;
 using osu.Game.Rulesets;
 using osu.Game.Rulesets.Objects;
 using osu.Game.Rulesets.Objects.Drawables;
+using osu.Game.Rulesets.Objects.Types;
 using osu.Game.Rulesets.Osu;
 using osu.Game.Rulesets.Osu.Objects;
 using osu.Game.Rulesets.UI;
 using osu.Game.Storyboards;
+using osuTK;
 using osuTK.Input;
 
 namespace osu.Game.Tests.Visual.Gameplay
@@ -36,13 +40,16 @@ namespace osu.Game.Tests.Visual.Gameplay
 
         protected override IBeatmap CreateBeatmap(RulesetInfo ruleset)
         {
+            ControlPointInfo controlPointInfo = new LegacyControlPointInfo();
+
             beatmap = new Beatmap
             {
                 BeatmapInfo = new BeatmapInfo
                 {
                     Difficulty = new BeatmapDifficulty { CircleSize = 6, SliderMultiplier = 3 },
                     Ruleset = ruleset
-                }
+                },
+                ControlPointInfo = controlPointInfo
             };
 
             const double start_offset = 8000;
@@ -51,7 +58,7 @@ namespace osu.Game.Tests.Visual.Gameplay
             // intentionally start objects a bit late so we can test the case of no alive objects.
             double t = start_offset;
 
-            beatmap.HitObjects.AddRange(new[]
+            beatmap.HitObjects.AddRange(new HitObject[]
             {
                 new HitCircle
                 {
@@ -71,12 +78,24 @@ namespace osu.Game.Tests.Visual.Gameplay
                 },
                 new HitCircle
                 {
-                    StartTime = t + spacing,
+                    StartTime = t += spacing,
+                },
+                new Slider
+                {
+                    StartTime = t += spacing,
+                    Path = new SliderPath(PathType.Linear, new[] { Vector2.Zero, Vector2.UnitY * 200 }),
                     Samples = new[] { new HitSampleInfo(HitSampleInfo.HIT_WHISTLE) },
                     SampleControlPoint = new SampleControlPoint { SampleBank = "soft" },
                 },
             });
 
+            // Add a change in volume halfway through final slider.
+            controlPointInfo.Add(t, new SampleControlPoint
+            {
+                SampleBank = "normal",
+                SampleVolume = 20,
+            });
+
             return beatmap;
         }
 
@@ -129,14 +148,36 @@ namespace osu.Game.Tests.Visual.Gameplay
             waitForAliveObjectIndex(3);
             checkValidObjectIndex(3);
 
-            AddStep("Seek into future", () => Beatmap.Value.Track.Seek(beatmap.HitObjects.Last().GetEndTime() + 10000));
+            seekBeforeIndex(4);
+            waitForAliveObjectIndex(4);
 
+            // Even before the object, we should prefer the first nested object's sample.
+            // This is because the (parent) object will only play its sample at the final EndTime.
+            AddAssert("check valid object is slider's first nested", () => sampleTriggerSource.GetMostValidObject(), () => Is.EqualTo(beatmap.HitObjects[4].NestedHitObjects.First()));
+
+            AddStep("seek to just before slider ends", () => Player.GameplayClockContainer.Seek(beatmap.HitObjects[4].GetEndTime() - 100));
+            waitForCatchUp();
+            AddUntilStep("wait until valid object is slider's last nested", () => sampleTriggerSource.GetMostValidObject(), () => Is.EqualTo(beatmap.HitObjects[4].NestedHitObjects.Last()));
+
+            // After we get far enough away, the samples of the object itself should be used, not any nested object.
+            AddStep("seek to further after slider", () => Player.GameplayClockContainer.Seek(beatmap.HitObjects[4].GetEndTime() + 1000));
+            waitForCatchUp();
+            AddUntilStep("wait until valid object is slider itself", () => sampleTriggerSource.GetMostValidObject(), () => Is.EqualTo(beatmap.HitObjects[4]));
+
+            AddStep("Seek into future", () => Player.GameplayClockContainer.Seek(beatmap.HitObjects.Last().GetEndTime() + 10000));
+            waitForCatchUp();
             waitForAliveObjectIndex(null);
-            checkValidObjectIndex(3);
+            checkValidObjectIndex(4);
         }
 
-        private void seekBeforeIndex(int index) =>
+        private void seekBeforeIndex(int index)
-            AddStep($"seek to just before object {index}", () => Beatmap.Value.Track.Seek(beatmap.HitObjects[index].StartTime - 100));
+        {
+            AddStep($"seek to just before object {index}", () => Player.GameplayClockContainer.Seek(beatmap.HitObjects[index].StartTime - 100));
+            waitForCatchUp();
+        }
+
+        private void waitForCatchUp() =>
+            AddUntilStep("wait for frame stable clock to catch up", () => Precision.AlmostEquals(Player.GameplayClockContainer.CurrentTime, Player.DrawableRuleset.FrameStableClock.CurrentTime));
 
         private void waitForAliveObjectIndex(int? index)
         {

osu.Game/Rulesets/UI/GameplaySampleTriggerSource.cs

@@ -7,6 +7,7 @@ using System.Linq;
 using osu.Framework.Graphics.Containers;
 using osu.Game.Audio;
 using osu.Game.Rulesets.Objects;
+using osu.Game.Rulesets.Objects.Drawables;
 using osu.Game.Skinning;
 
 namespace osu.Game.Rulesets.UI
@@ -68,27 +69,61 @@ namespace osu.Game.Rulesets.UI
         protected HitObject GetMostValidObject()
         {
             // The most optimal lookup case we have is when an object is alive. There are usually very few alive objects so there's no drawbacks in attempting this lookup each time.
-            var hitObject = hitObjectContainer.AliveObjects.FirstOrDefault(h => h.Result?.HasResult != true)?.HitObject;
+            var drawableHitObject = hitObjectContainer.AliveObjects.FirstOrDefault(h => h.Result?.HasResult != true);
 
-            // In the case a next object isn't available in drawable form, we need to do a somewhat expensive traversal to get a valid sound to play.
+            if (drawableHitObject != null)
-            if (hitObject == null)
             {
-                // This lookup can be skipped if the last entry is still valid (in the future and not yet hit).
+                // A hit object may have a more valid nested object.
-                if (fallbackObject == null || fallbackObject.Result?.HasResult == true)
+                drawableHitObject = getMostValidNestedDrawable(drawableHitObject);
-                {
-                    // We need to use lifetime entries to find the next object (we can't just use `hitObjectContainer.Objects` due to pooling - it may even be empty).
-                    // If required, we can make this lookup more efficient by adding support to get next-future-entry in LifetimeEntryManager.
-                    fallbackObject = hitObjectContainer.Entries
-                                                       .Where(e => e.Result?.HasResult != true).MinBy(e => e.HitObject.StartTime);
 
-                    // In the case there are no unjudged objects, the last hit object should be used instead.
+                return drawableHitObject.HitObject;
-                    fallbackObject ??= hitObjectContainer.Entries.LastOrDefault();
-                }
-
-                hitObject = fallbackObject?.HitObject;
             }
 
-            return hitObject;
+            // In the case a next object isn't available in drawable form, we need to do a somewhat expensive traversal to get a valid sound to play.
+            // This lookup can be skipped if the last entry is still valid (in the future and not yet hit).
+            if (fallbackObject == null || fallbackObject.Result?.HasResult == true)
+            {
+                // We need to use lifetime entries to find the next object (we can't just use `hitObjectContainer.Objects` due to pooling - it may even be empty).
+                // If required, we can make this lookup more efficient by adding support to get next-future-entry in LifetimeEntryManager.
+                fallbackObject = hitObjectContainer.Entries
+                                                   .Where(e => e.Result?.HasResult != true).MinBy(e => e.HitObject.StartTime);
+
+                if (fallbackObject != null)
+                    return getEarliestNestedObject(fallbackObject.HitObject);
+
+                // In the case there are no non-judged objects, the last hit object should be used instead.
+                fallbackObject ??= hitObjectContainer.Entries.LastOrDefault();
+            }
+
+            if (fallbackObject == null)
+                return null;
+
+            bool fallbackHasResult = fallbackObject.Result?.HasResult == true;
+
+            // If the fallback has been judged then we want the sample from the object itself.
+            if (fallbackHasResult)
+                return fallbackObject.HitObject;
+
+            // Else we want the earliest (including nested).
+            // In cases of nested objects, they will always have earlier sample data than their parent object.
+            return getEarliestNestedObject(fallbackObject.HitObject);
+        }
+
+        private DrawableHitObject getMostValidNestedDrawable(DrawableHitObject o)
+        {
+            var nestedWithoutResult = o.NestedHitObjects.FirstOrDefault(n => n.Result?.HasResult != true);
+
+            if (nestedWithoutResult == null)
+                return o;
+
+            return getMostValidNestedDrawable(nestedWithoutResult);
+        }
+
+        private HitObject getEarliestNestedObject(HitObject hitObject)
+        {
+            var nested = hitObject.NestedHitObjects.FirstOrDefault();
+
+            return nested != null ? getEarliestNestedObject(nested) : hitObject;
         }
 
         private SkinnableSound getNextSample()