-
Notifications
You must be signed in to change notification settings - Fork 91
Fix non-CL acc generation #275
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
base: master
Are you sure you want to change the base?
Conversation
} | ||
|
||
// Clamp goods if total amount is bigger than possible | ||
countGood -= Math.Clamp((int)(countGood + countMeh + countMiss - totalResultCount), 0, (int)countGood); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't this be =
instead of -=
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, this is correct. Thanks to increased weight (we add more 100s than in CL case) we can add more 100s than possible. This line is removing those extra impossible 100s.
totalResultCount
is the maximum amount of judgements we can have. And countGood + countMeh + countMiss
is total amount of judgements we have generated. If it's greater than first - remove the extra judgements.
This is only relevant for very low acc scores.
int relevantResultCount = totalResultCount - countMiss; | ||
// Relevant result count without misses (normal misses and slider-related misses) | ||
// We need to exclude them from judgement count so total value will be equal to desired after misses are accounted for | ||
double relevantResultCount; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
countSuccessfulHits
maybe? similar to https://github.com/ppy/osu/blob/master/osu.Game.Rulesets.Osu/Difficulty/OsuPerformanceCalculator.cs#L499
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, it's not "true" count successful hits because in non-CL case it's also weighted
Like it's just a number used for calculations, it doesn't have some objective meaning
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's fine even if its weighted, im mostly concerned with the naming being self-explanatory enough for people to grasp the idea behind all this without too much strain - right now if i see relevantResultCount
somewhere down the line it requires scrolling up to read the comment above it to remember what it actually is
double relevantResultCount; | ||
|
||
// If there's no classic slider accuracy - we need to weight circle judgements accordingly | ||
double normalJudgementWeight = 1.0; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
circleJudgementsWeight
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd also appreciate a comment as to why this exist and why we need to use it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For example let's assume that map has enough sliders so 10% of the score belongs to sliderends and sliderpoints. In this case we need to compensate for the fact that "circles" (includes sliderheads and spinners) only hold 90% of the influence they would've had in the CL scenario where they're the only type of judgments.
circleJudgementWeight is technically inaccurate (I probably need to fix comment above). This refers to 300s/100s/50s/misses. When "not normal" judgements are slider end hits and big tick hits.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nonNestedJudgementsWeight
then maybe? not really sure what'd be the best naming for this to make it self-explanatory. maybe an explanation comment would be enough, not sure
@tsunyoku any ideas?
|
||
int totalResultCount = beatmap.HitObjects.Count; | ||
|
||
int countLargeTicks = beatmap.HitObjects.Sum(obj => obj.NestedHitObjects.Count(x => x is SliderTick or SliderRepeat)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
count...
is used for hitresults here, so maybe total...
instead? preferably for the other usages in this file that already exist as well, to reduce the confusion
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
totalLargeTicks doesn't sounds good to me
totalResultCount
has "count" in it, and "totalResult" is a descriptor of what kind of count it is
maybe rename to largeTickCount
would be better?
It doesn't accounts for slider judgements affecting judgements, this PR is fixing this.
It's kinda broken for accuracies below 40% because it can't add slider misses, but it's not very important.