Merge pull request #4209 from peppy/fix-combo-color-fallbacks

Fix combo colour fallbacks when skin is not providing any

osu.Game.Rulesets.Catch/Objects/Drawable/DrawableCatchHitObject.cs

@@ -65,7 +65,7 @@ namespace osu.Game.Rulesets.Catch.Objects.Drawable
             base.SkinChanged(skin, allowFallback);
 
             if (HitObject is IHasComboInformation combo)
-                AccentColour = skin.GetValue<SkinConfiguration, Color4>(s => s.ComboColours.Count > 0 ? s.ComboColours[combo.ComboIndex % s.ComboColours.Count] : Color4.White);
+                AccentColour = skin.GetValue<SkinConfiguration, Color4?>(s => s.ComboColours.Count > 0 ? s.ComboColours[combo.ComboIndex % s.ComboColours.Count] : (Color4?)null) ?? Color4.White;
         }
 
         private const float preempt = 1000;

osu.Game.Rulesets.Osu/Objects/Drawables/DrawableOsuHitObject.cs

@@ -58,7 +58,7 @@ namespace osu.Game.Rulesets.Osu.Objects.Drawables
             base.SkinChanged(skin, allowFallback);
 
             if (HitObject is IHasComboInformation combo)
-                AccentColour = skin.GetValue<SkinConfiguration, Color4>(s => s.ComboColours.Count > 0 ? s.ComboColours[combo.ComboIndex % s.ComboColours.Count] : Color4.White);
+                AccentColour = skin.GetValue<SkinConfiguration, Color4?>(s => s.ComboColours.Count > 0 ? s.ComboColours[combo.ComboIndex % s.ComboColours.Count] : (Color4?)null) ?? Color4.White;
         }
 
         protected virtual void UpdatePreemptState() => this.FadeIn(HitObject.TimeFadeIn);

osu.Game.Rulesets.Osu/Objects/Drawables/DrawableSlider.cs

@@ -156,9 +156,9 @@ namespace osu.Game.Rulesets.Osu.Objects.Drawables
         {
             base.SkinChanged(skin, allowFallback);
 
-            Body.AccentColour = skin.GetValue<SkinConfiguration, Color4>(s => s.CustomColours.ContainsKey("SliderTrackOverride") ? s.CustomColours["SliderTrackOverride"] : Body.AccentColour);
+            Body.AccentColour = skin.GetValue<SkinConfiguration, Color4?>(s => s.CustomColours.ContainsKey("SliderTrackOverride") ? s.CustomColours["SliderTrackOverride"] : (Color4?)null) ?? Body.AccentColour;
-            Body.BorderColour = skin.GetValue<SkinConfiguration, Color4>(s => s.CustomColours.ContainsKey("SliderBorder") ? s.CustomColours["SliderBorder"] : Body.BorderColour);
+            Body.BorderColour = skin.GetValue<SkinConfiguration, Color4?>(s => s.CustomColours.ContainsKey("SliderBorder") ? s.CustomColours["SliderBorder"] : (Color4?)null) ?? Body.BorderColour;
-            Ball.AccentColour = skin.GetValue<SkinConfiguration, Color4>(s => s.CustomColours.ContainsKey("SliderBall") ? s.CustomColours["SliderBall"] : Ball.AccentColour);
+            Ball.AccentColour = skin.GetValue<SkinConfiguration, Color4?>(s => s.CustomColours.ContainsKey("SliderBall") ? s.CustomColours["SliderBall"] : (Color4?)null) ?? Ball.AccentColour;
         }
 
         protected override void CheckForResult(bool userTriggered, double timeOffset)

osu.Game.Tests/Resources/skin-empty.ini

@@ -0,0 +1,2 @@
+[General]
+Name: test skin

osu.Game.Tests/Resources/skin.ini

@@ -0,0 +1,8 @@
+[General]
+Name: test skin
+
+[Colours]
+Combo1 : 142,199,255
+Combo2 : 255,128,128
+Combo3 : 128,255,255
+Combo7 : 100,100,100,100

osu.Game.Tests/Skins/LegacySkinDecoderTest.cs

@@ -0,0 +1,44 @@
+// 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.Collections.Generic;
+using System.IO;
+using NUnit.Framework;
+using osu.Game.Skinning;
+using osu.Game.Tests.Resources;
+using osuTK.Graphics;
+
+namespace osu.Game.Tests.Skins
+{
+    [TestFixture]
+    public class LegacySkinDecoderTest
+    {
+        [TestCase(true)]
+        [TestCase(false)]
+        public void TestDecodeSkinColours(bool hasColours)
+        {
+            var decoder = new LegacySkinDecoder();
+            using (var resStream = TestResources.OpenResource(hasColours ? "skin.ini" : "skin-empty.ini"))
+            using (var stream = new StreamReader(resStream))
+            {
+                var comboColors = decoder.Decode(stream).ComboColours;
+
+                List<Color4> expectedColors;
+                if (hasColours)
+                    expectedColors = new List<Color4>
+                    {
+                        new Color4(142, 199, 255, 255),
+                        new Color4(255, 128, 128, 255),
+                        new Color4(128, 255, 255, 255),
+                        new Color4(100, 100, 100, 100),
+                    };
+                else
+                    expectedColors = new DefaultSkin().Configuration.ComboColours;
+
+                Assert.AreEqual(expectedColors.Count, comboColors.Count);
+                for (int i = 0; i < expectedColors.Count; i++)
+                    Assert.AreEqual(expectedColors[i], comboColors[i]);
+            }
+        }
+    }
+}

osu.Game/Skinning/DefaultSkin.cs

@@ -4,7 +4,6 @@
 using osu.Framework.Audio.Sample;
 using osu.Framework.Graphics;
 using osu.Framework.Graphics.Textures;
-using osuTK.Graphics;
 
 namespace osu.Game.Skinning
 {
@@ -13,16 +12,7 @@ namespace osu.Game.Skinning
         public DefaultSkin()
             : base(SkinInfo.Default)
         {
-            Configuration = new SkinConfiguration
+            Configuration = new SkinConfiguration();
-            {
-                ComboColours =
-                {
-                    new Color4(17, 136, 170, 255),
-                    new Color4(102, 136, 0, 255),
-                    new Color4(204, 102, 0, 255),
-                    new Color4(121, 9, 13, 255)
-                }
-            };
         }
 
         public override Drawable GetDrawableComponent(string componentName) => null;

osu.Game/Skinning/LocalSkinOverrideContainer.cs

@@ -12,6 +12,9 @@ using osu.Game.Configuration;
 
 namespace osu.Game.Skinning
 {
+    /// <summary>
+    /// A container which overrides existing skin options with beatmap-local values.
+    /// </summary>
     public class LocalSkinOverrideContainer : Container, ISkinSource
     {
         public event Action SourceChanged;
@@ -19,6 +22,14 @@ namespace osu.Game.Skinning
         private readonly Bindable<bool> beatmapSkins = new Bindable<bool>();
         private readonly Bindable<bool> beatmapHitsounds = new Bindable<bool>();
 
+        private readonly ISkinSource source;
+        private ISkinSource fallbackSource;
+
+        public LocalSkinOverrideContainer(ISkinSource source)
+        {
+            this.source = source;
+        }
+
         public Drawable GetDrawableComponent(string componentName)
         {
             Drawable sourceDrawable;
@@ -53,14 +64,6 @@ namespace osu.Game.Skinning
             return fallbackSource == null ? default : fallbackSource.GetValue(query);
         }
 
-        private readonly ISkinSource source;
-        private ISkinSource fallbackSource;
-
-        public LocalSkinOverrideContainer(ISkinSource source)
-        {
-            this.source = source;
-        }
-
         private void onSourceChanged() => SourceChanged?.Invoke();
 
         protected override IReadOnlyDependencyContainer CreateChildDependencies(IReadOnlyDependencyContainer parent)

osu.Game/Skinning/SkinConfiguration.cs

@@ -11,7 +11,13 @@ namespace osu.Game.Skinning
     {
         public readonly SkinInfo SkinInfo = new SkinInfo();
 
-        public List<Color4> ComboColours { get; set; } = new List<Color4>();
+        public List<Color4> ComboColours { get; set; } = new List<Color4>
+        {
+            new Color4(17, 136, 170, 255),
+            new Color4(102, 136, 0, 255),
+            new Color4(204, 102, 0, 255),
+            new Color4(121, 9, 13, 255)
+        };
 
         public Dictionary<string, Color4> CustomColours { get; set; } = new Dictionary<string, Color4>();