From 6100bf66a694dc6cac148c67689b3c4555418a83 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bart=C5=82omiej=20Dach?= Date: Sat, 20 Nov 2021 17:23:55 +0100 Subject: [PATCH 1/3] Clean up cancellation handling in `WorkingBeatmap` After the recent changes introducing cancellation support to `WorkingBeatmap`, it turned out that if the cancellation support was used, `GetPlayableBeatmap()` would raise timeout exceptions rather than the expected `OperationCanceledException`. To that end, split off a separate overload for the typical usage, that catches `OperationCanceledException` and converts them to beatmap load timeout exceptions, and use normal `OperationCanceledException`s in the overload that requires a cancellation token to work. --- osu.Game.Tests/Beatmaps/WorkingBeatmapTest.cs | 5 +- osu.Game/Beatmaps/IWorkingBeatmap.cs | 23 +++++++- osu.Game/Beatmaps/WorkingBeatmap.cs | 53 ++++++++----------- .../Play/HUD/PerformancePointsCounter.cs | 2 +- 4 files changed, 48 insertions(+), 35 deletions(-) diff --git a/osu.Game.Tests/Beatmaps/WorkingBeatmapTest.cs b/osu.Game.Tests/Beatmaps/WorkingBeatmapTest.cs index 6ad63a5fbb..5f70f08413 100644 --- a/osu.Game.Tests/Beatmaps/WorkingBeatmapTest.cs +++ b/osu.Game.Tests/Beatmaps/WorkingBeatmapTest.cs @@ -9,6 +9,7 @@ using Moq; using NUnit.Framework; using osu.Game.Beatmaps; using osu.Game.Rulesets; +using osu.Game.Rulesets.Mods; using osu.Game.Rulesets.Objects; using osu.Game.Rulesets.Osu; using osu.Game.Rulesets.Osu.Beatmaps; @@ -40,7 +41,7 @@ namespace osu.Game.Tests.Beatmaps Task.Factory.StartNew(() => { loadStarted.Set(); - Assert.Throws(() => working.GetPlayableBeatmap(new OsuRuleset().RulesetInfo, cancellationToken: cts.Token)); + Assert.Throws(() => working.GetPlayableBeatmap(new OsuRuleset().RulesetInfo, Array.Empty(), cts.Token)); loadCompleted.Set(); }, TaskCreationOptions.LongRunning); @@ -58,7 +59,7 @@ namespace osu.Game.Tests.Beatmaps { var working = new TestNeverLoadsWorkingBeatmap(); - Assert.Throws(() => working.GetPlayableBeatmap(new OsuRuleset().RulesetInfo)); + Assert.Throws(Is.InstanceOf(), () => working.GetPlayableBeatmap(new OsuRuleset().RulesetInfo)); working.ResetEvent.Set(); } diff --git a/osu.Game/Beatmaps/IWorkingBeatmap.cs b/osu.Game/Beatmaps/IWorkingBeatmap.cs index da977bb9d5..6cebce3c59 100644 --- a/osu.Game/Beatmaps/IWorkingBeatmap.cs +++ b/osu.Game/Beatmaps/IWorkingBeatmap.cs @@ -90,12 +90,31 @@ namespace osu.Game.Beatmaps /// have been applied, and s have been fully constructed. /// /// + /// + /// By default, the beatmap load process will be interrupted after 10 seconds. + /// For finer-grained control over the load process, use the + /// + /// overload instead. + /// /// The to create a playable for. /// The s to apply to the . - /// Cancellation token that cancels the beatmap loading process. If not provided, a default timeout of 10,000ms will be applied to the load process. /// The converted . /// If could not be converted to . - IBeatmap GetPlayableBeatmap(IRulesetInfo ruleset, IReadOnlyList mods = null, CancellationToken? cancellationToken = null); + IBeatmap GetPlayableBeatmap(IRulesetInfo ruleset, params Mod[] mods); + + /// + /// Constructs a playable from using the applicable converters for a specific . + /// + /// The returned is in a playable state - all and s + /// have been applied, and s have been fully constructed. + /// + /// + /// The to create a playable for. + /// The s to apply to the . + /// Cancellation token that cancels the beatmap loading process. + /// The converted . + /// If could not be converted to . + IBeatmap GetPlayableBeatmap(IRulesetInfo ruleset, IReadOnlyList mods, CancellationToken cancellationToken); /// /// Load a new audio track instance for this beatmap. This should be called once before accessing . diff --git a/osu.Game/Beatmaps/WorkingBeatmap.cs b/osu.Game/Beatmaps/WorkingBeatmap.cs index d2912229c6..a6f9f4e5f5 100644 --- a/osu.Game/Beatmaps/WorkingBeatmap.cs +++ b/osu.Game/Beatmaps/WorkingBeatmap.cs @@ -79,14 +79,24 @@ namespace osu.Game.Beatmaps /// The applicable . protected virtual IBeatmapConverter CreateBeatmapConverter(IBeatmap beatmap, Ruleset ruleset) => ruleset.CreateBeatmapConverter(beatmap); - public virtual IBeatmap GetPlayableBeatmap(IRulesetInfo ruleset, IReadOnlyList mods = null, CancellationToken? cancellationToken = null) + public IBeatmap GetPlayableBeatmap([NotNull] IRulesetInfo ruleset, params Mod[] mods) { - var token = cancellationToken ?? - // don't apply the default timeout when debugger is attached (may be breakpointing / debugging). - (Debugger.IsAttached ? new CancellationToken() : new CancellationTokenSource(10000).Token); - - mods ??= Array.Empty(); + try + { + using (var cancellationTokenSource = new CancellationTokenSource(10_000)) + { + // don't apply the default timeout when debugger is attached (may be breakpointing / debugging). + return GetPlayableBeatmap(ruleset, mods, Debugger.IsAttached ? new CancellationToken() : cancellationTokenSource.Token); + } + } + catch (OperationCanceledException) + { + throw new BeatmapLoadTimeoutException(BeatmapInfo); + } + } + public virtual IBeatmap GetPlayableBeatmap([NotNull] IRulesetInfo ruleset, [NotNull] IReadOnlyList mods, CancellationToken token) + { var rulesetInstance = ruleset.CreateInstance(); if (rulesetInstance == null) @@ -101,9 +111,7 @@ namespace osu.Game.Beatmaps // Apply conversion mods foreach (var mod in mods.OfType()) { - if (token.IsCancellationRequested) - throw new BeatmapLoadTimeoutException(BeatmapInfo); - + token.ThrowIfCancellationRequested(); mod.ApplyToBeatmapConverter(converter); } @@ -113,9 +121,7 @@ namespace osu.Game.Beatmaps // Apply conversion mods to the result foreach (var mod in mods.OfType()) { - if (token.IsCancellationRequested) - throw new BeatmapLoadTimeoutException(BeatmapInfo); - + token.ThrowIfCancellationRequested(); mod.ApplyToBeatmap(converted); } @@ -124,9 +130,7 @@ namespace osu.Game.Beatmaps { foreach (var mod in mods.OfType()) { - if (token.IsCancellationRequested) - throw new BeatmapLoadTimeoutException(BeatmapInfo); - + token.ThrowIfCancellationRequested(); mod.ApplyToDifficulty(converted.Difficulty); } } @@ -139,28 +143,17 @@ namespace osu.Game.Beatmaps processor?.PreProcess(); // Compute default values for hitobjects, including creating nested hitobjects in-case they're needed - try + foreach (var obj in converted.HitObjects) { - foreach (var obj in converted.HitObjects) - { - if (token.IsCancellationRequested) - throw new BeatmapLoadTimeoutException(BeatmapInfo); - - obj.ApplyDefaults(converted.ControlPointInfo, converted.Difficulty, token); - } - } - catch (OperationCanceledException) - { - throw new BeatmapLoadTimeoutException(BeatmapInfo); + token.ThrowIfCancellationRequested(); + obj.ApplyDefaults(converted.ControlPointInfo, converted.Difficulty, token); } foreach (var mod in mods.OfType()) { foreach (var obj in converted.HitObjects) { - if (token.IsCancellationRequested) - throw new BeatmapLoadTimeoutException(BeatmapInfo); - + token.ThrowIfCancellationRequested(); mod.ApplyToHitObject(obj); } } diff --git a/osu.Game/Screens/Play/HUD/PerformancePointsCounter.cs b/osu.Game/Screens/Play/HUD/PerformancePointsCounter.cs index 9cb5d3cd91..854f255269 100644 --- a/osu.Game/Screens/Play/HUD/PerformancePointsCounter.cs +++ b/osu.Game/Screens/Play/HUD/PerformancePointsCounter.cs @@ -216,7 +216,7 @@ namespace osu.Game.Screens.Play.HUD this.gameplayBeatmap = gameplayBeatmap; } - public override IBeatmap GetPlayableBeatmap(IRulesetInfo ruleset, IReadOnlyList mods = null, CancellationToken? cancellationToken = null) + public override IBeatmap GetPlayableBeatmap(IRulesetInfo ruleset, IReadOnlyList mods, CancellationToken cancellationToken) => gameplayBeatmap; protected override IBeatmap GetBeatmap() => gameplayBeatmap; From a7e45a9098209000a179861ad4c77531f996e450 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bart=C5=82omiej=20Dach?= Date: Sat, 20 Nov 2021 17:32:40 +0100 Subject: [PATCH 2/3] Log all non-cancellation errors in difficulty cache --- osu.Game/Beatmaps/BeatmapDifficultyCache.cs | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-) diff --git a/osu.Game/Beatmaps/BeatmapDifficultyCache.cs b/osu.Game/Beatmaps/BeatmapDifficultyCache.cs index 9106408e23..f444b597c9 100644 --- a/osu.Game/Beatmaps/BeatmapDifficultyCache.cs +++ b/osu.Game/Beatmaps/BeatmapDifficultyCache.cs @@ -294,15 +294,22 @@ namespace osu.Game.Beatmaps return new StarDifficulty(attributes); } - catch (BeatmapInvalidForRulesetException e) + catch (OperationCanceledException) + { + // no need to log, cancellations are expected as part of normal operation. + return null; + } + catch (BeatmapInvalidForRulesetException invalidForRuleset) { if (rulesetInfo.Equals(beatmapInfo.Ruleset)) - Logger.Error(e, $"Failed to convert {beatmapInfo.OnlineID} to the beatmap's default ruleset ({beatmapInfo.Ruleset})."); + Logger.Error(invalidForRuleset, $"Failed to convert {beatmapInfo.OnlineID} to the beatmap's default ruleset ({beatmapInfo.Ruleset})."); return null; } - catch + catch (Exception unknownException) { + Logger.Error(unknownException, "Failed to calculate beatmap difficulty"); + return null; } } From bb8e8bc4f084e19a536b2eb4239f97d41afbe3e0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bart=C5=82omiej=20Dach?= Date: Sun, 21 Nov 2021 11:30:45 +0100 Subject: [PATCH 3/3] Use consistent type for mod collection in all overloads --- osu.Game/Beatmaps/IWorkingBeatmap.cs | 2 +- osu.Game/Beatmaps/WorkingBeatmap.cs | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/osu.Game/Beatmaps/IWorkingBeatmap.cs b/osu.Game/Beatmaps/IWorkingBeatmap.cs index 6cebce3c59..717162fd7f 100644 --- a/osu.Game/Beatmaps/IWorkingBeatmap.cs +++ b/osu.Game/Beatmaps/IWorkingBeatmap.cs @@ -100,7 +100,7 @@ namespace osu.Game.Beatmaps /// The s to apply to the . /// The converted . /// If could not be converted to . - IBeatmap GetPlayableBeatmap(IRulesetInfo ruleset, params Mod[] mods); + IBeatmap GetPlayableBeatmap(IRulesetInfo ruleset, IReadOnlyList mods = null); /// /// Constructs a playable from using the applicable converters for a specific . diff --git a/osu.Game/Beatmaps/WorkingBeatmap.cs b/osu.Game/Beatmaps/WorkingBeatmap.cs index a6f9f4e5f5..a17f247d9b 100644 --- a/osu.Game/Beatmaps/WorkingBeatmap.cs +++ b/osu.Game/Beatmaps/WorkingBeatmap.cs @@ -79,14 +79,14 @@ namespace osu.Game.Beatmaps /// The applicable . protected virtual IBeatmapConverter CreateBeatmapConverter(IBeatmap beatmap, Ruleset ruleset) => ruleset.CreateBeatmapConverter(beatmap); - public IBeatmap GetPlayableBeatmap([NotNull] IRulesetInfo ruleset, params Mod[] mods) + public IBeatmap GetPlayableBeatmap([NotNull] IRulesetInfo ruleset, IReadOnlyList mods = null) { try { using (var cancellationTokenSource = new CancellationTokenSource(10_000)) { // don't apply the default timeout when debugger is attached (may be breakpointing / debugging). - return GetPlayableBeatmap(ruleset, mods, Debugger.IsAttached ? new CancellationToken() : cancellationTokenSource.Token); + return GetPlayableBeatmap(ruleset, mods ?? Array.Empty(), Debugger.IsAttached ? new CancellationToken() : cancellationTokenSource.Token); } } catch (OperationCanceledException)