Merge pull request #33209 from bdach/mania-combo-accounting-rewind

Fix combo accounting when rewinding mania replays

osu.Game.Rulesets.Mania.Tests/TestSceneReplayRewinding.cs

@@ -65,5 +65,61 @@ namespace osu.Game.Rulesets.Mania.Tests
 
             AddStep(@"exit player", () => currentPlayer.Exit());
         }
+
+        [Test]
+        public void TestCorrectComboAccountingForConcurrentObjects()
+        {
+            Score score = null!;
+
+            var beatmap = new ManiaBeatmap(new StageDefinition(4))
+            {
+                HitObjects =
+                {
+                    new Note
+                    {
+                        StartTime = 500,
+                        Column = 0,
+                    },
+                    new Note
+                    {
+                        StartTime = 500,
+                        Column = 2,
+                    },
+                    new HoldNote
+                    {
+                        StartTime = 1000,
+                        EndTime = 1500,
+                        Column = 1,
+                    }
+                }
+            };
+
+            AddStep(@"create replay", () => score = new Score
+            {
+                Replay = new Replay
+                {
+                    Frames =
+                    {
+                        new ManiaReplayFrame(500, ManiaAction.Key1, ManiaAction.Key3),
+                        new ManiaReplayFrame(520),
+                        new ManiaReplayFrame(1000, ManiaAction.Key2),
+                        new ManiaReplayFrame(1500),
+                    }
+                },
+                ScoreInfo = new ScoreInfo()
+            });
+
+            AddStep(@"set beatmap", () => Beatmap.Value = CreateWorkingBeatmap(beatmap));
+            AddStep(@"set ruleset", () => Ruleset.Value = beatmap.BeatmapInfo.Ruleset);
+            AddStep(@"push player", () => LoadScreen(currentPlayer = new ReplayPlayer(score)));
+
+            AddUntilStep(@"wait until player is loaded", () => currentPlayer.IsCurrentScreen());
+            AddUntilStep(@"wait for objects to be judged", () => currentPlayer.ChildrenOfType<IFrameStableClock>().Single().CurrentTime, () => Is.GreaterThan(1600));
+            AddStep(@"stop gameplay", () => currentPlayer.ChildrenOfType<GameplayClockContainer>().Single().Stop());
+            AddStep(@"seek to start", () => currentPlayer.Seek(0));
+            AddAssert(@"combo is 0", () => currentPlayer.GameplayState.ScoreProcessor.Combo.Value, () => Is.Zero);
+
+            AddStep(@"exit player", () => currentPlayer.Exit());
+        }
     }
 }

osu.Game.Tests/Rulesets/Scoring/ScoreProcessorTest.cs

@@ -421,6 +421,65 @@ namespace osu.Game.Tests.Rulesets.Scoring
             Assert.That(scoreProcessor.Rank.Value, Is.EqualTo(ScoreRank.SH));
         }
 
+        [Test]
+        public void TestComboAccounting([Values] bool shuffleResults)
+        {
+            var testBeatmap = new Beatmap
+            {
+                HitObjects = Enumerable.Range(1, 40).Select(i => new TestHitObject(HitResult.Perfect, HitResult.Miss)).ToList<HitObject>(),
+            };
+            scoreProcessor.ApplyBeatmap(testBeatmap);
+
+            var results = new List<JudgementResult>();
+            JudgementResult judgementResult;
+
+            for (int i = 0; i < 25; ++i)
+            {
+                judgementResult = new JudgementResult(testBeatmap.HitObjects[i], new TestJudgement(HitResult.Perfect, HitResult.Miss))
+                {
+                    Type = HitResult.Perfect
+                };
+                results.Add(judgementResult);
+                scoreProcessor.ApplyResult(judgementResult);
+                Assert.That(scoreProcessor.Combo.Value, Is.EqualTo(i + 1));
+            }
+
+            judgementResult = new JudgementResult(testBeatmap.HitObjects[25], new TestJudgement(HitResult.Perfect, HitResult.Miss))
+            {
+                Type = HitResult.Miss
+            };
+            results.Add(judgementResult);
+            scoreProcessor.ApplyResult(judgementResult);
+            Assert.That(scoreProcessor.Combo.Value, Is.EqualTo(0));
+
+            for (int i = 26; i < 40; ++i)
+            {
+                judgementResult = new JudgementResult(testBeatmap.HitObjects[i], new TestJudgement(HitResult.Perfect, HitResult.Miss))
+                {
+                    Type = HitResult.Perfect
+                };
+                results.Add(judgementResult);
+                scoreProcessor.ApplyResult(judgementResult);
+                Assert.That(scoreProcessor.Combo.Value, Is.EqualTo(i - 25));
+            }
+
+            Assert.That(scoreProcessor.MaximumStatistics[HitResult.Perfect], Is.EqualTo(40));
+            Assert.That(scoreProcessor.Combo.Value, Is.EqualTo(14));
+            Assert.That(scoreProcessor.HighestCombo.Value, Is.EqualTo(25));
+
+            // random shuffle is VERY extreme and overkill.
+            // it might not work correctly for any other `ScoreProcessor` property, and the intermediate results likely make no sense.
+            // the goal is only to demonstrate idempotency to zero when reverting all results.
+            var random = new Random(20250519);
+            var toRevert = shuffleResults ? results.OrderBy(_ => random.Next()).ToList() : Enumerable.Reverse(results);
+
+            foreach (var result in toRevert)
+                scoreProcessor.RevertResult(result);
+
+            Assert.That(scoreProcessor.Combo.Value, Is.Zero);
+            Assert.That(scoreProcessor.HighestCombo.Value, Is.Zero);
+        }
+
         private class TestJudgement : Judgement
         {
             public override HitResult MaxResult { get; }

osu.Game/Rulesets/Judgements/JudgementResult.cs

@@ -74,6 +74,11 @@ namespace osu.Game.Rulesets.Judgements
         /// </summary>
         public int HighestComboAtJudgement { get; internal set; }
 
+        /// <summary>
+        /// The highest combo achieved after this <see cref="JudgementResult"/> occurred.
+        /// </summary>
+        public int HighestComboAfterJudgement { get; internal set; }
+
         /// <summary>
         /// The health prior to this <see cref="JudgementResult"/> occurring.
         /// </summary>

osu.Game/Rulesets/Scoring/ScoreProcessor.cs

@@ -202,7 +202,6 @@ namespace osu.Game.Rulesets.Scoring
         {
             Ruleset = ruleset;
 
-            Combo.ValueChanged += combo => HighestCombo.Value = Math.Max(HighestCombo.Value, combo.NewValue);
             Accuracy.ValueChanged += _ => updateRank();
 
             Mods.ValueChanged += mods =>
@@ -238,7 +237,10 @@ namespace osu.Game.Rulesets.Scoring
             else if (result.Type.BreaksCombo())
                 Combo.Value = 0;
 
+            HighestCombo.Value = Math.Max(HighestCombo.Value, Combo.Value);
+
             result.ComboAfterJudgement = Combo.Value;
+            result.HighestComboAfterJudgement = HighestCombo.Value;
 
             if (result.Judgement.MaxResult.AffectsAccuracy())
             {
@@ -281,8 +283,11 @@ namespace osu.Game.Rulesets.Scoring
             if (!TrackHitEvents)
                 throw new InvalidOperationException(@$"Rewind is not supported when {nameof(TrackHitEvents)} is disabled.");
 
-            Combo.Value = result.ComboAtJudgement;
-            HighestCombo.Value = result.HighestComboAtJudgement;
+            // the reason this is written so funnily rather than just using `ComboAtJudgement`
+            // is to nullify impact of ordering when reverting concurrent judgement results
+            // (think mania and multiple judgements within a frame).
+            Combo.Value -= (result.ComboAfterJudgement - result.ComboAtJudgement);
+            HighestCombo.Value -= (result.HighestComboAfterJudgement - result.HighestComboAtJudgement);
 
             if (result.FailedAtJudgement && !ApplyNewJudgementsWhenFailed)
                 return;