Fix "spinner" conversion for mania-specific beatmaps #30984
Conversation
A big part of these changes is refactoring, which is somewhat necessary because it was previously implemented as two separate pathways which in-fact need to be joined at the hip when handling spinners. I've chosen to use `IHasLegacyHitObjectType` here because there's no other flag that allows us to tell `ConvertHold` apart from `ConvertSpinner`.
Better symbolises the intent of this generator which is to convert hitobjects in their most simple forms - anything with an end time converts to a hold or otherwise converts to a normal note.
In particular, the spinner one is the most relevant to this batch of changes.
smoogipoo
added
ruleset/osu!mania
area:beatmap-parsing
|
!diffcalc |
|
!diffcalc |
In particular, "EndTimeObject" is no longer correct - it's strictly used for spinners and not holds.
Normally not an issue, but some tests create their own hitobjects deriving from `ConvertHitObject`.
Also used in some tests (e.g. beatmaps containing `HitCircle`s).
|
Difficulty calculation failed: https://github.com/ppy/osu/actions/runs/12194988363 |
|
!diffcalc |
|
!diffcalc |
|
Unless I'm doing something wrong I can't find any differences in ranked maps, neither with the spreadsheets above nor offline testing + custom code over the https://data.ppy.sh beatmap dumps (which finds the case in the added test). So this is likely an isolated scenario and I think the PR is ready to be merged with no further considerations made for existing scores. |
|
So I was going to do minimal review on this, just basically a baseline check of a few things, and probably get it in, but something doesn't look right here. The two known-broken beatmaps given by the issue reporter are https://osu.ppy.sh/beatmapsets/814850#osu/1901200 and https://osu.ppy.sh/beatmapsets/2283436#mania/4869637. The first one is a bit annoying to do a cross-check on, so I checked the second, seeked to the first hold note, and... I'm not sure why but it's not the same?
The hold note moved, but... not where it should...? I even extracted conversion mappings for both maps and both are failing even on this branch, so I'm not sure what is going on... |
In particular, mania-specific beatmaps that normally go via the "passthrough" generator should not adjust the stored pattern value. The "spinner" generator, which was previously intended to be used for non-mania-specific beatmaps, is now valid even for mania-specific beatmaps, and uses this value. In other words, another way of writing this would be: ```csharp if (conversion is SpinnerPatternGenerator || conversion is PassThroughPatternGenerator) ? lastPattern : newPattern; ```
|
Good catch. Let's try this one again then... As for https://osu.ppy.sh/beatmapsets/814850#osu/1901200, the beatmap uses negative spinner durations which we clamp to 0 to avoid exceptions in other places of the game. We can throw an exception as we do for every other parsed value exceeding limits, but I'm choosing to ignore this for the time being to avoid potentially throwing many errors into the user's notifications. It's not just spinners - break times have to be validated too and I'm not sure how large the scope of that is: |
|
!diffcalc |
|
Well, the sheet is still empty. Not sure what to do other than merging at this point 😬 |
Fixes #30836
PRing as draft for now until the diffcalc spreadsheet is generated and (hopefully) shows some more differences I can cross check.
Some commentary provided in individual commits. tl;dr: Spinners need to go through the conversion path rather than converted as simple hold notes.