From b59ec551f6179d204cf3ee14219e8fe101e2844e Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Thu, 16 Feb 2023 18:13:11 +0900 Subject: [PATCH 1/7] Add test coverage of `GameplaySampleTriggerSource` not considering nested objects --- .../TestSceneGameplaySampleTriggerSource.cs | 41 ++++++++++++++++--- 1 file changed, 36 insertions(+), 5 deletions(-) diff --git a/osu.Game.Tests/Visual/Gameplay/TestSceneGameplaySampleTriggerSource.cs b/osu.Game.Tests/Visual/Gameplay/TestSceneGameplaySampleTriggerSource.cs index e7bdf7b9ba..86dfce438a 100644 --- a/osu.Game.Tests/Visual/Gameplay/TestSceneGameplaySampleTriggerSource.cs +++ b/osu.Game.Tests/Visual/Gameplay/TestSceneGameplaySampleTriggerSource.cs @@ -10,13 +10,16 @@ using osu.Framework.Timing; 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 +39,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 +57,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 +77,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; } @@ -128,10 +146,23 @@ 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 after slider", () => Beatmap.Value.Track.Seek(beatmap.HitObjects[4].GetEndTime() + 100)); + 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", () => Beatmap.Value.Track.Seek(beatmap.HitObjects[4].GetEndTime() + 1000)); + AddUntilStep("wait until valid object is slider itself", () => sampleTriggerSource.GetMostValidObject(), () => Is.EqualTo(beatmap.HitObjects[4])); + + AddStep("Seek into future", () => Beatmap.Value.Track.Seek(beatmap.HitObjects.Last().GetEndTime() + 10000)); waitForAliveObjectIndex(null); - checkValidObjectIndex(3); + checkValidObjectIndex(4); } private void seekBeforeIndex(int index) => From affa9507a1549e0d7393cd244ebc5b1b5195c1b7 Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Thu, 16 Feb 2023 17:34:38 +0900 Subject: [PATCH 2/7] Fix `GameplaySampleTriggerSource` not considering nested objects when determining the best sample to play --- .../UI/GameplaySampleTriggerSource.cs | 67 ++++++++++++++----- 1 file changed, 51 insertions(+), 16 deletions(-) diff --git a/osu.Game/Rulesets/UI/GameplaySampleTriggerSource.cs b/osu.Game/Rulesets/UI/GameplaySampleTriggerSource.cs index d2244df3b8..de16cc05c7 100644 --- a/osu.Game/Rulesets/UI/GameplaySampleTriggerSource.cs +++ b/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 (hitObject == null) + if (drawableHitObject != null) { - // 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); + // A hit object may have a more valid nested object. + drawableHitObject = getMostValidNestedDrawable(drawableHitObject); - // In the case there are no unjudged objects, the last hit object should be used instead. - fallbackObject ??= hitObjectContainer.Entries.LastOrDefault(); - } - - hitObject = fallbackObject?.HitObject; + return drawableHitObject.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 fallbackObject.HitObject; + + // In the case there are no unjudged 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() From 19d5293ad1f69a483a7d87df7feecad425589344 Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Fri, 17 Feb 2023 18:59:31 +0900 Subject: [PATCH 3/7] Change early return to also find the earliest nested object --- osu.Game/Rulesets/UI/GameplaySampleTriggerSource.cs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/osu.Game/Rulesets/UI/GameplaySampleTriggerSource.cs b/osu.Game/Rulesets/UI/GameplaySampleTriggerSource.cs index de16cc05c7..e1c03e49e3 100644 --- a/osu.Game/Rulesets/UI/GameplaySampleTriggerSource.cs +++ b/osu.Game/Rulesets/UI/GameplaySampleTriggerSource.cs @@ -89,9 +89,9 @@ namespace osu.Game.Rulesets.UI .Where(e => e.Result?.HasResult != true).MinBy(e => e.HitObject.StartTime); if (fallbackObject != null) - return fallbackObject.HitObject; + return getEarliestNestedObject(fallbackObject.HitObject); - // In the case there are no unjudged objects, the last hit object should be used instead. + // In the case there are no non-judged objects, the last hit object should be used instead. fallbackObject ??= hitObjectContainer.Entries.LastOrDefault(); } From e686b4393ef9b3c6d9a27636a824a9bb8c76f0d4 Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Tue, 21 Feb 2023 14:04:19 +0900 Subject: [PATCH 4/7] Add wait steps to ensure frame-stable clock has caught up before checking state --- .../Gameplay/TestSceneGameplaySampleTriggerSource.cs | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/osu.Game.Tests/Visual/Gameplay/TestSceneGameplaySampleTriggerSource.cs b/osu.Game.Tests/Visual/Gameplay/TestSceneGameplaySampleTriggerSource.cs index f9f5581b43..f7641c0cc9 100644 --- a/osu.Game.Tests/Visual/Gameplay/TestSceneGameplaySampleTriggerSource.cs +++ b/osu.Game.Tests/Visual/Gameplay/TestSceneGameplaySampleTriggerSource.cs @@ -7,6 +7,7 @@ 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; @@ -155,19 +156,28 @@ namespace osu.Game.Tests.Visual.Gameplay AddAssert("check valid object is slider's first nested", () => sampleTriggerSource.GetMostValidObject(), () => Is.EqualTo(beatmap.HitObjects[4].NestedHitObjects.First())); AddStep("seek to just after slider", () => Beatmap.Value.Track.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", () => Beatmap.Value.Track.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", () => Beatmap.Value.Track.Seek(beatmap.HitObjects.Last().GetEndTime() + 10000)); + waitForCatchUp(); waitForAliveObjectIndex(null); 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)); + waitForCatchUp(); + } + + private void waitForCatchUp() => + AddUntilStep("wait for frame stable clock to catch up", () => Precision.AlmostEquals(Beatmap.Value.Track.CurrentTime, Player.DrawableRuleset.FrameStableClock.CurrentTime)); private void waitForAliveObjectIndex(int? index) { From 9321ec29dc942b109f885c9d0b34302d581f7998 Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Tue, 21 Feb 2023 14:04:37 +0900 Subject: [PATCH 5/7] Update slider sample source asserts to match expected behaviour As pointed out in review, if the current time is after the end of the slider, the correct hit object to use for sample retrieval is the object itself, not any nested object. --- .../Visual/Gameplay/TestSceneGameplaySampleTriggerSource.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/osu.Game.Tests/Visual/Gameplay/TestSceneGameplaySampleTriggerSource.cs b/osu.Game.Tests/Visual/Gameplay/TestSceneGameplaySampleTriggerSource.cs index f7641c0cc9..7f4f1ed027 100644 --- a/osu.Game.Tests/Visual/Gameplay/TestSceneGameplaySampleTriggerSource.cs +++ b/osu.Game.Tests/Visual/Gameplay/TestSceneGameplaySampleTriggerSource.cs @@ -155,7 +155,7 @@ namespace osu.Game.Tests.Visual.Gameplay // 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 after slider", () => Beatmap.Value.Track.Seek(beatmap.HitObjects[4].GetEndTime() + 100)); + AddStep("seek to just before slider ends", () => Beatmap.Value.Track.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())); From a511e64fa5badba12ca279f0f90409a52883e9d5 Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Wed, 22 Feb 2023 14:41:20 +0900 Subject: [PATCH 6/7] Seek using `GameplayClockContainer` --- .../Gameplay/TestSceneGameplaySampleTriggerSource.cs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/osu.Game.Tests/Visual/Gameplay/TestSceneGameplaySampleTriggerSource.cs b/osu.Game.Tests/Visual/Gameplay/TestSceneGameplaySampleTriggerSource.cs index 7f4f1ed027..b30bef7c30 100644 --- a/osu.Game.Tests/Visual/Gameplay/TestSceneGameplaySampleTriggerSource.cs +++ b/osu.Game.Tests/Visual/Gameplay/TestSceneGameplaySampleTriggerSource.cs @@ -155,16 +155,16 @@ namespace osu.Game.Tests.Visual.Gameplay // 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", () => Beatmap.Value.Track.Seek(beatmap.HitObjects[4].GetEndTime() - 100)); + 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", () => Beatmap.Value.Track.Seek(beatmap.HitObjects[4].GetEndTime() + 1000)); + 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", () => Beatmap.Value.Track.Seek(beatmap.HitObjects.Last().GetEndTime() + 10000)); + AddStep("Seek into future", () => Player.GameplayClockContainer.Seek(beatmap.HitObjects.Last().GetEndTime() + 10000)); waitForCatchUp(); waitForAliveObjectIndex(null); checkValidObjectIndex(4); @@ -172,7 +172,7 @@ namespace osu.Game.Tests.Visual.Gameplay 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(); } From f61fbcf3fc28ef676f7eb28d14e5a557100ef4bb Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Wed, 22 Feb 2023 15:26:09 +0900 Subject: [PATCH 7/7] Update assertion to also check `GameplayClockContainer`'s current time --- .../Visual/Gameplay/TestSceneGameplaySampleTriggerSource.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/osu.Game.Tests/Visual/Gameplay/TestSceneGameplaySampleTriggerSource.cs b/osu.Game.Tests/Visual/Gameplay/TestSceneGameplaySampleTriggerSource.cs index b30bef7c30..31133f00d9 100644 --- a/osu.Game.Tests/Visual/Gameplay/TestSceneGameplaySampleTriggerSource.cs +++ b/osu.Game.Tests/Visual/Gameplay/TestSceneGameplaySampleTriggerSource.cs @@ -177,7 +177,7 @@ namespace osu.Game.Tests.Visual.Gameplay } private void waitForCatchUp() => - AddUntilStep("wait for frame stable clock to catch up", () => Precision.AlmostEquals(Beatmap.Value.Track.CurrentTime, Player.DrawableRuleset.FrameStableClock.CurrentTime)); + AddUntilStep("wait for frame stable clock to catch up", () => Precision.AlmostEquals(Player.GameplayClockContainer.CurrentTime, Player.DrawableRuleset.FrameStableClock.CurrentTime)); private void waitForAliveObjectIndex(int? index) {