ppy / osu

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

Star difficulty is updated too early on beatmap panels when changing active rulesets #11158

Open LittleEndu opened 3 years ago

LittleEndu commented 3 years ago

* doesn't seem to happen for every ruleset combination, but is consistent if that specific combination is broken

Screenshots or videos showing encountered issue: stardiff

osu!lazer version: 2020.1212.0

peppy commented 3 years ago

Could you explain what is wrong in more detail? Or what you'd expect to see? The issue I can see is that a mania beatmap is potentially being calculated using the catch ruleset during the transition, is that what you are trying to point out?

LittleEndu commented 3 years ago

Yeah, that's the issue. I would expect the difficulty icons to not update their star rating after it's calculated or the panels to leave faster so the wrong calculation isn't shown.

peppy commented 3 years ago

So the reason for this happening is that BeatmapDifficultyCache (located in OsuGameBase) is binding to the global ruleset bindable, while the rest of song select is using the local decoupled bindable.

Not sure on the correct way to fix this. The easiest solution I can devise is removing the decoupled bindable, but I believe this is intentionally here to avoid rapid ruleset changes causing great overhead.

bdach commented 3 years ago

I'm not sure that the decoupled ruleset is all that's wrong with the current setup (or even biggest thing wrong with it, to be honest). It's possible for the difficulty cache to attempt to request diffcalc for a ruleset-specific beatmap, but with the wrong ruleset:

2021-02-28-151714_949x333_scrot

I feel like if conversion is not possible for a beatmap (which it isn't in this case, or at least it shouldn't be attempted), then the beatmap's ruleset should always be used? Although then again, that's a bit ill-defined in this context, since in that case the incoming ruleset parameter would essentially be ignored...

The beatmap wedge is also affected by this, incidentally. So yeah, the solution here is to make sure that in the case of beatmaps that shouldn't follow the global ruleset bindables, they don't. Only issue is how to write it without if (rulesetID == 0) checks all over.

smoogipoo commented 3 years ago

Perhaps it's possible to create a new BeatmapDifficultyCache in the context you want? A global game-wide one and a local one for song select. Along this, probably separate out the tracking part into a separate component that uses a BeatmapDifficultyCache internally.

bdach commented 3 years ago

That doesn't do much for the issue, though. To expand on the above, consider a case of a mania beatmap, for instance: a mania difficulty icon shouldn't request a global-bindable-tracking difficulty anyway, since it doesn't make sense to run diffcalc for it in any ruleset other than mania. An osu! beatmap can (and should) request one, on the other side.

I think the other issue that compounds the decoupled ruleset bindable stuff is that working beatmap changes happen just a little bit later than ruleset changes due to filter logic, so it's possible to have:

mania working beatmap, mania ruleset
mania working beatmap, catch ruleset
catch working beatmap, catch ruleset

The confused intermediary state is there for not very long, but it's enough for the issue to manifest.

smoogipoo commented 3 years ago

I think it probably makes sense to use the beatmap's default ruleset in that case.