From da76358ee0530e4c5303cefc3c782ab051ec094f Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Thu, 17 Mar 2022 19:23:43 +0900 Subject: [PATCH 1/6] Expose the loading player in `PlayerLoader` --- osu.Game/Screens/Play/PlayerLoader.cs | 26 +++++++++++++++----------- 1 file changed, 15 insertions(+), 11 deletions(-) diff --git a/osu.Game/Screens/Play/PlayerLoader.cs b/osu.Game/Screens/Play/PlayerLoader.cs index 41eb822e39..384561d616 100644 --- a/osu.Game/Screens/Play/PlayerLoader.cs +++ b/osu.Game/Screens/Play/PlayerLoader.cs @@ -90,7 +90,7 @@ namespace osu.Game.Screens.Play private bool readyForPush => !playerConsumed // don't push unless the player is completely loaded - && player?.LoadState == LoadState.Ready + && CurrentPlayer?.LoadState == LoadState.Ready // don't push if the user is hovering one of the panes, unless they are idle. && (IsHovered || idleTracker.IsIdle.Value) // don't push if the user is dragging a slider or otherwise. @@ -100,10 +100,14 @@ namespace osu.Game.Screens.Play private readonly Func createPlayer; - private Player player; + /// + /// The instance being loaded by this screen. + /// + [CanBeNull] + public Player CurrentPlayer { get; private set; } /// - /// Whether the curent player instance has been consumed via . + /// Whether the current player instance has been consumed via . /// private bool playerConsumed; @@ -237,12 +241,12 @@ namespace osu.Game.Screens.Play { base.OnResuming(last); - var lastScore = player.Score; + var lastScore = CurrentPlayer.Score; AudioSettings.ReferenceScore.Value = lastScore?.ScoreInfo; // prepare for a retry. - player = null; + CurrentPlayer = null; playerConsumed = false; cancelLoad(); @@ -346,7 +350,7 @@ namespace osu.Game.Screens.Play Debug.Assert(!playerConsumed); playerConsumed = true; - return player; + return CurrentPlayer; } private void prepareNewPlayer() @@ -354,11 +358,11 @@ namespace osu.Game.Screens.Play if (!this.IsCurrentScreen()) return; - player = createPlayer(); - player.RestartCount = restartCount++; - player.RestartRequested = restartRequested; + CurrentPlayer = createPlayer(); + CurrentPlayer.RestartCount = restartCount++; + CurrentPlayer.RestartRequested = restartRequested; - LoadTask = LoadComponentAsync(player, _ => MetadataInfo.Loading = false); + LoadTask = LoadComponentAsync(CurrentPlayer, _ => MetadataInfo.Loading = false); } private void restartRequested() @@ -472,7 +476,7 @@ namespace osu.Game.Screens.Play if (isDisposing) { // if the player never got pushed, we should explicitly dispose it. - DisposalTask = LoadTask?.ContinueWith(_ => player?.Dispose()); + DisposalTask = LoadTask?.ContinueWith(_ => CurrentPlayer?.Dispose()); } } From dae5569e367acdacc1f6ee2c6f0f7c80da15762d Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Thu, 17 Mar 2022 19:36:33 +0900 Subject: [PATCH 2/6] Use `nullable` in `PlayerLoader` --- osu.Game/Screens/Play/PlayerLoader.cs | 54 ++++++++++++++------------- 1 file changed, 28 insertions(+), 26 deletions(-) diff --git a/osu.Game/Screens/Play/PlayerLoader.cs b/osu.Game/Screens/Play/PlayerLoader.cs index 384561d616..ba720af2a1 100644 --- a/osu.Game/Screens/Play/PlayerLoader.cs +++ b/osu.Game/Screens/Play/PlayerLoader.cs @@ -1,10 +1,11 @@ // Copyright (c) ppy Pty Ltd . Licensed under the MIT Licence. // See the LICENCE file in the repository root for full licence text. +#nullable enable + using System; using System.Diagnostics; using System.Threading.Tasks; -using JetBrains.Annotations; using ManagedBass.Fx; using osu.Framework.Allocation; using osu.Framework.Audio; @@ -48,31 +49,31 @@ namespace osu.Game.Screens.Play public override bool HandlePositionalInput => true; // We show the previous screen status - protected override UserActivity InitialActivity => null; + protected override UserActivity? InitialActivity => null; protected override bool PlayResumeSound => false; - protected BeatmapMetadataDisplay MetadataInfo { get; private set; } + protected BeatmapMetadataDisplay MetadataInfo { get; private set; } = null!; /// /// A fill flow containing the player settings groups, exposed for the ability to hide it from inheritors of the player loader. /// - protected FillFlowContainer PlayerSettings { get; private set; } + protected FillFlowContainer PlayerSettings { get; private set; } = null!; - protected VisualSettings VisualSettings { get; private set; } + protected VisualSettings VisualSettings { get; private set; } = null!; - protected AudioSettings AudioSettings { get; private set; } + protected AudioSettings AudioSettings { get; private set; } = null!; - protected Task LoadTask { get; private set; } + protected Task? LoadTask { get; private set; } - protected Task DisposalTask { get; private set; } + protected Task? DisposalTask { get; private set; } private bool backgroundBrightnessReduction; private readonly BindableDouble volumeAdjustment = new BindableDouble(1); - private AudioFilter lowPassFilter; - private AudioFilter highPassFilter; + private AudioFilter lowPassFilter = null!; + private AudioFilter highPassFilter = null!; protected bool BackgroundBrightnessReduction { @@ -94,47 +95,45 @@ namespace osu.Game.Screens.Play // don't push if the user is hovering one of the panes, unless they are idle. && (IsHovered || idleTracker.IsIdle.Value) // don't push if the user is dragging a slider or otherwise. - && inputManager?.DraggedDrawable == null + && inputManager.DraggedDrawable == null // don't push if a focused overlay is visible, like settings. - && inputManager?.FocusedDrawable == null; + && inputManager.FocusedDrawable == null; private readonly Func createPlayer; /// /// The instance being loaded by this screen. /// - [CanBeNull] - public Player CurrentPlayer { get; private set; } + public Player? CurrentPlayer { get; private set; } /// /// Whether the current player instance has been consumed via . /// private bool playerConsumed; - private LogoTrackingContainer content; + private LogoTrackingContainer content = null!; private bool hideOverlays; - private InputManager inputManager; + private InputManager inputManager = null!; - private IdleTracker idleTracker; + private IdleTracker idleTracker = null!; - private ScheduledDelegate scheduledPushPlayer; + private ScheduledDelegate? scheduledPushPlayer; - [CanBeNull] - private EpilepsyWarning epilepsyWarning; + private EpilepsyWarning? epilepsyWarning; [Resolved(CanBeNull = true)] - private NotificationOverlay notificationOverlay { get; set; } + private NotificationOverlay? notificationOverlay { get; set; } [Resolved(CanBeNull = true)] - private VolumeOverlay volumeOverlay { get; set; } + private VolumeOverlay? volumeOverlay { get; set; } [Resolved] - private AudioManager audioManager { get; set; } + private AudioManager audioManager { get; set; } = null!; [Resolved(CanBeNull = true)] - private BatteryInfo batteryInfo { get; set; } + private BatteryInfo? batteryInfo { get; set; } public PlayerLoader(Func createPlayer) { @@ -241,6 +240,8 @@ namespace osu.Game.Screens.Play { base.OnResuming(last); + Debug.Assert(CurrentPlayer != null); + var lastScore = CurrentPlayer.Score; AudioSettings.ReferenceScore.Value = lastScore?.ScoreInfo; @@ -348,6 +349,7 @@ namespace osu.Game.Screens.Play private Player consumePlayer() { Debug.Assert(!playerConsumed); + Debug.Assert(CurrentPlayer != null); playerConsumed = true; return CurrentPlayer; @@ -484,7 +486,7 @@ namespace osu.Game.Screens.Play #region Mute warning - private Bindable muteWarningShownOnce; + private Bindable muteWarningShownOnce = null!; private int restartCount; @@ -539,7 +541,7 @@ namespace osu.Game.Screens.Play #region Low battery warning - private Bindable batteryWarningShownOnce; + private Bindable batteryWarningShownOnce = null!; private void showBatteryWarningIfNeeded() { From 63998ad9f121221d4ba27478c2db10aa0d61dcb8 Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Thu, 17 Mar 2022 19:23:49 +0900 Subject: [PATCH 3/6] Add test coverage of `SpectatorPlayer` failing to seek on inopportune frame arrival time --- .../Visual/Gameplay/TestSceneSpectator.cs | 54 ++++++++++++++++++- .../Visual/Spectator/TestSpectatorClient.cs | 5 +- 2 files changed, 55 insertions(+), 4 deletions(-) diff --git a/osu.Game.Tests/Visual/Gameplay/TestSceneSpectator.cs b/osu.Game.Tests/Visual/Gameplay/TestSceneSpectator.cs index d614815316..8b420cebc8 100644 --- a/osu.Game.Tests/Visual/Gameplay/TestSceneSpectator.cs +++ b/osu.Game.Tests/Visual/Gameplay/TestSceneSpectator.cs @@ -70,6 +70,56 @@ namespace osu.Game.Tests.Visual.Gameplay }); } + [Test] + public void TestSeekToGameplayStartFramesArriveAfterPlayerLoad() + { + const double gameplay_start = 10000; + + loadSpectatingScreen(); + + start(); + + waitForPlayer(); + + sendFrames(startTime: gameplay_start); + + AddAssert("time is greater than seek target", () => currentFrameStableTime > gameplay_start); + } + + /// + /// Tests the same as but with the frames arriving just as is transitioning into existence. + /// + [Test] + public void TestSeekToGameplayStartFramesArriveAsPlayerLoaded() + { + const double gameplay_start = 10000; + + loadSpectatingScreen(); + + start(); + + AddUntilStep("wait for player loader", () => (Stack.CurrentScreen as PlayerLoader)?.IsLoaded == true); + + AddUntilStep("queue send frames on player load", () => + { + var loadingPlayer = (Stack.CurrentScreen as PlayerLoader)?.CurrentPlayer; + + if (loadingPlayer == null) + return false; + + loadingPlayer.OnLoadComplete += _ => + { + spectatorClient.SendFramesFromUser(streamingUser.Id, 10, gameplay_start); + }; + return true; + }); + + waitForPlayer(); + + AddUntilStep("state is playing", () => spectatorClient.WatchedUserStates[streamingUser.Id].State == SpectatedUserState.Playing); + AddAssert("time is greater than seek target", () => currentFrameStableTime > gameplay_start); + } + [Test] public void TestFrameStarvationAndResume() { @@ -319,9 +369,9 @@ namespace osu.Game.Tests.Visual.Gameplay private void checkPaused(bool state) => AddUntilStep($"game is {(state ? "paused" : "playing")}", () => player.ChildrenOfType().First().IsPaused.Value == state); - private void sendFrames(int count = 10) + private void sendFrames(int count = 10, double startTime = 0) { - AddStep("send frames", () => spectatorClient.SendFramesFromUser(streamingUser.Id, count)); + AddStep("send frames", () => spectatorClient.SendFramesFromUser(streamingUser.Id, count, startTime)); } private void loadSpectatingScreen() diff --git a/osu.Game/Tests/Visual/Spectator/TestSpectatorClient.cs b/osu.Game/Tests/Visual/Spectator/TestSpectatorClient.cs index f5da95bd7b..ac7cb43e02 100644 --- a/osu.Game/Tests/Visual/Spectator/TestSpectatorClient.cs +++ b/osu.Game/Tests/Visual/Spectator/TestSpectatorClient.cs @@ -88,7 +88,8 @@ namespace osu.Game.Tests.Visual.Spectator /// /// The user to send frames for. /// The total number of frames to send. - public void SendFramesFromUser(int userId, int count) + /// The time to start gameplay frames from. + public void SendFramesFromUser(int userId, int count, double startTime = 0) { var frames = new List(); @@ -102,7 +103,7 @@ namespace osu.Game.Tests.Visual.Spectator flush(); var buttonState = currentFrameIndex == lastFrameIndex ? ReplayButtonState.None : ReplayButtonState.Left1; - frames.Add(new LegacyReplayFrame(currentFrameIndex * 100, RNG.Next(0, 512), RNG.Next(0, 512), buttonState)); + frames.Add(new LegacyReplayFrame(currentFrameIndex * 100 + startTime, RNG.Next(0, 512), RNG.Next(0, 512), buttonState)); } flush(); From 2b59eff465ef38c6e2e8c6edf1f8bb368f7eb8a9 Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Sat, 19 Mar 2022 15:06:09 +0900 Subject: [PATCH 4/6] Fix spectator not starting from correct seek point (hotfix) --- .../Play/MasterGameplayClockContainer.cs | 28 ++++++++++++------- 1 file changed, 18 insertions(+), 10 deletions(-) diff --git a/osu.Game/Screens/Play/MasterGameplayClockContainer.cs b/osu.Game/Screens/Play/MasterGameplayClockContainer.cs index af58e9d910..410f257a44 100644 --- a/osu.Game/Screens/Play/MasterGameplayClockContainer.cs +++ b/osu.Game/Screens/Play/MasterGameplayClockContainer.cs @@ -78,17 +78,12 @@ namespace osu.Game.Screens.Play firstHitObjectTime = beatmap.Beatmap.HitObjects.First().StartTime; } - protected override void LoadComplete() + [BackgroundDependencyLoader] + private void load() { - base.LoadComplete(); - - userAudioOffset = config.GetBindable(OsuSetting.AudioOffset); - userAudioOffset.BindValueChanged(offset => userGlobalOffsetClock.Offset = offset.NewValue, true); - - beatmapOffsetSubscription = realm.SubscribeToPropertyChanged( - r => r.Find(beatmap.BeatmapInfo.ID)?.UserSettings, - settings => settings.Offset, - val => userBeatmapOffsetClock.Offset = val); + // TODO: This code should not be in the BDL load method, but is to avoid seeks being overwritten. + // See https://github.com/ppy/osu/issues/17267 for the issue. + // See https://github.com/ppy/osu/pull/17302 for a better fix which needs some more time. // sane default provided by ruleset. startOffset = gameplayStartTime; @@ -112,6 +107,19 @@ namespace osu.Game.Screens.Play Seek(startOffset); } + protected override void LoadComplete() + { + base.LoadComplete(); + + userAudioOffset = config.GetBindable(OsuSetting.AudioOffset); + userAudioOffset.BindValueChanged(offset => userGlobalOffsetClock.Offset = offset.NewValue, true); + + beatmapOffsetSubscription = realm.SubscribeToPropertyChanged( + r => r.Find(beatmap.BeatmapInfo.ID)?.UserSettings, + settings => settings.Offset, + val => userBeatmapOffsetClock.Offset = val); + } + protected override void OnIsPausedChanged(ValueChangedEvent isPaused) { // The source is stopped by a frequency fade first. From cf2212e79464aa30c0ea721d00bf71f955fbc1a6 Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Sat, 19 Mar 2022 17:20:54 +0900 Subject: [PATCH 5/6] Revert "Fix spectator not starting from correct seek point (hotfix)" This reverts commit 2b59eff465ef38c6e2e8c6edf1f8bb368f7eb8a9. --- .../Play/MasterGameplayClockContainer.cs | 28 +++++++------------ 1 file changed, 10 insertions(+), 18 deletions(-) diff --git a/osu.Game/Screens/Play/MasterGameplayClockContainer.cs b/osu.Game/Screens/Play/MasterGameplayClockContainer.cs index 410f257a44..af58e9d910 100644 --- a/osu.Game/Screens/Play/MasterGameplayClockContainer.cs +++ b/osu.Game/Screens/Play/MasterGameplayClockContainer.cs @@ -78,12 +78,17 @@ namespace osu.Game.Screens.Play firstHitObjectTime = beatmap.Beatmap.HitObjects.First().StartTime; } - [BackgroundDependencyLoader] - private void load() + protected override void LoadComplete() { - // TODO: This code should not be in the BDL load method, but is to avoid seeks being overwritten. - // See https://github.com/ppy/osu/issues/17267 for the issue. - // See https://github.com/ppy/osu/pull/17302 for a better fix which needs some more time. + base.LoadComplete(); + + userAudioOffset = config.GetBindable(OsuSetting.AudioOffset); + userAudioOffset.BindValueChanged(offset => userGlobalOffsetClock.Offset = offset.NewValue, true); + + beatmapOffsetSubscription = realm.SubscribeToPropertyChanged( + r => r.Find(beatmap.BeatmapInfo.ID)?.UserSettings, + settings => settings.Offset, + val => userBeatmapOffsetClock.Offset = val); // sane default provided by ruleset. startOffset = gameplayStartTime; @@ -107,19 +112,6 @@ namespace osu.Game.Screens.Play Seek(startOffset); } - protected override void LoadComplete() - { - base.LoadComplete(); - - userAudioOffset = config.GetBindable(OsuSetting.AudioOffset); - userAudioOffset.BindValueChanged(offset => userGlobalOffsetClock.Offset = offset.NewValue, true); - - beatmapOffsetSubscription = realm.SubscribeToPropertyChanged( - r => r.Find(beatmap.BeatmapInfo.ID)?.UserSettings, - settings => settings.Offset, - val => userBeatmapOffsetClock.Offset = val); - } - protected override void OnIsPausedChanged(ValueChangedEvent isPaused) { // The source is stopped by a frequency fade first. From a903aee17e4a8c44e2d1d56b8e964a7fc9b0c459 Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Sat, 19 Mar 2022 17:23:37 +0900 Subject: [PATCH 6/6] Use schedule to workaround seek issue instead --- osu.Game/Screens/Play/Player.cs | 20 +++++++++++++------- 1 file changed, 13 insertions(+), 7 deletions(-) diff --git a/osu.Game/Screens/Play/Player.cs b/osu.Game/Screens/Play/Player.cs index cb8f4b6020..73bdeb5783 100644 --- a/osu.Game/Screens/Play/Player.cs +++ b/osu.Game/Screens/Play/Player.cs @@ -615,16 +615,22 @@ namespace osu.Game.Screens.Play /// The destination time to seek to. internal void NonFrameStableSeek(double time) { - if (frameStablePlaybackResetDelegate?.Cancelled == false && !frameStablePlaybackResetDelegate.Completed) - frameStablePlaybackResetDelegate.RunTask(); + // TODO: This schedule should not be required and is a temporary hotfix. + // See https://github.com/ppy/osu/issues/17267 for the issue. + // See https://github.com/ppy/osu/pull/17302 for a better fix which needs some more time. + ScheduleAfterChildren(() => + { + if (frameStablePlaybackResetDelegate?.Cancelled == false && !frameStablePlaybackResetDelegate.Completed) + frameStablePlaybackResetDelegate.RunTask(); - bool wasFrameStable = DrawableRuleset.FrameStablePlayback; - DrawableRuleset.FrameStablePlayback = false; + bool wasFrameStable = DrawableRuleset.FrameStablePlayback; + DrawableRuleset.FrameStablePlayback = false; - Seek(time); + Seek(time); - // Delay resetting frame-stable playback for one frame to give the FrameStabilityContainer a chance to seek. - frameStablePlaybackResetDelegate = ScheduleAfterChildren(() => DrawableRuleset.FrameStablePlayback = wasFrameStable); + // Delay resetting frame-stable playback for one frame to give the FrameStabilityContainer a chance to seek. + frameStablePlaybackResetDelegate = ScheduleAfterChildren(() => DrawableRuleset.FrameStablePlayback = wasFrameStable); + }); } ///