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

Basic tl-tapping consideration for stamina #20558

Merged
merged 19 commits into from
Nov 7, 2022

Conversation

vunyunt
Copy link
Contributor

@vunyunt vunyunt commented Oct 3, 2022

Prereqs:

The main aim of this PR is to improve difficulty calculation for patterns with extremely fast and long mono patterns. In this case, the player can use more than 2 fingers to press the same keys (known as tl-tapping), making the original assumption of 2 fingers available for each colour false.

Difficulty Calculator Changes:

  • Refactored colour slightly to allow access to colour data in all notes instead of just the first note of each pattern chunk. The detection of first note is moved into colour evaluator.
  • Implemented variable available finger count for stamina. Mono notes more than 300ms away from a colour change is considered to have an available finger count of 4.
  • Removed convert specific nerfs in difficulty calculation.
  • Reduced the minimum note interval for stamina to an unrealistically small value (1ms). This is now only to prevent division by zero or negative values.

Performance Calculator Changes:

  • Introduction of a rulesetTaiko bool, which applies previous bonuses strictly to the taiko ruleset as a way to combat easy-to-memorise converts. This is temporary until the reading skill is implemented.
  • The HDFL bonus to accuracy has been changed to be able to give 0 bonus, and an increase cap to 1.1x. This was in response to issues discovered within very short maps paired with HDFL giving the same bonus as those with 1000 objects.

SR/PP spreadsheets:

With converts: https://docs.google.com/spreadsheets/d/1eS4BBq7Ku7oY62nHtOnRBbrnhSTiMphLiHpwyWwEcwA/edit#gid=686072955

Without converts: https://docs.google.com/spreadsheets/d/12tejBO5JFGvOfuAKnayHHcWv5MWxOfH591eT6Vhk0h4/edit

As of 9065cb0

Lawtrohux
Lawtrohux previously approved these changes Oct 3, 2022
Copy link
Member

@Lawtrohux Lawtrohux left a comment

Choose a reason for hiding this comment

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

looks fine to me. in the future when more skills are worked on we can hope that converts and their respective playstyles don't continue to need special consideration.

@Lawtrohux
Copy link
Member

@smoogipoo can we get a smoogisheet for this build with converts enabled please?

@smoogipoo
Copy link
Contributor

SR/PP sheet added to the description.

@smoogipoo smoogipoo requested a review from a team October 4, 2022 09:22
Lawtrohux
Lawtrohux previously approved these changes Oct 4, 2022
Copy link
Member

@Lawtrohux Lawtrohux left a comment

Choose a reason for hiding this comment

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

reapproving as smoogisheet was just to double check unseen values.

what i will say is that some loved converts gain absolute stupid sr, but this solely due to them being impossible maps in the first place, however this issue is past this fix/buff/nerf to things so ill leave that up to discretion having a significant amount of 10* - 40* converts.

@smoogipoo
Copy link
Contributor

So just to confirm, it's intended that https://osu.ppy.sh/scores/taiko/151614379 gains 300pp and becomes the new pp record?

@Lawtrohux
Copy link
Member

So just to confirm, it's intended that https://osu.ppy.sh/scores/taiko/151614379 gains 300pp and becomes the new pp record?

In regards to other scores gaining, this is justified. Converts were previously fixed nerfed 20% by the nature of the code, and are in a better place now then before. While tweaks still need to be made, this is planned for the next large pr within the coming week containing the rewrite of the rhythm skill.

Comment on lines +30 to +46
private static int availableFingersFor(TaikoDifficultyHitObject hitObject)
{
DifficultyHitObject? previousColourChange = hitObject.Colour.PreviousColourChange;
DifficultyHitObject? nextColourChange = hitObject.Colour.NextColourChange;

if (previousColourChange != null && hitObject.StartTime - previousColourChange.StartTime < 300)
{
return 2;
}

if (nextColourChange != null && nextColourChange.StartTime - hitObject.StartTime < 300)
{
return 2;
}

return 4;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is it 2 or 4 fingers, instead of 1 or 2 fingers?

Copy link
Member

Choose a reason for hiding this comment

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

mechanically, normal taiko players have 2 fingers available to hit each colour, with the TL playstyle, they use four and above fingers solely to hit the single colour.

2 fingers refers to the normal count, while 4 refers to the new count if the threshold is reached.

Comment on lines 52 to 55
foreach (var hitObject in monoStreak.HitObjects)
{
hitObject.Colour.MonoStreak = monoStreak;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Isn't it functionally equivalent to add:

hitObject.Colour.AlternatingMonoPattern = monoPattern;
hitObject.Colour.RepeatingPattern = repeatingHitPattern;

In this loop? Just to reduce the amount of looping.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah it should be, will apply this when I'm back

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Applied in 122064d

Copy link

@mangomizer mangomizer left a comment

Choose a reason for hiding this comment

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

Smoogisheet seems to feature mostly converts (which I agree is the main goal of this), and PR does a fair job in tackling this issue.

I would like to see the results for mode-specific maps (we should expect minimal changes) before making any call on this though (can't seem to find many in the smoogisheet).

@smoogipoo
Copy link
Contributor

I would like to see the results for mode-specific maps

Added to OP.

@smoogipoo
Copy link
Contributor

Updated the spreadsheets in the description because they were generated incorrectly (see ppy/osu-queue-score-statistics#102). Please re-check.

Lawtrohux
Lawtrohux previously approved these changes Nov 1, 2022
Copy link
Member

@Lawtrohux Lawtrohux left a comment

Choose a reason for hiding this comment

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

lgtm! the new values actually look better as per the accuracy issue

mangomizer
mangomizer previously approved these changes Nov 1, 2022
Copy link

@mangomizer mangomizer left a comment

Choose a reason for hiding this comment

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

I believe I've raised in the past that it would be useful to run it through a sample of "known" maps as a benchmark, even if just to confirm that the change is minimal.

If I am right in assuming that maps not showing up is indicative of minimal change (ie. <0.1* SR), approved.

@smoogipoo
Copy link
Contributor

If I am right in assuming that maps not showing up is indicative of minimal change (ie. <0.1* SR), approved.

Correct, it only shows SR differences >0.1*.

@Lawtrohux
Copy link
Member

I'll just quickly state that while the comment in the detection of rulesets is indeed temporary in regards to specific multipliers. It is planned to be used in a couple of others ways in future PR's, meaning that the related PR's to assist with this are not for a temporary function.

smoogipoo
smoogipoo previously approved these changes Nov 2, 2022
@smoogipoo smoogipoo dismissed stale reviews from mangomizer, Lawtrohux, and themself via 5448c02 November 2, 2022 01:15
smoogipoo
smoogipoo previously approved these changes Nov 2, 2022
@smoogipoo
Copy link
Contributor

@peppy or @bdach give this one a once over please.

@@ -43,6 +43,9 @@ protected override PerformanceAttributes CreatePerformanceAttributes(ScoreInfo s
if (totalSuccessfulHits > 0)
effectiveMissCount = Math.Max(1.0, 1000.0 / totalSuccessfulHits) * countMiss;

// TODO: The detection of rulesets is temporary until the leftover old skills have been reworked.
bool taikoSpecificBeatmap = score.BeatmapInfo.Ruleset.OnlineID == 1;
Copy link
Member

Choose a reason for hiding this comment

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

Would it be better to call this isConvert similar to everywhere else?

Copy link
Contributor

Choose a reason for hiding this comment

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

That's a fair point.

Copy link
Member

@peppy peppy left a comment

Choose a reason for hiding this comment

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

Seems fine code-wise

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Deployed
Development

Successfully merging this pull request may close these issues.

5 participants