From 8d0dd3961e056b2670f67412a48cfdf000ec1694 Mon Sep 17 00:00:00 2001 From: Salman Ahmed Date: Tue, 19 Apr 2022 03:25:18 +0300 Subject: [PATCH 01/11] Add failing test cases --- .../LegacyMainCirclePieceTest.cs | 133 ++++++++++++++++++ .../Skinning/Legacy/LegacyMainCirclePiece.cs | 19 ++- 2 files changed, 146 insertions(+), 6 deletions(-) create mode 100644 osu.Game.Rulesets.Osu.Tests/LegacyMainCirclePieceTest.cs diff --git a/osu.Game.Rulesets.Osu.Tests/LegacyMainCirclePieceTest.cs b/osu.Game.Rulesets.Osu.Tests/LegacyMainCirclePieceTest.cs new file mode 100644 index 0000000000..200dd2f593 --- /dev/null +++ b/osu.Game.Rulesets.Osu.Tests/LegacyMainCirclePieceTest.cs @@ -0,0 +1,133 @@ +// 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 NUnit.Framework; +using osu.Framework.Audio.Sample; +using osu.Framework.Bindables; +using osu.Framework.Graphics; +using osu.Framework.Graphics.OpenGL.Textures; +using osu.Framework.Graphics.Sprites; +using osu.Framework.Graphics.Textures; +using osu.Framework.Testing; +using osu.Game.Audio; +using osu.Game.Rulesets.Osu.Skinning.Legacy; +using osu.Game.Skinning; +using osu.Game.Tests.Visual; + +#nullable enable + +namespace osu.Game.Rulesets.Osu.Tests +{ + [HeadlessTest] + public class LegacyMainCirclePieceTest : OsuTestScene + { + private static readonly object?[][] texture_priority_cases = + { + // default priority lookup + new object?[] + { + // available textures + new[] { @"hitcircle", @"hitcircleoverlay" }, + // priority lookup + @"", + // expected circle and overlay + @"hitcircle", @"hitcircleoverlay", + }, + // custom priority lookup + new object?[] + { + new[] { @"hitcircle", @"hitcircleoverlay", @"sliderstartcircle", @"sliderstartcircleoverlay" }, + @"sliderstartcircle", + @"sliderstartcircle", @"sliderstartcircleoverlay", + }, + // when no sprites are available for the specified prefix, fall back to "hitcircle"/"hitcircleoverlay". + new object?[] + { + new[] { @"hitcircle", @"hitcircleoverlay" }, + @"sliderstartcircle", + @"hitcircle", @"hitcircleoverlay", + }, + // when a circle is available for the specified prefix but no overlay exists, no overlay is displayed. + new object?[] + { + new[] { @"hitcircle", @"hitcircleoverlay", @"sliderstartcircle" }, + @"sliderstartcircle", + @"sliderstartcircle", null + }, + // when no circle is available for the specified prefix but an overlay exists, the overlay is ignored. + new object?[] + { + new[] { @"hitcircle", @"hitcircleoverlay", @"sliderstartcircleoverlay" }, + @"sliderstartcircle", + @"hitcircle", @"hitcircleoverlay", + } + }; + + [TestCaseSource(nameof(texture_priority_cases))] + public void TestTexturePriorities(string[] textureFilenames, string priorityLookup, string? expectedCircle, string? expectedOverlay) + { + Sprite? circleSprite = null; + Sprite? overlaySprite = null; + + AddStep("load circle piece", () => + { + Child = new DependencyProvidingContainer + { + CachedDependencies = new (Type, object)[] + { + (typeof(ISkinSource), new TestSkin(textureFilenames)) + }, + Child = new LegacyMainCirclePiece(priorityLookup, false), + }; + + var sprites = this.ChildrenOfType().Where(s => s.Texture.AssetName != null).DistinctBy(s => s.Texture.AssetName).ToArray(); + Debug.Assert(sprites.Length <= 2); + + circleSprite = sprites.ElementAtOrDefault(0); + overlaySprite = sprites.ElementAtOrDefault(1); + }); + + AddAssert("check circle sprite", () => circleSprite?.Texture?.AssetName == expectedCircle); + AddAssert("check overlay sprite", () => overlaySprite?.Texture?.AssetName == expectedOverlay); + } + + private class TestSkin : ISkinSource + { + private readonly string[] textureFilenames; + + public TestSkin(string[] textureFilenames) + { + this.textureFilenames = textureFilenames; + } + + public Texture? GetTexture(string componentName, WrapMode wrapModeS, WrapMode wrapModeT) + { + if (textureFilenames.Contains(componentName)) + return new Texture(1, 1) { AssetName = componentName }; + + return null; + } + + public event Action SourceChanged + { + add { } + remove { } + } + + public IEnumerable AllSources { get; } = Enumerable.Empty(); + public Drawable? GetDrawableComponent(ISkinComponent component) => null; + public ISample? GetSample(ISampleInfo sampleInfo) => null; + + public IBindable? GetConfig(TLookup lookup) + where TLookup : notnull + where TValue : notnull + => null; + + public ISkin? FindProvider(Func lookupFunction) => lookupFunction(this) ? this : null; + } + } +} diff --git a/osu.Game.Rulesets.Osu/Skinning/Legacy/LegacyMainCirclePiece.cs b/osu.Game.Rulesets.Osu/Skinning/Legacy/LegacyMainCirclePiece.cs index c6007885be..829dbb02f1 100644 --- a/osu.Game.Rulesets.Osu/Skinning/Legacy/LegacyMainCirclePiece.cs +++ b/osu.Game.Rulesets.Osu/Skinning/Legacy/LegacyMainCirclePiece.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.Bindables; using osu.Framework.Graphics; @@ -43,7 +44,8 @@ namespace osu.Game.Rulesets.Osu.Skinning.Legacy private readonly Bindable accentColour = new Bindable(); private readonly IBindable indexInCurrentCombo = new Bindable(); - [Resolved] + [Resolved(canBeNull: true)] + [CanBeNull] private DrawableHitObject drawableObject { get; set; } [Resolved] @@ -107,8 +109,11 @@ namespace osu.Game.Rulesets.Osu.Skinning.Legacy if (overlayAboveNumber) OverlayLayer.ChangeChildDepth(hitCircleOverlay, float.MinValue); - accentColour.BindTo(drawableObject.AccentColour); - indexInCurrentCombo.BindTo(drawableOsuObject.IndexInCurrentComboBindable); + if (drawableOsuObject != null) + { + accentColour.BindTo(drawableOsuObject.AccentColour); + indexInCurrentCombo.BindTo(drawableOsuObject.IndexInCurrentComboBindable); + } Texture getTextureWithFallback(string name) { @@ -149,15 +154,17 @@ namespace osu.Game.Rulesets.Osu.Skinning.Legacy if (hasNumber) indexInCurrentCombo.BindValueChanged(index => hitCircleText.Text = (index.NewValue + 1).ToString(), true); - drawableObject.ApplyCustomUpdateState += updateStateTransforms; - updateStateTransforms(drawableObject, drawableObject.State.Value); + if (drawableObject != null) + drawableObject.ApplyCustomUpdateState += updateStateTransforms; + + updateStateTransforms(drawableObject, drawableObject?.State.Value ?? ArmedState.Idle); } private void updateStateTransforms(DrawableHitObject drawableHitObject, ArmedState state) { const double legacy_fade_duration = 240; - using (BeginAbsoluteSequence(drawableObject.HitStateUpdateTime)) + using (BeginAbsoluteSequence(drawableObject?.HitStateUpdateTime ?? 0)) { switch (state) { From ec7bb876b5f76416255fa04e71876f6f39980465 Mon Sep 17 00:00:00 2001 From: Salman Ahmed Date: Tue, 19 Apr 2022 07:12:05 +0300 Subject: [PATCH 02/11] Improve legacy circle texture lookup to match 1:1 with stable --- .../Skinning/Legacy/LegacyMainCirclePiece.cs | 49 +++++-------------- 1 file changed, 12 insertions(+), 37 deletions(-) diff --git a/osu.Game.Rulesets.Osu/Skinning/Legacy/LegacyMainCirclePiece.cs b/osu.Game.Rulesets.Osu/Skinning/Legacy/LegacyMainCirclePiece.cs index 829dbb02f1..5fe4a4b5df 100644 --- a/osu.Game.Rulesets.Osu/Skinning/Legacy/LegacyMainCirclePiece.cs +++ b/osu.Game.Rulesets.Osu/Skinning/Legacy/LegacyMainCirclePiece.cs @@ -23,9 +23,10 @@ namespace osu.Game.Rulesets.Osu.Skinning.Legacy { public override bool RemoveCompletedTransforms => false; - private readonly string priorityLookup; private readonly bool hasNumber; + private string priorityLookup; + public LegacyMainCirclePiece(string priorityLookup = null, bool hasNumber = true) { this.priorityLookup = priorityLookup; @@ -56,21 +57,19 @@ namespace osu.Game.Rulesets.Osu.Skinning.Legacy { var drawableOsuObject = (DrawableOsuHitObject)drawableObject; - bool allowFallback = false; - // attempt lookup using priority specification - Texture baseTexture = getTextureWithFallback(string.Empty); + Texture baseTexture = getTexture(string.Empty); - // if the base texture was not found without a fallback, switch on fallback mode and re-perform the lookup. + // if the base texture was not found using the priority specification, nullify the specification and fall back to "hitcircle". if (baseTexture == null) { - allowFallback = true; - baseTexture = getTextureWithFallback(string.Empty); + priorityLookup = null; + baseTexture = getTexture(string.Empty); } // at this point, any further texture fetches should be correctly using the priority source if the base texture was retrieved using it. // the flow above handles the case where a sliderendcircle.png is retrieved from the skin, but sliderendcircleoverlay.png doesn't exist. - // expected behaviour in this scenario is not showing the overlay, rather than using hitcircleoverlay.png (potentially from the default/fall-through skin). + // expected behaviour in this scenario is not showing the overlay, rather than using hitcircleoverlay.png. InternalChildren = new[] { @@ -83,7 +82,7 @@ namespace osu.Game.Rulesets.Osu.Skinning.Legacy { Anchor = Anchor.Centre, Origin = Anchor.Centre, - Child = hitCircleOverlay = new KiaiFlashingDrawable(() => getAnimationWithFallback(@"overlay", 1000 / 2d)) + Child = hitCircleOverlay = new KiaiFlashingDrawable(() => getAnimation(@"overlay", 1000 / 2d)) { Anchor = Anchor.Centre, Origin = Anchor.Centre, @@ -115,35 +114,11 @@ namespace osu.Game.Rulesets.Osu.Skinning.Legacy indexInCurrentCombo.BindTo(drawableOsuObject.IndexInCurrentComboBindable); } - Texture getTextureWithFallback(string name) - { - Texture tex = null; + Texture getTexture(string name) + => skin.GetTexture($"{priorityLookup ?? @"hitcircle"}{name}"); - if (!string.IsNullOrEmpty(priorityLookup)) - { - tex = skin.GetTexture($"{priorityLookup}{name}"); - - if (!allowFallback) - return tex; - } - - return tex ?? skin.GetTexture($"hitcircle{name}"); - } - - Drawable getAnimationWithFallback(string name, double frameLength) - { - Drawable animation = null; - - if (!string.IsNullOrEmpty(priorityLookup)) - { - animation = skin.GetAnimation($"{priorityLookup}{name}", true, true, frameLength: frameLength); - - if (!allowFallback) - return animation; - } - - return animation ?? skin.GetAnimation($"hitcircle{name}", true, true, frameLength: frameLength); - } + Drawable getAnimation(string name, double frameLength) + => skin.GetAnimation($"{priorityLookup ?? @"hitcircle"}{name}", true, true, frameLength: frameLength); } protected override void LoadComplete() From f92a41b6c3758027e315347fafb01abbac7aa678 Mon Sep 17 00:00:00 2001 From: Salman Ahmed Date: Tue, 19 Apr 2022 08:03:50 +0300 Subject: [PATCH 03/11] Replace local `TestSkin` implementation with simplified Moq setups --- .../LegacyMainCirclePieceTest.cs | 56 ++++--------------- 1 file changed, 10 insertions(+), 46 deletions(-) diff --git a/osu.Game.Rulesets.Osu.Tests/LegacyMainCirclePieceTest.cs b/osu.Game.Rulesets.Osu.Tests/LegacyMainCirclePieceTest.cs index 200dd2f593..766db25d71 100644 --- a/osu.Game.Rulesets.Osu.Tests/LegacyMainCirclePieceTest.cs +++ b/osu.Game.Rulesets.Osu.Tests/LegacyMainCirclePieceTest.cs @@ -1,25 +1,21 @@ // Copyright (c) ppy Pty Ltd . Licensed under the MIT Licence. // See the LICENCE file in the repository root for full licence text. +#nullable enable + using System; -using System.Collections.Generic; using System.Diagnostics; using System.Linq; +using Moq; using NUnit.Framework; -using osu.Framework.Audio.Sample; -using osu.Framework.Bindables; -using osu.Framework.Graphics; using osu.Framework.Graphics.OpenGL.Textures; using osu.Framework.Graphics.Sprites; using osu.Framework.Graphics.Textures; using osu.Framework.Testing; -using osu.Game.Audio; using osu.Game.Rulesets.Osu.Skinning.Legacy; using osu.Game.Skinning; using osu.Game.Tests.Visual; -#nullable enable - namespace osu.Game.Rulesets.Osu.Tests { [HeadlessTest] @@ -75,12 +71,15 @@ namespace osu.Game.Rulesets.Osu.Tests AddStep("load circle piece", () => { + var skin = new Mock(); + + skin.Setup(s => s.GetTexture(It.IsAny())).CallBase(); + skin.Setup(s => s.GetTexture(It.IsIn(textureFilenames), It.IsAny(), It.IsAny())) + .Returns((string componentName, WrapMode _, WrapMode __) => new Texture(1, 1) { AssetName = componentName }); + Child = new DependencyProvidingContainer { - CachedDependencies = new (Type, object)[] - { - (typeof(ISkinSource), new TestSkin(textureFilenames)) - }, + CachedDependencies = new (Type, object)[] { (typeof(ISkinSource), skin.Object) }, Child = new LegacyMainCirclePiece(priorityLookup, false), }; @@ -94,40 +93,5 @@ namespace osu.Game.Rulesets.Osu.Tests AddAssert("check circle sprite", () => circleSprite?.Texture?.AssetName == expectedCircle); AddAssert("check overlay sprite", () => overlaySprite?.Texture?.AssetName == expectedOverlay); } - - private class TestSkin : ISkinSource - { - private readonly string[] textureFilenames; - - public TestSkin(string[] textureFilenames) - { - this.textureFilenames = textureFilenames; - } - - public Texture? GetTexture(string componentName, WrapMode wrapModeS, WrapMode wrapModeT) - { - if (textureFilenames.Contains(componentName)) - return new Texture(1, 1) { AssetName = componentName }; - - return null; - } - - public event Action SourceChanged - { - add { } - remove { } - } - - public IEnumerable AllSources { get; } = Enumerable.Empty(); - public Drawable? GetDrawableComponent(ISkinComponent component) => null; - public ISample? GetSample(ISampleInfo sampleInfo) => null; - - public IBindable? GetConfig(TLookup lookup) - where TLookup : notnull - where TValue : notnull - => null; - - public ISkin? FindProvider(Func lookupFunction) => lookupFunction(this) ? this : null; - } } } From fd113953ace591b867081da881821f15dd356511 Mon Sep 17 00:00:00 2001 From: Salman Ahmed Date: Tue, 19 Apr 2022 08:06:39 +0300 Subject: [PATCH 04/11] Rename `prioritiyLookup` and add xmldoc --- .../Skinning/Legacy/LegacyMainCirclePiece.cs | 19 +++++++++++++------ 1 file changed, 13 insertions(+), 6 deletions(-) diff --git a/osu.Game.Rulesets.Osu/Skinning/Legacy/LegacyMainCirclePiece.cs b/osu.Game.Rulesets.Osu/Skinning/Legacy/LegacyMainCirclePiece.cs index 5fe4a4b5df..82bb4dcf2d 100644 --- a/osu.Game.Rulesets.Osu/Skinning/Legacy/LegacyMainCirclePiece.cs +++ b/osu.Game.Rulesets.Osu/Skinning/Legacy/LegacyMainCirclePiece.cs @@ -25,11 +25,18 @@ namespace osu.Game.Rulesets.Osu.Skinning.Legacy private readonly bool hasNumber; - private string priorityLookup; + /// + /// A prioritised prefix to perform texture lookups with. + /// + /// + /// If the "circle" texture could not be found with this prefix, + /// then it is nullified and the default prefix "hitcircle" is used instead. + /// + private string priorityLookupPrefix; - public LegacyMainCirclePiece(string priorityLookup = null, bool hasNumber = true) + public LegacyMainCirclePiece(string priorityLookupPrefix = null, bool hasNumber = true) { - this.priorityLookup = priorityLookup; + this.priorityLookupPrefix = priorityLookupPrefix; this.hasNumber = hasNumber; Size = new Vector2(OsuHitObject.OBJECT_RADIUS * 2); @@ -63,7 +70,7 @@ namespace osu.Game.Rulesets.Osu.Skinning.Legacy // if the base texture was not found using the priority specification, nullify the specification and fall back to "hitcircle". if (baseTexture == null) { - priorityLookup = null; + priorityLookupPrefix = null; baseTexture = getTexture(string.Empty); } @@ -115,10 +122,10 @@ namespace osu.Game.Rulesets.Osu.Skinning.Legacy } Texture getTexture(string name) - => skin.GetTexture($"{priorityLookup ?? @"hitcircle"}{name}"); + => skin.GetTexture($"{priorityLookupPrefix ?? @"hitcircle"}{name}"); Drawable getAnimation(string name, double frameLength) - => skin.GetAnimation($"{priorityLookup ?? @"hitcircle"}{name}", true, true, frameLength: frameLength); + => skin.GetAnimation($"{priorityLookupPrefix ?? @"hitcircle"}{name}", true, true, frameLength: frameLength); } protected override void LoadComplete() From b067924adacb6ecdc38d3d9ccdf52a30b44458f7 Mon Sep 17 00:00:00 2001 From: Salman Ahmed Date: Tue, 19 Apr 2022 08:08:02 +0300 Subject: [PATCH 05/11] Avoid applying state transforms when no object is present --- .../Skinning/Legacy/LegacyMainCirclePiece.cs | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/osu.Game.Rulesets.Osu/Skinning/Legacy/LegacyMainCirclePiece.cs b/osu.Game.Rulesets.Osu/Skinning/Legacy/LegacyMainCirclePiece.cs index 82bb4dcf2d..9d14041dd8 100644 --- a/osu.Game.Rulesets.Osu/Skinning/Legacy/LegacyMainCirclePiece.cs +++ b/osu.Game.Rulesets.Osu/Skinning/Legacy/LegacyMainCirclePiece.cs @@ -4,6 +4,7 @@ using JetBrains.Annotations; using osu.Framework.Allocation; using osu.Framework.Bindables; +using osu.Framework.Extensions.ObjectExtensions; using osu.Framework.Graphics; using osu.Framework.Graphics.Containers; using osu.Framework.Graphics.Sprites; @@ -137,16 +138,17 @@ namespace osu.Game.Rulesets.Osu.Skinning.Legacy indexInCurrentCombo.BindValueChanged(index => hitCircleText.Text = (index.NewValue + 1).ToString(), true); if (drawableObject != null) + { drawableObject.ApplyCustomUpdateState += updateStateTransforms; - - updateStateTransforms(drawableObject, drawableObject?.State.Value ?? ArmedState.Idle); + updateStateTransforms(drawableObject, drawableObject.State.Value); + } } private void updateStateTransforms(DrawableHitObject drawableHitObject, ArmedState state) { const double legacy_fade_duration = 240; - using (BeginAbsoluteSequence(drawableObject?.HitStateUpdateTime ?? 0)) + using (BeginAbsoluteSequence(drawableObject.AsNonNull().HitStateUpdateTime)) { switch (state) { From 75a6e9fd7f7bbf82325051c89fe7eab3caa60dcc Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Tue, 19 Apr 2022 18:10:10 +0900 Subject: [PATCH 06/11] Convert to use `nullable` and rearrange fields --- .../Skinning/Legacy/LegacyMainCirclePiece.cs | 47 +++++++++---------- 1 file changed, 23 insertions(+), 24 deletions(-) diff --git a/osu.Game.Rulesets.Osu/Skinning/Legacy/LegacyMainCirclePiece.cs b/osu.Game.Rulesets.Osu/Skinning/Legacy/LegacyMainCirclePiece.cs index 9d14041dd8..1fedc7daae 100644 --- a/osu.Game.Rulesets.Osu/Skinning/Legacy/LegacyMainCirclePiece.cs +++ b/osu.Game.Rulesets.Osu/Skinning/Legacy/LegacyMainCirclePiece.cs @@ -1,7 +1,7 @@ +#nullable enable // 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.Bindables; using osu.Framework.Extensions.ObjectExtensions; @@ -33,9 +33,25 @@ namespace osu.Game.Rulesets.Osu.Skinning.Legacy /// If the "circle" texture could not be found with this prefix, /// then it is nullified and the default prefix "hitcircle" is used instead. /// - private string priorityLookupPrefix; + private string? priorityLookupPrefix; - public LegacyMainCirclePiece(string priorityLookupPrefix = null, bool hasNumber = true) + private Drawable hitCircleSprite = null!; + + protected Container OverlayLayer { get; private set; } = null!; + + private Drawable hitCircleOverlay = null!; + private SkinnableSpriteText hitCircleText = null!; + + private readonly Bindable accentColour = new Bindable(); + private readonly IBindable indexInCurrentCombo = new Bindable(); + + [Resolved(canBeNull: true)] + private DrawableHitObject? drawableObject { get; set; } + + [Resolved] + private ISkinSource skin { get; set; } = null!; + + public LegacyMainCirclePiece(string? priorityLookupPrefix = null, bool hasNumber = true) { this.priorityLookupPrefix = priorityLookupPrefix; this.hasNumber = hasNumber; @@ -43,30 +59,13 @@ namespace osu.Game.Rulesets.Osu.Skinning.Legacy Size = new Vector2(OsuHitObject.OBJECT_RADIUS * 2); } - private Drawable hitCircleSprite; - - protected Container OverlayLayer { get; private set; } - - private Drawable hitCircleOverlay; - private SkinnableSpriteText hitCircleText; - - private readonly Bindable accentColour = new Bindable(); - private readonly IBindable indexInCurrentCombo = new Bindable(); - - [Resolved(canBeNull: true)] - [CanBeNull] - private DrawableHitObject drawableObject { get; set; } - - [Resolved] - private ISkinSource skin { get; set; } - [BackgroundDependencyLoader] private void load() { - var drawableOsuObject = (DrawableOsuHitObject)drawableObject; + var drawableOsuObject = (DrawableOsuHitObject?)drawableObject; // attempt lookup using priority specification - Texture baseTexture = getTexture(string.Empty); + Texture? baseTexture = getTexture(string.Empty); // if the base texture was not found using the priority specification, nullify the specification and fall back to "hitcircle". if (baseTexture == null) @@ -122,10 +121,10 @@ namespace osu.Game.Rulesets.Osu.Skinning.Legacy indexInCurrentCombo.BindTo(drawableOsuObject.IndexInCurrentComboBindable); } - Texture getTexture(string name) + Texture? getTexture(string name) => skin.GetTexture($"{priorityLookupPrefix ?? @"hitcircle"}{name}"); - Drawable getAnimation(string name, double frameLength) + Drawable? getAnimation(string name, double frameLength) => skin.GetAnimation($"{priorityLookupPrefix ?? @"hitcircle"}{name}", true, true, frameLength: frameLength); } From 033b556be58fe1b780d52d0b6c90b75eaea23f31 Mon Sep 17 00:00:00 2001 From: Salman Ahmed Date: Tue, 19 Apr 2022 19:39:06 +0300 Subject: [PATCH 07/11] Simplify texture lookup further --- .../Skinning/Legacy/LegacyMainCirclePiece.cs | 35 +++++-------------- 1 file changed, 9 insertions(+), 26 deletions(-) diff --git a/osu.Game.Rulesets.Osu/Skinning/Legacy/LegacyMainCirclePiece.cs b/osu.Game.Rulesets.Osu/Skinning/Legacy/LegacyMainCirclePiece.cs index 1fedc7daae..2107de61b6 100644 --- a/osu.Game.Rulesets.Osu/Skinning/Legacy/LegacyMainCirclePiece.cs +++ b/osu.Game.Rulesets.Osu/Skinning/Legacy/LegacyMainCirclePiece.cs @@ -8,7 +8,6 @@ using osu.Framework.Extensions.ObjectExtensions; using osu.Framework.Graphics; using osu.Framework.Graphics.Containers; using osu.Framework.Graphics.Sprites; -using osu.Framework.Graphics.Textures; using osu.Game.Graphics; using osu.Game.Graphics.Sprites; using osu.Game.Rulesets.Objects.Drawables; @@ -24,16 +23,12 @@ namespace osu.Game.Rulesets.Osu.Skinning.Legacy { public override bool RemoveCompletedTransforms => false; - private readonly bool hasNumber; - /// /// A prioritised prefix to perform texture lookups with. /// - /// - /// If the "circle" texture could not be found with this prefix, - /// then it is nullified and the default prefix "hitcircle" is used instead. - /// - private string? priorityLookupPrefix; + private readonly string? priorityLookupPrefix; + + private readonly bool hasNumber; private Drawable hitCircleSprite = null!; @@ -64,23 +59,17 @@ namespace osu.Game.Rulesets.Osu.Skinning.Legacy { var drawableOsuObject = (DrawableOsuHitObject?)drawableObject; - // attempt lookup using priority specification - Texture? baseTexture = getTexture(string.Empty); - - // if the base texture was not found using the priority specification, nullify the specification and fall back to "hitcircle". - if (baseTexture == null) - { - priorityLookupPrefix = null; - baseTexture = getTexture(string.Empty); - } + // if a base texture for the specified prefix exists, continue using it for subsequent lookups. + // otherwise fall back to the default prefix "hitcircle". + string circleName = (priorityLookupPrefix != null && skin.GetTexture(priorityLookupPrefix) != null) ? priorityLookupPrefix : @"hitcircle"; // at this point, any further texture fetches should be correctly using the priority source if the base texture was retrieved using it. - // the flow above handles the case where a sliderendcircle.png is retrieved from the skin, but sliderendcircleoverlay.png doesn't exist. + // the conditional above handles the case where a sliderendcircle.png is retrieved from the skin, but sliderendcircleoverlay.png doesn't exist. // expected behaviour in this scenario is not showing the overlay, rather than using hitcircleoverlay.png. InternalChildren = new[] { - hitCircleSprite = new KiaiFlashingDrawable(() => new Sprite { Texture = baseTexture }) + hitCircleSprite = new KiaiFlashingDrawable(() => new Sprite { Texture = skin.GetTexture(circleName) }) { Anchor = Anchor.Centre, Origin = Anchor.Centre, @@ -89,7 +78,7 @@ namespace osu.Game.Rulesets.Osu.Skinning.Legacy { Anchor = Anchor.Centre, Origin = Anchor.Centre, - Child = hitCircleOverlay = new KiaiFlashingDrawable(() => getAnimation(@"overlay", 1000 / 2d)) + Child = hitCircleOverlay = new KiaiFlashingDrawable(() => skin.GetAnimation(@$"{circleName}overlay", true, true, frameLength: 1000 / 2d)) { Anchor = Anchor.Centre, Origin = Anchor.Centre, @@ -120,12 +109,6 @@ namespace osu.Game.Rulesets.Osu.Skinning.Legacy accentColour.BindTo(drawableOsuObject.AccentColour); indexInCurrentCombo.BindTo(drawableOsuObject.IndexInCurrentComboBindable); } - - Texture? getTexture(string name) - => skin.GetTexture($"{priorityLookupPrefix ?? @"hitcircle"}{name}"); - - Drawable? getAnimation(string name, double frameLength) - => skin.GetAnimation($"{priorityLookupPrefix ?? @"hitcircle"}{name}", true, true, frameLength: frameLength); } protected override void LoadComplete() From 813dc2dd78fd0ba65bcae4caddd289be67447310 Mon Sep 17 00:00:00 2001 From: Salman Ahmed Date: Tue, 19 Apr 2022 19:46:29 +0300 Subject: [PATCH 08/11] Fix wrong prefix for default priority lookup in test --- osu.Game.Rulesets.Osu.Tests/LegacyMainCirclePieceTest.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/osu.Game.Rulesets.Osu.Tests/LegacyMainCirclePieceTest.cs b/osu.Game.Rulesets.Osu.Tests/LegacyMainCirclePieceTest.cs index 766db25d71..c9eeef3c01 100644 --- a/osu.Game.Rulesets.Osu.Tests/LegacyMainCirclePieceTest.cs +++ b/osu.Game.Rulesets.Osu.Tests/LegacyMainCirclePieceTest.cs @@ -29,7 +29,7 @@ namespace osu.Game.Rulesets.Osu.Tests // available textures new[] { @"hitcircle", @"hitcircleoverlay" }, // priority lookup - @"", + null, // expected circle and overlay @"hitcircle", @"hitcircleoverlay", }, From a96664295bfd6cec5adc4b9c222ca47707e5f207 Mon Sep 17 00:00:00 2001 From: Salman Ahmed Date: Tue, 19 Apr 2022 19:48:44 +0300 Subject: [PATCH 09/11] Fix nullability preprocessor placed over the copyright header --- osu.Game.Rulesets.Osu/Skinning/Legacy/LegacyMainCirclePiece.cs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/osu.Game.Rulesets.Osu/Skinning/Legacy/LegacyMainCirclePiece.cs b/osu.Game.Rulesets.Osu/Skinning/Legacy/LegacyMainCirclePiece.cs index 2107de61b6..db4d643046 100644 --- a/osu.Game.Rulesets.Osu/Skinning/Legacy/LegacyMainCirclePiece.cs +++ b/osu.Game.Rulesets.Osu/Skinning/Legacy/LegacyMainCirclePiece.cs @@ -1,4 +1,3 @@ -#nullable enable // Copyright (c) ppy Pty Ltd . Licensed under the MIT Licence. // See the LICENCE file in the repository root for full licence text. @@ -17,6 +16,8 @@ using osu.Game.Skinning; using osuTK; using osuTK.Graphics; +#nullable enable + namespace osu.Game.Rulesets.Osu.Skinning.Legacy { public class LegacyMainCirclePiece : CompositeDrawable From fd20c2bdcd7c174b27efded6d8e371ec2233ba1f Mon Sep 17 00:00:00 2001 From: Salman Ahmed Date: Wed, 20 Apr 2022 00:24:18 +0300 Subject: [PATCH 10/11] Change circle/overlay sprite fields to `protected` for better test assertion --- .../LegacyMainCirclePieceTest.cs | 25 ++++++++++++------- .../Skinning/Legacy/LegacyMainCirclePiece.cs | 20 +++++++-------- 2 files changed, 26 insertions(+), 19 deletions(-) diff --git a/osu.Game.Rulesets.Osu.Tests/LegacyMainCirclePieceTest.cs b/osu.Game.Rulesets.Osu.Tests/LegacyMainCirclePieceTest.cs index c9eeef3c01..fd5b6fd752 100644 --- a/osu.Game.Rulesets.Osu.Tests/LegacyMainCirclePieceTest.cs +++ b/osu.Game.Rulesets.Osu.Tests/LegacyMainCirclePieceTest.cs @@ -28,7 +28,7 @@ namespace osu.Game.Rulesets.Osu.Tests { // available textures new[] { @"hitcircle", @"hitcircleoverlay" }, - // priority lookup + // priority lookup prefix null, // expected circle and overlay @"hitcircle", @"hitcircleoverlay", @@ -66,8 +66,7 @@ namespace osu.Game.Rulesets.Osu.Tests [TestCaseSource(nameof(texture_priority_cases))] public void TestTexturePriorities(string[] textureFilenames, string priorityLookup, string? expectedCircle, string? expectedOverlay) { - Sprite? circleSprite = null; - Sprite? overlaySprite = null; + TestLegacyMainCirclePiece piece = null!; AddStep("load circle piece", () => { @@ -80,18 +79,26 @@ namespace osu.Game.Rulesets.Osu.Tests Child = new DependencyProvidingContainer { CachedDependencies = new (Type, object)[] { (typeof(ISkinSource), skin.Object) }, - Child = new LegacyMainCirclePiece(priorityLookup, false), + Child = piece = new TestLegacyMainCirclePiece(priorityLookup), }; var sprites = this.ChildrenOfType().Where(s => s.Texture.AssetName != null).DistinctBy(s => s.Texture.AssetName).ToArray(); Debug.Assert(sprites.Length <= 2); - - circleSprite = sprites.ElementAtOrDefault(0); - overlaySprite = sprites.ElementAtOrDefault(1); }); - AddAssert("check circle sprite", () => circleSprite?.Texture?.AssetName == expectedCircle); - AddAssert("check overlay sprite", () => overlaySprite?.Texture?.AssetName == expectedOverlay); + AddAssert("check circle sprite", () => piece.CircleSprite?.Texture?.AssetName == expectedCircle); + AddAssert("check overlay sprite", () => piece.OverlaySprite?.Texture?.AssetName == expectedOverlay); + } + + private class TestLegacyMainCirclePiece : LegacyMainCirclePiece + { + public new Sprite? CircleSprite => base.CircleSprite.ChildrenOfType().DistinctBy(s => s.Texture.AssetName).SingleOrDefault(); + public new Sprite? OverlaySprite => base.OverlaySprite.ChildrenOfType().DistinctBy(s => s.Texture.AssetName).SingleOrDefault(); + + public TestLegacyMainCirclePiece(string? priorityLookupPrefix) + : base(priorityLookupPrefix, false) + { + } } } } diff --git a/osu.Game.Rulesets.Osu/Skinning/Legacy/LegacyMainCirclePiece.cs b/osu.Game.Rulesets.Osu/Skinning/Legacy/LegacyMainCirclePiece.cs index db4d643046..391147648f 100644 --- a/osu.Game.Rulesets.Osu/Skinning/Legacy/LegacyMainCirclePiece.cs +++ b/osu.Game.Rulesets.Osu/Skinning/Legacy/LegacyMainCirclePiece.cs @@ -31,11 +31,11 @@ namespace osu.Game.Rulesets.Osu.Skinning.Legacy private readonly bool hasNumber; - private Drawable hitCircleSprite = null!; + protected Drawable CircleSprite = null!; + protected Drawable OverlaySprite = null!; protected Container OverlayLayer { get; private set; } = null!; - private Drawable hitCircleOverlay = null!; private SkinnableSpriteText hitCircleText = null!; private readonly Bindable accentColour = new Bindable(); @@ -70,7 +70,7 @@ namespace osu.Game.Rulesets.Osu.Skinning.Legacy InternalChildren = new[] { - hitCircleSprite = new KiaiFlashingDrawable(() => new Sprite { Texture = skin.GetTexture(circleName) }) + CircleSprite = new KiaiFlashingDrawable(() => new Sprite { Texture = skin.GetTexture(circleName) }) { Anchor = Anchor.Centre, Origin = Anchor.Centre, @@ -79,7 +79,7 @@ namespace osu.Game.Rulesets.Osu.Skinning.Legacy { Anchor = Anchor.Centre, Origin = Anchor.Centre, - Child = hitCircleOverlay = new KiaiFlashingDrawable(() => skin.GetAnimation(@$"{circleName}overlay", true, true, frameLength: 1000 / 2d)) + Child = OverlaySprite = new KiaiFlashingDrawable(() => skin.GetAnimation(@$"{circleName}overlay", true, true, frameLength: 1000 / 2d)) { Anchor = Anchor.Centre, Origin = Anchor.Centre, @@ -103,7 +103,7 @@ namespace osu.Game.Rulesets.Osu.Skinning.Legacy bool overlayAboveNumber = skin.GetConfig(OsuSkinConfiguration.HitCircleOverlayAboveNumber)?.Value ?? true; if (overlayAboveNumber) - OverlayLayer.ChangeChildDepth(hitCircleOverlay, float.MinValue); + OverlayLayer.ChangeChildDepth(OverlaySprite, float.MinValue); if (drawableOsuObject != null) { @@ -116,7 +116,7 @@ namespace osu.Game.Rulesets.Osu.Skinning.Legacy { base.LoadComplete(); - accentColour.BindValueChanged(colour => hitCircleSprite.Colour = LegacyColourCompatibility.DisallowZeroAlpha(colour.NewValue), true); + accentColour.BindValueChanged(colour => CircleSprite.Colour = LegacyColourCompatibility.DisallowZeroAlpha(colour.NewValue), true); if (hasNumber) indexInCurrentCombo.BindValueChanged(index => hitCircleText.Text = (index.NewValue + 1).ToString(), true); @@ -136,11 +136,11 @@ namespace osu.Game.Rulesets.Osu.Skinning.Legacy switch (state) { case ArmedState.Hit: - hitCircleSprite.FadeOut(legacy_fade_duration, Easing.Out); - hitCircleSprite.ScaleTo(1.4f, legacy_fade_duration, Easing.Out); + CircleSprite.FadeOut(legacy_fade_duration, Easing.Out); + CircleSprite.ScaleTo(1.4f, legacy_fade_duration, Easing.Out); - hitCircleOverlay.FadeOut(legacy_fade_duration, Easing.Out); - hitCircleOverlay.ScaleTo(1.4f, legacy_fade_duration, Easing.Out); + OverlaySprite.FadeOut(legacy_fade_duration, Easing.Out); + OverlaySprite.ScaleTo(1.4f, legacy_fade_duration, Easing.Out); if (hasNumber) { From 858c8f927f7baccc9328cb85847d0ea510d2b0a9 Mon Sep 17 00:00:00 2001 From: Salman Ahmed Date: Wed, 20 Apr 2022 00:27:02 +0300 Subject: [PATCH 11/11] Attach comment explaining purpose of `CallBase` --- osu.Game.Rulesets.Osu.Tests/LegacyMainCirclePieceTest.cs | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/osu.Game.Rulesets.Osu.Tests/LegacyMainCirclePieceTest.cs b/osu.Game.Rulesets.Osu.Tests/LegacyMainCirclePieceTest.cs index fd5b6fd752..d8c10b814d 100644 --- a/osu.Game.Rulesets.Osu.Tests/LegacyMainCirclePieceTest.cs +++ b/osu.Game.Rulesets.Osu.Tests/LegacyMainCirclePieceTest.cs @@ -72,7 +72,11 @@ namespace osu.Game.Rulesets.Osu.Tests { var skin = new Mock(); + // shouldn't be required as GetTexture(string) calls GetTexture(string, WrapMode, WrapMode) by default, + // but moq doesn't handle that well, therefore explicitly requiring to use `CallBase`: + // https://github.com/moq/moq4/issues/972 skin.Setup(s => s.GetTexture(It.IsAny())).CallBase(); + skin.Setup(s => s.GetTexture(It.IsIn(textureFilenames), It.IsAny(), It.IsAny())) .Returns((string componentName, WrapMode _, WrapMode __) => new Texture(1, 1) { AssetName = componentName });