Merge pull request #2827 from smoogipoo/editor-seek-snapping-fix

Fix editor not always scrolling beyond timing point beats

osu.Game.Tests/Visual/TestCaseEditorSeekSnapping.cs

@@ -1,8 +1,7 @@
 // Copyright (c) 2007-2018 ppy Pty Ltd <contact@ppy.sh>.
 // Licensed under the MIT Licence - https://raw.githubusercontent.com/ppy/osu/master/LICENCE
 
-using System;
+using NUnit.Framework;
-using System.Collections.Generic;
 using osu.Framework.Allocation;
 using osu.Framework.Extensions.Color4Extensions;
 using osu.Framework.Graphics;
@@ -10,9 +9,6 @@ using osu.Framework.Graphics.Containers;
 using osu.Framework.Graphics.Shapes;
 using osu.Game.Beatmaps;
 using osu.Game.Beatmaps.ControlPoints;
-using osu.Game.Rulesets;
-using osu.Game.Rulesets.Edit;
-using osu.Game.Rulesets.Edit.Tools;
 using osu.Game.Rulesets.Osu.Objects;
 using osu.Game.Tests.Beatmaps;
 using OpenTK;
@@ -20,10 +16,9 @@ using OpenTK.Graphics;
 
 namespace osu.Game.Tests.Visual
 {
+    [TestFixture]
     public class TestCaseEditorSeekSnapping : EditorClockTestCase
     {
-        public override IReadOnlyList<Type> RequiredTypes => new[] { typeof(HitObjectComposer) };
-
         public TestCaseEditorSeekSnapping()
         {
             BeatDivisor.Value = 4;
@@ -56,22 +51,13 @@ namespace osu.Game.Tests.Visual
             Beatmap.Value = new TestWorkingBeatmap(testBeatmap);
 
             Child = new TimingPointVisualiser(testBeatmap, 5000) { Clock = Clock };
-
-            testSeekNoSnapping();
-            testSeekSnappingOnBeat();
-            testSeekSnappingInBetweenBeat();
-            testSeekForwardNoSnapping();
-            testSeekForwardSnappingOnBeat();
-            testSeekForwardSnappingFromInBetweenBeat();
-            testSeekBackwardSnappingOnBeat();
-            testSeekBackwardSnappingFromInBetweenBeat();
-            testSeekingWithFloatingPointBeatLength();
         }
 
         /// <summary>
         /// Tests whether time is correctly seeked without snapping.
         /// </summary>
-        private void testSeekNoSnapping()
+        [Test]
+        public void TestSeekNoSnapping()
         {
             reset();
 
@@ -94,7 +80,8 @@ namespace osu.Game.Tests.Visual
         /// Tests whether seeking to exact beat times puts us on the beat time.
         /// These are the white/yellow ticks on the graph.
         /// </summary>
-        private void testSeekSnappingOnBeat()
+        [Test]
+        public void TestSeekSnappingOnBeat()
         {
             reset();
 
@@ -117,9 +104,9 @@ namespace osu.Game.Tests.Visual
         /// <summary>
         /// Tests whether seeking to somewhere in the middle between beats puts us on the expected beats.
         /// For example, snapping between a white/yellow beat should put us on either the yellow or white, depending on which one we're closer too.
-        /// If
         /// </summary>
-        private void testSeekSnappingInBetweenBeat()
+        [Test]
+        public void TestSeekSnappingInBetweenBeat()
         {
             reset();
 
@@ -140,7 +127,8 @@ namespace osu.Game.Tests.Visual
         /// <summary>
         /// Tests that when seeking forward with no beat snapping, beats are never explicitly snapped to, nor the next timing point (if we've skipped it).
         /// </summary>
-        private void testSeekForwardNoSnapping()
+        [Test]
+        public void TestSeekForwardNoSnapping()
         {
             reset();
 
@@ -159,7 +147,8 @@ namespace osu.Game.Tests.Visual
         /// <summary>
         /// Tests that when seeking forward with beat snapping, all beats are snapped to and timing points are never skipped.
         /// </summary>
-        private void testSeekForwardSnappingOnBeat()
+        [Test]
+        public void TestSeekForwardSnappingOnBeat()
         {
             reset();
 
@@ -181,7 +170,8 @@ namespace osu.Game.Tests.Visual
         /// Tests that when seeking forward from in-between two beats, the next beat or timing point is snapped to, and no beats are skipped.
         /// This will also test being extremely close to the next beat/timing point, to ensure rounding is not an issue.
         /// </summary>
-        private void testSeekForwardSnappingFromInBetweenBeat()
+        [Test]
+        public void TestSeekForwardSnappingFromInBetweenBeat()
         {
             reset();
 
@@ -214,21 +204,20 @@ namespace osu.Game.Tests.Visual
         /// <summary>
         /// Tests that when seeking backward with no beat snapping, beats are never explicitly snapped to, nor the next timing point (if we've skipped it).
         /// </summary>
-        private void testSeekBackwardNoSnapping()
+        [Test]
+        public void TestSeekBackwardNoSnapping()
         {
             reset();
 
             AddStep("Seek(450)", () => Clock.Seek(450));
             AddStep("SeekBackward", () => Clock.SeekBackward());
-            AddAssert("Time = 425", () => Clock.CurrentTime == 425);
+            AddAssert("Time = 400", () => Clock.CurrentTime == 400);
             AddStep("SeekBackward", () => Clock.SeekBackward());
-            AddAssert("Time = 375", () => Clock.CurrentTime == 375);
+            AddAssert("Time = 350", () => Clock.CurrentTime == 350);
             AddStep("SeekBackward", () => Clock.SeekBackward());
-            AddAssert("Time = 325", () => Clock.CurrentTime == 325);
+            AddAssert("Time = 150", () => Clock.CurrentTime == 150);
             AddStep("SeekBackward", () => Clock.SeekBackward());
-            AddAssert("Time = 125", () => Clock.CurrentTime == 125);
+            AddAssert("Time = 50", () => Clock.CurrentTime == 50);
-            AddStep("SeekBackward", () => Clock.SeekBackward());
-            AddAssert("Time = 25", () => Clock.CurrentTime == 25);
             AddStep("SeekBackward", () => Clock.SeekBackward());
             AddAssert("Time = 0", () => Clock.CurrentTime == 0);
         }
@@ -236,7 +225,8 @@ namespace osu.Game.Tests.Visual
         /// <summary>
         /// Tests that when seeking backward with beat snapping, all beats are snapped to and timing points are never skipped.
         /// </summary>
-        private void testSeekBackwardSnappingOnBeat()
+        [Test]
+        public void TestSeekBackwardSnappingOnBeat()
         {
             reset();
 
@@ -259,7 +249,8 @@ namespace osu.Game.Tests.Visual
         /// Tests that when seeking backward from in-between two beats, the previous beat or timing point is snapped to, and no beats are skipped.
         /// This will also test being extremely close to the previous beat/timing point, to ensure rounding is not an issue.
         /// </summary>
-        private void testSeekBackwardSnappingFromInBetweenBeat()
+        [Test]
+        public void TestSeekBackwardSnappingFromInBetweenBeat()
         {
             reset();
 
@@ -280,7 +271,8 @@ namespace osu.Game.Tests.Visual
         /// <summary>
         /// Tests that there are no rounding issues when snapping to beats within a timing point with a floating-point beatlength.
         /// </summary>
-        private void testSeekingWithFloatingPointBeatLength()
+        [Test]
+        public void TestSeekingWithFloatingPointBeatLength()
         {
             reset();
 
@@ -288,7 +280,7 @@ namespace osu.Game.Tests.Visual
 
             AddStep("Seek(0)", () => Clock.Seek(0));
 
-            for (int i = 0; i < 20; i++)
+            for (int i = 0; i < 9; i++)
             {
                 AddStep("SeekForward, Snap", () =>
                 {
@@ -298,7 +290,7 @@ namespace osu.Game.Tests.Visual
                 AddAssert("Time > lastTime", () => Clock.CurrentTime > lastTime);
             }
 
-            for (int i = 0; i < 20; i++)
+            for (int i = 0; i < 9; i++)
             {
                 AddStep("SeekBackward, Snap", () =>
                 {
@@ -316,16 +308,6 @@ namespace osu.Game.Tests.Visual
             AddStep("Reset", () => Clock.Seek(0));
         }
 
-        private class TestHitObjectComposer : HitObjectComposer
-        {
-            public TestHitObjectComposer(Ruleset ruleset)
-                : base(ruleset)
-            {
-            }
-
-            protected override IReadOnlyList<ICompositionTool> CompositionTools => new ICompositionTool[0];
-        }
-
         private class TimingPointVisualiser : CompositeDrawable
         {
             private readonly double length;

osu.Game/Screens/Edit/Screens/Compose/Timeline/Timeline.cs

@@ -62,7 +62,12 @@ namespace osu.Game.Screens.Edit.Screens.Compose.Timeline
         }
 
         /// <summary>
-        /// The track's time in the previous frame.
+        /// The timeline's scroll position in the last frame.
+        /// </summary>
+        private float lastScrollPosition;
+
+        /// <summary>
+        /// The track time in the last frame.
         /// </summary>
         private double lastTrackTime;
 
@@ -83,49 +88,51 @@ namespace osu.Game.Screens.Edit.Screens.Compose.Timeline
             // The extrema of track time should be positioned at the centre of the container when scrolled to the start or end
             Content.Margin = new MarginPadding { Horizontal = DrawWidth / 2 };
 
-            if (handlingDragInput)
+            // This needs to happen after transforms are updated, but before the scroll position is updated in base.UpdateAfterChildren
-            {
+            if (adjustableClock.IsRunning)
-                // The user is dragging - the track should always follow the timeline
-                seekTrackToCurrent();
-            }
-            else if (adjustableClock.IsRunning)
-            {
-                // If the user hasn't provided mouse input but the track is running, always follow the track
                 scrollToTrackTime();
-            }
+        }
-            else
+
+        protected override void UpdateAfterChildren()
+        {
+            base.UpdateAfterChildren();
+
+            if (handlingDragInput)
+                seekTrackToCurrent();
+            else if (!adjustableClock.IsRunning)
             {
-                // The track isn't playing, so we want to smooth-scroll once more, and re-enable wheel scrolling
+                // The track isn't running. There are two cases we have to be wary of:
-                // There are two cases we have to be wary of:
+                // 1) The user flick-drags on this timeline: We want the track to follow us
-                // 1) The user scrolls on this timeline: We want the track to follow us
                 // 2) The user changes the track time through some other means (scrolling in the editor or overview timeline): We want to follow the track time
 
-                // The simplest way to cover both cases is by checking that inter-frame track times are identical
+                // The simplest way to cover both cases is by checking whether the scroll position has changed and the audio hasn't been changed externally
-                if (adjustableClock.CurrentTime == lastTrackTime)
+                if (Current != lastScrollPosition && adjustableClock.CurrentTime == lastTrackTime)
-                {
-                    // The track hasn't been seeked externally
                     seekTrackToCurrent();
-                }
                 else
-                {
-                    // The track has been seeked externally
                     scrollToTrackTime();
-                }
             }
 
+            lastScrollPosition = Current;
             lastTrackTime = adjustableClock.CurrentTime;
+        }
 
-            void seekTrackToCurrent()
+        private void seekTrackToCurrent()
-            {
+        {
-                if (!(Beatmap.Value.Track is TrackVirtual))
+            var track = Beatmap.Value.Track;
-                    adjustableClock.Seek(Current / Content.DrawWidth * Beatmap.Value.Track.Length);
+            if (track is TrackVirtual || !track.IsLoaded)
-            }
+                return;
 
-            void scrollToTrackTime()
+            if (!(Beatmap.Value.Track is TrackVirtual))
-            {
+                adjustableClock.Seek(Current / Content.DrawWidth * Beatmap.Value.Track.Length);
-                if (!(Beatmap.Value.Track is TrackVirtual))
+        }
-                    ScrollTo((float)(adjustableClock.CurrentTime / Beatmap.Value.Track.Length) * Content.DrawWidth, false);
+
-            }
+        private void scrollToTrackTime()
+        {
+            var track = Beatmap.Value.Track;
+            if (track is TrackVirtual || !track.IsLoaded)
+                return;
+
+            ScrollTo((float)(adjustableClock.CurrentTime / Beatmap.Value.Track.Length) * Content.DrawWidth, false);
         }
 
         protected override bool OnMouseDown(InputState state, MouseDownEventArgs args)