Add clicks/second counter to HUD

[ItsShamed]

Addresses:

• Adding a KPS counter #19598

---

kps_showcase.mp4

Speed and mania players can now show off how fast they can tap!

This PR implements a KPS clicks/second counter as an ISkinnableDrawable component to use it with the skin editor, like the pp counter.
It's a list-based implementation which means that every input is stored with a double corresponding to the time the input was made. The actual KPS value is the number of timestamps within the last supposedly 1 second (can change if rate adjust mods are applied).

[bastianpedersen]

If this PR is in its final state, why mark it as a draft?

[bdach]

As for the tests, I don't think that doing them as done here is ever going to work very well - there's too much timing dependency and sensitivity and so I think it's not feasible to test in a non-flaky way. You're probably better off testing KeysPerSecondCalculator by using a manual clock and manually providing inputs to ensure the calculation works as intended.

[ItsShamed]

I'm still having hard time figuring out what to do for the tests. KeysPerSecondCalculator needs dependencies that I can't find manual alternatives to rely on. If you have any suggestions on how I can replace GameplayClock and the DrawableRuleset's IFrameStableClock I'm willing to take a look at it again! For now I think I will just empty the test scene, it just pollutes the CI for no reason.

[bdach]

Well the latest code you have here is hard-dependent on TrueGameplayRate which isn't something any other clock type can provide. Can't really substitute anything of your own in its place without extracting an interface and mocking.

[peppy]

• Tests failing
• Please don't use the KPS acronym anywhere in code, it's not one that people understand
• I didn't mean to make a new interface for attaching, was hoping the same one could work here. If you can't figure that out I can take a look at that part.

[ItsShamed]

Combine the calculator and the display. They don't need to be separate.

My initial goal with the calculator was to centralize the calculation and avoid using static stuff like in my initial implementation. Separating calculation and display makes the code cleaner and less cluttered in my opinion.

---

As for the attach process could you please elaborate more on how things could be improved?

[peppy]

Splitting out the calculation is fine, but I'd only consider doing it if we had more than one use case that would be using this. And I can't think of any others for now.

I'll re-review in the coming days and see how things look now.

[peppy]

As an update, I have some WIP changes to simplify the calculation logic and clean things up a bit.

A few things remaining:

• Check why TrueGameplayRate doesn't include the pause adjustment (I think this is due to it being sourced from the wrong clock)
• Fix tests not passing

Here's the diff if anyone beats me to cleaning things up:

diff --git a/osu.Game.Tests/Visual/Gameplay/TestSceneClicksPerSecond.cs b/osu.Game.Tests/Visual/Gameplay/TestSceneClicksPerSecond.cs
index a140460251..80b6cb558e 100644
--- a/osu.Game.Tests/Visual/Gameplay/TestSceneClicksPerSecond.cs
+++ b/osu.Game.Tests/Visual/Gameplay/TestSceneClicksPerSecond.cs
@@ -76,7 +76,7 @@ public void TestBasicConsistency()
                 seek(i * 10000);
                 advanceForwards(2);
                 int kps = i + 1;
-                AddAssert($"{kps} KPS", () => calculator.Value == kps);
+                AddAssert($"{kps} KPS", () => calculator.Value, () => Is.EqualTo(kps));
             }
         }
 
@@ -123,8 +123,8 @@ public void TestInputsDiscardedOnRewind()
 
         private void seekAllClocks(double time)
         {
-            gameplayClockContainer.Seek(time);
             manualClock.CurrentTime = time;
+            gameplayClockContainer.Seek(time);
         }
 
         protected override Ruleset CreateRuleset() => new OsuRuleset();
@@ -187,7 +187,7 @@ private void addInputs(IEnumerable<double> inputs)
             foreach (double timestamp in inputs)
             {
                 seekAllClocks(timestamp);
-                calculator.AddTimestamp();
+                calculator.AddInputTimestamp();
             }
 
             seekAllClocks(baseTime);
diff --git a/osu.Game/Rulesets/UI/IFrameStableClock.cs b/osu.Game/Rulesets/UI/IFrameStableClock.cs
index 569ef5e06c..5d7783fa3b 100644
--- a/osu.Game/Rulesets/UI/IFrameStableClock.cs
+++ b/osu.Game/Rulesets/UI/IFrameStableClock.cs
@@ -3,10 +3,11 @@
 
 using osu.Framework.Bindables;
 using osu.Framework.Timing;
+using osu.Game.Screens.Play;
 
 namespace osu.Game.Rulesets.UI
 {
-    public interface IFrameStableClock : IFrameBasedClock
+    public interface IFrameStableClock : IGameplayClock
     {
         IBindable<bool> IsCatchingUp { get; }
 
diff --git a/osu.Game/Rulesets/UI/RulesetInputManager.cs b/osu.Game/Rulesets/UI/RulesetInputManager.cs
index dcd2c4fb5d..1a97153f2f 100644
--- a/osu.Game/Rulesets/UI/RulesetInputManager.cs
+++ b/osu.Game/Rulesets/UI/RulesetInputManager.cs
@@ -207,7 +207,7 @@ public ActionListener(ClicksPerSecondCalculator calculator)
 
             public bool OnPressed(KeyBindingPressEvent<T> e)
             {
-                calculator.AddTimestamp();
+                calculator.AddInputTimestamp();
                 return false;
             }
 
diff --git a/osu.Game/Screens/Play/HUD/ClicksPerSecond/ClicksPerSecondCalculator.cs b/osu.Game/Screens/Play/HUD/ClicksPerSecond/ClicksPerSecondCalculator.cs
index 20ac923b82..cc7535ec75 100644
--- a/osu.Game/Screens/Play/HUD/ClicksPerSecond/ClicksPerSecondCalculator.cs
+++ b/osu.Game/Screens/Play/HUD/ClicksPerSecond/ClicksPerSecondCalculator.cs
@@ -2,7 +2,6 @@
 // See the LICENCE file in the repository root for full licence text.
 
 using System.Collections.Generic;
-using System.Linq;
 using osu.Framework.Allocation;
 using osu.Framework.Graphics;
 using osu.Game.Rulesets.UI;
@@ -11,56 +10,48 @@ namespace osu.Game.Screens.Play.HUD.ClicksPerSecond
 {
     public class ClicksPerSecondCalculator : Component
     {
-        private readonly List<double> timestamps;
+        private readonly List<double> timestamps = new List<double>();
 
         [Resolved]
         private IGameplayClock gameplayClock { get; set; } = null!;
 
-        [Resolved]
-        private DrawableRuleset drawableRuleset { get; set; } = null!;
-
-        private double rate;
-
-        // The latest timestamp GC seeked. Does not affect normal gameplay
-        // but prevents duplicate inputs on replays.
-        private double latestTime = double.NegativeInfinity;
+        [Resolved(canBeNull: true)]
+        private DrawableRuleset? drawableRuleset { get; set; }
 
         public int Value { get; private set; }
 
+        private IGameplayClock clock => drawableRuleset?.FrameStableClock ?? gameplayClock;
+
         public ClicksPerSecondCalculator()
         {
             RelativeSizeAxes = Axes.Both;
-            timestamps = new List<double>();
         }
 
+        public void AddInputTimestamp() => timestamps.Add(currentTime);
+
         protected override void Update()
         {
             base.Update();
 
-            // When pausing in replays (using the space bar) GC.TrueGameplayRate returns 0
-            // To prevent CPS value being 0, we store and use the last non-zero TrueGameplayRate
-            if (gameplayClock.TrueGameplayRate > 0)
-            {
-                rate = gameplayClock.TrueGameplayRate;
-            }
+            double latestValidTime = currentTime;
+            double earliestTimeValid = latestValidTime - 1000 * gameplayClock.TrueGameplayRate;
 
-            Value = timestamps.Count(timestamp =>
-            {
-                double window = 1000 * rate;
-                double relativeTime = drawableRuleset.FrameStableClock.CurrentTime - timestamp;
-                return relativeTime > 0 && relativeTime <= window;
-            });
-        }
+            int count = 0;
 
-        public void AddTimestamp()
-        {
-            // Discard inputs if current gameplay time is not the latest
-            // to prevent duplicate inputs
-            if (drawableRuleset.FrameStableClock.CurrentTime >= latestTime)
+            for (int i = timestamps.Count - 1; i >= 0; i--)
             {
-                timestamps.Add(drawableRuleset.FrameStableClock.CurrentTime);
-                latestTime = drawableRuleset.FrameStableClock.CurrentTime;
+                // handle rewinding by removing future timestamps as we go
+                if (timestamps[i] > latestValidTime)
+                {
+                    timestamps.RemoveAt(i);
+                    continue;
+                }
+
+                if (timestamps[i] >= earliestTimeValid)
+                    count++;
             }
+
+            Value = count;
         }
     }
 }

[ItsShamed]

Check why TrueGameplayRate doesn't include the pause adjustment (I think this is due to it being sourced from the wrong clock)

This is because MasterGameplayClockContainer is sourced from a beatmap Track and the rate will always be zero on pause because of pauseFreqAdjust - which changes the Track frequency hence its rate - being also zero on pause (removing it from the nonGameplayAdjustments has no effects):

osu/osu.Game/Screens/Play/MasterGameplayClockContainer.cs

Lines 227 to 231 in 343efa1

	track.AddAdjustment(AdjustableProperty.Frequency, pauseFreqAdjust);
	track.AddAdjustment(AdjustableProperty.Tempo, UserPlaybackRate);
	nonGameplayAdjustments.Add(pauseFreqAdjust);
	nonGameplayAdjustments.Add(UserPlaybackRate);

osu/osu.Game/Screens/Play/MasterGameplayClockContainer.cs

Lines 129 to 151 in 343efa1

	protected override void OnIsPausedChanged(ValueChangedEvent<bool> isPaused)
	{
	if (IsLoaded)
	{
	// During normal operation, the source is stopped after performing a frequency ramp.
	if (isPaused.NewValue)
	{
	this.TransformBindableTo(pauseFreqAdjust, 0, 200, Easing.Out).OnComplete(_ =>
	{
	if (IsPaused.Value == isPaused.NewValue)
	base.OnIsPausedChanged(isPaused);
	});
	}
	else
	this.TransformBindableTo(pauseFreqAdjust, 1, 200, Easing.In);
	}
	else
	{
	if (isPaused.NewValue)
	base.OnIsPausedChanged(isPaused);
	// If not yet loaded, we still want to ensure relevant state is correct, as it is used for offset calculations.
	pauseFreqAdjust.Value = isPaused.NewValue ? 0 : 1;

(applicable to the recent changes made to clocks)

I don't think that sourcing MGCC from a Track is wrong and that having a x0 TrueGameplayRate is a problem, though it could have been helpful to not having to deal with this in this PR.
I don't have any suggestions for the moment, but maybe this is start to find how things could be improved/changed.

[peppy]

Fixes to TrueGameplayRate will be done via #20157, so we can ignore that here for now.

[peppy]

I've rewritten tests from scratch as the previous test scene was very hard to work with. It's not testing as much, but I think has the same coverage.

[peppy]

Should do for now.