Merge pull request #13894 from peppy/fix-beat-synced-container-alt

Fix multiple issues with `BeatSyncedContainer`

osu.Game.Tests/Visual/UserInterface/TestSceneBeatSyncedContainer.cs

@@ -5,18 +5,19 @@ using System;
 using System.Collections.Generic;
 using System.Linq;
 using NUnit.Framework;
-using osu.Framework.Allocation;
 using osu.Framework.Audio.Track;
 using osu.Framework.Extensions.Color4Extensions;
 using osu.Framework.Graphics;
 using osu.Framework.Graphics.Containers;
 using osu.Framework.Graphics.Shapes;
-using osu.Framework.Timing;
+using osu.Framework.Testing;
+using osu.Framework.Utils;
 using osu.Game.Beatmaps.ControlPoints;
 using osu.Game.Graphics;
 using osu.Game.Graphics.Containers;
 using osu.Game.Graphics.Sprites;
-using osu.Game.Overlays;
+using osu.Game.Rulesets.Osu;
+using osu.Game.Screens.Play;
 using osuTK.Graphics;
 
 namespace osu.Game.Tests.Visual.UserInterface
@@ -24,37 +25,125 @@ namespace osu.Game.Tests.Visual.UserInterface
     [TestFixture]
     public class TestSceneBeatSyncedContainer : OsuTestScene
     {
-        private readonly NowPlayingOverlay np;
+        private TestBeatSyncedContainer beatContainer;
 
-        public TestSceneBeatSyncedContainer()
-        {
-            Clock = new FramedClock();
-            Clock.ProcessFrame();
+        private MasterGameplayClockContainer gameplayClockContainer;
 
-            AddRange(new Drawable[]
+        [SetUpSteps]
+        public void SetUpSteps()
         {
-                new BeatContainer
+            AddStep("Set beatmap", () =>
+            {
+                Beatmap.Value = CreateWorkingBeatmap(new OsuRuleset().RulesetInfo);
+            });
+
+            AddStep("Create beat sync container", () =>
+            {
+                Children = new Drawable[]
+                {
+                    gameplayClockContainer = new MasterGameplayClockContainer(Beatmap.Value, 0)
+                    {
+                        Child = beatContainer = new TestBeatSyncedContainer
                         {
                             Anchor = Anchor.BottomCentre,
                             Origin = Anchor.BottomCentre,
                         },
-                np = new NowPlayingOverlay
-                {
-                    Origin = Anchor.TopRight,
-                    Anchor = Anchor.TopRight,
                     }
+                };
             });
+
+            AddStep("Start playback", () => gameplayClockContainer.Start());
         }
 
-        protected override void LoadComplete()
+        [TestCase(false)]
+        [TestCase(true)]
+        public void TestDisallowMistimedEventFiring(bool allowMistimed)
         {
-            base.LoadComplete();
-            np.ToggleVisibility();
+            int? lastBeatIndex = null;
+            double? lastActuationTime = null;
+            TimingControlPoint lastTimingPoint = null;
+
+            AddStep($"set mistimed to {(allowMistimed ? "allowed" : "disallowed")}", () => beatContainer.AllowMistimedEventFiring = allowMistimed);
+
+            AddStep("Set time before zero", () =>
+            {
+                beatContainer.NewBeat = (i, timingControlPoint, effectControlPoint, channelAmplitudes) =>
+                {
+                    lastActuationTime = gameplayClockContainer.CurrentTime;
+                    lastTimingPoint = timingControlPoint;
+                    lastBeatIndex = i;
+                    beatContainer.NewBeat = null;
+                };
+
+                gameplayClockContainer.Seek(-1000);
+            });
+
+            AddUntilStep("wait for trigger", () => lastBeatIndex != null);
+
+            if (!allowMistimed)
+            {
+                AddAssert("trigger is near beat length", () => lastActuationTime != null && lastBeatIndex != null && Precision.AlmostEquals(lastTimingPoint.Time + lastBeatIndex.Value * lastTimingPoint.BeatLength, lastActuationTime.Value, BeatSyncedContainer.MISTIMED_ALLOWANCE));
+            }
+            else
+            {
+                AddAssert("trigger is not near beat length", () => lastActuationTime != null && lastBeatIndex != null && !Precision.AlmostEquals(lastTimingPoint.Time + lastBeatIndex.Value * lastTimingPoint.BeatLength, lastActuationTime.Value, BeatSyncedContainer.MISTIMED_ALLOWANCE));
+            }
         }
 
-        private class BeatContainer : BeatSyncedContainer
+        [Test]
+        public void TestNegativeBeatsStillUsingBeatmapTiming()
         {
-            private const int flash_layer_heigth = 150;
+            int? lastBeatIndex = null;
+            double? lastBpm = null;
+
+            AddStep("Set time before zero", () =>
+            {
+                beatContainer.NewBeat = (i, timingControlPoint, effectControlPoint, channelAmplitudes) =>
+                {
+                    lastBeatIndex = i;
+                    lastBpm = timingControlPoint.BPM;
+                };
+
+                gameplayClockContainer.Seek(-1000);
+            });
+
+            AddUntilStep("wait for trigger", () => lastBpm != null);
+            AddAssert("bpm is from beatmap", () => lastBpm != null && Precision.AlmostEquals(lastBpm.Value, 128));
+            AddAssert("beat index is less than zero", () => lastBeatIndex < 0);
+        }
+
+        [Test]
+        public void TestIdleBeatOnPausedClock()
+        {
+            double? lastBpm = null;
+
+            AddStep("bind event", () =>
+            {
+                beatContainer.NewBeat = (i, timingControlPoint, effectControlPoint, channelAmplitudes) => lastBpm = timingControlPoint.BPM;
+            });
+
+            AddUntilStep("wait for trigger", () => lastBpm != null);
+            AddAssert("bpm is from beatmap", () => lastBpm != null && Precision.AlmostEquals(lastBpm.Value, 128));
+
+            AddStep("pause gameplay clock", () =>
+            {
+                lastBpm = null;
+                gameplayClockContainer.Stop();
+            });
+
+            AddUntilStep("wait for trigger", () => lastBpm != null);
+            AddAssert("bpm is default", () => lastBpm != null && Precision.AlmostEquals(lastBpm.Value, 60));
+        }
+
+        private class TestBeatSyncedContainer : BeatSyncedContainer
+        {
+            private const int flash_layer_height = 150;
+
+            public new bool AllowMistimedEventFiring
+            {
+                get => base.AllowMistimedEventFiring;
+                set => base.AllowMistimedEventFiring = value;
+            }
 
             private readonly InfoString timingPointCount;
             private readonly InfoString currentTimingPoint;
@@ -64,13 +153,11 @@ namespace osu.Game.Tests.Visual.UserInterface
             private readonly InfoString adjustedBeatLength;
             private readonly InfoString timeUntilNextBeat;
             private readonly InfoString timeSinceLastBeat;
+            private readonly InfoString currentTime;
 
             private readonly Box flashLayer;
 
-            [Resolved]
-            private MusicController musicController { get; set; }
-
-            public BeatContainer()
+            public TestBeatSyncedContainer()
             {
                 RelativeSizeAxes = Axes.X;
                 AutoSizeAxes = Axes.Y;
@@ -82,7 +169,7 @@ namespace osu.Game.Tests.Visual.UserInterface
                         Anchor = Anchor.BottomLeft,
                         Origin = Anchor.BottomLeft,
                         AutoSizeAxes = Axes.Both,
-                        Margin = new MarginPadding { Bottom = flash_layer_heigth },
+                        Margin = new MarginPadding { Bottom = flash_layer_height },
                         Children = new Drawable[]
                         {
                             new Box
@@ -98,6 +185,7 @@ namespace osu.Game.Tests.Visual.UserInterface
                                 Direction = FillDirection.Vertical,
                                 Children = new Drawable[]
                                 {
+                                    currentTime = new InfoString(@"Current time"),
                                     timingPointCount = new InfoString(@"Timing points amount"),
                                     currentTimingPoint = new InfoString(@"Current timing point"),
                                     beatCount = new InfoString(@"Beats amount (in the current timing point)"),
@@ -116,7 +204,7 @@ namespace osu.Game.Tests.Visual.UserInterface
                         Anchor = Anchor.BottomCentre,
                         Origin = Anchor.BottomCentre,
                         RelativeSizeAxes = Axes.X,
-                        Height = flash_layer_heigth,
+                        Height = flash_layer_height,
                         Children = new Drawable[]
                         {
                             new Box
@@ -133,8 +221,13 @@ namespace osu.Game.Tests.Visual.UserInterface
                         }
                     }
                 };
+            }
 
-                Beatmap.ValueChanged += delegate
+            protected override void LoadComplete()
+            {
+                base.LoadComplete();
+
+                Beatmap.BindValueChanged(_ =>
                 {
                     timingPointCount.Value = 0;
                     currentTimingPoint.Value = 0;
@@ -144,7 +237,7 @@ namespace osu.Game.Tests.Visual.UserInterface
                     adjustedBeatLength.Value = 0;
                     timeUntilNextBeat.Value = 0;
                     timeSinceLastBeat.Value = 0;
-                };
+                }, true);
             }
 
             private List<TimingControlPoint> timingPoints => Beatmap.Value.Beatmap.ControlPointInfo.TimingPoints.ToList();
@@ -164,7 +257,7 @@ namespace osu.Game.Tests.Visual.UserInterface
                 if (timingPoints.Count == 0) return 0;
 
                 if (timingPoints[^1] == current)
-                    return (int)Math.Ceiling((musicController.CurrentTrack.Length - current.Time) / current.BeatLength);
+                    return (int)Math.Ceiling((BeatSyncClock.CurrentTime - current.Time) / current.BeatLength);
 
                 return (int)Math.Ceiling((getNextTimingPoint(current).Time - current.Time) / current.BeatLength);
             }
@@ -174,8 +267,11 @@ namespace osu.Game.Tests.Visual.UserInterface
                 base.Update();
                 timeUntilNextBeat.Value = TimeUntilNextBeat;
                 timeSinceLastBeat.Value = TimeSinceLastBeat;
+                currentTime.Value = BeatSyncClock.CurrentTime;
             }
 
+            public Action<int, TimingControlPoint, EffectControlPoint, ChannelAmplitudes> NewBeat;
+
             protected override void OnNewBeat(int beatIndex, TimingControlPoint timingPoint, EffectControlPoint effectPoint, ChannelAmplitudes amplitudes)
             {
                 base.OnNewBeat(beatIndex, timingPoint, effectPoint, amplitudes);
@@ -187,7 +283,9 @@ namespace osu.Game.Tests.Visual.UserInterface
                 beatsPerMinute.Value = 60000 / timingPoint.BeatLength;
                 adjustedBeatLength.Value = timingPoint.BeatLength;
 
-                flashLayer.FadeOutFromOne(timingPoint.BeatLength);
+                flashLayer.FadeOutFromOne(timingPoint.BeatLength / 4);
+
+                NewBeat?.Invoke(beatIndex, timingPoint, effectPoint, amplitudes);
             }
         }
 
@@ -200,7 +298,7 @@ namespace osu.Game.Tests.Visual.UserInterface
 
             public double Value
             {
-                set => valueText.Text = $"{value:G}";
+                set => valueText.Text = $"{value:0.##}";
             }
 
             public InfoString(string header)

osu.Game/Graphics/Containers/BeatSyncedContainer.cs

@@ -1,19 +1,32 @@
 // 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.Diagnostics;
 using osu.Framework.Allocation;
 using osu.Framework.Audio.Track;
 using osu.Framework.Bindables;
 using osu.Framework.Graphics.Containers;
+using osu.Framework.Timing;
 using osu.Game.Beatmaps;
 using osu.Game.Beatmaps.ControlPoints;
+using osu.Game.Screens.Play;
 
 namespace osu.Game.Graphics.Containers
 {
+    /// <summary>
+    /// A container which fires a callback when a new beat is reached.
+    /// Consumes a parent <see cref="GameplayClock"/> or <see cref="Beatmap"/> (whichever is first available).
+    /// </summary>
+    /// <remarks>
+    /// This container does not set its own clock to the source used for beat matching.
+    /// This means that if the beat source clock is playing faster or slower, animations may unexpectedly overlap.
+    /// Make sure this container's Clock is also set to the expected source (or within a parent element which provides this).
+    ///
+    /// This container will also trigger beat events when the beat matching clock is paused at <see cref="TimingControlPoint.DEFAULT"/>'s BPM.
+    /// </remarks>
     public class BeatSyncedContainer : Container
     {
-        protected readonly IBindable<WorkingBeatmap> Beatmap = new Bindable<WorkingBeatmap>();
-
         private int lastBeat;
         private TimingControlPoint lastTimingPoint;
 
@@ -23,6 +36,19 @@ namespace osu.Game.Graphics.Containers
         /// </summary>
         protected double EarlyActivationMilliseconds;
 
+        /// <summary>
+        /// While this container automatically applied an animation delay (meaning any animations inside a <see cref="OnNewBeat"/> implementation will
+        /// always be correctly timed), the event itself can potentially fire away from the related beat.
+        ///
+        /// By setting this to false, cases where the event is to be fired more than <see cref="MISTIMED_ALLOWANCE"/> from the related beat will be skipped.
+        /// </summary>
+        protected bool AllowMistimedEventFiring = true;
+
+        /// <summary>
+        /// The maximum deviance from the actual beat that an <see cref="OnNewBeat"/> can fire when <see cref="AllowMistimedEventFiring"/> is set to false.
+        /// </summary>
+        public const double MISTIMED_ALLOWANCE = 16;
+
         /// <summary>
         /// The time in milliseconds until the next beat.
         /// </summary>
@@ -43,16 +69,49 @@ namespace osu.Game.Graphics.Containers
         /// </summary>
         public double MinimumBeatLength { get; set; }
 
+        /// <summary>
+        /// Whether this container is currently tracking a beatmap's timing data.
+        /// </summary>
         protected bool IsBeatSyncedWithTrack { get; private set; }
 
+        protected virtual void OnNewBeat(int beatIndex, TimingControlPoint timingPoint, EffectControlPoint effectPoint, ChannelAmplitudes amplitudes)
+        {
+        }
+
+        [Resolved]
+        protected IBindable<WorkingBeatmap> Beatmap { get; private set; }
+
+        [Resolved(canBeNull: true)]
+        protected GameplayClock GameplayClock { get; private set; }
+
+        protected IClock BeatSyncClock
+        {
+            get
+            {
+                if (GameplayClock != null)
+                    return GameplayClock;
+
+                if (Beatmap.Value.TrackLoaded)
+                    return Beatmap.Value.Track;
+
+                return null;
+            }
+        }
+
         protected override void Update()
         {
             ITrack track = null;
             IBeatmap beatmap = null;
 
-            double currentTrackTime = 0;
-            TimingControlPoint timingPoint = null;
-            EffectControlPoint effectPoint = null;
+            TimingControlPoint timingPoint;
+            EffectControlPoint effectPoint;
+
+            IClock clock = BeatSyncClock;
+
+            if (clock == null)
+                return;
+
+            double currentTrackTime = clock.CurrentTime;
 
             if (Beatmap.Value.TrackLoaded && Beatmap.Value.BeatmapLoaded)
             {
@@ -60,23 +119,26 @@ namespace osu.Game.Graphics.Containers
                 beatmap = Beatmap.Value.Beatmap;
             }
 
-            if (track != null && beatmap != null && track.IsRunning && track.Length > 0)
+            IsBeatSyncedWithTrack = beatmap != null && clock.IsRunning && track?.Length > 0;
+
+            if (IsBeatSyncedWithTrack)
             {
-                currentTrackTime = track.CurrentTime + EarlyActivationMilliseconds;
+                Debug.Assert(beatmap != null);
 
                 timingPoint = beatmap.ControlPointInfo.TimingPointAt(currentTrackTime);
                 effectPoint = beatmap.ControlPointInfo.EffectPointAt(currentTrackTime);
             }
-
-            IsBeatSyncedWithTrack = timingPoint?.BeatLength > 0;
-
-            if (timingPoint == null || !IsBeatSyncedWithTrack)
+            else
             {
+                // this may be the case where the beat syncing clock has been paused.
+                // we still want to show an idle animation, so use this container's time instead.
                 currentTrackTime = Clock.CurrentTime;
                 timingPoint = TimingControlPoint.DEFAULT;
                 effectPoint = EffectControlPoint.DEFAULT;
             }
 
+            currentTrackTime += EarlyActivationMilliseconds;
+
             double beatLength = timingPoint.BeatLength / Divisor;
 
             while (beatLength < MinimumBeatLength)
@@ -89,7 +151,7 @@ namespace osu.Game.Graphics.Containers
                 beatIndex--;
 
             TimeUntilNextBeat = (timingPoint.Time - currentTrackTime) % beatLength;
-            if (TimeUntilNextBeat < 0)
+            if (TimeUntilNextBeat <= 0)
                 TimeUntilNextBeat += beatLength;
 
             TimeSinceLastBeat = beatLength - TimeUntilNextBeat;
@@ -97,21 +159,16 @@ namespace osu.Game.Graphics.Containers
             if (timingPoint == lastTimingPoint && beatIndex == lastBeat)
                 return;
 
+            // as this event is sometimes used for sound triggers where `BeginDelayedSequence` has no effect, avoid firing it if too far away from the beat.
+            // this can happen after a seek operation.
+            if (AllowMistimedEventFiring || Math.Abs(TimeSinceLastBeat) < MISTIMED_ALLOWANCE)
+            {
                 using (BeginDelayedSequence(-TimeSinceLastBeat))
                     OnNewBeat(beatIndex, timingPoint, effectPoint, track?.CurrentAmplitudes ?? ChannelAmplitudes.Empty);
+            }
 
             lastBeat = beatIndex;
             lastTimingPoint = timingPoint;
         }
-
-        [BackgroundDependencyLoader]
-        private void load(IBindable<WorkingBeatmap> beatmap)
-        {
-            Beatmap.BindTo(beatmap);
-        }
-
-        protected virtual void OnNewBeat(int beatIndex, TimingControlPoint timingPoint, EffectControlPoint effectPoint, ChannelAmplitudes amplitudes)
-        {
-        }
     }
 }

osu.Game/Screens/Menu/OsuLogo.cs

@@ -72,8 +72,6 @@ namespace osu.Game.Screens.Menu
             set => colourAndTriangles.FadeTo(value ? 1 : 0, transition_length, Easing.OutQuint);
         }
 
-        public bool BeatMatching = true;
-
         public override bool ReceivePositionalInputAt(Vector2 screenSpacePos) => logoContainer.ReceivePositionalInputAt(screenSpacePos);
 
         public bool Ripple
@@ -272,8 +270,6 @@ namespace osu.Game.Screens.Menu
         {
             base.OnNewBeat(beatIndex, timingPoint, effectPoint, amplitudes);
 
-            if (!BeatMatching) return;
-
             lastBeatIndex = beatIndex;
 
             var beatLength = timingPoint.BeatLength;

osu.Game/Screens/OsuScreen.cs

@@ -242,7 +242,6 @@ namespace osu.Game.Screens
             logo.Anchor = Anchor.TopLeft;
             logo.Origin = Anchor.Centre;
             logo.RelativePositionAxes = Axes.Both;
-            logo.BeatMatching = true;
             logo.Triangles = true;
             logo.Ripple = true;
         }