Fix toggling storyboard after first load not getting correct cl… (#5299)

Fix toggling storyboard after first load not getting correct clock

Co-authored-by: Dean Herbert <pe@ppy.sh>
Co-authored-by: Dan Balasescu <1329837+smoogipoo@users.noreply.github.com>

osu.Game.Tests/Visual/Background/TestSceneUserDimContainer.cs

@@ -10,6 +10,7 @@ using osu.Framework.Allocation;
 using osu.Framework.Audio;
 using osu.Framework.Bindables;
 using osu.Framework.Graphics;
+using osu.Framework.Graphics.Sprites;
 using osu.Framework.Input.Events;
 using osu.Framework.Input.States;
 using osu.Framework.Platform;
@@ -36,7 +37,7 @@ using osuTK.Graphics;
 namespace osu.Game.Tests.Visual.Background
 {
     [TestFixture]
-    public class TestSceneBackgroundScreenBeatmap : ManualInputManagerTestScene
+    public class TestSceneUserDimContainer : ManualInputManagerTestScene
     {
         public override IReadOnlyList<Type> RequiredTypes => new[]
         {
@@ -137,14 +138,14 @@ namespace osu.Game.Tests.Visual.Background
                 player.StoryboardEnabled.Value = true;
             });
             waitForDim();
-            AddAssert("Background is invisible, storyboard is visible", () => songSelect.IsBackgroundInvisible() && player.IsStoryboardVisible());
+            AddAssert("Background is invisible, storyboard is visible", () => songSelect.IsBackgroundInvisible() && player.IsStoryboardVisible);
             AddStep("Storyboard Disabled", () =>
             {
                 player.ReplacesBackground.Value = false;
                 player.StoryboardEnabled.Value = false;
             });
             waitForDim();
-            AddAssert("Background is visible, storyboard is invisible", () => songSelect.IsBackgroundVisible() && player.IsStoryboardInvisible());
+            AddAssert("Background is visible, storyboard is invisible", () => songSelect.IsBackgroundVisible() && !player.IsStoryboardVisible);
         }
 
         /// <summary>
@@ -241,14 +242,15 @@ namespace osu.Game.Tests.Visual.Background
         {
             player.StoryboardEnabled.Value = false;
             player.ReplacesBackground.Value = false;
-            player.CurrentStoryboardContainer.Add(new OsuSpriteText
+            player.DimmableStoryboard.Add(new OsuSpriteText
             {
-                Size = new Vector2(250, 50),
+                Size = new Vector2(500, 50),
                 Alpha = 1,
-                Colour = Color4.Tomato,
+                Colour = Color4.White,
                 Anchor = Anchor.Centre,
                 Origin = Anchor.Centre,
                 Text = "THIS IS A STORYBOARD",
+                Font = new FontUsage(size: 50)
             });
         });
 
@@ -300,7 +302,7 @@ namespace osu.Game.Tests.Visual.Background
 
             public bool IsBackgroundUndimmed() => ((FadeAccessibleBackground)Background).CurrentColour == Color4.White;
 
-            public bool IsUserBlurApplied() => ((FadeAccessibleBackground)Background).CurrentBlur == new Vector2((float)BlurLevel.Value * 25);
+            public bool IsUserBlurApplied() => ((FadeAccessibleBackground)Background).CurrentBlur == new Vector2((float)BlurLevel.Value * BackgroundScreenBeatmap.USER_BLUR_FACTOR);
 
             public bool IsUserBlurDisabled() => ((FadeAccessibleBackground)Background).CurrentBlur == new Vector2(0);
 
@@ -333,17 +335,7 @@ namespace osu.Game.Tests.Visual.Background
         {
             protected override BackgroundScreen CreateBackground() => new FadeAccessibleBackground(Beatmap.Value);
 
-            protected override UserDimContainer CreateStoryboardContainer()
-            {
-                return new TestUserDimContainer(true)
-                {
-                    RelativeSizeAxes = Axes.Both,
-                    Alpha = 1,
-                    EnableUserDim = { Value = true }
-                };
-            }
-
-            public UserDimContainer CurrentStoryboardContainer => StoryboardContainer;
+            public new DimmableStoryboard DimmableStoryboard => base.DimmableStoryboard;
 
             // Whether or not the player should be allowed to load.
             public bool BlockLoad;
@@ -357,9 +349,7 @@ namespace osu.Game.Tests.Visual.Background
             {
             }
 
-            public bool IsStoryboardVisible() => ((TestUserDimContainer)CurrentStoryboardContainer).CurrentAlpha == 1;
-
-            public bool IsStoryboardInvisible() => ((TestUserDimContainer)CurrentStoryboardContainer).CurrentAlpha <= 1;
+            public bool IsStoryboardVisible => DimmableStoryboard.ContentDisplayed;
 
             [BackgroundDependencyLoader]
             private void load(OsuConfigManager config, CancellationToken token)
@@ -392,15 +382,15 @@ namespace osu.Game.Tests.Visual.Background
 
         private class FadeAccessibleBackground : BackgroundScreenBeatmap
         {
-            protected override UserDimContainer CreateFadeContainer() => fadeContainer = new TestUserDimContainer { RelativeSizeAxes = Axes.Both };
+            protected override DimmableBackground CreateFadeContainer() => dimmable = new TestDimmableBackground { RelativeSizeAxes = Axes.Both };
 
-            public Color4 CurrentColour => fadeContainer.CurrentColour;
+            public Color4 CurrentColour => dimmable.CurrentColour;
 
-            public float CurrentAlpha => fadeContainer.CurrentAlpha;
+            public float CurrentAlpha => dimmable.CurrentAlpha;
 
             public Vector2 CurrentBlur => Background.BlurSigma;
 
-            private TestUserDimContainer fadeContainer;
+            private TestDimmableBackground dimmable;
 
             public FadeAccessibleBackground(WorkingBeatmap beatmap)
                 : base(beatmap)
@@ -408,15 +398,10 @@ namespace osu.Game.Tests.Visual.Background
             }
         }
 
-        private class TestUserDimContainer : UserDimContainer
+        private class TestDimmableBackground : BackgroundScreenBeatmap.DimmableBackground
         {
-            public Color4 CurrentColour => DimContainer.Colour;
-            public float CurrentAlpha => DimContainer.Alpha;
-
-            public TestUserDimContainer(bool isStoryboard = false)
-                : base(isStoryboard)
-            {
-            }
+            public Color4 CurrentColour => Content.Colour;
+            public float CurrentAlpha => Content.Alpha;
         }
     }
 }

osu.Game/Graphics/Containers/UserDimContainer.cs

@@ -1,31 +1,26 @@
 // 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.
 
-using System;
 using osu.Framework.Allocation;
 using osu.Framework.Bindables;
 using osu.Framework.Graphics;
 using osu.Framework.Graphics.Containers;
 using osu.Game.Configuration;
-using osu.Game.Graphics.Backgrounds;
-using osu.Game.Screens.Play;
-using osuTK;
 using osuTK.Graphics;
 
 namespace osu.Game.Graphics.Containers
 {
     /// <summary>
     /// A container that applies user-configured visual settings to its contents.
-    /// This container specifies behavior that applies to both Storyboards and Backgrounds.
     /// </summary>
-    public class UserDimContainer : Container
+    public abstract class UserDimContainer : Container
     {
-        private const float background_fade_duration = 800;
+        protected const float BACKGROUND_FADE_DURATION = 800;
 
         /// <summary>
         /// Whether or not user-configured dim levels should be applied to the container.
         /// </summary>
-        public readonly Bindable<bool> EnableUserDim = new Bindable<bool>();
+        public readonly Bindable<bool> EnableUserDim = new Bindable<bool>(true);
 
         /// <summary>
         /// Whether or not the storyboard loaded should completely hide the background behind it.
@@ -33,103 +28,58 @@ namespace osu.Game.Graphics.Containers
         public readonly Bindable<bool> StoryboardReplacesBackground = new Bindable<bool>();
 
         /// <summary>
-        /// The amount of blur to be applied to the background in addition to user-specified blur.
+        /// Whether the content of this container is currently being displayed.
         /// </summary>
-        /// <remarks>
-        /// Used in contexts where there can potentially be both user and screen-specified blurring occuring at the same time, such as in <see cref="PlayerLoader"/>
-        /// </remarks>
-        public readonly Bindable<float> BlurAmount = new Bindable<float>();
+        public bool ContentDisplayed { get; private set; }
 
-        private Bindable<double> userDimLevel { get; set; }
+        protected Bindable<double> UserDimLevel { get; private set; }
 
-        private Bindable<double> userBlurLevel { get; set; }
+        protected Bindable<bool> ShowStoryboard { get; private set; }
 
-        private Bindable<bool> showStoryboard { get; set; }
+        protected override Container<Drawable> Content => dimContent;
 
-        protected Container DimContainer { get; }
-
-        protected override Container<Drawable> Content => DimContainer;
-
-        private readonly bool isStoryboard;
-
-        /// <summary>
-        /// As an optimisation, we add the two blur portions to be applied rather than actually applying two separate blurs.
-        /// </summary>
-        private Vector2 blurTarget => EnableUserDim.Value
-            ? new Vector2(BlurAmount.Value + (float)userBlurLevel.Value * 25)
-            : new Vector2(BlurAmount.Value);
+        private Container dimContent { get; }
 
         /// <summary>
         /// Creates a new <see cref="UserDimContainer"/>.
         /// </summary>
-        /// <param name="isStoryboard"> Whether or not this instance contains a storyboard.
-        /// <remarks>
-        /// While both backgrounds and storyboards allow user dim levels to be applied, storyboards can be toggled via <see cref="showStoryboard"/>
-        /// and can cause backgrounds to become hidden via <see cref="StoryboardReplacesBackground"/>. Storyboards are also currently unable to be blurred.
-        /// </remarks>
-        /// </param>
-        public UserDimContainer(bool isStoryboard = false)
+        protected UserDimContainer()
         {
-            this.isStoryboard = isStoryboard;
-            AddInternal(DimContainer = new Container { RelativeSizeAxes = Axes.Both });
-        }
-
-        private Background background;
-
-        public Background Background
-        {
-            get => background;
-            set
-            {
-                base.Add(background = value);
-                background.BlurTo(blurTarget, 0, Easing.OutQuint);
-            }
-        }
-
-        public override void Add(Drawable drawable)
-        {
-            if (drawable is Background)
-                throw new InvalidOperationException($"Use {nameof(Background)} to set a background.");
-
-            base.Add(drawable);
+            AddInternal(dimContent = new Container { RelativeSizeAxes = Axes.Both });
         }
 
         [BackgroundDependencyLoader]
         private void load(OsuConfigManager config)
         {
-            userDimLevel = config.GetBindable<double>(OsuSetting.DimLevel);
-            userBlurLevel = config.GetBindable<double>(OsuSetting.BlurLevel);
-            showStoryboard = config.GetBindable<bool>(OsuSetting.ShowStoryboard);
+            UserDimLevel = config.GetBindable<double>(OsuSetting.DimLevel);
+            ShowStoryboard = config.GetBindable<bool>(OsuSetting.ShowStoryboard);
 
-            EnableUserDim.ValueChanged += _ => updateVisuals();
-            userDimLevel.ValueChanged += _ => updateVisuals();
-            userBlurLevel.ValueChanged += _ => updateVisuals();
-            showStoryboard.ValueChanged += _ => updateVisuals();
-            StoryboardReplacesBackground.ValueChanged += _ => updateVisuals();
-            BlurAmount.ValueChanged += _ => updateVisuals();
+            EnableUserDim.ValueChanged += _ => UpdateVisuals();
+            UserDimLevel.ValueChanged += _ => UpdateVisuals();
+            ShowStoryboard.ValueChanged += _ => UpdateVisuals();
+            StoryboardReplacesBackground.ValueChanged += _ => UpdateVisuals();
         }
 
         protected override void LoadComplete()
         {
             base.LoadComplete();
-            updateVisuals();
+            UpdateVisuals();
         }
 
-        private void updateVisuals()
+        /// <summary>
+        /// Whether the content of this container should currently be visible.
+        /// </summary>
+        protected virtual bool ShowDimContent => true;
+
+        /// <summary>
+        /// Should be invoked when any dependent dim level or user setting is changed and bring the visual state up-to-date.
+        /// </summary>
+        protected virtual void UpdateVisuals()
         {
-            if (isStoryboard)
-            {
-                DimContainer.FadeTo(!showStoryboard.Value || userDimLevel.Value == 1 ? 0 : 1, background_fade_duration, Easing.OutQuint);
-            }
-            else
-            {
-                // The background needs to be hidden in the case of it being replaced by the storyboard
-                DimContainer.FadeTo(showStoryboard.Value && StoryboardReplacesBackground.Value ? 0 : 1, background_fade_duration, Easing.OutQuint);
+            ContentDisplayed = ShowDimContent;
 
-                Background?.BlurTo(blurTarget, background_fade_duration, Easing.OutQuint);
-            }
-
-            DimContainer.FadeColour(EnableUserDim.Value ? OsuColour.Gray(1 - (float)userDimLevel.Value) : Color4.White, background_fade_duration, Easing.OutQuint);
+            dimContent.FadeTo((ContentDisplayed) ? 1 : 0, BACKGROUND_FADE_DURATION, Easing.OutQuint);
+            dimContent.FadeColour(EnableUserDim.Value ? OsuColour.Gray(1 - (float)UserDimLevel.Value) : Color4.White, BACKGROUND_FADE_DURATION, Easing.OutQuint);
         }
     }
 }

osu.Game/Screens/Backgrounds/BackgroundScreenBeatmap.cs

@@ -1,19 +1,28 @@
 // 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.
 
+using System;
 using System.Threading;
 using osu.Framework.Allocation;
 using osu.Framework.Bindables;
 using osu.Framework.Graphics;
 using osu.Framework.Graphics.Textures;
 using osu.Game.Beatmaps;
+using osu.Game.Configuration;
 using osu.Game.Graphics.Backgrounds;
 using osu.Game.Graphics.Containers;
+using osu.Game.Screens.Play;
+using osuTK;
 
 namespace osu.Game.Screens.Backgrounds
 {
     public class BackgroundScreenBeatmap : BackgroundScreen
     {
+        /// <summary>
+        /// The amount of blur to apply when full user blur is requested.
+        /// </summary>
+        public const float USER_BLUR_FACTOR = 25;
+
         protected Background Background;
 
         private WorkingBeatmap beatmap;
@@ -30,16 +39,17 @@ namespace osu.Game.Screens.Backgrounds
         /// </summary>
         public readonly Bindable<float> BlurAmount = new Bindable<float>();
 
-        private readonly UserDimContainer fadeContainer;
+        private readonly DimmableBackground dimmable;
 
-        protected virtual UserDimContainer CreateFadeContainer() => new UserDimContainer { RelativeSizeAxes = Axes.Both };
+        protected virtual DimmableBackground CreateFadeContainer() => new DimmableBackground { RelativeSizeAxes = Axes.Both };
 
         public BackgroundScreenBeatmap(WorkingBeatmap beatmap = null)
         {
             Beatmap = beatmap;
-            InternalChild = fadeContainer = CreateFadeContainer();
-            fadeContainer.EnableUserDim.BindTo(EnableUserDim);
-            fadeContainer.BlurAmount.BindTo(BlurAmount);
+
+            InternalChild = dimmable = CreateFadeContainer();
+            dimmable.EnableUserDim.BindTo(EnableUserDim);
+            dimmable.BlurAmount.BindTo(BlurAmount);
         }
 
         [BackgroundDependencyLoader]
@@ -86,8 +96,8 @@ namespace osu.Game.Screens.Backgrounds
             }
 
             b.Depth = newDepth;
-            fadeContainer.Background = Background = b;
-            StoryboardReplacesBackground.BindTo(fadeContainer.StoryboardReplacesBackground);
+            dimmable.Background = Background = b;
+            StoryboardReplacesBackground.BindTo(dimmable.StoryboardReplacesBackground);
         }
 
         public override bool Equals(BackgroundScreen other)
@@ -112,5 +122,64 @@ namespace osu.Game.Screens.Backgrounds
                 Sprite.Texture = Beatmap?.Background ?? textures.Get(@"Backgrounds/bg1");
             }
         }
+
+        public class DimmableBackground : UserDimContainer
+        {
+            /// <summary>
+            /// The amount of blur to be applied to the background in addition to user-specified blur.
+            /// </summary>
+            /// <remarks>
+            /// Used in contexts where there can potentially be both user and screen-specified blurring occuring at the same time, such as in <see cref="PlayerLoader"/>
+            /// </remarks>
+            public readonly Bindable<float> BlurAmount = new Bindable<float>();
+
+            public Background Background
+            {
+                get => background;
+                set
+                {
+                    background?.Expire();
+
+                    base.Add(background = value);
+                    background.BlurTo(blurTarget, 0, Easing.OutQuint);
+                }
+            }
+
+            private Bindable<double> userBlurLevel { get; set; }
+
+            private Background background;
+
+            public override void Add(Drawable drawable)
+            {
+                if (drawable is Background)
+                    throw new InvalidOperationException($"Use {nameof(Background)} to set a background.");
+
+                base.Add(drawable);
+            }
+
+            /// <summary>
+            /// As an optimisation, we add the two blur portions to be applied rather than actually applying two separate blurs.
+            /// </summary>
+            private Vector2 blurTarget => EnableUserDim.Value
+                ? new Vector2(BlurAmount.Value + (float)userBlurLevel.Value * USER_BLUR_FACTOR)
+                : new Vector2(BlurAmount.Value);
+
+            [BackgroundDependencyLoader]
+            private void load(OsuConfigManager config)
+            {
+                userBlurLevel = config.GetBindable<double>(OsuSetting.BlurLevel);
+                userBlurLevel.ValueChanged += _ => UpdateVisuals();
+                BlurAmount.ValueChanged += _ => UpdateVisuals();
+            }
+
+            protected override bool ShowDimContent => !ShowStoryboard.Value || !StoryboardReplacesBackground.Value; // The background needs to be hidden in the case of it being replaced by the storyboard
+
+            protected override void UpdateVisuals()
+            {
+                base.UpdateVisuals();
+
+                Background?.BlurTo(blurTarget, BACKGROUND_FADE_DURATION, Easing.OutQuint);
+            }
+        }
     }
 }

osu.Game/Screens/Play/DimmableStoryboard.cs

@@ -0,0 +1,55 @@
+// 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.
+
+using osu.Framework.Allocation;
+using osu.Game.Graphics.Containers;
+using osu.Game.Storyboards;
+using osu.Game.Storyboards.Drawables;
+
+namespace osu.Game.Screens.Play
+{
+    /// <summary>
+    /// A container that handles <see cref="Storyboard"/> loading, as well as applies user-specified visual settings to it.
+    /// </summary>
+    public class DimmableStoryboard : UserDimContainer
+    {
+        private readonly Storyboard storyboard;
+        private DrawableStoryboard drawableStoryboard;
+
+        public DimmableStoryboard(Storyboard storyboard)
+        {
+            this.storyboard = storyboard;
+        }
+
+        [BackgroundDependencyLoader]
+        private void load()
+        {
+            initializeStoryboard(false);
+        }
+
+        protected override void LoadComplete()
+        {
+            ShowStoryboard.BindValueChanged(_ => initializeStoryboard(true), true);
+            base.LoadComplete();
+        }
+
+        protected override bool ShowDimContent => ShowStoryboard.Value && UserDimLevel.Value < 1;
+
+        private void initializeStoryboard(bool async)
+        {
+            if (drawableStoryboard != null)
+                return;
+
+            if (!ShowStoryboard.Value)
+                return;
+
+            drawableStoryboard = storyboard.CreateDrawable();
+            drawableStoryboard.Masking = true;
+
+            if (async)
+                LoadComponentAsync(drawableStoryboard, Add);
+            else
+                Add(drawableStoryboard);
+        }
+    }
+}

osu.Game/Screens/Play/Player.cs

@@ -26,7 +26,6 @@ using osu.Game.Rulesets.UI;
 using osu.Game.Scoring;
 using osu.Game.Screens.Ranking;
 using osu.Game.Skinning;
-using osu.Game.Storyboards.Drawables;
 using osu.Game.Users;
 
 namespace osu.Game.Screens.Play
@@ -76,6 +75,8 @@ namespace osu.Game.Screens.Play
 
         protected GameplayClockContainer GameplayClockContainer { get; private set; }
 
+        protected DimmableStoryboard DimmableStoryboard { get; private set; }
+
         [Cached]
         [Cached(Type = typeof(IBindable<IReadOnlyList<Mod>>))]
         protected new readonly Bindable<IReadOnlyList<Mod>> Mods = new Bindable<IReadOnlyList<Mod>>(Array.Empty<Mod>());
@@ -109,7 +110,6 @@ namespace osu.Game.Screens.Play
             sampleRestart = audio.Samples.Get(@"Gameplay/restart");
 
             mouseWheelDisabled = config.GetBindable<bool>(OsuSetting.MouseDisableWheel);
-            showStoryboard = config.GetBindable<bool>(OsuSetting.ShowStoryboard);
 
             ScoreProcessor = DrawableRuleset.CreateScoreProcessor();
             ScoreProcessor.Mods.BindTo(Mods);
@@ -121,7 +121,7 @@ namespace osu.Game.Screens.Play
 
             GameplayClockContainer.Children = new[]
             {
-                StoryboardContainer = CreateStoryboardContainer(),
+                DimmableStoryboard = new DimmableStoryboard(Beatmap.Value.Storyboard) { RelativeSizeAxes = Axes.Both },
                 new ScalingContainer(ScalingMode.Gameplay)
                 {
                     Child = new LocalSkinOverrideContainer(working.Skin)
@@ -199,9 +199,6 @@ namespace osu.Game.Screens.Play
             // bind clock into components that require it
             DrawableRuleset.IsPaused.BindTo(GameplayClockContainer.IsPaused);
 
-            // load storyboard as part of player's load if we can
-            initializeStoryboard(false);
-
             // Bind ScoreProcessor to ourselves
             ScoreProcessor.AllJudged += onCompletion;
             ScoreProcessor.Failed += onFail;
@@ -334,41 +331,6 @@ namespace osu.Game.Screens.Play
 
         protected virtual Results CreateResults(ScoreInfo score) => new SoloResults(score);
 
-        #region Storyboard
-
-        private DrawableStoryboard storyboard;
-        protected UserDimContainer StoryboardContainer { get; private set; }
-
-        protected virtual UserDimContainer CreateStoryboardContainer() => new UserDimContainer(true)
-        {
-            RelativeSizeAxes = Axes.Both,
-            Alpha = 1,
-            EnableUserDim = { Value = true }
-        };
-
-        private Bindable<bool> showStoryboard;
-
-        private void initializeStoryboard(bool asyncLoad)
-        {
-            if (StoryboardContainer == null || storyboard != null)
-                return;
-
-            if (!showStoryboard.Value)
-                return;
-
-            var beatmap = Beatmap.Value;
-
-            storyboard = beatmap.Storyboard.CreateDrawable();
-            storyboard.Masking = true;
-
-            if (asyncLoad)
-                LoadComponentAsync(storyboard, StoryboardContainer.Add);
-            else
-                StoryboardContainer.Add(storyboard);
-        }
-
-        #endregion
-
         #region Fail Logic
 
         protected FailOverlay FailOverlay { get; private set; }
@@ -486,13 +448,11 @@ namespace osu.Game.Screens.Play
                 .Delay(250)
                 .FadeIn(250);
 
-            showStoryboard.ValueChanged += _ => initializeStoryboard(true);
-
             Background.EnableUserDim.Value = true;
             Background.BlurAmount.Value = 0;
 
             Background.StoryboardReplacesBackground.BindTo(storyboardReplacesBackground);
-            StoryboardContainer.StoryboardReplacesBackground.BindTo(storyboardReplacesBackground);
+            DimmableStoryboard.StoryboardReplacesBackground.BindTo(storyboardReplacesBackground);
 
             storyboardReplacesBackground.Value = Beatmap.Value.Storyboard.ReplacesBackground && Beatmap.Value.Storyboard.HasDrawable;