Fix editor displaying combo colours in effectively incorrect order

[bdach]

Addresses #27316.

Stable lies about the first combo colour being first; in the .osu file it is actually second. It does a thing in editor itself to correct
for this.

I don't particularly like this solution but don't see a better one. No tests because the cost of writing tests properly is higher than the cost of fixing, and I'm not sure this'll pass scrutiny to begin with, but can add if you figure it's worthwhile.

[frenzibyte]

I'm not sure how to feel about this. I find it better to not abide by stable's rules, and change the flow of the combo colours storage and retrieval so that the first combo colour actually matches the first combo in the beatmap. Took long to put this into practice, but here's the diff:

diff --git a/osu.Game/Beatmaps/Formats/LegacyBeatmapEncoder.cs b/osu.Game/Beatmaps/Formats/LegacyBeatmapEncoder.cs
index 290d29090a..831721e6ba 100644
--- a/osu.Game/Beatmaps/Formats/LegacyBeatmapEncoder.cs
+++ b/osu.Game/Beatmaps/Formats/LegacyBeatmapEncoder.cs
@@ -318,7 +318,8 @@ private void handleColours(TextWriter writer)
 
             for (int i = 0; i < colours.Count; i++)
             {
-                var comboColour = colours[i];
+                // since we shift combo colours backwards in LegacySkinDecoder, do the same here to maintain compatibility with stable.
+                var comboColour = colours[(i + colours.Count - 1) % colours.Count];
 
                 writer.Write(FormattableString.Invariant($"Combo{1 + i}: "));
                 writer.Write(FormattableString.Invariant($"{(byte)(comboColour.R * byte.MaxValue)},"));
diff --git a/osu.Game/Rulesets/Objects/Types/IHasComboInformation.cs b/osu.Game/Rulesets/Objects/Types/IHasComboInformation.cs
index 3aa68197ec..bfd57a92a9 100644
--- a/osu.Game/Rulesets/Objects/Types/IHasComboInformation.cs
+++ b/osu.Game/Rulesets/Objects/Types/IHasComboInformation.cs
@@ -71,11 +71,10 @@ public interface IHasComboInformation : IHasCombo
         /// </summary>
         /// <param name="combo">The combo information, should be <c>this</c>.</param>
         /// <param name="skin">The skin to retrieve the combo colour from.</param>
-        /// <param name="comboIndex">The index to retrieve the combo colour with.</param>
-        /// <returns></returns>
+        /// <param name="comboIndex">The index to retrieve the combo colour with. Note that this is a one-based index, as the first combo of the beatmap is assigned a combo index of 1.</param>
         protected static Color4 GetSkinComboColour(IHasComboInformation combo, ISkin skin, int comboIndex)
         {
-            return skin.GetConfig<SkinComboColourLookup, Color4>(new SkinComboColourLookup(comboIndex, combo))?.Value ?? Color4.White;
+            return skin.GetConfig<SkinComboColourLookup, Color4>(new SkinComboColourLookup(comboIndex - 1, combo))?.Value ?? Color4.White;
         }
 
         /// <summary>
diff --git a/osu.Game/Skinning/ArgonSkin.cs b/osu.Game/Skinning/ArgonSkin.cs
index 6fcab6a977..744795ddc3 100644
--- a/osu.Game/Skinning/ArgonSkin.cs
+++ b/osu.Game/Skinning/ArgonSkin.cs
@@ -272,6 +272,12 @@ public ArgonSkin(SkinInfo skin, IStorageResourceProvider resources)
         }
 
         private static Color4 getComboColour(IHasComboColours source, int colourIndex)
-            => source.ComboColours![colourIndex % source.ComboColours.Count];
+        {
+            // colour index may be -1 on beatmaps that start with a spinner, as spinners don't add to combo index and a beatmap with spinner as first object is assigned zero combo index.
+            if (colourIndex == -1)
+                colourIndex = source.ComboColours!.Count - 1;
+
+            return source.ComboColours![colourIndex % source.ComboColours.Count];
+        }
     }
 }
diff --git a/osu.Game/Skinning/LegacyBeatmapSkin.cs b/osu.Game/Skinning/LegacyBeatmapSkin.cs
index 9cd072b607..7e00a4e25f 100644
--- a/osu.Game/Skinning/LegacyBeatmapSkin.cs
+++ b/osu.Game/Skinning/LegacyBeatmapSkin.cs
@@ -86,7 +86,8 @@ private static IResourceStore<byte[]> createRealmBackedStore(BeatmapInfo beatmap
         }
 
         protected override IBindable<Color4>? GetComboColour(IHasComboColours source, int comboIndex, IHasComboInformation combo)
-            => base.GetComboColour(source, combo.ComboIndexWithOffsets, combo);
+            // combo indices are one-based, subtract one to map the first combo to the first combo colour.
+            => base.GetComboColour(source, combo.ComboIndexWithOffsets - 1, combo);
 
         public override ISample? GetSample(ISampleInfo sampleInfo)
         {
diff --git a/osu.Game/Skinning/LegacySkin.cs b/osu.Game/Skinning/LegacySkin.cs
index 816cfc0a2d..bebc723e88 100644
--- a/osu.Game/Skinning/LegacySkin.cs
+++ b/osu.Game/Skinning/LegacySkin.cs
@@ -292,7 +292,17 @@ protected override void ParseConfigurationStream(Stream stream)
         /// <param name="combo">Information on the combo whose using the returned colour.</param>
         protected virtual IBindable<Color4>? GetComboColour(IHasComboColours source, int colourIndex, IHasComboInformation combo)
         {
-            var colour = source.ComboColours?[colourIndex % source.ComboColours.Count];
+            Color4? colour = null;
+
+            if (source.ComboColours != null)
+            {
+                // colour index may be -1 on beatmaps that start with a spinner, as spinners don't add to combo index and a beatmap with spinner as first object is assigned zero combo index.
+                if (colourIndex == -1)
+                    colourIndex = source.ComboColours.Count - 1;
+
+                colour = source.ComboColours?[colourIndex % source.ComboColours.Count];
+            }
+
             return colour.HasValue ? new Bindable<Color4>(colour.Value) : null;
         }
 
diff --git a/osu.Game/Skinning/LegacySkinDecoder.cs b/osu.Game/Skinning/LegacySkinDecoder.cs
index 1270f69339..4e5632ab13 100644
--- a/osu.Game/Skinning/LegacySkinDecoder.cs
+++ b/osu.Game/Skinning/LegacySkinDecoder.cs
@@ -2,7 +2,9 @@
 // See the LICENCE file in the repository root for full licence text.
 
 using System.Globalization;
+using System.Linq;
 using osu.Game.Beatmaps.Formats;
+using osu.Game.IO;
 
 namespace osu.Game.Skinning
 {
@@ -63,5 +65,18 @@ protected override SkinConfiguration CreateTemplateObject()
             config.LegacyVersion = 1.0m;
             return config;
         }
+
+        protected override void ParseStreamInto(LineBufferedReader stream, SkinConfiguration output)
+        {
+            base.ParseStreamInto(stream, output);
+
+            // in stable, the second combo colour represents the first combo in the beatmap.
+            // we don't want that to stay the same in lazer.
+            if (output.CustomComboColours.Any())
+            {
+                output.CustomComboColours.Add(output.CustomComboColours[0]);
+                output.CustomComboColours.RemoveAt(0);
+            }
+        }
     }
 }
diff --git a/osu.Game/Skinning/SkinComboColourLookup.cs b/osu.Game/Skinning/SkinComboColourLookup.cs
index 33e35a96fb..9b1142b11d 100644
--- a/osu.Game/Skinning/SkinComboColourLookup.cs
+++ b/osu.Game/Skinning/SkinComboColourLookup.cs
@@ -8,7 +8,7 @@ namespace osu.Game.Skinning
     public class SkinComboColourLookup
     {
         /// <summary>
-        /// The index to use for deciding the combo colour.
+        /// A zero-based index of the combo colour in the skin.
         /// </summary>
         public readonly int ColourIndex;
 
diff --git a/osu.Game/Skinning/TrianglesSkin.cs b/osu.Game/Skinning/TrianglesSkin.cs
index a2dca5d333..f979154d0c 100644
--- a/osu.Game/Skinning/TrianglesSkin.cs
+++ b/osu.Game/Skinning/TrianglesSkin.cs
@@ -208,6 +208,12 @@ public TrianglesSkin(SkinInfo skin, IStorageResourceProvider resources)
         }
 
         private static Color4 getComboColour(IHasComboColours source, int colourIndex)
-            => source.ComboColours![colourIndex % source.ComboColours.Count];
+        {
+            // colour index may be -1 on beatmaps that start with a spinner, as spinners don't add to combo index and a beatmap with spinner as first object is assigned zero combo index.
+            if (colourIndex == -1)
+                colourIndex = source.ComboColours!.Count - 1;
+
+            return source.ComboColours![colourIndex % source.ComboColours.Count];
+        }
     }
 }

One thing that stands out is that there is the exception of a beatmap starting with a spinner, in which said spinner is assigned a combo index of 0, completely violating the idea that combo indices are one-based. This is locally handled in each place getComboColour is used by guarding against negative index lookup.

cc @ppy/team-client to get further thoughts on this.

[peppy]

This is how i would have done it 🤷 seems fine