From cf40282f1f30fb2d7d7a9c588157a0429b967e43 Mon Sep 17 00:00:00 2001 From: Salman Ahmed Date: Wed, 9 Jun 2021 13:34:42 +0300 Subject: [PATCH 01/54] Convert `LegacySkinTransformer`s to accept raw `ISkin`s rather than a full `ISkinSource` --- osu.Game.Rulesets.Catch/CatchRuleset.cs | 2 +- .../Legacy/CatchLegacySkinTransformer.cs | 14 +++--- osu.Game.Rulesets.Mania/ManiaRuleset.cs | 2 +- .../Legacy/ManiaLegacySkinTransformer.cs | 32 ++++++------- osu.Game.Rulesets.Osu/OsuRuleset.cs | 2 +- .../Legacy/OsuLegacySkinTransformer.cs | 47 +++++++------------ .../Legacy/TaikoLegacySkinTransformer.cs | 20 +++----- osu.Game.Rulesets.Taiko/TaikoRuleset.cs | 2 +- .../Gameplay/TestSceneBeatmapSkinFallbacks.cs | 6 +-- osu.Game/Rulesets/Ruleset.cs | 2 +- osu.Game/Skinning/LegacySkinTransformer.cs | 25 ++++------ 11 files changed, 61 insertions(+), 93 deletions(-) diff --git a/osu.Game.Rulesets.Catch/CatchRuleset.cs b/osu.Game.Rulesets.Catch/CatchRuleset.cs index 23ce444560..ec4c5dfe4c 100644 --- a/osu.Game.Rulesets.Catch/CatchRuleset.cs +++ b/osu.Game.Rulesets.Catch/CatchRuleset.cs @@ -175,7 +175,7 @@ namespace osu.Game.Rulesets.Catch public override DifficultyCalculator CreateDifficultyCalculator(WorkingBeatmap beatmap) => new CatchDifficultyCalculator(this, beatmap); - public override ISkin CreateLegacySkinProvider(ISkinSource source, IBeatmap beatmap) => new CatchLegacySkinTransformer(source); + public override ISkin CreateLegacySkinProvider(ISkin skin, IBeatmap beatmap) => new CatchLegacySkinTransformer(skin); public override PerformanceCalculator CreatePerformanceCalculator(DifficultyAttributes attributes, ScoreInfo score) => new CatchPerformanceCalculator(this, attributes, score); diff --git a/osu.Game.Rulesets.Catch/Skinning/Legacy/CatchLegacySkinTransformer.cs b/osu.Game.Rulesets.Catch/Skinning/Legacy/CatchLegacySkinTransformer.cs index 8c9e602cd4..9be47b3550 100644 --- a/osu.Game.Rulesets.Catch/Skinning/Legacy/CatchLegacySkinTransformer.cs +++ b/osu.Game.Rulesets.Catch/Skinning/Legacy/CatchLegacySkinTransformer.cs @@ -17,8 +17,8 @@ namespace osu.Game.Rulesets.Catch.Skinning.Legacy /// private bool providesComboCounter => this.HasFont(LegacyFont.Combo); - public CatchLegacySkinTransformer(ISkinSource source) - : base(source) + public CatchLegacySkinTransformer(ISkin skin) + : base(skin) { } @@ -29,7 +29,7 @@ namespace osu.Game.Rulesets.Catch.Skinning.Legacy switch (targetComponent.Target) { case SkinnableTarget.MainHUDComponents: - var components = Source.GetDrawableComponent(component) as SkinnableTargetComponentsContainer; + var components = Skin.GetDrawableComponent(component) as SkinnableTargetComponentsContainer; if (providesComboCounter && components != null) { @@ -79,13 +79,13 @@ namespace osu.Game.Rulesets.Catch.Skinning.Legacy case CatchSkinComponents.CatchComboCounter: if (providesComboCounter) - return new LegacyCatchComboCounter(Source); + return new LegacyCatchComboCounter(Skin); return null; } } - return Source.GetDrawableComponent(component); + return Skin.GetDrawableComponent(component); } public override IBindable GetConfig(TLookup lookup) @@ -93,7 +93,7 @@ namespace osu.Game.Rulesets.Catch.Skinning.Legacy switch (lookup) { case CatchSkinColour colour: - var result = (Bindable)Source.GetConfig(new SkinCustomColourLookup(colour)); + var result = (Bindable)Skin.GetConfig(new SkinCustomColourLookup(colour)); if (result == null) return null; @@ -101,7 +101,7 @@ namespace osu.Game.Rulesets.Catch.Skinning.Legacy return (IBindable)result; } - return Source.GetConfig(lookup); + return Skin.GetConfig(lookup); } } } diff --git a/osu.Game.Rulesets.Mania/ManiaRuleset.cs b/osu.Game.Rulesets.Mania/ManiaRuleset.cs index fbb9b3c466..fe736766d9 100644 --- a/osu.Game.Rulesets.Mania/ManiaRuleset.cs +++ b/osu.Game.Rulesets.Mania/ManiaRuleset.cs @@ -57,7 +57,7 @@ namespace osu.Game.Rulesets.Mania public override HitObjectComposer CreateHitObjectComposer() => new ManiaHitObjectComposer(this); - public override ISkin CreateLegacySkinProvider(ISkinSource source, IBeatmap beatmap) => new ManiaLegacySkinTransformer(source, beatmap); + public override ISkin CreateLegacySkinProvider(ISkin skin, IBeatmap beatmap) => new ManiaLegacySkinTransformer(skin, beatmap); public override IEnumerable ConvertFromLegacyMods(LegacyMods mods) { diff --git a/osu.Game.Rulesets.Mania/Skinning/Legacy/ManiaLegacySkinTransformer.cs b/osu.Game.Rulesets.Mania/Skinning/Legacy/ManiaLegacySkinTransformer.cs index 962a13ebea..7d4d303bc9 100644 --- a/osu.Game.Rulesets.Mania/Skinning/Legacy/ManiaLegacySkinTransformer.cs +++ b/osu.Game.Rulesets.Mania/Skinning/Legacy/ManiaLegacySkinTransformer.cs @@ -50,29 +50,25 @@ namespace osu.Game.Rulesets.Mania.Skinning.Legacy { HitResult.Miss, "mania-hit0" } }; - private Lazy isLegacySkin; + private readonly Lazy isLegacySkin; /// /// Whether texture for the keys exists. /// Used to determine if the mania ruleset is skinned. /// - private Lazy hasKeyTexture; + private readonly Lazy hasKeyTexture; - public ManiaLegacySkinTransformer(ISkinSource source, IBeatmap beatmap) - : base(source) + public ManiaLegacySkinTransformer(ISkin skin, IBeatmap beatmap) + : base(skin) { this.beatmap = (ManiaBeatmap)beatmap; - Source.SourceChanged += sourceChanged; - sourceChanged(); - } - - private void sourceChanged() - { - isLegacySkin = new Lazy(() => FindProvider(s => s.GetConfig(LegacySkinConfiguration.LegacySetting.Version) != null) != null); - hasKeyTexture = new Lazy(() => FindProvider(s => s.GetAnimation( - s.GetManiaSkinConfig(LegacyManiaSkinConfigurationLookups.KeyImage, 0)?.Value - ?? "mania-key1", true, true) != null) != null); + isLegacySkin = new Lazy(() => skin.GetConfig(LegacySkinConfiguration.LegacySetting.Version) != null); + hasKeyTexture = new Lazy(() => + { + var keyImage = this.GetManiaSkinConfig(LegacyManiaSkinConfigurationLookups.KeyImage, 0)?.Value ?? "mania-key1"; + return skin.GetAnimation(keyImage, true, true) != null; + }); } public override Drawable GetDrawableComponent(ISkinComponent component) @@ -125,7 +121,7 @@ namespace osu.Game.Rulesets.Mania.Skinning.Legacy break; } - return Source.GetDrawableComponent(component); + return Skin.GetDrawableComponent(component); } private Drawable getResult(HitResult result) @@ -146,15 +142,15 @@ namespace osu.Game.Rulesets.Mania.Skinning.Legacy if (sampleInfo is ConvertHitObjectParser.LegacyHitSampleInfo legacySample && legacySample.IsLayered) return new SampleVirtual(); - return Source.GetSample(sampleInfo); + return Skin.GetSample(sampleInfo); } public override IBindable GetConfig(TLookup lookup) { if (lookup is ManiaSkinConfigurationLookup maniaLookup) - return Source.GetConfig(new LegacyManiaSkinConfigurationLookup(beatmap.TotalColumns, maniaLookup.Lookup, maniaLookup.TargetColumn)); + return Skin.GetConfig(new LegacyManiaSkinConfigurationLookup(beatmap.TotalColumns, maniaLookup.Lookup, maniaLookup.TargetColumn)); - return Source.GetConfig(lookup); + return Skin.GetConfig(lookup); } } } diff --git a/osu.Game.Rulesets.Osu/OsuRuleset.cs b/osu.Game.Rulesets.Osu/OsuRuleset.cs index b50d3ad2b4..69e22dc45d 100644 --- a/osu.Game.Rulesets.Osu/OsuRuleset.cs +++ b/osu.Game.Rulesets.Osu/OsuRuleset.cs @@ -218,7 +218,7 @@ namespace osu.Game.Rulesets.Osu public override RulesetSettingsSubsection CreateSettings() => new OsuSettingsSubsection(this); - public override ISkin CreateLegacySkinProvider(ISkinSource source, IBeatmap beatmap) => new OsuLegacySkinTransformer(source); + public override ISkin CreateLegacySkinProvider(ISkin skin, IBeatmap beatmap) => new OsuLegacySkinTransformer(skin); public int LegacyID => 0; diff --git a/osu.Game.Rulesets.Osu/Skinning/Legacy/OsuLegacySkinTransformer.cs b/osu.Game.Rulesets.Osu/Skinning/Legacy/OsuLegacySkinTransformer.cs index 3267b48ebf..3ad3b7d30b 100644 --- a/osu.Game.Rulesets.Osu/Skinning/Legacy/OsuLegacySkinTransformer.cs +++ b/osu.Game.Rulesets.Osu/Skinning/Legacy/OsuLegacySkinTransformer.cs @@ -11,7 +11,7 @@ namespace osu.Game.Rulesets.Osu.Skinning.Legacy { public class OsuLegacySkinTransformer : LegacySkinTransformer { - private Lazy hasHitCircle; + private readonly Lazy hasHitCircle; /// /// On osu-stable, hitcircles have 5 pixels of transparent padding on each side to allow for shadows etc. @@ -20,16 +20,10 @@ namespace osu.Game.Rulesets.Osu.Skinning.Legacy /// public const float LEGACY_CIRCLE_RADIUS = 64 - 5; - public OsuLegacySkinTransformer(ISkinSource source) - : base(source) + public OsuLegacySkinTransformer(ISkin skin) + : base(skin) { - Source.SourceChanged += sourceChanged; - sourceChanged(); - } - - private void sourceChanged() - { - hasHitCircle = new Lazy(() => FindProvider(s => s.GetTexture("hitcircle") != null) != null); + hasHitCircle = new Lazy(() => Skin.GetTexture("hitcircle") != null); } public override Drawable GetDrawableComponent(ISkinComponent component) @@ -49,16 +43,13 @@ namespace osu.Game.Rulesets.Osu.Skinning.Legacy return followCircle; case OsuSkinComponents.SliderBall: - // specular and nd layers must come from the same source as the ball texure. - var ballProvider = Source.FindProvider(s => s.GetTexture("sliderb") != null || s.GetTexture("sliderb0") != null); - - var sliderBallContent = ballProvider.GetAnimation("sliderb", true, true, animationSeparator: ""); + var sliderBallContent = this.GetAnimation("sliderb", true, true, animationSeparator: ""); // todo: slider ball has a custom frame delay based on velocity // Math.Max((150 / Velocity) * GameBase.SIXTY_FRAME_TIME, GameBase.SIXTY_FRAME_TIME); if (sliderBallContent != null) - return new LegacySliderBall(sliderBallContent, ballProvider); + return new LegacySliderBall(sliderBallContent, this); return null; @@ -87,18 +78,14 @@ namespace osu.Game.Rulesets.Osu.Skinning.Legacy return null; case OsuSkinComponents.Cursor: - var cursorProvider = Source.FindProvider(s => s.GetTexture("cursor") != null); - - if (cursorProvider != null) - return new LegacyCursor(cursorProvider); + if (GetTexture("cursor") != null) + return new LegacyCursor(this); return null; case OsuSkinComponents.CursorTrail: - var trailProvider = Source.FindProvider(s => s.GetTexture("cursortrail") != null); - - if (trailProvider != null) - return new LegacyCursorTrail(trailProvider); + if (GetTexture("cursortrail") != null) + return new LegacyCursorTrail(this); return null; @@ -113,9 +100,9 @@ namespace osu.Game.Rulesets.Osu.Skinning.Legacy }; case OsuSkinComponents.SpinnerBody: - bool hasBackground = Source.GetTexture("spinner-background") != null; + bool hasBackground = GetTexture("spinner-background") != null; - if (Source.GetTexture("spinner-top") != null && !hasBackground) + if (GetTexture("spinner-top") != null && !hasBackground) return new LegacyNewStyleSpinner(); else if (hasBackground) return new LegacyOldStyleSpinner(); @@ -124,7 +111,7 @@ namespace osu.Game.Rulesets.Osu.Skinning.Legacy } } - return Source.GetDrawableComponent(component); + return Skin.GetDrawableComponent(component); } public override IBindable GetConfig(TLookup lookup) @@ -132,7 +119,7 @@ namespace osu.Game.Rulesets.Osu.Skinning.Legacy switch (lookup) { case OsuSkinColour colour: - return Source.GetConfig(new SkinCustomColourLookup(colour)); + return Skin.GetConfig(new SkinCustomColourLookup(colour)); case OsuSkinConfiguration osuLookup: switch (osuLookup) @@ -146,14 +133,14 @@ namespace osu.Game.Rulesets.Osu.Skinning.Legacy case OsuSkinConfiguration.HitCircleOverlayAboveNumber: // See https://osu.ppy.sh/help/wiki/Skinning/skin.ini#%5Bgeneral%5D // HitCircleOverlayAboveNumer (with typo) should still be supported for now. - return Source.GetConfig(OsuSkinConfiguration.HitCircleOverlayAboveNumber) ?? - Source.GetConfig(OsuSkinConfiguration.HitCircleOverlayAboveNumer); + return Skin.GetConfig(OsuSkinConfiguration.HitCircleOverlayAboveNumber) ?? + Skin.GetConfig(OsuSkinConfiguration.HitCircleOverlayAboveNumer); } break; } - return Source.GetConfig(lookup); + return Skin.GetConfig(lookup); } } } diff --git a/osu.Game.Rulesets.Taiko/Skinning/Legacy/TaikoLegacySkinTransformer.cs b/osu.Game.Rulesets.Taiko/Skinning/Legacy/TaikoLegacySkinTransformer.cs index 7ce0f6b93b..0122f9a1cd 100644 --- a/osu.Game.Rulesets.Taiko/Skinning/Legacy/TaikoLegacySkinTransformer.cs +++ b/osu.Game.Rulesets.Taiko/Skinning/Legacy/TaikoLegacySkinTransformer.cs @@ -15,18 +15,12 @@ namespace osu.Game.Rulesets.Taiko.Skinning.Legacy { public class TaikoLegacySkinTransformer : LegacySkinTransformer { - private Lazy hasExplosion; + private readonly Lazy hasExplosion; - public TaikoLegacySkinTransformer(ISkinSource source) - : base(source) + public TaikoLegacySkinTransformer(ISkin skin) + : base(skin) { - Source.SourceChanged += sourceChanged; - sourceChanged(); - } - - private void sourceChanged() - { - hasExplosion = new Lazy(() => Source.GetTexture(getHitName(TaikoSkinComponents.TaikoExplosionGreat)) != null); + hasExplosion = new Lazy(() => Skin.GetTexture(getHitName(TaikoSkinComponents.TaikoExplosionGreat)) != null); } public override Drawable GetDrawableComponent(ISkinComponent component) @@ -132,7 +126,7 @@ namespace osu.Game.Rulesets.Taiko.Skinning.Legacy } } - return Source.GetDrawableComponent(component); + return Skin.GetDrawableComponent(component); } private string getHitName(TaikoSkinComponents component) @@ -155,12 +149,12 @@ namespace osu.Game.Rulesets.Taiko.Skinning.Legacy public override ISample GetSample(ISampleInfo sampleInfo) { if (sampleInfo is HitSampleInfo hitSampleInfo) - return Source.GetSample(new LegacyTaikoSampleInfo(hitSampleInfo)); + return Skin.GetSample(new LegacyTaikoSampleInfo(hitSampleInfo)); return base.GetSample(sampleInfo); } - public override IBindable GetConfig(TLookup lookup) => Source.GetConfig(lookup); + public override IBindable GetConfig(TLookup lookup) => Skin.GetConfig(lookup); private class LegacyTaikoSampleInfo : HitSampleInfo { diff --git a/osu.Game.Rulesets.Taiko/TaikoRuleset.cs b/osu.Game.Rulesets.Taiko/TaikoRuleset.cs index 5854d4770c..ab5fcf6336 100644 --- a/osu.Game.Rulesets.Taiko/TaikoRuleset.cs +++ b/osu.Game.Rulesets.Taiko/TaikoRuleset.cs @@ -42,7 +42,7 @@ namespace osu.Game.Rulesets.Taiko public override IBeatmapConverter CreateBeatmapConverter(IBeatmap beatmap) => new TaikoBeatmapConverter(beatmap, this); - public override ISkin CreateLegacySkinProvider(ISkinSource source, IBeatmap beatmap) => new TaikoLegacySkinTransformer(source); + public override ISkin CreateLegacySkinProvider(ISkin skin, IBeatmap beatmap) => new TaikoLegacySkinTransformer(skin); public const string SHORT_NAME = "taiko"; diff --git a/osu.Game.Tests/Visual/Gameplay/TestSceneBeatmapSkinFallbacks.cs b/osu.Game.Tests/Visual/Gameplay/TestSceneBeatmapSkinFallbacks.cs index cc53e50884..13e84e335d 100644 --- a/osu.Game.Tests/Visual/Gameplay/TestSceneBeatmapSkinFallbacks.cs +++ b/osu.Game.Tests/Visual/Gameplay/TestSceneBeatmapSkinFallbacks.cs @@ -116,12 +116,12 @@ namespace osu.Game.Tests.Visual.Gameplay private class TestOsuRuleset : OsuRuleset { - public override ISkin CreateLegacySkinProvider(ISkinSource source, IBeatmap beatmap) => new TestOsuLegacySkinTransformer(source); + public override ISkin CreateLegacySkinProvider(ISkin skin, IBeatmap beatmap) => new TestOsuLegacySkinTransformer(skin); private class TestOsuLegacySkinTransformer : OsuLegacySkinTransformer { - public TestOsuLegacySkinTransformer(ISkinSource source) - : base(source) + public TestOsuLegacySkinTransformer(ISkin skin) + : base(skin) { } } diff --git a/osu.Game/Rulesets/Ruleset.cs b/osu.Game/Rulesets/Ruleset.cs index 7bdf84ace4..9f9f42eda4 100644 --- a/osu.Game/Rulesets/Ruleset.cs +++ b/osu.Game/Rulesets/Ruleset.cs @@ -127,7 +127,7 @@ namespace osu.Game.Rulesets [CanBeNull] public ModAutoplay GetAutoplayMod() => GetAllMods().OfType().FirstOrDefault(); - public virtual ISkin CreateLegacySkinProvider(ISkinSource source, IBeatmap beatmap) => null; + public virtual ISkin CreateLegacySkinProvider(ISkin skin, IBeatmap beatmap) => null; protected Ruleset() { diff --git a/osu.Game/Skinning/LegacySkinTransformer.cs b/osu.Game/Skinning/LegacySkinTransformer.cs index 651fdddb1b..cd896ab51e 100644 --- a/osu.Game/Skinning/LegacySkinTransformer.cs +++ b/osu.Game/Skinning/LegacySkinTransformer.cs @@ -1,7 +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 System; using osu.Framework.Audio.Sample; using osu.Framework.Bindables; using osu.Framework.Graphics; @@ -16,16 +15,16 @@ namespace osu.Game.Skinning /// /// Transformer used to handle support of legacy features for individual rulesets. /// - public abstract class LegacySkinTransformer : ISkinSource + public abstract class LegacySkinTransformer : ISkin { /// - /// Source of the which is being transformed. + /// The which is being transformed. /// - protected ISkinSource Source { get; } + protected ISkin Skin { get; } - protected LegacySkinTransformer(ISkinSource source) + protected LegacySkinTransformer(ISkin skin) { - Source = source; + Skin = skin; } public abstract Drawable GetDrawableComponent(ISkinComponent component); @@ -33,28 +32,20 @@ namespace osu.Game.Skinning public Texture GetTexture(string componentName) => GetTexture(componentName, default, default); public Texture GetTexture(string componentName, WrapMode wrapModeS, WrapMode wrapModeT) - => Source.GetTexture(componentName, wrapModeS, wrapModeT); + => Skin.GetTexture(componentName, wrapModeS, wrapModeT); public virtual ISample GetSample(ISampleInfo sampleInfo) { if (!(sampleInfo is ConvertHitObjectParser.LegacyHitSampleInfo legacySample)) - return Source.GetSample(sampleInfo); + return Skin.GetSample(sampleInfo); var playLayeredHitSounds = GetConfig(LegacySetting.LayeredHitSounds); if (legacySample.IsLayered && playLayeredHitSounds?.Value == false) return new SampleVirtual(); - return Source.GetSample(sampleInfo); + return Skin.GetSample(sampleInfo); } public abstract IBindable GetConfig(TLookup lookup); - - public ISkin FindProvider(Func lookupFunction) => Source.FindProvider(lookupFunction); - - public event Action SourceChanged - { - add { throw new NotSupportedException(); } - remove { } - } } } From 6538d44708646a2b32b4aa0b112d7d31d82d38ff Mon Sep 17 00:00:00 2001 From: Salman Ahmed Date: Wed, 9 Jun 2021 20:36:34 +0300 Subject: [PATCH 02/54] Make `SkinProvidingContainer` able to perform lookup on multiple skins Currently `protected` functionality for use in custom `SkinProvidingContainer`s, can be exposed to public constructors if it need to later on, but I'm not sure about doing that opposed to just nesting multiple `SkinProvidingContainer`. --- .../Gameplay/TestSceneSkinnableDrawable.cs | 2 +- .../Skinning/BeatmapSkinProvidingContainer.cs | 6 +- osu.Game/Skinning/SkinProvidingContainer.cs | 94 ++++++++++++------- 3 files changed, 64 insertions(+), 38 deletions(-) diff --git a/osu.Game.Tests/Visual/Gameplay/TestSceneSkinnableDrawable.cs b/osu.Game.Tests/Visual/Gameplay/TestSceneSkinnableDrawable.cs index 96418f6d28..77966e925a 100644 --- a/osu.Game.Tests/Visual/Gameplay/TestSceneSkinnableDrawable.cs +++ b/osu.Game.Tests/Visual/Gameplay/TestSceneSkinnableDrawable.cs @@ -167,7 +167,7 @@ namespace osu.Game.Tests.Visual.Gameplay public void Disable() { allow = false; - TriggerSourceChanged(); + OnSourceChanged(); } public SwitchableSkinProvidingContainer(ISkin skin) diff --git a/osu.Game/Skinning/BeatmapSkinProvidingContainer.cs b/osu.Game/Skinning/BeatmapSkinProvidingContainer.cs index 57c08a903f..f12f44e347 100644 --- a/osu.Game/Skinning/BeatmapSkinProvidingContainer.cs +++ b/osu.Game/Skinning/BeatmapSkinProvidingContainer.cs @@ -83,9 +83,9 @@ namespace osu.Game.Skinning [BackgroundDependencyLoader] private void load() { - beatmapSkins.BindValueChanged(_ => TriggerSourceChanged()); - beatmapColours.BindValueChanged(_ => TriggerSourceChanged()); - beatmapHitsounds.BindValueChanged(_ => TriggerSourceChanged()); + beatmapSkins.BindValueChanged(_ => OnSourceChanged()); + beatmapColours.BindValueChanged(_ => OnSourceChanged()); + beatmapHitsounds.BindValueChanged(_ => OnSourceChanged()); } } } diff --git a/osu.Game/Skinning/SkinProvidingContainer.cs b/osu.Game/Skinning/SkinProvidingContainer.cs index 0e16cf43ee..cc9d8d0e8d 100644 --- a/osu.Game/Skinning/SkinProvidingContainer.cs +++ b/osu.Game/Skinning/SkinProvidingContainer.cs @@ -2,6 +2,7 @@ // See the LICENCE file in the repository root for full licence text. using System; +using System.Collections.Generic; using JetBrains.Annotations; using osu.Framework.Allocation; using osu.Framework.Audio.Sample; @@ -21,8 +22,10 @@ namespace osu.Game.Skinning { public event Action SourceChanged; - [CanBeNull] - private readonly ISkin skin; + /// + /// The list of skins provided by this . + /// + protected readonly List SkinLayers = new List(); [CanBeNull] private ISkinSource fallbackSource; @@ -38,23 +41,30 @@ namespace osu.Game.Skinning protected virtual bool AllowColourLookup => true; public SkinProvidingContainer(ISkin skin) + : this() { - this.skin = skin; + SkinLayers.Add(skin); + } + protected SkinProvidingContainer() + { RelativeSizeAxes = Axes.Both; } public ISkin FindProvider(Func lookupFunction) { - if (skin is ISkinSource source) + foreach (var skin in SkinLayers) { - if (source.FindProvider(lookupFunction) is ISkin found) - return found; - } - else if (skin != null) - { - if (lookupFunction(skin)) - return skin; + if (skin is ISkinSource source) + { + if (source.FindProvider(lookupFunction) is ISkin found) + return found; + } + else if (skin != null) + { + if (lookupFunction(skin)) + return skin; + } } return fallbackSource?.FindProvider(lookupFunction); @@ -62,57 +72,73 @@ namespace osu.Game.Skinning public Drawable GetDrawableComponent(ISkinComponent component) { - Drawable sourceDrawable; - if (AllowDrawableLookup(component) && (sourceDrawable = skin?.GetDrawableComponent(component)) != null) - return sourceDrawable; + if (AllowDrawableLookup(component)) + { + foreach (var skin in SkinLayers) + { + Drawable sourceDrawable; + if ((sourceDrawable = skin?.GetDrawableComponent(component)) != null) + return sourceDrawable; + } + } return fallbackSource?.GetDrawableComponent(component); } public Texture GetTexture(string componentName, WrapMode wrapModeS, WrapMode wrapModeT) { - Texture sourceTexture; - if (AllowTextureLookup(componentName) && (sourceTexture = skin?.GetTexture(componentName, wrapModeS, wrapModeT)) != null) - return sourceTexture; + if (AllowTextureLookup(componentName)) + { + foreach (var skin in SkinLayers) + { + Texture sourceTexture; + if ((sourceTexture = skin?.GetTexture(componentName, wrapModeS, wrapModeT)) != null) + return sourceTexture; + } + } return fallbackSource?.GetTexture(componentName, wrapModeS, wrapModeT); } public ISample GetSample(ISampleInfo sampleInfo) { - ISample sourceChannel; - if (AllowSampleLookup(sampleInfo) && (sourceChannel = skin?.GetSample(sampleInfo)) != null) - return sourceChannel; + if (AllowSampleLookup(sampleInfo)) + { + foreach (var skin in SkinLayers) + { + ISample sourceSample; + if ((sourceSample = skin?.GetSample(sampleInfo)) != null) + return sourceSample; + } + } return fallbackSource?.GetSample(sampleInfo); } public IBindable GetConfig(TLookup lookup) { - if (skin != null) - { - if (lookup is GlobalSkinColours || lookup is SkinCustomColourLookup) - return lookupWithFallback(lookup, AllowColourLookup); + if (lookup is GlobalSkinColours || lookup is SkinCustomColourLookup) + return lookupWithFallback(lookup, AllowColourLookup); - return lookupWithFallback(lookup, AllowConfigurationLookup); - } - - return fallbackSource?.GetConfig(lookup); + return lookupWithFallback(lookup, AllowConfigurationLookup); } private IBindable lookupWithFallback(TLookup lookup, bool canUseSkinLookup) { if (canUseSkinLookup) { - var bindable = skin?.GetConfig(lookup); - if (bindable != null) - return bindable; + foreach (var skin in SkinLayers) + { + IBindable bindable; + if ((bindable = skin?.GetConfig(lookup)) != null) + return bindable; + } } return fallbackSource?.GetConfig(lookup); } - protected virtual void TriggerSourceChanged() => SourceChanged?.Invoke(); + protected virtual void OnSourceChanged() => SourceChanged?.Invoke(); protected override IReadOnlyDependencyContainer CreateChildDependencies(IReadOnlyDependencyContainer parent) { @@ -120,7 +146,7 @@ namespace osu.Game.Skinning fallbackSource = dependencies.Get(); if (fallbackSource != null) - fallbackSource.SourceChanged += TriggerSourceChanged; + fallbackSource.SourceChanged += OnSourceChanged; dependencies.CacheAs(this); @@ -135,7 +161,7 @@ namespace osu.Game.Skinning base.Dispose(isDisposing); if (fallbackSource != null) - fallbackSource.SourceChanged -= TriggerSourceChanged; + fallbackSource.SourceChanged -= OnSourceChanged; } } } From 9e652715ced79d2a6f22180c03d3e6024a10da8d Mon Sep 17 00:00:00 2001 From: Salman Ahmed Date: Wed, 9 Jun 2021 20:40:47 +0300 Subject: [PATCH 03/54] Expose the skin lookup layers of `SkinManager` to a property --- osu.Game/Skinning/SkinManager.cs | 44 +++++++++++++++++++++----------- 1 file changed, 29 insertions(+), 15 deletions(-) diff --git a/osu.Game/Skinning/SkinManager.cs b/osu.Game/Skinning/SkinManager.cs index 48d6b9254f..134156e44f 100644 --- a/osu.Game/Skinning/SkinManager.cs +++ b/osu.Game/Skinning/SkinManager.cs @@ -42,6 +42,24 @@ namespace osu.Game.Skinning public readonly Bindable CurrentSkin = new Bindable(); public readonly Bindable CurrentSkinInfo = new Bindable(SkinInfo.Default) { Default = SkinInfo.Default }; + /// + /// The skin layers of the currently selected user skin for performing lookups on, + /// in order of preference (user skin first, then fallback skins). + /// + public IEnumerable CurrentSkinLayers + { + get + { + yield return CurrentSkin.Value; + + // TODO: we also want to return a DefaultLegacySkin here if the current *beatmap* is providing any skinned elements. + // When attempting to address this, we may want to move the full DefaultLegacySkin fallback logic to within Player itself (to better allow + // for beatmap skin visibility). + if (CurrentSkin.Value is LegacySkin) + yield return defaultLegacySkin; + } + } + public override IEnumerable HandledExtensions => new[] { ".osk" }; protected override string[] HashableFileTypes => new[] { ".ini" }; @@ -220,11 +238,11 @@ namespace osu.Game.Skinning public ISkin FindProvider(Func lookupFunction) { - if (lookupFunction(CurrentSkin.Value)) - return CurrentSkin.Value; - - if (CurrentSkin.Value is LegacySkin && lookupFunction(defaultLegacySkin)) - return defaultLegacySkin; + foreach (var skin in CurrentSkinLayers) + { + if (lookupFunction(skin)) + return skin; + } return null; } @@ -232,16 +250,12 @@ namespace osu.Game.Skinning private T lookupWithFallback(Func func) where T : class { - var selectedSkin = func(CurrentSkin.Value); - - if (selectedSkin != null) - return selectedSkin; - - // TODO: we also want to return a DefaultLegacySkin here if the current *beatmap* is providing any skinned elements. - // When attempting to address this, we may want to move the full DefaultLegacySkin fallback logic to within Player itself (to better allow - // for beatmap skin visibility). - if (CurrentSkin.Value is LegacySkin) - return func(defaultLegacySkin); + foreach (var skin in CurrentSkinLayers) + { + var result = func(skin); + if (result != null) + return result; + } return null; } From 33a9cac398f446bb01b5e9e9bd9463f81edd62ae Mon Sep 17 00:00:00 2001 From: Salman Ahmed Date: Wed, 9 Jun 2021 20:41:16 +0300 Subject: [PATCH 04/54] Add special `RulesetSkinProvidingContainer` managing ruleset-compatible skin setup --- .../Skinning/RulesetSkinProvidingContainer.cs | 59 +++++++++++++++++++ 1 file changed, 59 insertions(+) create mode 100644 osu.Game/Skinning/RulesetSkinProvidingContainer.cs diff --git a/osu.Game/Skinning/RulesetSkinProvidingContainer.cs b/osu.Game/Skinning/RulesetSkinProvidingContainer.cs new file mode 100644 index 0000000000..dcf6281e38 --- /dev/null +++ b/osu.Game/Skinning/RulesetSkinProvidingContainer.cs @@ -0,0 +1,59 @@ +// Copyright (c) ppy Pty Ltd . Licensed under the MIT Licence. +// See the LICENCE file in the repository root for full licence text. + +using System.Linq; +using osu.Framework.Allocation; +using osu.Framework.Graphics; +using osu.Framework.Graphics.Containers; +using osu.Game.Beatmaps; +using osu.Game.Rulesets; + +namespace osu.Game.Skinning +{ + /// + /// A type of that provides access to the beatmap skin and user skin, + /// each transformed with the ruleset's own skin transformer individually. + /// + public class RulesetSkinProvidingContainer : SkinProvidingContainer + { + private readonly Ruleset ruleset; + private readonly IBeatmap beatmap; + + protected override Container Content { get; } + + public RulesetSkinProvidingContainer(Ruleset ruleset, IBeatmap beatmap, ISkin beatmapSkin) + { + this.ruleset = ruleset; + this.beatmap = beatmap; + + InternalChild = new BeatmapSkinProvidingContainer(ruleset.CreateLegacySkinProvider(beatmapSkin, beatmap)) + { + Child = Content = new Container + { + RelativeSizeAxes = Axes.Both, + } + }; + } + + [Resolved] + private SkinManager skinManager { get; set; } + + [BackgroundDependencyLoader] + private void load() + { + updateSkins(); + } + + protected override void OnSourceChanged() + { + updateSkins(); + base.OnSourceChanged(); + } + + private void updateSkins() + { + SkinLayers.Clear(); + SkinLayers.AddRange(skinManager.CurrentSkinLayers.Select(s => ruleset.CreateLegacySkinProvider(s, beatmap))); + } + } +} From e30f6581b384c35aa7dbe5e9d04ce254bb084e2d Mon Sep 17 00:00:00 2001 From: Salman Ahmed Date: Wed, 9 Jun 2021 21:30:26 +0300 Subject: [PATCH 05/54] Wrap gameplay content within a `RulesetSkinProvidingContainer` --- osu.Game/Screens/Edit/Compose/ComposeScreen.cs | 10 +--------- osu.Game/Screens/Play/Player.cs | 14 ++++---------- 2 files changed, 5 insertions(+), 19 deletions(-) diff --git a/osu.Game/Screens/Edit/Compose/ComposeScreen.cs b/osu.Game/Screens/Edit/Compose/ComposeScreen.cs index 61056aeced..b56f9bee14 100644 --- a/osu.Game/Screens/Edit/Compose/ComposeScreen.cs +++ b/osu.Game/Screens/Edit/Compose/ComposeScreen.cs @@ -73,15 +73,7 @@ namespace osu.Game.Screens.Edit.Compose { Debug.Assert(ruleset != null); - var beatmapSkinProvider = new BeatmapSkinProvidingContainer(beatmap.Value.Skin); - - // the beatmapSkinProvider is used as the fallback source here to allow the ruleset-specific skin implementation - // full access to all skin sources. - var rulesetSkinProvider = new SkinProvidingContainer(ruleset.CreateLegacySkinProvider(beatmapSkinProvider, EditorBeatmap.PlayableBeatmap)); - - // load the skinning hierarchy first. - // this is intentionally done in two stages to ensure things are in a loaded state before exposing the ruleset to skin sources. - return beatmapSkinProvider.WithChild(rulesetSkinProvider.WithChild(content)); + return new RulesetSkinProvidingContainer(ruleset, EditorBeatmap.PlayableBeatmap, beatmap.Value.Skin).WithChild(content); } #region Input Handling diff --git a/osu.Game/Screens/Play/Player.cs b/osu.Game/Screens/Play/Player.cs index f9036780aa..47c91cfc4d 100644 --- a/osu.Game/Screens/Play/Player.cs +++ b/osu.Game/Screens/Play/Player.cs @@ -234,29 +234,23 @@ namespace osu.Game.Screens.Play dependencies.CacheAs(GameplayBeatmap); - var beatmapSkinProvider = new BeatmapSkinProvidingContainer(Beatmap.Value.Skin); - - // the beatmapSkinProvider is used as the fallback source here to allow the ruleset-specific skin implementation - // full access to all skin sources. - var rulesetSkinProvider = new SkinProvidingContainer(GameplayRuleset.CreateLegacySkinProvider(beatmapSkinProvider, playableBeatmap)); + var rulesetSkinProvider = new RulesetSkinProvidingContainer(GameplayRuleset, playableBeatmap, Beatmap.Value.Skin); // load the skinning hierarchy first. // this is intentionally done in two stages to ensure things are in a loaded state before exposing the ruleset to skin sources. - GameplayClockContainer.Add(beatmapSkinProvider.WithChild(rulesetSkinProvider)); + GameplayClockContainer.Add(rulesetSkinProvider); rulesetSkinProvider.AddRange(new[] { - // underlay and gameplay should have access the to skinning sources. + // underlay and gameplay should have access to the skinning sources. createUnderlayComponents(), createGameplayComponents(Beatmap.Value, playableBeatmap) }); // also give the HUD a ruleset container to allow rulesets to potentially override HUD elements (used to disable combo counters etc.) // we may want to limit this in the future to disallow rulesets from outright replacing elements the user expects to be there. - var hudRulesetContainer = new SkinProvidingContainer(GameplayRuleset.CreateLegacySkinProvider(beatmapSkinProvider, playableBeatmap)); - // add the overlay components as a separate step as they proxy some elements from the above underlay/gameplay components. - GameplayClockContainer.Add(hudRulesetContainer.WithChild(createOverlayComponents(Beatmap.Value))); + rulesetSkinProvider.Add(createOverlayComponents(Beatmap.Value)); if (!DrawableRuleset.AllowGameplayOverlays) { From 1aaad7bfd442d74cfc28903fb4f4b35eb178edb7 Mon Sep 17 00:00:00 2001 From: Salman Ahmed Date: Wed, 9 Jun 2021 22:24:53 +0300 Subject: [PATCH 06/54] Apply few adjustments to skinning overlays comment --- osu.Game/Screens/Play/Player.cs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/osu.Game/Screens/Play/Player.cs b/osu.Game/Screens/Play/Player.cs index 47c91cfc4d..bec5181efe 100644 --- a/osu.Game/Screens/Play/Player.cs +++ b/osu.Game/Screens/Play/Player.cs @@ -247,9 +247,9 @@ namespace osu.Game.Screens.Play createGameplayComponents(Beatmap.Value, playableBeatmap) }); - // also give the HUD a ruleset container to allow rulesets to potentially override HUD elements (used to disable combo counters etc.) - // we may want to limit this in the future to disallow rulesets from outright replacing elements the user expects to be there. // add the overlay components as a separate step as they proxy some elements from the above underlay/gameplay components. + // also give the overlays the ruleset skin provider to allow rulesets to potentially override HUD elements (used to disable combo counters etc.) + // we may want to limit this in the future to disallow rulesets from outright replacing elements the user expects to be there. rulesetSkinProvider.Add(createOverlayComponents(Beatmap.Value)); if (!DrawableRuleset.AllowGameplayOverlays) From 18edbdd135dc31e989fe8f801b612a267ee6e415 Mon Sep 17 00:00:00 2001 From: Salman Ahmed Date: Thu, 10 Jun 2021 11:55:22 +0300 Subject: [PATCH 07/54] Remove mentioning of "layer" in skin providers `SkinSources` sounds better. --- osu.Game/Skinning/RulesetSkinProvidingContainer.cs | 4 ++-- osu.Game/Skinning/SkinProvidingContainer.cs | 14 +++++++------- 2 files changed, 9 insertions(+), 9 deletions(-) diff --git a/osu.Game/Skinning/RulesetSkinProvidingContainer.cs b/osu.Game/Skinning/RulesetSkinProvidingContainer.cs index dcf6281e38..1fe86d2873 100644 --- a/osu.Game/Skinning/RulesetSkinProvidingContainer.cs +++ b/osu.Game/Skinning/RulesetSkinProvidingContainer.cs @@ -52,8 +52,8 @@ namespace osu.Game.Skinning private void updateSkins() { - SkinLayers.Clear(); - SkinLayers.AddRange(skinManager.CurrentSkinLayers.Select(s => ruleset.CreateLegacySkinProvider(s, beatmap))); + SkinSources.Clear(); + SkinSources.AddRange(skinManager.CurrentSkinLayers.Select(s => ruleset.CreateLegacySkinProvider(s, beatmap))); } } } diff --git a/osu.Game/Skinning/SkinProvidingContainer.cs b/osu.Game/Skinning/SkinProvidingContainer.cs index cc9d8d0e8d..6686583a6f 100644 --- a/osu.Game/Skinning/SkinProvidingContainer.cs +++ b/osu.Game/Skinning/SkinProvidingContainer.cs @@ -25,7 +25,7 @@ namespace osu.Game.Skinning /// /// The list of skins provided by this . /// - protected readonly List SkinLayers = new List(); + protected readonly List SkinSources = new List(); [CanBeNull] private ISkinSource fallbackSource; @@ -43,7 +43,7 @@ namespace osu.Game.Skinning public SkinProvidingContainer(ISkin skin) : this() { - SkinLayers.Add(skin); + SkinSources.Add(skin); } protected SkinProvidingContainer() @@ -53,7 +53,7 @@ namespace osu.Game.Skinning public ISkin FindProvider(Func lookupFunction) { - foreach (var skin in SkinLayers) + foreach (var skin in SkinSources) { if (skin is ISkinSource source) { @@ -74,7 +74,7 @@ namespace osu.Game.Skinning { if (AllowDrawableLookup(component)) { - foreach (var skin in SkinLayers) + foreach (var skin in SkinSources) { Drawable sourceDrawable; if ((sourceDrawable = skin?.GetDrawableComponent(component)) != null) @@ -89,7 +89,7 @@ namespace osu.Game.Skinning { if (AllowTextureLookup(componentName)) { - foreach (var skin in SkinLayers) + foreach (var skin in SkinSources) { Texture sourceTexture; if ((sourceTexture = skin?.GetTexture(componentName, wrapModeS, wrapModeT)) != null) @@ -104,7 +104,7 @@ namespace osu.Game.Skinning { if (AllowSampleLookup(sampleInfo)) { - foreach (var skin in SkinLayers) + foreach (var skin in SkinSources) { ISample sourceSample; if ((sourceSample = skin?.GetSample(sampleInfo)) != null) @@ -127,7 +127,7 @@ namespace osu.Game.Skinning { if (canUseSkinLookup) { - foreach (var skin in SkinLayers) + foreach (var skin in SkinSources) { IBindable bindable; if ((bindable = skin?.GetConfig(lookup)) != null) From 530026b6755c6e5891969dd1d0c594da67c3ffd2 Mon Sep 17 00:00:00 2001 From: Salman Ahmed Date: Thu, 10 Jun 2021 11:56:13 +0300 Subject: [PATCH 08/54] Add simple xmldoc to ctors explaining their deal with `SkinSources` --- osu.Game/Skinning/SkinProvidingContainer.cs | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/osu.Game/Skinning/SkinProvidingContainer.cs b/osu.Game/Skinning/SkinProvidingContainer.cs index 6686583a6f..a7bc3ba379 100644 --- a/osu.Game/Skinning/SkinProvidingContainer.cs +++ b/osu.Game/Skinning/SkinProvidingContainer.cs @@ -40,12 +40,19 @@ namespace osu.Game.Skinning protected virtual bool AllowColourLookup => true; + /// + /// Constructs a new with a single skin added to the protected list. + /// public SkinProvidingContainer(ISkin skin) : this() { SkinSources.Add(skin); } + /// + /// Constructs a new with no sources. + /// Up to the implementation for adding to the list. + /// protected SkinProvidingContainer() { RelativeSizeAxes = Axes.Both; From 58cca9da06ecd1081b33f03ea57ea9cea8e88c0c Mon Sep 17 00:00:00 2001 From: Salman Ahmed Date: Thu, 10 Jun 2021 11:57:28 +0300 Subject: [PATCH 09/54] Revert "Expose the skin lookup layers of `SkinManager` to a property" This reverts commit 9e652715ced79d2a6f22180c03d3e6024a10da8d. --- osu.Game/Skinning/SkinManager.cs | 44 +++++++++++--------------------- 1 file changed, 15 insertions(+), 29 deletions(-) diff --git a/osu.Game/Skinning/SkinManager.cs b/osu.Game/Skinning/SkinManager.cs index 134156e44f..48d6b9254f 100644 --- a/osu.Game/Skinning/SkinManager.cs +++ b/osu.Game/Skinning/SkinManager.cs @@ -42,24 +42,6 @@ namespace osu.Game.Skinning public readonly Bindable CurrentSkin = new Bindable(); public readonly Bindable CurrentSkinInfo = new Bindable(SkinInfo.Default) { Default = SkinInfo.Default }; - /// - /// The skin layers of the currently selected user skin for performing lookups on, - /// in order of preference (user skin first, then fallback skins). - /// - public IEnumerable CurrentSkinLayers - { - get - { - yield return CurrentSkin.Value; - - // TODO: we also want to return a DefaultLegacySkin here if the current *beatmap* is providing any skinned elements. - // When attempting to address this, we may want to move the full DefaultLegacySkin fallback logic to within Player itself (to better allow - // for beatmap skin visibility). - if (CurrentSkin.Value is LegacySkin) - yield return defaultLegacySkin; - } - } - public override IEnumerable HandledExtensions => new[] { ".osk" }; protected override string[] HashableFileTypes => new[] { ".ini" }; @@ -238,11 +220,11 @@ namespace osu.Game.Skinning public ISkin FindProvider(Func lookupFunction) { - foreach (var skin in CurrentSkinLayers) - { - if (lookupFunction(skin)) - return skin; - } + if (lookupFunction(CurrentSkin.Value)) + return CurrentSkin.Value; + + if (CurrentSkin.Value is LegacySkin && lookupFunction(defaultLegacySkin)) + return defaultLegacySkin; return null; } @@ -250,12 +232,16 @@ namespace osu.Game.Skinning private T lookupWithFallback(Func func) where T : class { - foreach (var skin in CurrentSkinLayers) - { - var result = func(skin); - if (result != null) - return result; - } + var selectedSkin = func(CurrentSkin.Value); + + if (selectedSkin != null) + return selectedSkin; + + // TODO: we also want to return a DefaultLegacySkin here if the current *beatmap* is providing any skinned elements. + // When attempting to address this, we may want to move the full DefaultLegacySkin fallback logic to within Player itself (to better allow + // for beatmap skin visibility). + if (CurrentSkin.Value is LegacySkin) + return func(defaultLegacySkin); return null; } From 59be3588eb281d916ba68297f19e6eb4a5c362ed Mon Sep 17 00:00:00 2001 From: Salman Ahmed Date: Thu, 10 Jun 2021 12:17:51 +0300 Subject: [PATCH 10/54] Change `SkinSources` to a bindable list for binding `SourceChanged` events --- osu.Game/Skinning/SkinProvidingContainer.cs | 44 +++++++++++---------- 1 file changed, 23 insertions(+), 21 deletions(-) diff --git a/osu.Game/Skinning/SkinProvidingContainer.cs b/osu.Game/Skinning/SkinProvidingContainer.cs index 3739172367..fa0780ff29 100644 --- a/osu.Game/Skinning/SkinProvidingContainer.cs +++ b/osu.Game/Skinning/SkinProvidingContainer.cs @@ -2,7 +2,8 @@ // See the LICENCE file in the repository root for full licence text. using System; -using System.Collections.Generic; +using System.Collections.Specialized; +using System.Linq; using JetBrains.Annotations; using osu.Framework.Allocation; using osu.Framework.Audio.Sample; @@ -25,7 +26,7 @@ namespace osu.Game.Skinning /// /// The list of skins provided by this . /// - protected readonly List SkinSources = new List(); + protected readonly BindableList SkinSources = new BindableList(); [CanBeNull] private ISkinSource fallbackSource; @@ -61,8 +62,23 @@ namespace osu.Game.Skinning noFallbackLookupProxy = new NoFallbackProxy(this); - if (skin is ISkinSource source) - source.SourceChanged += TriggerSourceChanged; + SkinSources.BindCollectionChanged(((_, args) => + { + switch (args.Action) + { + case NotifyCollectionChangedAction.Remove: + foreach (var source in args.OldItems.Cast().OfType()) + source.SourceChanged -= OnSourceChanged; + + break; + + case NotifyCollectionChangedAction.Add: + foreach (var source in args.NewItems.Cast().OfType()) + source.SourceChanged += OnSourceChanged; + + break; + } + }), true); } public ISkin FindProvider(Func lookupFunction) @@ -146,21 +162,9 @@ namespace osu.Game.Skinning public IBindable GetConfig(TLookup lookup, bool fallback) { if (lookup is GlobalSkinColours || lookup is SkinCustomColourLookup) - return lookupWithFallback(lookup, AllowColourLookup); + return lookupWithFallback(lookup, AllowColourLookup, fallback); - return lookupWithFallback(lookup, AllowConfigurationLookup); - if (skin != null) - { - if (lookup is GlobalSkinColours || lookup is SkinCustomColourLookup) - return lookupWithFallback(lookup, AllowColourLookup, fallback); - - return lookupWithFallback(lookup, AllowConfigurationLookup, fallback); - } - - if (!fallback) - return null; - - return fallbackSource?.GetConfig(lookup); + return lookupWithFallback(lookup, AllowConfigurationLookup, fallback); } private IBindable lookupWithFallback(TLookup lookup, bool canUseSkinLookup, bool canUseFallback) @@ -205,10 +209,8 @@ namespace osu.Game.Skinning if (fallbackSource != null) fallbackSource.SourceChanged -= OnSourceChanged; - fallbackSource.SourceChanged -= TriggerSourceChanged; - if (skin is ISkinSource source) - source.SourceChanged -= TriggerSourceChanged; + SkinSources.Clear(); } private class NoFallbackProxy : ISkinSource From c3a2f2c2a4559b3740368065b78564c1306dd3cf Mon Sep 17 00:00:00 2001 From: Salman Ahmed Date: Thu, 10 Jun 2021 13:04:34 +0300 Subject: [PATCH 11/54] Expose default `SkinManager` providers for use in `RulesetSkinProvidingContainer` --- osu.Game/Skinning/RulesetSkinProvidingContainer.cs | 9 +++++++-- osu.Game/Skinning/SkinManager.cs | 14 ++++++++++++-- 2 files changed, 19 insertions(+), 4 deletions(-) diff --git a/osu.Game/Skinning/RulesetSkinProvidingContainer.cs b/osu.Game/Skinning/RulesetSkinProvidingContainer.cs index 1fe86d2873..eadf0e05b9 100644 --- a/osu.Game/Skinning/RulesetSkinProvidingContainer.cs +++ b/osu.Game/Skinning/RulesetSkinProvidingContainer.cs @@ -1,7 +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 System.Linq; using osu.Framework.Allocation; using osu.Framework.Graphics; using osu.Framework.Graphics.Containers; @@ -53,7 +52,13 @@ namespace osu.Game.Skinning private void updateSkins() { SkinSources.Clear(); - SkinSources.AddRange(skinManager.CurrentSkinLayers.Select(s => ruleset.CreateLegacySkinProvider(s, beatmap))); + + SkinSources.Add(ruleset.CreateLegacySkinProvider(skinManager.CurrentSkin.Value, beatmap)); + + if (skinManager.CurrentSkin.Value is LegacySkin) + SkinSources.Add(ruleset.CreateLegacySkinProvider(skinManager.DefaultLegacySkin, beatmap)); + + SkinSources.Add(ruleset.CreateLegacySkinProvider(skinManager.DefaultSkin, beatmap)); } } } diff --git a/osu.Game/Skinning/SkinManager.cs b/osu.Game/Skinning/SkinManager.cs index 9e274227a2..89f166dc2a 100644 --- a/osu.Game/Skinning/SkinManager.cs +++ b/osu.Game/Skinning/SkinManager.cs @@ -48,9 +48,19 @@ namespace osu.Game.Skinning protected override string ImportFromStablePath => "Skins"; + private readonly Skin defaultSkin; + + /// + /// An providing the resources of the default skin. + /// + public ISkin DefaultSkin => defaultSkin; + private readonly Skin defaultLegacySkin; - private readonly Skin defaultSkin; + /// + /// An providing the resources of the default legacy skin. + /// + public ISkin DefaultLegacySkin => defaultLegacySkin; public SkinManager(Storage storage, DatabaseContextFactory contextFactory, GameHost host, IResourceStore resources, AudioManager audio) : base(storage, contextFactory, new SkinStore(contextFactory, storage), host) @@ -84,7 +94,7 @@ namespace osu.Game.Skinning { var userSkins = GetAllUserSkins(); userSkins.Insert(0, SkinInfo.Default); - userSkins.Insert(1, DefaultLegacySkin.Info); + userSkins.Insert(1, Skinning.DefaultLegacySkin.Info); return userSkins; } From 26cdcc8d78f4bb29d2730c7ad291db0651014177 Mon Sep 17 00:00:00 2001 From: Salman Ahmed Date: Thu, 10 Jun 2021 13:05:44 +0300 Subject: [PATCH 12/54] Remove stale access to `Source` from master merge --- .../Skinning/Legacy/CatchLegacySkinTransformer.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/osu.Game.Rulesets.Catch/Skinning/Legacy/CatchLegacySkinTransformer.cs b/osu.Game.Rulesets.Catch/Skinning/Legacy/CatchLegacySkinTransformer.cs index a196ebedd7..a5a1d1504f 100644 --- a/osu.Game.Rulesets.Catch/Skinning/Legacy/CatchLegacySkinTransformer.cs +++ b/osu.Game.Rulesets.Catch/Skinning/Legacy/CatchLegacySkinTransformer.cs @@ -66,7 +66,7 @@ namespace osu.Game.Rulesets.Catch.Skinning.Legacy return null; case CatchSkinComponents.Catcher: - var version = Source.GetConfig(LegacySkinConfiguration.LegacySetting.Version)?.Value ?? 1; + var version = GetConfig(LegacySkinConfiguration.LegacySetting.Version)?.Value ?? 1; if (version < 2.3m) { From 5c9c424a0d032ec412d7be58953176d86886b233 Mon Sep 17 00:00:00 2001 From: Salman Ahmed Date: Thu, 10 Jun 2021 13:15:18 +0300 Subject: [PATCH 13/54] Switch state case placements for consistency Tickled me. --- osu.Game/Skinning/SkinProvidingContainer.cs | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/osu.Game/Skinning/SkinProvidingContainer.cs b/osu.Game/Skinning/SkinProvidingContainer.cs index fa0780ff29..74a465a91f 100644 --- a/osu.Game/Skinning/SkinProvidingContainer.cs +++ b/osu.Game/Skinning/SkinProvidingContainer.cs @@ -66,17 +66,17 @@ namespace osu.Game.Skinning { switch (args.Action) { - case NotifyCollectionChangedAction.Remove: - foreach (var source in args.OldItems.Cast().OfType()) - source.SourceChanged -= OnSourceChanged; - - break; - case NotifyCollectionChangedAction.Add: foreach (var source in args.NewItems.Cast().OfType()) source.SourceChanged += OnSourceChanged; break; + + case NotifyCollectionChangedAction.Remove: + foreach (var source in args.OldItems.Cast().OfType()) + source.SourceChanged -= OnSourceChanged; + + break; } }), true); } From 09a2d008d226c8d0448067ce6b37d8d4cce86f87 Mon Sep 17 00:00:00 2001 From: Salman Ahmed Date: Thu, 10 Jun 2021 13:41:41 +0300 Subject: [PATCH 14/54] Refrain from attempting to transform null skins --- osu.Game/Rulesets/Ruleset.cs | 2 +- osu.Game/Skinning/LegacySkinTransformer.cs | 7 +++++-- osu.Game/Skinning/RulesetSkinProvidingContainer.cs | 5 +++-- 3 files changed, 9 insertions(+), 5 deletions(-) diff --git a/osu.Game/Rulesets/Ruleset.cs b/osu.Game/Rulesets/Ruleset.cs index 9f9f42eda4..9fdaca88fd 100644 --- a/osu.Game/Rulesets/Ruleset.cs +++ b/osu.Game/Rulesets/Ruleset.cs @@ -127,7 +127,7 @@ namespace osu.Game.Rulesets [CanBeNull] public ModAutoplay GetAutoplayMod() => GetAllMods().OfType().FirstOrDefault(); - public virtual ISkin CreateLegacySkinProvider(ISkin skin, IBeatmap beatmap) => null; + public virtual ISkin CreateLegacySkinProvider([NotNull] ISkin skin, IBeatmap beatmap) => null; protected Ruleset() { diff --git a/osu.Game/Skinning/LegacySkinTransformer.cs b/osu.Game/Skinning/LegacySkinTransformer.cs index cd896ab51e..fedd63c7de 100644 --- a/osu.Game/Skinning/LegacySkinTransformer.cs +++ b/osu.Game/Skinning/LegacySkinTransformer.cs @@ -1,6 +1,8 @@ // 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 JetBrains.Annotations; using osu.Framework.Audio.Sample; using osu.Framework.Bindables; using osu.Framework.Graphics; @@ -20,11 +22,12 @@ namespace osu.Game.Skinning /// /// The which is being transformed. /// + [NotNull] protected ISkin Skin { get; } - protected LegacySkinTransformer(ISkin skin) + protected LegacySkinTransformer([NotNull] ISkin skin) { - Skin = skin; + Skin = skin ?? throw new ArgumentNullException(nameof(skin)); } public abstract Drawable GetDrawableComponent(ISkinComponent component); diff --git a/osu.Game/Skinning/RulesetSkinProvidingContainer.cs b/osu.Game/Skinning/RulesetSkinProvidingContainer.cs index eadf0e05b9..18399cbdb4 100644 --- a/osu.Game/Skinning/RulesetSkinProvidingContainer.cs +++ b/osu.Game/Skinning/RulesetSkinProvidingContainer.cs @@ -1,6 +1,7 @@ // Copyright (c) ppy Pty Ltd . Licensed under the MIT Licence. // See the LICENCE file in the repository root for full licence text. +using JetBrains.Annotations; using osu.Framework.Allocation; using osu.Framework.Graphics; using osu.Framework.Graphics.Containers; @@ -20,12 +21,12 @@ namespace osu.Game.Skinning protected override Container Content { get; } - public RulesetSkinProvidingContainer(Ruleset ruleset, IBeatmap beatmap, ISkin beatmapSkin) + public RulesetSkinProvidingContainer(Ruleset ruleset, IBeatmap beatmap, [CanBeNull] ISkin beatmapSkin) { this.ruleset = ruleset; this.beatmap = beatmap; - InternalChild = new BeatmapSkinProvidingContainer(ruleset.CreateLegacySkinProvider(beatmapSkin, beatmap)) + InternalChild = new BeatmapSkinProvidingContainer(beatmapSkin == null ? null : ruleset.CreateLegacySkinProvider(beatmapSkin, beatmap)) { Child = Content = new Container { From ef2c4fd0d8ec68e8754aa12cf5cf381a5f839ed9 Mon Sep 17 00:00:00 2001 From: Salman Ahmed Date: Thu, 10 Jun 2021 15:05:08 +0300 Subject: [PATCH 15/54] Make `RulesetSkinProvidingContainer` able to be overriden for testing purposes --- osu.Game/Screens/Play/Player.cs | 4 +++- .../Skinning/RulesetSkinProvidingContainer.cs | 20 +++++++++---------- 2 files changed, 13 insertions(+), 11 deletions(-) diff --git a/osu.Game/Screens/Play/Player.cs b/osu.Game/Screens/Play/Player.cs index bec5181efe..fbcc7ea96f 100644 --- a/osu.Game/Screens/Play/Player.cs +++ b/osu.Game/Screens/Play/Player.cs @@ -234,7 +234,7 @@ namespace osu.Game.Screens.Play dependencies.CacheAs(GameplayBeatmap); - var rulesetSkinProvider = new RulesetSkinProvidingContainer(GameplayRuleset, playableBeatmap, Beatmap.Value.Skin); + var rulesetSkinProvider = CreateRulesetSkinProvider(GameplayRuleset, playableBeatmap, Beatmap.Value.Skin); // load the skinning hierarchy first. // this is intentionally done in two stages to ensure things are in a loaded state before exposing the ruleset to skin sources. @@ -315,6 +315,8 @@ namespace osu.Game.Screens.Play protected virtual GameplayClockContainer CreateGameplayClockContainer(WorkingBeatmap beatmap, double gameplayStart) => new MasterGameplayClockContainer(beatmap, gameplayStart); + protected virtual RulesetSkinProvidingContainer CreateRulesetSkinProvider(Ruleset ruleset, IBeatmap beatmap, ISkin beatmapSkin) => new RulesetSkinProvidingContainer(ruleset, beatmap, beatmapSkin); + private Drawable createUnderlayComponents() => DimmableStoryboard = new DimmableStoryboard(Beatmap.Value.Storyboard) { RelativeSizeAxes = Axes.Both }; diff --git a/osu.Game/Skinning/RulesetSkinProvidingContainer.cs b/osu.Game/Skinning/RulesetSkinProvidingContainer.cs index 18399cbdb4..8087043230 100644 --- a/osu.Game/Skinning/RulesetSkinProvidingContainer.cs +++ b/osu.Game/Skinning/RulesetSkinProvidingContainer.cs @@ -16,15 +16,15 @@ namespace osu.Game.Skinning /// public class RulesetSkinProvidingContainer : SkinProvidingContainer { - private readonly Ruleset ruleset; - private readonly IBeatmap beatmap; + protected readonly Ruleset Ruleset; + protected readonly IBeatmap Beatmap; protected override Container Content { get; } public RulesetSkinProvidingContainer(Ruleset ruleset, IBeatmap beatmap, [CanBeNull] ISkin beatmapSkin) { - this.ruleset = ruleset; - this.beatmap = beatmap; + Ruleset = ruleset; + Beatmap = beatmap; InternalChild = new BeatmapSkinProvidingContainer(beatmapSkin == null ? null : ruleset.CreateLegacySkinProvider(beatmapSkin, beatmap)) { @@ -41,25 +41,25 @@ namespace osu.Game.Skinning [BackgroundDependencyLoader] private void load() { - updateSkins(); + UpdateSkins(); } protected override void OnSourceChanged() { - updateSkins(); + UpdateSkins(); base.OnSourceChanged(); } - private void updateSkins() + protected virtual void UpdateSkins() { SkinSources.Clear(); - SkinSources.Add(ruleset.CreateLegacySkinProvider(skinManager.CurrentSkin.Value, beatmap)); + SkinSources.Add(Ruleset.CreateLegacySkinProvider(skinManager.CurrentSkin.Value, Beatmap)); if (skinManager.CurrentSkin.Value is LegacySkin) - SkinSources.Add(ruleset.CreateLegacySkinProvider(skinManager.DefaultLegacySkin, beatmap)); + SkinSources.Add(Ruleset.CreateLegacySkinProvider(skinManager.DefaultLegacySkin, Beatmap)); - SkinSources.Add(ruleset.CreateLegacySkinProvider(skinManager.DefaultSkin, beatmap)); + SkinSources.Add(Ruleset.CreateLegacySkinProvider(skinManager.DefaultSkin, Beatmap)); } } } From 23d6c366acc34ee9f92a116ce2cace40008f3b04 Mon Sep 17 00:00:00 2001 From: Salman Ahmed Date: Thu, 10 Jun 2021 15:36:53 +0300 Subject: [PATCH 16/54] Add method for assigning arbitrary skins to player in test scenes --- osu.Game/Tests/Visual/PlayerTestScene.cs | 8 +++++++ osu.Game/Tests/Visual/TestPlayer.cs | 28 ++++++++++++++++++++++++ 2 files changed, 36 insertions(+) diff --git a/osu.Game/Tests/Visual/PlayerTestScene.cs b/osu.Game/Tests/Visual/PlayerTestScene.cs index 088e997de9..e42a043eec 100644 --- a/osu.Game/Tests/Visual/PlayerTestScene.cs +++ b/osu.Game/Tests/Visual/PlayerTestScene.cs @@ -10,6 +10,7 @@ using osu.Framework.Testing; using osu.Game.Configuration; using osu.Game.Rulesets; using osu.Game.Rulesets.Mods; +using osu.Game.Skinning; namespace osu.Game.Tests.Visual { @@ -78,6 +79,8 @@ namespace osu.Game.Tests.Visual } Player = CreatePlayer(ruleset); + Player.Skin = GetPlayerSkin(); + LoadScreen(Player); } @@ -93,6 +96,11 @@ namespace osu.Game.Tests.Visual [NotNull] protected abstract Ruleset CreatePlayerRuleset(); + /// + /// Creates an to be put inside the 's ruleset skin providing container. + /// + protected virtual ISkin GetPlayerSkin() => null; + protected sealed override Ruleset CreateRuleset() => CreatePlayerRuleset(); protected virtual TestPlayer CreatePlayer(Ruleset ruleset) => new TestPlayer(false, false); diff --git a/osu.Game/Tests/Visual/TestPlayer.cs b/osu.Game/Tests/Visual/TestPlayer.cs index 09da4db952..eecf8a2f6e 100644 --- a/osu.Game/Tests/Visual/TestPlayer.cs +++ b/osu.Game/Tests/Visual/TestPlayer.cs @@ -3,13 +3,17 @@ using System.Collections.Generic; using System.Linq; +using JetBrains.Annotations; using osu.Framework.Allocation; using osu.Framework.Bindables; +using osu.Game.Beatmaps; +using osu.Game.Rulesets; using osu.Game.Rulesets.Judgements; using osu.Game.Rulesets.Mods; using osu.Game.Rulesets.Scoring; using osu.Game.Rulesets.UI; using osu.Game.Screens.Play; +using osu.Game.Skinning; namespace osu.Game.Tests.Visual { @@ -18,6 +22,8 @@ namespace osu.Game.Tests.Visual /// public class TestPlayer : Player { + public ISkin Skin { get; set; } + protected override bool PauseOnFocusLost { get; } public new DrawableRuleset DrawableRuleset => base.DrawableRuleset; @@ -74,5 +80,27 @@ namespace osu.Game.Tests.Visual { ScoreProcessor.NewJudgement += r => Results.Add(r); } + + protected override RulesetSkinProvidingContainer CreateRulesetSkinProvider(Ruleset ruleset, IBeatmap beatmap, ISkin beatmapSkin) + => new TestSkinProvidingContainer(Skin, ruleset, beatmap, beatmapSkin); + + private class TestSkinProvidingContainer : RulesetSkinProvidingContainer + { + private readonly ISkin skin; + + public TestSkinProvidingContainer(ISkin skin, Ruleset ruleset, IBeatmap beatmap, [CanBeNull] ISkin beatmapSkin) + : base(ruleset, beatmap, beatmapSkin) + { + this.skin = skin; + } + + protected override void UpdateSkins() + { + base.UpdateSkins(); + + if (skin != null) + SkinSources.Insert(0, Ruleset.CreateLegacySkinProvider(skin, Beatmap)); + } + } } } From 680791301f70b8476bb40af982e6adba8254bcb5 Mon Sep 17 00:00:00 2001 From: Salman Ahmed Date: Thu, 10 Jun 2021 16:36:27 +0300 Subject: [PATCH 17/54] Consume new method rather than caching skin sources on top of `Player` --- .../TestSceneLegacyBeatmapSkin.cs | 7 +--- .../TestSceneSkinFallbacks.cs | 26 +----------- .../Tests/Beatmaps/HitObjectSampleTest.cs | 42 ++----------------- .../Beatmaps/LegacyBeatmapSkinColourTest.cs | 32 ++++---------- .../Tests/Visual/LegacySkinPlayerTestScene.cs | 17 +------- 5 files changed, 16 insertions(+), 108 deletions(-) diff --git a/osu.Game.Rulesets.Catch.Tests/TestSceneLegacyBeatmapSkin.cs b/osu.Game.Rulesets.Catch.Tests/TestSceneLegacyBeatmapSkin.cs index bc3daca16f..0077ff9e3c 100644 --- a/osu.Game.Rulesets.Catch.Tests/TestSceneLegacyBeatmapSkin.cs +++ b/osu.Game.Rulesets.Catch.Tests/TestSceneLegacyBeatmapSkin.cs @@ -101,15 +101,10 @@ namespace osu.Game.Rulesets.Catch.Tests AddAssert("is custom hyper dash fruit colours", () => ((CatchExposedPlayer)TestPlayer).UsableHyperDashFruitColour == TestSkin.HYPER_DASH_FRUIT_COLOUR); } - protected override ExposedPlayer CreateTestPlayer(bool userHasCustomColours) => new CatchExposedPlayer(userHasCustomColours); + protected override ExposedPlayer CreateTestPlayer() => new CatchExposedPlayer(); private class CatchExposedPlayer : ExposedPlayer { - public CatchExposedPlayer(bool userHasCustomColours) - : base(userHasCustomColours) - { - } - public Color4 UsableHyperDashColour => GameplayClockContainer.ChildrenOfType() .First() diff --git a/osu.Game.Rulesets.Osu.Tests/TestSceneSkinFallbacks.cs b/osu.Game.Rulesets.Osu.Tests/TestSceneSkinFallbacks.cs index fd523fffcb..2b45818aa9 100644 --- a/osu.Game.Rulesets.Osu.Tests/TestSceneSkinFallbacks.cs +++ b/osu.Game.Rulesets.Osu.Tests/TestSceneSkinFallbacks.cs @@ -21,7 +21,6 @@ using osu.Game.Graphics.Sprites; using osu.Game.Rulesets.Osu.Objects.Drawables; using osu.Game.Skinning; using osu.Game.Storyboards; -using osu.Game.Tests.Visual; namespace osu.Game.Rulesets.Osu.Tests { @@ -99,7 +98,7 @@ namespace osu.Game.Rulesets.Osu.Tests [Resolved] private AudioManager audio { get; set; } - protected override TestPlayer CreatePlayer(Ruleset ruleset) => new SkinProvidingPlayer(testUserSkin); + protected override ISkin GetPlayerSkin() => testUserSkin; protected override WorkingBeatmap CreateWorkingBeatmap(IBeatmap beatmap, Storyboard storyboard = null) => new CustomSkinWorkingBeatmap(beatmap, storyboard, Clock, audio, testBeatmapSkin); @@ -116,27 +115,6 @@ namespace osu.Game.Rulesets.Osu.Tests protected override ISkin GetSkin() => skin; } - public class SkinProvidingPlayer : TestPlayer - { - private readonly TestSource userSkin; - - public SkinProvidingPlayer(TestSource userSkin) - { - this.userSkin = userSkin; - } - - private DependencyContainer dependencies; - - protected override IReadOnlyDependencyContainer CreateChildDependencies(IReadOnlyDependencyContainer parent) - { - dependencies = new DependencyContainer(base.CreateChildDependencies(parent)); - - dependencies.CacheAs(userSkin); - - return dependencies; - } - } - public class TestSource : ISkinSource { private readonly string identifier; @@ -164,8 +142,8 @@ namespace osu.Game.Rulesets.Osu.Tests public ISample GetSample(ISampleInfo sampleInfo) => null; - public TValue GetValue(Func query) where TConfiguration : SkinConfiguration => default; public IBindable GetConfig(TLookup lookup) => null; + public ISkin FindProvider(Func lookupFunction) => null; public event Action SourceChanged; diff --git a/osu.Game/Tests/Beatmaps/HitObjectSampleTest.cs b/osu.Game/Tests/Beatmaps/HitObjectSampleTest.cs index 7ee6c519b7..7af0397726 100644 --- a/osu.Game/Tests/Beatmaps/HitObjectSampleTest.cs +++ b/osu.Game/Tests/Beatmaps/HitObjectSampleTest.cs @@ -47,15 +47,11 @@ namespace osu.Game.Tests.Beatmaps private readonly TestResourceStore userSkinResourceStore = new TestResourceStore(); private readonly TestResourceStore beatmapSkinResourceStore = new TestResourceStore(); - private SkinSourceDependencyContainer dependencies; private IBeatmap currentTestBeatmap; protected sealed override bool HasCustomSteps => true; protected override bool Autoplay => true; - protected sealed override IReadOnlyDependencyContainer CreateChildDependencies(IReadOnlyDependencyContainer parent) - => new DependencyContainer(dependencies = new SkinSourceDependencyContainer(base.CreateChildDependencies(parent))); - protected sealed override IBeatmap CreateBeatmap(RulesetInfo ruleset) => currentTestBeatmap; protected sealed override WorkingBeatmap CreateWorkingBeatmap(IBeatmap beatmap, Storyboard storyboard = null) @@ -63,6 +59,8 @@ namespace osu.Game.Tests.Beatmaps protected override TestPlayer CreatePlayer(Ruleset ruleset) => new TestPlayer(false); + protected override ISkin GetPlayerSkin() => Skin; + protected void CreateTestWithBeatmap(string filename) { CreateTest(() => @@ -109,8 +107,7 @@ namespace osu.Game.Tests.Beatmaps } }; - // Need to refresh the cached skin source to refresh the skin resource store. - dependencies.SkinSource = new SkinProvidingContainer(Skin = new LegacySkin(userSkinInfo, this)); + Skin = new LegacySkin(userSkinInfo, this); }); } @@ -132,39 +129,6 @@ namespace osu.Game.Tests.Beatmaps #endregion - private class SkinSourceDependencyContainer : IReadOnlyDependencyContainer - { - public ISkinSource SkinSource; - - private readonly IReadOnlyDependencyContainer fallback; - - public SkinSourceDependencyContainer(IReadOnlyDependencyContainer fallback) - { - this.fallback = fallback; - } - - public object Get(Type type) - { - if (type == typeof(ISkinSource)) - return SkinSource; - - return fallback.Get(type); - } - - public object Get(Type type, CacheInfo info) - { - if (type == typeof(ISkinSource)) - return SkinSource; - - return fallback.Get(type, info); - } - - public void Inject(T instance) where T : class - { - // Never used directly - } - } - private class TestResourceStore : IResourceStore { public readonly List PerformedLookups = new List(); diff --git a/osu.Game/Tests/Beatmaps/LegacyBeatmapSkinColourTest.cs b/osu.Game/Tests/Beatmaps/LegacyBeatmapSkinColourTest.cs index 2540b6d7da..1feb3eebbf 100644 --- a/osu.Game/Tests/Beatmaps/LegacyBeatmapSkinColourTest.cs +++ b/osu.Game/Tests/Beatmaps/LegacyBeatmapSkinColourTest.cs @@ -4,13 +4,11 @@ using System; using System.Collections.Generic; using System.Linq; -using osu.Framework.Allocation; using osu.Framework.Audio; 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; @@ -49,36 +47,24 @@ namespace osu.Game.Tests.Beatmaps protected virtual ExposedPlayer LoadBeatmap(bool userHasCustomColours) { - ExposedPlayer player; - Beatmap.Value = testBeatmap; - LoadScreen(player = CreateTestPlayer(userHasCustomColours)); + ExposedPlayer player = CreateTestPlayer(); + + player.Skin = new TestSkin(userHasCustomColours); + + LoadScreen(player); return player; } - protected virtual ExposedPlayer CreateTestPlayer(bool userHasCustomColours) => new ExposedPlayer(userHasCustomColours); + protected virtual ExposedPlayer CreateTestPlayer() => new ExposedPlayer(); - protected class ExposedPlayer : Player + protected class ExposedPlayer : TestPlayer { - protected readonly bool UserHasCustomColours; - - public ExposedPlayer(bool userHasCustomColours) - : base(new PlayerConfiguration - { - AllowPause = false, - ShowResults = false, - }) + public ExposedPlayer() + : base(false, false) { - 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 => diff --git a/osu.Game/Tests/Visual/LegacySkinPlayerTestScene.cs b/osu.Game/Tests/Visual/LegacySkinPlayerTestScene.cs index b810bbf6ae..14a928d3c1 100644 --- a/osu.Game/Tests/Visual/LegacySkinPlayerTestScene.cs +++ b/osu.Game/Tests/Visual/LegacySkinPlayerTestScene.cs @@ -5,7 +5,6 @@ using NUnit.Framework; using osu.Framework.Allocation; using osu.Framework.Extensions.IEnumerableExtensions; using osu.Framework.Testing; -using osu.Game.Rulesets; using osu.Game.Skinning; namespace osu.Game.Tests.Visual @@ -15,15 +14,12 @@ namespace osu.Game.Tests.Visual { protected LegacySkin LegacySkin { get; private set; } - private ISkinSource legacySkinSource; - - protected override TestPlayer CreatePlayer(Ruleset ruleset) => new SkinProvidingPlayer(legacySkinSource); + protected override ISkin GetPlayerSkin() => LegacySkin; [BackgroundDependencyLoader] private void load(SkinManager skins) { LegacySkin = new DefaultLegacySkin(skins); - legacySkinSource = new SkinProvidingContainer(LegacySkin); } [SetUpSteps] @@ -48,16 +44,5 @@ namespace osu.Game.Tests.Visual t.Reload(); })); } - - public class SkinProvidingPlayer : TestPlayer - { - [Cached(typeof(ISkinSource))] - private readonly ISkinSource skinSource; - - public SkinProvidingPlayer(ISkinSource skinSource) - { - this.skinSource = skinSource; - } - } } } From 2240e2c39c7b1bf3ebe846b3d8c40119086b0e58 Mon Sep 17 00:00:00 2001 From: Salman Ahmed Date: Thu, 10 Jun 2021 17:23:15 +0300 Subject: [PATCH 18/54] Refrain from attempting to clear skin sources in disposal `Drawable.Dispose` is usually in an asynchronous context (async disposals stuff) and therefore this could cause a "collection was modified; enumeration opeartion may not execute" exception. --- osu.Game/Skinning/SkinProvidingContainer.cs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/osu.Game/Skinning/SkinProvidingContainer.cs b/osu.Game/Skinning/SkinProvidingContainer.cs index 74a465a91f..078c666472 100644 --- a/osu.Game/Skinning/SkinProvidingContainer.cs +++ b/osu.Game/Skinning/SkinProvidingContainer.cs @@ -210,7 +210,8 @@ namespace osu.Game.Skinning if (fallbackSource != null) fallbackSource.SourceChanged -= OnSourceChanged; - SkinSources.Clear(); + foreach (var source in SkinSources.OfType()) + source.SourceChanged -= OnSourceChanged; } private class NoFallbackProxy : ISkinSource From b9050f91a440c8d10cd3637d0fd179c38298a9c8 Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Fri, 11 Jun 2021 14:49:35 +0900 Subject: [PATCH 19/54] Expose as `Skin`s and consume `SkinInfo` from instances --- osu.Game/Skinning/SkinManager.cs | 30 +++++++++++++----------------- 1 file changed, 13 insertions(+), 17 deletions(-) diff --git a/osu.Game/Skinning/SkinManager.cs b/osu.Game/Skinning/SkinManager.cs index 89f166dc2a..ffdbadf54c 100644 --- a/osu.Game/Skinning/SkinManager.cs +++ b/osu.Game/Skinning/SkinManager.cs @@ -48,19 +48,15 @@ namespace osu.Game.Skinning protected override string ImportFromStablePath => "Skins"; - private readonly Skin defaultSkin; - /// /// An providing the resources of the default skin. /// - public ISkin DefaultSkin => defaultSkin; - - private readonly Skin defaultLegacySkin; + public Skin DefaultSkin { get; } /// /// An providing the resources of the default legacy skin. /// - public ISkin DefaultLegacySkin => defaultLegacySkin; + public Skin DefaultLegacySkin { get; } public SkinManager(Storage storage, DatabaseContextFactory contextFactory, GameHost host, IResourceStore resources, AudioManager audio) : base(storage, contextFactory, new SkinStore(contextFactory, storage), host) @@ -69,12 +65,12 @@ namespace osu.Game.Skinning this.host = host; this.resources = resources; - defaultLegacySkin = new DefaultLegacySkin(this); - defaultSkin = new DefaultSkin(this); + DefaultLegacySkin = new DefaultLegacySkin(this); + DefaultSkin = new DefaultSkin(this); CurrentSkinInfo.ValueChanged += skin => CurrentSkin.Value = GetSkin(skin.NewValue); - CurrentSkin.Value = defaultSkin; + CurrentSkin.Value = DefaultSkin; CurrentSkin.ValueChanged += skin => { if (skin.NewValue.SkinInfo != CurrentSkinInfo.Value) @@ -93,8 +89,8 @@ namespace osu.Game.Skinning public List GetAllUsableSkins() { var userSkins = GetAllUserSkins(); - userSkins.Insert(0, SkinInfo.Default); - userSkins.Insert(1, Skinning.DefaultLegacySkin.Info); + userSkins.Insert(0, DefaultSkin.SkinInfo); + userSkins.Insert(1, DefaultLegacySkin.SkinInfo); return userSkins; } @@ -236,11 +232,11 @@ namespace osu.Game.Skinning if (lookupFunction(CurrentSkin.Value)) return CurrentSkin.Value; - if (CurrentSkin.Value is LegacySkin && lookupFunction(defaultLegacySkin)) - return defaultLegacySkin; + if (CurrentSkin.Value is LegacySkin && lookupFunction(DefaultLegacySkin)) + return DefaultLegacySkin; - if (lookupFunction(defaultSkin)) - return defaultSkin; + if (lookupFunction(DefaultSkin)) + return DefaultSkin; return null; } @@ -254,11 +250,11 @@ namespace osu.Game.Skinning // TODO: we also want to return a DefaultLegacySkin here if the current *beatmap* is providing any skinned elements. // When attempting to address this, we may want to move the full DefaultLegacySkin fallback logic to within Player itself (to better allow // for beatmap skin visibility). - if (CurrentSkin.Value is LegacySkin && lookupFunction(defaultLegacySkin) is T legacySourced) + if (CurrentSkin.Value is LegacySkin && lookupFunction(DefaultLegacySkin) is T legacySourced) return legacySourced; // Finally fall back to the (non-legacy) default. - return lookupFunction(defaultSkin); + return lookupFunction(DefaultSkin); } #region IResourceStorageProvider From debd359d2e5d36361757d37188b4498c530bdee0 Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Fri, 11 Jun 2021 14:50:21 +0900 Subject: [PATCH 20/54] Update xmldoc --- osu.Game/Skinning/SkinManager.cs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/osu.Game/Skinning/SkinManager.cs b/osu.Game/Skinning/SkinManager.cs index ffdbadf54c..660f44772c 100644 --- a/osu.Game/Skinning/SkinManager.cs +++ b/osu.Game/Skinning/SkinManager.cs @@ -49,12 +49,12 @@ namespace osu.Game.Skinning protected override string ImportFromStablePath => "Skins"; /// - /// An providing the resources of the default skin. + /// The default skin. /// public Skin DefaultSkin { get; } /// - /// An providing the resources of the default legacy skin. + /// The default legacy skin. /// public Skin DefaultLegacySkin { get; } From a985e3b8d3562e357d3f431482794999e40948b9 Mon Sep 17 00:00:00 2001 From: Salman Ahmed Date: Fri, 11 Jun 2021 11:25:07 +0300 Subject: [PATCH 21/54] Apply documentation settings for better readability Co-authored-by: Dean Herbert Co-authored-by: Dan Balasescu --- osu.Game/Skinning/SkinProvidingContainer.cs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/osu.Game/Skinning/SkinProvidingContainer.cs b/osu.Game/Skinning/SkinProvidingContainer.cs index 078c666472..0f2d8e2c22 100644 --- a/osu.Game/Skinning/SkinProvidingContainer.cs +++ b/osu.Game/Skinning/SkinProvidingContainer.cs @@ -24,7 +24,7 @@ namespace osu.Game.Skinning public event Action SourceChanged; /// - /// The list of skins provided by this . + /// Skins which should be exposed by this container, in order of lookup precedence. /// protected readonly BindableList SkinSources = new BindableList(); @@ -44,7 +44,7 @@ namespace osu.Game.Skinning protected virtual bool AllowColourLookup => true; /// - /// Constructs a new with a single skin added to the protected list. + /// Constructs a new initialised with a single skin source. /// public SkinProvidingContainer(ISkin skin) : this() @@ -54,7 +54,7 @@ namespace osu.Game.Skinning /// /// Constructs a new with no sources. - /// Up to the implementation for adding to the list. + /// Implementations can add or change sources through the list. /// protected SkinProvidingContainer() { From 813285275307871b77479effcf17ce9d797c845e Mon Sep 17 00:00:00 2001 From: Salman Ahmed Date: Fri, 11 Jun 2021 11:34:22 +0300 Subject: [PATCH 22/54] Add other affectable change action cases --- osu.Game/Skinning/SkinProvidingContainer.cs | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/osu.Game/Skinning/SkinProvidingContainer.cs b/osu.Game/Skinning/SkinProvidingContainer.cs index 0f2d8e2c22..ab33a66265 100644 --- a/osu.Game/Skinning/SkinProvidingContainer.cs +++ b/osu.Game/Skinning/SkinProvidingContainer.cs @@ -72,11 +72,21 @@ namespace osu.Game.Skinning break; + case NotifyCollectionChangedAction.Reset: case NotifyCollectionChangedAction.Remove: foreach (var source in args.OldItems.Cast().OfType()) source.SourceChanged -= OnSourceChanged; break; + + case NotifyCollectionChangedAction.Replace: + foreach (var source in args.OldItems.Cast().OfType()) + source.SourceChanged -= OnSourceChanged; + + foreach (var source in args.NewItems.Cast().OfType()) + source.SourceChanged += OnSourceChanged; + + break; } }), true); } From 2e01e611775c6a143d7363a51a9f03a4ef9a59d6 Mon Sep 17 00:00:00 2001 From: Salman Ahmed Date: Fri, 11 Jun 2021 11:46:29 +0300 Subject: [PATCH 23/54] Move TODO comment to correct location --- osu.Game/Skinning/RulesetSkinProvidingContainer.cs | 1 + osu.Game/Skinning/SkinManager.cs | 3 --- 2 files changed, 1 insertion(+), 3 deletions(-) diff --git a/osu.Game/Skinning/RulesetSkinProvidingContainer.cs b/osu.Game/Skinning/RulesetSkinProvidingContainer.cs index 8087043230..c57522726d 100644 --- a/osu.Game/Skinning/RulesetSkinProvidingContainer.cs +++ b/osu.Game/Skinning/RulesetSkinProvidingContainer.cs @@ -56,6 +56,7 @@ namespace osu.Game.Skinning SkinSources.Add(Ruleset.CreateLegacySkinProvider(skinManager.CurrentSkin.Value, Beatmap)); + // TODO: we also want to return a DefaultLegacySkin here if the current *beatmap* is providing any skinned elements. if (skinManager.CurrentSkin.Value is LegacySkin) SkinSources.Add(Ruleset.CreateLegacySkinProvider(skinManager.DefaultLegacySkin, Beatmap)); diff --git a/osu.Game/Skinning/SkinManager.cs b/osu.Game/Skinning/SkinManager.cs index 660f44772c..7acc52809f 100644 --- a/osu.Game/Skinning/SkinManager.cs +++ b/osu.Game/Skinning/SkinManager.cs @@ -247,9 +247,6 @@ namespace osu.Game.Skinning if (lookupFunction(CurrentSkin.Value) is T skinSourced) return skinSourced; - // TODO: we also want to return a DefaultLegacySkin here if the current *beatmap* is providing any skinned elements. - // When attempting to address this, we may want to move the full DefaultLegacySkin fallback logic to within Player itself (to better allow - // for beatmap skin visibility). if (CurrentSkin.Value is LegacySkin && lookupFunction(DefaultLegacySkin) is T legacySourced) return legacySourced; From 9e16359f18a7e90049a1dacb80bad9ff2fe8d130 Mon Sep 17 00:00:00 2001 From: Salman Ahmed Date: Fri, 11 Jun 2021 12:29:28 +0300 Subject: [PATCH 24/54] Refactor disallowing in `SkinProvidingContainer` to become per source Fixes `FindProvider` becoming completely broken, because of no way to perform the checks on one skin source. --- osu.Game/Skinning/SkinProvidingContainer.cs | 188 ++++++++++---------- 1 file changed, 96 insertions(+), 92 deletions(-) diff --git a/osu.Game/Skinning/SkinProvidingContainer.cs b/osu.Game/Skinning/SkinProvidingContainer.cs index ab33a66265..c9bb3a6ec4 100644 --- a/osu.Game/Skinning/SkinProvidingContainer.cs +++ b/osu.Game/Skinning/SkinProvidingContainer.cs @@ -2,6 +2,7 @@ // See the LICENCE file in the repository root for full licence text. using System; +using System.Collections.Generic; using System.Collections.Specialized; using System.Linq; using JetBrains.Annotations; @@ -28,11 +29,15 @@ namespace osu.Game.Skinning /// protected readonly BindableList SkinSources = new BindableList(); + /// + /// A dictionary mapping each from the + /// to one that performs the "allow lookup" checks before proceeding with a lookup. + /// + private readonly Dictionary disableableSkinSources = new Dictionary(); + [CanBeNull] private ISkinSource fallbackSource; - private readonly NoFallbackProxy noFallbackLookupProxy; - protected virtual bool AllowDrawableLookup(ISkinComponent component) => true; protected virtual bool AllowTextureLookup(string componentName) => true; @@ -60,31 +65,49 @@ namespace osu.Game.Skinning { RelativeSizeAxes = Axes.Both; - noFallbackLookupProxy = new NoFallbackProxy(this); - SkinSources.BindCollectionChanged(((_, args) => { switch (args.Action) { case NotifyCollectionChangedAction.Add: - foreach (var source in args.NewItems.Cast().OfType()) - source.SourceChanged += OnSourceChanged; + foreach (var skin in args.NewItems.Cast()) + { + disableableSkinSources.Add(skin, new DisableableSkinSource(skin, this)); + + if (skin is ISkinSource source) + source.SourceChanged += OnSourceChanged; + } break; case NotifyCollectionChangedAction.Reset: case NotifyCollectionChangedAction.Remove: - foreach (var source in args.OldItems.Cast().OfType()) - source.SourceChanged -= OnSourceChanged; + foreach (var skin in args.OldItems.Cast()) + { + disableableSkinSources.Remove(skin); + + if (skin is ISkinSource source) + source.SourceChanged -= OnSourceChanged; + } break; case NotifyCollectionChangedAction.Replace: - foreach (var source in args.OldItems.Cast().OfType()) - source.SourceChanged -= OnSourceChanged; + foreach (var skin in args.OldItems.Cast()) + { + disableableSkinSources.Remove(skin); - foreach (var source in args.NewItems.Cast().OfType()) - source.SourceChanged += OnSourceChanged; + if (skin is ISkinSource source) + source.SourceChanged -= OnSourceChanged; + } + + foreach (var skin in args.NewItems.Cast()) + { + disableableSkinSources.Add(skin, new DisableableSkinSource(skin, this)); + + if (skin is ISkinSource source) + source.SourceChanged += OnSourceChanged; + } break; } @@ -95,8 +118,7 @@ namespace osu.Game.Skinning { foreach (var skin in SkinSources) { - // a proxy must be used here to correctly pass through the "Allow" checks without implicitly falling back to the fallbackSource. - if (lookupFunction(noFallbackLookupProxy)) + if (lookupFunction(disableableSkinSources[skin])) return skin; } @@ -104,94 +126,50 @@ namespace osu.Game.Skinning } public Drawable GetDrawableComponent(ISkinComponent component) - => GetDrawableComponent(component, true); - - public Drawable GetDrawableComponent(ISkinComponent component, bool fallback) { - if (AllowDrawableLookup(component)) + foreach (var skin in SkinSources) { - foreach (var skin in SkinSources) - { - Drawable sourceDrawable; - if ((sourceDrawable = skin?.GetDrawableComponent(component)) != null) - return sourceDrawable; - } + Drawable sourceDrawable; + if ((sourceDrawable = disableableSkinSources[skin]?.GetDrawableComponent(component)) != null) + return sourceDrawable; } - if (!fallback) - return null; - return fallbackSource?.GetDrawableComponent(component); } public Texture GetTexture(string componentName, WrapMode wrapModeS, WrapMode wrapModeT) - => GetTexture(componentName, wrapModeS, wrapModeT, true); - - public Texture GetTexture(string componentName, WrapMode wrapModeS, WrapMode wrapModeT, bool fallback) { - if (AllowTextureLookup(componentName)) + foreach (var skin in SkinSources) { - foreach (var skin in SkinSources) - { - Texture sourceTexture; - if ((sourceTexture = skin?.GetTexture(componentName, wrapModeS, wrapModeT)) != null) - return sourceTexture; - } + Texture sourceTexture; + if ((sourceTexture = disableableSkinSources[skin]?.GetTexture(componentName, wrapModeS, wrapModeT)) != null) + return sourceTexture; } - if (!fallback) - return null; - return fallbackSource?.GetTexture(componentName, wrapModeS, wrapModeT); } public ISample GetSample(ISampleInfo sampleInfo) - => GetSample(sampleInfo, true); - - public ISample GetSample(ISampleInfo sampleInfo, bool fallback) { - if (AllowSampleLookup(sampleInfo)) + foreach (var skin in SkinSources) { - foreach (var skin in SkinSources) - { - ISample sourceSample; - if ((sourceSample = skin?.GetSample(sampleInfo)) != null) - return sourceSample; - } + ISample sourceSample; + if ((sourceSample = disableableSkinSources[skin]?.GetSample(sampleInfo)) != null) + return sourceSample; } - if (!fallback) - return null; - return fallbackSource?.GetSample(sampleInfo); } public IBindable GetConfig(TLookup lookup) - => GetConfig(lookup, true); - - public IBindable GetConfig(TLookup lookup, bool fallback) { - if (lookup is GlobalSkinColours || lookup is SkinCustomColourLookup) - return lookupWithFallback(lookup, AllowColourLookup, fallback); - - return lookupWithFallback(lookup, AllowConfigurationLookup, fallback); - } - - private IBindable lookupWithFallback(TLookup lookup, bool canUseSkinLookup, bool canUseFallback) - { - if (canUseSkinLookup) + foreach (var skin in SkinSources) { - foreach (var skin in SkinSources) - { - IBindable bindable; - if ((bindable = skin?.GetConfig(lookup)) != null) - return bindable; - } + IBindable bindable; + if ((bindable = disableableSkinSources[skin]?.GetConfig(lookup)) != null) + return bindable; } - if (!canUseFallback) - return null; - return fallbackSource?.GetConfig(lookup); } @@ -224,35 +202,61 @@ namespace osu.Game.Skinning source.SourceChanged -= OnSourceChanged; } - private class NoFallbackProxy : ISkinSource + private class DisableableSkinSource : ISkin { + private readonly ISkin skin; private readonly SkinProvidingContainer provider; - public NoFallbackProxy(SkinProvidingContainer provider) + public DisableableSkinSource(ISkin skin, SkinProvidingContainer provider) { + this.skin = skin; this.provider = provider; } public Drawable GetDrawableComponent(ISkinComponent component) - => provider.GetDrawableComponent(component, false); - - public Texture GetTexture(string componentName, WrapMode wrapModeS, WrapMode wrapModeT) - => provider.GetTexture(componentName, wrapModeS, wrapModeT, false); - - public ISample GetSample(ISampleInfo sampleInfo) - => provider.GetSample(sampleInfo, false); - - public IBindable GetConfig(TLookup lookup) - => provider.GetConfig(lookup, false); - - public event Action SourceChanged { - add => provider.SourceChanged += value; - remove => provider.SourceChanged -= value; + if (provider.AllowDrawableLookup(component)) + return skin.GetDrawableComponent(component); + + return null; } - public ISkin FindProvider(Func lookupFunction) => - provider.FindProvider(lookupFunction); + public Texture GetTexture(string componentName, WrapMode wrapModeS, WrapMode wrapModeT) + { + if (provider.AllowTextureLookup(componentName)) + return skin.GetTexture(componentName, wrapModeS, wrapModeT); + + return null; + } + + public ISample GetSample(ISampleInfo sampleInfo) + { + if (provider.AllowSampleLookup(sampleInfo)) + return skin.GetSample(sampleInfo); + + return null; + } + + public IBindable GetConfig(TLookup lookup) + { + switch (lookup) + { + case GlobalSkinColours _: + case SkinCustomColourLookup _: + if (provider.AllowColourLookup) + return skin.GetConfig(lookup); + + break; + + default: + if (provider.AllowConfigurationLookup) + return skin.GetConfig(lookup); + + break; + } + + return null; + } } } } From e59beffc4e7057254c4790a3b9112c5e6c09f3ff Mon Sep 17 00:00:00 2001 From: Salman Ahmed Date: Fri, 11 Jun 2021 12:44:25 +0300 Subject: [PATCH 25/54] Forward all base transformer lookup methods to `Skin` --- osu.Game/Skinning/LegacySkinTransformer.cs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/osu.Game/Skinning/LegacySkinTransformer.cs b/osu.Game/Skinning/LegacySkinTransformer.cs index fedd63c7de..92b7a04dee 100644 --- a/osu.Game/Skinning/LegacySkinTransformer.cs +++ b/osu.Game/Skinning/LegacySkinTransformer.cs @@ -30,7 +30,7 @@ namespace osu.Game.Skinning Skin = skin ?? throw new ArgumentNullException(nameof(skin)); } - public abstract Drawable GetDrawableComponent(ISkinComponent component); + public virtual Drawable GetDrawableComponent(ISkinComponent component) => Skin.GetDrawableComponent(component); public Texture GetTexture(string componentName) => GetTexture(componentName, default, default); @@ -49,6 +49,6 @@ namespace osu.Game.Skinning return Skin.GetSample(sampleInfo); } - public abstract IBindable GetConfig(TLookup lookup); + public virtual IBindable GetConfig(TLookup lookup) => Skin.GetConfig(lookup); } } From fbb856d84bfdfb2e5c4c5ee3fbfde90302864000 Mon Sep 17 00:00:00 2001 From: Salman Ahmed Date: Fri, 11 Jun 2021 12:44:44 +0300 Subject: [PATCH 26/54] Call `base` when overriding lookup methods Rather than arbitrarily accessing `Skin` here and there. --- .../Skinning/Legacy/CatchLegacySkinTransformer.cs | 8 ++++---- .../Skinning/Legacy/ManiaLegacySkinTransformer.cs | 12 ++++++------ .../Skinning/Legacy/OsuLegacySkinTransformer.cs | 12 ++++++------ .../Skinning/Legacy/TaikoLegacySkinTransformer.cs | 12 +++--------- 4 files changed, 19 insertions(+), 25 deletions(-) diff --git a/osu.Game.Rulesets.Catch/Skinning/Legacy/CatchLegacySkinTransformer.cs b/osu.Game.Rulesets.Catch/Skinning/Legacy/CatchLegacySkinTransformer.cs index a5a1d1504f..287ed1b4c7 100644 --- a/osu.Game.Rulesets.Catch/Skinning/Legacy/CatchLegacySkinTransformer.cs +++ b/osu.Game.Rulesets.Catch/Skinning/Legacy/CatchLegacySkinTransformer.cs @@ -29,7 +29,7 @@ namespace osu.Game.Rulesets.Catch.Skinning.Legacy switch (targetComponent.Target) { case SkinnableTarget.MainHUDComponents: - var components = Skin.GetDrawableComponent(component) as SkinnableTargetComponentsContainer; + var components = base.GetDrawableComponent(component) as SkinnableTargetComponentsContainer; if (providesComboCounter && components != null) { @@ -89,7 +89,7 @@ namespace osu.Game.Rulesets.Catch.Skinning.Legacy } } - return Skin.GetDrawableComponent(component); + return base.GetDrawableComponent(component); } public override IBindable GetConfig(TLookup lookup) @@ -97,7 +97,7 @@ namespace osu.Game.Rulesets.Catch.Skinning.Legacy switch (lookup) { case CatchSkinColour colour: - var result = (Bindable)Skin.GetConfig(new SkinCustomColourLookup(colour)); + var result = (Bindable)base.GetConfig(new SkinCustomColourLookup(colour)); if (result == null) return null; @@ -105,7 +105,7 @@ namespace osu.Game.Rulesets.Catch.Skinning.Legacy return (IBindable)result; } - return Skin.GetConfig(lookup); + return base.GetConfig(lookup); } } } diff --git a/osu.Game.Rulesets.Mania/Skinning/Legacy/ManiaLegacySkinTransformer.cs b/osu.Game.Rulesets.Mania/Skinning/Legacy/ManiaLegacySkinTransformer.cs index 7d4d303bc9..814a737034 100644 --- a/osu.Game.Rulesets.Mania/Skinning/Legacy/ManiaLegacySkinTransformer.cs +++ b/osu.Game.Rulesets.Mania/Skinning/Legacy/ManiaLegacySkinTransformer.cs @@ -63,11 +63,11 @@ namespace osu.Game.Rulesets.Mania.Skinning.Legacy { this.beatmap = (ManiaBeatmap)beatmap; - isLegacySkin = new Lazy(() => skin.GetConfig(LegacySkinConfiguration.LegacySetting.Version) != null); + isLegacySkin = new Lazy(() => GetConfig(LegacySkinConfiguration.LegacySetting.Version) != null); hasKeyTexture = new Lazy(() => { var keyImage = this.GetManiaSkinConfig(LegacyManiaSkinConfigurationLookups.KeyImage, 0)?.Value ?? "mania-key1"; - return skin.GetAnimation(keyImage, true, true) != null; + return this.GetAnimation(keyImage, true, true) != null; }); } @@ -121,7 +121,7 @@ namespace osu.Game.Rulesets.Mania.Skinning.Legacy break; } - return Skin.GetDrawableComponent(component); + return base.GetDrawableComponent(component); } private Drawable getResult(HitResult result) @@ -142,15 +142,15 @@ namespace osu.Game.Rulesets.Mania.Skinning.Legacy if (sampleInfo is ConvertHitObjectParser.LegacyHitSampleInfo legacySample && legacySample.IsLayered) return new SampleVirtual(); - return Skin.GetSample(sampleInfo); + return base.GetSample(sampleInfo); } public override IBindable GetConfig(TLookup lookup) { if (lookup is ManiaSkinConfigurationLookup maniaLookup) - return Skin.GetConfig(new LegacyManiaSkinConfigurationLookup(beatmap.TotalColumns, maniaLookup.Lookup, maniaLookup.TargetColumn)); + return base.GetConfig(new LegacyManiaSkinConfigurationLookup(beatmap.TotalColumns, maniaLookup.Lookup, maniaLookup.TargetColumn)); - return Skin.GetConfig(lookup); + return base.GetConfig(lookup); } } } diff --git a/osu.Game.Rulesets.Osu/Skinning/Legacy/OsuLegacySkinTransformer.cs b/osu.Game.Rulesets.Osu/Skinning/Legacy/OsuLegacySkinTransformer.cs index 3ad3b7d30b..41b0a88f11 100644 --- a/osu.Game.Rulesets.Osu/Skinning/Legacy/OsuLegacySkinTransformer.cs +++ b/osu.Game.Rulesets.Osu/Skinning/Legacy/OsuLegacySkinTransformer.cs @@ -23,7 +23,7 @@ namespace osu.Game.Rulesets.Osu.Skinning.Legacy public OsuLegacySkinTransformer(ISkin skin) : base(skin) { - hasHitCircle = new Lazy(() => Skin.GetTexture("hitcircle") != null); + hasHitCircle = new Lazy(() => GetTexture("hitcircle") != null); } public override Drawable GetDrawableComponent(ISkinComponent component) @@ -111,7 +111,7 @@ namespace osu.Game.Rulesets.Osu.Skinning.Legacy } } - return Skin.GetDrawableComponent(component); + return base.GetDrawableComponent(component); } public override IBindable GetConfig(TLookup lookup) @@ -119,7 +119,7 @@ namespace osu.Game.Rulesets.Osu.Skinning.Legacy switch (lookup) { case OsuSkinColour colour: - return Skin.GetConfig(new SkinCustomColourLookup(colour)); + return base.GetConfig(new SkinCustomColourLookup(colour)); case OsuSkinConfiguration osuLookup: switch (osuLookup) @@ -133,14 +133,14 @@ namespace osu.Game.Rulesets.Osu.Skinning.Legacy case OsuSkinConfiguration.HitCircleOverlayAboveNumber: // See https://osu.ppy.sh/help/wiki/Skinning/skin.ini#%5Bgeneral%5D // HitCircleOverlayAboveNumer (with typo) should still be supported for now. - return Skin.GetConfig(OsuSkinConfiguration.HitCircleOverlayAboveNumber) ?? - Skin.GetConfig(OsuSkinConfiguration.HitCircleOverlayAboveNumer); + return base.GetConfig(OsuSkinConfiguration.HitCircleOverlayAboveNumber) ?? + base.GetConfig(OsuSkinConfiguration.HitCircleOverlayAboveNumer); } break; } - return Skin.GetConfig(lookup); + return base.GetConfig(lookup); } } } diff --git a/osu.Game.Rulesets.Taiko/Skinning/Legacy/TaikoLegacySkinTransformer.cs b/osu.Game.Rulesets.Taiko/Skinning/Legacy/TaikoLegacySkinTransformer.cs index 0122f9a1cd..a3ecbbc436 100644 --- a/osu.Game.Rulesets.Taiko/Skinning/Legacy/TaikoLegacySkinTransformer.cs +++ b/osu.Game.Rulesets.Taiko/Skinning/Legacy/TaikoLegacySkinTransformer.cs @@ -4,7 +4,6 @@ using System; using System.Collections.Generic; using osu.Framework.Audio.Sample; -using osu.Framework.Bindables; using osu.Framework.Graphics; using osu.Game.Audio; using osu.Game.Rulesets.Scoring; @@ -20,7 +19,7 @@ namespace osu.Game.Rulesets.Taiko.Skinning.Legacy public TaikoLegacySkinTransformer(ISkin skin) : base(skin) { - hasExplosion = new Lazy(() => Skin.GetTexture(getHitName(TaikoSkinComponents.TaikoExplosionGreat)) != null); + hasExplosion = new Lazy(() => GetTexture(getHitName(TaikoSkinComponents.TaikoExplosionGreat)) != null); } public override Drawable GetDrawableComponent(ISkinComponent component) @@ -50,7 +49,6 @@ namespace osu.Game.Rulesets.Taiko.Skinning.Legacy case TaikoSkinComponents.CentreHit: case TaikoSkinComponents.RimHit: - if (GetTexture("taikohitcircle") != null) return new LegacyHit(taikoComponent.Component); @@ -85,7 +83,6 @@ namespace osu.Game.Rulesets.Taiko.Skinning.Legacy return null; case TaikoSkinComponents.TaikoExplosionMiss: - var missSprite = this.GetAnimation(getHitName(taikoComponent.Component), true, false); if (missSprite != null) return new LegacyHitExplosion(missSprite); @@ -94,7 +91,6 @@ namespace osu.Game.Rulesets.Taiko.Skinning.Legacy case TaikoSkinComponents.TaikoExplosionOk: case TaikoSkinComponents.TaikoExplosionGreat: - var hitName = getHitName(taikoComponent.Component); var hitSprite = this.GetAnimation(hitName, true, false); @@ -126,7 +122,7 @@ namespace osu.Game.Rulesets.Taiko.Skinning.Legacy } } - return Skin.GetDrawableComponent(component); + return base.GetDrawableComponent(component); } private string getHitName(TaikoSkinComponents component) @@ -149,13 +145,11 @@ namespace osu.Game.Rulesets.Taiko.Skinning.Legacy public override ISample GetSample(ISampleInfo sampleInfo) { if (sampleInfo is HitSampleInfo hitSampleInfo) - return Skin.GetSample(new LegacyTaikoSampleInfo(hitSampleInfo)); + return base.GetSample(new LegacyTaikoSampleInfo(hitSampleInfo)); return base.GetSample(sampleInfo); } - public override IBindable GetConfig(TLookup lookup) => Skin.GetConfig(lookup); - private class LegacyTaikoSampleInfo : HitSampleInfo { public LegacyTaikoSampleInfo(HitSampleInfo sampleInfo) From f20146d446d492ef9a1fa2036db0bc2dc27108b4 Mon Sep 17 00:00:00 2001 From: Salman Ahmed Date: Fri, 11 Jun 2021 12:58:38 +0300 Subject: [PATCH 27/54] Fix potentially adding null skin sources --- osu.Game/Skinning/SkinProvidingContainer.cs | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/osu.Game/Skinning/SkinProvidingContainer.cs b/osu.Game/Skinning/SkinProvidingContainer.cs index c9bb3a6ec4..315571e79b 100644 --- a/osu.Game/Skinning/SkinProvidingContainer.cs +++ b/osu.Game/Skinning/SkinProvidingContainer.cs @@ -51,10 +51,11 @@ namespace osu.Game.Skinning /// /// Constructs a new initialised with a single skin source. /// - public SkinProvidingContainer(ISkin skin) + public SkinProvidingContainer([CanBeNull] ISkin skin) : this() { - SkinSources.Add(skin); + if (skin != null) + SkinSources.Add(skin); } /// From 108a3deb27382b833f297abf79e63a53e8da1b4d Mon Sep 17 00:00:00 2001 From: Salman Ahmed Date: Fri, 11 Jun 2021 16:26:53 +0300 Subject: [PATCH 28/54] Also handle null `Ruleset.CreateLegacySkinProvider` values Let's just go this way for now, maybe it's a better choice to always create transformers and disallow null, but it's too much work and out of scope at this point --- .../Skinning/RulesetSkinProvidingContainer.cs | 20 +++++++++++++++---- 1 file changed, 16 insertions(+), 4 deletions(-) diff --git a/osu.Game/Skinning/RulesetSkinProvidingContainer.cs b/osu.Game/Skinning/RulesetSkinProvidingContainer.cs index c57522726d..b6a3bd7cda 100644 --- a/osu.Game/Skinning/RulesetSkinProvidingContainer.cs +++ b/osu.Game/Skinning/RulesetSkinProvidingContainer.cs @@ -26,7 +26,7 @@ namespace osu.Game.Skinning Ruleset = ruleset; Beatmap = beatmap; - InternalChild = new BeatmapSkinProvidingContainer(beatmapSkin == null ? null : ruleset.CreateLegacySkinProvider(beatmapSkin, beatmap)) + InternalChild = new BeatmapSkinProvidingContainer(GetRulesetTransformedSkin(beatmapSkin)) { Child = Content = new Container { @@ -54,13 +54,25 @@ namespace osu.Game.Skinning { SkinSources.Clear(); - SkinSources.Add(Ruleset.CreateLegacySkinProvider(skinManager.CurrentSkin.Value, Beatmap)); + SkinSources.Add(GetRulesetTransformedSkin(skinManager.CurrentSkin.Value)); // TODO: we also want to return a DefaultLegacySkin here if the current *beatmap* is providing any skinned elements. if (skinManager.CurrentSkin.Value is LegacySkin) - SkinSources.Add(Ruleset.CreateLegacySkinProvider(skinManager.DefaultLegacySkin, Beatmap)); + SkinSources.Add(GetRulesetTransformedSkin(skinManager.DefaultLegacySkin)); - SkinSources.Add(Ruleset.CreateLegacySkinProvider(skinManager.DefaultSkin, Beatmap)); + SkinSources.Add(GetRulesetTransformedSkin(skinManager.DefaultSkin)); + } + + protected ISkin GetRulesetTransformedSkin(ISkin skin) + { + if (skin == null) + return null; + + var rulesetTransformed = Ruleset.CreateLegacySkinProvider(skin, Beatmap); + if (rulesetTransformed != null) + return rulesetTransformed; + + return skin; } } } From d6d87e1975b3cd6145328e2849f358606acc17c9 Mon Sep 17 00:00:00 2001 From: Salman Ahmed Date: Fri, 11 Jun 2021 17:35:32 +0300 Subject: [PATCH 29/54] Move collection change bind to LoadComplete Best practice anyways --- osu.Game/Skinning/SkinProvidingContainer.cs | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/osu.Game/Skinning/SkinProvidingContainer.cs b/osu.Game/Skinning/SkinProvidingContainer.cs index 315571e79b..ac1b4d0395 100644 --- a/osu.Game/Skinning/SkinProvidingContainer.cs +++ b/osu.Game/Skinning/SkinProvidingContainer.cs @@ -65,6 +65,11 @@ namespace osu.Game.Skinning protected SkinProvidingContainer() { RelativeSizeAxes = Axes.Both; + } + + protected override void LoadComplete() + { + base.LoadComplete(); SkinSources.BindCollectionChanged(((_, args) => { From b6947c25ec5d5c3c40e50cf4ad7e3450009a8a68 Mon Sep 17 00:00:00 2001 From: Salman Ahmed Date: Fri, 11 Jun 2021 17:55:07 +0300 Subject: [PATCH 30/54] Fix potentially adding the same skin multiple times --- osu.Game/Skinning/RulesetSkinProvidingContainer.cs | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/osu.Game/Skinning/RulesetSkinProvidingContainer.cs b/osu.Game/Skinning/RulesetSkinProvidingContainer.cs index b6a3bd7cda..8a27899e89 100644 --- a/osu.Game/Skinning/RulesetSkinProvidingContainer.cs +++ b/osu.Game/Skinning/RulesetSkinProvidingContainer.cs @@ -57,10 +57,11 @@ namespace osu.Game.Skinning SkinSources.Add(GetRulesetTransformedSkin(skinManager.CurrentSkin.Value)); // TODO: we also want to return a DefaultLegacySkin here if the current *beatmap* is providing any skinned elements. - if (skinManager.CurrentSkin.Value is LegacySkin) + if (skinManager.CurrentSkin.Value is LegacySkin && skinManager.CurrentSkin.Value != skinManager.DefaultLegacySkin) SkinSources.Add(GetRulesetTransformedSkin(skinManager.DefaultLegacySkin)); - SkinSources.Add(GetRulesetTransformedSkin(skinManager.DefaultSkin)); + if (skinManager.CurrentSkin.Value != skinManager.DefaultSkin) + SkinSources.Add(GetRulesetTransformedSkin(skinManager.DefaultSkin)); } protected ISkin GetRulesetTransformedSkin(ISkin skin) From 8de0d33c5a36b8b9e9af4c035eb9a21caa25d3cf Mon Sep 17 00:00:00 2001 From: Salman Ahmed Date: Fri, 11 Jun 2021 17:59:28 +0300 Subject: [PATCH 31/54] Revert "Move collection change bind to LoadComplete" This reverts commit d6d87e1975b3cd6145328e2849f358606acc17c9. Actually that broke things due to the "disableable" instances not added early enough, revert for now. --- osu.Game/Skinning/SkinProvidingContainer.cs | 5 ----- 1 file changed, 5 deletions(-) diff --git a/osu.Game/Skinning/SkinProvidingContainer.cs b/osu.Game/Skinning/SkinProvidingContainer.cs index ac1b4d0395..315571e79b 100644 --- a/osu.Game/Skinning/SkinProvidingContainer.cs +++ b/osu.Game/Skinning/SkinProvidingContainer.cs @@ -65,11 +65,6 @@ namespace osu.Game.Skinning protected SkinProvidingContainer() { RelativeSizeAxes = Axes.Both; - } - - protected override void LoadComplete() - { - base.LoadComplete(); SkinSources.BindCollectionChanged(((_, args) => { From 521077b7148eb896792ed5c1a451074ab8957746 Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Wed, 16 Jun 2021 17:44:18 +0900 Subject: [PATCH 32/54] Make `getRulesetTransformedSkin` private --- osu.Game/Skinning/RulesetSkinProvidingContainer.cs | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/osu.Game/Skinning/RulesetSkinProvidingContainer.cs b/osu.Game/Skinning/RulesetSkinProvidingContainer.cs index 8a27899e89..1953bd499b 100644 --- a/osu.Game/Skinning/RulesetSkinProvidingContainer.cs +++ b/osu.Game/Skinning/RulesetSkinProvidingContainer.cs @@ -26,7 +26,7 @@ namespace osu.Game.Skinning Ruleset = ruleset; Beatmap = beatmap; - InternalChild = new BeatmapSkinProvidingContainer(GetRulesetTransformedSkin(beatmapSkin)) + InternalChild = new BeatmapSkinProvidingContainer(getRulesetTransformedSkin(beatmapSkin)) { Child = Content = new Container { @@ -54,17 +54,17 @@ namespace osu.Game.Skinning { SkinSources.Clear(); - SkinSources.Add(GetRulesetTransformedSkin(skinManager.CurrentSkin.Value)); + SkinSources.Add(getRulesetTransformedSkin(skinManager.CurrentSkin.Value)); // TODO: we also want to return a DefaultLegacySkin here if the current *beatmap* is providing any skinned elements. if (skinManager.CurrentSkin.Value is LegacySkin && skinManager.CurrentSkin.Value != skinManager.DefaultLegacySkin) - SkinSources.Add(GetRulesetTransformedSkin(skinManager.DefaultLegacySkin)); + SkinSources.Add(getRulesetTransformedSkin(skinManager.DefaultLegacySkin)); if (skinManager.CurrentSkin.Value != skinManager.DefaultSkin) - SkinSources.Add(GetRulesetTransformedSkin(skinManager.DefaultSkin)); + SkinSources.Add(getRulesetTransformedSkin(skinManager.DefaultSkin)); } - protected ISkin GetRulesetTransformedSkin(ISkin skin) + private ISkin getRulesetTransformedSkin(ISkin skin) { if (skin == null) return null; From 5ebf570ec4cdde860928b5c42da17695e3185d49 Mon Sep 17 00:00:00 2001 From: Salman Ahmed Date: Wed, 16 Jun 2021 16:48:30 +0300 Subject: [PATCH 33/54] Revert `GetRulesetTransformedSkin` accessibility change This reverts commit 521077b7148eb896792ed5c1a451074ab8957746. Forgot to do it when I made this `protected`, but subclasses in test scenes require this. --- osu.Game/Skinning/RulesetSkinProvidingContainer.cs | 10 +++++----- osu.Game/Tests/Visual/TestPlayer.cs | 2 +- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/osu.Game/Skinning/RulesetSkinProvidingContainer.cs b/osu.Game/Skinning/RulesetSkinProvidingContainer.cs index 1953bd499b..8a27899e89 100644 --- a/osu.Game/Skinning/RulesetSkinProvidingContainer.cs +++ b/osu.Game/Skinning/RulesetSkinProvidingContainer.cs @@ -26,7 +26,7 @@ namespace osu.Game.Skinning Ruleset = ruleset; Beatmap = beatmap; - InternalChild = new BeatmapSkinProvidingContainer(getRulesetTransformedSkin(beatmapSkin)) + InternalChild = new BeatmapSkinProvidingContainer(GetRulesetTransformedSkin(beatmapSkin)) { Child = Content = new Container { @@ -54,17 +54,17 @@ namespace osu.Game.Skinning { SkinSources.Clear(); - SkinSources.Add(getRulesetTransformedSkin(skinManager.CurrentSkin.Value)); + SkinSources.Add(GetRulesetTransformedSkin(skinManager.CurrentSkin.Value)); // TODO: we also want to return a DefaultLegacySkin here if the current *beatmap* is providing any skinned elements. if (skinManager.CurrentSkin.Value is LegacySkin && skinManager.CurrentSkin.Value != skinManager.DefaultLegacySkin) - SkinSources.Add(getRulesetTransformedSkin(skinManager.DefaultLegacySkin)); + SkinSources.Add(GetRulesetTransformedSkin(skinManager.DefaultLegacySkin)); if (skinManager.CurrentSkin.Value != skinManager.DefaultSkin) - SkinSources.Add(getRulesetTransformedSkin(skinManager.DefaultSkin)); + SkinSources.Add(GetRulesetTransformedSkin(skinManager.DefaultSkin)); } - private ISkin getRulesetTransformedSkin(ISkin skin) + protected ISkin GetRulesetTransformedSkin(ISkin skin) { if (skin == null) return null; diff --git a/osu.Game/Tests/Visual/TestPlayer.cs b/osu.Game/Tests/Visual/TestPlayer.cs index eecf8a2f6e..e1431b0658 100644 --- a/osu.Game/Tests/Visual/TestPlayer.cs +++ b/osu.Game/Tests/Visual/TestPlayer.cs @@ -99,7 +99,7 @@ namespace osu.Game.Tests.Visual base.UpdateSkins(); if (skin != null) - SkinSources.Insert(0, Ruleset.CreateLegacySkinProvider(skin, Beatmap)); + SkinSources.Insert(0, GetRulesetTransformedSkin(skin)); } } } From 52ddf08532fd4e31d042ec8a06d226814b769c52 Mon Sep 17 00:00:00 2001 From: Salman Ahmed Date: Wed, 16 Jun 2021 16:51:20 +0300 Subject: [PATCH 34/54] Consider not adding legacy skin transformers to non-legacy skins --- osu.Game/Skinning/RulesetSkinProvidingContainer.cs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/osu.Game/Skinning/RulesetSkinProvidingContainer.cs b/osu.Game/Skinning/RulesetSkinProvidingContainer.cs index 8a27899e89..13664897ac 100644 --- a/osu.Game/Skinning/RulesetSkinProvidingContainer.cs +++ b/osu.Game/Skinning/RulesetSkinProvidingContainer.cs @@ -12,7 +12,7 @@ namespace osu.Game.Skinning { /// /// A type of that provides access to the beatmap skin and user skin, - /// each transformed with the ruleset's own skin transformer individually. + /// with each legacy skin source transformed with the ruleset's legacy skin transformer. /// public class RulesetSkinProvidingContainer : SkinProvidingContainer { @@ -66,7 +66,7 @@ namespace osu.Game.Skinning protected ISkin GetRulesetTransformedSkin(ISkin skin) { - if (skin == null) + if (!(skin is LegacySkin)) return null; var rulesetTransformed = Ruleset.CreateLegacySkinProvider(skin, Beatmap); From 74ad6f9117f12c834a96a296c026824f002e3e17 Mon Sep 17 00:00:00 2001 From: Salman Ahmed Date: Wed, 16 Jun 2021 17:24:30 +0300 Subject: [PATCH 35/54] Remove default skin from the ruleset skin sources That one doesn't need any changes to it, can be fetched from the `SkinManager` instead. --- osu.Game/Skinning/RulesetSkinProvidingContainer.cs | 3 --- 1 file changed, 3 deletions(-) diff --git a/osu.Game/Skinning/RulesetSkinProvidingContainer.cs b/osu.Game/Skinning/RulesetSkinProvidingContainer.cs index 13664897ac..621e80ceb5 100644 --- a/osu.Game/Skinning/RulesetSkinProvidingContainer.cs +++ b/osu.Game/Skinning/RulesetSkinProvidingContainer.cs @@ -59,9 +59,6 @@ namespace osu.Game.Skinning // TODO: we also want to return a DefaultLegacySkin here if the current *beatmap* is providing any skinned elements. if (skinManager.CurrentSkin.Value is LegacySkin && skinManager.CurrentSkin.Value != skinManager.DefaultLegacySkin) SkinSources.Add(GetRulesetTransformedSkin(skinManager.DefaultLegacySkin)); - - if (skinManager.CurrentSkin.Value != skinManager.DefaultSkin) - SkinSources.Add(GetRulesetTransformedSkin(skinManager.DefaultSkin)); } protected ISkin GetRulesetTransformedSkin(ISkin skin) From 780388d174c63cb711d31377aaa521a4a3e29eb8 Mon Sep 17 00:00:00 2001 From: Salman Ahmed Date: Thu, 17 Jun 2021 03:48:25 +0300 Subject: [PATCH 36/54] Fix incorrect return value --- osu.Game/Skinning/RulesetSkinProvidingContainer.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/osu.Game/Skinning/RulesetSkinProvidingContainer.cs b/osu.Game/Skinning/RulesetSkinProvidingContainer.cs index 621e80ceb5..f11acd981a 100644 --- a/osu.Game/Skinning/RulesetSkinProvidingContainer.cs +++ b/osu.Game/Skinning/RulesetSkinProvidingContainer.cs @@ -64,7 +64,7 @@ namespace osu.Game.Skinning protected ISkin GetRulesetTransformedSkin(ISkin skin) { if (!(skin is LegacySkin)) - return null; + return skin; var rulesetTransformed = Ruleset.CreateLegacySkinProvider(skin, Beatmap); if (rulesetTransformed != null) From 5cfd0e32236a5a63eaf4132e6fe9dea22569bf2c Mon Sep 17 00:00:00 2001 From: Salman Ahmed Date: Mon, 21 Jun 2021 04:16:58 +0300 Subject: [PATCH 37/54] Remove implicit `LegacySkin` check and refactor anything using it --- .../Skinning/RulesetSkinProvidingContainer.cs | 31 +++++++++++++------ osu.Game/Tests/Visual/TestPlayer.cs | 2 +- 2 files changed, 22 insertions(+), 11 deletions(-) diff --git a/osu.Game/Skinning/RulesetSkinProvidingContainer.cs b/osu.Game/Skinning/RulesetSkinProvidingContainer.cs index f11acd981a..bbaeee98d8 100644 --- a/osu.Game/Skinning/RulesetSkinProvidingContainer.cs +++ b/osu.Game/Skinning/RulesetSkinProvidingContainer.cs @@ -26,7 +26,7 @@ namespace osu.Game.Skinning Ruleset = ruleset; Beatmap = beatmap; - InternalChild = new BeatmapSkinProvidingContainer(GetRulesetTransformedSkin(beatmapSkin)) + InternalChild = new BeatmapSkinProvidingContainer(beatmapSkin is LegacySkin ? GetLegacyRulesetTransformedSkin(beatmapSkin) : beatmapSkin) { Child = Content = new Container { @@ -54,23 +54,34 @@ namespace osu.Game.Skinning { SkinSources.Clear(); - SkinSources.Add(GetRulesetTransformedSkin(skinManager.CurrentSkin.Value)); + // TODO: we also want to insert a DefaultLegacySkin here if the current *beatmap* is providing any skinned elements. - // TODO: we also want to return a DefaultLegacySkin here if the current *beatmap* is providing any skinned elements. - if (skinManager.CurrentSkin.Value is LegacySkin && skinManager.CurrentSkin.Value != skinManager.DefaultLegacySkin) - SkinSources.Add(GetRulesetTransformedSkin(skinManager.DefaultLegacySkin)); + switch (skinManager.CurrentSkin.Value) + { + case LegacySkin currentLegacySkin: + SkinSources.Add(GetLegacyRulesetTransformedSkin(currentLegacySkin)); + + if (currentLegacySkin != skinManager.DefaultLegacySkin) + SkinSources.Add(GetLegacyRulesetTransformedSkin(skinManager.DefaultLegacySkin)); + + break; + + default: + SkinSources.Add(skinManager.CurrentSkin.Value); + break; + } } - protected ISkin GetRulesetTransformedSkin(ISkin skin) + protected ISkin GetLegacyRulesetTransformedSkin(ISkin legacySkin) { - if (!(skin is LegacySkin)) - return skin; + if (legacySkin == null) + return null; - var rulesetTransformed = Ruleset.CreateLegacySkinProvider(skin, Beatmap); + var rulesetTransformed = Ruleset.CreateLegacySkinProvider(legacySkin, Beatmap); if (rulesetTransformed != null) return rulesetTransformed; - return skin; + return legacySkin; } } } diff --git a/osu.Game/Tests/Visual/TestPlayer.cs b/osu.Game/Tests/Visual/TestPlayer.cs index e1431b0658..2be5d8ac9f 100644 --- a/osu.Game/Tests/Visual/TestPlayer.cs +++ b/osu.Game/Tests/Visual/TestPlayer.cs @@ -99,7 +99,7 @@ namespace osu.Game.Tests.Visual base.UpdateSkins(); if (skin != null) - SkinSources.Insert(0, GetRulesetTransformedSkin(skin)); + SkinSources.Insert(0, skin is LegacySkin ? GetLegacyRulesetTransformedSkin(skin) : skin); } } } From 11b78ad849d333f8cd0ad79267e7ed4506603daf Mon Sep 17 00:00:00 2001 From: Salman Ahmed Date: Mon, 21 Jun 2021 04:27:09 +0300 Subject: [PATCH 38/54] Make `TestPlayer` skin assigning logic not flaky --- .../Beatmaps/LegacyBeatmapSkinColourTest.cs | 2 +- osu.Game/Tests/Visual/PlayerTestScene.cs | 2 +- osu.Game/Tests/Visual/TestPlayer.cs | 20 ++++++++++++++++--- 3 files changed, 19 insertions(+), 5 deletions(-) diff --git a/osu.Game/Tests/Beatmaps/LegacyBeatmapSkinColourTest.cs b/osu.Game/Tests/Beatmaps/LegacyBeatmapSkinColourTest.cs index 1feb3eebbf..347b611579 100644 --- a/osu.Game/Tests/Beatmaps/LegacyBeatmapSkinColourTest.cs +++ b/osu.Game/Tests/Beatmaps/LegacyBeatmapSkinColourTest.cs @@ -51,7 +51,7 @@ namespace osu.Game.Tests.Beatmaps ExposedPlayer player = CreateTestPlayer(); - player.Skin = new TestSkin(userHasCustomColours); + player.SetSkin(new TestSkin(userHasCustomColours)); LoadScreen(player); diff --git a/osu.Game/Tests/Visual/PlayerTestScene.cs b/osu.Game/Tests/Visual/PlayerTestScene.cs index e42a043eec..f5fad895e2 100644 --- a/osu.Game/Tests/Visual/PlayerTestScene.cs +++ b/osu.Game/Tests/Visual/PlayerTestScene.cs @@ -79,7 +79,7 @@ namespace osu.Game.Tests.Visual } Player = CreatePlayer(ruleset); - Player.Skin = GetPlayerSkin(); + Player.SetSkin(GetPlayerSkin()); LoadScreen(Player); } diff --git a/osu.Game/Tests/Visual/TestPlayer.cs b/osu.Game/Tests/Visual/TestPlayer.cs index 2be5d8ac9f..19fd7068b9 100644 --- a/osu.Game/Tests/Visual/TestPlayer.cs +++ b/osu.Game/Tests/Visual/TestPlayer.cs @@ -1,7 +1,9 @@ // 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.Diagnostics; using System.Linq; using JetBrains.Annotations; using osu.Framework.Allocation; @@ -22,8 +24,6 @@ namespace osu.Game.Tests.Visual /// public class TestPlayer : Player { - public ISkin Skin { get; set; } - protected override bool PauseOnFocusLost { get; } public new DrawableRuleset DrawableRuleset => base.DrawableRuleset; @@ -81,8 +81,22 @@ namespace osu.Game.Tests.Visual ScoreProcessor.NewJudgement += r => Results.Add(r); } + public ISkin Skin { get; private set; } + + private TestSkinProvidingContainer rulesetSkinProvider; + + internal void SetSkin(ISkin skin) + { + Debug.Assert(rulesetSkinProvider == null); + + if (Skin != null) + throw new InvalidOperationException("A skin has already been set."); + + Skin = skin; + } + protected override RulesetSkinProvidingContainer CreateRulesetSkinProvider(Ruleset ruleset, IBeatmap beatmap, ISkin beatmapSkin) - => new TestSkinProvidingContainer(Skin, ruleset, beatmap, beatmapSkin); + => rulesetSkinProvider = new TestSkinProvidingContainer(Skin, ruleset, beatmap, beatmapSkin); private class TestSkinProvidingContainer : RulesetSkinProvidingContainer { From ebe0d43790a7d793dd6db2cb2ae8e1ce57d6d234 Mon Sep 17 00:00:00 2001 From: Salman Ahmed Date: Tue, 22 Jun 2021 02:51:00 +0300 Subject: [PATCH 39/54] Add ability to disallow falling back to parent skins --- osu.Game/Skinning/SkinProvidingContainer.cs | 14 +++++++++++--- 1 file changed, 11 insertions(+), 3 deletions(-) diff --git a/osu.Game/Skinning/SkinProvidingContainer.cs b/osu.Game/Skinning/SkinProvidingContainer.cs index 315571e79b..4435d924c2 100644 --- a/osu.Game/Skinning/SkinProvidingContainer.cs +++ b/osu.Game/Skinning/SkinProvidingContainer.cs @@ -38,6 +38,11 @@ namespace osu.Game.Skinning [CanBeNull] private ISkinSource fallbackSource; + /// + /// Whether falling back to parent s is allowed in this container. + /// + protected virtual bool AllowFallingBackToParent => true; + protected virtual bool AllowDrawableLookup(ISkinComponent component) => true; protected virtual bool AllowTextureLookup(string componentName) => true; @@ -180,9 +185,12 @@ namespace osu.Game.Skinning { var dependencies = new DependencyContainer(base.CreateChildDependencies(parent)); - fallbackSource = dependencies.Get(); - if (fallbackSource != null) - fallbackSource.SourceChanged += OnSourceChanged; + if (AllowFallingBackToParent) + { + fallbackSource = dependencies.Get(); + if (fallbackSource != null) + fallbackSource.SourceChanged += OnSourceChanged; + } dependencies.CacheAs(this); From d53a43cf3c65dbc1ede229e1dffa56f32f6a87da Mon Sep 17 00:00:00 2001 From: Salman Ahmed Date: Tue, 22 Jun 2021 02:52:37 +0300 Subject: [PATCH 40/54] Isolate `RulesetSkinProvidingContainer` from falling back to parent skin sources For simplicity of lookup order, and which sources are used for the lookup. --- .../Skinning/RulesetSkinProvidingContainer.cs | 27 +++++++++++++------ 1 file changed, 19 insertions(+), 8 deletions(-) diff --git a/osu.Game/Skinning/RulesetSkinProvidingContainer.cs b/osu.Game/Skinning/RulesetSkinProvidingContainer.cs index bbaeee98d8..54d366b2f4 100644 --- a/osu.Game/Skinning/RulesetSkinProvidingContainer.cs +++ b/osu.Game/Skinning/RulesetSkinProvidingContainer.cs @@ -7,18 +7,26 @@ using osu.Framework.Graphics; using osu.Framework.Graphics.Containers; using osu.Game.Beatmaps; using osu.Game.Rulesets; +using osu.Game.Rulesets.UI; namespace osu.Game.Skinning { /// - /// A type of that provides access to the beatmap skin and user skin, - /// with each legacy skin source transformed with the ruleset's legacy skin transformer. + /// A type of specialized for and other gameplay-related components. + /// Providing access to the skin sources and the beatmap skin each surrounded with the ruleset legacy skin transformer. + /// While also limiting lookups from falling back to any parent s out of this container. /// public class RulesetSkinProvidingContainer : SkinProvidingContainer { protected readonly Ruleset Ruleset; protected readonly IBeatmap Beatmap; + /// + /// This container already re-exposes all skin sources in a ruleset-usable form. + /// Therefore disallow falling back to any parent any further. + /// + protected override bool AllowFallingBackToParent => false; + protected override Container Content { get; } public RulesetSkinProvidingContainer(Ruleset ruleset, IBeatmap beatmap, [CanBeNull] ISkin beatmapSkin) @@ -42,12 +50,7 @@ namespace osu.Game.Skinning private void load() { UpdateSkins(); - } - - protected override void OnSourceChanged() - { - UpdateSkins(); - base.OnSourceChanged(); + skinManager.SourceChanged += UpdateSkins; } protected virtual void UpdateSkins() @@ -83,5 +86,13 @@ namespace osu.Game.Skinning return legacySkin; } + + protected override void Dispose(bool isDisposing) + { + base.Dispose(isDisposing); + + if (skinManager != null) + skinManager.SourceChanged -= UpdateSkins; + } } } From 97dbc7f20ecc19ee0d00516a51618212b28bc404 Mon Sep 17 00:00:00 2001 From: Salman Ahmed Date: Tue, 22 Jun 2021 02:54:34 +0300 Subject: [PATCH 41/54] Add back `SkinManager.DefaultSkin` to the ruleset skin lookup sources --- osu.Game/Skinning/RulesetSkinProvidingContainer.cs | 2 ++ 1 file changed, 2 insertions(+) diff --git a/osu.Game/Skinning/RulesetSkinProvidingContainer.cs b/osu.Game/Skinning/RulesetSkinProvidingContainer.cs index 54d366b2f4..8113597dee 100644 --- a/osu.Game/Skinning/RulesetSkinProvidingContainer.cs +++ b/osu.Game/Skinning/RulesetSkinProvidingContainer.cs @@ -73,6 +73,8 @@ namespace osu.Game.Skinning SkinSources.Add(skinManager.CurrentSkin.Value); break; } + + SkinSources.Add(skinManager.DefaultSkin); } protected ISkin GetLegacyRulesetTransformedSkin(ISkin legacySkin) From 9e5bb146d3aaa2936c07f2299a58ce36d56d82a4 Mon Sep 17 00:00:00 2001 From: Salman Ahmed Date: Tue, 22 Jun 2021 03:06:39 +0300 Subject: [PATCH 42/54] Add xmldoc to `SkinManager` The `` part comes from `BeatmapManager`, which I believe works correctly here as well, as this does handle the "storage and retrieval" of skins. --- osu.Game/Skinning/SkinManager.cs | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/osu.Game/Skinning/SkinManager.cs b/osu.Game/Skinning/SkinManager.cs index 7acc52809f..1f10177a9e 100644 --- a/osu.Game/Skinning/SkinManager.cs +++ b/osu.Game/Skinning/SkinManager.cs @@ -30,6 +30,13 @@ using osu.Game.IO.Archives; namespace osu.Game.Skinning { + /// + /// Handles the storage and retrieval of s. + /// + /// + /// This is also exposed and cached as on a game-wide level for general components across the game. + /// Lookups from gameplay components are instead covered by , and are never hit here. + /// [ExcludeFromDynamicCompile] public class SkinManager : ArchiveModelManager, ISkinSource, IStorageResourceProvider { From 627c857da8d0293625e9600c2e5176c333c0f0ee Mon Sep 17 00:00:00 2001 From: Salman Ahmed Date: Tue, 22 Jun 2021 03:44:32 +0300 Subject: [PATCH 43/54] Propagate `SourceChanged` events from `SkinManager` down in the ruleset skin container --- osu.Game/Skinning/RulesetSkinProvidingContainer.cs | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/osu.Game/Skinning/RulesetSkinProvidingContainer.cs b/osu.Game/Skinning/RulesetSkinProvidingContainer.cs index 8113597dee..4b3c3881c2 100644 --- a/osu.Game/Skinning/RulesetSkinProvidingContainer.cs +++ b/osu.Game/Skinning/RulesetSkinProvidingContainer.cs @@ -50,7 +50,13 @@ namespace osu.Game.Skinning private void load() { UpdateSkins(); - skinManager.SourceChanged += UpdateSkins; + skinManager.SourceChanged += OnSourceChanged; + } + + protected override void OnSourceChanged() + { + UpdateSkins(); + base.OnSourceChanged(); } protected virtual void UpdateSkins() From caa90bccc6c92ecc399900ad4774f308002c1260 Mon Sep 17 00:00:00 2001 From: Salman Ahmed Date: Tue, 22 Jun 2021 03:45:43 +0300 Subject: [PATCH 44/54] Fix default skin potentially added twice in `RulesetSkinProvidingContainer` --- osu.Game/Skinning/RulesetSkinProvidingContainer.cs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/osu.Game/Skinning/RulesetSkinProvidingContainer.cs b/osu.Game/Skinning/RulesetSkinProvidingContainer.cs index 4b3c3881c2..88cf70fa18 100644 --- a/osu.Game/Skinning/RulesetSkinProvidingContainer.cs +++ b/osu.Game/Skinning/RulesetSkinProvidingContainer.cs @@ -80,7 +80,8 @@ namespace osu.Game.Skinning break; } - SkinSources.Add(skinManager.DefaultSkin); + if (skinManager.CurrentSkin.Value != skinManager.DefaultSkin) + SkinSources.Add(skinManager.DefaultSkin); } protected ISkin GetLegacyRulesetTransformedSkin(ISkin legacySkin) From ec040ff3fc80e3f0374245564f16279caf547b3f Mon Sep 17 00:00:00 2001 From: Salman Ahmed Date: Tue, 22 Jun 2021 05:05:41 +0300 Subject: [PATCH 45/54] Fix leak due to not properly unbinding `SourceChanged` event on disposal --- osu.Game/Skinning/RulesetSkinProvidingContainer.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/osu.Game/Skinning/RulesetSkinProvidingContainer.cs b/osu.Game/Skinning/RulesetSkinProvidingContainer.cs index 88cf70fa18..b07d3f5199 100644 --- a/osu.Game/Skinning/RulesetSkinProvidingContainer.cs +++ b/osu.Game/Skinning/RulesetSkinProvidingContainer.cs @@ -101,7 +101,7 @@ namespace osu.Game.Skinning base.Dispose(isDisposing); if (skinManager != null) - skinManager.SourceChanged -= UpdateSkins; + skinManager.SourceChanged -= OnSourceChanged; } } } From 0ad189e357f93d023dbaec929713ad40e867df36 Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Tue, 22 Jun 2021 16:19:55 +0900 Subject: [PATCH 46/54] Expose skin sources via `ISkinSource` and revert to consuming based on hierarchy --- .../TestSceneCursorTrail.cs | 4 +++ .../TestSceneSkinFallbacks.cs | 3 ++ .../Gameplay/TestSceneSkinnableDrawable.cs | 3 ++ .../Gameplay/TestSceneSkinnableSound.cs | 2 ++ osu.Game/Skinning/ISkinSource.cs | 6 ++++ .../Skinning/RulesetSkinProvidingContainer.cs | 35 ++++++++----------- osu.Game/Skinning/SkinManager.cs | 26 ++++++++++---- osu.Game/Skinning/SkinProvidingContainer.cs | 15 ++++++++ .../Beatmaps/LegacyBeatmapSkinColourTest.cs | 1 + 9 files changed, 67 insertions(+), 28 deletions(-) diff --git a/osu.Game.Rulesets.Osu.Tests/TestSceneCursorTrail.cs b/osu.Game.Rulesets.Osu.Tests/TestSceneCursorTrail.cs index 46274e779b..fe962d3cb8 100644 --- a/osu.Game.Rulesets.Osu.Tests/TestSceneCursorTrail.cs +++ b/osu.Game.Rulesets.Osu.Tests/TestSceneCursorTrail.cs @@ -2,6 +2,8 @@ // 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.Sample; @@ -114,6 +116,8 @@ namespace osu.Game.Rulesets.Osu.Tests public ISkin FindProvider(Func lookupFunction) => null; + public IEnumerable AllSources => Enumerable.Empty(); + public event Action SourceChanged { add { } diff --git a/osu.Game.Rulesets.Osu.Tests/TestSceneSkinFallbacks.cs b/osu.Game.Rulesets.Osu.Tests/TestSceneSkinFallbacks.cs index 2b45818aa9..5ef27ab5ea 100644 --- a/osu.Game.Rulesets.Osu.Tests/TestSceneSkinFallbacks.cs +++ b/osu.Game.Rulesets.Osu.Tests/TestSceneSkinFallbacks.cs @@ -2,6 +2,7 @@ // 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; @@ -146,6 +147,8 @@ namespace osu.Game.Rulesets.Osu.Tests public ISkin FindProvider(Func lookupFunction) => null; + public IEnumerable AllSources => Enumerable.Empty(); + public event Action SourceChanged; private bool enabled = true; diff --git a/osu.Game.Tests/Visual/Gameplay/TestSceneSkinnableDrawable.cs b/osu.Game.Tests/Visual/Gameplay/TestSceneSkinnableDrawable.cs index 77966e925a..3317d8f80a 100644 --- a/osu.Game.Tests/Visual/Gameplay/TestSceneSkinnableDrawable.cs +++ b/osu.Game.Tests/Visual/Gameplay/TestSceneSkinnableDrawable.cs @@ -2,6 +2,7 @@ // See the LICENCE file in the repository root for full licence text. using System; +using System.Collections.Generic; using System.Globalization; using System.Linq; using NUnit.Framework; @@ -330,6 +331,8 @@ namespace osu.Game.Tests.Visual.Gameplay public ISkin FindProvider(Func lookupFunction) => throw new NotImplementedException(); + public IEnumerable AllSources => Enumerable.Empty(); + public event Action SourceChanged { add { } diff --git a/osu.Game.Tests/Visual/Gameplay/TestSceneSkinnableSound.cs b/osu.Game.Tests/Visual/Gameplay/TestSceneSkinnableSound.cs index 55ee01e0d5..59edb527eb 100644 --- a/osu.Game.Tests/Visual/Gameplay/TestSceneSkinnableSound.cs +++ b/osu.Game.Tests/Visual/Gameplay/TestSceneSkinnableSound.cs @@ -2,6 +2,7 @@ // 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; @@ -147,6 +148,7 @@ namespace osu.Game.Tests.Visual.Gameplay public ISample GetSample(ISampleInfo sampleInfo) => source?.GetSample(sampleInfo); public IBindable GetConfig(TLookup lookup) => source?.GetConfig(lookup); public ISkin FindProvider(Func lookupFunction) => source?.FindProvider(lookupFunction); + public IEnumerable AllSources => source.AllSources; public void TriggerSourceChanged() { diff --git a/osu.Game/Skinning/ISkinSource.cs b/osu.Game/Skinning/ISkinSource.cs index c7ebe91d64..ba3e2bf6ad 100644 --- a/osu.Game/Skinning/ISkinSource.cs +++ b/osu.Game/Skinning/ISkinSource.cs @@ -2,6 +2,7 @@ // See the LICENCE file in the repository root for full licence text. using System; +using System.Collections.Generic; using JetBrains.Annotations; namespace osu.Game.Skinning @@ -20,5 +21,10 @@ namespace osu.Game.Skinning /// The skin to be used for subsequent lookups, or null if none is available. [CanBeNull] ISkin FindProvider(Func lookupFunction); + + /// + /// Retrieve all sources available for lookup, with highest priority source first. + /// + IEnumerable AllSources { get; } } } diff --git a/osu.Game/Skinning/RulesetSkinProvidingContainer.cs b/osu.Game/Skinning/RulesetSkinProvidingContainer.cs index b07d3f5199..21a858977b 100644 --- a/osu.Game/Skinning/RulesetSkinProvidingContainer.cs +++ b/osu.Game/Skinning/RulesetSkinProvidingContainer.cs @@ -26,7 +26,6 @@ namespace osu.Game.Skinning /// Therefore disallow falling back to any parent any further. /// protected override bool AllowFallingBackToParent => false; - protected override Container Content { get; } public RulesetSkinProvidingContainer(Ruleset ruleset, IBeatmap beatmap, [CanBeNull] ISkin beatmapSkin) @@ -44,13 +43,13 @@ namespace osu.Game.Skinning } [Resolved] - private SkinManager skinManager { get; set; } + private ISkinSource skinSource { get; set; } [BackgroundDependencyLoader] private void load() { UpdateSkins(); - skinManager.SourceChanged += OnSourceChanged; + skinSource.SourceChanged += OnSourceChanged; } protected override void OnSourceChanged() @@ -63,25 +62,19 @@ namespace osu.Game.Skinning { SkinSources.Clear(); - // TODO: we also want to insert a DefaultLegacySkin here if the current *beatmap* is providing any skinned elements. - - switch (skinManager.CurrentSkin.Value) + foreach (var skin in skinSource.AllSources) { - case LegacySkin currentLegacySkin: - SkinSources.Add(GetLegacyRulesetTransformedSkin(currentLegacySkin)); + switch (skin) + { + case LegacySkin legacySkin: + SkinSources.Add(GetLegacyRulesetTransformedSkin(legacySkin)); + break; - if (currentLegacySkin != skinManager.DefaultLegacySkin) - SkinSources.Add(GetLegacyRulesetTransformedSkin(skinManager.DefaultLegacySkin)); - - break; - - default: - SkinSources.Add(skinManager.CurrentSkin.Value); - break; + default: + SkinSources.Add(skin); + break; + } } - - if (skinManager.CurrentSkin.Value != skinManager.DefaultSkin) - SkinSources.Add(skinManager.DefaultSkin); } protected ISkin GetLegacyRulesetTransformedSkin(ISkin legacySkin) @@ -100,8 +93,8 @@ namespace osu.Game.Skinning { base.Dispose(isDisposing); - if (skinManager != null) - skinManager.SourceChanged -= OnSourceChanged; + if (skinSource != null) + skinSource.SourceChanged -= OnSourceChanged; } } } diff --git a/osu.Game/Skinning/SkinManager.cs b/osu.Game/Skinning/SkinManager.cs index 1f10177a9e..6bd4888eb5 100644 --- a/osu.Game/Skinning/SkinManager.cs +++ b/osu.Game/Skinning/SkinManager.cs @@ -248,17 +248,29 @@ namespace osu.Game.Skinning return null; } + public IEnumerable AllSources + { + get + { + yield return CurrentSkin.Value; + + if (CurrentSkin.Value is LegacySkin) + yield return DefaultLegacySkin; + + yield return DefaultSkin; + } + } + private T lookupWithFallback(Func lookupFunction) where T : class { - if (lookupFunction(CurrentSkin.Value) is T skinSourced) - return skinSourced; + foreach (var source in AllSources) + { + if (lookupFunction(source) is T skinSourced) + return skinSourced; + } - if (CurrentSkin.Value is LegacySkin && lookupFunction(DefaultLegacySkin) is T legacySourced) - return legacySourced; - - // Finally fall back to the (non-legacy) default. - return lookupFunction(DefaultSkin); + return null; } #region IResourceStorageProvider diff --git a/osu.Game/Skinning/SkinProvidingContainer.cs b/osu.Game/Skinning/SkinProvidingContainer.cs index 4435d924c2..c83c299723 100644 --- a/osu.Game/Skinning/SkinProvidingContainer.cs +++ b/osu.Game/Skinning/SkinProvidingContainer.cs @@ -131,6 +131,21 @@ namespace osu.Game.Skinning return fallbackSource?.FindProvider(lookupFunction); } + public IEnumerable AllSources + { + get + { + foreach (var skin in SkinSources) + yield return skin; + + if (fallbackSource != null) + { + foreach (var skin in fallbackSource.AllSources) + yield return skin; + } + } + } + public Drawable GetDrawableComponent(ISkinComponent component) { foreach (var skin in SkinSources) diff --git a/osu.Game/Tests/Beatmaps/LegacyBeatmapSkinColourTest.cs b/osu.Game/Tests/Beatmaps/LegacyBeatmapSkinColourTest.cs index 347b611579..bb4768982a 100644 --- a/osu.Game/Tests/Beatmaps/LegacyBeatmapSkinColourTest.cs +++ b/osu.Game/Tests/Beatmaps/LegacyBeatmapSkinColourTest.cs @@ -146,6 +146,7 @@ namespace osu.Game.Tests.Beatmaps } public ISkin FindProvider(Func lookupFunction) => null; + public IEnumerable AllSources => Enumerable.Empty(); } } } From 14bdcef26b7da34ae0a1e1376b9c8e859ce42224 Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Tue, 22 Jun 2021 16:20:09 +0900 Subject: [PATCH 47/54] Add missing newline --- osu.Game/Skinning/RulesetSkinProvidingContainer.cs | 1 + 1 file changed, 1 insertion(+) diff --git a/osu.Game/Skinning/RulesetSkinProvidingContainer.cs b/osu.Game/Skinning/RulesetSkinProvidingContainer.cs index 21a858977b..f1cc3df3de 100644 --- a/osu.Game/Skinning/RulesetSkinProvidingContainer.cs +++ b/osu.Game/Skinning/RulesetSkinProvidingContainer.cs @@ -26,6 +26,7 @@ namespace osu.Game.Skinning /// Therefore disallow falling back to any parent any further. /// protected override bool AllowFallingBackToParent => false; + protected override Container Content { get; } public RulesetSkinProvidingContainer(Ruleset ruleset, IBeatmap beatmap, [CanBeNull] ISkin beatmapSkin) From b12adc6073b7d9c493a97bc9b2e8c5172fc46d88 Mon Sep 17 00:00:00 2001 From: Salman Ahmed Date: Tue, 22 Jun 2021 10:48:03 +0300 Subject: [PATCH 48/54] Remove all test skinning changes in favour of the `ISkinSource.AllSources` path --- .../TestSceneLegacyBeatmapSkin.cs | 7 +++- .../TestSceneSkinFallbacks.cs | 24 ++++++++++- osu.Game/Screens/Play/Player.cs | 4 +- .../Tests/Beatmaps/HitObjectSampleTest.cs | 42 +++++++++++++++++-- .../Beatmaps/LegacyBeatmapSkinColourTest.cs | 32 ++++++++++---- .../Tests/Visual/LegacySkinPlayerTestScene.cs | 17 +++++++- osu.Game/Tests/Visual/PlayerTestScene.cs | 8 ---- osu.Game/Tests/Visual/TestPlayer.cs | 42 ------------------- 8 files changed, 108 insertions(+), 68 deletions(-) diff --git a/osu.Game.Rulesets.Catch.Tests/TestSceneLegacyBeatmapSkin.cs b/osu.Game.Rulesets.Catch.Tests/TestSceneLegacyBeatmapSkin.cs index 0077ff9e3c..bc3daca16f 100644 --- a/osu.Game.Rulesets.Catch.Tests/TestSceneLegacyBeatmapSkin.cs +++ b/osu.Game.Rulesets.Catch.Tests/TestSceneLegacyBeatmapSkin.cs @@ -101,10 +101,15 @@ namespace osu.Game.Rulesets.Catch.Tests AddAssert("is custom hyper dash fruit colours", () => ((CatchExposedPlayer)TestPlayer).UsableHyperDashFruitColour == TestSkin.HYPER_DASH_FRUIT_COLOUR); } - protected override ExposedPlayer CreateTestPlayer() => new CatchExposedPlayer(); + protected override ExposedPlayer CreateTestPlayer(bool userHasCustomColours) => new CatchExposedPlayer(userHasCustomColours); private class CatchExposedPlayer : ExposedPlayer { + public CatchExposedPlayer(bool userHasCustomColours) + : base(userHasCustomColours) + { + } + public Color4 UsableHyperDashColour => GameplayClockContainer.ChildrenOfType() .First() diff --git a/osu.Game.Rulesets.Osu.Tests/TestSceneSkinFallbacks.cs b/osu.Game.Rulesets.Osu.Tests/TestSceneSkinFallbacks.cs index 5ef27ab5ea..334d27e0a9 100644 --- a/osu.Game.Rulesets.Osu.Tests/TestSceneSkinFallbacks.cs +++ b/osu.Game.Rulesets.Osu.Tests/TestSceneSkinFallbacks.cs @@ -22,6 +22,7 @@ using osu.Game.Graphics.Sprites; using osu.Game.Rulesets.Osu.Objects.Drawables; using osu.Game.Skinning; using osu.Game.Storyboards; +using osu.Game.Tests.Visual; namespace osu.Game.Rulesets.Osu.Tests { @@ -99,7 +100,7 @@ namespace osu.Game.Rulesets.Osu.Tests [Resolved] private AudioManager audio { get; set; } - protected override ISkin GetPlayerSkin() => testUserSkin; + protected override TestPlayer CreatePlayer(Ruleset ruleset) => new SkinProvidingPlayer(testUserSkin); protected override WorkingBeatmap CreateWorkingBeatmap(IBeatmap beatmap, Storyboard storyboard = null) => new CustomSkinWorkingBeatmap(beatmap, storyboard, Clock, audio, testBeatmapSkin); @@ -116,6 +117,27 @@ namespace osu.Game.Rulesets.Osu.Tests protected override ISkin GetSkin() => skin; } + public class SkinProvidingPlayer : TestPlayer + { + private readonly TestSource userSkin; + + public SkinProvidingPlayer(TestSource userSkin) + { + this.userSkin = userSkin; + } + + private DependencyContainer dependencies; + + protected override IReadOnlyDependencyContainer CreateChildDependencies(IReadOnlyDependencyContainer parent) + { + dependencies = new DependencyContainer(base.CreateChildDependencies(parent)); + + dependencies.CacheAs(userSkin); + + return dependencies; + } + } + public class TestSource : ISkinSource { private readonly string identifier; diff --git a/osu.Game/Screens/Play/Player.cs b/osu.Game/Screens/Play/Player.cs index efc5fcfbe5..58f60d14cf 100644 --- a/osu.Game/Screens/Play/Player.cs +++ b/osu.Game/Screens/Play/Player.cs @@ -228,7 +228,7 @@ namespace osu.Game.Screens.Play dependencies.CacheAs(GameplayBeatmap); - var rulesetSkinProvider = CreateRulesetSkinProvider(GameplayRuleset, playableBeatmap, Beatmap.Value.Skin); + var rulesetSkinProvider = new RulesetSkinProvidingContainer(GameplayRuleset, playableBeatmap, Beatmap.Value.Skin); // load the skinning hierarchy first. // this is intentionally done in two stages to ensure things are in a loaded state before exposing the ruleset to skin sources. @@ -309,8 +309,6 @@ namespace osu.Game.Screens.Play protected virtual GameplayClockContainer CreateGameplayClockContainer(WorkingBeatmap beatmap, double gameplayStart) => new MasterGameplayClockContainer(beatmap, gameplayStart); - protected virtual RulesetSkinProvidingContainer CreateRulesetSkinProvider(Ruleset ruleset, IBeatmap beatmap, ISkin beatmapSkin) => new RulesetSkinProvidingContainer(ruleset, beatmap, beatmapSkin); - private Drawable createUnderlayComponents() => DimmableStoryboard = new DimmableStoryboard(Beatmap.Value.Storyboard) { RelativeSizeAxes = Axes.Both }; diff --git a/osu.Game/Tests/Beatmaps/HitObjectSampleTest.cs b/osu.Game/Tests/Beatmaps/HitObjectSampleTest.cs index 7af0397726..7ee6c519b7 100644 --- a/osu.Game/Tests/Beatmaps/HitObjectSampleTest.cs +++ b/osu.Game/Tests/Beatmaps/HitObjectSampleTest.cs @@ -47,11 +47,15 @@ namespace osu.Game.Tests.Beatmaps private readonly TestResourceStore userSkinResourceStore = new TestResourceStore(); private readonly TestResourceStore beatmapSkinResourceStore = new TestResourceStore(); + private SkinSourceDependencyContainer dependencies; private IBeatmap currentTestBeatmap; protected sealed override bool HasCustomSteps => true; protected override bool Autoplay => true; + protected sealed override IReadOnlyDependencyContainer CreateChildDependencies(IReadOnlyDependencyContainer parent) + => new DependencyContainer(dependencies = new SkinSourceDependencyContainer(base.CreateChildDependencies(parent))); + protected sealed override IBeatmap CreateBeatmap(RulesetInfo ruleset) => currentTestBeatmap; protected sealed override WorkingBeatmap CreateWorkingBeatmap(IBeatmap beatmap, Storyboard storyboard = null) @@ -59,8 +63,6 @@ namespace osu.Game.Tests.Beatmaps protected override TestPlayer CreatePlayer(Ruleset ruleset) => new TestPlayer(false); - protected override ISkin GetPlayerSkin() => Skin; - protected void CreateTestWithBeatmap(string filename) { CreateTest(() => @@ -107,7 +109,8 @@ namespace osu.Game.Tests.Beatmaps } }; - Skin = new LegacySkin(userSkinInfo, this); + // Need to refresh the cached skin source to refresh the skin resource store. + dependencies.SkinSource = new SkinProvidingContainer(Skin = new LegacySkin(userSkinInfo, this)); }); } @@ -129,6 +132,39 @@ namespace osu.Game.Tests.Beatmaps #endregion + private class SkinSourceDependencyContainer : IReadOnlyDependencyContainer + { + public ISkinSource SkinSource; + + private readonly IReadOnlyDependencyContainer fallback; + + public SkinSourceDependencyContainer(IReadOnlyDependencyContainer fallback) + { + this.fallback = fallback; + } + + public object Get(Type type) + { + if (type == typeof(ISkinSource)) + return SkinSource; + + return fallback.Get(type); + } + + public object Get(Type type, CacheInfo info) + { + if (type == typeof(ISkinSource)) + return SkinSource; + + return fallback.Get(type, info); + } + + public void Inject(T instance) where T : class + { + // Never used directly + } + } + private class TestResourceStore : IResourceStore { public readonly List PerformedLookups = new List(); diff --git a/osu.Game/Tests/Beatmaps/LegacyBeatmapSkinColourTest.cs b/osu.Game/Tests/Beatmaps/LegacyBeatmapSkinColourTest.cs index bb4768982a..86c75c297a 100644 --- a/osu.Game/Tests/Beatmaps/LegacyBeatmapSkinColourTest.cs +++ b/osu.Game/Tests/Beatmaps/LegacyBeatmapSkinColourTest.cs @@ -4,11 +4,13 @@ using System; using System.Collections.Generic; using System.Linq; +using osu.Framework.Allocation; using osu.Framework.Audio; 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; @@ -47,24 +49,36 @@ namespace osu.Game.Tests.Beatmaps protected virtual ExposedPlayer LoadBeatmap(bool userHasCustomColours) { + ExposedPlayer player; + Beatmap.Value = testBeatmap; - ExposedPlayer player = CreateTestPlayer(); - - player.SetSkin(new TestSkin(userHasCustomColours)); - - LoadScreen(player); + LoadScreen(player = CreateTestPlayer(userHasCustomColours)); return player; } - protected virtual ExposedPlayer CreateTestPlayer() => new ExposedPlayer(); + protected virtual ExposedPlayer CreateTestPlayer(bool userHasCustomColours) => new ExposedPlayer(userHasCustomColours); - protected class ExposedPlayer : TestPlayer + protected class ExposedPlayer : Player { - public ExposedPlayer() - : base(false, false) + protected readonly bool UserHasCustomColours; + + public ExposedPlayer(bool userHasCustomColours) + : base(new PlayerConfiguration + { + AllowPause = false, + ShowResults = false, + }) { + 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 => diff --git a/osu.Game/Tests/Visual/LegacySkinPlayerTestScene.cs b/osu.Game/Tests/Visual/LegacySkinPlayerTestScene.cs index f4f351d46c..d74be70df8 100644 --- a/osu.Game/Tests/Visual/LegacySkinPlayerTestScene.cs +++ b/osu.Game/Tests/Visual/LegacySkinPlayerTestScene.cs @@ -6,6 +6,7 @@ using NUnit.Framework; using osu.Framework.Allocation; using osu.Framework.Extensions.IEnumerableExtensions; using osu.Framework.Testing; +using osu.Game.Rulesets; using osu.Game.Skinning; namespace osu.Game.Tests.Visual @@ -15,12 +16,15 @@ namespace osu.Game.Tests.Visual { protected LegacySkin LegacySkin { get; private set; } - protected override ISkin GetPlayerSkin() => LegacySkin; + private ISkinSource legacySkinSource; + + protected override TestPlayer CreatePlayer(Ruleset ruleset) => new SkinProvidingPlayer(legacySkinSource); [BackgroundDependencyLoader] private void load(SkinManager skins) { LegacySkin = new DefaultLegacySkin(skins); + legacySkinSource = new SkinProvidingContainer(LegacySkin); } [SetUpSteps] @@ -47,5 +51,16 @@ namespace osu.Game.Tests.Visual AddUntilStep("wait for components to load", () => this.ChildrenOfType().All(t => t.ComponentsLoaded)); } + + public class SkinProvidingPlayer : TestPlayer + { + [Cached(typeof(ISkinSource))] + private readonly ISkinSource skinSource; + + public SkinProvidingPlayer(ISkinSource skinSource) + { + this.skinSource = skinSource; + } + } } } diff --git a/osu.Game/Tests/Visual/PlayerTestScene.cs b/osu.Game/Tests/Visual/PlayerTestScene.cs index f5fad895e2..088e997de9 100644 --- a/osu.Game/Tests/Visual/PlayerTestScene.cs +++ b/osu.Game/Tests/Visual/PlayerTestScene.cs @@ -10,7 +10,6 @@ using osu.Framework.Testing; using osu.Game.Configuration; using osu.Game.Rulesets; using osu.Game.Rulesets.Mods; -using osu.Game.Skinning; namespace osu.Game.Tests.Visual { @@ -79,8 +78,6 @@ namespace osu.Game.Tests.Visual } Player = CreatePlayer(ruleset); - Player.SetSkin(GetPlayerSkin()); - LoadScreen(Player); } @@ -96,11 +93,6 @@ namespace osu.Game.Tests.Visual [NotNull] protected abstract Ruleset CreatePlayerRuleset(); - /// - /// Creates an to be put inside the 's ruleset skin providing container. - /// - protected virtual ISkin GetPlayerSkin() => null; - protected sealed override Ruleset CreateRuleset() => CreatePlayerRuleset(); protected virtual TestPlayer CreatePlayer(Ruleset ruleset) => new TestPlayer(false, false); diff --git a/osu.Game/Tests/Visual/TestPlayer.cs b/osu.Game/Tests/Visual/TestPlayer.cs index 19fd7068b9..09da4db952 100644 --- a/osu.Game/Tests/Visual/TestPlayer.cs +++ b/osu.Game/Tests/Visual/TestPlayer.cs @@ -1,21 +1,15 @@ // 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.Diagnostics; using System.Linq; -using JetBrains.Annotations; using osu.Framework.Allocation; using osu.Framework.Bindables; -using osu.Game.Beatmaps; -using osu.Game.Rulesets; using osu.Game.Rulesets.Judgements; using osu.Game.Rulesets.Mods; using osu.Game.Rulesets.Scoring; using osu.Game.Rulesets.UI; using osu.Game.Screens.Play; -using osu.Game.Skinning; namespace osu.Game.Tests.Visual { @@ -80,41 +74,5 @@ namespace osu.Game.Tests.Visual { ScoreProcessor.NewJudgement += r => Results.Add(r); } - - public ISkin Skin { get; private set; } - - private TestSkinProvidingContainer rulesetSkinProvider; - - internal void SetSkin(ISkin skin) - { - Debug.Assert(rulesetSkinProvider == null); - - if (Skin != null) - throw new InvalidOperationException("A skin has already been set."); - - Skin = skin; - } - - protected override RulesetSkinProvidingContainer CreateRulesetSkinProvider(Ruleset ruleset, IBeatmap beatmap, ISkin beatmapSkin) - => rulesetSkinProvider = new TestSkinProvidingContainer(Skin, ruleset, beatmap, beatmapSkin); - - private class TestSkinProvidingContainer : RulesetSkinProvidingContainer - { - private readonly ISkin skin; - - public TestSkinProvidingContainer(ISkin skin, Ruleset ruleset, IBeatmap beatmap, [CanBeNull] ISkin beatmapSkin) - : base(ruleset, beatmap, beatmapSkin) - { - this.skin = skin; - } - - protected override void UpdateSkins() - { - base.UpdateSkins(); - - if (skin != null) - SkinSources.Insert(0, skin is LegacySkin ? GetLegacyRulesetTransformedSkin(skin) : skin); - } - } } } From d0cdc07b1167fefd67bc1db900b9edf302104343 Mon Sep 17 00:00:00 2001 From: Salman Ahmed Date: Tue, 22 Jun 2021 10:49:21 +0300 Subject: [PATCH 49/54] Reuse `AllSources` when looking up on `FindProvider` --- osu.Game/Skinning/SkinManager.cs | 13 +++++-------- 1 file changed, 5 insertions(+), 8 deletions(-) diff --git a/osu.Game/Skinning/SkinManager.cs b/osu.Game/Skinning/SkinManager.cs index 6bd4888eb5..e220cad42a 100644 --- a/osu.Game/Skinning/SkinManager.cs +++ b/osu.Game/Skinning/SkinManager.cs @@ -236,14 +236,11 @@ namespace osu.Game.Skinning public ISkin FindProvider(Func lookupFunction) { - if (lookupFunction(CurrentSkin.Value)) - return CurrentSkin.Value; - - if (CurrentSkin.Value is LegacySkin && lookupFunction(DefaultLegacySkin)) - return DefaultLegacySkin; - - if (lookupFunction(DefaultSkin)) - return DefaultSkin; + foreach (var source in AllSources) + { + if (lookupFunction(source)) + return source; + } return null; } From c1284940e1da334498ca471952b2f8a4c8663033 Mon Sep 17 00:00:00 2001 From: Salman Ahmed Date: Tue, 22 Jun 2021 10:49:37 +0300 Subject: [PATCH 50/54] Fix potentially providing the same skin instance twice in `AllSources` --- osu.Game/Skinning/SkinManager.cs | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/osu.Game/Skinning/SkinManager.cs b/osu.Game/Skinning/SkinManager.cs index e220cad42a..3234cca0ac 100644 --- a/osu.Game/Skinning/SkinManager.cs +++ b/osu.Game/Skinning/SkinManager.cs @@ -251,10 +251,11 @@ namespace osu.Game.Skinning { yield return CurrentSkin.Value; - if (CurrentSkin.Value is LegacySkin) + if (CurrentSkin.Value is LegacySkin && CurrentSkin.Value != DefaultLegacySkin) yield return DefaultLegacySkin; - yield return DefaultSkin; + if (CurrentSkin.Value != DefaultSkin) + yield return DefaultSkin; } } From 31cbb36a647f6221506da9cce093a6f0ed850d4b Mon Sep 17 00:00:00 2001 From: Salman Ahmed Date: Tue, 22 Jun 2021 12:02:21 +0300 Subject: [PATCH 51/54] Implement `FindProvider` and `AllSources` properly on all test `ISkinSource`s --- osu.Game.Rulesets.Osu.Tests/TestSceneCursorTrail.cs | 4 ++-- osu.Game.Rulesets.Osu.Tests/TestSceneSkinFallbacks.cs | 4 ++-- osu.Game.Tests/Visual/Gameplay/TestSceneSkinnableDrawable.cs | 2 +- osu.Game.Tests/Visual/Gameplay/TestSceneSkinnableSound.cs | 4 ++-- osu.Game/Tests/Beatmaps/LegacyBeatmapSkinColourTest.cs | 5 +++-- 5 files changed, 10 insertions(+), 9 deletions(-) diff --git a/osu.Game.Rulesets.Osu.Tests/TestSceneCursorTrail.cs b/osu.Game.Rulesets.Osu.Tests/TestSceneCursorTrail.cs index fe962d3cb8..06bdb562e4 100644 --- a/osu.Game.Rulesets.Osu.Tests/TestSceneCursorTrail.cs +++ b/osu.Game.Rulesets.Osu.Tests/TestSceneCursorTrail.cs @@ -114,9 +114,9 @@ namespace osu.Game.Rulesets.Osu.Tests public IBindable GetConfig(TLookup lookup) => null; - public ISkin FindProvider(Func lookupFunction) => null; + public ISkin FindProvider(Func lookupFunction) => lookupFunction(this) ? this : null; - public IEnumerable AllSources => Enumerable.Empty(); + public IEnumerable AllSources => new[] { this }; public event Action SourceChanged { diff --git a/osu.Game.Rulesets.Osu.Tests/TestSceneSkinFallbacks.cs b/osu.Game.Rulesets.Osu.Tests/TestSceneSkinFallbacks.cs index 334d27e0a9..662cbaee68 100644 --- a/osu.Game.Rulesets.Osu.Tests/TestSceneSkinFallbacks.cs +++ b/osu.Game.Rulesets.Osu.Tests/TestSceneSkinFallbacks.cs @@ -167,9 +167,9 @@ namespace osu.Game.Rulesets.Osu.Tests public IBindable GetConfig(TLookup lookup) => null; - public ISkin FindProvider(Func lookupFunction) => null; + public ISkin FindProvider(Func lookupFunction) => lookupFunction(this) ? this : null; - public IEnumerable AllSources => Enumerable.Empty(); + public IEnumerable AllSources => new[] { this }; public event Action SourceChanged; diff --git a/osu.Game.Tests/Visual/Gameplay/TestSceneSkinnableDrawable.cs b/osu.Game.Tests/Visual/Gameplay/TestSceneSkinnableDrawable.cs index 3317d8f80a..f29fbbf52b 100644 --- a/osu.Game.Tests/Visual/Gameplay/TestSceneSkinnableDrawable.cs +++ b/osu.Game.Tests/Visual/Gameplay/TestSceneSkinnableDrawable.cs @@ -331,7 +331,7 @@ namespace osu.Game.Tests.Visual.Gameplay public ISkin FindProvider(Func lookupFunction) => throw new NotImplementedException(); - public IEnumerable AllSources => Enumerable.Empty(); + public IEnumerable AllSources => throw new NotImplementedException(); public event Action SourceChanged { diff --git a/osu.Game.Tests/Visual/Gameplay/TestSceneSkinnableSound.cs b/osu.Game.Tests/Visual/Gameplay/TestSceneSkinnableSound.cs index 59edb527eb..51f8b90c8f 100644 --- a/osu.Game.Tests/Visual/Gameplay/TestSceneSkinnableSound.cs +++ b/osu.Game.Tests/Visual/Gameplay/TestSceneSkinnableSound.cs @@ -147,8 +147,8 @@ namespace osu.Game.Tests.Visual.Gameplay public Texture GetTexture(string componentName, WrapMode wrapModeS, WrapMode wrapModeT) => source?.GetTexture(componentName, wrapModeS, wrapModeT); public ISample GetSample(ISampleInfo sampleInfo) => source?.GetSample(sampleInfo); public IBindable GetConfig(TLookup lookup) => source?.GetConfig(lookup); - public ISkin FindProvider(Func lookupFunction) => source?.FindProvider(lookupFunction); - public IEnumerable AllSources => source.AllSources; + public ISkin FindProvider(Func lookupFunction) => lookupFunction(this) ? this : source?.FindProvider(lookupFunction); + public IEnumerable AllSources => new[] { this }.Concat(source?.AllSources); public void TriggerSourceChanged() { diff --git a/osu.Game/Tests/Beatmaps/LegacyBeatmapSkinColourTest.cs b/osu.Game/Tests/Beatmaps/LegacyBeatmapSkinColourTest.cs index 86c75c297a..e0c2965fa0 100644 --- a/osu.Game/Tests/Beatmaps/LegacyBeatmapSkinColourTest.cs +++ b/osu.Game/Tests/Beatmaps/LegacyBeatmapSkinColourTest.cs @@ -159,8 +159,9 @@ namespace osu.Game.Tests.Beatmaps remove { } } - public ISkin FindProvider(Func lookupFunction) => null; - public IEnumerable AllSources => Enumerable.Empty(); + public ISkin FindProvider(Func lookupFunction) => lookupFunction(this) ? this : null; + + public IEnumerable AllSources => new[] { this }; } } } From ece63b9ba1ee6cc8683255dab0abf7d5cc439ee0 Mon Sep 17 00:00:00 2001 From: Salman Ahmed Date: Tue, 22 Jun 2021 12:03:55 +0300 Subject: [PATCH 52/54] Remove unused using directive --- osu.Game.Rulesets.Osu.Tests/TestSceneCursorTrail.cs | 1 - 1 file changed, 1 deletion(-) diff --git a/osu.Game.Rulesets.Osu.Tests/TestSceneCursorTrail.cs b/osu.Game.Rulesets.Osu.Tests/TestSceneCursorTrail.cs index 06bdb562e4..211b0e8145 100644 --- a/osu.Game.Rulesets.Osu.Tests/TestSceneCursorTrail.cs +++ b/osu.Game.Rulesets.Osu.Tests/TestSceneCursorTrail.cs @@ -3,7 +3,6 @@ using System; using System.Collections.Generic; -using System.Linq; using NUnit.Framework; using osu.Framework.Allocation; using osu.Framework.Audio.Sample; From 71e2815e7ec670693bf94ad8885e10b13015f338 Mon Sep 17 00:00:00 2001 From: Salman Ahmed Date: Tue, 22 Jun 2021 12:05:17 +0300 Subject: [PATCH 53/54] Update and improve code documentation Co-authored-by: Dean Herbert --- osu.Game/Skinning/RulesetSkinProvidingContainer.cs | 5 ++--- osu.Game/Skinning/SkinManager.cs | 4 ++-- 2 files changed, 4 insertions(+), 5 deletions(-) diff --git a/osu.Game/Skinning/RulesetSkinProvidingContainer.cs b/osu.Game/Skinning/RulesetSkinProvidingContainer.cs index f1cc3df3de..c48aeca99a 100644 --- a/osu.Game/Skinning/RulesetSkinProvidingContainer.cs +++ b/osu.Game/Skinning/RulesetSkinProvidingContainer.cs @@ -13,8 +13,7 @@ namespace osu.Game.Skinning { /// /// A type of specialized for and other gameplay-related components. - /// Providing access to the skin sources and the beatmap skin each surrounded with the ruleset legacy skin transformer. - /// While also limiting lookups from falling back to any parent s out of this container. + /// Providing access to parent skin sources and the beatmap skin each surrounded with the ruleset legacy skin transformer. /// public class RulesetSkinProvidingContainer : SkinProvidingContainer { @@ -22,7 +21,7 @@ namespace osu.Game.Skinning protected readonly IBeatmap Beatmap; /// - /// This container already re-exposes all skin sources in a ruleset-usable form. + /// This container already re-exposes all parent sources in a ruleset-usable form. /// Therefore disallow falling back to any parent any further. /// protected override bool AllowFallingBackToParent => false; diff --git a/osu.Game/Skinning/SkinManager.cs b/osu.Game/Skinning/SkinManager.cs index 3234cca0ac..4cde4cd2b8 100644 --- a/osu.Game/Skinning/SkinManager.cs +++ b/osu.Game/Skinning/SkinManager.cs @@ -34,8 +34,8 @@ namespace osu.Game.Skinning /// Handles the storage and retrieval of s. /// /// - /// This is also exposed and cached as on a game-wide level for general components across the game. - /// Lookups from gameplay components are instead covered by , and are never hit here. + /// This is also exposed and cached as to allow for any component to potentially have skinning support. + /// For gameplay components, see which adds extra legacy and toggle logic that may affect the lookup process. /// [ExcludeFromDynamicCompile] public class SkinManager : ArchiveModelManager, ISkinSource, IStorageResourceProvider From 37f7486fb1f2be6c5c7a2cf355266ec811265f77 Mon Sep 17 00:00:00 2001 From: Salman Ahmed Date: Tue, 22 Jun 2021 12:25:29 +0300 Subject: [PATCH 54/54] Fix potential null reference in LINQ method --- osu.Game.Tests/Visual/Gameplay/TestSceneSkinnableSound.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/osu.Game.Tests/Visual/Gameplay/TestSceneSkinnableSound.cs b/osu.Game.Tests/Visual/Gameplay/TestSceneSkinnableSound.cs index 51f8b90c8f..ccf13e1e8f 100644 --- a/osu.Game.Tests/Visual/Gameplay/TestSceneSkinnableSound.cs +++ b/osu.Game.Tests/Visual/Gameplay/TestSceneSkinnableSound.cs @@ -148,7 +148,7 @@ namespace osu.Game.Tests.Visual.Gameplay public ISample GetSample(ISampleInfo sampleInfo) => source?.GetSample(sampleInfo); public IBindable GetConfig(TLookup lookup) => source?.GetConfig(lookup); public ISkin FindProvider(Func lookupFunction) => lookupFunction(this) ? this : source?.FindProvider(lookupFunction); - public IEnumerable AllSources => new[] { this }.Concat(source?.AllSources); + public IEnumerable AllSources => new[] { this }.Concat(source?.AllSources ?? Enumerable.Empty()); public void TriggerSourceChanged() {