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

Jump to bottom

Fix issues switching to new beatmap in multiplayer spectator #24438

Merged
merged 4 commits into from
Aug 1, 2023
Merged

Fix issues switching to new beatmap in multiplayer spectator #24438

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
ed0c7e8
Add failing test coverage showing crash on multiple `StartGameplay` i…
peppy Aug 1, 2023
9fe9ea2
Fix potential crash on multiple `StartGameplay` calls in multiplayer …
peppy Aug 1, 2023
fe47dc2
Fix `onLoadRequested` getting early-exited too early in spectator sce…
peppy Aug 1, 2023
5b6c332
Merge branch 'master' into multi-spectator-flow-fixes
bdach Aug 1, 2023
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
14 changes: 14 additions & 0 deletions osu.Game.Tests/Visual/Multiplayer/TestSceneMultiSpectatorScreen.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -79,6 +79,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()
{
Expand Down
6 changes: 3 additions & 3 deletions osu.Game/Screens/OnlinePlay/Multiplayer/MultiplayerMatchSubScreen.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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())
Expand All @@ -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();
}

Expand Down
15 changes: 14 additions & 1 deletion osu.Game/Screens/OnlinePlay/Multiplayer/Spectate/MultiSpectatorScreen.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
{
Expand Down
Loading