From 896f2d8f74041d279dca91bb9dbe8b47e017d2b3 Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Thu, 1 Dec 2022 17:43:54 +0900 Subject: [PATCH 1/6] Fix multiple instances of last hitobject time being calculated incorrectly --- osu.Game.Rulesets.Osu/Mods/OsuModTargetPractice.cs | 5 ++--- osu.Game/Beatmaps/Beatmap.cs | 5 ++++- osu.Game/Beatmaps/IBeatmap.cs | 12 ++++++++++++ osu.Game/Rulesets/Objects/BarLineGenerator.cs | 7 ++----- .../UI/Scrolling/DrawableScrollingRuleset.cs | 2 +- osu.Game/Screens/Edit/Editor.cs | 7 ++----- osu.Game/Screens/Play/ReplayPlayer.cs | 4 +--- 7 files changed, 24 insertions(+), 18 deletions(-) diff --git a/osu.Game.Rulesets.Osu/Mods/OsuModTargetPractice.cs b/osu.Game.Rulesets.Osu/Mods/OsuModTargetPractice.cs index 55c20eebe9..77cf340b95 100644 --- a/osu.Game.Rulesets.Osu/Mods/OsuModTargetPractice.cs +++ b/osu.Game.Rulesets.Osu/Mods/OsuModTargetPractice.cs @@ -17,7 +17,6 @@ using osu.Game.Configuration; using osu.Game.Graphics; using osu.Game.Overlays.Settings; using osu.Game.Rulesets.Mods; -using osu.Game.Rulesets.Objects; using osu.Game.Rulesets.Objects.Drawables; using osu.Game.Rulesets.Objects.Types; using osu.Game.Rulesets.Osu.Beatmaps; @@ -196,8 +195,8 @@ namespace osu.Game.Rulesets.Osu.Mods private IEnumerable generateBeats(IBeatmap beatmap, IReadOnlyCollection originalHitObjects) { - double startTime = originalHitObjects.First().StartTime; - double endTime = originalHitObjects.Last().GetEndTime(); + double startTime = beatmap.HitObjects.First().StartTime; + double endTime = beatmap.GetLastObjectTime(); var beats = beatmap.ControlPointInfo.TimingPoints // Ignore timing points after endTime diff --git a/osu.Game/Beatmaps/Beatmap.cs b/osu.Game/Beatmaps/Beatmap.cs index 2d02fb6200..2ab4fe0c79 100644 --- a/osu.Game/Beatmaps/Beatmap.cs +++ b/osu.Game/Beatmaps/Beatmap.cs @@ -81,9 +81,12 @@ namespace osu.Game.Beatmaps public double GetMostCommonBeatLength() { + if (!HitObjects.Any()) + return ControlPointInfo.TimingPoints.LastOrDefault()?.Time ?? 0; + // The last playable time in the beatmap - the last timing point extends to this time. // Note: This is more accurate and may present different results because osu-stable didn't have the ability to calculate slider durations in this context. - double lastTime = HitObjects.LastOrDefault()?.GetEndTime() ?? ControlPointInfo.TimingPoints.LastOrDefault()?.Time ?? 0; + double lastTime = this.GetLastObjectTime(); var mostCommon = // Construct a set of (beatLength, duration) tuples for each individual timing point. diff --git a/osu.Game/Beatmaps/IBeatmap.cs b/osu.Game/Beatmaps/IBeatmap.cs index 0e892b6581..f6771f7adf 100644 --- a/osu.Game/Beatmaps/IBeatmap.cs +++ b/osu.Game/Beatmaps/IBeatmap.cs @@ -4,6 +4,7 @@ #nullable disable using System.Collections.Generic; +using System.Linq; using osu.Game.Beatmaps.ControlPoints; using osu.Game.Beatmaps.Timing; using osu.Game.Rulesets.Objects; @@ -102,5 +103,16 @@ namespace osu.Game.Beatmaps addCombo(nested, ref combo); } } + + /// + /// Find the absolute end time of the latest in a beatmap. Will throw if beatmap contains no objects. + /// + /// + /// This correctly accounts for rulesets which have concurrent hitobjects which may have durations, causing the .Last() object + /// to not necessarily have the latest end time. + /// + /// It's not super efficient so calls should be kept to a minimum. + /// + public static double GetLastObjectTime(this IBeatmap beatmap) => beatmap.HitObjects.Max(h => h.GetEndTime()); } } diff --git a/osu.Game/Rulesets/Objects/BarLineGenerator.cs b/osu.Game/Rulesets/Objects/BarLineGenerator.cs index 5c76c43f20..af32c7def3 100644 --- a/osu.Game/Rulesets/Objects/BarLineGenerator.cs +++ b/osu.Game/Rulesets/Objects/BarLineGenerator.cs @@ -27,11 +27,8 @@ namespace osu.Game.Rulesets.Objects if (beatmap.HitObjects.Count == 0) return; - HitObject firstObject = beatmap.HitObjects.First(); - HitObject lastObject = beatmap.HitObjects.Last(); - - double firstHitTime = firstObject.StartTime; - double lastHitTime = 1 + lastObject.GetEndTime(); + double firstHitTime = beatmap.HitObjects.First().StartTime; + double lastHitTime = 1 + beatmap.GetLastObjectTime(); var timingPoints = beatmap.ControlPointInfo.TimingPoints; diff --git a/osu.Game/Rulesets/UI/Scrolling/DrawableScrollingRuleset.cs b/osu.Game/Rulesets/UI/Scrolling/DrawableScrollingRuleset.cs index 52853d3979..123be0f117 100644 --- a/osu.Game/Rulesets/UI/Scrolling/DrawableScrollingRuleset.cs +++ b/osu.Game/Rulesets/UI/Scrolling/DrawableScrollingRuleset.cs @@ -114,7 +114,7 @@ namespace osu.Game.Rulesets.UI.Scrolling break; } - double lastObjectTime = Objects.LastOrDefault()?.GetEndTime() ?? double.MaxValue; + double lastObjectTime = Beatmap.GetLastObjectTime(); double baseBeatLength = TimingControlPoint.DEFAULT_BEAT_LENGTH; if (RelativeScaleBeatLengths) diff --git a/osu.Game/Screens/Edit/Editor.cs b/osu.Game/Screens/Edit/Editor.cs index 02130b9662..b8d308cc7c 100644 --- a/osu.Game/Screens/Edit/Editor.cs +++ b/osu.Game/Screens/Edit/Editor.cs @@ -40,7 +40,6 @@ using osu.Game.Overlays.Notifications; using osu.Game.Overlays.OSD; using osu.Game.Rulesets; using osu.Game.Rulesets.Edit; -using osu.Game.Rulesets.Objects; using osu.Game.Screens.Edit.Components.Menus; using osu.Game.Screens.Edit.Compose; using osu.Game.Screens.Edit.Compose.Components.Timeline; @@ -538,12 +537,10 @@ namespace osu.Game.Screens.Edit // Seek to last object time, or track end if already there. // Note that in osu-stable subsequent presses when at track end won't return to last object. // This has intentionally been changed to make it more useful. - double? lastObjectTime = editorBeatmap.HitObjects.LastOrDefault()?.GetEndTime(); - - if (lastObjectTime == null || clock.CurrentTime == lastObjectTime) + if (!editorBeatmap.HitObjects.Any() || clock.CurrentTime == editorBeatmap.GetLastObjectTime()) clock.Seek(clock.TrackLength); else - clock.Seek(lastObjectTime.Value); + clock.Seek(editorBeatmap.GetLastObjectTime()); return true; } diff --git a/osu.Game/Screens/Play/ReplayPlayer.cs b/osu.Game/Screens/Play/ReplayPlayer.cs index 4395b96139..c5ef6b1585 100644 --- a/osu.Game/Screens/Play/ReplayPlayer.cs +++ b/osu.Game/Screens/Play/ReplayPlayer.cs @@ -5,7 +5,6 @@ using System; using System.Collections.Generic; -using System.Linq; using System.Threading.Tasks; using osu.Framework.Bindables; using osu.Framework.Input.Bindings; @@ -13,7 +12,6 @@ using osu.Framework.Input.Events; using osu.Game.Beatmaps; using osu.Game.Input.Bindings; using osu.Game.Rulesets.Mods; -using osu.Game.Rulesets.Objects; using osu.Game.Scoring; using osu.Game.Screens.Play.HUD; using osu.Game.Screens.Ranking; @@ -94,7 +92,7 @@ namespace osu.Game.Screens.Play void keyboardSeek(int direction) { - double target = Math.Clamp(GameplayClockContainer.CurrentTime + direction * keyboard_seek_amount, 0, GameplayState.Beatmap.HitObjects.Last().GetEndTime()); + double target = Math.Clamp(GameplayClockContainer.CurrentTime + direction * keyboard_seek_amount, 0, GameplayState.Beatmap.GetLastObjectTime()); Seek(target); } From d9d48516fce4a8cfda3d8b91fd3f1aa55270ce8f Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Thu, 1 Dec 2022 18:04:04 +0900 Subject: [PATCH 2/6] Add test coverage of getting last object time --- .../Formats/LegacyBeatmapDecoderTest.cs | 18 +++++++++ .../mania-last-object-not-latest.osu | 39 +++++++++++++++++++ 2 files changed, 57 insertions(+) create mode 100644 osu.Game.Tests/Resources/mania-last-object-not-latest.osu diff --git a/osu.Game.Tests/Beatmaps/Formats/LegacyBeatmapDecoderTest.cs b/osu.Game.Tests/Beatmaps/Formats/LegacyBeatmapDecoderTest.cs index c6bdd25e8b..5787bd6066 100644 --- a/osu.Game.Tests/Beatmaps/Formats/LegacyBeatmapDecoderTest.cs +++ b/osu.Game.Tests/Beatmaps/Formats/LegacyBeatmapDecoderTest.cs @@ -314,6 +314,24 @@ namespace osu.Game.Tests.Beatmaps.Formats } } + [Test] + public void TestGetLastObjectTime() + { + var decoder = new LegacyBeatmapDecoder(); + + using (var resStream = TestResources.OpenResource("mania-last-object-not-latest.osu")) + using (var stream = new LineBufferedReader(resStream)) + { + var beatmap = decoder.Decode(stream); + + Assert.That(beatmap.HitObjects.Last().StartTime, Is.EqualTo(2494)); + Assert.That(beatmap.HitObjects.Last().GetEndTime(), Is.EqualTo(2494)); + + Assert.That(beatmap.HitObjects.Max(h => h.GetEndTime()), Is.EqualTo(2582)); + Assert.That(beatmap.GetLastObjectTime(), Is.EqualTo(2582)); + } + } + [Test] public void TestDecodeBeatmapComboOffsetsOsu() { diff --git a/osu.Game.Tests/Resources/mania-last-object-not-latest.osu b/osu.Game.Tests/Resources/mania-last-object-not-latest.osu new file mode 100644 index 0000000000..51893383d8 --- /dev/null +++ b/osu.Game.Tests/Resources/mania-last-object-not-latest.osu @@ -0,0 +1,39 @@ +osu file format v14 + +[General] +SampleSet: Normal +StackLeniency: 0.7 +Mode: 3 + +[Difficulty] +HPDrainRate:3 +CircleSize:5 +OverallDifficulty:8 +ApproachRate:8 +SliderMultiplier:3.59999990463257 +SliderTickRate:2 + +[TimingPoints] +24,352.941176470588,4,1,1,100,1,0 +6376,-50,4,1,1,100,0,0 + +[HitObjects] +51,192,24,1,0,0:0:0:0: +153,192,200,1,0,0:0:0:0: +358,192,376,1,0,0:0:0:0: +460,192,553,1,0,0:0:0:0: +460,192,729,128,0,1435:0:0:0:0: +358,192,906,128,0,1612:0:0:0:0: +256,192,1082,128,0,1788:0:0:0:0: +153,192,1259,128,0,1965:0:0:0:0: +51,192,1435,128,0,2141:0:0:0:0: +51,192,2318,1,12,0:0:0:0: +153,192,2318,1,4,0:0:0:0: +256,192,2318,1,6,0:0:0:0: +358,192,2318,1,14,0:0:0:0: +460,192,2318,1,0,0:0:0:0: +51,192,2494,128,0,2582:0:0:0:0: +153,192,2494,128,14,2582:0:0:0:0: +256,192,2494,128,6,2582:0:0:0:0: +358,192,2494,128,4,2582:0:0:0:0: +460,192,2494,1,12,0:0:0:0:0: From d8dd7e7e0ff8a9476949d3ab73e27777fa638e83 Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Thu, 1 Dec 2022 21:42:03 +0900 Subject: [PATCH 3/6] Fix test failures when no hitobjects are present in a scrolling ruleset --- osu.Game/Rulesets/UI/Scrolling/DrawableScrollingRuleset.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/osu.Game/Rulesets/UI/Scrolling/DrawableScrollingRuleset.cs b/osu.Game/Rulesets/UI/Scrolling/DrawableScrollingRuleset.cs index 123be0f117..5218ddfe15 100644 --- a/osu.Game/Rulesets/UI/Scrolling/DrawableScrollingRuleset.cs +++ b/osu.Game/Rulesets/UI/Scrolling/DrawableScrollingRuleset.cs @@ -114,7 +114,7 @@ namespace osu.Game.Rulesets.UI.Scrolling break; } - double lastObjectTime = Beatmap.GetLastObjectTime(); + double lastObjectTime = Beatmap.HitObjects.Any() ? Beatmap.GetLastObjectTime() : 0; double baseBeatLength = TimingControlPoint.DEFAULT_BEAT_LENGTH; if (RelativeScaleBeatLengths) From 809d02cda0156032360471aa5bf287c829529ea0 Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Fri, 2 Dec 2022 16:05:14 +0900 Subject: [PATCH 4/6] Fix two implementation oversights --- osu.Game/Beatmaps/Beatmap.cs | 8 +++++--- .../Rulesets/UI/Scrolling/DrawableScrollingRuleset.cs | 2 +- 2 files changed, 6 insertions(+), 4 deletions(-) diff --git a/osu.Game/Beatmaps/Beatmap.cs b/osu.Game/Beatmaps/Beatmap.cs index 2ab4fe0c79..416d655cc3 100644 --- a/osu.Game/Beatmaps/Beatmap.cs +++ b/osu.Game/Beatmaps/Beatmap.cs @@ -81,12 +81,14 @@ namespace osu.Game.Beatmaps public double GetMostCommonBeatLength() { - if (!HitObjects.Any()) - return ControlPointInfo.TimingPoints.LastOrDefault()?.Time ?? 0; + double lastTime; // The last playable time in the beatmap - the last timing point extends to this time. // Note: This is more accurate and may present different results because osu-stable didn't have the ability to calculate slider durations in this context. - double lastTime = this.GetLastObjectTime(); + if (!HitObjects.Any()) + lastTime = ControlPointInfo.TimingPoints.LastOrDefault()?.Time ?? 0; + else + lastTime = this.GetLastObjectTime(); var mostCommon = // Construct a set of (beatLength, duration) tuples for each individual timing point. diff --git a/osu.Game/Rulesets/UI/Scrolling/DrawableScrollingRuleset.cs b/osu.Game/Rulesets/UI/Scrolling/DrawableScrollingRuleset.cs index 5218ddfe15..4c7564b791 100644 --- a/osu.Game/Rulesets/UI/Scrolling/DrawableScrollingRuleset.cs +++ b/osu.Game/Rulesets/UI/Scrolling/DrawableScrollingRuleset.cs @@ -114,7 +114,7 @@ namespace osu.Game.Rulesets.UI.Scrolling break; } - double lastObjectTime = Beatmap.HitObjects.Any() ? Beatmap.GetLastObjectTime() : 0; + double lastObjectTime = Beatmap.HitObjects.Any() ? Beatmap.GetLastObjectTime() : double.MaxValue; double baseBeatLength = TimingControlPoint.DEFAULT_BEAT_LENGTH; if (RelativeScaleBeatLengths) From a2db0afe1922cb1e6a99247b222c4800b9bea36f Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Fri, 2 Dec 2022 16:07:24 +0900 Subject: [PATCH 5/6] Avoid querying `GetLastObjectTime` twice in editor seek flow --- osu.Game/Screens/Edit/Editor.cs | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/osu.Game/Screens/Edit/Editor.cs b/osu.Game/Screens/Edit/Editor.cs index b8d308cc7c..f3f2b8ad6b 100644 --- a/osu.Game/Screens/Edit/Editor.cs +++ b/osu.Game/Screens/Edit/Editor.cs @@ -537,10 +537,14 @@ namespace osu.Game.Screens.Edit // Seek to last object time, or track end if already there. // Note that in osu-stable subsequent presses when at track end won't return to last object. // This has intentionally been changed to make it more useful. - if (!editorBeatmap.HitObjects.Any() || clock.CurrentTime == editorBeatmap.GetLastObjectTime()) + if (!editorBeatmap.HitObjects.Any()) + { clock.Seek(clock.TrackLength); - else - clock.Seek(editorBeatmap.GetLastObjectTime()); + return true; + } + + double lastObjectTime = editorBeatmap.GetLastObjectTime(); + clock.Seek(clock.CurrentTime == lastObjectTime ? clock.TrackLength : lastObjectTime); return true; } From 25120060aa16f05d32d80119c1bf1bb0379bd020 Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Fri, 2 Dec 2022 16:09:26 +0900 Subject: [PATCH 6/6] Use new helper method in `ModTimeRamp` --- osu.Game/Rulesets/Mods/ModTimeRamp.cs | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/osu.Game/Rulesets/Mods/ModTimeRamp.cs b/osu.Game/Rulesets/Mods/ModTimeRamp.cs index c4cb41fb6a..7285315c3b 100644 --- a/osu.Game/Rulesets/Mods/ModTimeRamp.cs +++ b/osu.Game/Rulesets/Mods/ModTimeRamp.cs @@ -7,7 +7,6 @@ using osu.Framework.Audio; using osu.Framework.Bindables; using osu.Game.Beatmaps; using osu.Game.Configuration; -using osu.Game.Rulesets.Objects; using osu.Game.Rulesets.UI; namespace osu.Game.Rulesets.Mods @@ -71,7 +70,7 @@ namespace osu.Game.Rulesets.Mods SpeedChange.SetDefault(); double firstObjectStart = beatmap.HitObjects.FirstOrDefault()?.StartTime ?? 0; - double lastObjectEnd = beatmap.HitObjects.LastOrDefault()?.GetEndTime() ?? 0; + double lastObjectEnd = beatmap.HitObjects.Any() ? beatmap.GetLastObjectTime() : 0; beginRampTime = firstObjectStart; finalRateTime = firstObjectStart + FINAL_RATE_PROGRESS * (lastObjectEnd - firstObjectStart);