Merge pull request #12683 from frenzibyte/legacy-beatmap-combo-offset

Apply combo offsets "colour hax" only on beatmap skins

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

@@ -95,6 +95,14 @@ namespace osu.Game.Rulesets.Catch.Objects
             set => ComboIndexBindable.Value = value;
         }
 
+        public Bindable<int> ComboIndexWithOffsetsBindable { get; } = new Bindable<int>();
+
+        public int ComboIndexWithOffsets
+        {
+            get => ComboIndexWithOffsetsBindable.Value;
+            set => ComboIndexWithOffsetsBindable.Value = value;
+        }
+
         public Bindable<bool> LastInComboBindable { get; } = new Bindable<bool>();
 
         /// <summary>

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

@@ -1,16 +1,21 @@
 // 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.
 
+using System;
+using System.Collections.Generic;
 using System.Linq;
 using NUnit.Framework;
 using osu.Framework.Allocation;
 using osu.Framework.Audio;
 using osu.Game.Beatmaps;
 using osu.Game.Configuration;
+using osu.Game.Rulesets.Objects;
+using osu.Game.Rulesets.Objects.Drawables;
 using osu.Game.Rulesets.Osu.Objects;
 using osu.Game.Skinning;
 using osu.Game.Tests.Beatmaps;
 using osuTK;
+using osuTK.Graphics;
 
 namespace osu.Game.Rulesets.Osu.Tests
 {
@@ -77,23 +82,106 @@ namespace osu.Game.Rulesets.Osu.Tests
             AddAssert("is custom user skin colours", () => TestPlayer.UsableComboColours.SequenceEqual(TestSkin.Colours));
         }
 
+        [TestCase(true, true)]
+        [TestCase(true, false)]
+        [TestCase(false, true)]
+        [TestCase(false, false)]
+        public void TestComboOffsetWithBeatmapColours(bool userHasCustomColours, bool useBeatmapSkin)
+        {
+            PrepareBeatmap(() => new OsuCustomSkinWorkingBeatmap(audio, true, getHitCirclesWithLegacyOffsets()));
+            ConfigureTest(useBeatmapSkin, true, userHasCustomColours);
+            assertCorrectObjectComboColours("is beatmap skin colours with combo offsets applied",
+                TestBeatmapSkin.Colours,
+                (i, obj) => i + 1 + obj.ComboOffset);
+        }
+
+        [TestCase(true)]
+        [TestCase(false)]
+        public void TestComboOffsetWithIgnoredBeatmapColours(bool useBeatmapSkin)
+        {
+            PrepareBeatmap(() => new OsuCustomSkinWorkingBeatmap(audio, true, getHitCirclesWithLegacyOffsets()));
+            ConfigureTest(useBeatmapSkin, false, true);
+            assertCorrectObjectComboColours("is user skin colours without combo offsets applied",
+                TestSkin.Colours,
+                (i, _) => i + 1);
+        }
+
+        private void assertCorrectObjectComboColours(string description, Color4[] expectedColours, Func<int, OsuHitObject, int> nextExpectedComboIndex)
+        {
+            AddUntilStep("wait for objects to become alive", () =>
+                TestPlayer.DrawableRuleset.Playfield.AllHitObjects.Count() == TestPlayer.DrawableRuleset.Objects.Count());
+
+            AddAssert(description, () =>
+            {
+                int index = 0;
+
+                return TestPlayer.DrawableRuleset.Playfield.AllHitObjects.All(d =>
+                {
+                    index = nextExpectedComboIndex(index, (OsuHitObject)d.HitObject);
+                    return checkComboColour(d, expectedColours[index % expectedColours.Length]);
+                });
+            });
+
+            static bool checkComboColour(DrawableHitObject drawableHitObject, Color4 expectedColour)
+            {
+                return drawableHitObject.AccentColour.Value == expectedColour &&
+                       drawableHitObject.NestedHitObjects.All(n => checkComboColour(n, expectedColour));
+            }
+        }
+
+        private static IEnumerable<OsuHitObject> getHitCirclesWithLegacyOffsets()
+        {
+            var hitObjects = new List<OsuHitObject>();
+
+            for (int i = 0; i < 10; i++)
+            {
+                var hitObject = i % 2 == 0
+                    ? (OsuHitObject)new HitCircle()
+                    : new Slider
+                    {
+                        Path = new SliderPath(new[]
+                        {
+                            new PathControlPoint(new Vector2(0, 0)),
+                            new PathControlPoint(new Vector2(100, 0)),
+                        })
+                    };
+
+                hitObject.StartTime = i;
+                hitObject.Position = new Vector2(256, 192);
+                hitObject.NewCombo = true;
+                hitObject.ComboOffset = i;
+
+                hitObjects.Add(hitObject);
+            }
+
+            return hitObjects;
+        }
+
         private class OsuCustomSkinWorkingBeatmap : CustomSkinWorkingBeatmap
         {
-            public OsuCustomSkinWorkingBeatmap(AudioManager audio, bool hasColours)
+            public OsuCustomSkinWorkingBeatmap(AudioManager audio, bool hasColours, IEnumerable<OsuHitObject> hitObjects = null)
-                : base(createBeatmap(), audio, hasColours)
+                : base(createBeatmap(hitObjects), audio, hasColours)
             {
             }
 
-            private static IBeatmap createBeatmap() =>
+            private static IBeatmap createBeatmap(IEnumerable<OsuHitObject> hitObjects)
-                new Beatmap
+            {
+                var beatmap = new Beatmap
                 {
                     BeatmapInfo =
                     {
                         BeatmapSet = new BeatmapSetInfo(),
                         Ruleset = new OsuRuleset().RulesetInfo,
                     },
-                    HitObjects = { new HitCircle { Position = new Vector2(256, 192) } }
                 };
+
+                beatmap.HitObjects.AddRange(hitObjects ?? new[]
+                {
+                    new HitCircle { Position = new Vector2(256, 192) }
+                });
+
+                return beatmap;
+            }
         }
     }
 }

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

@@ -97,6 +97,14 @@ namespace osu.Game.Rulesets.Osu.Objects
             set => ComboIndexBindable.Value = value;
         }
 
+        public Bindable<int> ComboIndexWithOffsetsBindable { get; } = new Bindable<int>();
+
+        public int ComboIndexWithOffsets
+        {
+            get => ComboIndexWithOffsetsBindable.Value;
+            set => ComboIndexWithOffsetsBindable.Value = value;
+        }
+
         public Bindable<bool> LastInComboBindable { get; } = new Bindable<bool>();
 
         public bool LastInCombo

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

@@ -323,12 +323,12 @@ namespace osu.Game.Tests.Beatmaps.Formats
                 new OsuBeatmapProcessor(converted).PreProcess();
                 new OsuBeatmapProcessor(converted).PostProcess();
 
-                Assert.AreEqual(4, ((IHasComboInformation)converted.HitObjects.ElementAt(0)).ComboIndex);
+                Assert.AreEqual(4, ((IHasComboInformation)converted.HitObjects.ElementAt(0)).ComboIndexWithOffsets);
-                Assert.AreEqual(5, ((IHasComboInformation)converted.HitObjects.ElementAt(2)).ComboIndex);
+                Assert.AreEqual(5, ((IHasComboInformation)converted.HitObjects.ElementAt(2)).ComboIndexWithOffsets);
-                Assert.AreEqual(5, ((IHasComboInformation)converted.HitObjects.ElementAt(4)).ComboIndex);
+                Assert.AreEqual(5, ((IHasComboInformation)converted.HitObjects.ElementAt(4)).ComboIndexWithOffsets);
-                Assert.AreEqual(6, ((IHasComboInformation)converted.HitObjects.ElementAt(6)).ComboIndex);
+                Assert.AreEqual(6, ((IHasComboInformation)converted.HitObjects.ElementAt(6)).ComboIndexWithOffsets);
-                Assert.AreEqual(11, ((IHasComboInformation)converted.HitObjects.ElementAt(8)).ComboIndex);
+                Assert.AreEqual(11, ((IHasComboInformation)converted.HitObjects.ElementAt(8)).ComboIndexWithOffsets);
-                Assert.AreEqual(14, ((IHasComboInformation)converted.HitObjects.ElementAt(11)).ComboIndex);
+                Assert.AreEqual(14, ((IHasComboInformation)converted.HitObjects.ElementAt(11)).ComboIndexWithOffsets);
             }
         }
 
@@ -346,12 +346,12 @@ namespace osu.Game.Tests.Beatmaps.Formats
                 new CatchBeatmapProcessor(converted).PreProcess();
                 new CatchBeatmapProcessor(converted).PostProcess();
 
-                Assert.AreEqual(4, ((IHasComboInformation)converted.HitObjects.ElementAt(0)).ComboIndex);
+                Assert.AreEqual(4, ((IHasComboInformation)converted.HitObjects.ElementAt(0)).ComboIndexWithOffsets);
-                Assert.AreEqual(5, ((IHasComboInformation)converted.HitObjects.ElementAt(2)).ComboIndex);
+                Assert.AreEqual(5, ((IHasComboInformation)converted.HitObjects.ElementAt(2)).ComboIndexWithOffsets);
-                Assert.AreEqual(5, ((IHasComboInformation)converted.HitObjects.ElementAt(4)).ComboIndex);
+                Assert.AreEqual(5, ((IHasComboInformation)converted.HitObjects.ElementAt(4)).ComboIndexWithOffsets);
-                Assert.AreEqual(6, ((IHasComboInformation)converted.HitObjects.ElementAt(6)).ComboIndex);
+                Assert.AreEqual(6, ((IHasComboInformation)converted.HitObjects.ElementAt(6)).ComboIndexWithOffsets);
-                Assert.AreEqual(11, ((IHasComboInformation)converted.HitObjects.ElementAt(8)).ComboIndex);
+                Assert.AreEqual(11, ((IHasComboInformation)converted.HitObjects.ElementAt(8)).ComboIndexWithOffsets);
-                Assert.AreEqual(14, ((IHasComboInformation)converted.HitObjects.ElementAt(11)).ComboIndex);
+                Assert.AreEqual(14, ((IHasComboInformation)converted.HitObjects.ElementAt(11)).ComboIndexWithOffsets);
             }
         }
 

osu.Game.Tests/Gameplay/TestSceneHitObjectAccentColour.cs

@@ -100,6 +100,14 @@ namespace osu.Game.Tests.Gameplay
                 set => ComboIndexBindable.Value = value;
             }
 
+            public Bindable<int> ComboIndexWithOffsetsBindable { get; } = new Bindable<int>();
+
+            public int ComboIndexWithOffsets
+            {
+                get => ComboIndexWithOffsetsBindable.Value;
+                set => ComboIndexWithOffsetsBindable.Value = value;
+            }
+
             public Bindable<bool> LastInComboBindable { get; } = new Bindable<bool>();
 
             public bool LastInCombo

osu.Game/Beatmaps/BeatmapProcessor.cs

@@ -34,19 +34,19 @@ namespace osu.Game.Beatmaps
                     isFirst = false;
                 }
 
+                obj.ComboIndex = lastObj?.ComboIndex ?? 0;
+                obj.ComboIndexWithOffsets = lastObj?.ComboIndexWithOffsets ?? 0;
+                obj.IndexInCurrentCombo = (lastObj?.IndexInCurrentCombo + 1) ?? 0;
+
                 if (obj.NewCombo)
                 {
                     obj.IndexInCurrentCombo = 0;
-                    obj.ComboIndex = (lastObj?.ComboIndex ?? 0) + obj.ComboOffset + 1;
+                    obj.ComboIndex++;
+                    obj.ComboIndexWithOffsets += obj.ComboOffset + 1;
 
                     if (lastObj != null)
                         lastObj.LastInCombo = true;
                 }
-                else if (lastObj != null)
-                {
-                    obj.IndexInCurrentCombo = lastObj.IndexInCurrentCombo + 1;
-                    obj.ComboIndex = lastObj.ComboIndex;
-                }
 
                 lastObj = obj;
             }

osu.Game/Rulesets/Objects/Drawables/DrawableHitObject.cs

@@ -124,7 +124,9 @@ namespace osu.Game.Rulesets.Objects.Drawables
         public readonly Bindable<double> StartTimeBindable = new Bindable<double>();
         private readonly BindableList<HitSampleInfo> samplesBindable = new BindableList<HitSampleInfo>();
         private readonly Bindable<bool> userPositionalHitSounds = new Bindable<bool>();
+
         private readonly Bindable<int> comboIndexBindable = new Bindable<int>();
+        private readonly Bindable<int> comboIndexWithOffsetsBindable = new Bindable<int>();
 
         protected override bool RequiresChildrenUpdate => true;
 
@@ -185,7 +187,8 @@ namespace osu.Game.Rulesets.Objects.Drawables
         {
             base.LoadComplete();
 
-            comboIndexBindable.BindValueChanged(_ => UpdateComboColour(), true);
+            comboIndexBindable.BindValueChanged(_ => UpdateComboColour());
+            comboIndexWithOffsetsBindable.BindValueChanged(_ => UpdateComboColour(), true);
 
             updateState(ArmedState.Idle, true);
         }
@@ -250,7 +253,10 @@ namespace osu.Game.Rulesets.Objects.Drawables
             StartTimeBindable.BindValueChanged(onStartTimeChanged);
 
             if (HitObject is IHasComboInformation combo)
+            {
                 comboIndexBindable.BindTo(combo.ComboIndexBindable);
+                comboIndexWithOffsetsBindable.BindTo(combo.ComboIndexWithOffsetsBindable);
+            }
 
             samplesBindable.BindTo(HitObject.SamplesBindable);
             samplesBindable.BindCollectionChanged(onSamplesChanged, true);
@@ -275,8 +281,13 @@ namespace osu.Game.Rulesets.Objects.Drawables
         protected sealed override void OnFree(HitObjectLifetimeEntry entry)
         {
             StartTimeBindable.UnbindFrom(HitObject.StartTimeBindable);
+
             if (HitObject is IHasComboInformation combo)
+            {
                 comboIndexBindable.UnbindFrom(combo.ComboIndexBindable);
+                comboIndexWithOffsetsBindable.UnbindFrom(combo.ComboIndexWithOffsetsBindable);
+            }
+
             samplesBindable.UnbindFrom(HitObject.SamplesBindable);
 
             // Changes in start time trigger state updates. When a new hitobject is applied, OnApply() automatically performs a state update anyway.

osu.Game/Rulesets/Objects/HitObject.cs

@@ -118,6 +118,7 @@ namespace osu.Game.Rulesets.Objects
                 foreach (var n in NestedHitObjects.OfType<IHasComboInformation>())
                 {
                     n.ComboIndexBindable.BindTo(hasCombo.ComboIndexBindable);
+                    n.ComboIndexWithOffsetsBindable.BindTo(hasCombo.ComboIndexWithOffsetsBindable);
                     n.IndexInCurrentComboBindable.BindTo(hasCombo.IndexInCurrentComboBindable);
                 }
             }

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

@@ -15,17 +15,25 @@ namespace osu.Game.Rulesets.Objects.Types
         Bindable<int> IndexInCurrentComboBindable { get; }
 
         /// <summary>
-        /// The offset of this hitobject in the current combo.
+        /// The index of this hitobject in the current combo.
         /// </summary>
         int IndexInCurrentCombo { get; set; }
 
         Bindable<int> ComboIndexBindable { get; }
 
         /// <summary>
-        /// The offset of this combo in relation to the beatmap.
+        /// The index of this combo in relation to the beatmap.
         /// </summary>
         int ComboIndex { get; set; }
 
+        Bindable<int> ComboIndexWithOffsetsBindable { get; }
+
+        /// <summary>
+        /// The index of this combo in relation to the beatmap, with all aggregate <see cref="IHasCombo.ComboOffset"/>s applied.
+        /// This should be used instead of <see cref="ComboIndex"/> only when retrieving combo colours from the beatmap's skin.
+        /// </summary>
+        int ComboIndexWithOffsets { get; set; }
+
         /// <summary>
         /// Whether the HitObject starts a new combo.
         /// </summary>

osu.Game/Screens/Edit/Compose/Components/Timeline/TimelineHitObjectBlueprint.cs

@@ -37,7 +37,10 @@ namespace osu.Game.Screens.Edit.Compose.Components.Timeline
         private readonly Bindable<double> startTime;
 
         private Bindable<int> indexInCurrentComboBindable;
+
         private Bindable<int> comboIndexBindable;
+        private Bindable<int> comboIndexWithOffsetsBindable;
+
         private Bindable<Color4> displayColourBindable;
 
         private readonly ExtendableCircle circle;
@@ -120,7 +123,10 @@ namespace osu.Game.Screens.Edit.Compose.Components.Timeline
                     indexInCurrentComboBindable.BindValueChanged(_ => updateComboIndex(), true);
 
                     comboIndexBindable = comboInfo.ComboIndexBindable.GetBoundCopy();
-                    comboIndexBindable.BindValueChanged(_ => updateColour(), true);
+                    comboIndexWithOffsetsBindable = comboInfo.ComboIndexWithOffsetsBindable.GetBoundCopy();
+
+                    comboIndexBindable.BindValueChanged(_ => updateColour());
+                    comboIndexWithOffsetsBindable.BindValueChanged(_ => updateColour(), true);
 
                     skin.SourceChanged += updateColour;
                     break;

osu.Game/Skinning/LegacyBeatmapSkin.cs

@@ -7,8 +7,11 @@ using osu.Framework.Graphics;
 using osu.Framework.IO.Stores;
 using osu.Game.Audio;
 using osu.Game.Beatmaps;
+using osu.Game.Beatmaps.Formats;
 using osu.Game.IO;
 using osu.Game.Rulesets.Objects.Legacy;
+using osu.Game.Rulesets.Objects.Types;
+using osuTK.Graphics;
 
 namespace osu.Game.Skinning
 {
@@ -59,6 +62,9 @@ namespace osu.Game.Skinning
             return base.GetConfig<TLookup, TValue>(lookup);
         }
 
+        protected override IBindable<Color4> GetComboColour(IHasComboColours source, int comboIndex, IHasComboInformation combo)
+            => base.GetComboColour(source, combo.ComboIndexWithOffsets, combo);
+
         public override ISample GetSample(ISampleInfo sampleInfo)
         {
             if (sampleInfo is ConvertHitObjectParser.LegacyHitSampleInfo legacy && legacy.CustomSampleBank == 0)

osu.Game/Tests/Beatmaps/LegacyBeatmapSkinColourTest.cs

@@ -10,7 +10,6 @@ using osu.Framework.Bindables;
 using osu.Framework.IO.Stores;
 using osu.Framework.Testing;
 using osu.Game.Beatmaps;
-using osu.Game.Screens.Play;
 using osu.Game.Skinning;
 using osu.Game.Tests.Visual;
 using osuTK.Graphics;
@@ -60,16 +59,12 @@ namespace osu.Game.Tests.Beatmaps
 
         protected virtual ExposedPlayer CreateTestPlayer(bool userHasCustomColours) => new ExposedPlayer(userHasCustomColours);
 
-        protected class ExposedPlayer : Player
+        protected class ExposedPlayer : TestPlayer
         {
             protected readonly bool UserHasCustomColours;
 
             public ExposedPlayer(bool userHasCustomColours)
-                : base(new PlayerConfiguration
+                : base(false, false)
-                {
-                    AllowPause = false,
-                    ShowResults = false,
-                })
             {
                 UserHasCustomColours = userHasCustomColours;
             }
@@ -106,6 +101,8 @@ namespace osu.Game.Tests.Beatmaps
             {
                 new Color4(50, 100, 150, 255),
                 new Color4(40, 80, 120, 255),
+                new Color4(25, 50, 75, 255),
+                new Color4(10, 20, 30, 255),
             };
 
             public static readonly Color4 HYPER_DASH_COLOUR = Color4.DarkBlue;
@@ -133,6 +130,8 @@ namespace osu.Game.Tests.Beatmaps
             {
                 new Color4(150, 100, 50, 255),
                 new Color4(20, 20, 20, 255),
+                new Color4(75, 50, 25, 255),
+                new Color4(80, 80, 80, 255),
             };
 
             public static readonly Color4 HYPER_DASH_COLOUR = Color4.LightBlue;