Merge pull request #25565 from peppy/fix-spectator-quit-2

Fix spectator mode not showing when a spectated user quits correctly

osu.Game.Tests/Visual/Gameplay/TestSceneSpectator.cs

@@ -43,7 +43,7 @@ namespace osu.Game.Tests.Visual.Gameplay
 
         private TestSpectatorClient spectatorClient => dependenciesScreen.SpectatorClient;
         private DependenciesScreen dependenciesScreen;
-        private SoloSpectator spectatorScreen;
+        private SoloSpectatorScreen spectatorScreen;
 
         private BeatmapSetInfo importedBeatmap;
         private int importedBeatmapId;
@@ -100,19 +100,18 @@ namespace osu.Game.Tests.Visual.Gameplay
 
             start();
 
-            AddUntilStep("wait for player loader", () => (Stack.CurrentScreen as PlayerLoader)?.IsLoaded == true);
+            AddUntilStep("wait for player loader", () => this.ChildrenOfType<PlayerLoader>().SingleOrDefault()?.IsLoaded == true);
 
             AddUntilStep("queue send frames on player load", () =>
             {
-                var loadingPlayer = (Stack.CurrentScreen as PlayerLoader)?.CurrentPlayer;
+                var loadingPlayer = this.ChildrenOfType<PlayerLoader>().SingleOrDefault()?.CurrentPlayer;
 
                 if (loadingPlayer == null)
                     return false;
 
                 loadingPlayer.OnLoadComplete += _ =>
-                {
                     spectatorClient.SendFramesFromUser(streamingUser.Id, 10, gameplay_start);
-                };
+
                 return true;
             });
 
@@ -127,7 +126,7 @@ namespace osu.Game.Tests.Visual.Gameplay
         {
             loadSpectatingScreen();
 
-            AddAssert("screen hasn't changed", () => Stack.CurrentScreen is SoloSpectator);
+            AddAssert("screen hasn't changed", () => Stack.CurrentScreen is SoloSpectatorScreen);
 
             start();
             waitForPlayer();
@@ -255,7 +254,7 @@ namespace osu.Game.Tests.Visual.Gameplay
             start(-1234);
             sendFrames();
 
-            AddAssert("screen didn't change", () => Stack.CurrentScreen is SoloSpectator);
+            AddAssert("screen didn't change", () => Stack.CurrentScreen is SoloSpectatorScreen);
         }
 
         [Test]
@@ -333,6 +332,8 @@ namespace osu.Game.Tests.Visual.Gameplay
             AddStep("send quit", () => spectatorClient.SendEndPlay(streamingUser.Id));
             AddUntilStep("state is quit", () => spectatorClient.WatchedUserStates[streamingUser.Id].State == SpectatedUserState.Quit);
 
+            AddAssert("wait for player exit", () => Stack.CurrentScreen is SoloSpectatorScreen);
+
             start();
             sendFrames();
             waitForPlayer();
@@ -360,12 +361,12 @@ namespace osu.Game.Tests.Visual.Gameplay
         private OsuFramedReplayInputHandler replayHandler =>
             (OsuFramedReplayInputHandler)Stack.ChildrenOfType<OsuInputManager>().First().ReplayInputHandler;
 
-        private Player player => Stack.CurrentScreen as Player;
+        private Player player => this.ChildrenOfType<Player>().Single();
 
         private double currentFrameStableTime
             => player.ChildrenOfType<FrameStabilityContainer>().First().CurrentTime;
 
-        private void waitForPlayer() => AddUntilStep("wait for player", () => (Stack.CurrentScreen as Player)?.IsLoaded == true);
+        private void waitForPlayer() => AddUntilStep("wait for player", () => this.ChildrenOfType<Player>().SingleOrDefault()?.IsLoaded == true);
 
         private void start(int? beatmapId = null) => AddStep("start play", () => spectatorClient.SendStartPlay(streamingUser.Id, beatmapId ?? importedBeatmapId));
 
@@ -381,7 +382,7 @@ namespace osu.Game.Tests.Visual.Gameplay
 
         private void loadSpectatingScreen()
         {
-            AddStep("load spectator", () => LoadScreen(spectatorScreen = new SoloSpectator(streamingUser)));
+            AddStep("load spectator", () => LoadScreen(spectatorScreen = new SoloSpectatorScreen(streamingUser)));
             AddUntilStep("wait for screen load", () => spectatorScreen.LoadState == LoadState.Loaded);
         }
 

osu.Game/Online/Spectator/SpectatorClient.cs

@@ -48,7 +48,7 @@ namespace osu.Game.Online.Spectator
         /// <summary>
         /// Whether the local user is playing.
         /// </summary>
-        protected internal bool IsPlaying { get; private set; }
+        private bool isPlaying { get; set; }
 
         /// <summary>
         /// Called whenever new frames arrive from the server.
@@ -58,17 +58,17 @@ namespace osu.Game.Online.Spectator
         /// <summary>
         /// Called whenever a user starts a play session, or immediately if the user is being watched and currently in a play session.
         /// </summary>
-        public virtual event Action<int, SpectatorState>? OnUserBeganPlaying;
+        public event Action<int, SpectatorState>? OnUserBeganPlaying;
 
         /// <summary>
         /// Called whenever a user finishes a play session.
         /// </summary>
-        public virtual event Action<int, SpectatorState>? OnUserFinishedPlaying;
+        public event Action<int, SpectatorState>? OnUserFinishedPlaying;
 
         /// <summary>
         /// Called whenever a user-submitted score has been fully processed.
         /// </summary>
-        public virtual event Action<int, long>? OnUserScoreProcessed;
+        public event Action<int, long>? OnUserScoreProcessed;
 
         /// <summary>
         /// A dictionary containing all users currently being watched, with the number of watching components for each user.
@@ -114,7 +114,7 @@ namespace osu.Game.Online.Spectator
                     }
 
                     // re-send state in case it wasn't received
-                    if (IsPlaying)
+                    if (isPlaying)
                         // TODO: this is likely sent out of order after a reconnect scenario. needs further consideration.
                         BeginPlayingInternal(currentScoreToken, currentState);
                 }
@@ -179,10 +179,10 @@ namespace osu.Game.Online.Spectator
             // This schedule is only here to match the one below in `EndPlaying`.
             Schedule(() =>
             {
-                if (IsPlaying)
+                if (isPlaying)
                     throw new InvalidOperationException($"Cannot invoke {nameof(BeginPlaying)} when already playing");
 
-                IsPlaying = true;
+                isPlaying = true;
 
                 // transfer state at point of beginning play
                 currentState.BeatmapID = score.ScoreInfo.BeatmapInfo!.OnlineID;
@@ -202,7 +202,7 @@ namespace osu.Game.Online.Spectator
 
         public void HandleFrame(ReplayFrame frame) => Schedule(() =>
         {
-            if (!IsPlaying)
+            if (!isPlaying)
             {
                 Logger.Log($"Frames arrived at {nameof(SpectatorClient)} outside of gameplay scope and will be ignored.");
                 return;
@@ -224,7 +224,7 @@ namespace osu.Game.Online.Spectator
             // We probably need to find a better way to handle this...
             Schedule(() =>
             {
-                if (!IsPlaying)
+                if (!isPlaying)
                     return;
 
                 // Disposal can take some time, leading to EndPlaying potentially being called after a future play session.
@@ -235,7 +235,7 @@ namespace osu.Game.Online.Spectator
                 if (pendingFrames.Count > 0)
                     purgePendingFrames();
 
-                IsPlaying = false;
+                isPlaying = false;
                 currentBeatmap = null;
 
                 if (state.HasPassed)

osu.Game/Overlays/Dashboard/CurrentlyPlayingDisplay.cs

@@ -212,7 +212,7 @@ namespace osu.Game.Overlays.Dashboard
                                 Text = "Spectate",
                                 Anchor = Anchor.TopCentre,
                                 Origin = Anchor.TopCentre,
-                                Action = () => performer?.PerformFromScreen(s => s.Push(new SoloSpectator(User))),
+                                Action = () => performer?.PerformFromScreen(s => s.Push(new SoloSpectatorScreen(User))),
                                 Enabled = { Value = User.Id != api.LocalUser.Value.Id }
                             }
                         }

osu.Game/Screens/OnlinePlay/Multiplayer/Spectate/MultiSpectatorScreen.cs

@@ -228,7 +228,7 @@ namespace osu.Game.Screens.OnlinePlay.Multiplayer.Spectate
         {
         }
 
-        protected override void StartGameplay(int userId, SpectatorGameplayState spectatorGameplayState)
+        protected override void StartGameplay(int userId, SpectatorGameplayState spectatorGameplayState) => Schedule(() =>
         {
             var playerArea = instances.Single(i => i.UserId == userId);
 
@@ -242,9 +242,9 @@ namespace osu.Game.Screens.OnlinePlay.Multiplayer.Spectate
                 return;
 
             playerArea.LoadScore(spectatorGameplayState.Score);
-        }
+        });
 
-        protected override void QuitGameplay(int userId)
+        protected override void QuitGameplay(int userId) => Schedule(() =>
         {
             RemoveUser(userId);
 
@@ -252,7 +252,7 @@ namespace osu.Game.Screens.OnlinePlay.Multiplayer.Spectate
 
             instance.FadeColour(colours.Gray4, 400, Easing.OutQuint);
             syncManager.RemoveManagedClock(instance.SpectatorPlayerClock);
-        }
+        });
 
         public override bool OnBackButton()
         {

osu.Game/Screens/Play/SoloSpectatorScreen.cs

@@ -1,10 +1,7 @@
 // Copyright (c) ppy Pty Ltd <contact@ppy.sh>. Licensed under the MIT Licence.
 // See the LICENCE file in the repository root for full licence text.
 
-#nullable disable
-
 using System.Diagnostics;
-using JetBrains.Annotations;
 using osu.Framework.Allocation;
 using osu.Framework.Graphics;
 using osu.Framework.Graphics.Containers;
@@ -33,41 +30,40 @@ using osuTK;
 namespace osu.Game.Screens.Play
 {
     [Cached(typeof(IPreviewTrackOwner))]
-    public partial class SoloSpectator : SpectatorScreen, IPreviewTrackOwner
+    public partial class SoloSpectatorScreen : SpectatorScreen, IPreviewTrackOwner
     {
-        [NotNull]
+        [Resolved]
-        private readonly APIUser targetUser;
+        private IAPIProvider api { get; set; } = null!;
 
         [Resolved]
-        private IAPIProvider api { get; set; }
+        private PreviewTrackManager previewTrackManager { get; set; } = null!;
 
         [Resolved]
-        private PreviewTrackManager previewTrackManager { get; set; }
+        private BeatmapManager beatmaps { get; set; } = null!;
 
         [Resolved]
-        private BeatmapManager beatmaps { get; set; }
+        private BeatmapModelDownloader beatmapDownloader { get; set; } = null!;
-
-        [Resolved]
-        private BeatmapModelDownloader beatmapDownloader { get; set; }
 
         [Cached]
         private OverlayColourProvider colourProvider = new OverlayColourProvider(OverlayColourScheme.Purple);
 
-        private Container beatmapPanelContainer;
+        private Container beatmapPanelContainer = null!;
-        private RoundedButton watchButton;
+        private RoundedButton watchButton = null!;
-        private SettingsCheckbox automaticDownload;
+        private SettingsCheckbox automaticDownload = null!;
+
+        private readonly APIUser targetUser;
 
         /// <summary>
         /// The player's immediate online gameplay state.
         /// This doesn't always reflect the gameplay state being watched.
         /// </summary>
-        private SpectatorGameplayState immediateSpectatorGameplayState;
+        private SpectatorGameplayState? immediateSpectatorGameplayState;
 
-        private GetBeatmapSetRequest onlineBeatmapRequest;
+        private GetBeatmapSetRequest? onlineBeatmapRequest;
 
-        private APIBeatmapSet beatmapSet;
+        private APIBeatmapSet? beatmapSet;
 
-        public SoloSpectator([NotNull] APIUser targetUser)
+        public SoloSpectatorScreen(APIUser targetUser)
             : base(targetUser.Id)
         {
             this.targetUser = targetUser;
@@ -168,27 +164,33 @@ namespace osu.Game.Screens.Play
             automaticDownload.Current.BindValueChanged(_ => checkForAutomaticDownload());
         }
 
-        protected override void OnNewPlayingUserState(int userId, SpectatorState spectatorState)
+        protected override void OnNewPlayingUserState(int userId, SpectatorState spectatorState) => Schedule(() =>
         {
             clearDisplay();
             showBeatmapPanel(spectatorState);
-        }
+        });
 
-        protected override void StartGameplay(int userId, SpectatorGameplayState spectatorGameplayState)
+        protected override void StartGameplay(int userId, SpectatorGameplayState spectatorGameplayState) => Schedule(() =>
         {
             immediateSpectatorGameplayState = spectatorGameplayState;
             watchButton.Enabled.Value = true;
 
             scheduleStart(spectatorGameplayState);
-        }
+        });
 
         protected override void QuitGameplay(int userId)
+        {
+            // Importantly, don't schedule this call, as a child screen may be present (and will cause the schedule to not be run as expected).
+            this.MakeCurrent();
+
+            Schedule(() =>
             {
                 scheduledStart?.Cancel();
                 immediateSpectatorGameplayState = null;
                 watchButton.Enabled.Value = false;
 
                 clearDisplay();
+            });
         }
 
         private void clearDisplay()
@@ -199,10 +201,12 @@ namespace osu.Game.Screens.Play
             previewTrackManager.StopAnyPlaying(this);
         }
 
-        private ScheduledDelegate scheduledStart;
+        private ScheduledDelegate? scheduledStart;
 
-        private void scheduleStart(SpectatorGameplayState spectatorGameplayState)
+        private void scheduleStart(SpectatorGameplayState? spectatorGameplayState)
         {
+            Debug.Assert(spectatorGameplayState != null);
+
             // This function may be called multiple times in quick succession once the screen becomes current again.
             scheduledStart?.Cancel();
             scheduledStart = Schedule(() =>

osu.Game/Screens/Spectate/SpectatorScreen.cs

@@ -1,13 +1,10 @@
 // Copyright (c) ppy Pty Ltd <contact@ppy.sh>. Licensed under the MIT Licence.
 // See the LICENCE file in the repository root for full licence text.
 
-#nullable disable
-
 using System;
 using System.Collections.Generic;
 using System.Diagnostics;
 using System.Linq;
-using JetBrains.Annotations;
 using osu.Framework.Allocation;
 using osu.Framework.Bindables;
 using osu.Framework.Extensions;
@@ -33,22 +30,27 @@ namespace osu.Game.Screens.Spectate
         private readonly List<int> users = new List<int>();
 
         [Resolved]
-        private BeatmapManager beatmaps { get; set; }
+        private BeatmapManager beatmaps { get; set; } = null!;
 
         [Resolved]
-        private RulesetStore rulesets { get; set; }
+        private RulesetStore rulesets { get; set; } = null!;
 
         [Resolved]
-        private SpectatorClient spectatorClient { get; set; }
+        private SpectatorClient spectatorClient { get; set; } = null!;
 
         [Resolved]
-        private UserLookupCache userLookupCache { get; set; }
+        private UserLookupCache userLookupCache { get; set; } = null!;
+
+        [Resolved]
+        private RealmAccess realm { get; set; } = null!;
 
         private readonly IBindableDictionary<int, SpectatorState> userStates = new BindableDictionary<int, SpectatorState>();
 
         private readonly Dictionary<int, APIUser> userMap = new Dictionary<int, APIUser>();
         private readonly Dictionary<int, SpectatorGameplayState> gameplayStates = new Dictionary<int, SpectatorGameplayState>();
 
+        private IDisposable? realmSubscription;
+
         /// <summary>
         /// Creates a new <see cref="SpectatorScreen"/>.
         /// </summary>
@@ -58,11 +60,6 @@ namespace osu.Game.Screens.Spectate
             this.users.AddRange(users);
         }
 
-        [Resolved]
-        private RealmAccess realm { get; set; }
-
-        private IDisposable realmSubscription;
-
         protected override void LoadComplete()
         {
             base.LoadComplete();
@@ -90,7 +87,7 @@ namespace osu.Game.Screens.Spectate
             }));
         }
 
-        private void beatmapsChanged(IRealmCollection<BeatmapSetInfo> items, ChangeSet changes)
+        private void beatmapsChanged(IRealmCollection<BeatmapSetInfo> items, ChangeSet? changes)
         {
             if (changes?.InsertedIndices == null) return;
 
@@ -109,7 +106,7 @@ namespace osu.Game.Screens.Spectate
             }
         }
 
-        private void onUserStatesChanged(object sender, NotifyDictionaryChangedEventArgs<int, SpectatorState> e)
+        private void onUserStatesChanged(object? sender, NotifyDictionaryChangedEventArgs<int, SpectatorState> e)
         {
             switch (e.Action)
             {
@@ -132,7 +129,7 @@ namespace osu.Game.Screens.Spectate
             switch (newState.State)
             {
                 case SpectatedUserState.Playing:
-                    Schedule(() => OnNewPlayingUserState(userId, newState));
+                    OnNewPlayingUserState(userId, newState);
                     startGameplay(userId);
                     break;
 
@@ -176,7 +173,7 @@ namespace osu.Game.Screens.Spectate
             var gameplayState = new SpectatorGameplayState(score, resolvedRuleset, beatmaps.GetWorkingBeatmap(resolvedBeatmap));
 
             gameplayStates[userId] = gameplayState;
-            Schedule(() => StartGameplay(userId, gameplayState));
+            StartGameplay(userId, gameplayState);
         }
 
         /// <summary>
@@ -199,25 +196,28 @@ namespace osu.Game.Screens.Spectate
             markReceivedAllFrames(userId);
 
             gameplayStates.Remove(userId);
-            Schedule(() => QuitGameplay(userId));
+            QuitGameplay(userId);
         }
 
         /// <summary>
         /// Invoked when a spectated user's state has changed to a new state indicating the player is currently playing.
+        /// Thread safety is not guaranteed – should be scheduled as required.
         /// </summary>
         /// <param name="userId">The user whose state has changed.</param>
         /// <param name="spectatorState">The new state.</param>
-        protected abstract void OnNewPlayingUserState(int userId, [NotNull] SpectatorState spectatorState);
+        protected abstract void OnNewPlayingUserState(int userId, SpectatorState spectatorState);
 
         /// <summary>
         /// Starts gameplay for a user.
+        /// Thread safety is not guaranteed – should be scheduled as required.
         /// </summary>
         /// <param name="userId">The user to start gameplay for.</param>
         /// <param name="spectatorGameplayState">The gameplay state.</param>
-        protected abstract void StartGameplay(int userId, [NotNull] SpectatorGameplayState spectatorGameplayState);
+        protected abstract void StartGameplay(int userId, SpectatorGameplayState spectatorGameplayState);
 
         /// <summary>
         /// Quits gameplay for a user.
+        /// Thread safety is not guaranteed – should be scheduled as required.
         /// </summary>
         /// <param name="userId">The user to quit gameplay for.</param>
         protected abstract void QuitGameplay(int userId);
@@ -243,7 +243,7 @@ namespace osu.Game.Screens.Spectate
         {
             base.Dispose(isDisposing);
 
-            if (spectatorClient != null)
+            if (spectatorClient.IsNotNull())
             {
                 foreach ((int userId, var _) in userMap)
                     spectatorClient.StopWatchingUser(userId);