From bc6b64b1d7a2ffc5310a69a69a08a8b1a01f0be7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bart=C5=82omiej=20Dach?= Date: Sun, 17 May 2020 21:55:01 +0200 Subject: [PATCH 1/9] Add failing test --- .../NonVisual/BarLineGeneratorTest.cs | 73 +++++++++++++++++++ 1 file changed, 73 insertions(+) create mode 100644 osu.Game.Tests/NonVisual/BarLineGeneratorTest.cs diff --git a/osu.Game.Tests/NonVisual/BarLineGeneratorTest.cs b/osu.Game.Tests/NonVisual/BarLineGeneratorTest.cs new file mode 100644 index 0000000000..e663e1128e --- /dev/null +++ b/osu.Game.Tests/NonVisual/BarLineGeneratorTest.cs @@ -0,0 +1,73 @@ +// Copyright (c) ppy Pty Ltd . Licensed under the MIT Licence. +// See the LICENCE file in the repository root for full licence text. + +using System.Collections.Generic; +using NUnit.Framework; +using osu.Framework.Utils; +using osu.Game.Beatmaps; +using osu.Game.Beatmaps.ControlPoints; +using osu.Game.Beatmaps.Timing; +using osu.Game.Rulesets.Objects; + +namespace osu.Game.Tests.NonVisual +{ + public class BarLineGeneratorTest + { + [Test] + public void TestRoundingErrorCompensation() + { + // The aim of this test is to make sure bar line generation compensates for floating-point errors. + // The premise of the test is that we have a single timing point that should result in bar lines + // that start at a time point that is a whole number every seventh beat. + + // The fact it's every seventh beat is important - it's a number indivisible by 2, which makes + // it susceptible to rounding inaccuracies. In fact this was originally spotted in cases of maps + // that met exactly this criteria. + + const int beat_length_numerator = 2000; + const int beat_length_denominator = 7; + const TimeSignatures signature = TimeSignatures.SimpleQuadruple; + + var beatmap = new Beatmap + { + HitObjects = new List + { + new HitObject { StartTime = 0 }, + new HitObject { StartTime = 120_000 } + }, + ControlPointInfo = new ControlPointInfo() + }; + + beatmap.ControlPointInfo.Add(0, new TimingControlPoint + { + BeatLength = (double)beat_length_numerator / beat_length_denominator, + TimeSignature = signature + }); + + var barLines = new BarLineGenerator(beatmap).BarLines; + + for (int i = 0; i * beat_length_denominator < barLines.Count; i++) + { + var barLine = barLines[i * beat_length_denominator]; + var expectedTime = beat_length_numerator * (int)signature * i; + + // every seventh bar's start time should be at least greater than the whole number we expect. + // It cannot be less, as that can affect overlapping scroll algorithms + // (the previous timing point might be chosen incorrectly if this is not the case) + Assert.GreaterOrEqual(barLine.StartTime, expectedTime); + + // on the other side, make sure we don't stray too far from the expected time either. + Assert.IsTrue(Precision.AlmostEquals(barLine.StartTime, expectedTime)); + + // check major/minor lines for good measure too + Assert.AreEqual(i % (int)signature == 0, barLine.Major); + } + } + + private class BarLine : IBarLine + { + public double StartTime { get; set; } + public bool Major { get; set; } + } + } +} From 17ae392a759771801a8a3d325c4e630f4c60f770 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bart=C5=82omiej=20Dach?= Date: Sun, 17 May 2020 22:08:49 +0200 Subject: [PATCH 2/9] Apply rounding to bar line start times --- osu.Game/Rulesets/Objects/BarLineGenerator.cs | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/osu.Game/Rulesets/Objects/BarLineGenerator.cs b/osu.Game/Rulesets/Objects/BarLineGenerator.cs index 5588e9c0b7..9556b52735 100644 --- a/osu.Game/Rulesets/Objects/BarLineGenerator.cs +++ b/osu.Game/Rulesets/Objects/BarLineGenerator.cs @@ -1,6 +1,7 @@ // Copyright (c) ppy Pty Ltd . Licensed under the MIT Licence. // See the LICENCE file in the repository root for full licence text. +using System; using System.Collections.Generic; using System.Linq; using osu.Framework.Utils; @@ -46,6 +47,16 @@ namespace osu.Game.Rulesets.Objects for (double t = currentTimingPoint.Time; Precision.DefinitelyBigger(endTime, t); t += barLength, currentBeat++) { + var roundedTime = Math.Round(t, MidpointRounding.AwayFromZero); + + // in the case of some bar lengths, rounding errors can cause t to be slightly less than + // the expected whole number value due to floating point inaccuracies. + // if this is the case, apply rounding. + if (Precision.AlmostEquals(t, roundedTime)) + { + t = roundedTime; + } + BarLines.Add(new TBarLine { StartTime = t, From 8702a1b5a57ba9a70657b244eaec33797727caf7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bart=C5=82omiej=20Dach?= Date: Thu, 21 May 2020 20:10:51 +0200 Subject: [PATCH 3/9] Fix test scene regression --- .../Visual/Gameplay/TestSceneScrollingHitObjects.cs | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/osu.Game.Tests/Visual/Gameplay/TestSceneScrollingHitObjects.cs b/osu.Game.Tests/Visual/Gameplay/TestSceneScrollingHitObjects.cs index 0d15e495e3..20b040dbc3 100644 --- a/osu.Game.Tests/Visual/Gameplay/TestSceneScrollingHitObjects.cs +++ b/osu.Game.Tests/Visual/Gameplay/TestSceneScrollingHitObjects.cs @@ -16,6 +16,7 @@ using osu.Game.Configuration; using osu.Game.Rulesets.Mods; using osu.Game.Rulesets.Objects; using osu.Game.Rulesets.Objects.Drawables; +using osu.Game.Rulesets.Scoring; using osu.Game.Rulesets.Timing; using osu.Game.Rulesets.UI.Scrolling; using osuTK; @@ -221,7 +222,7 @@ namespace osu.Game.Tests.Visual.Gameplay private class TestDrawableControlPoint : DrawableHitObject { public TestDrawableControlPoint(ScrollingDirection direction, double time) - : base(new HitObject { StartTime = time }) + : base(new HitObject { StartTime = time, HitWindows = HitWindows.Empty }) { Origin = Anchor.Centre; @@ -252,7 +253,7 @@ namespace osu.Game.Tests.Visual.Gameplay private class TestDrawableHitObject : DrawableHitObject { public TestDrawableHitObject(double time) - : base(new HitObject { StartTime = time }) + : base(new HitObject { StartTime = time, HitWindows = HitWindows.Empty }) { Origin = Anchor.Custom; OriginPosition = new Vector2(75 / 4.0f); From 24d898c87031a62b21c98812e0ff2939392f7d5c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bart=C5=82omiej=20Dach?= Date: Thu, 21 May 2020 21:35:12 +0200 Subject: [PATCH 4/9] Demonstrate failure case in visual test scene --- .../Gameplay/TestSceneScrollingHitObjects.cs | 37 +++++++++++++++++-- 1 file changed, 33 insertions(+), 4 deletions(-) diff --git a/osu.Game.Tests/Visual/Gameplay/TestSceneScrollingHitObjects.cs b/osu.Game.Tests/Visual/Gameplay/TestSceneScrollingHitObjects.cs index 20b040dbc3..2f15e549f7 100644 --- a/osu.Game.Tests/Visual/Gameplay/TestSceneScrollingHitObjects.cs +++ b/osu.Game.Tests/Visual/Gameplay/TestSceneScrollingHitObjects.cs @@ -78,19 +78,18 @@ namespace osu.Game.Tests.Visual.Gameplay } }; - setUpHitObjects(); + hitObjectSpawnDelegate?.Cancel(); }); - private void setUpHitObjects() + private void setUpHitObjects() => AddStep("set up hit objects", () => { scrollContainers.ForEach(c => c.ControlPoints.Add(new MultiplierControlPoint(0))); for (int i = spawn_rate / 2; i <= time_range; i += spawn_rate) addHitObject(Time.Current + i); - hitObjectSpawnDelegate?.Cancel(); hitObjectSpawnDelegate = Scheduler.AddDelayed(() => addHitObject(Time.Current + time_range), spawn_rate, true); - } + }); private IList testControlPoints => new List { @@ -102,6 +101,8 @@ namespace osu.Game.Tests.Visual.Gameplay [Test] public void TestScrollAlgorithms() { + setUpHitObjects(); + AddStep("constant scroll", () => setScrollAlgorithm(ScrollVisualisationMethod.Constant)); AddStep("overlapping scroll", () => setScrollAlgorithm(ScrollVisualisationMethod.Overlapping)); AddStep("sequential scroll", () => setScrollAlgorithm(ScrollVisualisationMethod.Sequential)); @@ -114,6 +115,8 @@ namespace osu.Game.Tests.Visual.Gameplay [Test] public void TestConstantScrollLifetime() { + setUpHitObjects(); + AddStep("set constant scroll", () => setScrollAlgorithm(ScrollVisualisationMethod.Constant)); // scroll container time range must be less than the rate of spawning hitobjects // otherwise the hitobjects will spawn already partly visible on screen and look wrong @@ -123,14 +126,40 @@ namespace osu.Game.Tests.Visual.Gameplay [Test] public void TestSequentialScrollLifetime() { + setUpHitObjects(); + AddStep("set sequential scroll", () => setScrollAlgorithm(ScrollVisualisationMethod.Sequential)); AddStep("set time range", () => scrollContainers.ForEach(c => c.TimeRange = time_range / 2.0)); AddStep("add control points", () => addControlPoints(testControlPoints, Time.Current)); } + [Test] + public void TestSlowSequentialScroll() + { + AddStep("set sequential scroll", () => setScrollAlgorithm(ScrollVisualisationMethod.Sequential)); + AddStep("set time range", () => scrollContainers.ForEach(c => c.TimeRange = time_range)); + AddStep("add control points", () => addControlPoints( + new List + { + new MultiplierControlPoint { Velocity = 0.1 } + }, + Time.Current + time_range)); + + // All of the hit objects added below should be immediately visible on screen + AddStep("add hit objects", () => + { + for (int i = 0; i < 20; ++i) + { + addHitObject(Time.Current + time_range * (2 + 0.1 * i)); + } + }); + } + [Test] public void TestOverlappingScrollLifetime() { + setUpHitObjects(); + AddStep("set overlapping scroll", () => setScrollAlgorithm(ScrollVisualisationMethod.Overlapping)); AddStep("set time range", () => scrollContainers.ForEach(c => c.TimeRange = time_range / 2.0)); AddStep("add control points", () => addControlPoints(testControlPoints, Time.Current)); From 4299bd05b4ca268e192a4f4d469f69bed4c6415b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bart=C5=82omiej=20Dach?= Date: Thu, 21 May 2020 21:39:15 +0200 Subject: [PATCH 5/9] Add test cases for sequential scroll algorithm --- .../ScrollAlgorithms/SequentialScrollTest.cs | 18 ++++++++++++++++-- 1 file changed, 16 insertions(+), 2 deletions(-) diff --git a/osu.Game.Tests/ScrollAlgorithms/SequentialScrollTest.cs b/osu.Game.Tests/ScrollAlgorithms/SequentialScrollTest.cs index 1f0c069f8d..bd578dcbc4 100644 --- a/osu.Game.Tests/ScrollAlgorithms/SequentialScrollTest.cs +++ b/osu.Game.Tests/ScrollAlgorithms/SequentialScrollTest.cs @@ -29,8 +29,22 @@ namespace osu.Game.Tests.ScrollAlgorithms [Test] public void TestDisplayStartTime() { - // Sequential scroll algorithm approximates the start time - // This should be fixed in the future + // easy cases - time range adjusted for velocity fits within control point duration + Assert.AreEqual(2500, algorithm.GetDisplayStartTime(5000, 0, 2500, 1)); // 5000 - (2500 / 1) + Assert.AreEqual(13750, algorithm.GetDisplayStartTime(15000, 0, 2500, 1)); // 15000 - (2500 / 2) + Assert.AreEqual(20000, algorithm.GetDisplayStartTime(25000, 0, 2500, 1)); // 25000 - (2500 / 0.5) + + // hard case - time range adjusted for velocity exceeds control point duration + + // 1st multiplier point takes 10000 / 2500 = 4 scroll lengths + // 2nd multiplier point takes 10000 / (2500 / 2) = 8 scroll lengths + // 3rd multiplier point takes 2500 / (2500 * 2) = 0.5 scroll lengths up to hitobject start + + // absolute position of the hitobject = 1000 * (4 + 8 + 0.5) = 12500 + // minus one scroll length allowance = 12500 - 1000 = 11500 = 11.5 [scroll lengths] + // therefore the start time lies within the second multiplier point (because 11.5 < 4 + 8) + // its exact time position is = 10000 + 7.5 * (2500 / 2) = 19375 + Assert.AreEqual(19375, algorithm.GetDisplayStartTime(22500, 0, 2500, 1000)); } [Test] From 6f388b731ee97aedebc375370b429539cf8946d3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bart=C5=82omiej=20Dach?= Date: Thu, 21 May 2020 21:41:56 +0200 Subject: [PATCH 6/9] Fix display start time in sequential scroll algorithm --- .../UI/Scrolling/Algorithms/SequentialScrollAlgorithm.cs | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/osu.Game/Rulesets/UI/Scrolling/Algorithms/SequentialScrollAlgorithm.cs b/osu.Game/Rulesets/UI/Scrolling/Algorithms/SequentialScrollAlgorithm.cs index 41f9ebdb82..0052c877f6 100644 --- a/osu.Game/Rulesets/UI/Scrolling/Algorithms/SequentialScrollAlgorithm.cs +++ b/osu.Game/Rulesets/UI/Scrolling/Algorithms/SequentialScrollAlgorithm.cs @@ -22,8 +22,7 @@ namespace osu.Game.Rulesets.UI.Scrolling.Algorithms public double GetDisplayStartTime(double originTime, float offset, double timeRange, float scrollLength) { - double adjustedTime = TimeAt(-offset, originTime, timeRange, scrollLength); - return adjustedTime - timeRange - 1000; + return TimeAt(-(scrollLength + offset), originTime, timeRange, scrollLength); } public float GetLength(double startTime, double endTime, double timeRange, float scrollLength) From 3e0ee310d0e0d7dd529c1e7c8a3e59f4a4e8ec7e Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Fri, 22 May 2020 18:30:39 +0900 Subject: [PATCH 7/9] Remove now incorrect comment --- osu.Game/Rulesets/Edit/PlacementBlueprint.cs | 2 -- 1 file changed, 2 deletions(-) diff --git a/osu.Game/Rulesets/Edit/PlacementBlueprint.cs b/osu.Game/Rulesets/Edit/PlacementBlueprint.cs index f0b63f8ea5..e71ccc33a4 100644 --- a/osu.Game/Rulesets/Edit/PlacementBlueprint.cs +++ b/osu.Game/Rulesets/Edit/PlacementBlueprint.cs @@ -64,9 +64,7 @@ namespace osu.Game.Rulesets.Edit /// Whether this call is committing a value for HitObject.StartTime and continuing with further adjustments. protected void BeginPlacement(bool commitStart = false) { - // applies snapping to above time placementHandler.BeginPlacement(HitObject); - PlacementActive |= commitStart; } From af30d1201f0934fc49ee4857e0157611bdc4114c Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Fri, 22 May 2020 18:57:28 +0900 Subject: [PATCH 8/9] Fix slider path control point blueprint not working correctly --- .../Blueprints/Sliders/Components/PathControlPointPiece.cs | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/osu.Game.Rulesets.Osu/Edit/Blueprints/Sliders/Components/PathControlPointPiece.cs b/osu.Game.Rulesets.Osu/Edit/Blueprints/Sliders/Components/PathControlPointPiece.cs index 834bf1892f..c06904c0c2 100644 --- a/osu.Game.Rulesets.Osu/Edit/Blueprints/Sliders/Components/PathControlPointPiece.cs +++ b/osu.Game.Rulesets.Osu/Edit/Blueprints/Sliders/Components/PathControlPointPiece.cs @@ -162,8 +162,9 @@ namespace osu.Game.Rulesets.Osu.Edit.Blueprints.Sliders.Components if (ControlPoint == slider.Path.ControlPoints[0]) { // Special handling for the head control point - the position of the slider changes which means the snapped position and time have to be taken into account - var result = snapProvider?.SnapScreenSpacePositionToValidTime(e.MousePosition); - Vector2 movementDelta = (result?.ScreenSpacePosition ?? e.MousePosition) - slider.Position; + var result = snapProvider?.SnapScreenSpacePositionToValidTime(e.ScreenSpaceMousePosition); + + Vector2 movementDelta = Parent.ToLocalSpace(result?.ScreenSpacePosition ?? e.ScreenSpaceMousePosition) - slider.Position; slider.Position += movementDelta; slider.StartTime = result?.Time ?? slider.StartTime; From 5ea33f4c046ffdde090f21c67a9a26772083a24b Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Fri, 22 May 2020 19:23:07 +0900 Subject: [PATCH 9/9] Fix incorrect rounding in DragBar --- osu.Game/Screens/Edit/Compose/Components/Timeline/Timeline.cs | 4 ++-- .../Compose/Components/Timeline/TimelineHitObjectBlueprint.cs | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/osu.Game/Screens/Edit/Compose/Components/Timeline/Timeline.cs b/osu.Game/Screens/Edit/Compose/Components/Timeline/Timeline.cs index ec2b11c0cf..61ed1743a9 100644 --- a/osu.Game/Screens/Edit/Compose/Components/Timeline/Timeline.cs +++ b/osu.Game/Screens/Edit/Compose/Components/Timeline/Timeline.cs @@ -181,8 +181,8 @@ namespace osu.Game.Screens.Edit.Compose.Components.Timeline [Resolved] private IBeatSnapProvider beatSnapProvider { get; set; } - public SnapResult SnapScreenSpacePositionToValidTime(Vector2 position) => - new SnapResult(position, beatSnapProvider.SnapTime(getTimeFromPosition(Content.ToLocalSpace(position)))); + public SnapResult SnapScreenSpacePositionToValidTime(Vector2 screenSpacePosition) => + new SnapResult(screenSpacePosition, beatSnapProvider.SnapTime(getTimeFromPosition(Content.ToLocalSpace(screenSpacePosition)))); private double getTimeFromPosition(Vector2 localPosition) => (localPosition.X / Content.DrawWidth) * track.Length; diff --git a/osu.Game/Screens/Edit/Compose/Components/Timeline/TimelineHitObjectBlueprint.cs b/osu.Game/Screens/Edit/Compose/Components/Timeline/TimelineHitObjectBlueprint.cs index 03e05b75c5..dd2f7a833e 100644 --- a/osu.Game/Screens/Edit/Compose/Components/Timeline/TimelineHitObjectBlueprint.cs +++ b/osu.Game/Screens/Edit/Compose/Components/Timeline/TimelineHitObjectBlueprint.cs @@ -282,7 +282,7 @@ namespace osu.Game.Screens.Edit.Compose.Components.Timeline case IHasRepeats repeatHitObject: // find the number of repeats which can fit in the requested time. var lengthOfOneRepeat = repeatHitObject.Duration / (repeatHitObject.RepeatCount + 1); - var proposedCount = Math.Max(0, (int)((time - hitObject.StartTime) / lengthOfOneRepeat) - 1); + var proposedCount = Math.Max(0, (int)Math.Round((time - hitObject.StartTime) / lengthOfOneRepeat) - 1); if (proposedCount == repeatHitObject.RepeatCount) return;