search

ppy / osu

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

Attempting to change from a custom ruleset with mods selected to another results in a crash #30163

Closed naoei closed 3 weeks ago

naoei commented 3 weeks ago

Type

Crash to desktop

Bug description

I came across this crash while ensuring that my ruleset (tau) works as intended with the latest osu release. To cause the crash, switch the ruleest to tau, enable the Lite mod within the conversion section, then switch to mania.

Initial thoughts are that TauModLite uses the LT acronym, and thus osu tries to enable the mod due to mania having a similar acronym, however upon looking into mania's mod set, it doesn't appear to have any mods with LT as an acronym.

Offending line is as follows:

https://github.com/taulazer/tau/blob/abe5104bade1bd34392e197c475ba8c072a34137/osu.Game.Rulesets.Tau/Mods/TauModLite.cs#L42

Definitely needs some more investigation, however unsure if this is an issue on my end.

Screenshots or videos

No response

Version

2024.1009.0

Logs

1728466199.runtime.log

LumpBloom7 commented 3 weeks ago

I did some investigating, this problem has been around since 2024.412.x.

The root problem is that when a ruleset is changes, AdvancedStats.updateStatistics is called prior to gamewide SelectedMods being updated to remove/migrate mods from the previous ruleset, and it then uses the mods for the wrong ruleset when calling GetKeyCount().

The diff/PR that introduced the problem: https://github.com/ppy/osu/pull/27747/files#diff-7537e2f2818506dce8cc62f117ec907f9cb69ca88513b5224cc524474447e654L202

However the fact that our IApplicableToBeatmapConverter mods did a direct cast without performing a check on whether IBeatmapConverter was actually the type we expect is an oversight on our part as well.

naoei commented 3 weeks ago

However the fact that our IApplicableToBeatmapConverter mods did a direct cast without performing a check on whether IBeatmapConverter was actually the type we expect is an oversight on our part as well.

I don't think our method is wrong at all. It's weird to assume that our mods would be applied to other rulesets when switching rulesets. I did a bit of sanity check and confirmed that mania does the same type casting with their mods that implement IApplicableToBeatmapConverted.

https://github.com/ppy/osu/blob/a6f56036a2c5df3f0427200ceb6fb8c321068ad6/osu.Game.Rulesets.Mania/Mods/ManiaModDualStages.cs#L23

https://github.com/ppy/osu/blob/a6f56036a2c5df3f0427200ceb6fb8c321068ad6/osu.Game.Rulesets.Mania/Mods/ManiaKeyMod.cs#L22

These don't result in any crash with the other offcial rulesets, so the problem probably lies elsewhere.

LumpBloom7 commented 3 weeks ago

I meant from a reliability standpoint, it is a place where a safety check would have mitigated a hard crash, regardless of what official mods do. I know it isn't the source of the problem.

Other official rulesets don't cause the problem because none of them even have an IApplicableToBeatmapConverter mod. GetKeyCount is only called by AdvancedStats when the the ruleset is Mania, so the issue doesn't occur when switching from Mania to something else.