Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 3 commits into from
Dec 5, 2024
Merged

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

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
9140893
Fix score no longer being saved when quick-restarting after pass
frenzibyte Dec 1, 2024
53dce83
Fix restarting no longer working from results screen
frenzibyte Dec 1, 2024
fa87df6
Move non-current handling to `PerformExit`
frenzibyte Dec 4, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion osu.Game.Tests/Visual/Mods/TestSceneModFailCondition.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ public partial class TestSceneModFailCondition : ModTestScene
protected override TestPlayer CreateModPlayer(Ruleset ruleset)
{
var player = base.CreateModPlayer(ruleset);
player.RestartRequested = _ => restartRequested = true;
player.PrepareLoaderForRestart = _ => restartRequested = true;
return player;
}

Expand Down
14 changes: 10 additions & 4 deletions osu.Game/Screens/Play/Player.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -83,7 +83,7 @@ public abstract partial class Player : ScreenWithBeatmapBackground, ISamplePlayb
/// </summary>
protected virtual bool PauseOnFocusLost => true;

public Action<bool> RestartRequested;
public Action<bool> PrepareLoaderForRestart;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


private bool isRestarting;
private bool skipExitTransition;
Expand Down Expand Up @@ -719,10 +719,16 @@ public bool Restart(bool quickRestart = false)
// stopping here is to ensure music doesn't become audible after exiting back to PlayerLoader.
musicController.Stop();

if (RestartRequested != null)
skipExitTransition = quickRestart;
PrepareLoaderForRestart?.Invoke(quickRestart);

if (!this.IsCurrentScreen())
{
skipExitTransition = quickRestart;
RestartRequested?.Invoke(quickRestart);
// 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();
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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);
         }

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

@frenzibyte frenzibyte Dec 3, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

return true;
}

Expand Down
6 changes: 2 additions & 4 deletions osu.Game/Screens/Play/PlayerLoader.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -457,7 +457,7 @@ private void prepareNewPlayer()
CurrentPlayer = createPlayer();
CurrentPlayer.Configuration.AutomaticallySkipIntro |= quickRestart;
CurrentPlayer.RestartCount = restartCount++;
CurrentPlayer.RestartRequested = restartRequested;
CurrentPlayer.PrepareLoaderForRestart = prepareForRestart;

LoadTask = LoadComponentAsync(CurrentPlayer, _ =>
{
Expand All @@ -470,13 +470,11 @@ protected virtual void OnPlayerLoaded()
{
}

private void restartRequested(bool quickRestartRequested)
private void prepareForRestart(bool quickRestartRequested)
{
quickRestart = quickRestartRequested;
hideOverlays = true;
ValidForResume = true;

this.MakeCurrent();
}

private void contentIn(double delayBeforeSideDisplays = 0)
Expand Down