Merge pull request #17878 from frenzibyte/legacy-hitcircle-sprite-priority

Improve legacy hitcircle texture lookup to match 1:1 with stable

osu.Game.Rulesets.Osu.Tests/LegacyMainCirclePieceTest.cs

@@ -0,0 +1,108 @@
+// Copyright (c) ppy Pty Ltd <contact@ppy.sh>. Licensed under the MIT Licence.
+// See the LICENCE file in the repository root for full licence text.
+
+#nullable enable
+
+using System;
+using System.Diagnostics;
+using System.Linq;
+using Moq;
+using NUnit.Framework;
+using osu.Framework.Graphics.OpenGL.Textures;
+using osu.Framework.Graphics.Sprites;
+using osu.Framework.Graphics.Textures;
+using osu.Framework.Testing;
+using osu.Game.Rulesets.Osu.Skinning.Legacy;
+using osu.Game.Skinning;
+using osu.Game.Tests.Visual;
+
+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 prefix
+                null,
+                // 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)
+        {
+            TestLegacyMainCirclePiece piece = null!;
+
+            AddStep("load circle piece", () =>
+            {
+                var skin = new Mock<ISkinSource>();
+
+                // 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<string>())).CallBase();
+
+                skin.Setup(s => s.GetTexture(It.IsIn(textureFilenames), It.IsAny<WrapMode>(), It.IsAny<WrapMode>()))
+                    .Returns((string componentName, WrapMode _, WrapMode __) => new Texture(1, 1) { AssetName = componentName });
+
+                Child = new DependencyProvidingContainer
+                {
+                    CachedDependencies = new (Type, object)[] { (typeof(ISkinSource), skin.Object) },
+                    Child = piece = new TestLegacyMainCirclePiece(priorityLookup),
+                };
+
+                var sprites = this.ChildrenOfType<Sprite>().Where(s => s.Texture.AssetName != null).DistinctBy(s => s.Texture.AssetName).ToArray();
+                Debug.Assert(sprites.Length <= 2);
+            });
+
+            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<Sprite>().DistinctBy(s => s.Texture.AssetName).SingleOrDefault();
+            public new Sprite? OverlaySprite => base.OverlaySprite.ChildrenOfType<Sprite>().DistinctBy(s => s.Texture.AssetName).SingleOrDefault();
+
+            public TestLegacyMainCirclePiece(string? priorityLookupPrefix)
+                : base(priorityLookupPrefix, false)
+            {
+            }
+        }
+    }
+}

osu.Game.Rulesets.Osu/Skinning/Legacy/LegacyMainCirclePiece.cs

@@ -3,10 +3,10 @@
 
 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;
-using osu.Framework.Graphics.Textures;
 using osu.Game.Graphics;
 using osu.Game.Graphics.Sprites;
 using osu.Game.Rulesets.Objects.Drawables;
@@ -16,63 +16,61 @@ using osu.Game.Skinning;
 using osuTK;
 using osuTK.Graphics;
 
+#nullable enable
+
 namespace osu.Game.Rulesets.Osu.Skinning.Legacy
 {
     public class LegacyMainCirclePiece : CompositeDrawable
     {
         public override bool RemoveCompletedTransforms => false;
 
-        private readonly string priorityLookup;
+        /// <summary>
+        /// A prioritised prefix to perform texture lookups with.
+        /// </summary>
+        private readonly string? priorityLookupPrefix;
+
         private readonly bool hasNumber;
 
-        public LegacyMainCirclePiece(string priorityLookup = null, bool hasNumber = true)
+        protected Drawable CircleSprite = null!;
+        protected Drawable OverlaySprite = null!;
+
+        protected Container OverlayLayer { get; private set; } = null!;
+
+        private SkinnableSpriteText hitCircleText = null!;
+
+        private readonly Bindable<Color4> accentColour = new Bindable<Color4>();
+        private readonly IBindable<int> indexInCurrentCombo = new Bindable<int>();
+
+        [Resolved(canBeNull: true)]
+        private DrawableHitObject? drawableObject { get; set; }
+
+        [Resolved]
+        private ISkinSource skin { get; set; } = null!;
+
+        public LegacyMainCirclePiece(string? priorityLookupPrefix = null, bool hasNumber = true)
         {
-            this.priorityLookup = priorityLookup;
+            this.priorityLookupPrefix = priorityLookupPrefix;
             this.hasNumber = hasNumber;
 
             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<Color4> accentColour = new Bindable<Color4>();
-        private readonly IBindable<int> indexInCurrentCombo = new Bindable<int>();
-
-        [Resolved]
-        private DrawableHitObject drawableObject { get; set; }
-
-        [Resolved]
-        private ISkinSource skin { get; set; }
-
         [BackgroundDependencyLoader]
         private void load()
         {
-            var drawableOsuObject = (DrawableOsuHitObject)drawableObject;
+            var drawableOsuObject = (DrawableOsuHitObject?)drawableObject;
 
-            bool allowFallback = false;
+            // if a base texture for the specified prefix exists, continue using it for subsequent lookups.
-
+            // otherwise fall back to the default prefix "hitcircle".
-            // attempt lookup using priority specification
+            string circleName = (priorityLookupPrefix != null && skin.GetTexture(priorityLookupPrefix) != null) ? priorityLookupPrefix : @"hitcircle";
-            Texture baseTexture = getTextureWithFallback(string.Empty);
-
-            // if the base texture was not found without a fallback, switch on fallback mode and re-perform the lookup.
-            if (baseTexture == null)
-            {
-                allowFallback = true;
-                baseTexture = getTextureWithFallback(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.
+            // 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 (potentially from the default/fall-through skin).
+            // expected behaviour in this scenario is not showing the overlay, rather than using hitcircleoverlay.png.
 
             InternalChildren = new[]
             {
-                hitCircleSprite = new KiaiFlashingDrawable(() => new Sprite { Texture = baseTexture })
+                CircleSprite = new KiaiFlashingDrawable(() => new Sprite { Texture = skin.GetTexture(circleName) })
                 {
                     Anchor = Anchor.Centre,
                     Origin = Anchor.Centre,
@@ -81,7 +79,7 @@ namespace osu.Game.Rulesets.Osu.Skinning.Legacy
                 {
                     Anchor = Anchor.Centre,
                     Origin = Anchor.Centre,
-                    Child = hitCircleOverlay = new KiaiFlashingDrawable(() => getAnimationWithFallback(@"overlay", 1000 / 2d))
+                    Child = OverlaySprite = new KiaiFlashingDrawable(() => skin.GetAnimation(@$"{circleName}overlay", true, true, frameLength: 1000 / 2d))
                     {
                         Anchor = Anchor.Centre,
                         Origin = Anchor.Centre,
@@ -105,39 +103,12 @@ namespace osu.Game.Rulesets.Osu.Skinning.Legacy
             bool overlayAboveNumber = skin.GetConfig<OsuSkinConfiguration, bool>(OsuSkinConfiguration.HitCircleOverlayAboveNumber)?.Value ?? true;
 
             if (overlayAboveNumber)
-                OverlayLayer.ChangeChildDepth(hitCircleOverlay, float.MinValue);
+                OverlayLayer.ChangeChildDepth(OverlaySprite, float.MinValue);
 
-            accentColour.BindTo(drawableObject.AccentColour);
+            if (drawableOsuObject != null)
-            indexInCurrentCombo.BindTo(drawableOsuObject.IndexInCurrentComboBindable);
-
-            Texture getTextureWithFallback(string name)
             {
-                Texture tex = null;
+                accentColour.BindTo(drawableOsuObject.AccentColour);
-
+                indexInCurrentCombo.BindTo(drawableOsuObject.IndexInCurrentComboBindable);
-                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);
             }
         }
 
@@ -145,28 +116,31 @@ 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);
 
-            drawableObject.ApplyCustomUpdateState += updateStateTransforms;
+            if (drawableObject != null)
-            updateStateTransforms(drawableObject, drawableObject.State.Value);
+            {
+                drawableObject.ApplyCustomUpdateState += updateStateTransforms;
+                updateStateTransforms(drawableObject, drawableObject.State.Value);
+            }
         }
 
         private void updateStateTransforms(DrawableHitObject drawableHitObject, ArmedState state)
         {
             const double legacy_fade_duration = 240;
 
-            using (BeginAbsoluteSequence(drawableObject.HitStateUpdateTime))
+            using (BeginAbsoluteSequence(drawableObject.AsNonNull().HitStateUpdateTime))
             {
                 switch (state)
                 {
                     case ArmedState.Hit:
-                        hitCircleSprite.FadeOut(legacy_fade_duration, Easing.Out);
+                        CircleSprite.FadeOut(legacy_fade_duration, Easing.Out);
-                        hitCircleSprite.ScaleTo(1.4f, legacy_fade_duration, Easing.Out);
+                        CircleSprite.ScaleTo(1.4f, legacy_fade_duration, Easing.Out);
 
-                        hitCircleOverlay.FadeOut(legacy_fade_duration, Easing.Out);
+                        OverlaySprite.FadeOut(legacy_fade_duration, Easing.Out);
-                        hitCircleOverlay.ScaleTo(1.4f, legacy_fade_duration, Easing.Out);
+                        OverlaySprite.ScaleTo(1.4f, legacy_fade_duration, Easing.Out);
 
                         if (hasNumber)
                         {