Revert making ComboOffsets legacy and define BeatmapSkinComboIndex instead

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

@@ -39,6 +39,7 @@ namespace osu.Game.Rulesets.Catch.Beatmaps
                         RepeatCount = curveData.RepeatCount,
                         X = xPositionData?.X ?? 0,
                         NewCombo = comboData?.NewCombo ?? false,
+                        ComboOffset = comboData?.ComboOffset ?? 0,
                         LegacyLastTickOffset = (obj as IHasLegacyLastTickOffset)?.LegacyLastTickOffset ?? 0,
                         LegacyConvertedY = yPositionData?.Y ?? CatchHitObject.DEFAULT_LEGACY_CONVERT_Y
                     }.Yield();
@@ -50,6 +51,7 @@ namespace osu.Game.Rulesets.Catch.Beatmaps
                         Samples = obj.Samples,
                         Duration = endTime.Duration,
                         NewCombo = comboData?.NewCombo ?? false,
+                        ComboOffset = comboData?.ComboOffset ?? 0,
                     }.Yield();
 
                 default:
@@ -58,6 +60,7 @@ namespace osu.Game.Rulesets.Catch.Beatmaps
                         StartTime = obj.StartTime,
                         Samples = obj.Samples,
                         NewCombo = comboData?.NewCombo ?? false,
+                        ComboOffset = comboData?.ComboOffset ?? 0,
                         X = xPositionData?.X ?? 0,
                         LegacyConvertedY = yPositionData?.Y ?? CatchHitObject.DEFAULT_LEGACY_CONVERT_Y
                     }.Yield();

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

@@ -77,6 +77,8 @@ namespace osu.Game.Rulesets.Catch.Objects
 
         public virtual bool NewCombo { get; set; }
 
+        public int ComboOffset { get; set; }
+
         public Bindable<int> IndexInCurrentComboBindable { get; } = new Bindable<int>();
 
         public int IndexInCurrentCombo
@@ -93,6 +95,8 @@ namespace osu.Game.Rulesets.Catch.Objects
             set => ComboIndexBindable.Value = value;
         }
 
+        public int BeatmapSkinComboIndex { get; set; }
+
         public Bindable<bool> LastInComboBindable { get; } = new Bindable<bool>();
 
         /// <summary>

osu.Game.Rulesets.Osu.Tests/TestSceneLegacyBeatmapSkin.cs

@@ -90,7 +90,7 @@ namespace osu.Game.Rulesets.Osu.Tests
             ConfigureTest(useBeatmapSkin, true, userHasCustomColours);
             assertCorrectObjectComboColours("is beatmap skin colours with legacy offsets applied",
                 TestBeatmapSkin.Colours,
-                (i, obj) => i + 1 + obj.LegacyBeatmapComboOffset);
+                (i, obj) => i + 1 + obj.ComboOffset);
         }
 
         [TestCase(true)]
@@ -136,7 +136,7 @@ namespace osu.Game.Rulesets.Osu.Tests
                     StartTime = i,
                     Position = new Vector2(256, 192),
                     NewCombo = true,
-                    LegacyBeatmapComboOffset = i,
+                    ComboOffset = i,
                 });
             }
 

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

@@ -40,7 +40,7 @@ namespace osu.Game.Rulesets.Osu.Beatmaps
                         RepeatCount = curveData.RepeatCount,
                         Position = positionData?.Position ?? Vector2.Zero,
                         NewCombo = comboData?.NewCombo ?? false,
-                        LegacyBeatmapComboOffset = (original as IHasLegacyBeatmapComboOffset)?.LegacyBeatmapComboOffset ?? 0,
+                        ComboOffset = comboData?.ComboOffset ?? 0,
                         LegacyLastTickOffset = (original as IHasLegacyLastTickOffset)?.LegacyLastTickOffset,
                         // prior to v8, speed multipliers don't adjust for how many ticks are generated over the same distance.
                         // this results in more (or less) ticks being generated in <v8 maps for the same time duration.
@@ -55,7 +55,7 @@ namespace osu.Game.Rulesets.Osu.Beatmaps
                         EndTime = endTimeData.EndTime,
                         Position = positionData?.Position ?? OsuPlayfield.BASE_SIZE / 2,
                         NewCombo = comboData?.NewCombo ?? false,
-                        LegacyBeatmapComboOffset = (original as IHasLegacyBeatmapComboOffset)?.LegacyBeatmapComboOffset ?? 0,
+                        ComboOffset = comboData?.ComboOffset ?? 0,
                     }.Yield();
 
                 default:
@@ -65,7 +65,7 @@ namespace osu.Game.Rulesets.Osu.Beatmaps
                         Samples = original.Samples,
                         Position = positionData?.Position ?? Vector2.Zero,
                         NewCombo = comboData?.NewCombo ?? false,
-                        LegacyBeatmapComboOffset = (original as IHasLegacyBeatmapComboOffset)?.LegacyBeatmapComboOffset ?? 0,
+                        ComboOffset = comboData?.ComboOffset ?? 0,
                     }.Yield();
             }
         }

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

@@ -2,7 +2,6 @@
 // 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;
@@ -20,23 +19,6 @@ namespace osu.Game.Rulesets.Osu.Beatmaps
         {
         }
 
-        public override void PreProcess()
-        {
-            base.PreProcess();
-
-            OsuHitObject lastObj = null;
-
-            foreach (var obj in Beatmap.HitObjects.OfType<OsuHitObject>())
-            {
-                if (obj.NewCombo)
-                    obj.LegacyBeatmapComboIndex = (lastObj?.LegacyBeatmapComboIndex ?? 0) + obj.LegacyBeatmapComboOffset + 1;
-                else if (lastObj != null)
-                    obj.LegacyBeatmapComboIndex = lastObj.LegacyBeatmapComboIndex;
-
-                lastObj = obj;
-            }
-        }
-
         public override void PostProcess()
         {
             base.PostProcess();

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

@@ -14,7 +14,7 @@ using osu.Game.Rulesets.Scoring;
 
 namespace osu.Game.Rulesets.Osu.Objects
 {
-    public abstract class OsuHitObject : HitObject, IHasComboInformation, IHasPosition, IHasLegacyBeatmapComboOffset
+    public abstract class OsuHitObject : HitObject, IHasComboInformation, IHasPosition
     {
         /// <summary>
         /// The radius of hit objects (ie. the radius of a <see cref="HitCircle"/>).
@@ -73,6 +73,14 @@ namespace osu.Game.Rulesets.Osu.Objects
 
         public virtual bool NewCombo { get; set; }
 
+        public readonly Bindable<int> ComboOffsetBindable = new Bindable<int>();
+
+        public int ComboOffset
+        {
+            get => ComboOffsetBindable.Value;
+            set => ComboOffsetBindable.Value = value;
+        }
+
         public Bindable<int> IndexInCurrentComboBindable { get; } = new Bindable<int>();
 
         public virtual int IndexInCurrentCombo
@@ -89,6 +97,8 @@ namespace osu.Game.Rulesets.Osu.Objects
             set => ComboIndexBindable.Value = value;
         }
 
+        public int BeatmapSkinComboIndex { get; set; }
+
         public Bindable<bool> LastInComboBindable { get; } = new Bindable<bool>();
 
         public bool LastInCombo
@@ -97,10 +107,6 @@ namespace osu.Game.Rulesets.Osu.Objects
             set => LastInComboBindable.Value = value;
         }
 
-        public int LegacyBeatmapComboOffset { get; set; }
-
-        public int LegacyBeatmapComboIndex { get; set; }
-
         protected OsuHitObject()
         {
             StackHeightBindable.BindValueChanged(height =>

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

@@ -14,6 +14,8 @@ using osu.Game.Rulesets.Objects.Types;
 using osu.Game.Beatmaps.Formats;
 using osu.Game.Beatmaps.Timing;
 using osu.Game.IO;
+using osu.Game.Rulesets.Catch;
+using osu.Game.Rulesets.Catch.Beatmaps;
 using osu.Game.Rulesets.Mods;
 using osu.Game.Rulesets.Objects;
 using osu.Game.Rulesets.Objects.Legacy;
@@ -308,7 +310,7 @@ namespace osu.Game.Tests.Beatmaps.Formats
         }
 
         [Test]
-        public void TestDecodeLegacyBeatmapComboOffsets()
+        public void TestDecodeBeatmapComboOffsetsOsu()
         {
             var decoder = new LegacyBeatmapDecoder();
 
@@ -321,12 +323,35 @@ namespace osu.Game.Tests.Beatmaps.Formats
                 new OsuBeatmapProcessor(converted).PreProcess();
                 new OsuBeatmapProcessor(converted).PostProcess();
 
-                Assert.AreEqual(4, ((IHasLegacyBeatmapComboOffset)converted.HitObjects.ElementAt(0)).LegacyBeatmapComboIndex);
+                Assert.AreEqual(4, ((IHasComboInformation)converted.HitObjects.ElementAt(0)).BeatmapSkinComboIndex);
-                Assert.AreEqual(5, ((IHasLegacyBeatmapComboOffset)converted.HitObjects.ElementAt(2)).LegacyBeatmapComboIndex);
+                Assert.AreEqual(5, ((IHasComboInformation)converted.HitObjects.ElementAt(2)).BeatmapSkinComboIndex);
-                Assert.AreEqual(5, ((IHasLegacyBeatmapComboOffset)converted.HitObjects.ElementAt(4)).LegacyBeatmapComboIndex);
+                Assert.AreEqual(5, ((IHasComboInformation)converted.HitObjects.ElementAt(4)).BeatmapSkinComboIndex);
-                Assert.AreEqual(6, ((IHasLegacyBeatmapComboOffset)converted.HitObjects.ElementAt(6)).LegacyBeatmapComboIndex);
+                Assert.AreEqual(6, ((IHasComboInformation)converted.HitObjects.ElementAt(6)).BeatmapSkinComboIndex);
-                Assert.AreEqual(11, ((IHasLegacyBeatmapComboOffset)converted.HitObjects.ElementAt(8)).LegacyBeatmapComboIndex);
+                Assert.AreEqual(11, ((IHasComboInformation)converted.HitObjects.ElementAt(8)).BeatmapSkinComboIndex);
-                Assert.AreEqual(14, ((IHasLegacyBeatmapComboOffset)converted.HitObjects.ElementAt(11)).LegacyBeatmapComboIndex);
+                Assert.AreEqual(14, ((IHasComboInformation)converted.HitObjects.ElementAt(11)).BeatmapSkinComboIndex);
+            }
+        }
+
+        [Test]
+        public void TestDecodeBeatmapComboOffsetsCatch()
+        {
+            var decoder = new LegacyBeatmapDecoder();
+
+            using (var resStream = TestResources.OpenResource("hitobject-combo-offset.osu"))
+            using (var stream = new LineBufferedReader(resStream))
+            {
+                var beatmap = decoder.Decode(stream);
+
+                var converted = new CatchBeatmapConverter(beatmap, new CatchRuleset()).Convert();
+                new CatchBeatmapProcessor(converted).PreProcess();
+                new CatchBeatmapProcessor(converted).PostProcess();
+
+                Assert.AreEqual(4, ((IHasComboInformation)converted.HitObjects.ElementAt(0)).BeatmapSkinComboIndex);
+                Assert.AreEqual(5, ((IHasComboInformation)converted.HitObjects.ElementAt(2)).BeatmapSkinComboIndex);
+                Assert.AreEqual(5, ((IHasComboInformation)converted.HitObjects.ElementAt(4)).BeatmapSkinComboIndex);
+                Assert.AreEqual(6, ((IHasComboInformation)converted.HitObjects.ElementAt(6)).BeatmapSkinComboIndex);
+                Assert.AreEqual(11, ((IHasComboInformation)converted.HitObjects.ElementAt(8)).BeatmapSkinComboIndex);
+                Assert.AreEqual(14, ((IHasComboInformation)converted.HitObjects.ElementAt(11)).BeatmapSkinComboIndex);
             }
         }
 

osu.Game.Tests/Gameplay/TestSceneHitObjectAccentColour.cs

@@ -82,6 +82,7 @@ namespace osu.Game.Tests.Gameplay
         private class TestHitObjectWithCombo : ConvertHitObject, IHasComboInformation
         {
             public bool NewCombo { get; set; }
+            public int ComboOffset => 0;
 
             public Bindable<int> IndexInCurrentComboBindable { get; } = new Bindable<int>();
 
@@ -99,6 +100,8 @@ namespace osu.Game.Tests.Gameplay
                 set => ComboIndexBindable.Value = value;
             }
 
+            public int BeatmapSkinComboIndex { get; set; }
+
             public Bindable<bool> LastInComboBindable { get; } = new Bindable<bool>();
 
             public bool LastInCombo

osu.Game/Beatmaps/BeatmapProcessor.cs

@@ -38,6 +38,7 @@ namespace osu.Game.Beatmaps
                 {
                     obj.IndexInCurrentCombo = 0;
                     obj.ComboIndex = (lastObj?.ComboIndex ?? 0) + 1;
+                    obj.BeatmapSkinComboIndex = (lastObj?.BeatmapSkinComboIndex ?? 0) + obj.ComboOffset + 1;
 
                     if (lastObj != null)
                         lastObj.LastInCombo = true;
@@ -46,6 +47,7 @@ namespace osu.Game.Beatmaps
                 {
                     obj.IndexInCurrentCombo = lastObj.IndexInCurrentCombo + 1;
                     obj.ComboIndex = lastObj.ComboIndex;
+                    obj.BeatmapSkinComboIndex = lastObj.BeatmapSkinComboIndex;
                 }
 
                 lastObj = obj;

osu.Game/Beatmaps/Formats/LegacyBeatmapEncoder.cs

@@ -289,11 +289,10 @@ namespace osu.Game.Beatmaps.Formats
 
             if (hitObject is IHasCombo combo)
             {
+                type = (LegacyHitObjectType)(combo.ComboOffset << 4);
+
                 if (combo.NewCombo)
                     type |= LegacyHitObjectType.NewCombo;
-
-                if (hitObject is IHasLegacyBeatmapComboOffset comboOffset)
-                    type |= (LegacyHitObjectType)(comboOffset.LegacyBeatmapComboOffset << 4);
             }
 
             switch (hitObject)

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

@@ -18,5 +18,7 @@ namespace osu.Game.Rulesets.Objects.Legacy.Catch
         public Vector2 Position { get; set; }
 
         public bool NewCombo { get; set; }
+
+        public int ComboOffset { get; set; }
     }
 }

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

@@ -18,16 +18,21 @@ namespace osu.Game.Rulesets.Objects.Legacy.Catch
         }
 
         private bool forceNewCombo;
+        private int extraComboOffset;
 
         protected override HitObject CreateHit(Vector2 position, bool newCombo, int comboOffset)
         {
             newCombo |= forceNewCombo;
+            comboOffset += extraComboOffset;
+
             forceNewCombo = false;
+            extraComboOffset = 0;
 
             return new ConvertHit
             {
                 Position = position,
                 NewCombo = newCombo,
+                ComboOffset = comboOffset
             };
         }
 
@@ -35,12 +40,16 @@ namespace osu.Game.Rulesets.Objects.Legacy.Catch
                                                   List<IList<HitSampleInfo>> nodeSamples)
         {
             newCombo |= forceNewCombo;
+            comboOffset += extraComboOffset;
+
             forceNewCombo = false;
+            extraComboOffset = 0;
 
             return new ConvertSlider
             {
                 Position = position,
                 NewCombo = FirstObject || newCombo,
+                ComboOffset = comboOffset,
                 Path = new SliderPath(controlPoints, length),
                 NodeSamples = nodeSamples,
                 RepeatCount = repeatCount
@@ -49,6 +58,11 @@ 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
+            // Their combo offset is still added to that next hitobject's combo index
+            forceNewCombo |= FormatVersion <= 8 || newCombo;
+            extraComboOffset += comboOffset;
+
             return new ConvertSpinner
             {
                 Duration = duration

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

@@ -18,5 +18,7 @@ namespace osu.Game.Rulesets.Objects.Legacy.Catch
         public Vector2 Position { get; set; }
 
         public bool NewCombo { get; set; }
+
+        public int ComboOffset { get; set; }
     }
 }

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

@@ -17,5 +17,7 @@ namespace osu.Game.Rulesets.Objects.Legacy.Catch
         public float X => 256; // Required for CatchBeatmapConverter
 
         public bool NewCombo { get; set; }
+
+        public int ComboOffset { get; set; }
     }
 }

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

@@ -9,7 +9,7 @@ namespace osu.Game.Rulesets.Objects.Legacy.Osu
     /// <summary>
     /// Legacy osu! Hit-type, used for parsing Beatmaps.
     /// </summary>
-    internal sealed class ConvertHit : ConvertHitObject, IHasPosition, IHasCombo, IHasLegacyBeatmapComboOffset
+    internal sealed class ConvertHit : ConvertHitObject, IHasPosition, IHasCombo
     {
         public Vector2 Position { get; set; }
 
@@ -19,8 +19,6 @@ namespace osu.Game.Rulesets.Objects.Legacy.Osu
 
         public bool NewCombo { get; set; }
 
-        public int LegacyBeatmapComboOffset { get; set; }
+        public int ComboOffset { get; set; }
-
-        int IHasLegacyBeatmapComboOffset.LegacyBeatmapComboIndex { get; set; }
     }
 }

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

@@ -32,7 +32,7 @@ namespace osu.Game.Rulesets.Objects.Legacy.Osu
             {
                 Position = position,
                 NewCombo = FirstObject || newCombo,
-                LegacyBeatmapComboOffset = comboOffset
+                ComboOffset = comboOffset
             };
         }
 
@@ -49,7 +49,7 @@ namespace osu.Game.Rulesets.Objects.Legacy.Osu
             {
                 Position = position,
                 NewCombo = FirstObject || newCombo,
-                LegacyBeatmapComboOffset = comboOffset,
+                ComboOffset = comboOffset,
                 Path = new SliderPath(controlPoints, length),
                 NodeSamples = nodeSamples,
                 RepeatCount = repeatCount

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

@@ -9,7 +9,7 @@ namespace osu.Game.Rulesets.Objects.Legacy.Osu
     /// <summary>
     /// Legacy osu! Slider-type, used for parsing Beatmaps.
     /// </summary>
-    internal sealed class ConvertSlider : Legacy.ConvertSlider, IHasPosition, IHasCombo, IHasLegacyBeatmapComboOffset
+    internal sealed class ConvertSlider : Legacy.ConvertSlider, IHasPosition, IHasCombo
     {
         public Vector2 Position { get; set; }
 
@@ -19,8 +19,6 @@ namespace osu.Game.Rulesets.Objects.Legacy.Osu
 
         public bool NewCombo { get; set; }
 
-        public int LegacyBeatmapComboOffset { get; set; }
+        public int ComboOffset { get; set; }
-
-        int IHasLegacyBeatmapComboOffset.LegacyBeatmapComboIndex { get; set; }
     }
 }

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

@@ -22,5 +22,7 @@ namespace osu.Game.Rulesets.Objects.Legacy.Osu
         public float Y => Position.Y;
 
         public bool NewCombo { get; set; }
+
+        public int ComboOffset { get; set; }
     }
 }

osu.Game/Rulesets/Objects/Types/IHasCombo.cs

@@ -12,5 +12,10 @@ namespace osu.Game.Rulesets.Objects.Types
         /// Whether the HitObject starts a new combo.
         /// </summary>
         bool NewCombo { get; }
+
+        /// <summary>
+        /// When starting a new combo, the offset of the new combo relative to the current one.
+        /// </summary>
+        int ComboOffset { get; }
     }
 }

osu.Game/Rulesets/Objects/Types/IHasComboInformation.cs

@@ -26,6 +26,12 @@ namespace osu.Game.Rulesets.Objects.Types
         /// </summary>
         int ComboIndex { get; set; }
 
+        /// <summary>
+        /// A <see cref="ComboIndex"/> with the <see cref="IHasCombo.ComboOffset"/> of this and all previous hitobjects applied to it.
+        /// This is used primarily for beatmap skins during combo colour retrieval, rather than the regular <see cref="ComboIndex"/>.
+        /// </summary>
+        int BeatmapSkinComboIndex { get; set; }
+
         /// <summary>
         /// Whether the HitObject starts a new combo.
         /// </summary>

osu.Game/Rulesets/Objects/Types/IHasLegacyBeatmapComboOffset.cs

@@ -1,24 +0,0 @@
-// 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.
-
-namespace osu.Game.Rulesets.Objects.Types
-{
-    /// <summary>
-    /// A type of <see cref="HitObject"/> that has a combo index with arbitrary offsets applied to use when retrieving legacy beatmap combo colours.
-    /// This is done in stable for hitobjects to skip combo colours from the beatmap skin (known as "colour hax").
-    /// See https://osu.ppy.sh/wiki/en/osu%21_File_Formats/Osu_%28file_format%29#type for more information.
-    /// </summary>
-    public interface IHasLegacyBeatmapComboOffset
-    {
-        /// <summary>
-        /// The legacy offset of the new combo relative to the current one, when starting a new combo.
-        /// </summary>
-        int LegacyBeatmapComboOffset { get; }
-
-        /// <summary>
-        /// The combo index with the <see cref="LegacyBeatmapComboOffset"/> applied,
-        /// to use for legacy beatmap skins to decide on the combo colour.
-        /// </summary>
-        int LegacyBeatmapComboIndex { get; set; }
-    }
-}

osu.Game/Skinning/LegacyBeatmapSkin.cs

@@ -63,12 +63,7 @@ namespace osu.Game.Skinning
         }
 
         protected override IBindable<Color4> GetComboColour(IHasComboColours source, int comboIndex, IHasComboInformation combo)
-        {
+            => base.GetComboColour(source, combo.BeatmapSkinComboIndex, combo);
-            if (combo is IHasLegacyBeatmapComboOffset legacyBeatmapCombo)
-                return base.GetComboColour(source, legacyBeatmapCombo.LegacyBeatmapComboIndex, combo);
-
-            return base.GetComboColour(source, comboIndex, combo);
-        }
 
         public override ISample GetSample(ISampleInfo sampleInfo)
         {