From 00250972c34caba4c5f1ab8dbdd29fa0ba80c697 Mon Sep 17 00:00:00 2001 From: Liam DeVoe Date: Thu, 6 Jul 2023 02:31:12 -0400 Subject: [PATCH 1/9] skip frames after a negative frame until the negative time is "paid back" --- osu.Game/Scoring/Legacy/LegacyScoreDecoder.cs | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-) diff --git a/osu.Game/Scoring/Legacy/LegacyScoreDecoder.cs b/osu.Game/Scoring/Legacy/LegacyScoreDecoder.cs index c6461840aa..f91c96efb6 100644 --- a/osu.Game/Scoring/Legacy/LegacyScoreDecoder.cs +++ b/osu.Game/Scoring/Legacy/LegacyScoreDecoder.cs @@ -269,6 +269,13 @@ namespace osu.Game.Scoring.Legacy float lastTime = beatmapOffset; ReplayFrame currentFrame = null; + // the negative time amount that must be "paid back" by positive frames before we start including frames again. + // When a negative frame occurs in a replay, all future frames are skipped until the sum total of their times + // 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; + string[] frames = reader.ReadToEnd().Split(','); for (int i = 0; i < frames.Length; i++) @@ -296,9 +303,13 @@ namespace osu.Game.Scoring.Legacy // 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); + + // 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 // but for now we'll achieve equal playback to stable by skipping negative frames - if (diff < 0) + if (timeDeficit < 0) continue; currentFrame = convertFrame(new LegacyReplayFrame(lastTime, From cc6646c82b13e021514a0465b118383d8e96ba7f Mon Sep 17 00:00:00 2001 From: Liam DeVoe Date: Thu, 6 Jul 2023 17:13:33 -0400 Subject: [PATCH 2/9] properly handle negative frame before a break this was causing replay data before the skip to be...skipped. --- osu.Game/Scoring/Legacy/LegacyScoreDecoder.cs | 20 ++++++++++++++++--- 1 file changed, 17 insertions(+), 3 deletions(-) diff --git a/osu.Game/Scoring/Legacy/LegacyScoreDecoder.cs b/osu.Game/Scoring/Legacy/LegacyScoreDecoder.cs index f91c96efb6..63465652e8 100644 --- a/osu.Game/Scoring/Legacy/LegacyScoreDecoder.cs +++ b/osu.Game/Scoring/Legacy/LegacyScoreDecoder.cs @@ -267,6 +267,7 @@ namespace osu.Game.Scoring.Legacy private void readLegacyReplay(Replay replay, StreamReader reader) { float lastTime = beatmapOffset; + bool skipFramesPresent = false; ReplayFrame currentFrame = null; // the negative time amount that must be "paid back" by positive frames before we start including frames again. @@ -298,18 +299,31 @@ namespace osu.Game.Scoring.Legacy lastTime += diff; if (i < 2 && mouseX == 256 && mouseY == -500) + { // at the start of the replay, stable places two replay frames, at time 0 and SkipBoundary - 1, respectively. // both frames use a position of (256, -500). // ignore these frames as they serve no real purpose (and can even mislead ruleset-specific handlers - see mania) + skipFramesPresent = true; continue; + } - timeDeficit += diff; - timeDeficit = Math.Min(0, timeDeficit); + // if the skip frames inserted by stable are present, the third frame will have a large negative time + // roughly equal to SkipBoundary. We don't want this to count towards the deficit: doing so would cause + // the replay data before the skip to be, well, skipped. + // In other words, this frame, if present, is a different kind of negative frame. It sets the "offset" + // for the beginning of the replay. This is the only negative frame to be handled in such a way. + bool isNegativeBreakFrame = i == 2 && skipFramesPresent && diff < 0; + + if (!isNegativeBreakFrame) + { + timeDeficit += diff; + timeDeficit = Math.Min(0, timeDeficit); + } // 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 // but for now we'll achieve equal playback to stable by skipping negative frames - if (timeDeficit < 0) + if (timeDeficit < 0 || isNegativeBreakFrame) continue; currentFrame = convertFrame(new LegacyReplayFrame(lastTime, From 217b07810fb497f571fadfeb4d015394a43075d0 Mon Sep 17 00:00:00 2001 From: Liam DeVoe Date: Thu, 27 Jul 2023 02:12:21 -0400 Subject: [PATCH 3/9] don't skip the negative break frame investigation reveals this frame is played back by stable --- osu.Game/Scoring/Legacy/LegacyScoreDecoder.cs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/osu.Game/Scoring/Legacy/LegacyScoreDecoder.cs b/osu.Game/Scoring/Legacy/LegacyScoreDecoder.cs index 63465652e8..8b1b24ce95 100644 --- a/osu.Game/Scoring/Legacy/LegacyScoreDecoder.cs +++ b/osu.Game/Scoring/Legacy/LegacyScoreDecoder.cs @@ -1,4 +1,4 @@ -// Copyright (c) ppy Pty Ltd . Licensed under the MIT Licence. +// Copyright (c) ppy Pty Ltd . Licensed under the MIT Licence. // See the LICENCE file in the repository root for full licence text. #nullable disable @@ -323,7 +323,7 @@ namespace osu.Game.Scoring.Legacy // 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 // but for now we'll achieve equal playback to stable by skipping negative frames - if (timeDeficit < 0 || isNegativeBreakFrame) + if (timeDeficit < 0) continue; currentFrame = convertFrame(new LegacyReplayFrame(lastTime, From a93561cab05807be032e963b37092ff349f12fc1 Mon Sep 17 00:00:00 2001 From: Liam DeVoe Date: Thu, 27 Jul 2023 02:12:43 -0400 Subject: [PATCH 4/9] remove resolved comment --- osu.Game/Scoring/Legacy/LegacyScoreDecoder.cs | 2 -- 1 file changed, 2 deletions(-) diff --git a/osu.Game/Scoring/Legacy/LegacyScoreDecoder.cs b/osu.Game/Scoring/Legacy/LegacyScoreDecoder.cs index 8b1b24ce95..79224b7d4f 100644 --- a/osu.Game/Scoring/Legacy/LegacyScoreDecoder.cs +++ b/osu.Game/Scoring/Legacy/LegacyScoreDecoder.cs @@ -321,8 +321,6 @@ namespace osu.Game.Scoring.Legacy } // 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 - // but for now we'll achieve equal playback to stable by skipping negative frames if (timeDeficit < 0) continue; From 61760f614a9900e5cd71a298a8979ca69d9b8eb0 Mon Sep 17 00:00:00 2001 From: Liam DeVoe Date: Thu, 27 Jul 2023 16:34:18 -0400 Subject: [PATCH 5/9] fix legacy score decode tests for negative frame --- .../Beatmaps/Formats/LegacyScoreDecoderTest.cs | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/osu.Game.Tests/Beatmaps/Formats/LegacyScoreDecoderTest.cs b/osu.Game.Tests/Beatmaps/Formats/LegacyScoreDecoderTest.cs index 93cda34ef7..89b6d76e54 100644 --- a/osu.Game.Tests/Beatmaps/Formats/LegacyScoreDecoderTest.cs +++ b/osu.Game.Tests/Beatmaps/Formats/LegacyScoreDecoderTest.cs @@ -92,17 +92,20 @@ namespace osu.Game.Tests.Beatmaps.Formats [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); using (var resourceStream = TestResources.OpenResource("Replays/mania-replay.osr")) { var score = decoder.Parse(resourceStream); + int offset = offsetApplied ? LegacyBeatmapDecoder.EARLY_VERSION_TIMING_OFFSET : 0; - 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[0].Time, Is.EqualTo(first_frame_time + offset)); + Assert.That(score.Replay.Frames[1].Time, Is.EqualTo(second_frame_time + offset)); + Assert.That(score.Replay.Frames[2].Time, Is.EqualTo(third_frame_time + offset)); } } From 7d174dd8bb4998ac264463067b798fae14541f0c Mon Sep 17 00:00:00 2001 From: Liam DeVoe Date: Thu, 27 Jul 2023 17:20:54 -0400 Subject: [PATCH 6/9] dont count any of first three frames towards time deficit --- osu.Game/Scoring/Legacy/LegacyScoreDecoder.cs | 21 ++++++++----------- 1 file changed, 9 insertions(+), 12 deletions(-) diff --git a/osu.Game/Scoring/Legacy/LegacyScoreDecoder.cs b/osu.Game/Scoring/Legacy/LegacyScoreDecoder.cs index 79224b7d4f..eceaada399 100644 --- a/osu.Game/Scoring/Legacy/LegacyScoreDecoder.cs +++ b/osu.Game/Scoring/Legacy/LegacyScoreDecoder.cs @@ -267,7 +267,6 @@ namespace osu.Game.Scoring.Legacy private void readLegacyReplay(Replay replay, StreamReader reader) { float lastTime = beatmapOffset; - bool skipFramesPresent = false; ReplayFrame currentFrame = null; // the negative time amount that must be "paid back" by positive frames before we start including frames again. @@ -299,22 +298,20 @@ namespace osu.Game.Scoring.Legacy lastTime += diff; if (i < 2 && mouseX == 256 && mouseY == -500) - { // at the start of the replay, stable places two replay frames, at time 0 and SkipBoundary - 1, respectively. // both frames use a position of (256, -500). // ignore these frames as they serve no real purpose (and can even mislead ruleset-specific handlers - see mania) - skipFramesPresent = true; continue; - } - // if the skip frames inserted by stable are present, the third frame will have a large negative time - // roughly equal to SkipBoundary. We don't want this to count towards the deficit: doing so would cause - // the replay data before the skip to be, well, skipped. - // In other words, this frame, if present, is a different kind of negative frame. It sets the "offset" - // for the beginning of the replay. This is the only negative frame to be handled in such a way. - bool isNegativeBreakFrame = i == 2 && skipFramesPresent && diff < 0; - - if (!isNegativeBreakFrame) + // negative frames are only counted towards the deficit after the very beginning of the replay. + // When the two skip frames are present (see directly above), the third frame will have a large + // negative time roughly equal to SkipBoundary. This shouldn't be counted towards the deficit, otherwise + // any replay data before the skip would be, well, skipped. + // + // On testing against stable it appears that stable ignores the negative time of *any* of the first + // three frames, regardless of if the skip frames are present. Hence the condition here. + // But this may be incorrect and need to be revisited later. + if (i > 2) { timeDeficit += diff; timeDeficit = Math.Min(0, timeDeficit); From 04ef04b9026dfe84c323e3d08204f5a722fb5d74 Mon Sep 17 00:00:00 2001 From: Liam DeVoe Date: Thu, 27 Jul 2023 21:12:08 -0400 Subject: [PATCH 7/9] only ignore the first negative frame among the first 3 replay frames --- osu.Game/Scoring/Legacy/LegacyScoreDecoder.cs | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-) diff --git a/osu.Game/Scoring/Legacy/LegacyScoreDecoder.cs b/osu.Game/Scoring/Legacy/LegacyScoreDecoder.cs index eceaada399..fdeda24c75 100644 --- a/osu.Game/Scoring/Legacy/LegacyScoreDecoder.cs +++ b/osu.Game/Scoring/Legacy/LegacyScoreDecoder.cs @@ -267,6 +267,7 @@ namespace osu.Game.Scoring.Legacy private void readLegacyReplay(Replay replay, StreamReader reader) { float lastTime = beatmapOffset; + bool negativeFrameEncounted = false; ReplayFrame currentFrame = null; // the negative time amount that must be "paid back" by positive frames before we start including frames again. @@ -308,15 +309,19 @@ namespace osu.Game.Scoring.Legacy // negative time roughly equal to SkipBoundary. This shouldn't be counted towards the deficit, otherwise // any replay data before the skip would be, well, skipped. // - // On testing against stable it appears that stable ignores the negative time of *any* of the first - // three frames, regardless of if the skip frames are present. Hence the condition here. - // But this may be incorrect and need to be revisited later. - if (i > 2) + // On testing against stable, it appears that stable ignores the negative time of only the first + // negative frame of the first three replay frames, regardless of if the skip frames are present. + // Hence the condition here. + // But there is a possibility this is incorrect and may need to be revisited later. + if (i > 2 || negativeFrameEncounted) { timeDeficit += diff; timeDeficit = Math.Min(0, timeDeficit); } + if (diff < 0) + negativeFrameEncounted = true; + // still paying back the deficit from a negative frame. Skip this frame. if (timeDeficit < 0) continue; From 5f7028b5741f88e5c68e05cbd878ecc110d32eec Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bart=C5=82omiej=20Dach?= Date: Thu, 21 Mar 2024 17:45:56 +0100 Subject: [PATCH 8/9] Add failing tests --- .../Formats/LegacyScoreDecoderTest.cs | 89 +++++++++++++++++++ 1 file changed, 89 insertions(+) diff --git a/osu.Game.Tests/Beatmaps/Formats/LegacyScoreDecoderTest.cs b/osu.Game.Tests/Beatmaps/Formats/LegacyScoreDecoderTest.cs index 5dae86d9e9..050259c2fa 100644 --- a/osu.Game.Tests/Beatmaps/Formats/LegacyScoreDecoderTest.cs +++ b/osu.Game.Tests/Beatmaps/Formats/LegacyScoreDecoderTest.cs @@ -31,6 +31,7 @@ using osu.Game.Rulesets.Taiko; using osu.Game.Scoring; using osu.Game.Scoring.Legacy; using osu.Game.Tests.Resources; +using osuTK; namespace osu.Game.Tests.Beatmaps.Formats { @@ -178,6 +179,94 @@ namespace osu.Game.Tests.Beatmaps.Formats Assert.That(decodedAfterEncode.Replay.Frames[1].Time, Is.EqualTo(second_frame_time)); } + [Test] + public void TestNegativeFrameSkipped() + { + var ruleset = new OsuRuleset().RulesetInfo; + var scoreInfo = TestResources.CreateTestScoreInfo(ruleset); + var beatmap = new TestBeatmap(ruleset); + + var score = new Score + { + ScoreInfo = scoreInfo, + Replay = new Replay + { + Frames = new List + { + new OsuReplayFrame(0, new Vector2()), + new OsuReplayFrame(1000, OsuPlayfield.BASE_SIZE), + new OsuReplayFrame(500, OsuPlayfield.BASE_SIZE / 2), + new OsuReplayFrame(2000, OsuPlayfield.BASE_SIZE), + } + } + }; + + var decodedAfterEncode = encodeThenDecode(LegacyScoreEncoder.LATEST_VERSION, score, beatmap); + + Assert.That(decodedAfterEncode.Replay.Frames, Has.Count.EqualTo(3)); + Assert.That(decodedAfterEncode.Replay.Frames[0].Time, Is.EqualTo(0)); + Assert.That(decodedAfterEncode.Replay.Frames[1].Time, Is.EqualTo(1000)); + Assert.That(decodedAfterEncode.Replay.Frames[2].Time, Is.EqualTo(2000)); + } + + [Test] + public void FirstTwoFramesSwappedIfInWrongOrder() + { + var ruleset = new OsuRuleset().RulesetInfo; + var scoreInfo = TestResources.CreateTestScoreInfo(ruleset); + var beatmap = new TestBeatmap(ruleset); + + var score = new Score + { + ScoreInfo = scoreInfo, + Replay = new Replay + { + Frames = new List + { + new OsuReplayFrame(100, new Vector2()), + new OsuReplayFrame(50, OsuPlayfield.BASE_SIZE / 2), + new OsuReplayFrame(1000, OsuPlayfield.BASE_SIZE), + } + } + }; + + var decodedAfterEncode = encodeThenDecode(LegacyScoreEncoder.LATEST_VERSION, score, beatmap); + + Assert.That(decodedAfterEncode.Replay.Frames, Has.Count.EqualTo(3)); + Assert.That(decodedAfterEncode.Replay.Frames[0].Time, Is.EqualTo(0)); + Assert.That(decodedAfterEncode.Replay.Frames[1].Time, Is.EqualTo(100)); + Assert.That(decodedAfterEncode.Replay.Frames[2].Time, Is.EqualTo(1000)); + } + + [Test] + public void FirstTwoFramesPulledTowardThirdIfTheyAreAfterIt() + { + var ruleset = new OsuRuleset().RulesetInfo; + var scoreInfo = TestResources.CreateTestScoreInfo(ruleset); + var beatmap = new TestBeatmap(ruleset); + + var score = new Score + { + ScoreInfo = scoreInfo, + Replay = new Replay + { + Frames = new List + { + new OsuReplayFrame(0, new Vector2()), + new OsuReplayFrame(500, OsuPlayfield.BASE_SIZE / 2), + new OsuReplayFrame(-1500, OsuPlayfield.BASE_SIZE), + } + } + }; + + var decodedAfterEncode = encodeThenDecode(LegacyScoreEncoder.LATEST_VERSION, score, beatmap); + + Assert.That(decodedAfterEncode.Replay.Frames, Has.Count.EqualTo(3)); + Assert.That(decodedAfterEncode.Replay.Frames[0].Time, Is.EqualTo(-1500)); + Assert.That(decodedAfterEncode.Replay.Frames[1].Time, Is.EqualTo(-1500)); + Assert.That(decodedAfterEncode.Replay.Frames[2].Time, Is.EqualTo(-1500)); + } + [Test] public void TestCultureInvariance() { From 990a07af0eb7070b2e92ed37c033bf183721e299 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bart=C5=82omiej=20Dach?= Date: Thu, 21 Mar 2024 21:01:51 +0100 Subject: [PATCH 9/9] Rewrite handling of legacy replay frame quirks to match stable closer --- osu.Game/Scoring/Legacy/LegacyScoreDecoder.cs | 77 +++++++++---------- 1 file changed, 37 insertions(+), 40 deletions(-) diff --git a/osu.Game/Scoring/Legacy/LegacyScoreDecoder.cs b/osu.Game/Scoring/Legacy/LegacyScoreDecoder.cs index b16cdffe82..af514a4b59 100644 --- a/osu.Game/Scoring/Legacy/LegacyScoreDecoder.cs +++ b/osu.Game/Scoring/Legacy/LegacyScoreDecoder.cs @@ -21,6 +21,7 @@ using osu.Game.Rulesets; using osu.Game.Rulesets.Mods; using osu.Game.Rulesets.Replays; using osu.Game.Rulesets.Scoring; +using osuTK; using SharpCompress.Compressors.LZMA; namespace osu.Game.Scoring.Legacy @@ -240,15 +241,7 @@ namespace osu.Game.Scoring.Legacy private void readLegacyReplay(Replay replay, StreamReader reader) { float lastTime = beatmapOffset; - bool negativeFrameEncounted = false; - ReplayFrame currentFrame = null; - - // the negative time amount that must be "paid back" by positive frames before we start including frames again. - // When a negative frame occurs in a replay, all future frames are skipped until the sum total of their times - // 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; + var legacyFrames = new List(); string[] frames = reader.ReadToEnd().Split(','); @@ -271,40 +264,44 @@ namespace osu.Game.Scoring.Legacy lastTime += diff; - if (i < 2 && mouseX == 256 && mouseY == -500) - // at the start of the replay, stable places two replay frames, at time 0 and SkipBoundary - 1, respectively. - // both frames use a position of (256, -500). - // ignore these frames as they serve no real purpose (and can even mislead ruleset-specific handlers - see mania) - continue; - - // negative frames are only counted towards the deficit after the very beginning of the replay. - // When the two skip frames are present (see directly above), the third frame will have a large - // negative time roughly equal to SkipBoundary. This shouldn't be counted towards the deficit, otherwise - // any replay data before the skip would be, well, skipped. - // - // On testing against stable, it appears that stable ignores the negative time of only the first - // negative frame of the first three replay frames, regardless of if the skip frames are present. - // Hence the condition here. - // But there is a possibility this is incorrect and may need to be revisited later. - if (i > 2 || negativeFrameEncounted) - { - timeDeficit += diff; - timeDeficit = Math.Min(0, timeDeficit); - } - - if (diff < 0) - negativeFrameEncounted = true; - - // still paying back the deficit from a negative frame. Skip this frame. - if (timeDeficit < 0) - continue; - - currentFrame = convertFrame(new LegacyReplayFrame(lastTime, + legacyFrames.Add(new LegacyReplayFrame(lastTime, mouseX, mouseY, - (ReplayButtonState)Parsing.ParseInt(split[3])), currentFrame); + (ReplayButtonState)Parsing.ParseInt(split[3]))); + } - replay.Frames.Add(currentFrame); + // https://github.com/peppy/osu-stable-reference/blob/e53980dd76857ee899f66ce519ba1597e7874f28/osu!/GameModes/Play/ReplayWatcher.cs#L62-L67 + if (legacyFrames.Count >= 2 && legacyFrames[1].Time < legacyFrames[0].Time) + { + legacyFrames[1].Time = legacyFrames[0].Time; + legacyFrames[0].Time = 0; + } + + // https://github.com/peppy/osu-stable-reference/blob/e53980dd76857ee899f66ce519ba1597e7874f28/osu!/GameModes/Play/ReplayWatcher.cs#L69-L71 + if (legacyFrames.Count >= 3 && legacyFrames[0].Time > legacyFrames[2].Time) + legacyFrames[0].Time = legacyFrames[1].Time = legacyFrames[2].Time; + + // at the start of the replay, stable places two replay frames, at time 0 and SkipBoundary - 1, respectively. + // both frames use a position of (256, -500). + // ignore these frames as they serve no real purpose (and can even mislead ruleset-specific handlers - see mania) + if (legacyFrames.Count >= 2 && legacyFrames[1].Position == new Vector2(256, -500)) + legacyFrames.RemoveAt(1); + + if (legacyFrames.Count >= 1 && legacyFrames[0].Position == new Vector2(256, -500)) + legacyFrames.RemoveAt(0); + + ReplayFrame currentFrame = null; + + foreach (var legacyFrame in legacyFrames) + { + // never allow backwards time traversal in relation to the current frame. + // this handles frames with negative delta. + // this doesn't match stable 100% as stable will do something similar to adding an interpolated "intermediate frame" + // at the point wherein time flow changes from backwards to forwards, but it'll do for now. + if (currentFrame != null && legacyFrame.Time < currentFrame.Time) + continue; + + replay.Frames.Add(currentFrame = convertFrame(legacyFrame, currentFrame)); } }