Fix `SkinEditorOverlay` freezing when `ReplayPlayer` screen exits early #26149

rushiiMachine · 2023-12-26T20:25:39Z

This addresses issue #25877

Originally when popping in, the ReplayPlayer was loaded first (if previous screen was MainMenu), and afterwards the SkinEditor component was loaded asynchronously. However, if the ReplayPlayer screen exits quickly (like in the event the beatmap has no objects), the skin editor component has not finished initializing (this is before it was even added to the component tree, so it's still not marked Visible), then the screen exiting will cause OsuGame to call SetTarget(newScreen) -> setTarget(...) which sees that the cached skinEditor is not visible yet, and hides/nulls the field. This is the point where LoadComponentAsync(editor, ...) finishes, and the callback sees that the cached skinEditor field is now different (null) than the one that was loaded, and never adds it to the component tree. This occurrence is unhandled and as such the SkinEditorOverlay never hides itself, consuming all input infinitely.

This PR changes the loading to start loading the ReplayPlayer after the SkinEditor has been loaded and added to the component tree. Additionally, this lowers the exit delay for ReplayPlayer and changes the "no hit objects" notification to not be an error since it's a controlled exit.

Originally when popping in, the ReplayPlayer was loaded first (if previous screen was MainMenu), and afterwards the SkinEditor component was loaded asynchronously. However, if the ReplayPlayer screen exits quickly (like in the event the beatmap has no objects), the skin editor component has not finished initializing (this is before it was even added to the component tree, so it's still not marked `Visible`), then the screen exiting will cause `OsuGame` to call SetTarget(newScreen) -> setTarget(...) which sees that the cached `skinEditor` is not visible yet, and hides/nulls the field. This is the point where LoadComponentAsync(editor, ...) finishes, and the callback sees that the cached skinEditor field is now different (null) than the one that was loaded, and never adds it to the component tree. This occurrence is unhandled and as such the SkinEditorOverlay never hides itself, consuming all input infinitely. This PR changes the loading to start loading the ReplayPlayer *after* the SkinEditor has been loaded and added to the component tree. Additionally, this lowers the exit delay for ReplayPlayer and changes the "no hit objects" notification to not be an error since it's a controlled exit.

bdach

Test coverage would be useful (if feasible)

bdach · 2023-12-26T20:29:23Z

osu.Game/Screens/Play/Player.cs

@@ -547,7 +547,7 @@ private IBeatmap loadPlayableBeatmap(Mod[] gameplayMods, CancellationToken cance

                if (playable.HitObjects.Count == 0)
                {
-                    Logger.Log("Beatmap contains no hit objects!", level: LogLevel.Error);
+                    Logger.Log("Beatmap contains no hit objects!", level: LogLevel.Important);


Please change this back. This is an error condition.

This is an error condition

Why so? It's a handled exit that can be caused by users intentionally (not like an uncaught exception).
I can spot another condition in Player that looks like it should actually be shown as an error

osu/osu.Game/Screens/Play/Player.cs

Lines 202 to 205 in 668ce93

if (gameplayMods.Any(m => m is UnknownMod))

{

Logger.Log("Gameplay was started with an unknown mod applied.", level: LogLevel.Important);

return;

I can spot another condition in Player that looks like it should actually be shown as an error

sure, it probably should be. point is that it's an unexpected condition and it should be communicated to the user as such. i believe stable also posts an error-ish notification on this same circumstance.

The game also posts Important level, FWIW. One may argue that important is more correct here because it's not the game erroring but the beatmap just being wrong, dunno.

osu/osu.Game/OsuGame.cs

Lines 1179 to 1190 in cc800a1

if (entry.Level < LogLevel.Important || entry.Target > LoggingTarget.Database || entry.Target == null) return;

const int short_term_display_limit = 3;

if (recentLogCount < short_term_display_limit)

{

Schedule(() => Notifications.Post(new SimpleErrorNotification

{

Icon = entry.Level == LogLevel.Important ? FontAwesome.Solid.ExclamationCircle : FontAwesome.Solid.Bomb,

Text = entry.Message.Truncate(256) + (entry.Exception != null && IsDeployedBuild ? "\n\nThis error has been automatically reported to the devs." : string.Empty),

}));

}

One may argue that important is more correct here because it's not the game erroring but the beatmap just being wrong

Yeah that's what makes more sense to me. When I see the bomb icon I think that something has gone wrong, not just a controlled exit condition.
Seems very odd to report an issue not actually caused by the game itself as an error. Don't Error level logs also get reported to sentry?

Don't Error level logs also get reported to sentry?

If there is no associated exception (which there is not in this case) then it will not be.

i believe stable also posts an error-ish notification on this same circumstance.

Stable handles this in an even worse way, just presenting an error of "beatmap could not be loaded"

bdach · 2023-12-26T20:30:53Z

osu.Game/Overlays/SkinEditor/SkinEditorOverlay.cs

@@ -316,7 +316,7 @@ protected override void LoadComplete()
                base.LoadComplete();

                if (!LoadedBeatmapSuccessfully)
-                    Scheduler.AddDelayed(this.Exit, 3000);
+                    Scheduler.AddDelayed(this.Exit, RESULTS_DISPLAY_DELAY);


Why reuse a results-related const? What does one have to do with the other?

Also why was this shortened to begin with?

The delay was way too awkwardly long from a ux perspective, given it's a completely blank screen with only the background. For comparison, when trying to actually play a map with 0 objects, the delay before returning to song select isn't 3s like here either.

bdach · 2023-12-26T20:34:02Z

and the callback sees that the cached skinEditor field is now different (null) than the one that was loaded, and never adds it to the component tree. This occurrence is unhandled and as such the SkinEditorOverlay never hides itself, consuming all input infinitely.

How does reordering the operations fix this?

It sounds to me that the skin editor should be doing something to hide itself in this case rather than remain in a zombie state consuming all input forevermore. The presence of player or whatever other screen seems purely incidental in this.

rushiiMachine · 2023-12-26T20:40:22Z

How does reordering the operations fix this?

Since the SkinEditor is added into the component tree and therefore marked Visible first, in the event the ReplayPlayer screen gets added and then exits afterwards does not cause setTarget() to nullify the skinEditor, since it's already marked Visisble
This whole loading scheme seems pretty jank and easily fragile though.

It sounds to me that the skin editor should be doing something to hide itself

Then it would exit back into the main menu, I don't think that would be correct behavior either.

Test coverage would be useful (if feasible)

Truthfully I have no idea how I would even test this, ideas would be welcome though

bdach · 2023-12-26T20:53:58Z

What I'm getting from this exchange is that the real problem appears to be that SetTarget() expects the skin editor to be loaded. Which generally does not have to be true in any circumstance where that method is called.

So I'd say the real fix is to delay the call until the skin editor is ready to receive it. Probably by plugging in another condition into this check:

osu/osu.Game/Overlays/SkinEditor/SkinEditorOverlay.cs

Lines 255 to 259 in 668ce93

    
           if (!target.IsLoaded) 
        
           { 
        
               Scheduler.AddOnce(setTarget, target); 
        
               return; 
        
           }

If the SkinEditor was created already but not finished initializing, wait for it to initialize before handling a screen change, which could possibly null the skin editor. Additionally, in the case the skin editor is already loaded but hidden, make sure to show gameplay.

peppy · 2024-01-15T05:15:21Z

osu.Game/Overlays/SkinEditor/SkinEditorOverlay.cs

@@ -316,7 +320,7 @@ protected override void LoadComplete()
                base.LoadComplete();

                if (!LoadedBeatmapSuccessfully)
-                    Scheduler.AddDelayed(this.Exit, 3000);


what's going on here?

#26149 (comment)

peppy · 2024-01-15T05:16:03Z

No test coverage? Seems worthwhile to have, and will help with review.

bdach · 2024-02-22T13:22:01Z

As far as I can tell this fixes the (pretty bad issue) and adding a test is going to be annoying, so I'll just get this in on manual testing.

I'll also keep the irrelevant log severity / transition duration changes because they don't really matter.

pull-request-size bot added the size/S label Dec 26, 2023

bdach requested changes Dec 26, 2023

View reviewed changes

Don't reuse results delay const

85a768d

peppy reviewed Jan 15, 2024

View reviewed changes

peppy added the area:skin-editor label Jan 15, 2024

Merge branch 'master' into fix-skin-editor-init-fail

f9e92c6

bdach approved these changes Feb 22, 2024

View reviewed changes

bdach enabled auto-merge February 22, 2024 13:22

bdach merged commit 9018579 into ppy:master Feb 22, 2024
10 of 11 checks passed

rushiiMachine deleted the fix-skin-editor-init-fail branch February 22, 2024 22:24

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix `SkinEditorOverlay` freezing when `ReplayPlayer` screen exits early #26149

Fix `SkinEditorOverlay` freezing when `ReplayPlayer` screen exits early #26149

rushiiMachine commented Dec 26, 2023 •

edited

Loading

bdach left a comment

bdach Dec 26, 2023

rushiiMachine Dec 26, 2023 •

edited

Loading

bdach Dec 26, 2023

peppy Dec 27, 2023

rushiiMachine Dec 27, 2023 •

edited

Loading

bdach Dec 27, 2023

rushiiMachine Dec 27, 2023

bdach Dec 26, 2023 •

edited

Loading

bdach Dec 26, 2023

rushiiMachine Dec 27, 2023

bdach commented Dec 26, 2023 •

edited

Loading

rushiiMachine commented Dec 26, 2023 •

edited

Loading

bdach commented Dec 26, 2023

peppy Jan 15, 2024

rushiiMachine Jan 15, 2024

peppy commented Jan 15, 2024

bdach commented Feb 22, 2024

	if (gameplayMods.Any(m => m is UnknownMod))
	{
	Logger.Log("Gameplay was started with an unknown mod applied.", level: LogLevel.Important);
	return;

	if (entry.Level < LogLevel.Important \|\| entry.Target > LoggingTarget.Database \|\| entry.Target == null) return;

	const int short_term_display_limit = 3;

	if (recentLogCount < short_term_display_limit)
	{
	Schedule(() => Notifications.Post(new SimpleErrorNotification
	{
	Icon = entry.Level == LogLevel.Important ? FontAwesome.Solid.ExclamationCircle : FontAwesome.Solid.Bomb,
	Text = entry.Message.Truncate(256) + (entry.Exception != null && IsDeployedBuild ? "\n\nThis error has been automatically reported to the devs." : string.Empty),
	}));
	}

Fix SkinEditorOverlay freezing when ReplayPlayer screen exits early #26149

Fix SkinEditorOverlay freezing when ReplayPlayer screen exits early #26149

Conversation

rushiiMachine commented Dec 26, 2023 • edited Loading

bdach left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rushiiMachine Dec 26, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rushiiMachine Dec 27, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bdach Dec 26, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bdach commented Dec 26, 2023 • edited Loading

rushiiMachine commented Dec 26, 2023 • edited Loading

bdach commented Dec 26, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

peppy commented Jan 15, 2024

bdach commented Feb 22, 2024

Fix `SkinEditorOverlay` freezing when `ReplayPlayer` screen exits early #26149

Fix `SkinEditorOverlay` freezing when `ReplayPlayer` screen exits early #26149

rushiiMachine commented Dec 26, 2023 •

edited

Loading

rushiiMachine Dec 26, 2023 •

edited

Loading

rushiiMachine Dec 27, 2023 •

edited

Loading

bdach Dec 26, 2023 •

edited

Loading

bdach commented Dec 26, 2023 •

edited

Loading

rushiiMachine commented Dec 26, 2023 •

edited

Loading