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 default beatmap not being correctly set after aborting new beatmap creation #32340

Merged
merged 2 commits into from
Mar 12, 2025
Merged

Fix default beatmap not being correctly set after aborting new beatmap creation #32340

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
65cdcb4
Fix default beatmap not being correctly set after aborting new beatma…
peppy Mar 12, 2025
16afd5f
Use reference check rather than `Guid` comparison
peppy Mar 12, 2025
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: 2 additions & 0 deletions osu.Game.Tests/Visual/Editing/TestSceneEditorBeatmapCreation.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -94,6 +94,8 @@ public void TestExitWithoutSave()
AddStep("release", () => InputManager.ReleaseButton(MouseButton.Left));

AddAssert("new beatmap not persisted", () => beatmapManager.QueryBeatmapSet(s => s.ID == editorBeatmap.BeatmapInfo.BeatmapSet.AsNonNull().ID)?.Value.DeletePending == true);

AddUntilStep("wait for default beatmap", () => Editor.Beatmap.Value is DummyWorkingBeatmap);
}

[Test]
Expand Down
2 changes: 1 addition & 1 deletion osu.Game/Beatmaps/WorkingBeatmapCache.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -89,7 +89,7 @@ public void Invalidate(BeatmapInfo info)

public virtual WorkingBeatmap GetWorkingBeatmap([CanBeNull] BeatmapInfo beatmapInfo)
{
if (beatmapInfo?.BeatmapSet == null)
if (beatmapInfo?.ID == DefaultBeatmap.BeatmapInfo.ID || beatmapInfo?.BeatmapSet == null)
Copy link
Collaborator

@bdach bdach Mar 12, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

I'm not sure what it is precisely but I'm deeply uncomfortable with this id check. DefaultBeatmap's ID isn't even fixed to some constant value, it just looks to be getting a random Guid.NewGuid() via the default BeatmapInfo ctor.

Is there any reason we can't go with something like the following instead?

diff --git a/osu.Game/Screens/Edit/Editor.cs b/osu.Game/Screens/Edit/Editor.cs
index f56380a34d..925dd169e9 100644
--- a/osu.Game/Screens/Edit/Editor.cs
+++ b/osu.Game/Screens/Edit/Editor.cs
@@ -915,6 +915,9 @@ public override void OnSuspending(ScreenTransitionEvent e)
 
         private void refetchBeatmap()
         {
+            if (Beatmap.IsDefault)
+                return;
+
             // To update the game-wide beatmap with any changes, perform a re-fetch on exit/suspend.
             // This is required as the editor makes its local changes via EditorBeatmap
             // (which are not propagated outwards to a potentially cached WorkingBeatmap).

Copy link
Member Author

@peppy peppy Mar 12, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

We could do that as well, but I'd want to guard in WorkingBeatmapCache as well, since this is bound to occur via some other means. Right now, doing a GetWorkingBeatmap call will have a chance of converting a default/dummy beatmap into something that resembles it, but isn't considered default.

diff --git a/osu.Game/Beatmaps/WorkingBeatmapCache.cs b/osu.Game/Beatmaps/WorkingBeatmapCache.cs
index bd125deddf..30bbbbc1fe 100644
--- a/osu.Game/Beatmaps/WorkingBeatmapCache.cs
+++ b/osu.Game/Beatmaps/WorkingBeatmapCache.cs
@@ -89,7 +89,7 @@ public void Invalidate(BeatmapInfo info)
 
         public virtual WorkingBeatmap GetWorkingBeatmap([CanBeNull] BeatmapInfo beatmapInfo)
         {
-            if (beatmapInfo?.ID == DefaultBeatmap.BeatmapInfo.ID || beatmapInfo?.BeatmapSet == null)
+            if (beatmapInfo == null || ReferenceEquals(beatmapInfo, DefaultBeatmap.BeatmapInfo))
                 return DefaultBeatmap;
 
             lock (workingCache)

I'm guessing this will sit better with you? It seems to work just as well (waiting on tests to confirm that we don't need to check BeatmapSet for null, but it seems really weird that we would need to be).

Copy link
Collaborator

Choose a reason for hiding this comment

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

If it works then I suppose I could live with a reference equality check sure. Seems less prone to accidental breakage.

return DefaultBeatmap;

lock (workingCache)
Expand Down