Merge pull request #17703 from peppy/fix-storyboard-fallback-animation-frame-count-weirdness

Fix `DrawableStoryboardAnimation` to handle skin fallback frame count similar to stable

osu.Game.Tests/Visual/Gameplay/TestSceneDrawableStoryboardSprite.cs

@@ -7,11 +7,10 @@ using NUnit.Framework;
 using osu.Framework.Allocation;
 using osu.Framework.Extensions.IEnumerableExtensions;
 using osu.Framework.Graphics;
+using osu.Framework.Graphics.Sprites;
 using osu.Framework.Testing;
-using osu.Framework.Utils;
 using osu.Game.Rulesets;
 using osu.Game.Rulesets.Osu;
-using osu.Game.Skinning;
 using osu.Game.Storyboards;
 using osu.Game.Storyboards.Drawables;
 using osuTK;
@@ -36,7 +35,8 @@ namespace osu.Game.Tests.Visual.Gameplay
 
             AddStep("create sprites", () => SetContents(_ => createSprite(lookup_name, Anchor.TopLeft, Vector2.Zero)));
 
-            assertSpritesFromSkin(false);
+            AddAssert("sprite didn't find texture", () =>
+                sprites.All(sprite => sprite.ChildrenOfType<Sprite>().All(s => s.Texture == null)));
         }
 
         [Test]
@@ -48,9 +48,12 @@ namespace osu.Game.Tests.Visual.Gameplay
 
             AddStep("create sprites", () => SetContents(_ => createSprite(lookup_name, Anchor.TopLeft, Vector2.Zero)));
 
-            assertSpritesFromSkin(true);
+            // Only checking for at least one sprite that succeeded, as not all skins in this test provide the hitcircleoverlay texture.
+            AddAssert("sprite found texture", () =>
+                sprites.Any(sprite => sprite.ChildrenOfType<Sprite>().All(s => s.Texture != null)));
 
-            AddAssert("skinnable sprite has correct size", () => sprites.Any(s => Precision.AlmostEquals(s.ChildrenOfType<SkinnableSprite>().Single().Size, new Vector2(128, 128))));
+            AddAssert("skinnable sprite has correct size", () =>
+                sprites.Any(sprite => sprite.ChildrenOfType<Sprite>().All(s => s.Size == new Vector2(128))));
         }
 
         [Test]
@@ -104,9 +107,5 @@ namespace osu.Game.Tests.Visual.Gameplay
                 s.LifetimeStart = double.MinValue;
                 s.LifetimeEnd = double.MaxValue;
             });
-
-        private void assertSpritesFromSkin(bool fromSkin) =>
-            AddAssert($"sprites are {(fromSkin ? "from skin" : "from storyboard")}",
-                () => sprites.All(sprite => sprite.ChildrenOfType<SkinnableSprite>().Any() == fromSkin));
     }
 }

osu.Game/Skinning/LegacySkinExtensions.cs

@@ -1,10 +1,12 @@
 // 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.Collections.Generic;
+using System.Diagnostics;
 using System.Linq;
-using JetBrains.Annotations;
 using osu.Framework.Allocation;
 using osu.Framework.Bindables;
 using osu.Framework.Graphics;
@@ -18,39 +20,32 @@ namespace osu.Game.Skinning
 {
     public static class LegacySkinExtensions
     {
-        [CanBeNull]
-        public static Drawable GetAnimation(this ISkin source, string componentName, bool animatable, bool looping, bool applyConfigFrameRate = false, string animationSeparator = "-",
-                                            bool startAtCurrentTime = true, double? frameLength = null)
+        public static Drawable? GetAnimation(this ISkin? source, string componentName, bool animatable, bool looping, bool applyConfigFrameRate = false, string animationSeparator = "-",
+                                             bool startAtCurrentTime = true, double? frameLength = null)
             => source.GetAnimation(componentName, default, default, animatable, looping, applyConfigFrameRate, animationSeparator, startAtCurrentTime, frameLength);
 
-        [CanBeNull]
-        public static Drawable GetAnimation(this ISkin source, string componentName, WrapMode wrapModeS, WrapMode wrapModeT, bool animatable, bool looping, bool applyConfigFrameRate = false,
-                                            string animationSeparator = "-",
-                                            bool startAtCurrentTime = true, double? frameLength = null)
+        public static Drawable? GetAnimation(this ISkin? source, string componentName, WrapMode wrapModeS, WrapMode wrapModeT, bool animatable, bool looping, bool applyConfigFrameRate = false,
+                                             string animationSeparator = "-", bool startAtCurrentTime = true, double? frameLength = null)
         {
-            Texture texture;
-
-            // find the first source which provides either the animated or non-animated version.
-            ISkin skin = (source as ISkinSource)?.FindProvider(s =>
-            {
-                if (animatable && s.GetTexture(getFrameName(0)) != null)
-                    return true;
-
-                return s.GetTexture(componentName, wrapModeS, wrapModeT) != null;
-            }) ?? source;
-
-            if (skin == null)
+            if (source == null)
                 return null;
 
-            if (animatable)
-            {
-                var textures = getTextures().ToArray();
+            var textures = GetTextures(source, componentName, wrapModeS, wrapModeT, animatable, animationSeparator, out var retrievalSource);
+
+            switch (textures.Length)
+            {
+                case 0:
+                    return null;
+
+                case 1:
+                    return new Sprite { Texture = textures[0] };
+
+                default:
+                    Debug.Assert(retrievalSource != null);
 
-                if (textures.Length > 0)
-                {
                     var animation = new SkinnableTextureAnimation(startAtCurrentTime)
                     {
-                        DefaultFrameLength = frameLength ?? getFrameLength(skin, applyConfigFrameRate, textures),
+                        DefaultFrameLength = frameLength ?? getFrameLength(retrievalSource, applyConfigFrameRate, textures),
                         Loop = looping,
                     };
 
@@ -58,19 +53,46 @@ namespace osu.Game.Skinning
                         animation.AddFrame(t);
 
                     return animation;
-                }
+            }
+        }
+
+        public static Texture[] GetTextures(this ISkin? source, string componentName, WrapMode wrapModeS, WrapMode wrapModeT, bool animatable, string animationSeparator, out ISkin? retrievalSource)
+        {
+            retrievalSource = null;
+
+            if (source == null)
+                return Array.Empty<Texture>();
+
+            // find the first source which provides either the animated or non-animated version.
+            retrievalSource = (source as ISkinSource)?.FindProvider(s =>
+            {
+                if (animatable && s.GetTexture(getFrameName(0)) != null)
+                    return true;
+
+                return s.GetTexture(componentName, wrapModeS, wrapModeT) != null;
+            }) ?? source;
+
+            if (animatable)
+            {
+                var textures = getTextures(retrievalSource).ToArray();
+
+                if (textures.Length > 0)
+                    return textures;
             }
 
             // if an animation was not allowed or not found, fall back to a sprite retrieval.
-            if ((texture = skin.GetTexture(componentName, wrapModeS, wrapModeT)) != null)
-                return new Sprite { Texture = texture };
+            var singleTexture = retrievalSource.GetTexture(componentName, wrapModeS, wrapModeT);
 
-            return null;
+            return singleTexture != null
+                ? new[] { singleTexture }
+                : Array.Empty<Texture>();
 
-            IEnumerable<Texture> getTextures()
+            IEnumerable<Texture> getTextures(ISkin skin)
             {
                 for (int i = 0; true; i++)
                 {
+                    Texture? texture;
+
                     if ((texture = skin.GetTexture(getFrameName(i), wrapModeS, wrapModeT)) == null)
                         break;
 
@@ -130,7 +152,7 @@ namespace osu.Game.Skinning
         public class SkinnableTextureAnimation : TextureAnimation
         {
             [Resolved(canBeNull: true)]
-            private IAnimationTimeReference timeReference { get; set; }
+            private IAnimationTimeReference? timeReference { get; set; }
 
             private readonly Bindable<double> animationStartTime = new BindableDouble();
 

osu.Game/Storyboards/Drawables/DrawableStoryboardAnimation.cs

@@ -2,10 +2,10 @@
 // See the LICENCE file in the repository root for full licence text.
 
 using System;
+using System.IO;
 using osu.Framework.Allocation;
 using osu.Framework.Graphics;
 using osu.Framework.Graphics.Animations;
-using osu.Framework.Graphics.Sprites;
 using osu.Framework.Graphics.Textures;
 using osu.Framework.Utils;
 using osu.Game.Skinning;
@@ -13,7 +13,7 @@ using osuTK;
 
 namespace osu.Game.Storyboards.Drawables
 {
-    public class DrawableStoryboardAnimation : DrawableAnimation, IFlippable, IVectorScalable
+    public class DrawableStoryboardAnimation : TextureAnimation, IFlippable, IVectorScalable
     {
         public StoryboardAnimation Animation { get; }
 
@@ -90,25 +90,52 @@ namespace osu.Game.Storyboards.Drawables
             LifetimeEnd = animation.EndTime;
         }
 
-        protected override Vector2 GetCurrentDisplaySize()
-        {
-            Texture texture = (CurrentFrame as Sprite)?.Texture
-                              ?? ((CurrentFrame as SkinnableSprite)?.Drawable as Sprite)?.Texture;
-
-            return new Vector2(texture?.DisplayWidth ?? 0, texture?.DisplayHeight ?? 0);
-        }
+        [Resolved]
+        private ISkinSource skin { get; set; }
 
         [BackgroundDependencyLoader]
         private void load(TextureStore textureStore, Storyboard storyboard)
         {
-            for (int frameIndex = 0; frameIndex < Animation.FrameCount; frameIndex++)
+            int frameIndex = 0;
+
+            Texture frameTexture = storyboard.GetTextureFromPath(getFramePath(frameIndex), textureStore);
+
+            if (frameTexture != null)
             {
-                string framePath = Animation.Path.Replace(".", frameIndex + ".");
-                Drawable frame = storyboard.CreateSpriteFromResourcePath(framePath, textureStore) ?? Empty();
-                AddFrame(frame, Animation.FrameDelay);
+                // sourcing from storyboard.
+                for (frameIndex = 0; frameIndex < Animation.FrameCount; frameIndex++)
+                {
+                    AddFrame(storyboard.GetTextureFromPath(getFramePath(frameIndex), textureStore), Animation.FrameDelay);
+                }
+            }
+            else if (storyboard.UseSkinSprites)
+            {
+                // fallback to skin if required.
+                skin.SourceChanged += skinSourceChanged;
+                skinSourceChanged();
             }
 
             Animation.ApplyTransforms(this);
         }
+
+        private void skinSourceChanged()
+        {
+            ClearFrames();
+
+            // When reading from a skin, we match stables weird behaviour where `FrameCount` is ignored
+            // and resources are retrieved until the end of the animation.
+            foreach (var texture in skin.GetTextures(Path.GetFileNameWithoutExtension(Animation.Path), default, default, true, string.Empty, out _))
+                AddFrame(texture, Animation.FrameDelay);
+        }
+
+        private string getFramePath(int i) => Animation.Path.Replace(".", $"{i}.");
+
+        protected override void Dispose(bool isDisposing)
+        {
+            base.Dispose(isDisposing);
+
+            if (skin != null)
+                skin.SourceChanged -= skinSourceChanged;
+        }
     }
 }

osu.Game/Storyboards/Drawables/DrawableStoryboardSprite.cs

@@ -4,14 +4,15 @@
 using System;
 using osu.Framework.Allocation;
 using osu.Framework.Graphics;
-using osu.Framework.Graphics.Containers;
+using osu.Framework.Graphics.Sprites;
 using osu.Framework.Graphics.Textures;
 using osu.Framework.Utils;
+using osu.Game.Skinning;
 using osuTK;
 
 namespace osu.Game.Storyboards.Drawables
 {
-    public class DrawableStoryboardSprite : CompositeDrawable, IFlippable, IVectorScalable
+    public class DrawableStoryboardSprite : Sprite, IFlippable, IVectorScalable
     {
         public StoryboardSprite Sprite { get; }
 
@@ -85,19 +86,33 @@ namespace osu.Game.Storyboards.Drawables
 
             LifetimeStart = sprite.StartTime;
             LifetimeEnd = sprite.EndTime;
-
-            AutoSizeAxes = Axes.Both;
         }
 
+        [Resolved]
+        private ISkinSource skin { get; set; }
+
         [BackgroundDependencyLoader]
         private void load(TextureStore textureStore, Storyboard storyboard)
         {
-            var drawable = storyboard.CreateSpriteFromResourcePath(Sprite.Path, textureStore);
+            Texture = storyboard.GetTextureFromPath(Sprite.Path, textureStore);
 
-            if (drawable != null)
-                InternalChild = drawable;
+            if (Texture == null && storyboard.UseSkinSprites)
+            {
+                skin.SourceChanged += skinSourceChanged;
+                skinSourceChanged();
+            }
 
             Sprite.ApplyTransforms(this);
         }
+
+        private void skinSourceChanged() => Texture = skin.GetTexture(Sprite.Path);
+
+        protected override void Dispose(bool isDisposing)
+        {
+            base.Dispose(isDisposing);
+
+            if (skin != null)
+                skin.SourceChanged -= skinSourceChanged;
+        }
     }
 }

osu.Game/Storyboards/Storyboard.cs

@@ -4,13 +4,10 @@
 using System;
 using System.Collections.Generic;
 using System.Linq;
-using osu.Framework.Graphics;
-using osu.Framework.Graphics.Sprites;
 using osu.Framework.Graphics.Textures;
 using osu.Game.Beatmaps;
 using osu.Game.Extensions;
 using osu.Game.Rulesets.Mods;
-using osu.Game.Skinning;
 using osu.Game.Storyboards.Drawables;
 
 namespace osu.Game.Storyboards
@@ -94,25 +91,14 @@ namespace osu.Game.Storyboards
         public DrawableStoryboard CreateDrawable(IReadOnlyList<Mod> mods = null) =>
             new DrawableStoryboard(this, mods);
 
-        public Drawable CreateSpriteFromResourcePath(string path, TextureStore textureStore)
+        public Texture GetTextureFromPath(string path, TextureStore textureStore)
         {
-            Drawable drawable = null;
-
             string storyboardPath = BeatmapInfo.BeatmapSet?.Files.FirstOrDefault(f => f.Filename.Equals(path, StringComparison.OrdinalIgnoreCase))?.File.GetStoragePath();
 
             if (!string.IsNullOrEmpty(storyboardPath))
-                drawable = new Sprite { Texture = textureStore.Get(storyboardPath) };
-            // if the texture isn't available locally in the beatmap, some storyboards choose to source from the underlying skin lookup hierarchy.
-            else if (UseSkinSprites)
-            {
-                drawable = new SkinnableSprite(path)
-                {
-                    RelativeSizeAxes = Axes.None,
-                    AutoSizeAxes = Axes.Both,
-                };
-            }
+                return textureStore.Get(storyboardPath);
 
-            return drawable;
+            return null;
         }
     }
 }