Merge pull request #11713 from peppy/fix-error-exit-during-results-transition

Fix a potential crash when exiting play during the results screen transition

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

@@ -56,9 +56,7 @@ namespace osu.Game.Tests.Visual.Gameplay
             pauseAndConfirm();
             resume();
 
-            confirmClockRunning(false);
+            confirmPausedWithNoOverlay();
-            confirmPauseOverlayShown(false);
-
             AddStep("click to resume", () => InputManager.Click(MouseButton.Left));
 
             confirmClockRunning(true);
@@ -71,15 +69,14 @@ namespace osu.Game.Tests.Visual.Gameplay
             AddUntilStep("wait for hitobjects", () => Player.HealthProcessor.Health.Value < 1);
 
             pauseAndConfirm();
-
             resume();
-            confirmClockRunning(false);
-            confirmPauseOverlayShown(false);
 
+            confirmPausedWithNoOverlay();
             pauseAndConfirm();
 
             AddUntilStep("resume overlay is not active", () => Player.DrawableRuleset.ResumeOverlay.State.Value == Visibility.Hidden);
             confirmPaused();
+            confirmNotExited();
         }
 
         [Test]
@@ -94,33 +91,45 @@ namespace osu.Game.Tests.Visual.Gameplay
         }
 
         [Test]
-        public void TestPauseTooSoon()
+        public void TestUserPauseDuringCooldownTooSoon()
         {
             AddStep("move cursor outside", () => InputManager.MoveMouseTo(Player.ScreenSpaceDrawQuad.TopLeft - new Vector2(10)));
 
             pauseAndConfirm();
 
             resume();
-            pause();
+            pauseFromUserExitKey();
 
-            confirmClockRunning(true);
+            confirmResumed();
-            confirmPauseOverlayShown(false);
+            confirmNotExited();
         }
 
         [Test]
-        public void TestExitTooSoon()
+        public void TestQuickExitDuringCooldownTooSoon()
+        {
+            AddStep("move cursor outside", () => InputManager.MoveMouseTo(Player.ScreenSpaceDrawQuad.TopLeft - new Vector2(10)));
+
+            pauseAndConfirm();
+
+            resume();
+            AddStep("pause via exit key", () => Player.ExitViaQuickExit());
+
+            confirmResumed();
+            AddAssert("exited", () => !Player.IsCurrentScreen());
+        }
+
+        [Test]
+        public void TestExitSoonAfterResumeSucceeds()
         {
             AddStep("seek before gameplay", () => Player.GameplayClockContainer.Seek(-5000));
 
             pauseAndConfirm();
             resume();
 
-            AddStep("exit too soon", () => Player.Exit());
+            AddStep("exit quick", () => Player.Exit());
 
-            confirmClockRunning(true);
+            confirmResumed();
-            confirmPauseOverlayShown(false);
+            AddAssert("exited", () => !Player.IsCurrentScreen());
-
-            AddAssert("not exited", () => Player.IsCurrentScreen());
         }
 
         [Test]
@@ -131,22 +140,37 @@ namespace osu.Game.Tests.Visual.Gameplay
 
             confirmClockRunning(false);
 
-            pause();
+            AddStep("pause via forced pause", () => Player.Pause());
-
-            confirmClockRunning(false);
-            confirmPauseOverlayShown(false);
 
+            confirmPausedWithNoOverlay();
             AddAssert("fail overlay still shown", () => Player.FailOverlayVisible);
 
             exitAndConfirm();
         }
 
         [Test]
-        public void TestExitFromFailedGameplay()
+        public void TestExitFromFailedGameplayAfterFailAnimation()
         {
             AddUntilStep("wait for fail", () => Player.HasFailed);
-            AddStep("exit", () => Player.Exit());
+            AddUntilStep("wait for fail overlay shown", () => Player.FailOverlayVisible);
 
+            confirmClockRunning(false);
+
+            AddStep("exit via user pause", () => Player.ExitViaPause());
+            confirmExited();
+        }
+
+        [Test]
+        public void TestExitFromFailedGameplayDuringFailAnimation()
+        {
+            AddUntilStep("wait for fail", () => Player.HasFailed);
+
+            // will finish the fail animation and show the fail/pause screen.
+            AddStep("attempt exit via pause key", () => Player.ExitViaPause());
+            AddAssert("fail overlay shown", () => Player.FailOverlayVisible);
+
+            // will actually exit.
+            AddStep("exit via pause key", () => Player.ExitViaPause());
             confirmExited();
         }
 
@@ -245,7 +269,7 @@ namespace osu.Game.Tests.Visual.Gameplay
 
         private void pauseAndConfirm()
         {
-            pause();
+            pauseFromUserExitKey();
             confirmPaused();
         }
 
@@ -257,7 +281,7 @@ namespace osu.Game.Tests.Visual.Gameplay
 
         private void exitAndConfirm()
         {
-            AddUntilStep("player not exited", () => Player.IsCurrentScreen());
+            confirmNotExited();
             AddStep("exit", () => Player.Exit());
             confirmExited();
             confirmNoTrackAdjustments();
@@ -266,7 +290,7 @@ namespace osu.Game.Tests.Visual.Gameplay
         private void confirmPaused()
         {
             confirmClockRunning(false);
-            AddAssert("player not exited", () => Player.IsCurrentScreen());
+            confirmNotExited();
             AddAssert("player not failed", () => !Player.HasFailed);
             AddAssert("pause overlay shown", () => Player.PauseOverlayVisible);
         }
@@ -277,18 +301,22 @@ namespace osu.Game.Tests.Visual.Gameplay
             confirmPauseOverlayShown(false);
         }
 
-        private void confirmExited()
+        private void confirmPausedWithNoOverlay()
         {
-            AddUntilStep("player exited", () => !Player.IsCurrentScreen());
+            confirmClockRunning(false);
+            confirmPauseOverlayShown(false);
         }
 
+        private void confirmExited() => AddUntilStep("player exited", () => !Player.IsCurrentScreen());
+        private void confirmNotExited() => AddAssert("player not exited", () => Player.IsCurrentScreen());
+
         private void confirmNoTrackAdjustments()
         {
             AddAssert("track has no adjustments", () => Beatmap.Value.Track.AggregateFrequency.Value == 1);
         }
 
         private void restart() => AddStep("restart", () => Player.Restart());
-        private void pause() => AddStep("pause", () => Player.Pause());
+        private void pauseFromUserExitKey() => AddStep("user pause", () => Player.ExitViaPause());
         private void resume() => AddStep("resume", () => Player.Resume());
 
         private void confirmPauseOverlayShown(bool isShown) =>
@@ -307,6 +335,10 @@ namespace osu.Game.Tests.Visual.Gameplay
 
             public bool PauseOverlayVisible => PauseOverlay.State.Value == Visibility.Visible;
 
+            public void ExitViaPause() => PerformExit(true);
+
+            public void ExitViaQuickExit() => PerformExit(false);
+
             public override void OnEntering(IScreen last)
             {
                 base.OnEntering(last);

osu.Game.Tests/Visual/Navigation/OsuGameTestScene.cs

@@ -17,6 +17,7 @@ using osu.Game.Graphics.UserInterface;
 using osu.Game.Online.API;
 using osu.Game.Overlays;
 using osu.Game.Rulesets;
+using osu.Game.Rulesets.Mods;
 using osu.Game.Scoring;
 using osu.Game.Screens;
 using osu.Game.Screens.Menu;
@@ -115,6 +116,8 @@ namespace osu.Game.Tests.Visual.Navigation
 
             public new Bindable<RulesetInfo> Ruleset => base.Ruleset;
 
+            public new Bindable<IReadOnlyList<Mod>> SelectedMods => base.SelectedMods;
+
             // if we don't do this, when running under nUnit the version that gets populated is that of nUnit.
             public override string Version => "test game";
 

osu.Game.Tests/Visual/Navigation/TestSceneScreenNavigation.cs

@@ -11,7 +11,9 @@ using osu.Game.Beatmaps;
 using osu.Game.Overlays;
 using osu.Game.Overlays.Mods;
 using osu.Game.Overlays.Toolbar;
+using osu.Game.Rulesets.Osu.Mods;
 using osu.Game.Screens.Play;
+using osu.Game.Screens.Ranking;
 using osu.Game.Screens.Select;
 using osu.Game.Screens.Select.Options;
 using osu.Game.Tests.Beatmaps.IO;
@@ -41,6 +43,30 @@ namespace osu.Game.Tests.Visual.Navigation
             exitViaEscapeAndConfirm();
         }
 
+        [Test]
+        public void TestRetryFromResults()
+        {
+            Player player = null;
+            ResultsScreen results = null;
+
+            WorkingBeatmap beatmap() => Game.Beatmap.Value;
+
+            PushAndConfirm(() => new TestSongSelect());
+
+            AddStep("import beatmap", () => ImportBeatmapTest.LoadOszIntoOsu(Game, virtualTrack: true).Wait());
+
+            AddUntilStep("wait for selected", () => !Game.Beatmap.IsDefault);
+
+            AddStep("set autoplay", () => Game.SelectedMods.Value = new[] { new OsuModAutoplay() });
+
+            AddStep("press enter", () => InputManager.Key(Key.Enter));
+            AddUntilStep("wait for player", () => (player = Game.ScreenStack.CurrentScreen as Player) != null);
+            AddStep("seek to end", () => beatmap().Track.Seek(beatmap().Track.Length));
+            AddUntilStep("wait for pass", () => (results = Game.ScreenStack.CurrentScreen as ResultsScreen) != null && results.IsLoaded);
+            AddStep("attempt to retry", () => results.ChildrenOfType<HotkeyRetryOverlay>().First().Action());
+            AddUntilStep("wait for player", () => Game.ScreenStack.CurrentScreen != player && Game.ScreenStack.CurrentScreen is Player);
+        }
+
         [TestCase(true)]
         [TestCase(false)]
         public void TestSongContinuesAfterExitPlayer(bool withUserPause)

osu.Game/Screens/Play/Player.cs

@@ -339,7 +339,7 @@ namespace osu.Game.Screens.Play
                     {
                         HoldToQuit =
                         {
-                            Action = performUserRequestedExit,
+                            Action = () => PerformExit(true),
                             IsPaused = { BindTarget = GameplayClockContainer.IsPaused }
                         },
                         PlayerSettingsOverlay = { PlaybackSettings = { UserPlaybackRate = { BindTarget = GameplayClockContainer.UserPlaybackRate } } },
@@ -363,14 +363,14 @@ namespace osu.Game.Screens.Play
                     FailOverlay = new FailOverlay
                     {
                         OnRetry = Restart,
-                        OnQuit = performUserRequestedExit,
+                        OnQuit = () => PerformExit(true),
                     },
                     PauseOverlay = new PauseOverlay
                     {
                         OnResume = Resume,
                         Retries = RestartCount,
                         OnRetry = Restart,
-                        OnQuit = performUserRequestedExit,
+                        OnQuit = () => PerformExit(true),
                     },
                     new HotkeyExitOverlay
                     {
@@ -379,7 +379,7 @@ namespace osu.Game.Screens.Play
                             if (!this.IsCurrentScreen()) return;
 
                             fadeOut(true);
-                            PerformExit(true);
+                            PerformExit(false);
                         },
                     },
                     failAnimation = new FailAnimation(DrawableRuleset) { OnComplete = onFailComplete, },
@@ -478,23 +478,43 @@ namespace osu.Game.Screens.Play
         /// <summary>
         /// Exits the <see cref="Player"/>.
         /// </summary>
-        /// <param name="userRequested">
+        /// <param name="showDialogFirst">
-        /// Whether the exit is requested by the user, or a higher-level game component.
+        /// Whether the pause or fail dialog should be shown before performing an exit.
-        /// Pausing is allowed only in the former case.
+        /// If true and a dialog is not yet displayed, the exit will be blocked the the relevant dialog will display instead.
         /// </param>
-        protected void PerformExit(bool userRequested)
+        protected void PerformExit(bool showDialogFirst)
         {
             // if a restart has been requested, cancel any pending completion (user has shown intent to restart).
             completionProgressDelegate?.Cancel();
 
-            ValidForResume = false;
+            // there is a chance that the exit was performed after the transition to results has started.
+            // we want to give the user what they want, so forcefully return to this screen (to proceed with the upwards exit process).
+            if (!this.IsCurrentScreen())
+            {
+                ValidForResume = false;
+                this.MakeCurrent();
+                return;
+            }
 
-            if (!this.IsCurrentScreen()) return;
+            bool pauseOrFailDialogVisible =
+                PauseOverlay.State.Value == Visibility.Visible || FailOverlay.State.Value == Visibility.Visible;
 
-            if (userRequested)
+            if (showDialogFirst && !pauseOrFailDialogVisible)
-                performUserRequestedExit();
+            {
-            else
+                // if the fail animation is currently in progress, accelerate it (it will show the pause dialog on completion).
-                this.Exit();
+                if (ValidForResume && HasFailed)
+                {
+                    failAnimation.FinishTransforms(true);
+                    return;
+                }
+
+                // in the case a dialog needs to be shown, attempt to pause and show it.
+                // this may fail (see internal checks in Pause()) at which point the exit attempt will be aborted.
+                Pause();
+                return;
+            }
+
+            this.Exit();
         }
 
         private void performUserRequestedSkip()
@@ -508,20 +528,6 @@ namespace osu.Game.Screens.Play
             updateSampleDisabledState();
         }
 
-        private void performUserRequestedExit()
-        {
-            if (ValidForResume && HasFailed && !FailOverlay.IsPresent)
-            {
-                failAnimation.FinishTransforms(true);
-                return;
-            }
-
-            if (canPause)
-                Pause();
-            else
-                this.Exit();
-        }
-
         /// <summary>
         /// Restart gameplay via a parent <see cref="PlayerLoader"/>.
         /// <remarks>This can be called from a child screen in order to trigger the restart process.</remarks>
@@ -538,10 +544,7 @@ namespace osu.Game.Screens.Play
             sampleRestart?.Play();
             RestartRequested?.Invoke();
 
-            if (this.IsCurrentScreen())
+            PerformExit(false);
-                PerformExit(true);
-            else
-                this.MakeCurrent();
         }
 
         private ScheduledDelegate completionProgressDelegate;
@@ -809,14 +812,6 @@ namespace osu.Game.Screens.Play
                 return true;
             }
 
-            // ValidForResume is false when restarting
-            if (ValidForResume)
-            {
-                if (pauseCooldownActive && !GameplayClockContainer.IsPaused.Value)
-                    // still want to block if we are within the cooldown period and not already paused.
-                    return true;
-            }
-
             // GameplayClockContainer performs seeks / start / stop operations on the beatmap's track.
             // as we are no longer the current screen, we cannot guarantee the track is still usable.
             GameplayClockContainer?.StopUsingBeatmapClock();