Change S rank to require no miss #26630
Conversation
peppy
force-pushed
the
s-rank-change
branch
2 times, most recently
from
3a8686d
to
f27ae87
Compare
peppy
force-pushed
the
s-rank-change
branch
2 times, most recently
from
2817351
to
604565d
Compare
Just to make sure nobody tries any "funny" business.
Would be weird to degrade a score due to *no* misses wouldn't it?
|
I've just pushed my migration changes, but also some fixes to the previous code. Of particular note are: cb8ec48 (some extra API safety), 20ed7e1 (bug), fdd499a (bug). Rest is migration code & general cleanup that was permitted by it. It has been lightly tested but not too much. I will do more testing of it tomorrow if I don't wake up to this being merged. |
It does look pretty dumb to be checking if something is `null` *after calling an instance method on it*...
|
20ed7e1 is weird because i could've sworn i fixed and force pushed this very early... |
|
I'm a little confused about this change, particularly this rationale:
I just spent a while trying to find any place where people were formally polled about this, and I suppose the linked comment is basically that, however, the discussion did not end there. #4184 (comment) saw near-unanimous approval for what I interpreted as basing grades solely on accuracy (since FC is already represented in other ways). Precisely this seems to be what was ultimately moved forward with (#4184 (comment)), so I'm not sure where this reversal is coming from. |
User acceptance is the issue. We're staying with what people are used to (from stable) so we can make forward progress. Thanks for understanding. |
|
Thanks for clarifying! I wish I could see a formal vote on this or something like that, but I assume this kind of insight is just gleaned from the nebulous general consensus across different social media? That's unfortunate, but I understand that these things are rarely very clear. |
|
Our goal right now is to get things live and start to get feedback from actually playing the game against existing leaderboards. This change is easier than trying to figure some "FC" marker that people are happy with. Grades can be recalculated at any point, so if there's enough community push we can change them down the road based on more well formed feedback. |
|
That makes sense, thank you for explaining! Can't wait to see lazer become the new face of osu :) |
|
Provisional osu! score spreadsheet with these changes: https://docs.google.com/spreadsheets/d/16FPqye6GWHZDZPAd0zPsRd3JTBxeNdjRF7LiP1EjgAI/edit#gid=364418891 I'm not sure why scores are getting nerfed to the ground. Will require an investigation (on top of fixing the failing tests). |
Co-authored-by: Walavouchey <36758269+Walavouchey@users.noreply.github.com>
This subtle detail was messing with server-side score import flows. Server-side, legacy total score will *already* be in `LegacyTotalScore` from the start, and `TotalScore` will be zero until recomputed via `StandardisedScoreMigrationTools.UpdateFromLegacy()` - so in that context, attempting to move it across is incorrect.
Non-legacy scores *are* subject to upgrades again - albeit it is *rank* upgrades that they are subject to.
Chances are that because we've broken rank API, it would utterly fail for all custom rulesets anyhow.
|
The sheet breakage mentioned above is fixed by cb87d6c. Tested locally using the server-side verify command. I think my migration portion is ready for light review at least, although some further testing is probably warranted. To that end I'll queue up a score calc run now and hopefully it passes given smoogipoo/diffcalc-sheet-generator#1 is now merged... If it passes then I'll queue up the rest just to be sure. osu! should be matching as per above so I'll skip it. !diffcalc |
|
Empty is a good omen... !diffcalc |
|
!diffcalc |
|
Sheets look good. Not sure there is much more testing I can do here. |
| .AsEnumerable() | ||
| // must be done after materialisation, as realm doesn't support | ||
| // filtering on nested property predicates or projection via `.Select()` | ||
| .Where(s => s.Ruleset.IsLegacyRuleset()) |
There was a problem hiding this comment.
Right, I removed this thinking it was checking legacy scores, but turns out this is important I guess.
not-s-rank.mp4 |
As proposed in the original proposal. Got left out somewhere along the way.
CleanShot.2024-01-22.at.12.39.15.mp4