From 4a34a5c738fe5d0a765f85c1b167926bdc678952 Mon Sep 17 00:00:00 2001 From: Dan Balasescu Date: Tue, 30 Nov 2021 11:11:42 +0900 Subject: [PATCH 01/21] Refactor difficulty adjustment mod combinations test --- ...DifficultyAdjustmentModCombinationsTest.cs | 136 ++++++++++-------- 1 file changed, 75 insertions(+), 61 deletions(-) diff --git a/osu.Game.Tests/NonVisual/DifficultyAdjustmentModCombinationsTest.cs b/osu.Game.Tests/NonVisual/DifficultyAdjustmentModCombinationsTest.cs index e458e66ab7..612927337f 100644 --- a/osu.Game.Tests/NonVisual/DifficultyAdjustmentModCombinationsTest.cs +++ b/osu.Game.Tests/NonVisual/DifficultyAdjustmentModCombinationsTest.cs @@ -3,12 +3,14 @@ using System; using System.Collections.Generic; +using System.Linq; using NUnit.Framework; using osu.Game.Beatmaps; using osu.Game.Rulesets.Difficulty; using osu.Game.Rulesets.Difficulty.Preprocessing; using osu.Game.Rulesets.Difficulty.Skills; using osu.Game.Rulesets.Mods; +using osu.Game.Utils; namespace osu.Game.Tests.NonVisual { @@ -20,8 +22,10 @@ namespace osu.Game.Tests.NonVisual { var combinations = new TestLegacyDifficultyCalculator().CreateDifficultyAdjustmentModCombinations(); - Assert.AreEqual(1, combinations.Length); - Assert.IsTrue(combinations[0] is ModNoMod); + assertCombinations(new[] + { + new[] { typeof(ModNoMod) } + }, combinations); } [Test] @@ -29,9 +33,11 @@ namespace osu.Game.Tests.NonVisual { var combinations = new TestLegacyDifficultyCalculator(new ModA()).CreateDifficultyAdjustmentModCombinations(); - Assert.AreEqual(2, combinations.Length); - Assert.IsTrue(combinations[0] is ModNoMod); - Assert.IsTrue(combinations[1] is ModA); + assertCombinations(new[] + { + new[] { typeof(ModNoMod) }, + new[] { typeof(ModA) } + }, combinations); } [Test] @@ -39,14 +45,13 @@ namespace osu.Game.Tests.NonVisual { var combinations = new TestLegacyDifficultyCalculator(new ModA(), new ModB()).CreateDifficultyAdjustmentModCombinations(); - Assert.AreEqual(4, combinations.Length); - Assert.IsTrue(combinations[0] is ModNoMod); - Assert.IsTrue(combinations[1] is ModA); - Assert.IsTrue(combinations[2] is MultiMod); - Assert.IsTrue(combinations[3] is ModB); - - Assert.IsTrue(((MultiMod)combinations[2]).Mods[0] is ModA); - Assert.IsTrue(((MultiMod)combinations[2]).Mods[1] is ModB); + assertCombinations(new[] + { + new[] { typeof(ModNoMod) }, + new[] { typeof(ModA) }, + new[] { typeof(ModA), typeof(ModB) }, + new[] { typeof(ModB) } + }, combinations); } [Test] @@ -54,10 +59,12 @@ namespace osu.Game.Tests.NonVisual { var combinations = new TestLegacyDifficultyCalculator(new ModA(), new ModIncompatibleWithA()).CreateDifficultyAdjustmentModCombinations(); - Assert.AreEqual(3, combinations.Length); - Assert.IsTrue(combinations[0] is ModNoMod); - Assert.IsTrue(combinations[1] is ModA); - Assert.IsTrue(combinations[2] is ModIncompatibleWithA); + assertCombinations(new[] + { + new[] { typeof(ModNoMod) }, + new[] { typeof(ModA) }, + new[] { typeof(ModIncompatibleWithA) } + }, combinations); } [Test] @@ -65,22 +72,17 @@ namespace osu.Game.Tests.NonVisual { var combinations = new TestLegacyDifficultyCalculator(new ModA(), new ModB(), new ModIncompatibleWithA(), new ModIncompatibleWithAAndB()).CreateDifficultyAdjustmentModCombinations(); - Assert.AreEqual(8, combinations.Length); - Assert.IsTrue(combinations[0] is ModNoMod); - Assert.IsTrue(combinations[1] is ModA); - Assert.IsTrue(combinations[2] is MultiMod); - Assert.IsTrue(combinations[3] is ModB); - Assert.IsTrue(combinations[4] is MultiMod); - Assert.IsTrue(combinations[5] is ModIncompatibleWithA); - Assert.IsTrue(combinations[6] is MultiMod); - Assert.IsTrue(combinations[7] is ModIncompatibleWithAAndB); - - Assert.IsTrue(((MultiMod)combinations[2]).Mods[0] is ModA); - Assert.IsTrue(((MultiMod)combinations[2]).Mods[1] is ModB); - Assert.IsTrue(((MultiMod)combinations[4]).Mods[0] is ModB); - Assert.IsTrue(((MultiMod)combinations[4]).Mods[1] is ModIncompatibleWithA); - Assert.IsTrue(((MultiMod)combinations[6]).Mods[0] is ModIncompatibleWithA); - Assert.IsTrue(((MultiMod)combinations[6]).Mods[1] is ModIncompatibleWithAAndB); + assertCombinations(new[] + { + new[] { typeof(ModNoMod) }, + new[] { typeof(ModA) }, + new[] { typeof(ModA), typeof(ModB) }, + new[] { typeof(ModB) }, + new[] { typeof(ModB), typeof(ModIncompatibleWithA) }, + new[] { typeof(ModIncompatibleWithA) }, + new[] { typeof(ModIncompatibleWithA), typeof(ModIncompatibleWithAAndB) }, + new[] { typeof(ModIncompatibleWithAAndB) }, + }, combinations); } [Test] @@ -88,10 +90,12 @@ namespace osu.Game.Tests.NonVisual { var combinations = new TestLegacyDifficultyCalculator(new ModAofA(), new ModIncompatibleWithAofA()).CreateDifficultyAdjustmentModCombinations(); - Assert.AreEqual(3, combinations.Length); - Assert.IsTrue(combinations[0] is ModNoMod); - Assert.IsTrue(combinations[1] is ModAofA); - Assert.IsTrue(combinations[2] is ModIncompatibleWithAofA); + assertCombinations(new[] + { + new[] { typeof(ModNoMod) }, + new[] { typeof(ModAofA) }, + new[] { typeof(ModIncompatibleWithAofA) } + }, combinations); } [Test] @@ -99,17 +103,13 @@ namespace osu.Game.Tests.NonVisual { var combinations = new TestLegacyDifficultyCalculator(new ModA(), new MultiMod(new ModB(), new ModC())).CreateDifficultyAdjustmentModCombinations(); - Assert.AreEqual(4, combinations.Length); - Assert.IsTrue(combinations[0] is ModNoMod); - Assert.IsTrue(combinations[1] is ModA); - Assert.IsTrue(combinations[2] is MultiMod); - Assert.IsTrue(combinations[3] is MultiMod); - - Assert.IsTrue(((MultiMod)combinations[2]).Mods[0] is ModA); - Assert.IsTrue(((MultiMod)combinations[2]).Mods[1] is ModB); - Assert.IsTrue(((MultiMod)combinations[2]).Mods[2] is ModC); - Assert.IsTrue(((MultiMod)combinations[3]).Mods[0] is ModB); - Assert.IsTrue(((MultiMod)combinations[3]).Mods[1] is ModC); + assertCombinations(new[] + { + new[] { typeof(ModNoMod) }, + new[] { typeof(ModA) }, + new[] { typeof(ModA), typeof(ModB), typeof(ModC) }, + new[] { typeof(ModB), typeof(ModC) } + }, combinations); } [Test] @@ -117,13 +117,12 @@ namespace osu.Game.Tests.NonVisual { var combinations = new TestLegacyDifficultyCalculator(new ModA(), new MultiMod(new ModB(), new ModIncompatibleWithA())).CreateDifficultyAdjustmentModCombinations(); - Assert.AreEqual(3, combinations.Length); - Assert.IsTrue(combinations[0] is ModNoMod); - Assert.IsTrue(combinations[1] is ModA); - Assert.IsTrue(combinations[2] is MultiMod); - - Assert.IsTrue(((MultiMod)combinations[2]).Mods[0] is ModB); - Assert.IsTrue(((MultiMod)combinations[2]).Mods[1] is ModIncompatibleWithA); + assertCombinations(new[] + { + new[] { typeof(ModNoMod) }, + new[] { typeof(ModA) }, + new[] { typeof(ModB), typeof(ModIncompatibleWithA) } + }, combinations); } [Test] @@ -131,13 +130,28 @@ namespace osu.Game.Tests.NonVisual { var combinations = new TestLegacyDifficultyCalculator(new ModA(), new MultiMod(new ModA(), new ModB())).CreateDifficultyAdjustmentModCombinations(); - Assert.AreEqual(3, combinations.Length); - Assert.IsTrue(combinations[0] is ModNoMod); - Assert.IsTrue(combinations[1] is ModA); - Assert.IsTrue(combinations[2] is MultiMod); + assertCombinations(new[] + { + new[] { typeof(ModNoMod) }, + new[] { typeof(ModA) }, + new[] { typeof(ModA), typeof(ModB) } + }, combinations); + } - Assert.IsTrue(((MultiMod)combinations[2]).Mods[0] is ModA); - Assert.IsTrue(((MultiMod)combinations[2]).Mods[1] is ModB); + private void assertCombinations(Type[][] expectedCombinations, Mod[] actualCombinations) + { + Assert.AreEqual(expectedCombinations.Length, actualCombinations.Length); + + foreach (Type[] expected in expectedCombinations) + { + Type[] actualTypes = ModUtils.FlattenMods(actualCombinations).Select(m => m.GetType()).ToArray(); + + Assert.Multiple(() => + { + foreach (var expectedType in expected) + Assert.Contains(expectedType, actualTypes); + }); + } } private class ModA : Mod From 049d9ce5ef5b906278b39bf83d9e5eac17284993 Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Tue, 30 Nov 2021 17:09:32 +0900 Subject: [PATCH 02/21] Add test coverage of match type propagating to other users' settings --- .../Visual/Multiplayer/TestSceneTeamVersus.cs | 20 +++++++++++++++++++ 1 file changed, 20 insertions(+) diff --git a/osu.Game.Tests/Visual/Multiplayer/TestSceneTeamVersus.cs b/osu.Game.Tests/Visual/Multiplayer/TestSceneTeamVersus.cs index c70906927e..7763e11609 100644 --- a/osu.Game.Tests/Visual/Multiplayer/TestSceneTeamVersus.cs +++ b/osu.Game.Tests/Visual/Multiplayer/TestSceneTeamVersus.cs @@ -11,6 +11,7 @@ using osu.Framework.Testing; using osu.Game.Beatmaps; using osu.Game.Database; using osu.Game.Online.API.Requests.Responses; +using osu.Game.Online.Multiplayer; using osu.Game.Online.Multiplayer.MatchTypes.TeamVersus; using osu.Game.Online.Rooms; using osu.Game.Rulesets; @@ -118,6 +119,25 @@ namespace osu.Game.Tests.Visual.Multiplayer AddAssert("user still on team 0", () => (client.Room?.Users.FirstOrDefault()?.MatchState as TeamVersusUserState)?.TeamID == 0); } + [Test] + public void TestSettingsUpdatedWhenChangingQueueMode() + { + createRoom(() => new Room + { + Name = { Value = "Test Room" }, + Type = { Value = MatchType.TeamVersus } + }); + + AddUntilStep("match type versus", () => client.APIRoom?.Type.Value == MatchType.HeadToHead); + + AddStep("change match type", () => client.ChangeSettings(new MultiplayerRoomSettings + { + MatchType = MatchType.TeamVersus + })); + + AddUntilStep("api room updated", () => client.APIRoom?.Type.Value == MatchType.TeamVersus); + } + [Test] public void TestChangeTypeViaMatchSettings() { From 25b9575de01d4a7d35dc19ece9b6748506f60cab Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Tue, 30 Nov 2021 17:09:40 +0900 Subject: [PATCH 03/21] Fix missing transfer of match type to settings --- osu.Game/Online/Multiplayer/MultiplayerClient.cs | 1 + 1 file changed, 1 insertion(+) diff --git a/osu.Game/Online/Multiplayer/MultiplayerClient.cs b/osu.Game/Online/Multiplayer/MultiplayerClient.cs index 4c472164d6..02fb94133a 100644 --- a/osu.Game/Online/Multiplayer/MultiplayerClient.cs +++ b/osu.Game/Online/Multiplayer/MultiplayerClient.cs @@ -694,6 +694,7 @@ namespace osu.Game.Online.Multiplayer Room.Settings = settings; APIRoom.Name.Value = Room.Settings.Name; APIRoom.Password.Value = Room.Settings.Password; + APIRoom.Type.Value = Room.Settings.MatchType; APIRoom.QueueMode.Value = Room.Settings.QueueMode; RoomUpdated?.Invoke(); From 8de06803a8f3285d683c8db38a818e5a9c4b4ced Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Tue, 30 Nov 2021 17:16:30 +0900 Subject: [PATCH 04/21] Fix incorrect test step text Co-authored-by: Dan Balasescu --- osu.Game.Tests/Visual/Multiplayer/TestSceneTeamVersus.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/osu.Game.Tests/Visual/Multiplayer/TestSceneTeamVersus.cs b/osu.Game.Tests/Visual/Multiplayer/TestSceneTeamVersus.cs index 7763e11609..e00939a4de 100644 --- a/osu.Game.Tests/Visual/Multiplayer/TestSceneTeamVersus.cs +++ b/osu.Game.Tests/Visual/Multiplayer/TestSceneTeamVersus.cs @@ -120,7 +120,7 @@ namespace osu.Game.Tests.Visual.Multiplayer } [Test] - public void TestSettingsUpdatedWhenChangingQueueMode() + public void TestSettingsUpdatedWhenChangingMatchType() { createRoom(() => new Room { From 23e297d414815477ed71f4e6e9360aee2bb8f7ce Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Tue, 30 Nov 2021 19:08:51 +0900 Subject: [PATCH 05/21] Log output response sizes Visibility is the first step towards action. Or something. --- osu.Game/Online/API/APIRequest.cs | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/osu.Game/Online/API/APIRequest.cs b/osu.Game/Online/API/APIRequest.cs index 43195811dc..efb0b102d0 100644 --- a/osu.Game/Online/API/APIRequest.cs +++ b/osu.Game/Online/API/APIRequest.cs @@ -38,7 +38,12 @@ namespace osu.Game.Online.API protected override void PostProcess() { base.PostProcess(); - Response = ((OsuJsonWebRequest)WebRequest)?.ResponseObject; + + if (WebRequest != null) + { + Response = ((OsuJsonWebRequest)WebRequest).ResponseObject; + Logger.Log($"{GetType()} finished with response size of {WebRequest.ResponseStream.Length:#,0} bytes"); + } } internal void TriggerSuccess(T result) From 87883f1fe433608090aebdeaa981632310fa5158 Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Tue, 30 Nov 2021 19:27:43 +0900 Subject: [PATCH 06/21] Add `BeatmapLookupCache` --- osu.Game/Database/BeatmapLookupCache.cs | 143 ++++++++++++++++++++++++ osu.Game/OsuGameBase.cs | 4 + 2 files changed, 147 insertions(+) create mode 100644 osu.Game/Database/BeatmapLookupCache.cs diff --git a/osu.Game/Database/BeatmapLookupCache.cs b/osu.Game/Database/BeatmapLookupCache.cs new file mode 100644 index 0000000000..c4e20d59b6 --- /dev/null +++ b/osu.Game/Database/BeatmapLookupCache.cs @@ -0,0 +1,143 @@ +// Copyright (c) ppy Pty Ltd . Licensed under the MIT Licence. +// See the LICENCE file in the repository root for full licence text. + +using System.Collections.Generic; +using System.Linq; +using System.Threading; +using System.Threading.Tasks; +using JetBrains.Annotations; +using osu.Framework.Allocation; +using osu.Game.Online.API; +using osu.Game.Online.API.Requests; +using osu.Game.Online.API.Requests.Responses; + +namespace osu.Game.Database +{ + // This class is based on `UserLookupCache` which is well tested. + // If modifications are to be made here, a base abstract implementation should likely be created and shared between the two. + public class BeatmapLookupCache : MemoryCachingComponent + { + [Resolved] + private IAPIProvider api { get; set; } + + /// + /// Perform an API lookup on the specified beatmap, populating a model. + /// + /// The beatmap to lookup. + /// An optional cancellation token. + /// The populated beatmap, or null if the beatmap does not exist or the request could not be satisfied. + [ItemCanBeNull] + public Task GetBeatmapAsync(int beatmapId, CancellationToken token = default) => GetAsync(beatmapId, token); + + /// + /// Perform an API lookup on the specified beatmaps, populating a model. + /// + /// The beatmaps to lookup. + /// An optional cancellation token. + /// The populated beatmaps. May include null results for failed retrievals. + public Task GetBeatmapsAsync(int[] beatmapIds, CancellationToken token = default) + { + var beatmapLookupTasks = new List>(); + + foreach (int u in beatmapIds) + { + beatmapLookupTasks.Add(GetBeatmapAsync(u, token).ContinueWith(task => + { + if (!task.IsCompletedSuccessfully) + return null; + + return task.Result; + }, token)); + } + + return Task.WhenAll(beatmapLookupTasks); + } + + protected override async Task ComputeValueAsync(int lookup, CancellationToken token = default) + => await queryBeatmap(lookup).ConfigureAwait(false); + + private readonly Queue<(int id, TaskCompletionSource)> pendingBeatmapTasks = new Queue<(int, TaskCompletionSource)>(); + private Task pendingRequestTask; + private readonly object taskAssignmentLock = new object(); + + private Task queryBeatmap(int beatmapId) + { + lock (taskAssignmentLock) + { + var tcs = new TaskCompletionSource(); + + // Add to the queue. + pendingBeatmapTasks.Enqueue((beatmapId, tcs)); + + // Create a request task if there's not already one. + if (pendingRequestTask == null) + createNewTask(); + + return tcs.Task; + } + } + + private void performLookup() + { + // contains at most 50 unique beatmap IDs from beatmapTasks, which is used to perform the lookup. + var beatmapTasks = new Dictionary>>(); + + // Grab at most 50 unique beatmap IDs from the queue. + lock (taskAssignmentLock) + { + while (pendingBeatmapTasks.Count > 0 && beatmapTasks.Count < 1) + { + (int id, TaskCompletionSource task) next = pendingBeatmapTasks.Dequeue(); + + // Perform a secondary check for existence, in case the beatmap was queried in a previous batch. + if (CheckExists(next.id, out var existing)) + next.task.SetResult(existing); + else + { + if (beatmapTasks.TryGetValue(next.id, out var tasks)) + tasks.Add(next.task); + else + beatmapTasks[next.id] = new List> { next.task }; + } + } + } + + // Query the beatmaps. + var request = new GetBeatmapRequest(new APIBeatmap { OnlineID = beatmapTasks.Keys.First() }); + + // rather than queueing, we maintain our own single-threaded request stream. + // todo: we probably want retry logic here. + api.Perform(request); + + // Create a new request task if there's still more beatmaps to query. + lock (taskAssignmentLock) + { + pendingRequestTask = null; + if (pendingBeatmapTasks.Count > 0) + createNewTask(); + } + + List foundBeatmaps = new List { request.Response }; + + foreach (var beatmap in foundBeatmaps) + { + if (beatmapTasks.TryGetValue(beatmap.OnlineID, out var tasks)) + { + foreach (var task in tasks) + task.SetResult(beatmap); + + beatmapTasks.Remove(beatmap.OnlineID); + } + } + + // if any tasks remain which were not satisfied, return null. + foreach (var tasks in beatmapTasks.Values) + { + foreach (var task in tasks) + task.SetResult(null); + } + } + + private void createNewTask() => pendingRequestTask = Task.Run(performLookup); + } +} diff --git a/osu.Game/OsuGameBase.cs b/osu.Game/OsuGameBase.cs index 6eb67b34e8..34344f8022 100644 --- a/osu.Game/OsuGameBase.cs +++ b/osu.Game/OsuGameBase.cs @@ -142,6 +142,7 @@ namespace osu.Game private BeatmapDifficultyCache difficultyCache; private UserLookupCache userCache; + private BeatmapLookupCache beatmapCache; private FileStore fileStore; @@ -265,6 +266,9 @@ namespace osu.Game dependencies.Cache(userCache = new UserLookupCache()); AddInternal(userCache); + dependencies.Cache(beatmapCache = new BeatmapLookupCache()); + AddInternal(beatmapCache); + var scorePerformanceManager = new ScorePerformanceCache(); dependencies.Cache(scorePerformanceManager); AddInternal(scorePerformanceManager); From f58c5cd9c0f2d9a6ffaf0457d11cb62cc124249e Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Tue, 30 Nov 2021 19:36:25 +0900 Subject: [PATCH 07/21] Update `MultiplayerClient` to use `BeatmapLookupCache` --- .../Online/Multiplayer/MultiplayerClient.cs | 20 +++++--------- .../Multiplayer/OnlineMultiplayerClient.cs | 27 +++++-------------- .../Multiplayer/TestMultiplayerClient.cs | 15 ++++++----- 3 files changed, 20 insertions(+), 42 deletions(-) diff --git a/osu.Game/Online/Multiplayer/MultiplayerClient.cs b/osu.Game/Online/Multiplayer/MultiplayerClient.cs index 4c472164d6..aa91d5e56f 100644 --- a/osu.Game/Online/Multiplayer/MultiplayerClient.cs +++ b/osu.Game/Online/Multiplayer/MultiplayerClient.cs @@ -702,15 +702,7 @@ namespace osu.Game.Online.Multiplayer private async Task createPlaylistItem(MultiplayerPlaylistItem item) { - var set = await GetOnlineBeatmapSet(item.BeatmapID).ConfigureAwait(false); - - // The incoming response is deserialised without circular reference handling currently. - // Because we require using metadata from this instance, populate the nested beatmaps' sets manually here. - foreach (var b in set.Beatmaps) - b.BeatmapSet = set; - - var beatmap = set.Beatmaps.Single(b => b.OnlineID == item.BeatmapID); - beatmap.Checksum = item.BeatmapChecksum; + var apiBeatmap = await GetOnlineBeatmapSet(item.BeatmapID).ConfigureAwait(false); var ruleset = Rulesets.GetRuleset(item.RulesetID); var rulesetInstance = ruleset.CreateInstance(); @@ -719,7 +711,7 @@ namespace osu.Game.Online.Multiplayer { ID = item.ID, OwnerID = item.OwnerID, - Beatmap = { Value = beatmap }, + Beatmap = { Value = apiBeatmap }, Ruleset = { Value = ruleset }, Expired = item.Expired }; @@ -731,12 +723,12 @@ namespace osu.Game.Online.Multiplayer } /// - /// Retrieves a from an online source. + /// Retrieves a from an online source. /// - /// The beatmap set ID. + /// The beatmap ID. /// A token to cancel the request. - /// The retrieval task. - protected abstract Task GetOnlineBeatmapSet(int beatmapId, CancellationToken cancellationToken = default); + /// The retrieval task. + protected abstract Task GetOnlineBeatmapSet(int beatmapId, CancellationToken cancellationToken = default); /// /// For the provided user ID, update whether the user is included in . diff --git a/osu.Game/Online/Multiplayer/OnlineMultiplayerClient.cs b/osu.Game/Online/Multiplayer/OnlineMultiplayerClient.cs index 7308c03ec3..de974cb3e1 100644 --- a/osu.Game/Online/Multiplayer/OnlineMultiplayerClient.cs +++ b/osu.Game/Online/Multiplayer/OnlineMultiplayerClient.cs @@ -9,8 +9,8 @@ using System.Threading.Tasks; using Microsoft.AspNetCore.SignalR.Client; using osu.Framework.Allocation; using osu.Framework.Bindables; +using osu.Game.Database; using osu.Game.Online.API; -using osu.Game.Online.API.Requests; using osu.Game.Online.API.Requests.Responses; using osu.Game.Online.Rooms; @@ -29,6 +29,9 @@ namespace osu.Game.Online.Multiplayer private HubConnection? connection => connector?.CurrentConnection; + [Resolved] + private BeatmapLookupCache beatmapLookupCache { get; set; } = null!; + public OnlineMultiplayerClient(EndpointConfiguration endpoints) { endpoint = endpoints.MultiplayerEndpointUrl; @@ -159,27 +162,9 @@ namespace osu.Game.Online.Multiplayer return connection.InvokeAsync(nameof(IMultiplayerServer.AddPlaylistItem), item); } - protected override Task GetOnlineBeatmapSet(int beatmapId, CancellationToken cancellationToken = default) + protected override Task GetOnlineBeatmapSet(int beatmapId, CancellationToken cancellationToken = default) { - var tcs = new TaskCompletionSource(); - var req = new GetBeatmapSetRequest(beatmapId, BeatmapSetLookupType.BeatmapId); - - req.Success += res => - { - if (cancellationToken.IsCancellationRequested) - { - tcs.SetCanceled(); - return; - } - - tcs.SetResult(res); - }; - - req.Failure += e => tcs.SetException(e); - - API.Queue(req); - - return tcs.Task; + return beatmapLookupCache.GetBeatmapAsync(beatmapId, cancellationToken); } protected override void Dispose(bool isDisposing) diff --git a/osu.Game/Tests/Visual/Multiplayer/TestMultiplayerClient.cs b/osu.Game/Tests/Visual/Multiplayer/TestMultiplayerClient.cs index 2d77e17513..39199ae304 100644 --- a/osu.Game/Tests/Visual/Multiplayer/TestMultiplayerClient.cs +++ b/osu.Game/Tests/Visual/Multiplayer/TestMultiplayerClient.cs @@ -336,7 +336,7 @@ namespace osu.Game.Tests.Visual.Multiplayer public override Task AddPlaylistItem(MultiplayerPlaylistItem item) => AddUserPlaylistItem(api.LocalUser.Value.OnlineID, item); - protected override Task GetOnlineBeatmapSet(int beatmapId, CancellationToken cancellationToken = default) + protected override Task GetOnlineBeatmapSet(int beatmapId, CancellationToken cancellationToken = default) { IBeatmapSetInfo? set = roomManager.ServerSideRooms.SelectMany(r => r.Playlist) .FirstOrDefault(p => p.BeatmapID == beatmapId)?.Beatmap.Value.BeatmapSet @@ -345,13 +345,14 @@ namespace osu.Game.Tests.Visual.Multiplayer if (set == null) throw new InvalidOperationException("Beatmap not found."); - var apiSet = new APIBeatmapSet + return Task.FromResult(new APIBeatmap { - OnlineID = set.OnlineID, - Beatmaps = set.Beatmaps.Select(b => new APIBeatmap { OnlineID = b.OnlineID }).ToArray(), - }; - - return Task.FromResult(apiSet); + BeatmapSet = new APIBeatmapSet + { + OnlineID = set.OnlineID, + }, + OnlineID = set.Beatmaps.First().OnlineID + }); } private async Task changeMatchType(MatchType type) From 01bc330d1ce8468cecd95f83bca02be187730867 Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Tue, 30 Nov 2021 19:42:46 +0900 Subject: [PATCH 08/21] Rename method to match new purpose --- osu.Game/Online/Multiplayer/MultiplayerClient.cs | 4 ++-- osu.Game/Online/Multiplayer/OnlineMultiplayerClient.cs | 2 +- osu.Game/Tests/Visual/Multiplayer/TestMultiplayerClient.cs | 2 +- 3 files changed, 4 insertions(+), 4 deletions(-) diff --git a/osu.Game/Online/Multiplayer/MultiplayerClient.cs b/osu.Game/Online/Multiplayer/MultiplayerClient.cs index aa91d5e56f..478da983cd 100644 --- a/osu.Game/Online/Multiplayer/MultiplayerClient.cs +++ b/osu.Game/Online/Multiplayer/MultiplayerClient.cs @@ -702,7 +702,7 @@ namespace osu.Game.Online.Multiplayer private async Task createPlaylistItem(MultiplayerPlaylistItem item) { - var apiBeatmap = await GetOnlineBeatmapSet(item.BeatmapID).ConfigureAwait(false); + var apiBeatmap = await GetAPIBeatmap(item.BeatmapID).ConfigureAwait(false); var ruleset = Rulesets.GetRuleset(item.RulesetID); var rulesetInstance = ruleset.CreateInstance(); @@ -728,7 +728,7 @@ namespace osu.Game.Online.Multiplayer /// The beatmap ID. /// A token to cancel the request. /// The retrieval task. - protected abstract Task GetOnlineBeatmapSet(int beatmapId, CancellationToken cancellationToken = default); + protected abstract Task GetAPIBeatmap(int beatmapId, CancellationToken cancellationToken = default); /// /// For the provided user ID, update whether the user is included in . diff --git a/osu.Game/Online/Multiplayer/OnlineMultiplayerClient.cs b/osu.Game/Online/Multiplayer/OnlineMultiplayerClient.cs index de974cb3e1..41687b54b0 100644 --- a/osu.Game/Online/Multiplayer/OnlineMultiplayerClient.cs +++ b/osu.Game/Online/Multiplayer/OnlineMultiplayerClient.cs @@ -162,7 +162,7 @@ namespace osu.Game.Online.Multiplayer return connection.InvokeAsync(nameof(IMultiplayerServer.AddPlaylistItem), item); } - protected override Task GetOnlineBeatmapSet(int beatmapId, CancellationToken cancellationToken = default) + protected override Task GetAPIBeatmap(int beatmapId, CancellationToken cancellationToken = default) { return beatmapLookupCache.GetBeatmapAsync(beatmapId, cancellationToken); } diff --git a/osu.Game/Tests/Visual/Multiplayer/TestMultiplayerClient.cs b/osu.Game/Tests/Visual/Multiplayer/TestMultiplayerClient.cs index 39199ae304..037dccc63f 100644 --- a/osu.Game/Tests/Visual/Multiplayer/TestMultiplayerClient.cs +++ b/osu.Game/Tests/Visual/Multiplayer/TestMultiplayerClient.cs @@ -336,7 +336,7 @@ namespace osu.Game.Tests.Visual.Multiplayer public override Task AddPlaylistItem(MultiplayerPlaylistItem item) => AddUserPlaylistItem(api.LocalUser.Value.OnlineID, item); - protected override Task GetOnlineBeatmapSet(int beatmapId, CancellationToken cancellationToken = default) + protected override Task GetAPIBeatmap(int beatmapId, CancellationToken cancellationToken = default) { IBeatmapSetInfo? set = roomManager.ServerSideRooms.SelectMany(r => r.Playlist) .FirstOrDefault(p => p.BeatmapID == beatmapId)?.Beatmap.Value.BeatmapSet From 0fae10500a2860933f7125da02b1c4c2e09d80b1 Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Tue, 30 Nov 2021 20:02:15 +0900 Subject: [PATCH 09/21] Fix failing tests --- .../Tests/Visual/Multiplayer/TestMultiplayerClient.cs | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/osu.Game/Tests/Visual/Multiplayer/TestMultiplayerClient.cs b/osu.Game/Tests/Visual/Multiplayer/TestMultiplayerClient.cs index 037dccc63f..05b7c11e34 100644 --- a/osu.Game/Tests/Visual/Multiplayer/TestMultiplayerClient.cs +++ b/osu.Game/Tests/Visual/Multiplayer/TestMultiplayerClient.cs @@ -347,11 +347,9 @@ namespace osu.Game.Tests.Visual.Multiplayer return Task.FromResult(new APIBeatmap { - BeatmapSet = new APIBeatmapSet - { - OnlineID = set.OnlineID, - }, - OnlineID = set.Beatmaps.First().OnlineID + BeatmapSet = new APIBeatmapSet { OnlineID = set.OnlineID }, + OnlineID = beatmapId, + Checksum = set.Beatmaps.First(b => b.OnlineID == beatmapId).MD5Hash }); } From d18417aa2706d1eae6ac1094d942d4538372c0e9 Mon Sep 17 00:00:00 2001 From: Salman Ahmed Date: Tue, 23 Nov 2021 12:04:27 +0300 Subject: [PATCH 10/21] Enable build-only iOS CI --- .github/workflows/ci.yml | 4 ---- 1 file changed, 4 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 68f8ef51ef..3c52802cf6 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -77,10 +77,6 @@ jobs: run: msbuild osu.Android/osu.Android.csproj /restore /p:Configuration=Debug build-only-ios: - # While this workflow technically *can* run, it fails as iOS builds are blocked by multiple issues. - # See https://github.com/ppy/osu-framework/issues/4677 for the details. - # The job can be unblocked once those issues are resolved and game deployments can happen again. - if: false name: Build only (iOS) runs-on: macos-latest timeout-minutes: 60 From 03e1305b3f54639fd4375d715faaf7257c8adec2 Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Wed, 1 Dec 2021 01:55:14 +0900 Subject: [PATCH 11/21] Fix toast display potentially causing a child mutation before load complete --- osu.Game/Overlays/OnScreenDisplay.cs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/osu.Game/Overlays/OnScreenDisplay.cs b/osu.Game/Overlays/OnScreenDisplay.cs index af6d24fc65..be9d3cd794 100644 --- a/osu.Game/Overlays/OnScreenDisplay.cs +++ b/osu.Game/Overlays/OnScreenDisplay.cs @@ -95,13 +95,13 @@ namespace osu.Game.Overlays /// Displays the provided temporarily. /// /// - public void Display(Toast toast) + public void Display(Toast toast) => Schedule(() => { box.Child = toast; DisplayTemporarily(box); - } + }); - private void displayTrackedSettingChange(SettingDescription description) => Schedule(() => Display(new TrackedSettingToast(description))); + private void displayTrackedSettingChange(SettingDescription description) => Display(new TrackedSettingToast(description)); private TransformSequence fadeIn; private ScheduledDelegate fadeOut; From dda7142f48226f9db18e96efd8ce06b1fb286f47 Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Wed, 1 Dec 2021 02:06:40 +0900 Subject: [PATCH 12/21] Fix double screen exit in multiplayer song select tests potentially causing failure --- .../Visual/Multiplayer/TestSceneMultiplayerMatchSongSelect.cs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/osu.Game.Tests/Visual/Multiplayer/TestSceneMultiplayerMatchSongSelect.cs b/osu.Game.Tests/Visual/Multiplayer/TestSceneMultiplayerMatchSongSelect.cs index 84b24ba3a1..a5229702a8 100644 --- a/osu.Game.Tests/Visual/Multiplayer/TestSceneMultiplayerMatchSongSelect.cs +++ b/osu.Game.Tests/Visual/Multiplayer/TestSceneMultiplayerMatchSongSelect.cs @@ -144,7 +144,8 @@ namespace osu.Game.Tests.Visual.Multiplayer AddStep("set mods", () => SelectedMods.Value = new[] { new TaikoModDoubleTime() }); AddStep("confirm selection", () => songSelect.FinaliseSelection()); - AddStep("exit song select", () => songSelect.Exit()); + + AddUntilStep("song select exited", () => !songSelect.IsCurrentScreen()); AddAssert("beatmap not changed", () => Beatmap.Value.BeatmapInfo.Equals(selectedBeatmap)); AddAssert("ruleset not changed", () => Ruleset.Value.Equals(new TaikoRuleset().RulesetInfo)); From b74b09eb3a7f18634bc84696ba9a57dbca0f7f1d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bart=C5=82omiej=20Dach?= Date: Tue, 30 Nov 2021 20:56:11 +0100 Subject: [PATCH 13/21] Add extra until step to make cause of potential failures more obvious --- osu.Game.Tests/Visual/Multiplayer/TestSceneTeamVersus.cs | 1 + 1 file changed, 1 insertion(+) diff --git a/osu.Game.Tests/Visual/Multiplayer/TestSceneTeamVersus.cs b/osu.Game.Tests/Visual/Multiplayer/TestSceneTeamVersus.cs index e00939a4de..ab9dc9c989 100644 --- a/osu.Game.Tests/Visual/Multiplayer/TestSceneTeamVersus.cs +++ b/osu.Game.Tests/Visual/Multiplayer/TestSceneTeamVersus.cs @@ -172,6 +172,7 @@ namespace osu.Game.Tests.Visual.Multiplayer AddUntilStep("wait for room open", () => this.ChildrenOfType().FirstOrDefault()?.IsLoaded == true); AddWaitStep("wait for transition", 2); + AddUntilStep("create room button enabled", () => this.ChildrenOfType().Single().Enabled.Value); AddStep("create room", () => { InputManager.MoveMouseTo(this.ChildrenOfType().Single()); From de034b4d9c93d09e9ce4695fbccf13fe648f9414 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bart=C5=82omiej=20Dach?= Date: Tue, 30 Nov 2021 20:57:02 +0100 Subject: [PATCH 14/21] Fix test failures due to wrong asserts & uninitialised playlist * The "create room" button was disabled headless due to not specifying the imported beatmap. In visual tests it seems to work due to selecting from the local database randomly. * The test asserts are brought in line with expectations. --- .../Visual/Multiplayer/TestSceneTeamVersus.cs | 14 +++++++++++--- 1 file changed, 11 insertions(+), 3 deletions(-) diff --git a/osu.Game.Tests/Visual/Multiplayer/TestSceneTeamVersus.cs b/osu.Game.Tests/Visual/Multiplayer/TestSceneTeamVersus.cs index ab9dc9c989..981989c28a 100644 --- a/osu.Game.Tests/Visual/Multiplayer/TestSceneTeamVersus.cs +++ b/osu.Game.Tests/Visual/Multiplayer/TestSceneTeamVersus.cs @@ -125,17 +125,25 @@ namespace osu.Game.Tests.Visual.Multiplayer createRoom(() => new Room { Name = { Value = "Test Room" }, - Type = { Value = MatchType.TeamVersus } + Type = { Value = MatchType.HeadToHead }, + Playlist = + { + new PlaylistItem + { + Beatmap = { Value = beatmaps.GetWorkingBeatmap(importedSet.Beatmaps.First(b => b.RulesetID == 0)).BeatmapInfo }, + Ruleset = { Value = new OsuRuleset().RulesetInfo }, + } + } }); - AddUntilStep("match type versus", () => client.APIRoom?.Type.Value == MatchType.HeadToHead); + AddUntilStep("match type head to head", () => client.APIRoom?.Type.Value == MatchType.HeadToHead); AddStep("change match type", () => client.ChangeSettings(new MultiplayerRoomSettings { MatchType = MatchType.TeamVersus })); - AddUntilStep("api room updated", () => client.APIRoom?.Type.Value == MatchType.TeamVersus); + AddUntilStep("api room updated to team versus", () => client.APIRoom?.Type.Value == MatchType.TeamVersus); } [Test] From 0479027f644c686cf65d3ee9ceb65e14bd0e42e4 Mon Sep 17 00:00:00 2001 From: Dan Balasescu Date: Wed, 1 Dec 2021 11:29:02 +0900 Subject: [PATCH 15/21] Apply assertion fix from review --- .../DifficultyAdjustmentModCombinationsTest.cs | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/osu.Game.Tests/NonVisual/DifficultyAdjustmentModCombinationsTest.cs b/osu.Game.Tests/NonVisual/DifficultyAdjustmentModCombinationsTest.cs index 612927337f..ae8eec2629 100644 --- a/osu.Game.Tests/NonVisual/DifficultyAdjustmentModCombinationsTest.cs +++ b/osu.Game.Tests/NonVisual/DifficultyAdjustmentModCombinationsTest.cs @@ -142,16 +142,16 @@ namespace osu.Game.Tests.NonVisual { Assert.AreEqual(expectedCombinations.Length, actualCombinations.Length); - foreach (Type[] expected in expectedCombinations) + Assert.Multiple(() => { - Type[] actualTypes = ModUtils.FlattenMods(actualCombinations).Select(m => m.GetType()).ToArray(); - - Assert.Multiple(() => + for (int i = 0; i < expectedCombinations.Length; ++i) { - foreach (var expectedType in expected) - Assert.Contains(expectedType, actualTypes); - }); - } + Type[] expectedTypes = expectedCombinations[i]; + Type[] actualTypes = ModUtils.FlattenMod(actualCombinations[i]).Select(m => m.GetType()).ToArray(); + + Assert.That(expectedTypes, Is.EquivalentTo(actualTypes)); + } + }); } private class ModA : Mod From 4306420922ef927a88101f1322b3cc5b181ce2ba Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Wed, 1 Dec 2021 15:09:51 +0900 Subject: [PATCH 16/21] Add extension methods to add extra safety to realm subscriptions Also adjusts the naming and documentation to make it (hopefully) easier to understand what this method/process implies. --- osu.Game.Tests/Database/GeneralUsageTests.cs | 6 +- osu.Game.Tests/Database/RealmLiveTests.cs | 2 +- osu.Game/Database/RealmObjectExtensions.cs | 106 ++++++++++++++++++ .../Bindings/DatabasedKeyBindingContainer.cs | 3 +- 4 files changed, 111 insertions(+), 6 deletions(-) diff --git a/osu.Game.Tests/Database/GeneralUsageTests.cs b/osu.Game.Tests/Database/GeneralUsageTests.cs index 841bf2de43..2285b22a3a 100644 --- a/osu.Game.Tests/Database/GeneralUsageTests.cs +++ b/osu.Game.Tests/Database/GeneralUsageTests.cs @@ -5,8 +5,8 @@ using System; using System.Threading; using System.Threading.Tasks; using NUnit.Framework; +using osu.Game.Database; using osu.Game.Models; -using Realms; #nullable enable @@ -48,7 +48,7 @@ namespace osu.Game.Tests.Database using (var context = realmFactory.CreateContext()) { - var subscription = context.All().SubscribeForNotifications((sender, changes, error) => + var subscription = context.All().QueryAsyncWithNotifications((sender, changes, error) => { using (realmFactory.CreateContext()) { @@ -61,7 +61,7 @@ namespace osu.Game.Tests.Database { } - subscription.Dispose(); + subscription?.Dispose(); } Assert.IsTrue(callbackRan); diff --git a/osu.Game.Tests/Database/RealmLiveTests.cs b/osu.Game.Tests/Database/RealmLiveTests.cs index 8ab19c8329..9b6769b788 100644 --- a/osu.Game.Tests/Database/RealmLiveTests.cs +++ b/osu.Game.Tests/Database/RealmLiveTests.cs @@ -208,7 +208,7 @@ namespace osu.Game.Tests.Database using (var updateThreadContext = realmFactory.CreateContext()) { - updateThreadContext.All().SubscribeForNotifications(gotChange); + updateThreadContext.All().QueryAsyncWithNotifications(gotChange); ILive? liveBeatmap = null; Task.Factory.StartNew(() => diff --git a/osu.Game/Database/RealmObjectExtensions.cs b/osu.Game/Database/RealmObjectExtensions.cs index ac4ca436ad..170756e743 100644 --- a/osu.Game/Database/RealmObjectExtensions.cs +++ b/osu.Game/Database/RealmObjectExtensions.cs @@ -1,12 +1,16 @@ // Copyright (c) ppy Pty Ltd . Licensed under the MIT Licence. // See the LICENCE file in the repository root for full licence text. +using System; using System.Collections.Generic; using System.Linq; using AutoMapper; +using osu.Framework.Development; using osu.Game.Input.Bindings; using Realms; +#nullable enable + namespace osu.Game.Database { public static class RealmObjectExtensions @@ -60,5 +64,107 @@ namespace osu.Game.Database { return new RealmLive(realmObject); } + + /// + /// Register a callback to be invoked each time this changes. + /// + /// + /// + /// This adds osu! specific thread and managed state safety checks on top of . + /// + /// + /// The first callback will be invoked with the initial after the asynchronous query completes, + /// and then called again after each write transaction which changes either any of the objects in the collection, or + /// which objects are in the collection. The changes parameter will + /// be null the first time the callback is invoked with the initial results. For each call after that, + /// it will contain information about which rows in the results were added, removed or modified. + /// + /// + /// If a write transaction did not modify any objects in this , the callback is not invoked at all. + /// If an error occurs the callback will be invoked with null for the sender parameter and a non-null error. + /// Currently the only errors that can occur are when opening the on the background worker thread. + /// + /// + /// At the time when the block is called, the object will be fully evaluated + /// and up-to-date, and as long as you do not perform a write transaction on the same thread + /// or explicitly call , accessing it will never perform blocking work. + /// + /// + /// Notifications are delivered via the standard event loop, and so can't be delivered while the event loop is blocked by other activity. + /// When notifications can't be delivered instantly, multiple notifications may be coalesced into a single notification. + /// This can include the notification with the initial collection. + /// + /// + /// The to observe for changes. + /// The callback to be invoked with the updated . + /// + /// A subscription token. It must be kept alive for as long as you want to receive change notifications. + /// To stop receiving notifications, call . + /// + /// + /// + public static IDisposable? QueryAsyncWithNotifications(this IRealmCollection collection, NotificationCallbackDelegate callback) + where T : RealmObjectBase + { + // Subscriptions can only work on the main thread. + if (!ThreadSafety.IsUpdateThread) + throw new InvalidOperationException("Cannot subscribe for realm notifications from a non-update thread."); + + return collection.SubscribeForNotifications(callback); + } + + /// + /// A convenience method that casts to and subscribes for change notifications. + /// + /// + /// This adds osu! specific thread and managed state safety checks on top of . + /// + /// The to observe for changes. + /// Type of the elements in the list. + /// + /// The callback to be invoked with the updated . + /// + /// A subscription token. It must be kept alive for as long as you want to receive change notifications. + /// To stop receiving notifications, call . + /// + /// May be null in the case the provided collection is not managed. + /// + public static IDisposable? QueryAsyncWithNotifications(this IQueryable list, NotificationCallbackDelegate callback) + where T : RealmObjectBase + { + // Subscribing to non-managed instances doesn't work. + // In this usage, the skin may be non-managed in tests. + if (!(list is IRealmCollection realmCollection)) + return null; + + return QueryAsyncWithNotifications(realmCollection, callback); + } + + /// + /// A convenience method that casts to and subscribes for change notifications. + /// + /// + /// This adds osu! specific thread and managed state safety checks on top of . + /// + /// The to observe for changes. + /// Type of the elements in the list. + /// + /// The callback to be invoked with the updated . + /// + /// A subscription token. It must be kept alive for as long as you want to receive change notifications. + /// To stop receiving notifications, call . + /// + /// May be null in the case the provided collection is not managed. + /// + public static IDisposable? QueryAsyncWithNotifications(this IList list, NotificationCallbackDelegate callback) + where T : RealmObjectBase + { + // Subscribing to non-managed instances doesn't work. + // In this usage, the skin may be non-managed in tests. + if (!(list is IRealmCollection realmCollection)) + return null; + + return QueryAsyncWithNotifications(realmCollection, callback); + } } } diff --git a/osu.Game/Input/Bindings/DatabasedKeyBindingContainer.cs b/osu.Game/Input/Bindings/DatabasedKeyBindingContainer.cs index baa5b9ff9c..f95c884fe5 100644 --- a/osu.Game/Input/Bindings/DatabasedKeyBindingContainer.cs +++ b/osu.Game/Input/Bindings/DatabasedKeyBindingContainer.cs @@ -8,7 +8,6 @@ using osu.Framework.Allocation; using osu.Framework.Input.Bindings; using osu.Game.Database; using osu.Game.Rulesets; -using Realms; namespace osu.Game.Input.Bindings { @@ -56,7 +55,7 @@ namespace osu.Game.Input.Bindings .Where(b => b.RulesetName == rulesetName && b.Variant == variant); realmSubscription = realmKeyBindings - .SubscribeForNotifications((sender, changes, error) => + .QueryAsyncWithNotifications((sender, changes, error) => { // first subscription ignored as we are handling this in LoadComplete. if (changes == null) From a439209535ec8a439aab350225b1e6640c6e3cc6 Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Wed, 1 Dec 2021 15:23:28 +0900 Subject: [PATCH 17/21] Add `BannedSymbols` rules for `SubscribeForNotifications` variants we use --- CodeAnalysis/BannedSymbols.txt | 3 +++ 1 file changed, 3 insertions(+) diff --git a/CodeAnalysis/BannedSymbols.txt b/CodeAnalysis/BannedSymbols.txt index b72803482d..c567adc0ae 100644 --- a/CodeAnalysis/BannedSymbols.txt +++ b/CodeAnalysis/BannedSymbols.txt @@ -10,3 +10,6 @@ T:Microsoft.EntityFrameworkCore.Internal.EnumerableExtensions;Don't use internal T:Microsoft.EntityFrameworkCore.Internal.TypeExtensions;Don't use internal extension methods. T:NuGet.Packaging.CollectionExtensions;Don't use internal extension methods. M:System.Enum.HasFlag(System.Enum);Use osu.Framework.Extensions.EnumExtensions.HasFlagFast() instead. +M:Realms.IRealmCollection`1.SubscribeForNotifications`1(Realms.NotificationCallbackDelegate{``0});Use osu.Game.Database.RealmObjectExtensions.QueryAsyncWithNotifications(IRealmCollection,NotificationCallbackDelegate) instead. +M:Realms.CollectionExtensions.SubscribeForNotifications`1(System.Linq.IQueryable{``0},Realms.NotificationCallbackDelegate{``0});Use osu.Game.Database.RealmObjectExtensions.QueryAsyncWithNotifications(IQueryable,NotificationCallbackDelegate) instead. +M:Realms.CollectionExtensions.SubscribeForNotifications`1(System.Collections.Generic.IList{``0},Realms.NotificationCallbackDelegate{``0});Use osu.Game.Database.RealmObjectExtensions.QueryAsyncWithNotifications(IList,NotificationCallbackDelegate) instead. From 09817ff161aa6768aa6fbf9828b62f572d075402 Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Wed, 1 Dec 2021 15:27:10 +0900 Subject: [PATCH 18/21] Add missing `returns` additional documentation to main method call --- osu.Game/Database/RealmObjectExtensions.cs | 2 ++ 1 file changed, 2 insertions(+) diff --git a/osu.Game/Database/RealmObjectExtensions.cs b/osu.Game/Database/RealmObjectExtensions.cs index 170756e743..cb38c910da 100644 --- a/osu.Game/Database/RealmObjectExtensions.cs +++ b/osu.Game/Database/RealmObjectExtensions.cs @@ -100,6 +100,8 @@ namespace osu.Game.Database /// /// A subscription token. It must be kept alive for as long as you want to receive change notifications. /// To stop receiving notifications, call . + /// + /// May be null in the case the provided collection is not managed. /// /// /// From 81f82c24c39014decc49b676e68cca913db95645 Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Wed, 1 Dec 2021 17:45:41 +0900 Subject: [PATCH 19/21] Use new API endpoint to do batch lookups --- osu.Game/Database/BeatmapLookupCache.cs | 19 ++++++++------- .../Online/API/Requests/GetBeatmapsRequest.cs | 24 +++++++++++++++++++ .../API/Requests/GetBeatmapsResponse.cs | 15 ++++++++++++ 3 files changed, 50 insertions(+), 8 deletions(-) create mode 100644 osu.Game/Online/API/Requests/GetBeatmapsRequest.cs create mode 100644 osu.Game/Online/API/Requests/GetBeatmapsResponse.cs diff --git a/osu.Game/Database/BeatmapLookupCache.cs b/osu.Game/Database/BeatmapLookupCache.cs index c4e20d59b6..2082031714 100644 --- a/osu.Game/Database/BeatmapLookupCache.cs +++ b/osu.Game/Database/BeatmapLookupCache.cs @@ -85,7 +85,7 @@ namespace osu.Game.Database // Grab at most 50 unique beatmap IDs from the queue. lock (taskAssignmentLock) { - while (pendingBeatmapTasks.Count > 0 && beatmapTasks.Count < 1) + while (pendingBeatmapTasks.Count > 0 && beatmapTasks.Count < 50) { (int id, TaskCompletionSource task) next = pendingBeatmapTasks.Dequeue(); @@ -103,7 +103,7 @@ namespace osu.Game.Database } // Query the beatmaps. - var request = new GetBeatmapRequest(new APIBeatmap { OnlineID = beatmapTasks.Keys.First() }); + var request = new GetBeatmapsRequest(beatmapTasks.Keys.ToArray()); // rather than queueing, we maintain our own single-threaded request stream. // todo: we probably want retry logic here. @@ -117,16 +117,19 @@ namespace osu.Game.Database createNewTask(); } - List foundBeatmaps = new List { request.Response }; + List foundBeatmaps = request.Response?.Beatmaps; - foreach (var beatmap in foundBeatmaps) + if (foundBeatmaps != null) { - if (beatmapTasks.TryGetValue(beatmap.OnlineID, out var tasks)) + foreach (var beatmap in foundBeatmaps) { - foreach (var task in tasks) - task.SetResult(beatmap); + if (beatmapTasks.TryGetValue(beatmap.OnlineID, out var tasks)) + { + foreach (var task in tasks) + task.SetResult(beatmap); - beatmapTasks.Remove(beatmap.OnlineID); + beatmapTasks.Remove(beatmap.OnlineID); + } } } diff --git a/osu.Game/Online/API/Requests/GetBeatmapsRequest.cs b/osu.Game/Online/API/Requests/GetBeatmapsRequest.cs new file mode 100644 index 0000000000..1d71e22b77 --- /dev/null +++ b/osu.Game/Online/API/Requests/GetBeatmapsRequest.cs @@ -0,0 +1,24 @@ +// Copyright (c) ppy Pty Ltd . Licensed under the MIT Licence. +// See the LICENCE file in the repository root for full licence text. + +using System; + +namespace osu.Game.Online.API.Requests +{ + public class GetBeatmapsRequest : APIRequest + { + private readonly int[] beatmapIds; + + private const int max_ids_per_request = 50; + + public GetBeatmapsRequest(int[] beatmapIds) + { + if (beatmapIds.Length > max_ids_per_request) + throw new ArgumentException($"{nameof(GetBeatmapsRequest)} calls only support up to {max_ids_per_request} IDs at once"); + + this.beatmapIds = beatmapIds; + } + + protected override string Target => "beatmaps/?ids[]=" + string.Join("&ids[]=", beatmapIds); + } +} diff --git a/osu.Game/Online/API/Requests/GetBeatmapsResponse.cs b/osu.Game/Online/API/Requests/GetBeatmapsResponse.cs new file mode 100644 index 0000000000..c450c3269c --- /dev/null +++ b/osu.Game/Online/API/Requests/GetBeatmapsResponse.cs @@ -0,0 +1,15 @@ +// Copyright (c) ppy Pty Ltd . Licensed under the MIT Licence. +// See the LICENCE file in the repository root for full licence text. + +using System.Collections.Generic; +using Newtonsoft.Json; +using osu.Game.Online.API.Requests.Responses; + +namespace osu.Game.Online.API.Requests +{ + public class GetBeatmapsResponse : ResponseWithCursor + { + [JsonProperty("beatmaps")] + public List Beatmaps; + } +} From bf5a186a2b03774d7f51dd6a591a7d529894f534 Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Wed, 1 Dec 2021 17:47:16 +0900 Subject: [PATCH 20/21] Add early abort to avoid sending empty lookup requests --- osu.Game/Database/BeatmapLookupCache.cs | 3 +++ osu.Game/Database/UserLookupCache.cs | 3 +++ 2 files changed, 6 insertions(+) diff --git a/osu.Game/Database/BeatmapLookupCache.cs b/osu.Game/Database/BeatmapLookupCache.cs index 2082031714..c6f8244494 100644 --- a/osu.Game/Database/BeatmapLookupCache.cs +++ b/osu.Game/Database/BeatmapLookupCache.cs @@ -102,6 +102,9 @@ namespace osu.Game.Database } } + if (beatmapTasks.Count == 0) + return; + // Query the beatmaps. var request = new GetBeatmapsRequest(beatmapTasks.Keys.ToArray()); diff --git a/osu.Game/Database/UserLookupCache.cs b/osu.Game/Database/UserLookupCache.cs index dae2d2549c..26f4e9fb3b 100644 --- a/osu.Game/Database/UserLookupCache.cs +++ b/osu.Game/Database/UserLookupCache.cs @@ -100,6 +100,9 @@ namespace osu.Game.Database } } + if (userTasks.Count == 0) + return; + // Query the users. var request = new GetUsersRequest(userTasks.Keys.ToArray()); From 685bdd522e1111749243e95b5dca854bd7064b31 Mon Sep 17 00:00:00 2001 From: Dan Balasescu Date: Wed, 1 Dec 2021 20:17:26 +0900 Subject: [PATCH 21/21] Replace 'skin' in comments with 'instance' --- osu.Game/Database/RealmObjectExtensions.cs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/osu.Game/Database/RealmObjectExtensions.cs b/osu.Game/Database/RealmObjectExtensions.cs index cb38c910da..b38e21453c 100644 --- a/osu.Game/Database/RealmObjectExtensions.cs +++ b/osu.Game/Database/RealmObjectExtensions.cs @@ -135,7 +135,7 @@ namespace osu.Game.Database where T : RealmObjectBase { // Subscribing to non-managed instances doesn't work. - // In this usage, the skin may be non-managed in tests. + // In this usage, the instance may be non-managed in tests. if (!(list is IRealmCollection realmCollection)) return null; @@ -162,7 +162,7 @@ namespace osu.Game.Database where T : RealmObjectBase { // Subscribing to non-managed instances doesn't work. - // In this usage, the skin may be non-managed in tests. + // In this usage, the instance may be non-managed in tests. if (!(list is IRealmCollection realmCollection)) return null;