Fix score no longer being saved when quick-restarting after pass

[frenzibyte]

• Closes Replay is not saved when quick retrying at the end of a beatmap #30642

The PerformExit path ensures the score is imported even if the user restarts after passing the beatmap during the one second completion delay RESULTS_DISPLAY_DELAY.

In #30623, Restart no longer calls PerformExit, therefore discarding the score for the scenario in question.

I've resolved the issue by still calling PerformExit rather than letting the PlayerLoader do the exiting process.

I wanted to write a test case for this but timing is critical, I'll have to change the constant to be overridable and make my test case set the delay to infinity, but it feels weird to break parts of the component itself in order to test it. I can still write a test case if wanted.

[peppy]

Tests failing.

[peppy]

The logic seems sound, but tests need addressing.

[bdach]

This hurts my head. I guess at least there's an inline comment next to it but it's still a solution I'd classify as convoluted at best.

Can this not be done any simpler? I'd much rather have a duplicated PerformExit() call in the original code in the RestartRequested block or something than this.

[peppy]

@bdach does this read any better to you?

diff --git a/osu.Game/Screens/Play/Player.cs b/osu.Game/Screens/Play/Player.cs
index 3a0a0613f3..1866ed26ce 100644
--- a/osu.Game/Screens/Play/Player.cs
+++ b/osu.Game/Screens/Play/Player.cs
@@ -646,7 +646,6 @@ protected bool PerformExit(bool skipTransition = false)
             // import current score if possible.
             prepareAndImportScoreAsync();
 
-            // Screen may not be current if a restart has been performed.
             if (this.IsCurrentScreen())
             {
                 skipExitTransition = skipTransition;
@@ -657,6 +656,12 @@ protected bool PerformExit(bool skipTransition = false)
                 // - the pause / fail dialog was requested but couldn't be displayed due to the type or state of this Player instance.
                 this.Exit();
             }
+            else
+            {
+                // May be restarting from results screen.
+                if (this.GetChildScreen() != null)
+                    this.MakeCurrent();
+            }
 
             return true;
         }
@@ -722,16 +727,6 @@ public bool Restart(bool quickRestart = false)
             skipExitTransition = quickRestart;
             PrepareLoaderForRestart?.Invoke(quickRestart);
 
-            if (!this.IsCurrentScreen())
-            {
-                // if we're called externally (i.e. from results screen),
-                // use MakeCurrent to exit results screen as well as this player screen
-                // since ValidForResume = false in here
-                Debug.Assert(!ValidForResume);
-                this.MakeCurrent();
-                return true;
-            }
-
             return PerformExit(quickRestart);
         }
 

[bdach]

The problem I have is the whole "making a non-valid-for-resume screen current in order to implicitly exit it" shebang. Not the positioning of it in code. So I guess that's a no.

[frenzibyte]

An alternative option I can imagine would be to move the Restart method to PlayerLoader instead and choose between calling Player.PerformExit() and PlayerLoader.MakeCurrent() based on whether player is current screen. That will solve the hack of making a non-valid-for-resume screen current.

It also generally makes sense for PlayerLoader to expose restarting as that's what's really doing the "restarting" process.

[bdach]

An alternative option I can imagine would be to move the Restart method to PlayerLoader instead and choose between calling Player.PerformExit() and PlayerLoader.MakeCurrent() based on whether player is current screen.

Yeah I don't know if I'd consider this better.

Maybe this is fine if there really is no saner alternative, I dunno.

[peppy]

I say we just choose one and go with it. Can reassess next time the code is touched.

[frenzibyte]

I will say that I have attempted my proposal above just to see where it goes and how bad/good would it be, but upon stumbling across some existing test code that relies on the current structure I halted down immediately as totally not worth the effort.

I like peppy's diff as it makes the logic look more appropriate than it is currently, pushed in fa87df6.

[bdach]

What's with the rename? I preferred the old name.

[peppy]

I'd say it's because the PlayerLoader is no longer responsible for performing the restart, ie. calling this.MakeCurrent(). the Requested terminology implies that another component is going to make the decision of whether a restart should actually occur.

[frenzibyte]

I have ignored adding back the !ValidForResume assertion as I think it's a given since the screen is not current and the current screen is presumed to be the results screen.