Refactor TrueGameplayRate to account for only gameplay adjustments, no matter what

[peppy]

RFC

Closes #20146

[frenzibyte]

Coming from the discussion in Discord, I've imagined doing this a bit differently by exposing an AudioAdjustments instance that's used for applying gameplay adjustments and sourcing the rate from there immediately, without implementing the interface on MasterGameplayClockContainer.

Here's a quick diff I've came up with, haven't tested if it works yet:

diff

diff --git a/osu.Game.Tests/NonVisual/GameplayClockContainerTest.cs b/osu.Game.Tests/NonVisual/GameplayClockContainerTest.cs
index 95bf1ab354..80f0aaeb55 100644
--- a/osu.Game.Tests/NonVisual/GameplayClockContainerTest.cs
+++ b/osu.Game.Tests/NonVisual/GameplayClockContainerTest.cs
@@ -1,8 +1,9 @@
 // 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.Collections.Generic;
 using NUnit.Framework;
+using osu.Framework.Audio;
+using osu.Framework.Bindables;
 using osu.Framework.Timing;
 using osu.Game.Screens.Play;
 
@@ -23,11 +24,10 @@ public void TestTrueGameplayRateWithGameplayAdjustment(double underlyingClockRat
 
         private class TestGameplayClockContainer : GameplayClockContainer
         {
-            public override IEnumerable<double> GameplayAdjustments => new[] { 2.0 };
-
             public TestGameplayClockContainer(IFrameBasedClock underlyingClock)
                 : base(underlyingClock)
             {
+                GameplayAdjustments.AddAdjustment(AdjustableProperty.Frequency, new BindableDouble(2.0));
             }
         }
     }
diff --git a/osu.Game/Rulesets/UI/FrameStabilityContainer.cs b/osu.Game/Rulesets/UI/FrameStabilityContainer.cs
index 4f4a2d908d..f0c7a398eb 100644
--- a/osu.Game/Rulesets/UI/FrameStabilityContainer.cs
+++ b/osu.Game/Rulesets/UI/FrameStabilityContainer.cs
@@ -2,10 +2,9 @@
 // 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 osu.Framework.Allocation;
+using osu.Framework.Audio;
 using osu.Framework.Bindables;
 using osu.Framework.Graphics;
 using osu.Framework.Graphics.Containers;
@@ -264,7 +263,9 @@ private void applyFrameStability(ref double proposedTime)
 
         public double StartTime => parentGameplayClock?.StartTime ?? 0;
 
-        public IEnumerable<double> GameplayAdjustments => parentGameplayClock?.GameplayAdjustments ?? Enumerable.Empty<double>();
+        private readonly AudioAdjustments gameplayAdjustments = new AudioAdjustments();
+
+        public IAdjustableAudioComponent GameplayAdjustments => parentGameplayClock?.GameplayAdjustments ?? gameplayAdjustments;
 
         #endregion
 
diff --git a/osu.Game/Screens/Play/GameplayClockContainer.cs b/osu.Game/Screens/Play/GameplayClockContainer.cs
index 5dfaf2d584..e64c628fa0 100644
--- a/osu.Game/Screens/Play/GameplayClockContainer.cs
+++ b/osu.Game/Screens/Play/GameplayClockContainer.cs
@@ -2,9 +2,8 @@
 // See the LICENCE file in the repository root for full licence text.
 
 using System;
-using System.Collections.Generic;
-using System.Linq;
 using osu.Framework.Allocation;
+using osu.Framework.Audio;
 using osu.Framework.Bindables;
 using osu.Framework.Graphics;
 using osu.Framework.Graphics.Containers;
@@ -45,7 +44,7 @@ public class GameplayClockContainer : Container, IAdjustableClock, IGameplayCloc
         /// </remarks>
         public double StartTime { get; protected set; }
 
-        public virtual IEnumerable<double> GameplayAdjustments => Enumerable.Empty<double>();
+        public IAdjustableAudioComponent GameplayAdjustments { get; } = new AudioAdjustments();
 
         private readonly BindableBool isPaused = new BindableBool(true);
 
diff --git a/osu.Game/Screens/Play/GameplayClockExtensions.cs b/osu.Game/Screens/Play/GameplayClockExtensions.cs
index b683c61f63..d16d6f9ba0 100644
--- a/osu.Game/Screens/Play/GameplayClockExtensions.cs
+++ b/osu.Game/Screens/Play/GameplayClockExtensions.cs
@@ -1,8 +1,6 @@
 // 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;
-
 namespace osu.Game.Screens.Play
 {
     public static class GameplayClockExtensions
@@ -11,12 +9,6 @@ public static class GameplayClockExtensions
         /// The rate of gameplay when playback is at 100%.
         /// This excludes any seeking / user adjustments.
         /// </summary>
-        public static double GetTrueGameplayRate(this IGameplayClock clock)
-        {
-            double rate = Math.Sign(clock.Rate);
-            foreach (double a in clock.GameplayAdjustments)
-                rate *= a;
-            return rate;
-        }
+        public static double GetTrueGameplayRate(this IGameplayClock clock) => clock.GameplayAdjustments.AggregateFrequency.Value * clock.GameplayAdjustments.AggregateTempo.Value;
     }
 }
diff --git a/osu.Game/Screens/Play/IGameplayClock.cs b/osu.Game/Screens/Play/IGameplayClock.cs
index 7c50b9d407..c58d2dbcac 100644
--- a/osu.Game/Screens/Play/IGameplayClock.cs
+++ b/osu.Game/Screens/Play/IGameplayClock.cs
@@ -1,7 +1,7 @@
 // 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.Collections.Generic;
+using osu.Framework.Audio;
 using osu.Framework.Bindables;
 using osu.Framework.Timing;
 
@@ -19,9 +19,9 @@ public interface IGameplayClock : IFrameBasedClock
         double StartTime { get; }
 
         /// <summary>
-        /// All adjustments applied to this clock which don't come from gameplay or mods.
+        /// All adjustments applied to this clock which come from gameplay or mods.
         /// </summary>
-        IEnumerable<double> GameplayAdjustments { get; }
+        IAdjustableAudioComponent GameplayAdjustments { get; }
 
         IBindable<bool> IsPaused { get; }
     }
diff --git a/osu.Game/Screens/Play/MasterGameplayClockContainer.cs b/osu.Game/Screens/Play/MasterGameplayClockContainer.cs
index 7c30f86125..226ce8b0d8 100644
--- a/osu.Game/Screens/Play/MasterGameplayClockContainer.cs
+++ b/osu.Game/Screens/Play/MasterGameplayClockContainer.cs
@@ -2,13 +2,11 @@
 // See the LICENCE file in the repository root for full licence text.
 
 using System;
-using System.Collections.Generic;
 using System.Linq;
 using osu.Framework.Audio;
 using osu.Framework.Audio.Track;
 using osu.Framework.Bindables;
 using osu.Framework.Graphics;
-using osu.Framework.Graphics.Containers;
 using osu.Framework.Timing;
 using osu.Game.Beatmaps;
 using osu.Game.Beatmaps.ControlPoints;
@@ -25,7 +23,7 @@ namespace osu.Game.Screens.Play
     /// <remarks>
     /// This is intended to be used as a single controller for gameplay, or as a reference source for other <see cref="GameplayClockContainer"/>s.
     /// </remarks>
-    public class MasterGameplayClockContainer : GameplayClockContainer, IBeatSyncProvider, IAdjustableAudioComponent
+    public class MasterGameplayClockContainer : GameplayClockContainer, IBeatSyncProvider
     {
         /// <summary>
         /// Duration before gameplay start time required before skip button displays.
@@ -57,11 +55,6 @@ public class MasterGameplayClockContainer : GameplayClockContainer, IBeatSyncPro
         /// </summary>
         private double? actualStopTime;
 
-        /// <summary>
-        /// Maintained solely to delegate <see cref="IAdjustableAudioComponent"/> pieces to (to maintain parent lookups).
-        /// </summary>
-        private readonly AudioContainer audioContainer;
-
         /// <summary>
         /// Create a new master gameplay clock container.
         /// </summary>
@@ -75,8 +68,6 @@ public MasterGameplayClockContainer(WorkingBeatmap beatmap, double skipTargetTim
             this.skipTargetTime = skipTargetTime;
 
             StartTime = findEarliestStartTime();
-
-            AddInternal(audioContainer = new AudioContainer());
         }
 
         private double findEarliestStartTime()
@@ -202,6 +193,7 @@ private void addSourceClockAdjustments()
             if (speedAdjustmentsApplied)
                 return;
 
+            track.BindAdjustments(GameplayAdjustments);
             track.AddAdjustment(AdjustableProperty.Frequency, GameplayClock.ExternalPauseFrequencyAdjust);
             track.AddAdjustment(AdjustableProperty.Tempo, UserPlaybackRate);
 
@@ -213,6 +205,7 @@ private void removeSourceClockAdjustments()
             if (!speedAdjustmentsApplied)
                 return;
 
+            track.UnbindAdjustments(GameplayAdjustments);
             track.RemoveAdjustment(AdjustableProperty.Frequency, GameplayClock.ExternalPauseFrequencyAdjust);
             track.RemoveAdjustment(AdjustableProperty.Tempo, UserPlaybackRate);
 
@@ -229,49 +222,5 @@ protected override void Dispose(bool isDisposing)
         IClock IBeatSyncProvider.Clock => this;
 
         ChannelAmplitudes IHasAmplitudes.CurrentAmplitudes => beatmap.TrackLoaded ? beatmap.Track.CurrentAmplitudes : ChannelAmplitudes.Empty;
-
-        private readonly List<IBindable<double>> speedAdjustments = new List<IBindable<double>>();
-
-        public override IEnumerable<double> GameplayAdjustments => speedAdjustments.Select(bindable => bindable.Value);
-
-        void IAdjustableAudioComponent.AddAdjustment(AdjustableProperty type, IBindable<double> adjustBindable)
-        {
-            speedAdjustments.Add(adjustBindable);
-            track.AddAdjustment(type, adjustBindable);
-        }
-
-        void IAdjustableAudioComponent.RemoveAdjustment(AdjustableProperty type, IBindable<double> adjustBindable)
-        {
-            speedAdjustments.Remove(adjustBindable);
-            track.RemoveAdjustment(type, adjustBindable);
-        }
-
-        void IAdjustableAudioComponent.RemoveAllAdjustments(AdjustableProperty type) => audioContainer.RemoveAllAdjustments(type);
-
-        void IAdjustableAudioComponent.BindAdjustments(IAggregateAudioAdjustment component) => audioContainer.BindAdjustments(component);
-
-        void IAdjustableAudioComponent.UnbindAdjustments(IAggregateAudioAdjustment component) => audioContainer.UnbindAdjustments(component);
-
-        BindableNumber<double> IAdjustableAudioComponent.Volume => audioContainer.Volume;
-
-        BindableNumber<double> IAdjustableAudioComponent.Balance => audioContainer.Balance;
-
-        BindableNumber<double> IAdjustableAudioComponent.Frequency => audioContainer.Frequency;
-
-        BindableNumber<double> IAdjustableAudioComponent.Tempo => audioContainer.Tempo;
-
-        public override void ResetSpeedAdjustments()
-        {
-            track.RemoveAllAdjustments(AdjustableProperty.Frequency);
-            track.RemoveAllAdjustments(AdjustableProperty.Tempo);
-        }
-
-        IBindable<double> IAggregateAudioAdjustment.AggregateVolume => audioContainer.AggregateVolume;
-
-        IBindable<double> IAggregateAudioAdjustment.AggregateBalance => audioContainer.AggregateBalance;
-
-        IBindable<double> IAggregateAudioAdjustment.AggregateFrequency => audioContainer.AggregateFrequency;
-
-        IBindable<double> IAggregateAudioAdjustment.AggregateTempo => audioContainer.AggregateTempo;
     }
 }
diff --git a/osu.Game/Screens/Play/Player.cs b/osu.Game/Screens/Play/Player.cs
index 21a02fbe0b..198d9da570 100644
--- a/osu.Game/Screens/Play/Player.cs
+++ b/osu.Game/Screens/Play/Player.cs
@@ -999,11 +999,12 @@ public override void OnEntering(ScreenTransitionEvent e)
             // Our mods are local copies of the global mods so they need to be re-applied to the track.
             // This is done through the music controller (for now), because resetting speed adjustments on the beatmap track also removes adjustments provided by DrawableTrack.
             // Todo: In the future, player will receive in a track and will probably not have to worry about this...
-            if (GameplayClockContainer is IAdjustableAudioComponent adjustableClock)
+            if (GameplayClockContainer is MasterGameplayClockContainer master)
             {
-                GameplayClockContainer.ResetSpeedAdjustments();
+                musicController.ResetTrackAdjustments();
+
                 foreach (var mod in GameplayState.Mods.OfType<IApplicableToTrack>())
-                    mod.ApplyToTrack(adjustableClock);
+                    mod.ApplyToTrack(master.GameplayAdjustments);
             }
 
             updateGameplayState();

[peppy]

Here's a quick diff I've came up with, haven't tested if it works yet:

I think this could work better. I'm not sure about the ResetAdjustments call being reverted to musicController though, I preferred removing musicController from the equation. I'll give it a try.

[peppy]

I've applied the diff with minor fixes, seems to work quite well.

Of note though, this PR will not work with multiplayer spectating right now (both before and after your proposed changes are applied). Why this is the case should be immediately obvious, but it was previously relying on a bit of luck to work correctly.

I'll try and figure a sane solution to making this work again... and maybe add test coverage, maybe.

[peppy]

I've fixed multiplayer spectating not working. Not 100% confident on the solution (and managed to reproduce a weird sync catch-up issue I'm investigating... not yet sure if this change is responsible for it, but looking at the code I can't see how it could be) can't reproduce. or rather, it's to be expected because of how i'm testing (pausing with debug, so frames aren't being sent for the longest peiod).

[frenzibyte]

So far this reads pretty well to me (save for the multi-spectator changes). I was initially under the thought that GetTrueGameplayRate will never be negative unless influenced by a mod, but it appears the use case in spinner actually relies on it being negative to correctly decrease the spinner rotation, so I guess it's fine for now.

[frenzibyte]

Feels weird to use MusicController.ResetTrackAdjustments when already having access over the track that is to be adjusted, i.e. maybe this could just use track.ResetSpeedAdjustments or track.RemoveAllAdjustments?

Seeing that the method internally re-adds the mod adjustments if allowed, maybe it should be renamed to RefreshTrackAdjustments or something of the sorts.

[smoogipoo]

Gonna keep this one as it is for now to not pile up more changes in this PR.

[frenzibyte]

Out of curiosity, could there be a case where audio adjustments differ between spectated players? Would imagine only having to bind once rather than switching every time the candidate player area changes.

[peppy]

Maybe. The reason I did it this way was in the case the original player quit or lagged out.

The main case it's important is time ramp, where the adjustments are not constant.

[frenzibyte]

As per the test failures, the dictionary should also be filled when BeginPlayingInternal is called.

[peppy]

While the single test failure may look related, it seems to be something which has existed forever and can likely be ignored (https://teamcity.ppy.sh/buildConfiguration/Osu_Build/8026?hideProblemsFromDependencies=false&hideTestsFromDependencies=false&expandBuildTestsSection=true as one example)

[smoogipoo]

I'm still not really happy with the structure of things, where everything's doing everything (e.g. player applies the mods to something exposed by gameplay clock container, but then the gameplay clock container is the one that applies the adjustments to the track), but at least I've removed one indirection via SpectatorPlayerClock and hope to keep it that way as it's the number one thing that I couldn't handle.

Because something like that, to me, indicates it's handled by something else like the sync manager (which handles everything else related to SpectatorPlayerClock), but that wasn't the case here.

Looking back on it, I think it could be fun allowing different rate-adjustment mods per-player and having the ones that finished earlier enter a spectator view or something like that. All of this is to say that the binding inside MultiSpectatorScreen may be fine for the foreseeable future.

[peppy]

Are you against making this a property redirecting to player.ClockAdjustmentsFromMods? Every instance of AudioAdjustments calling BindAdjustments makes the flow harder to follow for me, due to the bind being single directional (as mentioned by the comment that was removed in your changes).

I think the comment should remain in some way because it's a huge gotcha if you do it the wrong way around.

[peppy]

Also may be able to get by with only exposing as IAggregateAudioAdjustment, which would make the flow a bit easier to follow (you can be sure that the adjustments are read only if so).

[smoogipoo]

I originally had it that way but it's null for a while during player's load, and I just didn't want to deal with peppering code in just the right way to avoid a nullref when this is referenced too early...

At best I could add a remark comment here?

[peppy]

Can expose publicly as IAggregateAudioAdjustment and leave the AudioAdjustment as private, should still be better I think.

[smoogipoo]

Can expose publicly as IAggregateAudioAdjustment

Oh, I didn't realise IAggregateAudioAdjustment was readonly. Done.

[smoogipoo]

Please make sure you're fine with my changes and i'll hit the green button.

[peppy]

Make sure you test this to make sure nothing funny happens with moving it in here.

[smoogipoo]

I did not mean to make this change.

[peppy]

Changes look fine.

[smoogipoo]

Is this test failure relevant? https://github.com/ppy/osu/runs/8246729162?check_suite_focus=true

[peppy]

Doubtful