Fix combo/combo colouring issues around spinners

osu.Game.Rulesets.Catch/Beatmaps/CatchBeatmapProcessor.cs

@@ -23,6 +23,22 @@ namespace osu.Game.Rulesets.Catch.Beatmaps
         {
         }
 
+        public override void PreProcess()
+        {
+            IHasComboInformation? lastObj = null;
+
+            // For sanity, ensures that both the first hitobject and the first hitobject after a banana shower start a new combo.
+            // This is normally enforced by the legacy decoder, but is not enforced by the editor.
+            foreach (var obj in Beatmap.HitObjects.OfType<IHasComboInformation>())
+            {
+                if (obj is not BananaShower && (lastObj == null || lastObj is BananaShower))
+                    obj.NewCombo = true;
+                lastObj = obj;
+            }
+
+            base.PreProcess();
+        }
+
         public override void PostProcess()
         {
             base.PostProcess();

osu.Game.Rulesets.Catch/Objects/CatchHitObject.cs

@@ -155,6 +155,31 @@ namespace osu.Game.Rulesets.Catch.Objects
             Scale = LegacyRulesetExtensions.CalculateScaleFromCircleSize(difficulty.CircleSize);
         }
 
+        public void UpdateComboInformation(IHasComboInformation? lastObj)
+        {
+            ComboIndex = lastObj?.ComboIndex ?? 0;
+            ComboIndexWithOffsets = lastObj?.ComboIndexWithOffsets ?? 0;
+            IndexInCurrentCombo = (lastObj?.IndexInCurrentCombo + 1) ?? 0;
+
+            if (this is BananaShower)
+            {
+                // For the purpose of combo colours, spinners never start a new combo even if they are flagged as doing so.
+                return;
+            }
+
+            // At decode time, the first hitobject in the beatmap and the first hitobject after a banana shower are both enforced to be a new combo,
+            // but this isn't directly enforced by the editor so the extra checks against the last hitobject are duplicated here.
+            if (NewCombo || lastObj == null || lastObj is BananaShower)
+            {
+                IndexInCurrentCombo = 0;
+                ComboIndex++;
+                ComboIndexWithOffsets += ComboOffset + 1;
+
+                if (lastObj != null)
+                    lastObj.LastInCombo = true;
+            }
+        }
+
         protected override HitWindows CreateHitWindows() => HitWindows.Empty;
 
         #region Hit object conversion

osu.Game.Rulesets.Osu/Beatmaps/OsuBeatmapProcessor.cs

@@ -2,9 +2,11 @@
 // See the LICENCE file in the repository root for full licence text.
 
 using System;
+using System.Linq;
 using osu.Framework.Graphics;
 using osu.Game.Beatmaps;
 using osu.Game.Rulesets.Objects;
+using osu.Game.Rulesets.Objects.Types;
 using osu.Game.Rulesets.Osu.Objects;
 using osuTK;
 
@@ -19,6 +21,22 @@ namespace osu.Game.Rulesets.Osu.Beatmaps
         {
         }
 
+        public override void PreProcess()
+        {
+            IHasComboInformation? lastObj = null;
+
+            // For sanity, ensures that both the first hitobject and the first hitobject after a spinner start a new combo.
+            // This is normally enforced by the legacy decoder, but is not enforced by the editor.
+            foreach (var obj in Beatmap.HitObjects.OfType<IHasComboInformation>())
+            {
+                if (obj is not Spinner && (lastObj == null || lastObj is Spinner))
+                    obj.NewCombo = true;
+                lastObj = obj;
+            }
+
+            base.PreProcess();
+        }
+
         public override void PostProcess()
         {
             base.PostProcess();
@@ -95,15 +113,15 @@ namespace osu.Game.Rulesets.Osu.Beatmaps
             {
                 int n = i;
                 /* We should check every note which has not yet got a stack.
-                    * Consider the case we have two interwound stacks and this will make sense.
+                 * Consider the case we have two interwound stacks and this will make sense.
-                    *
+                 *
-                    * o <-1      o <-2
+                 * o <-1      o <-2
-                    *  o <-3      o <-4
+                 *  o <-3      o <-4
-                    *
+                 *
-                    * We first process starting from 4 and handle 2,
+                 * We first process starting from 4 and handle 2,
-                    * then we come backwards on the i loop iteration until we reach 3 and handle 1.
+                 * then we come backwards on the i loop iteration until we reach 3 and handle 1.
-                    * 2 and 1 will be ignored in the i loop because they already have a stack value.
+                 * 2 and 1 will be ignored in the i loop because they already have a stack value.
-                    */
+                 */
 
                 OsuHitObject objectI = beatmap.HitObjects[i];
                 if (objectI.StackHeight != 0 || objectI is Spinner) continue;
@@ -111,9 +129,9 @@ namespace osu.Game.Rulesets.Osu.Beatmaps
                 double stackThreshold = objectI.TimePreempt * beatmap.BeatmapInfo.StackLeniency;
 
                 /* If this object is a hitcircle, then we enter this "special" case.
-                    * It either ends with a stack of hitcircles only, or a stack of hitcircles that are underneath a slider.
+                 * It either ends with a stack of hitcircles only, or a stack of hitcircles that are underneath a slider.
-                    * Any other case is handled by the "is Slider" code below this.
+                 * Any other case is handled by the "is Slider" code below this.
-                    */
+                 */
                 if (objectI is HitCircle)
                 {
                     while (--n >= 0)
@@ -135,10 +153,10 @@ namespace osu.Game.Rulesets.Osu.Beatmaps
                         }
 
                         /* This is a special case where hticircles are moved DOWN and RIGHT (negative stacking) if they are under the *last* slider in a stacked pattern.
-                            *    o==o <- slider is at original location
+                         *    o==o <- slider is at original location
-                            *        o <- hitCircle has stack of -1
+                         *        o <- hitCircle has stack of -1
-                            *         o <- hitCircle has stack of -2
+                         *         o <- hitCircle has stack of -2
-                            */
+                         */
                         if (objectN is Slider && Vector2Extensions.Distance(objectN.EndPosition, objectI.Position) < stack_distance)
                         {
                             int offset = objectI.StackHeight - objectN.StackHeight + 1;
@@ -169,8 +187,8 @@ namespace osu.Game.Rulesets.Osu.Beatmaps
                 else if (objectI is Slider)
                 {
                     /* We have hit the first slider in a possible stack.
-                        * From this point on, we ALWAYS stack positive regardless.
+                     * From this point on, we ALWAYS stack positive regardless.
-                        */
+                     */
                     while (--n >= startIndex)
                     {
                         OsuHitObject objectN = beatmap.HitObjects[n];

osu.Game.Rulesets.Osu/Objects/OsuHitObject.cs

@@ -159,6 +159,31 @@ namespace osu.Game.Rulesets.Osu.Objects
             Scale = LegacyRulesetExtensions.CalculateScaleFromCircleSize(difficulty.CircleSize, true);
         }
 
+        public void UpdateComboInformation(IHasComboInformation? lastObj)
+        {
+            ComboIndex = lastObj?.ComboIndex ?? 0;
+            ComboIndexWithOffsets = lastObj?.ComboIndexWithOffsets ?? 0;
+            IndexInCurrentCombo = (lastObj?.IndexInCurrentCombo + 1) ?? 0;
+
+            if (this is Spinner)
+            {
+                // For the purpose of combo colours, spinners never start a new combo even if they are flagged as doing so.
+                return;
+            }
+
+            // At decode time, the first hitobject in the beatmap and the first hitobject after a spinner are both enforced to be a new combo,
+            // but this isn't directly enforced by the editor so the extra checks against the last hitobject are duplicated here.
+            if (NewCombo || lastObj == null || lastObj is Spinner)
+            {
+                IndexInCurrentCombo = 0;
+                ComboIndex++;
+                ComboIndexWithOffsets += ComboOffset + 1;
+
+                if (lastObj != null)
+                    lastObj.LastInCombo = true;
+            }
+        }
+
         protected override HitWindows CreateHitWindows() => new OsuHitWindows();
     }
 }

osu.Game.Tests/Beatmaps/Formats/LegacyBeatmapDecoderTest.cs

@@ -12,6 +12,7 @@ using osu.Game.Beatmaps.Formats;
 using osu.Game.Beatmaps.Legacy;
 using osu.Game.Beatmaps.Timing;
 using osu.Game.IO;
+using osu.Game.Rulesets;
 using osu.Game.Rulesets.Catch;
 using osu.Game.Rulesets.Catch.Beatmaps;
 using osu.Game.Rulesets.Mods;
@@ -1108,5 +1109,52 @@ namespace osu.Game.Tests.Beatmaps.Formats
                 Assert.That(((IHasCombo)beatmap.HitObjects[2]).NewCombo, Is.False);
             }
         }
+
+        /// <summary>
+        /// Test cases that involve a spinner between two hitobjects.
+        /// </summary>
+        [Test]
+        public void TestSpinnerNewComboBetweenObjects([Values("osu", "catch")] string rulesetName)
+        {
+            var decoder = new LegacyBeatmapDecoder { ApplyOffsets = false };
+
+            using (var resStream = TestResources.OpenResource("spinner-between-objects.osu"))
+            using (var stream = new LineBufferedReader(resStream))
+            {
+                Ruleset ruleset;
+
+                switch (rulesetName)
+                {
+                    case "osu":
+                        ruleset = new OsuRuleset();
+                        break;
+
+                    case "catch":
+                        ruleset = new CatchRuleset();
+                        break;
+
+                    default:
+                        throw new ArgumentOutOfRangeException(nameof(rulesetName), rulesetName, null);
+                }
+
+                var working = new TestWorkingBeatmap(decoder.Decode(stream));
+                var playable = working.GetPlayableBeatmap(ruleset.RulesetInfo, Array.Empty<Mod>());
+
+                // There's no good way to figure out these values other than to compare (in code) with osu!stable...
+
+                Assert.That(((IHasComboInformation)playable.HitObjects[0]).ComboIndexWithOffsets, Is.EqualTo(1));
+                Assert.That(((IHasComboInformation)playable.HitObjects[2]).ComboIndexWithOffsets, Is.EqualTo(2));
+                Assert.That(((IHasComboInformation)playable.HitObjects[3]).ComboIndexWithOffsets, Is.EqualTo(2));
+                Assert.That(((IHasComboInformation)playable.HitObjects[5]).ComboIndexWithOffsets, Is.EqualTo(3));
+                Assert.That(((IHasComboInformation)playable.HitObjects[6]).ComboIndexWithOffsets, Is.EqualTo(3));
+                Assert.That(((IHasComboInformation)playable.HitObjects[8]).ComboIndexWithOffsets, Is.EqualTo(4));
+                Assert.That(((IHasComboInformation)playable.HitObjects[9]).ComboIndexWithOffsets, Is.EqualTo(4));
+                Assert.That(((IHasComboInformation)playable.HitObjects[11]).ComboIndexWithOffsets, Is.EqualTo(5));
+                Assert.That(((IHasComboInformation)playable.HitObjects[12]).ComboIndexWithOffsets, Is.EqualTo(6));
+                Assert.That(((IHasComboInformation)playable.HitObjects[14]).ComboIndexWithOffsets, Is.EqualTo(7));
+                Assert.That(((IHasComboInformation)playable.HitObjects[15]).ComboIndexWithOffsets, Is.EqualTo(8));
+                Assert.That(((IHasComboInformation)playable.HitObjects[17]).ComboIndexWithOffsets, Is.EqualTo(9));
+            }
+        }
     }
 }

osu.Game.Tests/Resources/spinner-between-objects.osu

@@ -0,0 +1,38 @@
+osu file format v14
+
+[General]
+Mode: 0
+
+[TimingPoints]
+0,571.428571428571,4,2,1,5,1,0
+
+[HitObjects]
+// +C -> +C -> +C
+104,95,0,5,0,0:0:0:0:
+256,192,1000,12,0,2000,0:0:0:0:
+178,171,3000,5,0,0:0:0:0:
+
+// -C -> +C -> +C
+178,171,4000,1,0,0:0:0:0:
+256,192,5000,12,0,6000,0:0:0:0:
+178,171,7000,5,0,0:0:0:0:
+
+// -C -> -C -> +C
+178,171,8000,1,0,0:0:0:0:
+256,192,9000,8,0,10000,0:0:0:0:
+178,171,11000,5,0,0:0:0:0:
+
+// -C -> -C -> -C
+178,171,12000,1,0,0:0:0:0:
+256,192,13000,8,0,14000,0:0:0:0:
+178,171,15000,1,0,0:0:0:0:
+
+// +C -> -C -> -C
+178,171,16000,5,0,0:0:0:0:
+256,192,17000,8,0,18000,0:0:0:0:
+178,171,19000,1,0,0:0:0:0:
+
+// +C -> +C -> -C
+178,171,20000,5,0,0:0:0:0:
+256,192,21000,12,0,22000,0:0:0:0:
+178,171,23000,1,0,0:0:0:0:

osu.Game/Beatmaps/BeatmapProcessor.cs

@@ -24,12 +24,6 @@ namespace osu.Game.Beatmaps
 
             foreach (var obj in Beatmap.HitObjects.OfType<IHasComboInformation>())
             {
-                if (lastObj == null)
-                {
-                    // first hitobject should always be marked as a new combo for sanity.
-                    obj.NewCombo = true;
-                }
-
                 obj.UpdateComboInformation(lastObj);
                 lastObj = obj;
             }

osu.Game/Rulesets/Objects/Legacy/Catch/ConvertHitObjectParser.cs

@@ -14,26 +14,19 @@ namespace osu.Game.Rulesets.Objects.Legacy.Catch
     /// </summary>
     public class ConvertHitObjectParser : Legacy.ConvertHitObjectParser
     {
+        private ConvertHitObject lastObject;
+
         public ConvertHitObjectParser(double offset, int formatVersion)
             : base(offset, formatVersion)
         {
         }
 
-        private bool forceNewCombo;
-        private int extraComboOffset;
-
         protected override HitObject CreateHit(Vector2 position, bool newCombo, int comboOffset)
         {
-            newCombo |= forceNewCombo;
+            return lastObject = new ConvertHit
-            comboOffset += extraComboOffset;
-
-            forceNewCombo = false;
-            extraComboOffset = 0;
-
-            return new ConvertHit
             {
                 Position = position,
-                NewCombo = newCombo,
+                NewCombo = FirstObject || lastObject is ConvertSpinner || newCombo,
                 ComboOffset = comboOffset
             };
         }
@@ -41,16 +34,10 @@ namespace osu.Game.Rulesets.Objects.Legacy.Catch
         protected override HitObject CreateSlider(Vector2 position, bool newCombo, int comboOffset, PathControlPoint[] controlPoints, double? length, int repeatCount,
                                                   IList<IList<HitSampleInfo>> nodeSamples)
         {
-            newCombo |= forceNewCombo;
+            return lastObject = new ConvertSlider
-            comboOffset += extraComboOffset;
-
-            forceNewCombo = false;
-            extraComboOffset = 0;
-
-            return new ConvertSlider
             {
                 Position = position,
-                NewCombo = FirstObject || newCombo,
+                NewCombo = FirstObject || lastObject is ConvertSpinner || newCombo,
                 ComboOffset = comboOffset,
                 Path = new SliderPath(controlPoints, length),
                 NodeSamples = nodeSamples,
@@ -60,20 +47,17 @@ namespace osu.Game.Rulesets.Objects.Legacy.Catch
 
         protected override HitObject CreateSpinner(Vector2 position, bool newCombo, int comboOffset, double duration)
         {
-            // Convert spinners don't create the new combo themselves, but force the next non-spinner hitobject to create a new combo
+            return lastObject = new ConvertSpinner
-            // Their combo offset is still added to that next hitobject's combo index
-            forceNewCombo |= FormatVersion <= 8 || newCombo;
-            extraComboOffset += comboOffset;
-
-            return new ConvertSpinner
             {
-                Duration = duration
+                Duration = duration,
+                NewCombo = newCombo
+                // Spinners cannot have combo offset.
             };
         }
 
         protected override HitObject CreateHold(Vector2 position, bool newCombo, int comboOffset, double duration)
         {
-            return null;
+            return lastObject = null;
         }
     }
 }

osu.Game/Rulesets/Objects/Legacy/Osu/ConvertHitObjectParser.cs

@@ -14,26 +14,19 @@ namespace osu.Game.Rulesets.Objects.Legacy.Osu
     /// </summary>
     public class ConvertHitObjectParser : Legacy.ConvertHitObjectParser
     {
+        private ConvertHitObject lastObject;
+
         public ConvertHitObjectParser(double offset, int formatVersion)
             : base(offset, formatVersion)
         {
         }
 
-        private bool forceNewCombo;
-        private int extraComboOffset;
-
         protected override HitObject CreateHit(Vector2 position, bool newCombo, int comboOffset)
         {
-            newCombo |= forceNewCombo;
+            return lastObject = new ConvertHit
-            comboOffset += extraComboOffset;
-
-            forceNewCombo = false;
-            extraComboOffset = 0;
-
-            return new ConvertHit
             {
                 Position = position,
-                NewCombo = FirstObject || newCombo,
+                NewCombo = FirstObject || lastObject is ConvertSpinner || newCombo,
                 ComboOffset = comboOffset
             };
         }
@@ -41,16 +34,10 @@ namespace osu.Game.Rulesets.Objects.Legacy.Osu
         protected override HitObject CreateSlider(Vector2 position, bool newCombo, int comboOffset, PathControlPoint[] controlPoints, double? length, int repeatCount,
                                                   IList<IList<HitSampleInfo>> nodeSamples)
         {
-            newCombo |= forceNewCombo;
+            return lastObject = new ConvertSlider
-            comboOffset += extraComboOffset;
-
-            forceNewCombo = false;
-            extraComboOffset = 0;
-
-            return new ConvertSlider
             {
                 Position = position,
-                NewCombo = FirstObject || newCombo,
+                NewCombo = FirstObject || lastObject is ConvertSpinner || newCombo,
                 ComboOffset = comboOffset,
                 Path = new SliderPath(controlPoints, length),
                 NodeSamples = nodeSamples,
@@ -60,21 +47,18 @@ namespace osu.Game.Rulesets.Objects.Legacy.Osu
 
         protected override HitObject CreateSpinner(Vector2 position, bool newCombo, int comboOffset, double duration)
         {
-            // Convert spinners don't create the new combo themselves, but force the next non-spinner hitobject to create a new combo
+            return lastObject = new ConvertSpinner
-            // Their combo offset is still added to that next hitobject's combo index
-            forceNewCombo |= FormatVersion <= 8 || newCombo;
-            extraComboOffset += comboOffset;
-
-            return new ConvertSpinner
             {
                 Position = position,
-                Duration = duration
+                Duration = duration,
+                NewCombo = newCombo
+                // Spinners cannot have combo offset.
             };
         }
 
         protected override HitObject CreateHold(Vector2 position, bool newCombo, int comboOffset, double duration)
         {
-            return null;
+            return lastObject = null;
         }
     }
 }