Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add key conversion mod support for osu!mania beatmaps #27374

Open
wants to merge 17 commits into
base: master
Choose a base branch
from

Conversation

DaiterGG
Copy link

@DaiterGG DaiterGG commented Feb 25, 2024

added support for mania to mania conversion
fist time contributing so unsure about pretty much everything, but mainly the way I added ApplyToBeatmap() method

Closes #18562

public override Type[] IncompatibleMods => new[]
public void ApplyToBeatmap(IBeatmap beatmap)
{
if (beatmap.BeatmapInfo.Ruleset.ShortName != "mania")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this check is needed since there is no valid case this mod is ever used outside of mania.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This mod convert std maps to different key counts.
And this conversion shouldn't apply for those

Copy link
Author

@DaiterGG DaiterGG Feb 25, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It can be removed since all checks later in this method will fail

@frenzibyte frenzibyte changed the title Mania key mod Add key conversion mod support for osu!mania beatmaps Feb 25, 2024
Copy link
Collaborator

@bdach bdach left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Structurally none of this works as far as I'm concerned. The code performing the conversion should be in ManiaBeatmapConverter. Because this PR isn't putting it there, ManiaModDualStages is probably going to break (and if it were to be implemented similarly, there'd be code duplication and/or an introduced order dependency). The IsForCurrentRuleset conditional in the keymod should be gone because the beatmap converter should be handling it. And so on and so forth.

Not even looking at the implementation details because this just can't work structurally as is.

@DaiterGG
Copy link
Author

I took this approach because it's just easier for me.
Gonna look at different way now, thank you

@bdach
Copy link
Collaborator

bdach commented Feb 26, 2024

Just as a reference, I'd like the current conversion algorithm from osu! to mania to be clearly delineated from the changing of key count in mania maps specifically, something like so:

if (isForCurrentRuleset && TargetColumns != columns)
{
   // this PR goes here
}
else
{
    // existing conversion code goes here
}

@DaiterGG
Copy link
Author

DaiterGG commented Mar 1, 2024

9e4e3c93-0be7-49d6-944e-6bc515a28b55.mp4

here is some visuals for new algorithm
it preserves more original structure where possible without deleting more notes

@DaiterGG DaiterGG requested a review from bdach March 1, 2024 17:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

osu!mania key mods can be applied to mania-first beatmaps, even though they do nothing
4 participants