ppy / osu

rhythm is just a *click* away!
https://osu.ppy.sh
MIT License
15.13k stars 2.25k forks source link

SS rank in osu!mania requires all judgements to be PERFECT #24238

Open bdach opened 1 year ago

bdach commented 1 year ago

Type

Game behaviour

Bug description

Currently, the user's score rank is calculated entirely based on their accuracy:

https://github.com/ppy/osu/blob/50e6f0aee9c0a297a7c5d0b05b3306ab177d6abb/osu.Game/Rulesets/Scoring/ScoreProcessor.cs#L446-L463

In mania, the highest hit result is PERFECT:

https://github.com/ppy/osu/blob/50e6f0aee9c0a297a7c5d0b05b3306ab177d6abb/osu.Game.Rulesets.Mania/ManiaRuleset.cs#L378-L390

Therefore, hitting GREATs causes an accuracy drop, which means that it is not possible to achieve an SS rank with GREAT judgements at all, which is a departure from what stable does without ScoreV2 active, wherein an SS rank requires just all GREATs or better, and probably needlessly harsh in general.

This was discussed on discord, and the potential solution for this would be to convert the extra 50 score for a PERFECT compared to a GREAT to bonus score.

Screenshots or videos

n/a

Version

current master

Logs

n/a

alexpado commented 1 year ago

I did manage to modify this image

My only issue is that I modified this (setting to 300): https://github.com/ppy/osu/blob/22163020c348c8ad1545b95d33665277425f4a1f/osu.Game/Rulesets/Judgements/Judgement.cs#L162-L163

As far as I know, this affect EVERY rulesets, which is a big nope.

Even if I override the ToNumericResult static method, this would be pointless: https://github.com/ppy/osu/blob/22163020c348c8ad1545b95d33665277425f4a1f/osu.Game/Rulesets/Scoring/ScoreProcessor.cs#L225-L226

It calls directly the static method contained in the Judgement class.

As only the score counts toward the accuracy, the value should be overridable per-rulesets. https://github.com/ppy/osu/blob/22163020c348c8ad1545b95d33665277425f4a1f/osu.Game/Rulesets/Scoring/ScoreProcessor.cs#L306-L308

From there, I think adding an overridable method that returns the value of the JudgmentResult could be possible, letting any class extending ScoreProcessor freely modify the hit value.

I don't know if it's the right approach, so I'll wait for feedback before going further.

Edit: For reference, here's the same play without any edits after watching the replay image

bdach commented 1 year ago

@alexpado that is not the correct direction to proceed with. changing the numerical value of PERFECTs is the wrong approach. PERFECT should be worth 350 315 points, to make sure that total score is positively affected by hitting more PERFECTs.

it's going to either be a question of moving the extra 50 score to an entirely separate bonus judgement, or changing the rank awarding method to no longer be based on pure accuracy, or something along similar lines. but PERFECTs must continue to grant more points than GREATs.

alexpado commented 1 year ago

I overlooked this part, my bad.

My question regarding allowing rulesets to change hit values still stand though, which would highly change how things are handled, but could be done in a discussion.

For our issue at hand, what about giving a "weight" for each HitResult which could be used as a separate value from score to process accuracy ? The number could be the score removing the bonus, just like I did to "fix" the issue.

        public static int ToAccuracyWeight(HitResult result)
        {
            switch (result)
            {
                default:
                    return 0;

                case HitResult.SmallTickHit:
                    return 10;

                case HitResult.LargeTickHit:
                    return 30;

                case HitResult.Meh:
                    return 50;

                case HitResult.Ok:
                    return 100;

                case HitResult.Good:
                    return 200;

                case HitResult.Great:
                    return 300;

                case HitResult.Perfect:
                    return 300;

                case HitResult.SmallBonus:
                    return SMALL_BONUS_SCORE;

                case HitResult.LargeBonus:
                    return LARGE_BONUS_SCORE;
            }
        }

This mean that each HitResult could affect independently accuracy and score, while conserving the bonus for score processing.

Also, each HitResult could have their accuracy weigth fine-tuned to make some hit more important for accuracy than others.

Side question: You are stating that PERFECT should be worth 350 points, but actually in the code it is 315, should it be changed to 350 ?

EDIT: Again, this would affect every ruleset, as all of them share the same process. Is this really okay ?

bdach commented 1 year ago

This one I'm not sure what to do with, there are many distinct directions you could pick here, every one as easy to implement as the other, but I have no real idea what the community wants.

@smoogipoo did you perhaps gather some feedback on this at COE? would you want to handle this one yourself?

smoogipoo commented 1 year ago

The general discussion, before and during COE, seems to be to try and get the extra score of PERFECTs into bonus score. Reasoning being, or is supported by, other VSRGs doing the same.

bdach commented 1 year ago

Alright well that seems to settle it. Moving to "needs implementation", seems like something virtually any one of us should be able to handle (but will leave to you if you're so inclined, obviously).

bdach commented 1 year ago

Hmm well maybe one more thing: is that bonus score from PERFECTs gonna be capped or uncapped? That's still something we probably need to figure out before an implementation.

Heads up: crosstalk with https://github.com/ppy/osu-queue-score-statistics/issues/133 here.

smoogipoo commented 1 year ago

There was no discussion on it being capped, afaik.

stockfishcooker commented 1 year ago

I'd say let the bonus be capped just like ScoreV1 and ScoreV2. The fantastic judgments most likely will only come in handy for breaking ties between SSes made by top players, and I don't want it to be too influencing for normal play. Of course, I'm not a top player, and this is just an opinion (I only have 2 SSes right now lol). I would like to hear other players that are better than me share their thoughts.

bdach commented 1 year ago

@stockfishcooker at this point we're primarily looking for already-formed opinions from established groups of players rather than opinions of individuals who happen to have access to github, so unfortunately your comment has little bearing on the final direction and therefore does not help us very much. I hope you understand.

peppy commented 11 months ago

I'm going to give this one a try.

peppy commented 11 months ago

The most feasible approach I can think of would be to adjust ToNumericResult(HitResult.Perfect) to return 300 (as touched on earlier in this thread), and then add a nested bonus judgement which triggers the actual extra score for perfect hits.

Whether this scoring change can be applied globally in the currently static context (with xmldoc explaining how Perfect works) or whether it needs to be somehow customisable per ruleset is another question though.

In the interest of keeping things simple I'd propose obliterating it globally as we are still early in development and custom rulesets can cope with the change (and add their own bonus judgements if they want perfect to give more score), but would need more opinions (@smoogipoo @bdach).

aka:

diff --git a/osu.Game/Rulesets/Judgements/Judgement.cs b/osu.Game/Rulesets/Judgements/Judgement.cs
index f60b3a6c02..cd1e81046d 100644
--- a/osu.Game/Rulesets/Judgements/Judgement.cs
+++ b/osu.Game/Rulesets/Judgements/Judgement.cs
@@ -190,10 +190,9 @@ public static int ToNumericResult(HitResult result)
                     return 200;

                 case HitResult.Great:
-                    return 300;
-
+                // Perfect doesn't actually give more score / accuracy directly.
                 case HitResult.Perfect:
-                    return 315;
+                    return 300;

                 case HitResult.SmallBonus:
                     return SMALL_BONUS_SCORE;
diff --git a/osu.Game/Rulesets/Scoring/HitResult.cs b/osu.Game/Rulesets/Scoring/HitResult.cs
index ccd1f49de4..fed338b012 100644
--- a/osu.Game/Rulesets/Scoring/HitResult.cs
+++ b/osu.Game/Rulesets/Scoring/HitResult.cs
@@ -55,6 +55,13 @@ public enum HitResult
         [Order(1)]
         Great,

+        /// <summary>
+        /// This is an optional timing window tighter than <see cref="Great"/>.
+        /// </summary>
+        /// <remarks>
+        /// By default, this does not give any bonus accuracy or score.
+        /// To have it affect scoring, consider adding a nested bonus object.
+        /// </remarks>
         [Description(@"Perfect")]
         [EnumMember(Value = "perfect")]
         [Order(0)]
peppy commented 11 months ago

With regards to the method being static, we may actually need to revisit this for osu!taiko scoring fixes (see https://github.com/ppy/osu/issues/9087)...

bdach commented 11 months ago

I can think of would be to adjust ToNumericResult(HitResult.Perfect) to return 300 (as touched on earlier in this thread), and then add a nested bonus judgement which triggers the actual extra score for perfect hits.

The nested judgement I can agree with for sure, but I'm not sold on scaling HitResult.Perfect back to 300. The primary reason is the result screen.

If you have notes give a Perfect and a nested LargeBonus, you're going to have the count of perfects displaying basically twice which feels bad and redundant. I'd say it should probably be Great + bonus? Dunno.

Health is going to also be an issue here, by the way. LargeBonus has the same HP increase as Great. Making https://github.com/ppy/osu/pull/25052 relevant.

peppy commented 11 months ago

If you have notes give a Perfect and a nested LargeBonus, you're going to have the count of perfects displaying basically twice which feels bad and redundant

I'd argue that people have come to expect the Perfect counter to exist and moving away from that (aka only showing bonus) will likely not go down well. FWIW I was also thinking in that direction immediately, and even had the proposal typed up, but nuked it all because I don't think it will work from a user-acceptance perspective.

Health is going to also be an issue here, by the way

Will have to deal with that somehow, yeah.

bdach commented 11 months ago

I'd argue that people have come to expect the Perfect counter to exist and moving away from that (aka only showing bonus) will likely not go down well

Fair point.

I think we may be able to just hide away the bonus counter so that it doesn't look weird and redundant. Not declaring it in Ruleset.GetValidHitResults() will probably do it (but needs a proper testing and sweep of usages to ensure no unintended consequences).

peppy commented 11 months ago

Yeah, I accidentally found that one out (by forgetting to add the new judgement to that list and wondering why it displayed nowhere).

I've got this working pretty well in limited gameplay testing, so PR'd it for further comment. Feels like a pretty solid direction to me.

bdach commented 9 months ago

After https://github.com/ppy/osu/pull/25945, this issue is relevant again.

peppy commented 9 months ago

Dunno where to stand on this. I guess we listen to people for some weeks and whoever's voice is louder wins?

Personal opinion is that I still agree with bonus scoring, potentially with the bonus reduced to a negligible amount to avoid bonus on long maps overpowering short maps. Dunno.

Rekkonnect commented 9 months ago

Playing mania for years, I barely score any SSes with the stable algorithm, let alone 1,000,000s. We can tell the difference between SS and 1M; the latter being a "real" SS with tournament acc calculation. Maybe a mode where we could see the classic and the tournament acc would be preferred.

It's worthy to note that currently, both MAX and 300 (PERFECT and GREAT respectively) contribute towards 100% score for classic acc, whereas in tournaments GREAT judgements count less than 100% acc, though not too harshly. pp calculation was recently adjusted and is based on tournament accuracy rather than score, though in the client we see the classic accuracy. I'd definitely prefer seeing both kinds of accuracy calculations with a toggle, and showing the respective rank (A, S, SS) based on the currently displayed acc. For example:

Classic 100% SS Tournament 99.66% S

This also makes it possible to show an A instead of S in certain edge cases close to the boundaries of the ranks, if too many GREATs are scored.

bdach commented 9 months ago

Toggle is 200% not happening and toggle-dependent ranks are 200,000,000% not happening. We are dealing with enough complexity as is. This is non-negotiable.

This does not only impact client. It impacts web, it impacts user statistics (counts of ranks achieved, etc.). Your proposal is unsustainable.

Rekkonnect commented 9 months ago

From what I can tell, we want a singular rank for each play, to which I stand by keeping the current stable. SS is pretty hard in itself, and S is too common at higher stars. I think that ultimately this system might need a greater revamp, but at the end of the day it's decoratory. The highest the rank has is on the profile and medals afaic.

Airkek commented 8 months ago

Will 95.00% 'S' play with old scoring become 94.xx% 'A' with new one? SM5/Simply Love theme (theme for ITG players with it's own scoring for groovestats) have two modes: first counts 'Fantastic' and 'Excellent' as 100% when second counts only 'Fantastic' as 100%. In any way groovestats accepts only first mode, where 'Fantastic' equals to 'Excellent'. So it would be nice if osu!mania will count 'perfect' and 'great' as same accuracy-point (as in stable)

bdach commented 8 months ago

Will 95.00% 'S' play with old scoring become 94.xx% 'A' with new one?

Possible. We'll see what the community feedback is.

So it would be nice if osu!mania will count 'perfect' and 'great' as same accuracy-point (as in stable)

I am completely unable to parse the word salad in the first half of that sentence but if you are suggesting having two accuracy systems functioning in parallel behind some toggle or whatever - that is not happening.

Airkek commented 8 months ago

if you are suggesting having two accuracy systems functioning in parallel behind some toggle or whatever

im not suggesting this

peppy commented 8 months ago

This has since been changed in line with what is asked for in this discussion. Closing for now.

bdach commented 8 months ago

This has since been changed in line with what is asked for in this discussion

Has it? Where? The title of the issue continues to be accurate as far as I'm aware?

If the decision here is "we're not fixing this", then fine, but that's not what the above says.

peppy commented 8 months ago

Oh, we undid the fix didn't we...

I don't see much point in keeping this issue open regardless. Let's wait for discussions or hopefully not.

mcendu commented 4 months ago

I am starting a poll in G&R.

bdach commented 1 month ago

@peppy this keeps popping up, most recently here. Do we want to reconsider mania SS rank again, or should I never bring it up again?

smoogipoo commented 1 month ago

At the point where osu, taiko, and catch all override ScoreProcessor.RankFromScore(), I see no reason why mania couldn't also. Thankfully, the method signature has changed ever so slightly to accommodate this, since this issue was made :)

bdach commented 1 month ago

Thankfully, the method signature has changed ever so slightly to accommodate this, since this issue was made :)

Is also why I mentioned this again.

The biggest pain point in all of this is that we'd have to reverify ranks for all (mania) scores again.

peppy commented 1 month ago

I'm fine with this direction yep.