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 "Single Tap" mod for osu! ruleset #17781

Closed
wants to merge 4 commits into from
Closed

Conversation

Euro20179
Copy link

This adds a mod that only allows single tapping as discussed here

It is stacked ontop of the alternate mod, because that felt like the most natural place to put it.

@Euro20179
Copy link
Author

I've kinda changed the layout, but before I make a push here's a screenshot, the mod in the code is called OsuModTapBlocking.cs
image

@peppy
Copy link
Member

peppy commented Apr 12, 2022

I believe the proposal was to abstract the shared logic, not combine both into a single mod.

@peppy
Copy link
Member

peppy commented Apr 12, 2022

To answer your question that should have been posted here: You can find many examples of this, look for abstract implementations of mods, such as ModRateAdjust.

@Euro20179
Copy link
Author

To answer your question that should have been posted here:

Noted,

look for abstract implementations of mods, such as ModRateAdjust

And noted.

@Euro20179
Copy link
Author

Alternate:
image

Single tap:
image

If anyone has ideas for an icon for single tap I'll add that before I push here.

The mods are seperated into ModInputBlocker where most of the code is, ModSingleTap, ModAlternate, where stuff like the mod name is. and OsuModSingleTap, OsuModAlternate. Where the logic of whether or not the tap should count is.

There's also tests for single tap and alternate.

@ikkue
Copy link

ikkue commented Apr 12, 2022

If anyone has ideas for an icon for single tap I'll add that before I push here.

I think the icon should be an image of a single keycap with an optional index finger over it. But the index finger might get confused with Touch Device, hence "optional".

@Euro20179
Copy link
Author

I think the icon should be an image of a single keycap

I think this is a good idea as well, but:
a) I can't design anything
b) FontAwesome doesn't have a keycap, the closest that I can find is a square, but it's comically large and looks bad

@frenzibyte frenzibyte changed the title osu mod: single tap Add "Single Tap" mod for osu! ruleset Apr 12, 2022
@frenzibyte
Copy link
Member

frenzibyte commented Apr 12, 2022

If anyone has ideas for an icon for single tap I'll add that before I push here.

I would say push for now and leave picking icons for later. It's very likely for this mod to not have any icons assigned to it for the time being.

The mods are seperated into ModInputBlocker where most of the code is, ModSingleTap, ModAlternate, where stuff like the mod name is. and OsuModSingleTap, OsuModAlternate. Where the logic of whether or not the tap should count is.

Abstract mod classes which derive into multiple implementations usually have Mod as a suffix rather than a prefix, i.e. this should be InputBlockingMod or otherwise. Also, I would suggest this mod is left at the osu! ruleset project for the time being, rather than unnecessarily moving it out when no other rulesets currently implement.

@Euro20179
Copy link
Author

Also, I would suggest this mod is left at the osu! ruleset project for the time being, rather than unnecessarily moving it out when no other rulesets currently implement.

Well if we're doing this I'll probably remove ModAlternate, and ModSingleTap and just put them in OsuModAlternate, and OsuModSingleTap unless you guys think otherwise.

@frenzibyte
Copy link
Member

That's part of what I meant there, yes. Don't see any needs for adding abstract mod classes if it's only implemented for one ruleset.

@Euro20179
Copy link
Author

Euro20179 commented Apr 12, 2022

Ugh I was so careful not to make useless pushes, but I messed up the description in single tap, before I make another push, lemme know of anything else I should change.

@PercyDan54
Copy link
Contributor

Ugh I was so careful not to make useless pushes, but I messed up the description in single tap, before I make another push, lemme know of anything else I should change.

There's multiple code formatting issue, try running InspectCode.ps1 locally

Comment on lines +58 to +66
OsuHitObject firstHitObject;
try
{
firstHitObject = ruleset.Beatmap.HitObjects[0];
}
catch(IndexOutOfRangeException)
{
firstHitObject = null;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd just use .FirstOrDefault()

Copy link
Member

Choose a reason for hiding this comment

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

I would like to add that there's not really any reason to alter existing modern code if it doesn't affect anything, to avoid actually making it worse.

@FunnyFortniteFan
Copy link

If anyone has ideas for an icon for single tap I'll add that before I push here.
Perhaps a jackhammer would work well?

Copy link
Contributor

@PercyDan54 PercyDan54 left a comment

Choose a reason for hiding this comment

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

For the icon I think HandPointUp

public override string Name => "Single Tap";
public override string Acronym => "ST";
public override string Description => @"Alternate tapping!";
public override Type[] IncompatibleMods => new[] { typeof(ModAutoplay) };
Copy link
Contributor

Choose a reason for hiding this comment

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

Should also be incompatible with Relax

Comment on lines -21 to +14
public override string Name => @"Alternate";
public override string Acronym => @"AL";
public override string Name => "Alternate";
public override string Acronym => "AL";
Copy link
Contributor

Choose a reason for hiding this comment

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

Revert this

@Euro20179
Copy link
Author

Ok, I've fixed the couple issues you guys pointed out, and ran InspectCode.sh.
It told me to put the copyright header so i did, then dotnet jb inspectcode "osu.Desktop.slnf" --output="inspectcodereport.xml" --caches-home="inspectcode" --verbosity=WARN outputs a bunch of the same error, here's the header


--- EXCEPTION #1/1 [LoggerException]
Message = “Unknown tools version: 17.0”
ExceptionPath = Root
ClassName = JetBrains.Util.LoggerException
HResult = COR_E_APPLICATION=80131600

So I ran dotnet format on osu.Game.Rulesets.Osu and osu.Game.Rulesets.Tests.Osu which fixes presumably some of the code errors, as it said it "NamingSylteCodeFixProvider doesn't support Fix All in Solution"
In addition it formatted a bunch of code I didn't touch. If that's ok then great, otherwise I'll spend more time to try to debug that jetbrains error.

@PercyDan54
Copy link
Contributor

Ok, I've fixed the couple issues you guys pointed out, and ran InspectCode.sh. It told me to put the copyright header so i did, then dotnet jb inspectcode "osu.Desktop.slnf" --output="inspectcodereport.xml" --caches-home="inspectcode" --verbosity=WARN outputs a bunch of the same error, here's the header


--- EXCEPTION #1/1 [LoggerException]
Message = “Unknown tools version: 17.0”
ExceptionPath = Root
ClassName = JetBrains.Util.LoggerException
HResult = COR_E_APPLICATION=80131600

So I ran dotnet format on osu.Game.Rulesets.Osu and osu.Game.Rulesets.Tests.Osu which fixes presumably some of the code errors, as it said it "NamingSylteCodeFixProvider doesn't support Fix All in Solution" In addition it formatted a bunch of code I didn't touch. If that's ok then great, otherwise I'll spend more time to try to debug that jetbrains error.

You could try to use Rider EAP or ReSharper EAP as you don't need to buy a license for them, and fix formatting inspections manually as a alternative.

@Euro20179
Copy link
Author

You could try to use Rider EAP or ReSharper EAP as you don't need to buy a license for them, and fix formatting inspections manually as a alternative.

I downloaded one of them I forget now, but it didn't work. I then tried installing resharper with dotnet tool install -g jetbrains.resharper.globaltools , and it didn't work.
But then I realized that if it wasn't installed, the error wouldn't occur, long story short I could not find a solution

Is there a list of formatting guidelines I can follow, I found .editorconfig, but it seems the rules are very inconsistently followed, unless I'm misreading it.

I have done some formatting, like putting spaces after if, and ordering the using statements correctly.

@frenzibyte
Copy link
Member

frenzibyte commented Apr 15, 2022

You could try to use Rider EAP or ReSharper EAP as you don't need to buy a license for them, and fix formatting inspections manually as a alternative.

I downloaded one of them I forget now, but it didn't work. I then tried installing resharper with dotnet tool install -g jetbrains.resharper.globaltools , and it didn't work. But then I realized that if it wasn't installed, the error wouldn't occur, long story short I could not find a solution

I'm not sure what you mean by Rider or ReSharper just straight up not working for you, but I'll leave that for you to continue figuring it out (would suggest starting here).

In general, you should have got code-style analysis working on your end before submitting this PR, as mentioned in the contributing guidelines:

CleanShot 2022-04-15 at 23 45 40@2x

@peppy
Copy link
Member

peppy commented Jul 12, 2022

This PR is heavily conflicted since a4ca8bf. Will require some reconsideration. Maybe to the point of opening a new PR? @Euro20179 do you still have interest in tackling this one?

@Euro20179
Copy link
Author

do you still have interest in tackling this one?

Not particularly.

@peppy
Copy link
Member

peppy commented Jul 12, 2022

No worries. Will close this for now and we can revisit at a later point.

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.

6 participants