From 58654f28b67e4ce29d50125237e075007f0ec7ff Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Tue, 28 Jan 2020 12:48:24 +0900 Subject: [PATCH 1/3] Fix beat snap implementation being incorrect --- osu.Game/Rulesets/Edit/HitObjectComposer.cs | 4 ++-- osu.Game/Rulesets/Edit/IBeatSnapProvider.cs | 8 ++++---- .../Screens/Edit/Compose/Components/Timeline/Timeline.cs | 2 +- osu.Game/Screens/Edit/Editor.cs | 2 +- osu.Game/Screens/Edit/EditorBeatmap.cs | 8 +++++--- 5 files changed, 13 insertions(+), 11 deletions(-) diff --git a/osu.Game/Rulesets/Edit/HitObjectComposer.cs b/osu.Game/Rulesets/Edit/HitObjectComposer.cs index 9ee3bacf9b..252b418523 100644 --- a/osu.Game/Rulesets/Edit/HitObjectComposer.cs +++ b/osu.Game/Rulesets/Edit/HitObjectComposer.cs @@ -275,10 +275,10 @@ namespace osu.Game.Rulesets.Edit } public override double GetSnappedDurationFromDistance(double referenceTime, float distance) - => beatSnapProvider.SnapTime(referenceTime, DistanceToDuration(referenceTime, distance)); + => beatSnapProvider.SnapTime(referenceTime + DistanceToDuration(referenceTime, distance), referenceTime) - referenceTime; public override float GetSnappedDistanceFromDistance(double referenceTime, float distance) - => DurationToDistance(referenceTime, beatSnapProvider.SnapTime(referenceTime, DistanceToDuration(referenceTime, distance))); + => DurationToDistance(referenceTime, beatSnapProvider.SnapTime(DistanceToDuration(referenceTime, distance), referenceTime)); protected override void Dispose(bool isDisposing) { diff --git a/osu.Game/Rulesets/Edit/IBeatSnapProvider.cs b/osu.Game/Rulesets/Edit/IBeatSnapProvider.cs index e1daafaebe..616f854cd7 100644 --- a/osu.Game/Rulesets/Edit/IBeatSnapProvider.cs +++ b/osu.Game/Rulesets/Edit/IBeatSnapProvider.cs @@ -8,10 +8,10 @@ namespace osu.Game.Rulesets.Edit /// /// Snaps a duration to the closest beat of a timing point applicable at the reference time. /// - /// The time of the timing point which resides in. - /// The duration to snap. - /// A value that represents snapped to the closest beat of the timing point. - double SnapTime(double referenceTime, double duration); + /// The time to snap. + /// An optional reference point to use for timing point lookup. + /// A value that represents snapped to the closest beat of the timing point. + double SnapTime(double time, double? referenceTime = null); /// /// Get the most appropriate beat length at a given time. diff --git a/osu.Game/Screens/Edit/Compose/Components/Timeline/Timeline.cs b/osu.Game/Screens/Edit/Compose/Components/Timeline/Timeline.cs index 96395696c3..2dd7ad79ba 100644 --- a/osu.Game/Screens/Edit/Compose/Components/Timeline/Timeline.cs +++ b/osu.Game/Screens/Edit/Compose/Components/Timeline/Timeline.cs @@ -177,7 +177,7 @@ namespace osu.Game.Screens.Edit.Compose.Components.Timeline public (Vector2 position, double time) GetSnappedPosition(Vector2 position, double time) { var targetTime = (position.X / Content.DrawWidth) * track.Length; - return (position, beatSnapProvider.SnapTime(targetTime, targetTime)); + return (position, beatSnapProvider.SnapTime(targetTime)); } public float GetBeatSnapDistanceAt(double referenceTime) => throw new NotImplementedException(); diff --git a/osu.Game/Screens/Edit/Editor.cs b/osu.Game/Screens/Edit/Editor.cs index eae94a3c8e..8c7270d3a2 100644 --- a/osu.Game/Screens/Edit/Editor.cs +++ b/osu.Game/Screens/Edit/Editor.cs @@ -348,7 +348,7 @@ namespace osu.Game.Screens.Edit beatmapManager.Export(Beatmap.Value.BeatmapSetInfo); } - public double SnapTime(double referenceTime, double duration) => editorBeatmap.SnapTime(referenceTime, duration); + public double SnapTime(double time, double? referenceTime) => editorBeatmap.SnapTime(time, referenceTime); public double GetBeatLengthAtTime(double referenceTime) => editorBeatmap.GetBeatLengthAtTime(referenceTime); diff --git a/osu.Game/Screens/Edit/EditorBeatmap.cs b/osu.Game/Screens/Edit/EditorBeatmap.cs index 9c75d40bec..2d3ecf583e 100644 --- a/osu.Game/Screens/Edit/EditorBeatmap.cs +++ b/osu.Game/Screens/Edit/EditorBeatmap.cs @@ -128,12 +128,14 @@ namespace osu.Game.Screens.Edit return list.Count - 1; } - public double SnapTime(double referenceTime, double duration) + public double SnapTime(double time, double? referenceTime) { - double beatLength = GetBeatLengthAtTime(referenceTime); + var timingPoint = ControlPointInfo.TimingPointAt(referenceTime ?? time); + + var beatLength = timingPoint.BeatLength / BeatDivisor; // A 1ms offset prevents rounding errors due to minute variations in duration - return (int)((duration + 1) / beatLength) * beatLength; + return timingPoint.Time + (int)Math.Round(((time - timingPoint.Time) + 1) / beatLength) * beatLength; } public double GetBeatLengthAtTime(double referenceTime) => ControlPointInfo.TimingPointAt(referenceTime).BeatLength / BeatDivisor; From 596a01661cfed0e2dc275c105754fb6c4304adb1 Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Tue, 28 Jan 2020 13:42:22 +0900 Subject: [PATCH 2/3] Remove 1ms offset and update tests --- ...tSceneHitObjectComposerDistanceSnapping.cs | 30 ++++++++++--------- osu.Game/Screens/Edit/EditorBeatmap.cs | 2 +- 2 files changed, 17 insertions(+), 15 deletions(-) diff --git a/osu.Game.Tests/Editor/TestSceneHitObjectComposerDistanceSnapping.cs b/osu.Game.Tests/Editor/TestSceneHitObjectComposerDistanceSnapping.cs index e825df5a3f..5a4e76d586 100644 --- a/osu.Game.Tests/Editor/TestSceneHitObjectComposerDistanceSnapping.cs +++ b/osu.Game.Tests/Editor/TestSceneHitObjectComposerDistanceSnapping.cs @@ -118,17 +118,19 @@ namespace osu.Game.Tests.Editor [Test] public void TestGetSnappedDurationFromDistance() { - assertSnappedDuration(50, 0); + assertSnappedDuration(0, 0); + assertSnappedDuration(50, 1000); assertSnappedDuration(100, 1000); - assertSnappedDuration(150, 1000); + assertSnappedDuration(150, 2000); assertSnappedDuration(200, 2000); - assertSnappedDuration(250, 2000); + assertSnappedDuration(250, 3000); AddStep("set slider multiplier = 2", () => composer.EditorBeatmap.BeatmapInfo.BaseDifficulty.SliderMultiplier = 2); + assertSnappedDuration(0, 0); assertSnappedDuration(50, 0); - assertSnappedDuration(100, 0); - assertSnappedDuration(150, 0); + assertSnappedDuration(100, 1000); + assertSnappedDuration(150, 1000); assertSnappedDuration(200, 1000); assertSnappedDuration(250, 1000); @@ -139,8 +141,8 @@ namespace osu.Game.Tests.Editor }); assertSnappedDuration(50, 0); - assertSnappedDuration(100, 0); - assertSnappedDuration(150, 0); + assertSnappedDuration(100, 500); + assertSnappedDuration(150, 500); assertSnappedDuration(200, 500); assertSnappedDuration(250, 500); assertSnappedDuration(400, 1000); @@ -149,17 +151,17 @@ namespace osu.Game.Tests.Editor [Test] public void GetSnappedDistanceFromDistance() { - assertSnappedDistance(50, 0); + assertSnappedDistance(50, 100); assertSnappedDistance(100, 100); - assertSnappedDistance(150, 100); + assertSnappedDistance(150, 200); assertSnappedDistance(200, 200); - assertSnappedDistance(250, 200); + assertSnappedDistance(250, 300); AddStep("set slider multiplier = 2", () => composer.EditorBeatmap.BeatmapInfo.BaseDifficulty.SliderMultiplier = 2); assertSnappedDistance(50, 0); - assertSnappedDistance(100, 0); - assertSnappedDistance(150, 0); + assertSnappedDistance(100, 200); + assertSnappedDistance(150, 200); assertSnappedDistance(200, 200); assertSnappedDistance(250, 200); @@ -170,8 +172,8 @@ namespace osu.Game.Tests.Editor }); assertSnappedDistance(50, 0); - assertSnappedDistance(100, 0); - assertSnappedDistance(150, 0); + assertSnappedDistance(100, 200); + assertSnappedDistance(150, 200); assertSnappedDistance(200, 200); assertSnappedDistance(250, 200); assertSnappedDistance(400, 400); diff --git a/osu.Game/Screens/Edit/EditorBeatmap.cs b/osu.Game/Screens/Edit/EditorBeatmap.cs index 2d3ecf583e..385afc2392 100644 --- a/osu.Game/Screens/Edit/EditorBeatmap.cs +++ b/osu.Game/Screens/Edit/EditorBeatmap.cs @@ -135,7 +135,7 @@ namespace osu.Game.Screens.Edit var beatLength = timingPoint.BeatLength / BeatDivisor; // A 1ms offset prevents rounding errors due to minute variations in duration - return timingPoint.Time + (int)Math.Round(((time - timingPoint.Time) + 1) / beatLength) * beatLength; + return timingPoint.Time + (int)Math.Round((time - timingPoint.Time) / beatLength, MidpointRounding.AwayFromZero) * beatLength; } public double GetBeatLengthAtTime(double referenceTime) => ControlPointInfo.TimingPointAt(referenceTime).BeatLength / BeatDivisor; From 9a47428bfd9fa68fd1ba9924d579f28260bec861 Mon Sep 17 00:00:00 2001 From: smoogipoo Date: Wed, 29 Jan 2020 14:40:42 +0900 Subject: [PATCH 3/3] Remove out of date comment --- osu.Game/Screens/Edit/EditorBeatmap.cs | 2 -- 1 file changed, 2 deletions(-) diff --git a/osu.Game/Screens/Edit/EditorBeatmap.cs b/osu.Game/Screens/Edit/EditorBeatmap.cs index 385afc2392..6edd62fa67 100644 --- a/osu.Game/Screens/Edit/EditorBeatmap.cs +++ b/osu.Game/Screens/Edit/EditorBeatmap.cs @@ -131,10 +131,8 @@ namespace osu.Game.Screens.Edit public double SnapTime(double time, double? referenceTime) { var timingPoint = ControlPointInfo.TimingPointAt(referenceTime ?? time); - var beatLength = timingPoint.BeatLength / BeatDivisor; - // A 1ms offset prevents rounding errors due to minute variations in duration return timingPoint.Time + (int)Math.Round((time - timingPoint.Time) / beatLength, MidpointRounding.AwayFromZero) * beatLength; }