Handle replay frames with negative time delta appropriately

[tybug]

Note: everything below is conjecture. Someone with access to stable source should confirm. However, I have empirical evidence in support of this (read: I ran a bunch of replays through stable).

Lazer does not currently handle frames with a negative frame time (delta_t) the same as stable. Right now, any frames with a negative frame time are dropped. This matches stable. However, stable also skips all future frames of the replay until the sum total of their frame times is greater than or equal to that negative frame. I think of this as "paying back" the deficit of the negative frame time, and skipping any frames which are used as payment.

Consider the following replay. Note that the notation here uses delta time (difference from last frame), not cumulative time.

delta_t	x	y	deficit	kept (master)	kept (this pr + stable)
10	50	100	0	✅	✅
2_000	400	100	0	✅	✅
2_000	400	200	0	✅	✅
-5_000	150	150	-5_000	❌	❌
500	20	200	-4_500	✅	❌
500	400	150	-4_000	✅	❌
500	80	300	-3_500	✅	❌
2_000	250	250	-1_500	✅	❌
6_000	10	10	0	✅	✅

You can see this behavior in action in this replay (map), with replay data corresponding to the table above. In lazer, the replay jumps around, because it is playing the intermediate frames. In stable, you can see visually that the playback only has 4 (technically 5: see below) frames, as it dropped the intermediate ones.

Now. There's an additional complication which I don't address in this PR. Stable not only drops frames in the fashion described above, but also inserts a new frame when the deficit turns from negative to positive. You can see this in the replay linked above: there are only 4 viable frames present in the osr, but playing back in stable results in 5 frames, with an extra frame inserted at (210, 210) and at the same time as the (400, 200) frame.

I have a branch which computes this special frame, seemingly correctly for all replays I've tried. But the logic is convoluted: it involves linear interpolations and tracking the last frame before a deficit is introduced. Not to mention I have no idea what ButtonPress to assign to the frame. I find it hard to believe that this is the exact logic used by stable, so I'm holding off on cleaning it up and PRing it until I get confirmation on the direction forward here.

This PR may fix some longstanding "lazer playback doesn't match stable" issues.

[bdach]

Relevant stable source:

        internal void ReadReplayData(string replayData)
        {
            if (replayData.Length > 0)
            {
                string[] replaylines = replayData.Split(',');


                bReplayFrame lastF;
                if (Replay.Count > 0)
                    lastF = Replay[Replay.Count - 1];
                else
                    lastF = new bReplayFrame(0, 0, 0, pButtonState.None);


                foreach (string replayline in replaylines)
                {
                    if (replayline.Length == 0)
                        continue;


                    string[] data = replayline.Split('|');


                    if (data.Length < 4)
                        continue;
                    if (data[0] == @"-12345")
                    {
                        Seed = int.Parse(data[3]);
                        continue;
                    }


                    pButtonState buttons = (pButtonState)Enum.Parse(typeof(pButtonState), data[3]);


                    bReplayFrame newF = new bReplayFrame(int.Parse(data[0]) + lastF.time,
                        float.Parse(data[1], GameBase.nfi),
                        float.Parse(data[2], GameBase.nfi),
                        buttons);
                    Replay.Add(newF);


                    lastF = newF;
                }
            }
        }

                            if (runningSlow && ReplayScore.AllowFrameSkipping)
                                while (ReplayFrame < count - 1 && !ReplayScore.Replay[ReplayFrame].mouseLeft &&
                                       !ReplayScore.Replay[ReplayFrame].mouseRight &&
                                       ReplayScore.Replay[ReplayFrame + 1].time <= AudioEngine.Time &&
                                       ReplayScore.Replay[ReplayFrame].mouseLeft == leftButtonLastBool &&
                                       ReplayScore.Replay[ReplayFrame].mouseRight == rightButtonLastBool &&
                                       ReplayScore.Replay[ReplayFrame].buttonState == lastButtonState &&
                                       !Player.IsSpinning)
                                {
                                    if (AudioEngine.AudioFrequency == AudioEngine.InitialFrequency) Debug.Print(@"replay frame {0} skipped!!!!", ReplayFrame);
                                    ReplayFrame += direction;
                                }

which appears to confirm the negative skip hypothesis.

I don't know about cc6646c, though. This is what is in the replay of the failing test:

so why is the test asserting that the first frame should start at 48 rather than 31? What I basically did was

diff --git a/osu.Game.Tests/Beatmaps/Formats/LegacyScoreDecoderTest.cs b/osu.Game.Tests/Beatmaps/Formats/LegacyScoreDecoderTest.cs
index 93cda34ef7..cf2f0576c9 100644
--- a/osu.Game.Tests/Beatmaps/Formats/LegacyScoreDecoderTest.cs
+++ b/osu.Game.Tests/Beatmaps/Formats/LegacyScoreDecoderTest.cs
@@ -92,8 +92,9 @@ public void TestDecodeTaikoReplay()
         [TestCase(LegacyBeatmapDecoder.LATEST_VERSION, false)]
         public void TestLegacyBeatmapReplayOffsetsDecode(int beatmapVersion, bool offsetApplied)
         {
-            const double first_frame_time = 48;
-            const double second_frame_time = 65;
+            const double first_frame_time = 31;
+            const double second_frame_time = 48;
+            const double third_frame_time = 65;
 
             var decoder = new TestLegacyScoreDecoder(beatmapVersion);
 
@@ -103,6 +104,7 @@ public void TestLegacyBeatmapReplayOffsetsDecode(int beatmapVersion, bool offset
 
                 Assert.That(score.Replay.Frames[0].Time, Is.EqualTo(first_frame_time + (offsetApplied ? LegacyBeatmapDecoder.EARLY_VERSION_TIMING_OFFSET : 0)));
                 Assert.That(score.Replay.Frames[1].Time, Is.EqualTo(second_frame_time + (offsetApplied ? LegacyBeatmapDecoder.EARLY_VERSION_TIMING_OFFSET : 0)));
+                Assert.That(score.Replay.Frames[2].Time, Is.EqualTo(third_frame_time + (offsetApplied ? LegacyBeatmapDecoder.EARLY_VERSION_TIMING_OFFSET : 0)));
             }
         }
 
diff --git a/osu.Game/Scoring/Legacy/LegacyScoreDecoder.cs b/osu.Game/Scoring/Legacy/LegacyScoreDecoder.cs
index f91c96efb6..897bc52bc4 100644
--- a/osu.Game/Scoring/Legacy/LegacyScoreDecoder.cs
+++ b/osu.Game/Scoring/Legacy/LegacyScoreDecoder.cs
@@ -274,7 +274,7 @@ private void readLegacyReplay(Replay replay, StreamReader reader)
             // is equal to or greater than the time of that negative frame.
             // This value will be negative if we are in a time deficit, ie we have a negative frame that must be paid back.
             // Otherwise it will be 0.
-            float timeDeficit = 0;
+            float? timeDeficit = null;
 
             string[] frames = reader.ReadToEnd().Split(',');
 
@@ -303,8 +303,7 @@ private void readLegacyReplay(Replay replay, StreamReader reader)
                     // ignore these frames as they serve no real purpose (and can even mislead ruleset-specific handlers - see mania)
                     continue;
 
-                timeDeficit += diff;
-                timeDeficit = Math.Min(0, timeDeficit);
+                timeDeficit = timeDeficit == null ? 0 : Math.Min(0, timeDeficit.Value + diff);
 
                 // still paying back the deficit from a negative frame. Skip this frame.
                 // Todo: At some point we probably want to rewind and play back the negative-time frames

which seems both less complex and more correct?

[tybug]

so why is the test asserting that the first frame should start at 48 rather than 31?

why do you think the first frame should start at 31? The way I read it, from your screenshot:

frame	delta_t	t
1	0	0
2	3994	3994
3	-3963	31
4	17	48

except frames 1 and 2 get dropped because they're the two skip frames, and frame 3 gets dropped because it's negative. Which leaves frame 4 as the new frame 1 for the purposes of the test, with a time of 48.

I think the disagreement is in whether to skip frame 3 above - your diff seems roughly equivalent to cc6646c, except it doesn't skip frame 3 even if it's negative.

I honestly don't know whether skipping it or not is correct, but my guess would be that it should be skipped. Haven't checked against stable yet though.

[bdach]

why do you think the first frame should start at 31?

I guess I don't have a good answer to that. It's mostly a hunch based on the fact that I don't see stable doing anything explicit with the skip frames, so I expect this to work in a "sorta kinda" simple way.

I would go and empirically check but I am hoist on my own petard as the failing replay was made by me, on a custom beatmap, which is now lost to time, so there's no way to play the thing back on stable right now. Will probably have to go and check with another replay...

[tybug]

I've tested against stable, and things are a bit weird. By modifying https://osu.ppy.sh/scores/osu/2991509422's third frame to have a keypress of M1 and a location of (10, 10) (modified.osr), you can see that the M1 counter increments (or rather, turns from "M1" to a 0, as it's during a break section). So the frame is being played back in some fashion. But, the cursor doesn't appear at (10, 10) by my eye, even for a frame. So it's...not being played back?

Honestly, I'm happy to chalk this up to "nobody will ever notice", pick one or the other, and call it a day. Including or not including this negative frame has no actual impact on gameplay: it will never be on a hitobject since it's during break time, and I don't think it will ever have a keypress either.

[bdach]

By modifying https://osu.ppy.sh/scores/osu/2991509422's third frame to have a keypress of M1 and a location of (10, 10) (modified.osr), you can see that the M1 counter increments (or rather, turns from "M1" to a 0, as it's during a break section). So the frame is being played back in some fashion. But, the cursor doesn't appear at (10, 10) by my eye, even for a frame. So it's...not being played back?

I've checked this replay, and I sometimes (not every playback) can observe a single cursor star particle falling out of what looks about right for (10, 10):

star.mp4

The debugger also definitively shows the frame being played back:

The fact that the cursor doesn't appear there is likely masked by fade in transforms somewhere. I haven't found precisely where they are, but I don't really think that is necessary in light of the existing evidence above.

[tybug]

Thanks for investigating! That's more than enough proof for me. I've made it so the initial frame does not get skipped.

In fact, your proposed diff prompted me to look further into this, and in my testing it appears that stable ignores the time of only the first negative frame it encounters among the first 3 frames of the replay. For instance, frames with times (-10, -10, 0) results in a deficit of -10 (first frame not counted), while (-10, 0, 0) results in a deficit of 0, as does (0, -10, 0) and (0, 0, -10). I've changed the code accordingly.

I spent quite a while looking at adding tests for this, but it would require manually creating test osrs with specific negative frames matching hitobjs in a test(scene) beatmap, which is a fair amount of work. I'm willing to do this if you think it's worthwhile.

[bdach]

For instance, frames with times (-10, -10, 0) results in a deficit of -10 (first frame not counted), while (-10, 0, 0) results in a deficit of 0, as does (0, -10, 0) and (0, 0, -10). I've changed the code accordingly.

Are you able to provide the methodology of testing this? In other words, do you have replays prepared that can be used to check this or something? This assertion is again surprising to me as I see no evidence of stable doing anything of that sort in source, so I want to verify independently.

[tybug]

Was writing up the methodology and replays but I can't even reproduce my own results anymore...I dunno if I was drunk while testing or what. There was something weird going on with the replays I was testing, but my brain might have ascribed this arbitrary rule to explain it without proper backing.

I'll look into this properly after coe.

[bdach]

So necro time...

After some further digging I found this in stable:

            //Fixes a bug that is hard-recorded into many replays where the first two invisible-mouse frames are the wrong way around.
            if (InputManager.ReplayScore.Replay.Count >= 2 && InputManager.ReplayScore.Replay[1].time < InputManager.ReplayScore.Replay[0].time)
            {
                InputManager.ReplayScore.Replay[1].time = InputManager.ReplayScore.Replay[0].time;
                InputManager.ReplayScore.Replay[0].time = 0;
            }

            //Fixes a bug that is hard-recorded into many replays where the first two invisible-mouse frames are happening after the replay started (if the replay starts before 0)
            if (InputManager.ReplayScore.Replay.Count >= 3 && InputManager.ReplayScore.Replay[0].time > InputManager.ReplayScore.Replay[2].time)
                InputManager.ReplayScore.Replay[0].time = InputManager.ReplayScore.Replay[1].time = InputManager.ReplayScore.Replay[2].time;

which may explain the first-three-frames weirdness as described above?

I'll attempt to put that into a test case...

[bdach]

I wrote the tests and rewrote the code in a way that seems simpler and should match the stable quirks (990a07a), but this probably needs more testing from me still.

@tybug I'm not sure if you're inclined to go back to this ancient PR, but if you could take a look at what I did here and maybe see if this checks out against your recollections of what was happening then that'd be swell. @ppy/team-client could maybe appreciate a look from one of you too as to whether you'd even consider merging this in this state.

[tybug]

I'm honestly kind of shocked this negative time business never came up in a real user issue. (or maybe it has and we never figured it out.) I still think this should probably be fixed sooner than later though! Some stable replays are subtly broken because of this.

which may explain the first-three-frames weirdness

This is fascinating. I never really played around with early negative frames, so I'm not confident on what stable actually does here. Whereas I am pretty confident in the general negative frame assessment in the pr description. I guess that is to say I don't really have any insight here.

[frenzibyte]

Shouldn't we also be worried about the other states that stable checks for in https://github.com/peppy/osu-stable-reference/blob/7519cafd1823f1879c0d9c991ba0e5c7fd3bfa02/osu!/Input/InputManager.cs#L640-L651? (e.g. mouse and button states)

[bdach]

Out of scope of this PR. I'm not even sure what that does, or why stable has that logic disabled for catch and mania (go check AllowFrameSkipping values). I'd rather not port that across if it can be avoided at all.

[tybug]

In my experience the keypress states don't matter for this negative frame logic, but I've never explicitly checked, fwiw.

[frenzibyte]

One thing I don't understand here is when does stable register negative frame deltas in replays. The replay you have attached in description does not perfectly match between stable and this PR (presumably because of the "interpolated" frame mentioned in OP), and the proposed logic does not closely follow stable as mentioned in #24138 (comment), and I cannot find any existing issue that suffers from incorrect negative replay playback as far as I checked, so I don't really know where this is helping exactly. Abstaining from further review.

[smoogipoo]

This makes sense to me.