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.

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

Editor does not read properties ignored on BeatmapInfo correctly #20883

Open
salmonslay opened this issue Oct 23, 2022 · 3 comments · May be fixed by #23418 or #28473
Open

Editor does not read properties ignored on BeatmapInfo correctly #20883

salmonslay opened this issue Oct 23, 2022 · 3 comments · May be fixed by #23418 or #28473
Labels
area:beatmap-parsing .osu file format parsing area:editor priority:1 Very important. Feels bad without fix. Affects the majority of users.

Comments

@salmonslay
Copy link
Contributor

salmonslay commented Oct 23, 2022

Type

Game behaviour

Bug description

When importing a beatmap, lazer will always default to using CountdownType.Normal with 0 offset, ignoring any values that the imported beatmap is using. This both applies to beatmaps imported from stable using the in-game wizard, .osz files and the in-game beatmap browser.

It seems like changing the offset in the editor actually gets saved, but turning off or changing the countdown speed is not.

I'll see if I can fix this myself as I need it for #20584. see comment

Screenshots or videos

Stable:
image

Lazer:
image

image

Version

master

Logs

@salmonslay
Copy link
Contributor Author

It seems like these values are correctly decoded;

case @"Countdown":
beatmap.BeatmapInfo.Countdown = (CountdownType)Enum.Parse(typeof(CountdownType), pair.Value);
break;
case @"CountdownOffset":
beatmap.BeatmapInfo.CountdownOffset = Parsing.ParseInt(pair.Value);
break;

I also found a commit stating that CountdownType is intentionally ignored for now (d7fe358), and there was a discussion #14480 (review) about moving these values elsewhere. Would anyone else mind looking into this instead?

@bdach bdach changed the title Beatmaps will ignore their countdown metadata when imported Editor does not read countdown type correctly Oct 23, 2022
@bdach
Copy link
Collaborator

bdach commented Oct 23, 2022

Renamed issue to describe the actual problem better.

The actual problem here - as outlined above - is that the beatmap decoder is supposed to fill the ignored countdown fields, but given the current structure of things this does not always happen. In particular the BeatmapInfo instance passed to the editor comes from the carousel, which does not attempt to fill the countdown values.

Obvious fixes such as the following do not work:

diff --git a/osu.Game/Screens/Select/SongSelect.cs b/osu.Game/Screens/Select/SongSelect.cs
index ece94b5365..cd631a4b44 100644
--- a/osu.Game/Screens/Select/SongSelect.cs
+++ b/osu.Game/Screens/Select/SongSelect.cs
@@ -346,7 +346,7 @@ public void Edit(BeatmapInfo beatmapInfo = null)
             if (!AllowEditing)
                 throw new InvalidOperationException($"Attempted to edit when {nameof(AllowEditing)} is disabled");

-            Beatmap.Value = beatmaps.GetWorkingBeatmap(beatmapInfo ?? beatmapInfoNoDebounce);
+            Beatmap.Value = beatmaps.GetWorkingBeatmap(beatmapInfo ?? beatmapInfoNoDebounce, true);
             this.Push(new EditorLoader());
         }

because WorkingBeatmap only decodes when accessing .Beatmap.

@bdach
Copy link
Collaborator

bdach commented May 3, 2023

@bdach bdach changed the title Editor does not read countdown type correctly Editor does not read properties ignored on BeatmapInfo correctly May 3, 2023
@Ryooshuu Ryooshuu linked a pull request May 6, 2023 that will close this issue
@bdach bdach added the priority:1 Very important. Feels bad without fix. Affects the majority of users. label May 14, 2024
@bdach bdach linked a pull request Jun 13, 2024 that will close this issue
1 task
bdach added a commit to bdach/osu that referenced this issue Aug 20, 2024
In my view this is a nice change, but do note that on its own it does
nothing to fix ppy#29492, because of
`BeatmapInfo` reference management foibles when opening the editor. See
also: ppy#20883 (comment),
ppy#28473.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:beatmap-parsing .osu file format parsing area:editor priority:1 Very important. Feels bad without fix. Affects the majority of users.
Projects
Status: In Review
2 participants