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

Don't punish slider hit windows in difficulty calculations when slider head accuracy is in use #28267

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

Conversation

tsunyoku
Copy link
Sponsor Member

@tsunyoku tsunyoku commented May 20, 2024

In similar vein to #27063. When slider head accuracy is in use, the logic process of "this makes hit windows easier" no longer applies.

This will affect difficulty attributes and will rely on "lazer no mod" and CL not re-using the same attributes (I think they currently do?)

This is unlikely to cause noticable differences on the majority of rhythm-heavy maps as they tend to not have a lot of slider rhythm.

@pull-request-size pull-request-size bot added size/M and removed size/S labels May 20, 2024
@tsunyoku tsunyoku requested a review from a team May 20, 2024 18:20
@tsunyoku tsunyoku changed the title Don't punish slider hit windows in rhythm calculations when slider head accuracy is in use Don't punish slider hit windows in difficulty calculations when slider head accuracy is in use May 20, 2024
@apollo-dw
Copy link
Member

Manually tested some maps. Most are unchanged as expected, but the single biggest change I saw by far was on DeviousPanda's Asteroid Field of DECAPLETS. Between classic and nomod, it goes from 1005pp to 1257pp. I think this is reasonable given the map though...

@smoogipoo bit of a tough ask, but in the future I expect to run into more "adds calculation for mods" PRs that have few set scores. Would it be feasible to generate sheets for these proposals where we get "nomod vs. [mod]" comparisons across all ranked & loved maps?

@Givikap120
Copy link
Contributor

isn't it will be faster and more clean to just use bool isClassicSliders in rhythm evaluator instead of just pushing all those mods stuff here?

@tsunyoku
Copy link
Sponsor Member Author

I think either case could be argued but I chose to pass mods to be more explicit (and in case we end up doing more with them in future) but I'm really fine with either.

@bdach
Copy link
Collaborator

bdach commented May 21, 2024

This will affect difficulty attributes and will rely on "lazer no mod" and CL not re-using the same attributes (I think they currently do?)

Yes, CL is basically treated the same way as nomod right now.

@smoogipoo
Copy link
Contributor

I expect to run into more "adds calculation for mods" PRs that have few set scores. Would it be feasible to generate sheets for these proposals where we get "nomod vs. [mod]" comparisons across all ranked & loved maps?

Is this already achieved by the MOD_FILTERS env variable? Or is there another case to handle?

@smoogipoo
Copy link
Contributor

Let's try it I guess, if I'm understanding correctly you want...

!diffcalc
MOD_FILTERS=+CL

Copy link

github-actions bot commented May 21, 2024

@smoogipoo
Copy link
Contributor

!diffcalc
MOD_FILTERS=-CL

Copy link

github-actions bot commented May 21, 2024

@smoogipoo
Copy link
Contributor

This needs OsuModClassic to be added to

protected override Mod[] DifficultyAdjustmentMods => new Mod[]
{
new OsuModTouchDevice(),
new OsuModDoubleTime(),
new OsuModHalfTime(),
new OsuModEasy(),
new OsuModHardRock(),
new OsuModFlashlight(),
new MultiMod(new OsuModFlashlight(), new OsuModHidden())
};

Which brings up another issue - that we're going to be doubling the storage of this in the database and it might need to wait for realtime difficulty.

The sheet is likely empty because of this - osu-difficulty-calculator (the server-side component) attaches the classic mod to all beatmaps here:

public IEnumerable<DifficultyAttributes> CalculateAllLegacyCombinations(CancellationToken cancellationToken = default)
{
var rulesetInstance = ruleset.CreateInstance();
foreach (var combination in CreateDifficultyAdjustmentModCombinations())
{
Mod classicMod = rulesetInstance.CreateMod<ModClassic>();
var finalCombination = ModUtils.FlattenMod(combination);
if (classicMod != null)
finalCombination = finalCombination.Append(classicMod);
yield return Calculate(finalCombination.ToArray(), cancellationToken);
}
}

@tsunyoku
Copy link
Sponsor Member Author

Oh dear. Yeah I thought something like this would happen... I'll add OsuModClassic to the list of difficulty adjustment mods, but what do you suggest for the rest? Do we remove the line that attaches classic mod to all beatmaps?

@smoogipoo
Copy link
Contributor

I'm not immediately sure. It will need a team discussion that we haven't had yet and infra considerations.

Copy link
Member

@stanriders stanriders left a comment

Choose a reason for hiding this comment

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

It's unfortunate that we'll have to have this change stuck in limbo for now, but that's to be expected with the new lazer mods (and especially mod settings)

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

Successfully merging this pull request may close these issues.

None yet

6 participants