Add difficulty attributes to facilitate conversion from legacy score, and convert existing scores #24072
Conversation
Firstly, this is intended to be a float division. Secondly, dividing integers by 0 results in an exception, but dividing non-zero floats by 0 results in +/- infinity which will be clamped to the upper range. In particular, this occurs when the beatmap has 1 hitobject (0 drain length).
osu.Game/Scoring/ScoreInfo.cs
Outdated
| /// <remarks> | ||
| /// This may not match the version stored in the replay files. | ||
| /// </remarks> | ||
| public int Version { get; set; } = LegacyScoreEncoder.LATEST_VERSION; |
There was a problem hiding this comment.
So my main question is... what is this versioning, exactly? It's explicitly not the replay version (and I'm not asking why not, as per OP - the reasons are not given, but I'm sure you have them and they are good), however it's still using the replay versioning scheme. Is this versioning the scoring algorithm itself?
If yes, can we name this ScoreVersion or ScoringVersion or similar explicitly? Because I am 99% confident that with the Version name and the shared versioning scheme someone is going to mistake this for replay version and be very sad later. The prefix will hopefully make stop and think.
There was a problem hiding this comment.
Would just renaming to ScoreVersion be enough for you? I don't really know how to explain it better than I did in the xmldoc, it's intended to represent the version of TotalScore.
There was a problem hiding this comment.
Okay, so it is the versioning of the scoring algorithm itself, and as such I'd be totally fine with ScoreVersion or TotalScoreVersion as long as you are.
| if (score.ScoreInfo.IsLegacyScore) | ||
| score.ScoreInfo.LegacyTotalScore = score.ScoreInfo.TotalScore; |
There was a problem hiding this comment.
I kinda think this should be in ScoreImporter.Populate()? As in
diff --git a/osu.Game/Scoring/Legacy/LegacyScoreDecoder.cs b/osu.Game/Scoring/Legacy/LegacyScoreDecoder.cs
index bf592d5988..c6461840aa 100644
--- a/osu.Game/Scoring/Legacy/LegacyScoreDecoder.cs
+++ b/osu.Game/Scoring/Legacy/LegacyScoreDecoder.cs
@@ -123,9 +123,6 @@ public Score Parse(Stream stream)
PopulateAccuracy(score.ScoreInfo);
- if (score.ScoreInfo.IsLegacyScore)
- score.ScoreInfo.LegacyTotalScore = score.ScoreInfo.TotalScore;
-
// before returning for database import, we must restore the database-sourced BeatmapInfo.
// if not, the clone operation in GetPlayableBeatmap will cause a dereference and subsequent database exception.
score.ScoreInfo.BeatmapInfo = workingBeatmap.BeatmapInfo;
diff --git a/osu.Game/Scoring/ScoreImporter.cs b/osu.Game/Scoring/ScoreImporter.cs
index eb57f9a560..5ada2a410d 100644
--- a/osu.Game/Scoring/ScoreImporter.cs
+++ b/osu.Game/Scoring/ScoreImporter.cs
@@ -89,7 +89,10 @@ protected override void Populate(ScoreInfo model, ArchiveReader? archive, Realm
if (StandardisedScoreMigrationTools.ShouldMigrateToNewStandardised(model))
model.TotalScore = StandardisedScoreMigrationTools.GetNewStandardised(model);
else if (model.IsLegacyScore)
+ {
+ model.LegacyTotalScore = model.TotalScore;
model.TotalScore = StandardisedScoreMigrationTools.ConvertFromLegacyTotalScore(model, beatmaps());
+ }
}
/// <summary>
Keeps that stuff in one place rather than smeared across. Unless this needs to be strictly here?
There was a problem hiding this comment.
I'm not fully sold on this, reason being there could be an external consumer of LegacyScoreDecoder that would have to be aware of it not setting LegacyTotalScore, but I've applied the change.
There was a problem hiding this comment.
reason being there could be an external consumer of
LegacyScoreDecoderthat would have to be aware of it not settingLegacyTotalScore
I don't really find that argument overly convincing given that the initial way this was written was doing things half-and-half anyway. Like sure, LegacyTotalScore would be set for decoded scores with that initial code, but the decoder didn't do score conversion anyway, so it was just a second place to get the same (probably wrong) data as TotalScore would be post-decode sans-population.
osu.Game.Rulesets.Mania/Difficulty/ManiaDifficultyCalculator.cs
Outdated
Show resolved
Hide resolved
| model.LegacyTotalScore = model.TotalScore; | ||
| model.TotalScore = StandardisedScoreMigrationTools.ConvertFromLegacyTotalScore(model, beatmaps()); |
There was a problem hiding this comment.
Now that I look at this again - shouldn't this be setting model.Version (or, model.ScoreVersion, if that rename happens, I guess) to LegacyScoreEncoder.LATEST_VERSION, too? And probably unconditionally, while we're at it.
There's possibly an argument here that it might be something that the StandardisedScoreMigration classes should be doing themselves, except for the fact that right now they are mostly pure methods, and if you had them set the version, they would cease to be pure. Which is also a problem when you consider that a willy-nilly write to an attached realm model instance may throw if done outside of a transaction.
There was a problem hiding this comment.
Okay never mind I guess this will happen anyway because of the default value applied to ScoreInfo.Version? Which feels... risky in the future (what if we forget to check after a scoring change N months down the line and accidentally assign a version that is too high for the score? e.g. when reimporting lazer scores?), but works for now, I guess.
| @@ -27,7 +27,6 @@ public override IEnumerable<(int attributeId, object value)> ToDatabaseAttribute | |||
| // Todo: osu!catch should not output star rating in the 'aim' attribute. | |||
| yield return (ATTRIB_ID_AIM, StarRating); | |||
| yield return (ATTRIB_ID_APPROACH_RATE, ApproachRate); | |||
| yield return (ATTRIB_ID_MAX_COMBO, MaxCombo); | |||
There was a problem hiding this comment.
This attribute hasn't been removed, just moved from each ruleset down to the base DifficultyAttributes class.
The reasoning for why it was this way is that, historically, some rulesets (e.g. mania) didn't store max combo. This has changed over the years and we now store max combo for all rulesets, but this code hadn't been adjusted in line with that.
osu.Game/Database/RealmAccess.cs
Outdated
| // Set a sane default while background processing runs. | ||
| score.LegacyTotalScore = score.TotalScore; |
There was a problem hiding this comment.
As a note, it's not a "default" - it's required.
|
Let's move forward, and get this into dev hands for local testing. |
Resolves #23756
LegacyAccuracyScore,LegacyComboScore, andLegacyBonusScoreRatio. These are used byosu-queue-score-statisticsto import from the high score tables.ScoreInfo.LegacyTotalScore. This is something we have inSoloScoreInfotoo, which is stored server side. Its primary purpose is to not mangleScoreInfo.TotalScorefor if we have to re-convert scores again in the future (makes the process a bit simpler).ScoreInfo.Version. This doesn't track the version that's stored in the replay file, but indicates whether a score is out-of-date. Whether the "version" stored in the replay file should track this value is something to be considered for another day because it has wider-reaching implications (e.g. exporting from lazer and importing into stable).BackgroundBeatmapProcessorthat converts fromScoreInfo.LegacyTotalScore.Some sample leaderboards can be found here (of top-1000 users only): https://docs.google.com/spreadsheets/d/1Ogpfa5PhMNtKOYwrxt7n1zAQ5w_YVzkm79pC8tB7hpU/edit#gid=0
On my PC, this seems to process at around 30-50 scores per second, and does not process during gameplay:
2023-06-28.16-00-35.mp4