ppy / osu

rhythm is just a *click* away!
https://osu.ppy.sh
MIT License
15.32k stars 2.28k forks source link

Editor fails to save difficulty name #14716

Closed bdach closed 3 years ago

bdach commented 3 years ago

The title describes the concrete case that I've hit today but there's quite a bit more under the surface. Anyhow, reproduction steps are:

  1. Open editor
  2. Change difficulty name
  3. Save beatmap
  4. Enter song select
  5. The difficulty name is still blank

https://user-images.githubusercontent.com/20418176/136991987-1c857ab3-eff2-40c5-9eff-0b3eead22b2d.mp4

The immediate cause is that BeatmapManager.Save() has two sources to read BeatmapInfo from: the BeatmapInfo param, or the IBeatmap param. In normal usages it doesn't matter which one is used (although it's still a bit dodgy that there are two), but in the taiko case the wrong one is chosen:

https://github.com/ppy/osu/blob/ebfd5cbe4fd7201e9ede3482d34ad2c7c223e067/osu.Game/Beatmaps/BeatmapManager.cs#L268-L269

And the reason why it is the wrong one is this clone in the taiko converter:

https://github.com/ppy/osu/blob/ebfd5cbe4fd7201e9ede3482d34ad2c7c223e067/osu.Game.Rulesets.Taiko/Beatmaps/TaikoBeatmapConverter.cs#L49-L54

Which makes it so that changes to beatmapInfo from the editor beatmap do not propagate back to playableBeatmap and therefore causes the wrong read.

Not sure where to proceed with this, but in my immediate view this needs to be approached in a two-pronged manner: Save() should use one source of truth, and the converter should probably not be overwriting arbitrary properties on unknown-origin beatmaps willy-nilly. The only reason I'm opening this as an issue instead of doing just that is that I'm not sure if that is what we want.

bdach commented 3 years ago

14980 inadvertently fixed this by removing the BeatmapInfo clone from TaikoBeatmapConverter - but there is another failure case that I will write up in another issue soon, and that I have a proposal of a fix for.

Either way closing this as resolved.

bdach commented 3 years ago

Regressed with #15055 again, but this time on all rulesets.