Replace usages of Mod.ScoreMultiplier with new score multiplier API#37845
Replace usages of Mod.ScoreMultiplier with new score multiplier API#37845bdach wants to merge 6 commits into
Mod.ScoreMultiplier with new score multiplier API#37845Conversation
…n via superclass static fields
Revealed by `ScoreMultiplierCalculatorTest` starting to fail due to
interference from `OsuScoreMultiplierCalculator`.
It's not ideal from a performance standpoint but it's the simplest
choice for now. Tricks could be pulled to salvage the static. One is
```csharp
public class ScoreMultiplierCalculator<T>
where T : ScoreMultiplierCalculator<T>
{
}
```
This works because of generics internals; static instance members are
not shared between different specialisations of a generic class. (It
trips a ReSharper inspection too, which would have to be silenced.)
From a performance standpoint this is not ideal, but a significant chunk
of migrated usages already precede the construction of the calculator
via the known-expensive `RulesetInfo.CreateInstance()`, and the paths
that actually construct the calculator do not appear that hot. If need
be, this can be handled by actually caching ruleset instances and their
derivative subcomponents.
|
Just reading the OP alone:
I choose the abandon option. Because I can't deal with any more complexity, and the number of scores people care about that are affected is likely going to be in the hundreds (across the whole userbase). |
|
|
||
| foreach (var mod in score.Mods) | ||
| modMultiplier *= mod.ScoreMultiplier; | ||
| // TODO: When mod multipliers are changed, this is going to break |
There was a problem hiding this comment.
I'm okay with this PR. Did you want to leave this in place or shall we just commit to this being broken and remove the TODO (I'd probably prefer this).
There was a problem hiding this comment.
Do you just want the TODOs removed or both relevant code paths entirely excised?
There was a problem hiding this comment.
I'm not sure leaving the old code would be a good move unless we're going to rename ScoreMultiplier to LegacyScoreMultiplier and leave it in place. Maybe that's a direction we can take and maintain some kind of sanity here? But that would mean all imported scores going forwards would also be treated in this way, which won't (fully) match server side handling of the primary use case (ie where users switch to lazer and embrace the new scoring).
Probably leave in place and just drop the TODO, leave the disclaimer?
There was a problem hiding this comment.
I'm not sure leaving the old code would be a good move unless we're going to rename
ScoreMultipliertoLegacyScoreMultiplierand leave it in place. Maybe that's a direction we can take and maintain some kind of sanity here?
That could be done. It would however mean LegacyScoreMultiplier would essentially need to exist forever.
But that would mean all imported scores going forwards would also be treated in this way, which won't (fully) match server side handling of the primary use case (ie where users switch to lazer and embrace the new scoring).
This statement is a bit disjointed and thus I'm not sure I'm parsing it correctly, but to attempt to clarify once more:
StandardisedScoreMigrationTools.GetOldStandardised() is only relevant to scores pre-June 2023 and probably does nothing of use anymore. It could be deleted pretty safely.
LegacyScoreDecoder.PopulateTotalScoreWithoutMods() is the problematic one. It is relevant to lazer replays only, and only to ones recorded between January and May 2024. That is because since May 2024 TotalScoreWithoutMods is written to the .osr, as part of the JSON-serialised-then-LZMA-compressed LegacyReplaySoloScoreInfo.
Eventually we will need a client-side reprocessing of total scores. For scores post-May 2024, we can use TotalScoreWithoutMods from either realm or the replays because it's there. But what do you do if you import a replay from Feb 2024 that does not have this data? You can
- Keep the old multipliers somehow and therefore correctly backwards-populate the total score without mods, to apply the new correct multipliers back onto that
- Just remove TODOs and attempt to ignore the issue, which will use the old multipliers and back-populate the wrong value of
TotalScoreWithoutMods - Refuse to back-populate at all and leave
TotalScoreWithoutMods = 0. Which will still preserve the score's total score with the old multipliers but will probably not result in more breakage in the future if the multipliers need to be touched ever again.
There was a problem hiding this comment.
Yeah this shows that I am lacking understanding and we've made this way too complicated over the years.
I'll just offer general guidance for now: If this only affects client side scores, it doesn't matter if they don't match 1:1. The number of users on lazer during that period that will have scores that they care about and actually need to compare with newer scores will be low.
That said if there's not much overhead from making the old multipliers work, it may be beneficial in the future to have the original multipliers still in the mods class, who knows. As long as they are clearly marked, they won't be hurting anyone.
There was a problem hiding this comment.
I will say that from a maintenance standpoint the route that simplifies the most would be to disregard the problematic Jan-May 2024 scores.
That means that going forward the forward migration logic for local scores would be "if total score without mods is present, then recalculate total score based on that and mods and correct if mismatching; if total score without mods is present, throw up hands and keep everything as it was".
The cost is that someone might import a score from the affected time period from e.g. the leaderboard and then report an issue to us that its total score is wrong after import.
That said if there's not much overhead from making the old multipliers work, it may be beneficial in the future to have the original multipliers still in the mods class, who knows.
A sketch of the overhead would be:
- Keep the old multipliers in place, probably by renaming the entire class to
Legacyor something and then adding a new one for the new multipliers - Give
Ruleset.CreateScoreMultiplierCalculator()aversionparameter that corresponds toScoreInfo.TotalScoreVersion, defaulting to latest. This would return the correct calculator for the given replay / score version. - Have the replay decode path request the calculator for its original replay version and use that, which would mean the backpopulation path can function correctly.
A possible supplementary fix that could be considered is a one-shot server-side fix that would work by rewriting the affected replays to contain TotalScoreWithoutMods. I believe that should be doable because nothing should be using hashes of .osrs as far as I know.
It wouldn't fix users' local replays, but it would address the "score downloaded from leaderboard has different total" issue.
During review, I would like to direct particular attention to the following changes:
Migrate usages that will break in the future without interventions (with disclaimers)
When migrating usages across I found two that would have been silently broken if the mod multipliers were allowed to be changed in place as attempted in #37355.
The less relevant one of them is
StandardisedScoreMigrationTools.GetOldStandardised(). That code path was added in #23892 to switch away from the original standardised scoring. In particular it predates the release of pp and unified leaderboards, and so all scores that would need to be migrated by it will only exist in long-standing lazer users' local databases. It additionally has likely been essentially made dead since due to multipliers being changed since, as well. I advise removal of this one.The more relevant site is
LegacyScoreDecoder.PopulateTotalScoreWithoutMods(). As a reminder, lazer scores started submitting to unified leaderboards and granting pp in the 2024.130.2 release.ScoreInfo.TotalScoreWithoutModswas added, and started getting encoded to replays, in the 2024.518.0 release. That means there are four months' worth of lazer replays wherein the total score without mods will be missing from replays.The possible avenues I can see here are:
I would like @ppy/team-client review comments on this in particular. cc @tsunyoku for when you are drafting up the PR to introduce the new score multipliers.
Migrate song select to new score multiplier API
This was a confusing change to write because of the way song selects hook their mod overlays up to global bindables. In particular different things happen in different circumstances.
SongSelect.CreateModOverlay(), which is called by the baseSongSelect, the mod overlay is automatically bound to global bindables viaSongSelect.on{ArrivingAt,Leaving}Screen().SongSelect, manual hook-up is required.Fix score multiplier registrations being shared between implementations via superclass static fields
Revealed by
ScoreMultiplierCalculatorTeststarting to fail due to interference fromOsuScoreMultiplierCalculator.It's not ideal from a performance standpoint but it's the simplest choice for now. Tricks could be pulled to salvage the static. One is
This works because of generics internals; static instance members are not shared between different specialisations of a generic class. It is also very unintuitive, so I would rather not. (It trips a ReSharper inspection too, which would have to be silenced.)
From a performance standpoint this is not ideal, but a significant chunk of migrated usages already precede the construction of the calculator via the known-expensive
RulesetInfo.CreateInstance(), and the paths that actually construct the calculator do not appear to be that hot. If need be, this can be handled by actually caching ruleset instances and their derivative subcomponents.