diff --git a/osu.Game.Rulesets.Osu.Tests/TestSceneLegacyBeatmapSkin.cs b/osu.Game.Rulesets.Osu.Tests/TestSceneLegacyBeatmapSkin.cs new file mode 100644 index 0000000000..4676f14655 --- /dev/null +++ b/osu.Game.Rulesets.Osu.Tests/TestSceneLegacyBeatmapSkin.cs @@ -0,0 +1,155 @@ +// Copyright (c) ppy Pty Ltd . 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.Framework.Graphics; +using osu.Framework.IO.Stores; +using osu.Framework.Testing; +using osu.Game.Beatmaps; +using osu.Game.Rulesets.Osu.Objects; +using osu.Game.Screens; +using osu.Game.Screens.Play; +using osu.Game.Skinning; +using osu.Game.Tests.Visual; +using osuTK; +using osuTK.Graphics; + +namespace osu.Game.Rulesets.Osu.Tests +{ + public class TestSceneLegacyBeatmapSkin : OsuTestScene + { + [Resolved] + private AudioManager audio { get; set; } + + [TestCase(true)] + [TestCase(false)] + public void TestBeatmapComboColours(bool customSkinColoursPresent) + { + ExposedPlayer player = null; + + AddStep("load coloured beatmap", () => player = loadBeatmap(customSkinColoursPresent, true)); + AddUntilStep("wait for player", () => player.IsLoaded); + + AddAssert("is beatmap skin colours", () => player.UsableComboColours.SequenceEqual(TestBeatmapSkin.Colours)); + } + + [Test] + public void TestBeatmapNoComboColours() + { + ExposedPlayer player = null; + + AddStep("load no-colour beatmap", () => player = loadBeatmap(false, false)); + AddUntilStep("wait for player", () => player.IsLoaded); + + AddAssert("is default user skin colours", () => player.UsableComboColours.SequenceEqual(SkinConfiguration.DefaultComboColours)); + } + + [Test] + public void TestBeatmapNoComboColoursSkinOverride() + { + ExposedPlayer player = null; + + AddStep("load custom-skin colour", () => player = loadBeatmap(true, false)); + AddUntilStep("wait for player", () => player.IsLoaded); + + AddAssert("is custom user skin colours", () => player.UsableComboColours.SequenceEqual(TestSkin.Colours)); + } + + private ExposedPlayer loadBeatmap(bool userHasCustomColours, bool beatmapHasColours) + { + ExposedPlayer player; + + Beatmap.Value = new CustomSkinWorkingBeatmap(audio, beatmapHasColours); + Child = new OsuScreenStack(player = new ExposedPlayer(userHasCustomColours)) { RelativeSizeAxes = Axes.Both }; + + return player; + } + + private class ExposedPlayer : Player + { + private readonly bool userHasCustomColours; + + public ExposedPlayer(bool userHasCustomColours) + : base(false, false) + { + this.userHasCustomColours = userHasCustomColours; + } + + protected override IReadOnlyDependencyContainer CreateChildDependencies(IReadOnlyDependencyContainer parent) + { + var dependencies = new DependencyContainer(base.CreateChildDependencies(parent)); + dependencies.CacheAs(new TestSkin(userHasCustomColours)); + return dependencies; + } + + public IReadOnlyList UsableComboColours => + GameplayClockContainer.ChildrenOfType() + .First() + .GetConfig>(GlobalSkinConfiguration.ComboColours)?.Value; + } + + private class CustomSkinWorkingBeatmap : ClockBackedTestWorkingBeatmap + { + private readonly bool hasColours; + + public CustomSkinWorkingBeatmap(AudioManager audio, bool hasColours) + : base(new Beatmap + { + BeatmapInfo = + { + BeatmapSet = new BeatmapSetInfo(), + Ruleset = new OsuRuleset().RulesetInfo, + }, + HitObjects = { new HitCircle { Position = new Vector2(256, 192) } } + }, null, null, audio) + { + this.hasColours = hasColours; + } + + protected override ISkin GetSkin() => new TestBeatmapSkin(BeatmapInfo, hasColours); + } + + private class TestBeatmapSkin : LegacyBeatmapSkin + { + public static Color4[] Colours { get; } = + { + new Color4(50, 100, 150, 255), + new Color4(40, 80, 120, 255), + }; + + public TestBeatmapSkin(BeatmapInfo beatmap, bool hasColours) + : base(beatmap, new ResourceStore(), null) + { + if (hasColours) + Configuration.AddComboColours(Colours); + } + } + + private class TestSkin : LegacySkin, ISkinSource + { + public static Color4[] Colours { get; } = + { + new Color4(150, 100, 50, 255), + new Color4(20, 20, 20, 255), + }; + + public TestSkin(bool hasCustomColours) + : base(new SkinInfo(), null, null, string.Empty) + { + if (hasCustomColours) + Configuration.AddComboColours(Colours); + } + + public event Action SourceChanged + { + add { } + remove { } + } + } + } +} diff --git a/osu.Game.Tests/Gameplay/TestSceneHitObjectAccentColour.cs b/osu.Game.Tests/Gameplay/TestSceneHitObjectAccentColour.cs index 6d7159a825..c6d1f9da29 100644 --- a/osu.Game.Tests/Gameplay/TestSceneHitObjectAccentColour.cs +++ b/osu.Game.Tests/Gameplay/TestSceneHitObjectAccentColour.cs @@ -130,7 +130,7 @@ namespace osu.Game.Tests.Gameplay switch (global) { case GlobalSkinConfiguration.ComboColours: - return SkinUtils.As(new Bindable>(ComboColours)); + return SkinUtils.As(new Bindable>(ComboColours)); } break; diff --git a/osu.Game.Tests/Skins/LegacySkinDecoderTest.cs b/osu.Game.Tests/Skins/LegacySkinDecoderTest.cs index f68d49dd3e..cef38bbbb8 100644 --- a/osu.Game.Tests/Skins/LegacySkinDecoderTest.cs +++ b/osu.Game.Tests/Skins/LegacySkinDecoderTest.cs @@ -13,31 +13,22 @@ namespace osu.Game.Tests.Skins [TestFixture] public class LegacySkinDecoderTest { - [TestCase(true)] - [TestCase(false)] - public void TestDecodeSkinColours(bool hasColours) + [Test] + public void TestDecodeSkinColours() { var decoder = new LegacySkinDecoder(); - using (var resStream = TestResources.OpenResource(hasColours ? "skin.ini" : "skin-empty.ini")) + using (var resStream = TestResources.OpenResource("skin.ini")) using (var stream = new LineBufferedReader(resStream)) { var comboColors = decoder.Decode(stream).ComboColours; - - List expectedColors; - - if (hasColours) + var expectedColors = new List { - expectedColors = new List - { - 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; + new Color4(142, 199, 255, 255), + new Color4(255, 128, 128, 255), + new Color4(128, 255, 255, 255), + new Color4(100, 100, 100, 100), + }; Assert.AreEqual(expectedColors.Count, comboColors.Count); for (int i = 0; i < expectedColors.Count; i++) @@ -45,6 +36,37 @@ namespace osu.Game.Tests.Skins } } + [Test] + public void TestDecodeEmptySkinColours() + { + var decoder = new LegacySkinDecoder(); + + using (var resStream = TestResources.OpenResource("skin-empty.ini")) + using (var stream = new LineBufferedReader(resStream)) + { + var comboColors = decoder.Decode(stream).ComboColours; + var expectedColors = SkinConfiguration.DefaultComboColours; + + Assert.AreEqual(expectedColors.Count, comboColors.Count); + for (int i = 0; i < expectedColors.Count; i++) + Assert.AreEqual(expectedColors[i], comboColors[i]); + } + } + + [Test] + public void TestDecodeEmptySkinColoursNoFallback() + { + var decoder = new LegacySkinDecoder(); + + using (var resStream = TestResources.OpenResource("skin-empty.ini")) + using (var stream = new LineBufferedReader(resStream)) + { + var skinConfiguration = decoder.Decode(stream); + skinConfiguration.AllowDefaultComboColoursFallback = false; + Assert.IsNull(skinConfiguration.ComboColours); + } + } + [Test] public void TestDecodeGeneral() { diff --git a/osu.Game.Tests/Skins/TestSceneSkinConfigurationLookup.cs b/osu.Game.Tests/Skins/TestSceneSkinConfigurationLookup.cs index 8b9c648442..ed54cc982d 100644 --- a/osu.Game.Tests/Skins/TestSceneSkinConfigurationLookup.cs +++ b/osu.Game.Tests/Skins/TestSceneSkinConfigurationLookup.cs @@ -2,6 +2,7 @@ // See the LICENCE file in the repository root for full licence text. using System.Collections.Generic; +using System.Linq; using NUnit.Framework; using osu.Framework.Allocation; using osu.Framework.Audio.Sample; @@ -21,8 +22,8 @@ namespace osu.Game.Tests.Skins [HeadlessTest] public class TestSceneSkinConfigurationLookup : OsuTestScene { - private LegacySkin source1; - private LegacySkin source2; + private SkinSource source1; + private SkinSource source2; private SkinRequester requester; [SetUp] @@ -94,7 +95,7 @@ namespace osu.Game.Tests.Skins [Test] public void TestGlobalLookup() { - AddAssert("Check combo colours", () => requester.GetConfig>(GlobalSkinConfiguration.ComboColours)?.Value?.Count > 0); + AddAssert("Check combo colours", () => requester.GetConfig>(GlobalSkinConfiguration.ComboColours)?.Value?.Count > 0); } [Test] @@ -116,6 +117,28 @@ namespace osu.Game.Tests.Skins }); } + [Test] + public void TestEmptyComboColours() + { + AddAssert("Check retrieved combo colours is skin default colours", () => + requester.GetConfig>(GlobalSkinConfiguration.ComboColours)?.Value?.SequenceEqual(SkinConfiguration.DefaultComboColours) ?? false); + } + + [Test] + public void TestEmptyComboColoursNoFallback() + { + AddStep("Add custom combo colours to source1", () => source1.Configuration.AddComboColours( + new Color4(100, 150, 200, 255), + new Color4(55, 110, 166, 255), + new Color4(75, 125, 175, 255) + )); + + AddStep("Disallow default colours fallback in source2", () => source2.Configuration.AllowDefaultComboColoursFallback = false); + + AddAssert("Check retrieved combo colours from source1", () => + requester.GetConfig>(GlobalSkinConfiguration.ComboColours)?.Value?.SequenceEqual(source1.Configuration.ComboColours) ?? false); + } + [Test] public void TestLegacyVersionLookup() { diff --git a/osu.Game/Beatmaps/Formats/IHasComboColours.cs b/osu.Game/Beatmaps/Formats/IHasComboColours.cs index 4c15cb96d1..41c85db063 100644 --- a/osu.Game/Beatmaps/Formats/IHasComboColours.cs +++ b/osu.Game/Beatmaps/Formats/IHasComboColours.cs @@ -8,6 +8,14 @@ namespace osu.Game.Beatmaps.Formats { public interface IHasComboColours { - List ComboColours { get; set; } + /// + /// Retrieves the list of combo colours for presentation only. + /// + IReadOnlyList ComboColours { get; } + + /// + /// Adds combo colours to the list. + /// + void AddComboColours(params Color4[] colours); } } diff --git a/osu.Game/Beatmaps/Formats/LegacyDecoder.cs b/osu.Game/Beatmaps/Formats/LegacyDecoder.cs index b1585d04c5..f55e24245b 100644 --- a/osu.Game/Beatmaps/Formats/LegacyDecoder.cs +++ b/osu.Game/Beatmaps/Formats/LegacyDecoder.cs @@ -77,8 +77,6 @@ namespace osu.Game.Beatmaps.Formats return line; } - private bool hasComboColours; - private void handleColours(T output, string line) { var pair = SplitKeyVal(line); @@ -105,14 +103,7 @@ namespace osu.Game.Beatmaps.Formats { if (!(output is IHasComboColours tHasComboColours)) return; - if (!hasComboColours) - { - // remove default colours. - tHasComboColours.ComboColours.Clear(); - hasComboColours = true; - } - - tHasComboColours.ComboColours.Add(colour); + tHasComboColours.AddComboColours(colour); } else { diff --git a/osu.Game/Rulesets/Objects/Drawables/DrawableHitObject.cs b/osu.Game/Rulesets/Objects/Drawables/DrawableHitObject.cs index 386805d7e5..a959fee9be 100644 --- a/osu.Game/Rulesets/Objects/Drawables/DrawableHitObject.cs +++ b/osu.Game/Rulesets/Objects/Drawables/DrawableHitObject.cs @@ -356,7 +356,7 @@ namespace osu.Game.Rulesets.Objects.Drawables { if (HitObject is IHasComboInformation combo) { - var comboColours = CurrentSkin.GetConfig>(GlobalSkinConfiguration.ComboColours)?.Value; + var comboColours = CurrentSkin.GetConfig>(GlobalSkinConfiguration.ComboColours)?.Value; AccentColour.Value = comboColours?.Count > 0 ? comboColours[combo.ComboIndex % comboColours.Count] : Color4.White; } } diff --git a/osu.Game/Skinning/DefaultLegacySkin.cs b/osu.Game/Skinning/DefaultLegacySkin.cs index 0caf2d19e9..1929a7e5d2 100644 --- a/osu.Game/Skinning/DefaultLegacySkin.cs +++ b/osu.Game/Skinning/DefaultLegacySkin.cs @@ -13,13 +13,12 @@ namespace osu.Game.Skinning : base(Info, storage, audioManager, string.Empty) { Configuration.CustomColours["SliderBall"] = new Color4(2, 170, 255, 255); - Configuration.ComboColours.AddRange(new[] - { + Configuration.AddComboColours( new Color4(255, 192, 0, 255), new Color4(0, 202, 0, 255), new Color4(18, 124, 255, 255), - new Color4(242, 24, 57, 255), - }); + new Color4(242, 24, 57, 255) + ); Configuration.LegacyVersion = 2.0m; } diff --git a/osu.Game/Skinning/DefaultSkin.cs b/osu.Game/Skinning/DefaultSkin.cs index 529c1afca5..2a065ea3d7 100644 --- a/osu.Game/Skinning/DefaultSkin.cs +++ b/osu.Game/Skinning/DefaultSkin.cs @@ -35,7 +35,7 @@ namespace osu.Game.Skinning switch (global) { case GlobalSkinConfiguration.ComboColours: - return SkinUtils.As(new Bindable>(Configuration.ComboColours)); + return SkinUtils.As(new Bindable>(Configuration.ComboColours)); } break; diff --git a/osu.Game/Skinning/DefaultSkinConfiguration.cs b/osu.Game/Skinning/DefaultSkinConfiguration.cs index cd5975edac..5842ee82ee 100644 --- a/osu.Game/Skinning/DefaultSkinConfiguration.cs +++ b/osu.Game/Skinning/DefaultSkinConfiguration.cs @@ -1,8 +1,6 @@ // Copyright (c) ppy Pty Ltd . Licensed under the MIT Licence. // See the LICENCE file in the repository root for full licence text. -using osuTK.Graphics; - namespace osu.Game.Skinning { /// @@ -10,15 +8,5 @@ namespace osu.Game.Skinning /// public class DefaultSkinConfiguration : SkinConfiguration { - public DefaultSkinConfiguration() - { - ComboColours.AddRange(new[] - { - new Color4(255, 192, 0, 255), - new Color4(0, 202, 0, 255), - new Color4(18, 124, 255, 255), - new Color4(242, 24, 57, 255), - }); - } } } diff --git a/osu.Game/Skinning/LegacyBeatmapSkin.cs b/osu.Game/Skinning/LegacyBeatmapSkin.cs index 6770da3c66..fa7e895a28 100644 --- a/osu.Game/Skinning/LegacyBeatmapSkin.cs +++ b/osu.Game/Skinning/LegacyBeatmapSkin.cs @@ -12,6 +12,8 @@ namespace osu.Game.Skinning public LegacyBeatmapSkin(BeatmapInfo beatmap, IResourceStore storage, AudioManager audioManager) : base(createSkinInfo(beatmap), new LegacySkinResourceStore(beatmap.BeatmapSet, storage), audioManager, beatmap.Path) { + // Disallow default colours fallback on beatmap skins to allow using parent skin combo colours. (via SkinProvidingContainer) + Configuration.AllowDefaultComboColoursFallback = false; } private static SkinInfo createSkinInfo(BeatmapInfo beatmap) => diff --git a/osu.Game/Skinning/LegacySkin.cs b/osu.Game/Skinning/LegacySkin.cs index 868e3921bb..48c520986a 100644 --- a/osu.Game/Skinning/LegacySkin.cs +++ b/osu.Game/Skinning/LegacySkin.cs @@ -72,7 +72,11 @@ namespace osu.Game.Skinning switch (global) { case GlobalSkinConfiguration.ComboColours: - return SkinUtils.As(new Bindable>(Configuration.ComboColours)); + var comboColours = Configuration.ComboColours; + if (comboColours != null) + return SkinUtils.As(new Bindable>(comboColours)); + + break; } break; diff --git a/osu.Game/Skinning/LegacySkinConfiguration.cs b/osu.Game/Skinning/LegacySkinConfiguration.cs index b1679bd464..027f5b8883 100644 --- a/osu.Game/Skinning/LegacySkinConfiguration.cs +++ b/osu.Game/Skinning/LegacySkinConfiguration.cs @@ -3,7 +3,7 @@ namespace osu.Game.Skinning { - public class LegacySkinConfiguration : DefaultSkinConfiguration + public class LegacySkinConfiguration : SkinConfiguration { public const decimal LATEST_VERSION = 2.7m; diff --git a/osu.Game/Skinning/SkinConfiguration.cs b/osu.Game/Skinning/SkinConfiguration.cs index 54aac86e3c..a55870aa6d 100644 --- a/osu.Game/Skinning/SkinConfiguration.cs +++ b/osu.Game/Skinning/SkinConfiguration.cs @@ -14,7 +14,36 @@ namespace osu.Game.Skinning { public readonly SkinInfo SkinInfo = new SkinInfo(); - public List ComboColours { get; set; } = new List(); + /// + /// Whether to allow as a fallback list for when no combo colours are provided. + /// + internal bool AllowDefaultComboColoursFallback = true; + + public static List DefaultComboColours { get; } = new List + { + new Color4(255, 192, 0, 255), + new Color4(0, 202, 0, 255), + new Color4(18, 124, 255, 255), + new Color4(242, 24, 57, 255), + }; + + private readonly List comboColours = new List(); + + public IReadOnlyList ComboColours + { + get + { + if (comboColours.Count > 0) + return comboColours; + + if (AllowDefaultComboColoursFallback) + return DefaultComboColours; + + return null; + } + } + + public void AddComboColours(params Color4[] colours) => comboColours.AddRange(colours); public Dictionary CustomColours { get; set; } = new Dictionary();