From 69e2e309717382f6ae63d5b24205a249f089b562 Mon Sep 17 00:00:00 2001 From: Salman Ahmed Date: Sat, 23 Apr 2022 01:06:35 +0300 Subject: [PATCH 1/6] Remove legacy skin component fallback lookup Has always been a source of hiding actual issues like in https://github.com/ppy/osu/issues/17745 (`sliderfollowcircle` looked up from fallback because there's no transformer), and https://github.com/ppy/osu/issues/17934 (because there's a texture `ok` and that matches the component name `HitResult.Ok`). --- osu.Game/Skinning/LegacySkin.cs | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/osu.Game/Skinning/LegacySkin.cs b/osu.Game/Skinning/LegacySkin.cs index aed6026d69..1b3b0eea18 100644 --- a/osu.Game/Skinning/LegacySkin.cs +++ b/osu.Game/Skinning/LegacySkin.cs @@ -391,9 +391,10 @@ namespace osu.Game.Skinning } return null; - } - return this.GetAnimation(component.LookupName, false, false); + default: + throw new ArgumentOutOfRangeException(nameof(component)); + } } private Texture? getParticleTexture(HitResult result) From 61078910a64de149e999f970146837b3334d8bfa Mon Sep 17 00:00:00 2001 From: Salman Ahmed Date: Sat, 23 Apr 2022 01:08:53 +0300 Subject: [PATCH 2/6] Handle all skin component types explicitly --- .../Skinning/Legacy/CatchLegacySkinTransformer.cs | 3 +++ .../Skinning/Legacy/ManiaLegacySkinTransformer.cs | 5 +++-- .../Skinning/Legacy/OsuLegacySkinTransformer.cs | 6 ++++++ .../Skinning/Legacy/TaikoLegacySkinTransformer.cs | 7 +++++++ osu.Game/Skinning/DefaultSkin.cs | 2 +- osu.Game/Skinning/LegacySkin.cs | 5 ++++- osu.Game/Skinning/SkinnableSprite.cs | 2 +- .../Skinning/UnsupportedSkinComponentException.cs | 15 +++++++++++++++ 8 files changed, 40 insertions(+), 5 deletions(-) create mode 100644 osu.Game/Skinning/UnsupportedSkinComponentException.cs diff --git a/osu.Game.Rulesets.Catch/Skinning/Legacy/CatchLegacySkinTransformer.cs b/osu.Game.Rulesets.Catch/Skinning/Legacy/CatchLegacySkinTransformer.cs index b2a555f89d..04b522b404 100644 --- a/osu.Game.Rulesets.Catch/Skinning/Legacy/CatchLegacySkinTransformer.cs +++ b/osu.Game.Rulesets.Catch/Skinning/Legacy/CatchLegacySkinTransformer.cs @@ -90,6 +90,9 @@ namespace osu.Game.Rulesets.Catch.Skinning.Legacy return new LegacyHitExplosion(); return null; + + default: + throw new UnsupportedSkinComponentException(component); } } diff --git a/osu.Game.Rulesets.Mania/Skinning/Legacy/ManiaLegacySkinTransformer.cs b/osu.Game.Rulesets.Mania/Skinning/Legacy/ManiaLegacySkinTransformer.cs index 431bd77402..315b4444c2 100644 --- a/osu.Game.Rulesets.Mania/Skinning/Legacy/ManiaLegacySkinTransformer.cs +++ b/osu.Game.Rulesets.Mania/Skinning/Legacy/ManiaLegacySkinTransformer.cs @@ -116,9 +116,10 @@ namespace osu.Game.Rulesets.Mania.Skinning.Legacy case ManiaSkinComponents.StageForeground: return new LegacyStageForeground(); - } - break; + default: + throw new UnsupportedSkinComponentException(component); + } } return base.GetDrawableComponent(component); diff --git a/osu.Game.Rulesets.Osu/Skinning/Legacy/OsuLegacySkinTransformer.cs b/osu.Game.Rulesets.Osu/Skinning/Legacy/OsuLegacySkinTransformer.cs index 900ad6f6d3..572185e6e1 100644 --- a/osu.Game.Rulesets.Osu/Skinning/Legacy/OsuLegacySkinTransformer.cs +++ b/osu.Game.Rulesets.Osu/Skinning/Legacy/OsuLegacySkinTransformer.cs @@ -35,6 +35,9 @@ namespace osu.Game.Rulesets.Osu.Skinning.Legacy case OsuSkinComponents.FollowPoint: return this.GetAnimation(component.LookupName, true, true, true, startAtCurrentTime: false); + case OsuSkinComponents.SliderScorePoint: + return this.GetAnimation(component.LookupName, false, false); + case OsuSkinComponents.SliderFollowCircle: var followCircle = this.GetAnimation("sliderfollowcircle", true, true, true); if (followCircle != null) @@ -123,6 +126,9 @@ namespace osu.Game.Rulesets.Osu.Skinning.Legacy case OsuSkinComponents.ApproachCircle: return new LegacyApproachCircle(); + + default: + throw new UnsupportedSkinComponentException(component); } } diff --git a/osu.Game.Rulesets.Taiko/Skinning/Legacy/TaikoLegacySkinTransformer.cs b/osu.Game.Rulesets.Taiko/Skinning/Legacy/TaikoLegacySkinTransformer.cs index bbc8f0abea..af5921b0fb 100644 --- a/osu.Game.Rulesets.Taiko/Skinning/Legacy/TaikoLegacySkinTransformer.cs +++ b/osu.Game.Rulesets.Taiko/Skinning/Legacy/TaikoLegacySkinTransformer.cs @@ -57,6 +57,10 @@ namespace osu.Game.Rulesets.Taiko.Skinning.Legacy case TaikoSkinComponents.DrumRollTick: return this.GetAnimation("sliderscorepoint", false, false); + case TaikoSkinComponents.Swell: + // todo: support taiko legacy swell (https://github.com/ppy/osu/issues/13601). + return null; + case TaikoSkinComponents.HitTarget: if (GetTexture("taikobigcircle") != null) return new TaikoLegacyHitTarget(); @@ -119,6 +123,9 @@ namespace osu.Game.Rulesets.Taiko.Skinning.Legacy case TaikoSkinComponents.Mascot: return new DrawableTaikoMascot(); + + default: + throw new UnsupportedSkinComponentException(component); } } diff --git a/osu.Game/Skinning/DefaultSkin.cs b/osu.Game/Skinning/DefaultSkin.cs index 119b0ec9ad..fb24084659 100644 --- a/osu.Game/Skinning/DefaultSkin.cs +++ b/osu.Game/Skinning/DefaultSkin.cs @@ -155,7 +155,7 @@ namespace osu.Game.Skinning return skinnableTargetWrapper; } - break; + return null; } switch (component.LookupName) diff --git a/osu.Game/Skinning/LegacySkin.cs b/osu.Game/Skinning/LegacySkin.cs index 1b3b0eea18..b65ba8b04c 100644 --- a/osu.Game/Skinning/LegacySkin.cs +++ b/osu.Game/Skinning/LegacySkin.cs @@ -392,8 +392,11 @@ namespace osu.Game.Skinning return null; + case SkinnableSprite.SpriteComponent sprite: + return this.GetAnimation(sprite.LookupName, false, false); + default: - throw new ArgumentOutOfRangeException(nameof(component)); + throw new UnsupportedSkinComponentException(component); } } diff --git a/osu.Game/Skinning/SkinnableSprite.cs b/osu.Game/Skinning/SkinnableSprite.cs index 4b4d7fe2c6..af9ff0f1d7 100644 --- a/osu.Game/Skinning/SkinnableSprite.cs +++ b/osu.Game/Skinning/SkinnableSprite.cs @@ -65,7 +65,7 @@ namespace osu.Game.Skinning public bool UsesFixedAnchor { get; set; } - private class SpriteComponent : ISkinComponent + public class SpriteComponent : ISkinComponent { public string LookupName { get; set; } diff --git a/osu.Game/Skinning/UnsupportedSkinComponentException.cs b/osu.Game/Skinning/UnsupportedSkinComponentException.cs new file mode 100644 index 0000000000..643f29c3db --- /dev/null +++ b/osu.Game/Skinning/UnsupportedSkinComponentException.cs @@ -0,0 +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; + +namespace osu.Game.Skinning +{ + public class UnsupportedSkinComponentException : Exception + { + public UnsupportedSkinComponentException(ISkinComponent component) + : base($@"Unsupported component type: {component.GetType()}(""{component.LookupName}"").") + { + } + } +} From 89519086c89fd8897e65816677f1ca088e743907 Mon Sep 17 00:00:00 2001 From: Salman Ahmed Date: Sat, 23 Apr 2022 03:16:58 +0300 Subject: [PATCH 3/6] Fix `SkinnableTestScene` unnecessarily adding a base raw skin layer --- osu.Game/Tests/Visual/SkinnableTestScene.cs | 18 ++++++++++-------- 1 file changed, 10 insertions(+), 8 deletions(-) diff --git a/osu.Game/Tests/Visual/SkinnableTestScene.cs b/osu.Game/Tests/Visual/SkinnableTestScene.cs index 2e1ca09fe4..296ed80e37 100644 --- a/osu.Game/Tests/Visual/SkinnableTestScene.cs +++ b/osu.Game/Tests/Visual/SkinnableTestScene.cs @@ -74,11 +74,15 @@ namespace osu.Game.Tests.Visual createdDrawables.Add(created); - SkinProvidingContainer mainProvider; Container childContainer; OutlineBox outlineBox; SkinProvidingContainer skinProvider; + ISkin provider = skin; + + if (provider is LegacySkin legacyProvider) + provider = Ruleset.Value.CreateInstance().CreateLegacySkinProvider(legacyProvider, beatmap); + var children = new Container { RelativeSizeAxes = Axes.Both, @@ -107,12 +111,10 @@ namespace osu.Game.Tests.Visual Children = new Drawable[] { outlineBox = new OutlineBox(), - (mainProvider = new SkinProvidingContainer(skin)).WithChild( - skinProvider = new SkinProvidingContainer(Ruleset.Value.CreateInstance().CreateLegacySkinProvider(mainProvider, beatmap)) - { - Child = created, - } - ) + skinProvider = new SkinProvidingContainer(provider) + { + Child = created, + } } }, } @@ -130,7 +132,7 @@ namespace osu.Game.Tests.Visual { bool autoSize = created.RelativeSizeAxes == Axes.None; - foreach (var c in new[] { mainProvider, childContainer, skinProvider }) + foreach (var c in new[] { childContainer, skinProvider }) { c.RelativeSizeAxes = Axes.None; c.AutoSizeAxes = Axes.None; From 610d61dce9dcc1be01acf3dce37ea254b6125942 Mon Sep 17 00:00:00 2001 From: Salman Ahmed Date: Sat, 23 Apr 2022 03:17:18 +0300 Subject: [PATCH 4/6] Fix `TestSceneSliderApplication` not applying legacy skin transformer --- osu.Game.Rulesets.Osu.Tests/TestSceneSliderApplication.cs | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/osu.Game.Rulesets.Osu.Tests/TestSceneSliderApplication.cs b/osu.Game.Rulesets.Osu.Tests/TestSceneSliderApplication.cs index d673b7a6ac..a40ae611d8 100644 --- a/osu.Game.Rulesets.Osu.Tests/TestSceneSliderApplication.cs +++ b/osu.Game.Rulesets.Osu.Tests/TestSceneSliderApplication.cs @@ -70,7 +70,9 @@ namespace osu.Game.Rulesets.Osu.Tests var tintingSkin = skinManager.GetSkin(DefaultLegacySkin.CreateInfo()); tintingSkin.Configuration.ConfigDictionary["AllowSliderBallTint"] = "1"; - Child = new SkinProvidingContainer(tintingSkin) + var provider = Ruleset.Value.CreateInstance().CreateLegacySkinProvider(tintingSkin, Beatmap.Value.Beatmap); + + Child = new SkinProvidingContainer(provider) { RelativeSizeAxes = Axes.Both, Child = dho = new DrawableSlider(prepareObject(new Slider From 4bf11df57c9ac2ac19646f2ac83a94b75be2279c Mon Sep 17 00:00:00 2001 From: Salman Ahmed Date: Sat, 23 Apr 2022 19:11:54 +0300 Subject: [PATCH 5/6] Change `SpriteComponent` accessibility to `internal` --- osu.Game/Skinning/SkinnableSprite.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/osu.Game/Skinning/SkinnableSprite.cs b/osu.Game/Skinning/SkinnableSprite.cs index af9ff0f1d7..21b34fcd27 100644 --- a/osu.Game/Skinning/SkinnableSprite.cs +++ b/osu.Game/Skinning/SkinnableSprite.cs @@ -65,7 +65,7 @@ namespace osu.Game.Skinning public bool UsesFixedAnchor { get; set; } - public class SpriteComponent : ISkinComponent + internal class SpriteComponent : ISkinComponent { public string LookupName { get; set; } From d2780c2c50557b1bdbfc1bec444ae9814dd08e80 Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Mon, 25 Apr 2022 19:06:47 +0900 Subject: [PATCH 6/6] Add a touch more detail to the unsupported skin component exception --- osu.Game/Skinning/UnsupportedSkinComponentException.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/osu.Game/Skinning/UnsupportedSkinComponentException.cs b/osu.Game/Skinning/UnsupportedSkinComponentException.cs index 643f29c3db..7f0dd51d5b 100644 --- a/osu.Game/Skinning/UnsupportedSkinComponentException.cs +++ b/osu.Game/Skinning/UnsupportedSkinComponentException.cs @@ -8,7 +8,7 @@ namespace osu.Game.Skinning public class UnsupportedSkinComponentException : Exception { public UnsupportedSkinComponentException(ISkinComponent component) - : base($@"Unsupported component type: {component.GetType()}(""{component.LookupName}"").") + : base($@"Unsupported component type: {component.GetType()} (lookup: ""{component.LookupName}"").") { } }