-
-
Notifications
You must be signed in to change notification settings - Fork 2.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add clicks/second counter to HUD #19599
Conversation
The counter implementaiton is now list based, and will not invalidate previous hits by removing them but by testing if they are within the 1 second span, allowing better integration with replays and spectators.
If this PR is in its final state, why mark it as a draft? |
osu.Game/Screens/Play/HUD/KPSCounter/KeysPerSecondCalculator.cs
Outdated
Show resolved
Hide resolved
osu.Game/Screens/Play/HUD/KPSCounter/KeysPerSecondCalculator.cs
Outdated
Show resolved
Hide resolved
osu.Game/Screens/Play/HUD/KPSCounter/KeysPerSecondCalculator.cs
Outdated
Show resolved
Hide resolved
osu.Game/Screens/Play/HUD/KPSCounter/KeysPerSecondCalculator.cs
Outdated
Show resolved
Hide resolved
osu.Game/Screens/Play/HUD/KPSCounter/KeysPerSecondCalculator.cs
Outdated
Show resolved
Hide resolved
osu.Game/Screens/Play/HUD/KPSCounter/KeysPerSecondCalculator.cs
Outdated
Show resolved
Hide resolved
osu.Game.Tests/Visual/Gameplay/TestSceneKeysPerSecondCounter.cs
Outdated
Show resolved
Hide resolved
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 |
- Remove '#nullable disable' in KeysPerSecondCalculator and KeysPerSecondCounter - Remove KeysPerSecondCalculator IDisposable implementation - Make KeysPerSecondCalculator static instance initialized once by KeysPerSecondCounters - Auto transfer dependencies from KeysPerSecondCounter to KeysPerSecondCalculator using Resolved properties - Add internal reset logic to KeysPerSecondCalculator and make it independent from Player - Use GameplayClock.TrueGameplayRate to get real-time rate. If 0 then it defaults to the last non 0 rate if no such mod is enabled
osu.Game/Screens/Play/HUD/KPSCounter/KeysPerSecondCalculator.cs
Outdated
Show resolved
Hide resolved
osu.Game/Screens/Play/HUD/KPSCounter/KeysPerSecondCalculator.cs
Outdated
Show resolved
Hide resolved
gathering KPS Calculator now uses DI to retrieve the clocks. Using `HUDOverlay` it is now cached for `KeysPerSecondCounter`s to resolve it. This also allows to make an "Attach" flow like `KeyCounter`.
I'm still having hard time figuring out what to do for the tests. |
Well the latest code you have here is hard-dependent on |
`KeysPerSecondCounter` now depends on `KeysPerSecondCalculator` via the `BackgroundDependencyLoaderAttribute` method, making it appear only in a gameplay context without requiring `GameplayClock` without using it.
Created private mock classes to use them in place of `GameplayClock` and `DrawableRuleset`.
|
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? |
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. |
As an update, I have some WIP changes to simplify the calculation logic and clean things up a bit. A few things remaining:
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;
}
}
}
|
This is because osu/osu.Game/Screens/Play/MasterGameplayClockContainer.cs Lines 227 to 231 in 343efa1
osu/osu.Game/Screens/Play/MasterGameplayClockContainer.cs Lines 129 to 151 in 343efa1
I don't think that sourcing MGCC from a |
Fixes to |
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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should do for now.
Addresses:
kps_showcase.mp4
This PR implements a
KPSclicks/second counter as anISkinnableDrawable
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).