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... |
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.