Fix many behavioural issues and add tests

osu.Game.Rulesets.Catch/Replays/CatchFramedReplayInputHandler.cs

@@ -2,6 +2,7 @@
 // See the LICENCE file in the repository root for full licence text.
 
 using System.Collections.Generic;
+using System.Diagnostics;
 using osu.Framework.Input.StateChanges;
 using osu.Framework.MathUtils;
 using osu.Game.Replays;
@@ -27,7 +28,9 @@ namespace osu.Game.Rulesets.Catch.Replays
                 if (frame == null)
                     return null;
 
-                return Interpolation.ValueAt(CurrentTime, frame.Position, NextFrame.Position, frame.Time, NextFrame.Time);
+                Debug.Assert(CurrentTime != null);
+
+                return Interpolation.ValueAt(CurrentTime.Value, frame.Position, NextFrame.Position, frame.Time, NextFrame.Time);
             }
         }
 

osu.Game.Rulesets.Osu/Replays/OsuFramedReplayInputHandler.cs

@@ -2,6 +2,7 @@
 // See the LICENCE file in the repository root for full licence text.
 
 using System.Collections.Generic;
+using System.Diagnostics;
 using System.Linq;
 using osu.Framework.Input.StateChanges;
 using osu.Framework.MathUtils;
@@ -29,7 +30,9 @@ namespace osu.Game.Rulesets.Osu.Replays
                 if (frame == null)
                     return null;
 
-                return Interpolation.ValueAt(CurrentTime, frame.Position, NextFrame.Position, frame.Time, NextFrame.Time);
+                Debug.Assert(CurrentTime != null);
+
+                return Interpolation.ValueAt(CurrentTime.Value, frame.Position, NextFrame.Position, frame.Time, NextFrame.Time);
             }
         }
 

osu.Game.Tests/NonVisual/FramedReplayinputHandlerTest.cs

@@ -0,0 +1,211 @@
+// Copyright (c) ppy Pty Ltd <contact@ppy.sh>. 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.Game.Replays;
+using osu.Game.Rulesets.Replays;
+
+namespace osu.Game.Tests.NonVisual
+{
+    [TestFixture]
+    public class FramedReplayinputHandlerTest
+    {
+        private Replay replay;
+        private TestInputHandler handler;
+
+        [SetUp]
+        public void SetUp()
+        {
+            handler = new TestInputHandler(replay = new Replay
+            {
+                Frames = new List<ReplayFrame>
+                {
+                    new TestReplayFrame(0),
+                    new TestReplayFrame(1000),
+                    new TestReplayFrame(2000),
+                    new TestReplayFrame(3000, true),
+                    new TestReplayFrame(4000, true),
+                    new TestReplayFrame(5000, true),
+                    new TestReplayFrame(7000, true),
+                    new TestReplayFrame(8000),
+                }
+            });
+        }
+
+        [Test]
+        public void TestNormalPlayback()
+        {
+            Assert.IsNull(handler.CurrentFrame);
+
+            confirmCurrentFrame(null);
+            confirmNextFrame(0);
+
+            setTime(0, 0);
+            confirmCurrentFrame(0);
+            confirmNextFrame(1);
+
+            //if we hit the first frame perfectly, time should progress to it.
+            setTime(1000, 1000);
+            confirmCurrentFrame(1);
+            confirmNextFrame(2);
+
+            //in between non-important frames should progress based on input.
+            setTime(1200, 1200);
+            confirmCurrentFrame(1);
+
+            setTime(1400, 1400);
+            confirmCurrentFrame(1);
+
+            // progressing beyond the next frame should force time to that frame once.
+            setTime(2200, 2000);
+            confirmCurrentFrame(2);
+
+            // second attempt should progress to input time
+            setTime(2200, 2200);
+            confirmCurrentFrame(2);
+
+            // entering important section
+            setTime(3000, 3000);
+            confirmCurrentFrame(3);
+
+            // cannot progress within
+            setTime(3500, null);
+            confirmCurrentFrame(3);
+
+            setTime(4000, 4000);
+            confirmCurrentFrame(4);
+
+            // still cannot progress
+            setTime(4500, null);
+            confirmCurrentFrame(4);
+
+            setTime(5200, 5000);
+            confirmCurrentFrame(5);
+
+            // important section AllowedImportantTimeSpan allowance
+            setTime(5200, 5200);
+            confirmCurrentFrame(5);
+
+            setTime(7200, 7000);
+            confirmCurrentFrame(6);
+
+            setTime(7200, null);
+            confirmCurrentFrame(6);
+
+            // exited important section
+            setTime(8200, 8000);
+            confirmCurrentFrame(7);
+            confirmNextFrame(null);
+
+            setTime(8200, 8200);
+            confirmCurrentFrame(7);
+            confirmNextFrame(null);
+        }
+
+        [Test]
+        public void TestIntroTime()
+        {
+            setTime(-1000, -1000);
+            confirmCurrentFrame(null);
+            confirmNextFrame(0);
+
+            setTime(-500, -500);
+            confirmCurrentFrame(null);
+            confirmNextFrame(0);
+
+            setTime(0, 0);
+            confirmCurrentFrame(0);
+            confirmNextFrame(1);
+        }
+
+        [Test]
+        public void TestBasicRewind()
+        {
+            setTime(3000, 0);
+            setTime(3000, 1000);
+            setTime(3000, 2000);
+            setTime(3000, 3000);
+            confirmCurrentFrame(3);
+            confirmNextFrame(4);
+
+            setTime(1980, 2000);
+            confirmCurrentFrame(2);
+            confirmNextFrame(1);
+
+            setTime(1980, 1980);
+            confirmCurrentFrame(2);
+            confirmNextFrame(1);
+
+            setTime(1200, 1200);
+            confirmCurrentFrame(2);
+            confirmNextFrame(1);
+
+            setTime(-500, 1000);
+            confirmCurrentFrame(1);
+            confirmNextFrame(0);
+
+            setTime(-500, 0);
+            confirmCurrentFrame(0);
+            confirmNextFrame(null);
+
+            setTime(-500, -500);
+            confirmCurrentFrame(0);
+            confirmNextFrame(null);
+        }
+
+        private void setTime(double set, double? expect)
+        {
+            Assert.AreEqual(expect, handler.SetFrameFromTime(set));
+        }
+
+        private void confirmCurrentFrame(int? frame)
+        {
+            if (frame.HasValue)
+            {
+                Assert.IsNotNull(handler.CurrentFrame);
+                Assert.AreEqual(replay.Frames[frame.Value].Time, handler.CurrentFrame.Time);
+            }
+            else
+            {
+                Assert.IsNull(handler.CurrentFrame);
+            }
+        }
+
+        private void confirmNextFrame(int? frame)
+        {
+            if (frame.HasValue)
+            {
+                Assert.IsNotNull(handler.NextFrame);
+                Assert.AreEqual(replay.Frames[frame.Value].Time, handler.NextFrame.Time);
+            }
+            else
+            {
+                Assert.IsNull(handler.NextFrame);
+            }
+        }
+
+        private class TestReplayFrame : ReplayFrame
+        {
+            public readonly bool IsImportant;
+
+            public TestReplayFrame(double time, bool isImportant = false)
+                : base(time)
+            {
+                IsImportant = isImportant;
+            }
+        }
+
+        private class TestInputHandler : FramedReplayInputHandler<TestReplayFrame>
+        {
+            public TestInputHandler(Replay replay)
+                : base(replay)
+            {
+            }
+
+            protected override double AllowedImportantTimeSpan => 1000;
+
+            protected override bool IsImportant(TestReplayFrame frame) => frame?.IsImportant ?? false;
+        }
+    }
+}

osu.Game/Rulesets/Replays/FramedReplayInputHandler.cs

@@ -22,7 +22,7 @@ namespace osu.Game.Rulesets.Replays
         protected List<ReplayFrame> Frames => replay.Frames;
 
         public TFrame CurrentFrame => !HasFrames || !currentFrameIndex.HasValue ? null : (TFrame)Frames[currentFrameIndex.Value];
-        public TFrame NextFrame => !HasFrames ? null : (TFrame)Frames[nextFrameIndex];
+        public TFrame NextFrame => !HasFrames || ((currentDirection > 0 && currentFrameIndex == Frames.Count - 1) || (currentDirection < 0 && currentFrameIndex == 0)) ? null : (TFrame)Frames[nextFrameIndex];
 
         private int? currentFrameIndex;
 
@@ -48,7 +48,10 @@ namespace osu.Game.Rulesets.Replays
 
         private const double sixty_frame_time = 1000.0 / 60;
 
-        protected double CurrentTime { get; private set; }
+        protected virtual double AllowedImportantTimeSpan => sixty_frame_time * 1.2;
+
+        protected double? CurrentTime { get; private set; }
+
         private int currentDirection;
 
         /// <summary>
@@ -64,7 +67,7 @@ namespace osu.Game.Rulesets.Replays
             //a button is in a pressed state
             IsImportant(currentDirection > 0 ? CurrentFrame : NextFrame) &&
             //the next frame is within an allowable time span
-            Math.Abs(CurrentTime - NextFrame?.Time ?? 0) <= sixty_frame_time * 1.2;
+            Math.Abs(CurrentTime - NextFrame?.Time ?? 0) <= AllowedImportantTimeSpan;
 
         protected virtual bool IsImportant(TFrame frame) => false;
 
@@ -77,27 +80,34 @@ namespace osu.Game.Rulesets.Replays
         /// <returns>The usable time value. If null, we should not advance time as we do not have enough data.</returns>
         public override double? SetFrameFromTime(double time)
         {
-            currentDirection = time.CompareTo(CurrentTime);
+            if (!CurrentTime.HasValue)
-            if (currentDirection == 0) currentDirection = 1;
+            {
+                CurrentTime = time;
+                currentDirection = 1;
+            }
+            else
+            {
+                currentDirection = time.CompareTo(CurrentTime);
+                if (currentDirection == 0) currentDirection = 1;
+            }
 
             if (HasFrames)
             {
-                // check if the next frame is in the "future" for the current playback direction
+                // check if the next frame is valid for the current playback direction.
-                if (currentDirection != time.CompareTo(NextFrame.Time))
+                // validity is if the next frame is equal or "earlier"
+                var compare = time.CompareTo(NextFrame?.Time);
+
+                if (compare == 0 || compare == currentDirection)
+                {
+                    if (advanceFrame())
+                        return CurrentTime = CurrentFrame.Time;
+                }
+                else
                 {
                     // if we didn't change frames, we need to ensure we are allowed to run frames in between, else return null.
                     if (inImportantSection)
                         return null;
                 }
-                else if (advanceFrame())
-                {
-                    // If going backwards, we need to execute once _before_ the frame time to reverse any judgements
-                    // that would occur as a result of this frame in forward playback
-                    if (currentDirection == -1)
-                        return CurrentTime = CurrentFrame.Time - 1;
-
-                    return CurrentTime = CurrentFrame.Time;
-                }
             }
 
             return CurrentTime = time;