Merge pull request #12254 from smoogipoo/spectator-refactor

Move frame-handling spectator logic into abstract base class

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

@@ -3,6 +3,8 @@
 
 using System.Collections.Generic;
 using System.Linq;
+using System.Threading;
+using System.Threading.Tasks;
 using NUnit.Framework;
 using osu.Framework.Allocation;
 using osu.Framework.Bindables;
@@ -11,6 +13,7 @@ using osu.Framework.Screens;
 using osu.Framework.Testing;
 using osu.Framework.Utils;
 using osu.Game.Beatmaps;
+using osu.Game.Database;
 using osu.Game.Online;
 using osu.Game.Online.Spectator;
 using osu.Game.Replays.Legacy;
@@ -29,10 +32,13 @@ namespace osu.Game.Tests.Visual.Gameplay
         [Cached(typeof(SpectatorStreamingClient))]
         private TestSpectatorStreamingClient testSpectatorStreamingClient = new TestSpectatorStreamingClient();
 
+        [Cached(typeof(UserLookupCache))]
+        private UserLookupCache lookupCache = new TestUserLookupCache();
+
         // used just to show beatmap card for the time being.
         protected override bool UseOnlineAPI => true;
 
-        private Spectator spectatorScreen;
+        private SoloSpectator spectatorScreen;
 
         [Resolved]
         private OsuGameBase game { get; set; }
@@ -69,7 +75,7 @@ namespace osu.Game.Tests.Visual.Gameplay
         {
             loadSpectatingScreen();
 
-            AddAssert("screen hasn't changed", () => Stack.CurrentScreen is Spectator);
+            AddAssert("screen hasn't changed", () => Stack.CurrentScreen is SoloSpectator);
 
             start();
             sendFrames();
@@ -195,7 +201,7 @@ namespace osu.Game.Tests.Visual.Gameplay
             start(-1234);
             sendFrames();
 
-            AddAssert("screen didn't change", () => Stack.CurrentScreen is Spectator);
+            AddAssert("screen didn't change", () => Stack.CurrentScreen is SoloSpectator);
         }
 
         private OsuFramedReplayInputHandler replayHandler =>
@@ -226,7 +232,7 @@ namespace osu.Game.Tests.Visual.Gameplay
 
         private void loadSpectatingScreen()
         {
-            AddStep("load screen", () => LoadScreen(spectatorScreen = new Spectator(testSpectatorStreamingClient.StreamingUser)));
+            AddStep("load screen", () => LoadScreen(spectatorScreen = new SoloSpectator(testSpectatorStreamingClient.StreamingUser)));
             AddUntilStep("wait for screen load", () => spectatorScreen.LoadState == LoadState.Loaded);
         }
 
@@ -301,5 +307,14 @@ namespace osu.Game.Tests.Visual.Gameplay
                 });
             }
         }
+
+        internal class TestUserLookupCache : UserLookupCache
+        {
+            protected override Task<User> ComputeValueAsync(int lookup, CancellationToken token = default) => Task.FromResult(new User
+            {
+                Id = lookup,
+                Username = $"User {lookup}"
+            });
+        }
     }
 }

osu.Game/Overlays/Dashboard/CurrentlyPlayingDisplay.cs

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

osu.Game/Screens/Play/SoloSpectator.cs

@@ -1,18 +1,15 @@
 // 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.
 
-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.Graphics;
 using osu.Framework.Graphics.Containers;
 using osu.Framework.Graphics.Shapes;
 using osu.Framework.Graphics.Sprites;
 using osu.Framework.Screens;
+using osu.Framework.Threading;
 using osu.Game.Audio;
 using osu.Game.Beatmaps;
 using osu.Game.Configuration;
@@ -24,73 +21,49 @@ using osu.Game.Online.API.Requests;
 using osu.Game.Online.Spectator;
 using osu.Game.Overlays.BeatmapListing.Panels;
 using osu.Game.Overlays.Settings;
-using osu.Game.Replays;
 using osu.Game.Rulesets;
-using osu.Game.Rulesets.Mods;
-using osu.Game.Rulesets.Replays;
-using osu.Game.Rulesets.Replays.Types;
-using osu.Game.Scoring;
 using osu.Game.Screens.OnlinePlay.Match.Components;
+using osu.Game.Screens.Spectate;
 using osu.Game.Users;
 using osuTK;
 
 namespace osu.Game.Screens.Play
 {
     [Cached(typeof(IPreviewTrackOwner))]
-    public class Spectator : OsuScreen, IPreviewTrackOwner
+    public class SoloSpectator : SpectatorScreen, IPreviewTrackOwner
     {
+        [NotNull]
         private readonly User targetUser;
 
-        [Resolved]
-        private Bindable<WorkingBeatmap> beatmap { get; set; }
-
-        [Resolved]
-        private Bindable<RulesetInfo> ruleset { get; set; }
-
-        private Ruleset rulesetInstance;
-
-        [Resolved]
-        private Bindable<IReadOnlyList<Mod>> mods { get; set; }
-
         [Resolved]
         private IAPIProvider api { get; set; }
 
         [Resolved]
-        private SpectatorStreamingClient spectatorStreaming { get; set; }
+        private PreviewTrackManager previewTrackManager { get; set; }
-
-        [Resolved]
-        private BeatmapManager beatmaps { get; set; }
 
         [Resolved]
         private RulesetStore rulesets { get; set; }
 
         [Resolved]
-        private PreviewTrackManager previewTrackManager { get; set; }
+        private BeatmapManager beatmaps { get; set; }
-
-        private Score score;
-
-        private readonly object scoreLock = new object();
 
         private Container beatmapPanelContainer;
-
-        private SpectatorState state;
-
-        private IBindable<WeakReference<BeatmapSetInfo>> managerUpdated;
-
         private TriangleButton watchButton;
-
         private SettingsCheckbox automaticDownload;
-
         private BeatmapSetInfo onlineBeatmap;
 
         /// <summary>
-        /// Becomes true if a new state is waiting to be loaded (while this screen was not active).
+        /// The player's immediate online gameplay state.
+        /// This doesn't always reflect the gameplay state being watched.
         /// </summary>
-        private bool newStatePending;
+        private GameplayState immediateGameplayState;
 
-        public Spectator([NotNull] User targetUser)
+        private GetBeatmapSetRequest onlineBeatmapRequest;
+
+        public SoloSpectator([NotNull] User targetUser)
+            : base(targetUser.Id)
         {
-            this.targetUser = targetUser ?? throw new ArgumentNullException(nameof(targetUser));
+            this.targetUser = targetUser;
         }
 
         [BackgroundDependencyLoader]
@@ -173,7 +146,7 @@ namespace osu.Game.Screens.Play
                                 Width = 250,
                                 Anchor = Anchor.Centre,
                                 Origin = Anchor.Centre,
-                                Action = attemptStart,
+                                Action = () => scheduleStart(immediateGameplayState),
                                 Enabled = { Value = false }
                             }
                         }
@@ -185,169 +158,76 @@ namespace osu.Game.Screens.Play
         protected override void LoadComplete()
         {
             base.LoadComplete();
-
-            spectatorStreaming.OnUserBeganPlaying += userBeganPlaying;
-            spectatorStreaming.OnUserFinishedPlaying += userFinishedPlaying;
-            spectatorStreaming.OnNewFrames += userSentFrames;
-
-            spectatorStreaming.WatchUser(targetUser.Id);
-
-            managerUpdated = beatmaps.ItemUpdated.GetBoundCopy();
-            managerUpdated.BindValueChanged(beatmapUpdated);
-
             automaticDownload.Current.BindValueChanged(_ => checkForAutomaticDownload());
         }
 
-        private void beatmapUpdated(ValueChangedEvent<WeakReference<BeatmapSetInfo>> beatmap)
+        protected override void OnUserStateChanged(int userId, SpectatorState spectatorState)
         {
-            if (beatmap.NewValue.TryGetTarget(out var beatmapSet) && beatmapSet.Beatmaps.Any(b => b.OnlineBeatmapID == state.BeatmapID))
+            clearDisplay();
-                Schedule(attemptStart);
+            showBeatmapPanel(spectatorState);
         }
 
-        private void userSentFrames(int userId, FrameDataBundle data)
+        protected override void StartGameplay(int userId, GameplayState gameplayState)
         {
-            // this is not scheduled as it handles propagation of frames even when in a child screen (at which point we are not alive).
+            immediateGameplayState = gameplayState;
-            // probably not the safest way to handle this.
+            watchButton.Enabled.Value = true;
 
-            if (userId != targetUser.Id)
+            scheduleStart(gameplayState);
-                return;
-
-            lock (scoreLock)
-            {
-                // this should never happen as the server sends the user's state on watching,
-                // but is here as a safety measure.
-                if (score == null)
-                    return;
-
-                // rulesetInstance should be guaranteed to be in sync with the score via scoreLock.
-                Debug.Assert(rulesetInstance != null && rulesetInstance.RulesetInfo.Equals(score.ScoreInfo.Ruleset));
-
-                foreach (var frame in data.Frames)
-                {
-                    IConvertibleReplayFrame convertibleFrame = rulesetInstance.CreateConvertibleReplayFrame();
-                    convertibleFrame.FromLegacy(frame, beatmap.Value.Beatmap);
-
-                    var convertedFrame = (ReplayFrame)convertibleFrame;
-                    convertedFrame.Time = frame.Time;
-
-                    score.Replay.Frames.Add(convertedFrame);
-                }
-            }
         }
 
-        private void userBeganPlaying(int userId, SpectatorState state)
+        protected override void EndGameplay(int userId)
         {
-            if (userId != targetUser.Id)
+            scheduledStart?.Cancel();
-                return;
+            immediateGameplayState = null;
+            watchButton.Enabled.Value = false;
 
-            this.state = state;
+            clearDisplay();
-
-            if (this.IsCurrentScreen())
-                Schedule(attemptStart);
-            else
-                newStatePending = true;
-        }
-
-        public override void OnResuming(IScreen last)
-        {
-            base.OnResuming(last);
-
-            if (newStatePending)
-            {
-                attemptStart();
-                newStatePending = false;
-            }
-        }
-
-        private void userFinishedPlaying(int userId, SpectatorState state)
-        {
-            if (userId != targetUser.Id)
-                return;
-
-            lock (scoreLock)
-            {
-                if (score != null)
-                {
-                    score.Replay.HasReceivedAllFrames = true;
-                    score = null;
-                }
-            }
-
-            Schedule(clearDisplay);
         }
 
         private void clearDisplay()
         {
             watchButton.Enabled.Value = false;
+            onlineBeatmapRequest?.Cancel();
             beatmapPanelContainer.Clear();
             previewTrackManager.StopAnyPlaying(this);
         }
 
-        private void attemptStart()
+        private ScheduledDelegate scheduledStart;
+
+        private void scheduleStart(GameplayState gameplayState)
         {
-            clearDisplay();
+            // This function may be called multiple times in quick succession once the screen becomes current again.
-            showBeatmapPanel(state);
+            scheduledStart?.Cancel();
-
+            scheduledStart = Schedule(() =>
-            var resolvedRuleset = rulesets.AvailableRulesets.FirstOrDefault(r => r.ID == state.RulesetID)?.CreateInstance();
-
-            // ruleset not available
-            if (resolvedRuleset == null)
-                return;
-
-            if (state.BeatmapID == null)
-                return;
-
-            var resolvedBeatmap = beatmaps.QueryBeatmap(b => b.OnlineBeatmapID == state.BeatmapID);
-
-            if (resolvedBeatmap == null)
             {
-                return;
+                if (this.IsCurrentScreen())
-            }
+                    start();
+                else
+                    scheduleStart(gameplayState);
+            });
 
-            lock (scoreLock)
+            void start()
             {
-                score = new Score
+                Beatmap.Value = gameplayState.Beatmap;
-                {
+                Ruleset.Value = gameplayState.Ruleset.RulesetInfo;
-                    ScoreInfo = new ScoreInfo
-                    {
-                        Beatmap = resolvedBeatmap,
-                        User = targetUser,
-                        Mods = state.Mods.Select(m => m.ToMod(resolvedRuleset)).ToArray(),
-                        Ruleset = resolvedRuleset.RulesetInfo,
-                    },
-                    Replay = new Replay { HasReceivedAllFrames = false },
-                };
 
-                ruleset.Value = resolvedRuleset.RulesetInfo;
+                this.Push(new SpectatorPlayerLoader(gameplayState.Score));
-                rulesetInstance = resolvedRuleset;
-
-                beatmap.Value = beatmaps.GetWorkingBeatmap(resolvedBeatmap);
-                watchButton.Enabled.Value = true;
-
-                this.Push(new SpectatorPlayerLoader(score));
             }
         }
 
         private void showBeatmapPanel(SpectatorState state)
         {
-            if (state?.BeatmapID == null)
+            Debug.Assert(state.BeatmapID != null);
-            {
-                onlineBeatmap = null;
-                return;
-            }
 
-            var req = new GetBeatmapSetRequest(state.BeatmapID.Value, BeatmapSetLookupType.BeatmapId);
+            onlineBeatmapRequest = new GetBeatmapSetRequest(state.BeatmapID.Value, BeatmapSetLookupType.BeatmapId);
-            req.Success += res => Schedule(() =>
+            onlineBeatmapRequest.Success += res => Schedule(() =>
             {
-                if (state != this.state)
-                    return;
-
                 onlineBeatmap = res.ToBeatmapSet(rulesets);
                 beatmapPanelContainer.Child = new GridBeatmapPanel(onlineBeatmap);
                 checkForAutomaticDownload();
             });
 
-            api.Queue(req);
+            api.Queue(onlineBeatmapRequest);
         }
 
         private void checkForAutomaticDownload()
@@ -369,21 +249,5 @@ namespace osu.Game.Screens.Play
             previewTrackManager.StopAnyPlaying(this);
             return base.OnExiting(next);
         }
-
-        protected override void Dispose(bool isDisposing)
-        {
-            base.Dispose(isDisposing);
-
-            if (spectatorStreaming != null)
-            {
-                spectatorStreaming.OnUserBeganPlaying -= userBeganPlaying;
-                spectatorStreaming.OnUserFinishedPlaying -= userFinishedPlaying;
-                spectatorStreaming.OnNewFrames -= userSentFrames;
-
-                spectatorStreaming.StopWatchingUser(targetUser.Id);
-            }
-
-            managerUpdated?.UnbindAll();
-        }
     }
 }

osu.Game/Screens/Spectate/GameplayState.cs

@@ -0,0 +1,37 @@
+// 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.
+
+using osu.Game.Beatmaps;
+using osu.Game.Rulesets;
+using osu.Game.Scoring;
+
+namespace osu.Game.Screens.Spectate
+{
+    /// <summary>
+    /// The gameplay state of a spectated user. This class is immutable.
+    /// </summary>
+    public class GameplayState
+    {
+        /// <summary>
+        /// The score which the user is playing.
+        /// </summary>
+        public readonly Score Score;
+
+        /// <summary>
+        /// The ruleset which the user is playing.
+        /// </summary>
+        public readonly Ruleset Ruleset;
+
+        /// <summary>
+        /// The beatmap which the user is playing.
+        /// </summary>
+        public readonly WorkingBeatmap Beatmap;
+
+        public GameplayState(Score score, Ruleset ruleset, WorkingBeatmap beatmap)
+        {
+            Score = score;
+            Ruleset = ruleset;
+            Beatmap = beatmap;
+        }
+    }
+}

osu.Game/Screens/Spectate/SpectatorScreen.cs

@@ -0,0 +1,238 @@
+// 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.
+
+using System;
+using System.Collections.Generic;
+using System.Diagnostics;
+using System.Linq;
+using System.Threading.Tasks;
+using JetBrains.Annotations;
+using osu.Framework.Allocation;
+using osu.Framework.Bindables;
+using osu.Game.Beatmaps;
+using osu.Game.Database;
+using osu.Game.Online.Spectator;
+using osu.Game.Replays;
+using osu.Game.Rulesets;
+using osu.Game.Rulesets.Replays;
+using osu.Game.Rulesets.Replays.Types;
+using osu.Game.Scoring;
+using osu.Game.Users;
+
+namespace osu.Game.Screens.Spectate
+{
+    /// <summary>
+    /// A <see cref="OsuScreen"/> which spectates one or more users.
+    /// </summary>
+    public abstract class SpectatorScreen : OsuScreen
+    {
+        private readonly int[] userIds;
+
+        [Resolved]
+        private BeatmapManager beatmaps { get; set; }
+
+        [Resolved]
+        private RulesetStore rulesets { get; set; }
+
+        [Resolved]
+        private SpectatorStreamingClient spectatorClient { get; set; }
+
+        [Resolved]
+        private UserLookupCache userLookupCache { get; set; }
+
+        // A lock is used to synchronise access to spectator/gameplay states, since this class is a screen which may become non-current and stop receiving updates at any point.
+        private readonly object stateLock = new object();
+
+        private readonly Dictionary<int, User> userMap = new Dictionary<int, User>();
+        private readonly Dictionary<int, SpectatorState> spectatorStates = new Dictionary<int, SpectatorState>();
+        private readonly Dictionary<int, GameplayState> gameplayStates = new Dictionary<int, GameplayState>();
+
+        private IBindable<WeakReference<BeatmapSetInfo>> managerUpdated;
+
+        /// <summary>
+        /// Creates a new <see cref="SpectatorScreen"/>.
+        /// </summary>
+        /// <param name="userIds">The users to spectate.</param>
+        protected SpectatorScreen(params int[] userIds)
+        {
+            this.userIds = userIds;
+        }
+
+        protected override void LoadComplete()
+        {
+            base.LoadComplete();
+
+            spectatorClient.OnUserBeganPlaying += userBeganPlaying;
+            spectatorClient.OnUserFinishedPlaying += userFinishedPlaying;
+            spectatorClient.OnNewFrames += userSentFrames;
+
+            foreach (var id in userIds)
+            {
+                userLookupCache.GetUserAsync(id).ContinueWith(u => Schedule(() =>
+                {
+                    if (u.Result == null)
+                        return;
+
+                    lock (stateLock)
+                        userMap[id] = u.Result;
+
+                    spectatorClient.WatchUser(id);
+                }), TaskContinuationOptions.OnlyOnRanToCompletion);
+            }
+
+            managerUpdated = beatmaps.ItemUpdated.GetBoundCopy();
+            managerUpdated.BindValueChanged(beatmapUpdated);
+        }
+
+        private void beatmapUpdated(ValueChangedEvent<WeakReference<BeatmapSetInfo>> e)
+        {
+            if (!e.NewValue.TryGetTarget(out var beatmapSet))
+                return;
+
+            lock (stateLock)
+            {
+                foreach (var (userId, state) in spectatorStates)
+                {
+                    if (beatmapSet.Beatmaps.Any(b => b.OnlineBeatmapID == state.BeatmapID))
+                        updateGameplayState(userId);
+                }
+            }
+        }
+
+        private void userBeganPlaying(int userId, SpectatorState state)
+        {
+            if (state.RulesetID == null || state.BeatmapID == null)
+                return;
+
+            lock (stateLock)
+            {
+                if (!userMap.ContainsKey(userId))
+                    return;
+
+                spectatorStates[userId] = state;
+                Schedule(() => OnUserStateChanged(userId, state));
+
+                updateGameplayState(userId);
+            }
+        }
+
+        private void updateGameplayState(int userId)
+        {
+            lock (stateLock)
+            {
+                Debug.Assert(userMap.ContainsKey(userId));
+
+                var spectatorState = spectatorStates[userId];
+                var user = userMap[userId];
+
+                var resolvedRuleset = rulesets.AvailableRulesets.FirstOrDefault(r => r.ID == spectatorState.RulesetID)?.CreateInstance();
+                if (resolvedRuleset == null)
+                    return;
+
+                var resolvedBeatmap = beatmaps.QueryBeatmap(b => b.OnlineBeatmapID == spectatorState.BeatmapID);
+                if (resolvedBeatmap == null)
+                    return;
+
+                var score = new Score
+                {
+                    ScoreInfo = new ScoreInfo
+                    {
+                        Beatmap = resolvedBeatmap,
+                        User = user,
+                        Mods = spectatorState.Mods.Select(m => m.ToMod(resolvedRuleset)).ToArray(),
+                        Ruleset = resolvedRuleset.RulesetInfo,
+                    },
+                    Replay = new Replay { HasReceivedAllFrames = false },
+                };
+
+                var gameplayState = new GameplayState(score, resolvedRuleset, beatmaps.GetWorkingBeatmap(resolvedBeatmap));
+
+                gameplayStates[userId] = gameplayState;
+                Schedule(() => StartGameplay(userId, gameplayState));
+            }
+        }
+
+        private void userSentFrames(int userId, FrameDataBundle bundle)
+        {
+            lock (stateLock)
+            {
+                if (!userMap.ContainsKey(userId))
+                    return;
+
+                if (!gameplayStates.TryGetValue(userId, out var gameplayState))
+                    return;
+
+                // The ruleset instance should be guaranteed to be in sync with the score via ScoreLock.
+                Debug.Assert(gameplayState.Ruleset != null && gameplayState.Ruleset.RulesetInfo.Equals(gameplayState.Score.ScoreInfo.Ruleset));
+
+                foreach (var frame in bundle.Frames)
+                {
+                    IConvertibleReplayFrame convertibleFrame = gameplayState.Ruleset.CreateConvertibleReplayFrame();
+                    convertibleFrame.FromLegacy(frame, gameplayState.Beatmap.Beatmap);
+
+                    var convertedFrame = (ReplayFrame)convertibleFrame;
+                    convertedFrame.Time = frame.Time;
+
+                    gameplayState.Score.Replay.Frames.Add(convertedFrame);
+                }
+            }
+        }
+
+        private void userFinishedPlaying(int userId, SpectatorState state)
+        {
+            lock (stateLock)
+            {
+                if (!userMap.ContainsKey(userId))
+                    return;
+
+                if (!gameplayStates.TryGetValue(userId, out var gameplayState))
+                    return;
+
+                gameplayState.Score.Replay.HasReceivedAllFrames = true;
+
+                gameplayStates.Remove(userId);
+                Schedule(() => EndGameplay(userId));
+            }
+        }
+
+        /// <summary>
+        /// Invoked when a spectated user's state has changed.
+        /// </summary>
+        /// <param name="userId">The user whose state has changed.</param>
+        /// <param name="spectatorState">The new state.</param>
+        protected abstract void OnUserStateChanged(int userId, [NotNull] SpectatorState spectatorState);
+
+        /// <summary>
+        /// Starts gameplay for a user.
+        /// </summary>
+        /// <param name="userId">The user to start gameplay for.</param>
+        /// <param name="gameplayState">The gameplay state.</param>
+        protected abstract void StartGameplay(int userId, [NotNull] GameplayState gameplayState);
+
+        /// <summary>
+        /// Ends gameplay for a user.
+        /// </summary>
+        /// <param name="userId">The user to end gameplay for.</param>
+        protected abstract void EndGameplay(int userId);
+
+        protected override void Dispose(bool isDisposing)
+        {
+            base.Dispose(isDisposing);
+
+            if (spectatorClient != null)
+            {
+                spectatorClient.OnUserBeganPlaying -= userBeganPlaying;
+                spectatorClient.OnUserFinishedPlaying -= userFinishedPlaying;
+                spectatorClient.OnNewFrames -= userSentFrames;
+
+                lock (stateLock)
+                {
+                    foreach (var (userId, _) in userMap)
+                        spectatorClient.StopWatchingUser(userId);
+                }
+            }
+
+            managerUpdated?.UnbindAll();
+        }
+    }
+}