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.

Sign up for GitHub

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

Reset mod multiplier to 1 for classic mod in rulesets other than osu! #27816

Closed
wants to merge 4 commits into from
+217 −7
Closed

Reset mod multiplier to 1 for classic mod in rulesets other than osu! #27816

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
3d3e05d
Update mod multiplier for classic mod in rulesets other than osu!
bdach Apr 9, 2024
bc14094
Add background reprocessing job for mod multipliers
bdach Apr 9, 2024
83beba2
Add failing tests for multiple migrations not applying at once
bdach Apr 12, 2024
004eaaf
Fix rank upgrade job preventing mod multiplier updates
bdach Apr 12, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions osu.Game.Rulesets.Osu/Mods/OsuModClassic.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,8 @@ namespace osu.Game.Rulesets.Osu.Mods
{
public class OsuModClassic : ModClassic, IApplicableToHitObject, IApplicableToDrawableHitObject, IApplicableToDrawableRuleset<OsuHitObject>, IApplicableHealthProcessor
{
public override double ScoreMultiplier => 0.96;

public override Type[] IncompatibleMods => base.IncompatibleMods.Append(typeof(OsuModStrictTracking)).ToArray();

[SettingSource("No slider head accuracy requirement", "Scores sliders proportionally to the number of ticks hit.")]
Expand Down
30 changes: 30 additions & 0 deletions osu.Game.Tests/Database/BackgroundDataStoreProcessorTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
using osu.Framework.Testing;
using osu.Game.Beatmaps;
using osu.Game.Database;
using osu.Game.Online.API;
using osu.Game.Rulesets;
using osu.Game.Scoring;
using osu.Game.Scoring.Legacy;
Expand Down Expand Up @@ -211,6 +212,35 @@ public void TestCustomRulesetScoreNotSubjectToUpgrades([Values] bool available)
AddAssert("Score version not upgraded", () => Realm.Run(r => r.Find<ScoreInfo>(scoreInfo.ID)!.TotalScoreVersion), () => Is.EqualTo(30000001));
}

[Test]
public void TestClassicModMultiplierChange()
{
AddStep("Add scores", () =>
{
string[] rulesets = ["osu", "taiko", "fruits", "mania"];

foreach (string ruleset in rulesets)
{
Realm.Write(r =>
{
r.Add(new ScoreInfo(ruleset: r.Find<RulesetInfo>(ruleset), beatmap: r.All<BeatmapInfo>().First())
{
TotalScoreVersion = 30000016,
TotalScore = 960_000,
APIMods = [new APIMod { Acronym = "CL" }]
});
});
}
});

AddStep("Run background processor", () => Add(new TestBackgroundDataStoreProcessor()));

AddUntilStep("Three scores updated", () => Realm.Run(r => r.All<ScoreInfo>().Count(score => score.TotalScore == 1_000_000)), () => Is.EqualTo(3));
AddUntilStep("osu! score preserved", () => Realm.Run(r => r.All<ScoreInfo>().Count(score => score.TotalScore == 960_000)), () => Is.EqualTo(1));
AddAssert("No fails", () => Realm.Run(r => r.All<ScoreInfo>().Count(score => score.BackgroundReprocessingFailed)), () => Is.Zero);
AddAssert("All score versions upgraded", () => Realm.Run(r => r.All<ScoreInfo>().Count(score => score.TotalScoreVersion >= 30000017)), () => Is.EqualTo(4));
}

public partial class TestBackgroundDataStoreProcessor : BackgroundDataStoreProcessor
{
protected override int TimeToSleepDuringGameplay => 10;
Expand Down
92 changes: 91 additions & 1 deletion osu.Game/Database/BackgroundDataStoreProcessor.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,6 +74,7 @@ protected override void LoadComplete()
processScoresWithMissingStatistics();
convertLegacyTotalScoreToStandardised();
upgradeScoreRanks();
upgradeModMultipliers();
}, TaskCreationOptions.LongRunning).ContinueWith(t =>
{
if (t.Exception?.InnerException is ObjectDisposedException)
Expand Down Expand Up @@ -322,7 +323,7 @@ private void convertLegacyTotalScoreToStandardised()
.Where(s => !s.BackgroundReprocessingFailed
&& s.BeatmapInfo != null
&& s.IsLegacyScore
&& s.TotalScoreVersion < LegacyScoreEncoder.LATEST_VERSION)
&& s.TotalScoreVersion < 30000016) // last total score version associated with changes to the score estimation algorithm
.AsEnumerable()
// must be done after materialisation, as realm doesn't want to support
// nested property predicates
Expand Down Expand Up @@ -436,6 +437,95 @@ private void upgradeScoreRanks()
completeNotification(notification, processedCount, scoreIds.Count, failedCount);
}

private record ModMultiplierChange(
string ModAcronym,
string RulesetName,
double OldMultiplier,
double NewMultiplier,
int ScoreVersion);

private static readonly ModMultiplierChange[] mod_multiplier_changes =
[
new ModMultiplierChange("CL", "taiko", 0.96, 1, 30000017),
new ModMultiplierChange("CL", "fruits", 0.96, 1, 30000017),
new ModMultiplierChange("CL", "mania", 0.96, 1, 30000017),
];

private void upgradeModMultipliers()
{
Logger.Log("Performing mod multiplier upgrades...");

int latestMultiplierChange = mod_multiplier_changes.Max(change => change.ScoreVersion);

var scores = realmAccess
.Run(r => r.All<ScoreInfo>()
.Where(score => score.TotalScoreVersion < latestMultiplierChange && !score.BackgroundReprocessingFailed)
.Detach())
// must be done after materialisation, as realm doesn't support
// filtering on nested property predicates or projection via `.Select()`
.Where(s => s.Ruleset.IsLegacyRuleset())
.ToList();

if (scores.Count == 0)
return;

var notification = showProgressNotification(scores.Count, "Adjusting mod multipliers for scores", "scores now use latest mod multipliers");

int processedCount = 0;
int failedCount = 0;

foreach (var score in scores)
{
if (notification?.State == ProgressNotificationState.Cancelled)
break;

updateNotificationProgress(notification, processedCount, scores.Count);

sleepIfRequired();

double? newTotalScore = null;

try
{
foreach (var multiplierChange in mod_multiplier_changes)
{
if (score.TotalScoreVersion >= multiplierChange.ScoreVersion
|| score.Ruleset.ShortName != multiplierChange.RulesetName
|| score.APIMods.All(m => m.Acronym != multiplierChange.ModAcronym))
{
continue;
}

newTotalScore ??= score.TotalScore;
newTotalScore /= multiplierChange.OldMultiplier;
newTotalScore *= multiplierChange.NewMultiplier;
}

realmAccess.Write(r =>
{
var refetched = r.Find<ScoreInfo>(score.ID)!;
if (newTotalScore != null)
refetched.TotalScore = (long)newTotalScore.Value;
refetched.TotalScoreVersion = LegacyScoreEncoder.LATEST_VERSION;
Copy link
Member

Choose a reason for hiding this comment

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

We're doing this in three different methods in this processor class now, alongside local queries for current version. This means if two migrations need to happen, only the first to execute will actually get run, unless I'm missing something.

May need to re-think how these are being applied. Either applying all total score changes in one method, or updating the TotalScoreVersion as a final step instead of locally?

Copy link
Member

Choose a reason for hiding this comment

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

Another option would be changing the version value to be incremented once per change (similar to realm versioning) and specifically apply migrations based on the imminent version number (then incrementing by one on application), but that's a bit of an annoying structural/logic change to make initially.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

You're correct but this is also very annoying to handle properly.

I tried 004eaaf I guess, but also that commit is unsafe because if you happen to kill the game when it's updating the total score versions, it'll apply the mod multiplier change twice.

The big problem here is that the migration is not idempotent, i.e. there is unrecoverable data loss in a way. This is yet another argument to store the raw score pre-multipliers again I guess.

I dunno. I'd probably personally just close this PR and re-do from scratch properly start-to-end with storing the pre-multiplier value to client db and to server db, but you seem insistent on pushing this out so if you have better ideas then feel free to take a shot.

});

++processedCount;
}
catch (ObjectDisposedException)
{
throw;
}
catch (Exception e)
{
Logger.Log($"Failed to update rank score {score.ID}: {e}");
realmAccess.Write(r => r.Find<ScoreInfo>(score.ID)!.BackgroundReprocessingFailed = true);
++failedCount;
}
}

completeNotification(notification, processedCount, scores.Count, failedCount);
}

private void updateNotificationProgress(ProgressNotification? notification, int processedCount, int totalCount)
{
if (notification == null)
Expand Down
2 changes: 1 addition & 1 deletion osu.Game/Rulesets/Mods/ModClassic.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ public abstract class ModClassic : Mod

public override string Acronym => "CL";

public override double ScoreMultiplier => 0.96;
public override double ScoreMultiplier => 1;

public override IconUsage? Icon => FontAwesome.Solid.History;

Expand Down
3 changes: 2 additions & 1 deletion osu.Game/Scoring/Legacy/LegacyScoreEncoder.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,9 +47,10 @@ public class LegacyScoreEncoder
/// <item><description>30000014: Fix edge cases in conversion for osu! scores on selected beatmaps. Reconvert all scores.</description></item>
/// <item><description>30000015: Fix osu! standardised score estimation algorithm violating basic invariants. Reconvert all scores.</description></item>
/// <item><description>30000016: Fix taiko standardised score estimation algorithm not including swell tick score gain into bonus portion. Reconvert all scores.</description></item>
/// <item><description>30000017: Change multiplier of classic mod from 0.96x to 1.00x on all rulesets except osu!. Reconvert all relevant scores.</description></item>
/// </list>
/// </remarks>
public const int LATEST_VERSION = 30000016;
public const int LATEST_VERSION = 30000017;

/// <summary>
/// The first stable-compatible YYYYMMDD format version given to lazer usage of replays.
Expand Down
Loading