Fix crashes when attempting to change from a custom ruleset with mods selected to another #30195
Conversation
Have brought this up (likely IRL) that it needs to not exist, and be handled more local to the places that debounce needs to apply. Although I don't know how feasible that will be to make happen. |
|
I did a quick test on this branch, and the crash still occurs for the same reason. (mods not being valid for the updated ruleset) Swapping these two lines around seems to avoid the crash by ensuring that mods are in a sane state beforehand. The ruleset will still be wrong when updating the statistics after mod changes, but it should correct itself when the method is run again when the ruleset changes. |
Closes ppy#30163. If I'm to be blunt, the decoupled stuff in song select makes my head spin. I spent a solid 20 minutes thinking how I was going to fix this one but then finally realised that generally most of the cause there was the fact that `AdvancedStats` was seeing the new rulesets *before* the "ensure global selected mods are valid for current ruleset" logic, and so decided to just _delay_ that until the decoupled transfer thingamajig happens. I was honestly considering combining `BeatmapInfo`, `Ruleset`, and `Mods` into one property on `AdvancedStats`. I figured I'd rather not push my luck and try the baseline version first, but I honestly think that direction is going to be required at some point to properly corral all of the decoupled madness taking place in song select.
This is now removable after `AdvancedStats` has been weaned off the global mods bindable. I think this is a win all things considered?
bdach
force-pushed
the
advanced-stats-stupid-shenanigans
branch
from
96f621c to
687bdad
Compare
I... have no idea how that happened. I swear on anything holy that I did test, with that line somewhere in there, then went to commit, noticed it missing, and added it back where I thought it should have gone. And probably tested after? But clearly not. Anyway, the fact that swapping these lines "fixes" this truly speaks to the quality of this fix. I'll let the review process judge this. I have nothing better to offer, other than maybe this:
|
RFC. Closes #30163.
If I'm to be blunt, the decoupled stuff in song select makes my head spin. I spent a solid 20 minutes thinking how I was going to fix this one but then finally realised that generally most of the cause there was the fact that
AdvancedStatswas seeing the new rulesets before the "ensure global selected mods are valid for current ruleset" logic, and so decided to just delay that until the decoupled transfer thingamajig happens.I was honestly considering combining
BeatmapInfo,Ruleset, andModsinto one property onAdvancedStats. I figured I'd rather not push my luck and try the baseline version first, but I honestly think that direction is going to be required at some point to properly corral all of the decoupled madness taking place in song select.The nice thing is that due to this, one previous hack is now removable after
AdvancedStatshas been weaned off the global mods bindable. I think this is a win all things considered? But I'm unsure of how ugly this in general is compared to the current state...