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

Mania mod invert revork #27508

Closed
wants to merge 10 commits into from
Closed

Mania mod invert revork #27508

wants to merge 10 commits into from

Conversation

DaiterGG
Copy link

@DaiterGG DaiterGG commented Mar 6, 2024

Closes #25212
Treat hold note as one location
and
Add a checkbox option for different long note conversion
Screenshot_17
Screenshot_18

@pull-request-size pull-request-size bot added size/M and removed size/XS labels Mar 6, 2024
@DaiterGG DaiterGG changed the title Mod invert Mania mod invert revork Mar 6, 2024
@nferhat
Copy link

nferhat commented Dec 18, 2024

@DaiterGG Could you please make this mergable? This fix is quite needed ngl

Comment on lines +34 to +35
[SettingSource("Invert Long Notes", "Invert long notes into nothing.")]
public BindableBool FullInvert { get; } = new BindableBool();
Copy link
Collaborator

Choose a reason for hiding this comment

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

needs a better description because i have no idea what this is supposed to be

also i assume the current logic for this in master is the same as if this was set to true. so either this will have to be changed to default to true, or all existing scores with invert in the database will need to be updated for correct playback. not sure what about replays either. i guess they'd just be hard-broken?

(startTime: h.EndTime, samples: h.GetNodeSamples(1))
}))
.OrderBy(h => h.startTime).ToList();
List<(double startTime, IList<HitSampleInfo> samples, string type)> locations;
Copy link
Collaborator

Choose a reason for hiding this comment

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

why is this using a string for type? use an enum? or bool even, if you must

Comment on lines +63 to +64
if (locations[i].type == "release")
continue;
Copy link
Collaborator

Choose a reason for hiding this comment

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

doing this changes the behaviour of the mod completely, no? even if FullInvert is on?

this PR really needs a proper description because i have no idea what anything is here, and i shouldn't have to reverse-engineer the purpose of everything here from first principles

@DaiterGG
Copy link
Author

DaiterGG commented Dec 28, 2024

If I remember correctly, Checkbox off is - Extended
on is - Inversed
terminology is well described here - #22845
I switched to an alternative client but I can put some effort to merged this.
One thing I concerned about is old replays compatibility, as I'm not familiar with code base at all,
and this pr changes default behavior completely with both options.

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 "Invert" mod creates unwanted jacks
4 participants