From 740a6f16c79a606652649fc9e71cb607bfb343d9 Mon Sep 17 00:00:00 2001 From: Dan Balasescu Date: Wed, 15 Dec 2021 16:52:50 +0900 Subject: [PATCH 1/3] Fix exception when updating the room's visual playlist --- .../Multiplayer/TestSceneMultiplayer.cs | 43 +++++++++++++++++-- .../Match/Playlist/MultiplayerPlaylist.cs | 6 ++- 2 files changed, 45 insertions(+), 4 deletions(-) diff --git a/osu.Game.Tests/Visual/Multiplayer/TestSceneMultiplayer.cs b/osu.Game.Tests/Visual/Multiplayer/TestSceneMultiplayer.cs index bc2902480d..2ea7f2541f 100644 --- a/osu.Game.Tests/Visual/Multiplayer/TestSceneMultiplayer.cs +++ b/osu.Game.Tests/Visual/Multiplayer/TestSceneMultiplayer.cs @@ -30,6 +30,7 @@ using osu.Game.Screens.OnlinePlay.Match; using osu.Game.Screens.OnlinePlay.Multiplayer; using osu.Game.Screens.OnlinePlay.Multiplayer.Match; +using osu.Game.Screens.OnlinePlay.Multiplayer.Match.Playlist; using osu.Game.Screens.Play; using osu.Game.Screens.Ranking; using osu.Game.Screens.Spectate; @@ -594,9 +595,7 @@ public void TestGameplayFlow() } }); - pressReadyButton(); - pressReadyButton(); - AddUntilStep("wait for player", () => multiplayerScreenStack.CurrentScreen is Player); + enterGameplay(); // Gameplay runs in real-time, so we need to incrementally check if gameplay has finished in order to not time out. for (double i = 1000; i < TestResources.QUICK_BEATMAP_LENGTH; i += 1000) @@ -656,6 +655,44 @@ public void TestRoomSettingsReQueriedWhenJoiningRoom() }); } + [Test] + public void TestItemAddedAndDeletedByOtherUserDuringGameplay() + { + createRoom(() => new Room + { + Name = { Value = "Test Room" }, + QueueMode = { Value = QueueMode.AllPlayers }, + Playlist = + { + new PlaylistItem + { + Beatmap = { Value = beatmaps.GetWorkingBeatmap(importedSet.Beatmaps.First(b => b.RulesetID == 0)).BeatmapInfo }, + Ruleset = { Value = new OsuRuleset().RulesetInfo }, + } + } + }); + + enterGameplay(); + + AddStep("join other user", () => client.AddUser(new APIUser { Id = 1234 })); + AddStep("add item as other user", () => client.AddUserPlaylistItem(1234, new MultiplayerPlaylistItem(new PlaylistItem + { + BeatmapID = beatmaps.GetWorkingBeatmap(importedSet.Beatmaps.First(b => b.RulesetID == 0)).BeatmapInfo.OnlineID ?? -1 + }))); + AddStep("delete item as other user", () => client.RemoveUserPlaylistItem(1234, 2)); + + AddUntilStep("wait for item to be deleted", () => client.Room?.Playlist.Count == 1); + AddStep("exit gameplay as initial user", () => multiplayerScreenStack.MultiplayerScreen.MakeCurrent()); + AddUntilStep("queue is empty", () => this.ChildrenOfType().Single().Items.Count == 0); + } + + private void enterGameplay() + { + pressReadyButton(); + pressReadyButton(); + AddUntilStep("wait for player", () => multiplayerScreenStack.CurrentScreen is Player); + } + private ReadyButton readyButton => this.ChildrenOfType().Single(); private void pressReadyButton() diff --git a/osu.Game/Screens/OnlinePlay/Multiplayer/Match/Playlist/MultiplayerPlaylist.cs b/osu.Game/Screens/OnlinePlay/Multiplayer/Match/Playlist/MultiplayerPlaylist.cs index 4971489769..7b90532cce 100644 --- a/osu.Game/Screens/OnlinePlay/Multiplayer/Match/Playlist/MultiplayerPlaylist.cs +++ b/osu.Game/Screens/OnlinePlay/Multiplayer/Match/Playlist/MultiplayerPlaylist.cs @@ -121,7 +121,11 @@ protected override void PlaylistItemChanged(MultiplayerPlaylistItem item) private void addItemToLists(MultiplayerPlaylistItem item) { - var apiItem = Playlist.Single(i => i.ID == item.ID); + var apiItem = Playlist.SingleOrDefault(i => i.ID == item.ID); + + // Item could have been removed from the playlist while the local player was in gameplay. + if (apiItem == null) + return; if (item.Expired) historyList.Items.Add(apiItem); From d22e1b90010ea570f03f4aad8450f72bce2d02e9 Mon Sep 17 00:00:00 2001 From: Dan Balasescu Date: Wed, 15 Dec 2021 16:59:34 +0900 Subject: [PATCH 2/3] Add another until step to guard against async test issues --- osu.Game.Tests/Visual/Multiplayer/TestSceneMultiplayer.cs | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/osu.Game.Tests/Visual/Multiplayer/TestSceneMultiplayer.cs b/osu.Game.Tests/Visual/Multiplayer/TestSceneMultiplayer.cs index 2ea7f2541f..1d951c257c 100644 --- a/osu.Game.Tests/Visual/Multiplayer/TestSceneMultiplayer.cs +++ b/osu.Game.Tests/Visual/Multiplayer/TestSceneMultiplayer.cs @@ -679,9 +679,12 @@ public void TestItemAddedAndDeletedByOtherUserDuringGameplay() { BeatmapID = beatmaps.GetWorkingBeatmap(importedSet.Beatmaps.First(b => b.RulesetID == 0)).BeatmapInfo.OnlineID ?? -1 }))); - AddStep("delete item as other user", () => client.RemoveUserPlaylistItem(1234, 2)); - AddUntilStep("wait for item to be deleted", () => client.Room?.Playlist.Count == 1); + AddUntilStep("item arrived in playlist", () => client.Room?.Playlist.Count == 2); + + AddStep("delete item as other user", () => client.RemoveUserPlaylistItem(1234, 2)); + AddUntilStep("item removed from playlist", () => client.Room?.Playlist.Count == 1); + AddStep("exit gameplay as initial user", () => multiplayerScreenStack.MultiplayerScreen.MakeCurrent()); AddUntilStep("queue is empty", () => this.ChildrenOfType().Single().Items.Count == 0); } From 39a0a2113219ef3319da9dd4e6e0c023b5f0ce52 Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Wed, 15 Dec 2021 17:30:09 +0900 Subject: [PATCH 3/3] Add test coverage of same scenario without deletion --- .../Multiplayer/TestSceneMultiplayer.cs | 31 +++++++++++++++++++ 1 file changed, 31 insertions(+) diff --git a/osu.Game.Tests/Visual/Multiplayer/TestSceneMultiplayer.cs b/osu.Game.Tests/Visual/Multiplayer/TestSceneMultiplayer.cs index 1d951c257c..ed8d50589f 100644 --- a/osu.Game.Tests/Visual/Multiplayer/TestSceneMultiplayer.cs +++ b/osu.Game.Tests/Visual/Multiplayer/TestSceneMultiplayer.cs @@ -655,6 +655,37 @@ public void TestRoomSettingsReQueriedWhenJoiningRoom() }); } + [Test] + public void TestItemAddedByOtherUserDuringGameplay() + { + createRoom(() => new Room + { + Name = { Value = "Test Room" }, + QueueMode = { Value = QueueMode.AllPlayers }, + Playlist = + { + new PlaylistItem + { + Beatmap = { Value = beatmaps.GetWorkingBeatmap(importedSet.Beatmaps.First(b => b.RulesetID == 0)).BeatmapInfo }, + Ruleset = { Value = new OsuRuleset().RulesetInfo }, + } + } + }); + + enterGameplay(); + + AddStep("join other user", () => client.AddUser(new APIUser { Id = 1234 })); + AddStep("add item as other user", () => client.AddUserPlaylistItem(1234, new MultiplayerPlaylistItem(new PlaylistItem + { + BeatmapID = beatmaps.GetWorkingBeatmap(importedSet.Beatmaps.First(b => b.RulesetID == 0)).BeatmapInfo.OnlineID ?? -1 + }))); + + AddUntilStep("item arrived in playlist", () => client.Room?.Playlist.Count == 2); + + AddStep("exit gameplay as initial user", () => multiplayerScreenStack.MultiplayerScreen.MakeCurrent()); + AddUntilStep("queue contains item", () => this.ChildrenOfType().Single().Items.Single().ID == 2); + } + [Test] public void TestItemAddedAndDeletedByOtherUserDuringGameplay() {