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 "Hold Off" mod (no long notes) #16658

Merged
merged 16 commits into from Feb 2, 2022
Merged

Add "Hold Off" mod (no long notes) #16658

merged 16 commits into from Feb 2, 2022

Conversation

Spooghetti420
Copy link
Contributor

Hello, I have made an osu!mania mod which converts all held notes into single-tap notes, one at the start and one at the end. It has a customization which allows users to select how long a held note needs to be for an end note to be added, if at all.
I saw this mod concept while my friend was playing Quaver and thought it might be a fun addition to osu! too. I hope the code is of acceptable quality, though I wasn't sure how to write a test, so sorry for any inconveniences.
I also don't feel awfully confident about the icon that represents the mod, nor the descriptions/tooltips for customization, so, by all means, please change them as necessary.
Thanks

Copy link
Member

@frenzibyte frenzibyte left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution.

I have made a few notes on the code, in addition to CI results. It would be best to have basic test coverage for this mod (see all osu.Game.Rulesets.Mania.Tests/Mods/TestSceneManiaMod*.cs for example)

osu.Game.Rulesets.Mania/Mods/ManiaModNoLongNotes.cs Outdated Show resolved Hide resolved
osu.Game.Rulesets.Mania/Mods/ManiaModNoLongNotes.cs Outdated Show resolved Hide resolved
osu.Game.Rulesets.Mania/Mods/ManiaModNoLongNotes.cs Outdated Show resolved Hide resolved
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.

there will be code style inspections due to too many empty lines here - please run inspectcode locally to resolve

osu.Game.Rulesets.Mania/Mods/ManiaModNoLongNotes.cs Outdated Show resolved Hide resolved
osu.Game.Rulesets.Mania/Mods/ManiaModNoLongNotes.cs Outdated Show resolved Hide resolved
osu.Game.Rulesets.Mania/Mods/ManiaModNoLongNotes.cs Outdated Show resolved Hide resolved
osu.Game.Rulesets.Mania/Mods/ManiaModNoLongNotes.cs Outdated Show resolved Hide resolved
@pull-request-size pull-request-size bot added size/L and removed size/M labels Jan 27, 2022
@Spooghetti420
Copy link
Contributor Author

Thanks for the suggestions, everyone. I've implemented all of the improvements for now, except the test "suite" is still a little lacking :-) It took a good deal to get it running, so maybe I'll return to it to add some more tests tomorrow, though I still don't know what test ideas to try out.
On a separate note — apologies for once again screwing up the formatting. I ran dotnet format before committing, but I guess that doesn't correct every single issue. I could push a commit to fix this, but I'm unsure as to whether this is a good idea.

@Spooghetti420
Copy link
Contributor Author

Just changed the "threshold" we had before to derive from the beat value of a note rather than from its time in ms, so now only notes that last over 1/2 a beat (by default) get an end note. (Is the way I implemented it a good way to get a hold note's note value, though?)

Like @peppy and @smoogipoo said, it might be too excessive to have this kind of control specifically over end notes, but I included them anyway to see whether this setting is any good. Currently there's a dropdown that allows 1/1, 1/2, 1/4, etc. to be set as the thresholds, though please scrutinize this feature if it's not really appropriate.
Likewise, I've written several more tests with this setting concept in mind, though I cannot see them being useful if we remove the dropdown. I cannot guarantee the code is up to standard, but I've done my best for now. Thanks to everyone for all the feedback once again :)

@peppy peppy changed the title Add No Long Notes mod Add "Hold Off" mod (no long notes) Jan 29, 2022
@smoogipoo
Copy link
Contributor

I think this mod should be made incompatible with ManiaModInvert, and be grouped under the same button.

@peppy
Copy link
Sponsor Member

peppy commented Jan 31, 2022

Agree with incompatible. Disagree with same button (they are functionally very different and should be top-level visible).

@Spooghetti420
Copy link
Contributor Author

Spooghetti420 commented Jan 31, 2022

I also don't know about grouping under the same button, but I have gone and made incompatible with Invert. I also did wonder whether they should be incompatible from the beginning, but I wasn't sure enough to commit it.

Co-authored-by: Bartłomiej Dach <dach.bartlomiej@gmail.com>
bdach
bdach previously approved these changes Feb 1, 2022
@peppy peppy merged commit 4b64670 into ppy:master Feb 2, 2022
@smoogipoo
Copy link
Contributor

smoogipoo commented Feb 2, 2022

For future time travelers who may be interested in improving/adjusting this, my intentions regarding beat length weren't implemented as I suggested them to be. Rather than adding an ending note when the length exceeds some beat length value, my intention was to add a note every X beat length.

It basically results in the following difference:
Original map: https://user-images.githubusercontent.com/1329837/152097084-a4b3dda6-d9c6-440f-80dd-ae85e2d69eff.mp4
This PR: https://user-images.githubusercontent.com/1329837/152097070-9aea110e-480d-47f9-9495-c0a24de8ae85.mp4
This PR with END_NOTE_ALLOW_THRESHOLD = 1: https://user-images.githubusercontent.com/1329837/152097048-f51bf7bc-254b-4219-91ef-258b8b357ed5.mp4
My suggestion with adding notes every 1 beatlength: https://user-images.githubusercontent.com/1329837/152097105-b8280067-bfe6-4666-8e68-2f3fdcaa2bfa.mp4

This takes advantage of two facts:

  • Typically holds (whether in o!m or in osu!) end slightly before the next note, either 1/4 or 1/2 beat away, which allows the next note to arrive on the 1/1 beat.
  • Hold ends in mania are typically hitsound-less for this same reason, so this current implementation generates a bunch of dead jack notes.

@peppy
Copy link
Sponsor Member

peppy commented Feb 2, 2022

Sounds better. Could notes not just be added at every tick?

@smoogipoo
Copy link
Contributor

Haven't explored that/don't know if mania properly uses tick rate at all (because it's not a thing in osu-stable), but possibly.

peppy pushed a commit to ppy/osu-web that referenced this pull request Feb 4, 2022
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.

None yet

8 participants