Skip to content
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.

Sign up for GitHub

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

Cancel difficulty calculation after 10 seconds by default #32208

Merged
merged 1 commit into from
Mar 4, 2025
+12 −7
Merged

Cancel difficulty calculation after 10 seconds by default #32208

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 12 additions & 7 deletions osu.Game/Rulesets/Difficulty/DifficultyCalculator.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,11 @@ public DifficultyAttributes Calculate(CancellationToken cancellationToken = defa
/// <returns>A structure describing the difficulty of the beatmap.</returns>
public DifficultyAttributes Calculate([NotNull] IEnumerable<Mod> mods, CancellationToken cancellationToken = default)
{
using var timedCancellationSource = new CancellationTokenSource(TimeSpan.FromSeconds(10));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I get this is 100% a nitpick but we might want to avoid creating this when it's not being used. Just to set a good example for the rest of diffcalc (which is definitely where any allocation issue will lie 🤷).

diff --git a/osu.Game/Rulesets/Difficulty/DifficultyCalculator.cs b/osu.Game/Rulesets/Difficulty/DifficultyCalculator.cs
index add24f7866..7d01e678b1 100644
--- a/osu.Game/Rulesets/Difficulty/DifficultyCalculator.cs
+++ b/osu.Game/Rulesets/Difficulty/DifficultyCalculator.cs
@@ -62,13 +62,18 @@ public DifficultyAttributes Calculate(CancellationToken cancellationToken = defa
         /// <returns>A structure describing the difficulty of the beatmap.</returns>
         public DifficultyAttributes Calculate([NotNull] IEnumerable<Mod> mods, CancellationToken cancellationToken = default)
         {
-            using var timedCancellationSource = new CancellationTokenSource(TimeSpan.FromSeconds(10));
-
-            if (!cancellationToken.CanBeCanceled)
-                cancellationToken = timedCancellationSource.Token;
-
-            cancellationToken.ThrowIfCancellationRequested();
-            preProcess(mods, cancellationToken);
+            if (cancellationToken.CanBeCanceled)
+            {
+                cancellationToken.ThrowIfCancellationRequested();
+                preProcess(mods, cancellationToken);
+            }
+            else
+            {
+                // Difficulty cancellation can potentially get stuck in weird beatmaps, so force a timeout if no cancellation
+                // mechanism was provided.
+                using (var timedCancellationSource = new CancellationTokenSource(TimeSpan.FromSeconds(10)))
+                    preProcess(mods, timedCancellationSource.Token);
+            }
 
             var skills = CreateSkills(Beatmap, playableMods, clockRate);
 
@@ -103,19 +108,24 @@ public List<TimedDifficultyAttributes> CalculateTimed(CancellationToken cancella
         /// <returns>The set of <see cref="TimedDifficultyAttributes"/>.</returns>
         public List<TimedDifficultyAttributes> CalculateTimed([NotNull] IEnumerable<Mod> mods, CancellationToken cancellationToken = default)
         {
-            using var timedCancellationSource = new CancellationTokenSource(TimeSpan.FromSeconds(10));
-
-            if (!cancellationToken.CanBeCanceled)
-                cancellationToken = timedCancellationSource.Token;
-
-            cancellationToken.ThrowIfCancellationRequested();
-            preProcess(mods, cancellationToken);
-
             var attribs = new List<TimedDifficultyAttributes>();
 
             if (!Beatmap.HitObjects.Any())
                 return attribs;
 
+            if (cancellationToken.CanBeCanceled)
+            {
+                cancellationToken.ThrowIfCancellationRequested();
+                preProcess(mods, cancellationToken);
+            }
+            else
+            {
+                // Difficulty cancellation can potentially get stuck in weird beatmaps, so force a timeout if no cancellation
+                // mechanism was provided.
+                using (var timedCancellationSource = new CancellationTokenSource(TimeSpan.FromSeconds(10)))
+                    preProcess(mods, timedCancellationSource.Token);
+            }
+
             var skills = CreateSkills(Beatmap, playableMods, clockRate);
             var progressiveBeatmap = new ProgressiveCalculationBeatmap(Beatmap);
             var difficultyObjects = getDifficultyHitObjects().ToArray();

Copy link
Contributor Author

@smoogipoo smoogipoo Mar 4, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The important part isn't the preProcess(), it's the usage in the loop below. In other words, the lifetime of the CTS has to be for the entire method.

One way of structuring it would be a nested method, like so:

diff --git a/osu.Game/Rulesets/Difficulty/DifficultyCalculator.cs b/osu.Game/Rulesets/Difficulty/DifficultyCalculator.cs
index add24f7866..624a8d72bf 100644
--- a/osu.Game/Rulesets/Difficulty/DifficultyCalculator.cs
+++ b/osu.Game/Rulesets/Difficulty/DifficultyCalculator.cs
@@ -62,29 +62,33 @@ public DifficultyAttributes Calculate(CancellationToken cancellationToken = defa
         /// <returns>A structure describing the difficulty of the beatmap.</returns>
         public DifficultyAttributes Calculate([NotNull] IEnumerable<Mod> mods, CancellationToken cancellationToken = default)
         {
-            using var timedCancellationSource = new CancellationTokenSource(TimeSpan.FromSeconds(10));
+            if (cancellationToken.CanBeCanceled)
+                return doTheThing(cancellationToken);
 
-            if (!cancellationToken.CanBeCanceled)
-                cancellationToken = timedCancellationSource.Token;
+            using (var timedCancellationSource = new CancellationTokenSource(TimeSpan.FromSeconds(10)))
+                return doTheThing(timedCancellationSource.Token);
 
-            cancellationToken.ThrowIfCancellationRequested();
-            preProcess(mods, cancellationToken);
+            DifficultyAttributes doTheThing(CancellationToken token)
+            {
+                token.ThrowIfCancellationRequested();
+                preProcess(mods, token);
 
-            var skills = CreateSkills(Beatmap, playableMods, clockRate);
+                var skills = CreateSkills(Beatmap, playableMods, clockRate);
 
-            if (!Beatmap.HitObjects.Any())
-                return CreateDifficultyAttributes(Beatmap, playableMods, skills, clockRate);
+                if (!Beatmap.HitObjects.Any())
+                    return CreateDifficultyAttributes(Beatmap, playableMods, skills, clockRate);
 
-            foreach (var hitObject in getDifficultyHitObjects())
-            {
-                foreach (var skill in skills)
+                foreach (var hitObject in getDifficultyHitObjects())
                 {
-                    cancellationToken.ThrowIfCancellationRequested();
-                    skill.Process(hitObject);
+                    foreach (var skill in skills)
+                    {
+                        token.ThrowIfCancellationRequested();
+                        skill.Process(hitObject);
+                    }
                 }
-            }
 
-            return CreateDifficultyAttributes(Beatmap, playableMods, skills, clockRate);
+                return CreateDifficultyAttributes(Beatmap, playableMods, skills, clockRate);
+            }
         }
 
         /// <summary>

But imo this isn't really worth it? If you disagree and feel that it is then I can push it.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah right, i missed that.

let's just leave it


if (!cancellationToken.CanBeCanceled)
cancellationToken = timedCancellationSource.Token;

cancellationToken.ThrowIfCancellationRequested();
preProcess(mods, cancellationToken);

Expand Down Expand Up @@ -98,6 +103,11 @@ public List<TimedDifficultyAttributes> CalculateTimed(CancellationToken cancella
/// <returns>The set of <see cref="TimedDifficultyAttributes"/>.</returns>
public List<TimedDifficultyAttributes> CalculateTimed([NotNull] IEnumerable<Mod> mods, CancellationToken cancellationToken = default)
{
using var timedCancellationSource = new CancellationTokenSource(TimeSpan.FromSeconds(10));

if (!cancellationToken.CanBeCanceled)
cancellationToken = timedCancellationSource.Token;

cancellationToken.ThrowIfCancellationRequested();
preProcess(mods, cancellationToken);

Expand Down Expand Up @@ -166,15 +176,10 @@ public IEnumerable<DifficultyAttributes> CalculateAllLegacyCombinations(Cancella
/// </summary>
/// <param name="mods">The original list of <see cref="Mod"/>s.</param>
/// <param name="cancellationToken">The cancellation token.</param>
private void preProcess([NotNull] IEnumerable<Mod> mods, CancellationToken cancellationToken = default)
private void preProcess([NotNull] IEnumerable<Mod> mods, CancellationToken cancellationToken)
{
playableMods = mods.Select(m => m.DeepClone()).ToArray();

// Only pass through the cancellation token if it's non-default.
// This allows for the default timeout to be applied for playable beatmap construction.
Beatmap = cancellationToken == default
? beatmap.GetPlayableBeatmap(ruleset, playableMods)
: beatmap.GetPlayableBeatmap(ruleset, playableMods, cancellationToken);
Beatmap = beatmap.GetPlayableBeatmap(ruleset, playableMods, cancellationToken);

var track = new TrackVirtual(10000);
playableMods.OfType<IApplicableToTrack>().ForEach(m => m.ApplyToTrack(track));
Expand Down
Loading