-
-
Notifications
You must be signed in to change notification settings - Fork 2.3k
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
Improve performance of UR calculations #30868
Changes from all commits
a1916d1
605fe71
33d725e
5668258
ea68d4b
bbe8f2e
d903d38
d6cf1db
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -5,6 +5,7 @@ | |
using System.Collections.Generic; | ||
using System.Diagnostics; | ||
using System.Linq; | ||
using osu.Game.Rulesets.Objects; | ||
|
||
namespace osu.Game.Rulesets.Scoring | ||
{ | ||
|
@@ -20,32 +21,36 @@ public static class HitEventExtensions | |
/// A non-null <see langword="double"/> value if unstable rate could be calculated, | ||
/// and <see langword="null"/> if unstable rate cannot be calculated due to <paramref name="hitEvents"/> being empty. | ||
/// </returns> | ||
public static double? CalculateUnstableRate(this IEnumerable<HitEvent> hitEvents) | ||
public static UnstableRateCalculationResult? CalculateUnstableRate(this IReadOnlyList<HitEvent> hitEvents, UnstableRateCalculationResult? result = null) | ||
{ | ||
Debug.Assert(hitEvents.All(ev => ev.GameplayRate != null)); | ||
|
||
int count = 0; | ||
double mean = 0; | ||
double sumOfSquares = 0; | ||
result ??= new UnstableRateCalculationResult(); | ||
|
||
foreach (var e in hitEvents) | ||
// Handle rewinding in the simplest way possible. | ||
if (hitEvents.Count < result.EventCount + 1) | ||
result = new UnstableRateCalculationResult(); | ||
|
||
for (int i = result.EventCount; i < hitEvents.Count; i++) | ||
{ | ||
HitEvent e = hitEvents[i]; | ||
|
||
if (!AffectsUnstableRate(e)) | ||
continue; | ||
|
||
count++; | ||
result.EventCount++; | ||
|
||
// Division by gameplay rate is to account for TimeOffset scaling with gameplay rate. | ||
double currentValue = e.TimeOffset / e.GameplayRate!.Value; | ||
double nextMean = mean + (currentValue - mean) / count; | ||
sumOfSquares += (currentValue - mean) * (currentValue - nextMean); | ||
mean = nextMean; | ||
double nextMean = result.Mean + (currentValue - result.Mean) / result.EventCount; | ||
result.SumOfSquares += (currentValue - result.Mean) * (currentValue - nextMean); | ||
result.Mean = nextMean; | ||
} | ||
|
||
if (count == 0) | ||
if (result.EventCount == 0) | ||
return null; | ||
|
||
return 10.0 * Math.Sqrt(sumOfSquares / count); | ||
return result; | ||
} | ||
|
||
/// <summary> | ||
|
@@ -65,6 +70,39 @@ public static class HitEventExtensions | |
return timeOffsets.Average(); | ||
} | ||
|
||
public static bool AffectsUnstableRate(HitEvent e) => !(e.HitObject.HitWindows is HitWindows.EmptyHitWindows) && e.Result.IsHit(); | ||
public static bool AffectsUnstableRate(HitEvent e) => AffectsUnstableRate(e.HitObject, e.Result); | ||
public static bool AffectsUnstableRate(HitObject hitObject, HitResult result) => hitObject.HitWindows != HitWindows.Empty && result.IsHit(); | ||
|
||
/// <summary> | ||
/// Data type returned by <see cref="HitEventExtensions.CalculateUnstableRate"/> which allows efficient incremental processing. | ||
/// </summary> | ||
/// <remarks> | ||
/// This should be passed back into future <see cref="HitEventExtensions.CalculateUnstableRate"/> calls as a parameter. | ||
/// | ||
/// The optimisations used here rely on hit events being a consecutive sequence from a single gameplay session. | ||
/// When a new gameplay session is started, any existing results should be disposed. | ||
/// </remarks> | ||
public class UnstableRateCalculationResult | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. My only query is why this isn't a Minor nitpick, though. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I can try a But maybe this was just local to the benchmark and not relevant in real-world scenarios. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If it has a significant overhead, I'd say leave it then. |
||
{ | ||
/// <summary> | ||
/// Total events processed. For internal incremental calculation use. | ||
/// </summary> | ||
public int EventCount; | ||
|
||
/// <summary> | ||
/// Last sum-of-squares value. For internal incremental calculation use. | ||
/// </summary> | ||
public double SumOfSquares; | ||
|
||
/// <summary> | ||
/// Last mean value. For internal incremental calculation use. | ||
/// </summary> | ||
public double Mean; | ||
|
||
/// <summary> | ||
/// The unstable rate. | ||
/// </summary> | ||
public double Result => EventCount == 0 ? 0 : 10.0 * Math.Sqrt(SumOfSquares / EventCount); | ||
} | ||
} | ||
} |
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.
I'm sure people are going to want changes to this, but I'm not sure how strict review will be so I've just left it without any safeties initially.
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.
I'd rename
NextProcessableIndex
toEventCount
probably (with the necessary off-by-one adjustments) but otherwise I'm not sure I have much of a problem with this...?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.
Actually doesn't require off-by-one adjustments since it already kinda is the event count (at least post-loop-execution).