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

Jump to bottom

Fix SkinEditorOverlay freezing when ReplayPlayer screen exits early #26149

Merged
merged 4 commits into from
Feb 22, 2024
Merged

Fix SkinEditorOverlay freezing when ReplayPlayer screen exits early #26149

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
1d4db3b
Fix `SkinEditorOverlay` freezing when `ReplayPlayer` screen exits early
rushiiMachine Dec 26, 2023
85a768d
Don't reuse results delay const
rushiiMachine Dec 26, 2023
0c8b551
SkinEditor lifetime fix & show gameplay
rushiiMachine Dec 27, 2023
f9e92c6
Merge branch 'master' into fix-skin-editor-init-fail
bdach Feb 22, 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
14 changes: 9 additions & 5 deletions osu.Game/Overlays/SkinEditor/SkinEditorOverlay.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -103,13 +103,14 @@ protected override void PopIn()
{
globallyDisableBeatmapSkinSetting();

if (lastTargetScreen is MainMenu)
PresentGameplay();

if (skinEditor != null)
{
disableNestedInputManagers();
skinEditor.Show();

if (lastTargetScreen is MainMenu)
PresentGameplay();

return;
}

Expand All @@ -126,6 +127,9 @@ protected override void PopIn()

AddInternal(editor);

if (lastTargetScreen is MainMenu)
PresentGameplay();

Debug.Assert(lastTargetScreen != null);

SetTarget(lastTargetScreen);
Expand Down Expand Up @@ -270,7 +274,7 @@ private void setTarget(OsuScreen? target)

Debug.Assert(skinEditor != null);

if (!target.IsLoaded)
if (!target.IsLoaded || !skinEditor.IsLoaded)
{
Scheduler.AddOnce(setTarget, target);
return;
Expand Down Expand Up @@ -350,7 +354,7 @@ protected override void LoadComplete()
base.LoadComplete();

if (!LoadedBeatmapSuccessfully)
Scheduler.AddDelayed(this.Exit, 3000);
Copy link
Sponsor Member

Choose a reason for hiding this comment

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

what's going on here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

#26149 (comment)

Scheduler.AddDelayed(this.Exit, 1000);
}

protected override void Update()
Expand Down
2 changes: 1 addition & 1 deletion osu.Game/Screens/Play/Player.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -545,7 +545,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);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please change this back. This is an error condition.

Copy link
Contributor Author

@rushiiMachine rushiiMachine Dec 26, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

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;

Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Sponsor Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@rushiiMachine rushiiMachine Dec 27, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

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?

Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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"

return null;
}
}
Expand Down
Loading