From ed0c7e888085b798dda471b00a905058ca734e90 Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Tue, 1 Aug 2023 19:21:46 +0900 Subject: [PATCH 1/3] Add failing test coverage showing crash on multiple `StartGameplay` invocations --- .../Multiplayer/TestSceneMultiSpectatorScreen.cs | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/osu.Game.Tests/Visual/Multiplayer/TestSceneMultiSpectatorScreen.cs b/osu.Game.Tests/Visual/Multiplayer/TestSceneMultiSpectatorScreen.cs index e81dc87d4fc2..864c987569fe 100644 --- a/osu.Game.Tests/Visual/Multiplayer/TestSceneMultiSpectatorScreen.cs +++ b/osu.Game.Tests/Visual/Multiplayer/TestSceneMultiSpectatorScreen.cs @@ -78,6 +78,20 @@ public void TestGeneral(int count) AddWaitStep("wait a bit", 20); } + [Test] + public void TestMultipleStartRequests() + { + int[] userIds = getPlayerIds(2); + + start(userIds); + loadSpectateScreen(); + + sendFrames(userIds, 1000); + AddWaitStep("wait a bit", 20); + + start(userIds); + } + [Test] public void TestDelayedStart() { From 9fe9ea2c90545e9ce546ffd0cebb9224b20a6cac Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Tue, 1 Aug 2023 19:18:18 +0900 Subject: [PATCH 2/3] Fix potential crash on multiple `StartGameplay` calls in multiplayer spectator --- .../Multiplayer/Spectate/MultiSpectatorScreen.cs | 15 ++++++++++++++- 1 file changed, 14 insertions(+), 1 deletion(-) diff --git a/osu.Game/Screens/OnlinePlay/Multiplayer/Spectate/MultiSpectatorScreen.cs b/osu.Game/Screens/OnlinePlay/Multiplayer/Spectate/MultiSpectatorScreen.cs index 7528ff406478..afc10d0c0a5c 100644 --- a/osu.Game/Screens/OnlinePlay/Multiplayer/Spectate/MultiSpectatorScreen.cs +++ b/osu.Game/Screens/OnlinePlay/Multiplayer/Spectate/MultiSpectatorScreen.cs @@ -214,7 +214,20 @@ protected override void OnNewPlayingUserState(int userId, SpectatorState spectat } protected override void StartGameplay(int userId, SpectatorGameplayState spectatorGameplayState) - => instances.Single(i => i.UserId == userId).LoadScore(spectatorGameplayState.Score); + { + var playerArea = instances.Single(i => i.UserId == userId); + + // The multiplayer spectator flow requires the client to return to a higher level screen + // (ie. StartGameplay should only be called once per player). + // + // Meanwhile, the solo spectator flow supports multiple `StartGameplay` calls. + // To ensure we don't crash out in an edge case where this is called more than once in multiplayer, + // guard against re-entry for the same player. + if (playerArea.Score != null) + return; + + playerArea.LoadScore(spectatorGameplayState.Score); + } protected override void QuitGameplay(int userId) { From fe47dc291bff3559ff13473dd384c3afbd73a481 Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Tue, 1 Aug 2023 19:08:26 +0900 Subject: [PATCH 3/3] Fix `onLoadRequested` getting early-exited too early in spectator scenarios In some scenarios, multiplayer spectator would not tick over to the next beatmap. Here's an example: - Room has two items queued - Local user starts download of both - First beatmap starts and download is complete - First beatmap ends (spectating is active) - Second beatmap starts but download is not complete In this scenario, the local client will get stuck at the spectator screen due to the `onLoadRequested`-invoked screen change being early exited. It would require manual recovery (clicking back button) to return to a sane state. --- .../OnlinePlay/Multiplayer/MultiplayerMatchSubScreen.cs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/osu.Game/Screens/OnlinePlay/Multiplayer/MultiplayerMatchSubScreen.cs b/osu.Game/Screens/OnlinePlay/Multiplayer/MultiplayerMatchSubScreen.cs index f13b47c0f583..7c12e6eab5f9 100644 --- a/osu.Game/Screens/OnlinePlay/Multiplayer/MultiplayerMatchSubScreen.cs +++ b/osu.Game/Screens/OnlinePlay/Multiplayer/MultiplayerMatchSubScreen.cs @@ -371,9 +371,6 @@ private void handleRoomLost() => Schedule(() => private void onLoadRequested() { - if (BeatmapAvailability.Value.State != DownloadState.LocallyAvailable) - return; - // In the case of spectating, IMultiplayerClient.LoadRequested can be fired while the game is still spectating a previous session. // For now, we want to game to switch to the new game so need to request exiting from the play screen. if (!ParentScreen.IsCurrentScreen()) @@ -391,6 +388,9 @@ private void onLoadRequested() if (client.LocalUser?.State == MultiplayerUserState.Spectating && (SelectedItem.Value == null || Beatmap.IsDefault)) return; + if (BeatmapAvailability.Value.State != DownloadState.LocallyAvailable) + return; + StartPlay(); }