Addresses discussions such as https://github.com/ppy/osu/discussions/26407
or https://github.com/ppy/osu/discussions/25914 wherein:
- the game would attempt to convert scores for custom rulesets, which
makes no sense, especially so when they're not there,
- the game would also "recalculate" lazer scores, but that was never the
intention or was never supported; the game would just increment the
score version on those but still include them in the converted tally.
Partially addresses https://github.com/ppy/osu/discussions/26416
As pointed out in the discussion thread above, the total score
conversion process for mania was using accuracy directly from the
replay. In mania accuracy is calculated differently in score V1 than in
score V2, which meant that scores coming from stable were treated more
favourably (due to weighting GREAT and PERFECT equally).
To fix, recompute accuracy locally and use that for the accuracy
portion.
Note that this will still not be (and cannot be made) 100% accurate, as
in stable score V2, as well as in lazer, hold notes are *two*
judgements, not one as in stable score V1, meaning that full and correct
score statistics are not available without playing back the replay.
The effects of the change can be previewed on the following spreadsheet:
https://docs.google.com/spreadsheets/d/1wxD4UwLjwcr7n9y5Yq7EN0lgiLBN93kpd4gBnAlG-E0/edit#gid=1711190356
Top 5 changed scores with replays:
| score | master | this PR | replay |
| :------------------------------------------------------------------------------------------------------------------------------- | ------: | ------: | ------: |
| [Outlasted on Uwa!! So Holiday by toby fox [[4K] easy] (0.71\*)](https://osu.ppy.sh/scores/mania/460404716) | 935,917 | 927,269 | 920,579 |
| [ag0 on Emotional Uplifting Orchestral by bradbreeck [[4K] Rocket's Normal] (0.76\*)](https://osu.ppy.sh/scores/mania/453133066) | 921,636 | 913,535 | 875,549 |
| [rlarkgus on Zen Zen Zense by Gom (HoneyWorks) [[5K] Normal] (1.68\*)](https://osu.ppy.sh/scores/mania/458368312) | 934,340 | 926,787 | 918,855 |
| [YuJJun on Harumachi Clover by R3 Music Box [4K Catastrophe] (1.80\*)](https://osu.ppy.sh/scores/mania/548215786) | 918,606 | 911,111 | 885,454 |
| [Fritte on 45-byou by respon feat. Hatsune Miku & Megpoid [[5K] Normal] (1.52\*)](https://osu.ppy.sh/scores/mania/516079410) | 900,024 | 892,569 | 907,456 |
It was sort of assuming that the user can't be anything but online when
opening, thus forcing the status to online via the immediately-run value
change callback.
Closes https://github.com/ppy/osu/issues/26035.
`submitOnFailOrQuit()`, as the name suggests, can be called both when
the player has failed, or when the player screen is being exited from.
Notably, when perfect mod with auto-retry is active, the two happen
almost simultaneously.
This double call exposes a data race in `submitScore()` concerning the
handling of `scoreSubmissionSource`. The race could be experimentally
confirmed by applying the following patch:
diff --git a/osu.Game/Screens/Play/SubmittingPlayer.cs b/osu.Game/Screens/Play/SubmittingPlayer.cs
index 83adf1f960..76dd29bbdb 100644
--- a/osu.Game/Screens/Play/SubmittingPlayer.cs
+++ b/osu.Game/Screens/Play/SubmittingPlayer.cs
@@ -228,6 +228,7 @@ private Task submitScore(Score score)
return Task.CompletedTask;
}
+ Logger.Log($"{nameof(scoreSubmissionSource)} is {(scoreSubmissionSource == null ? "null" : "not null")}");
if (scoreSubmissionSource != null)
return scoreSubmissionSource.Task;
@@ -237,6 +238,7 @@ private Task submitScore(Score score)
Logger.Log($"Beginning score submission (token:{token.Value})...");
+ Logger.Log($"creating new {nameof(scoreSubmissionSource)}");
scoreSubmissionSource = new TaskCompletionSource<bool>();
var request = CreateSubmissionRequest(score, token.Value);
which would result in the following log output:
[runtime] 2024-01-02 09:54:13 [verbose]: scoreSubmissionSource is null
[runtime] 2024-01-02 09:54:13 [verbose]: scoreSubmissionSource is null
[runtime] 2024-01-02 09:54:13 [verbose]: Beginning score submission (token:36780)...
[runtime] 2024-01-02 09:54:13 [verbose]: creating new scoreSubmissionSource
[runtime] 2024-01-02 09:54:13 [verbose]: Beginning score submission (token:36780)...
[runtime] 2024-01-02 09:54:13 [verbose]: creating new scoreSubmissionSource
[network] 2024-01-02 09:54:13 [verbose]: Performing request osu.Game.Online.Solo.SubmitSoloScoreRequest
[network] 2024-01-02 09:54:14 [verbose]: Request to https://dev.ppy.sh/api/v2/beatmaps/869310/solo/scores/36780 successfully completed!
[network] 2024-01-02 09:54:14 [verbose]: SubmitSoloScoreRequest finished with response size of 639 bytes
[network] 2024-01-02 09:54:14 [verbose]: Performing request osu.Game.Online.Solo.SubmitSoloScoreRequest
[runtime] 2024-01-02 09:54:14 [verbose]: Score submission completed! (token:36780 id:20247)
[network] 2024-01-02 09:54:14 [verbose]: Request to https://dev.ppy.sh/api/v2/beatmaps/869310/solo/scores/36780 successfully completed!
[network] 2024-01-02 09:54:14 [verbose]: SubmitSoloScoreRequest finished with response size of 639 bytes
[runtime] 2024-01-02 09:54:14 [error]: An unhandled error has occurred.
[runtime] 2024-01-02 09:54:14 [error]: System.InvalidOperationException: An attempt was made to transition a task to a final state when it had already completed.
[runtime] 2024-01-02 09:54:14 [error]: at osu.Game.Screens.Play.SubmittingPlayer.<>c__DisplayClass30_0.<submitScore>b__0(MultiplayerScore s) in /home/dachb/Documents/opensource/osu/osu.Game/Screens/Play/SubmittingPlayer.cs:line 250
The intention of the submission logic was to only ever create one
`scoreSubmissionSource`, and then reuse this one if a redundant
submission request was made. However, because of the temporal proximity
of fail and quit in this particular case, combined with the fact that
the calls to `submitScore()` are taking place on TPL threads, means that
there is a read-write data race on `scoreSubmissionSource`, wherein the
source can be actually created twice.
This leads to two concurrent score submission requests, which, upon
completion, attempt to transition only _the second_
`scoreSubmissionSource` to a final state (this is because the API
success/failure request callbacks capture `this`, i.e. the entire
`SubmittingPlayer` instance, rather than the `scoreSubmissionSource`
reference specifically).
To fix, ensure correct synchronisation on the read-write critical
section, which should prevent the `scoreSubmissionSource` from being
created multiple times.
Standardised score conversion would return a negative total score for
https://osu.ppy.sh/scores/taiko/182230346.
The underlying reason for this is that the estimation of the accuracy
portion for a score can be above the actual accuracy portion in the
taiko ruleset.
When calculating the maximum accuracy portion achievable,
`TaikoLegacyScoreSimulator` will include the extra 300 points from
a double hit on a strong hit, per strong hit. However, this double hit
is not factored into accuracy.
Both of the aforementioned facts mean that in taiko
maximumLegacyAccuracyScore * score.Accuracy
- which normally in other rulesets can be used pretty reliably as the
exact number of points gained from the accuracy portion - is an
estimate in the case of taiko, and an _upper_ estimate at that,
because it implicitly assumes that the user has also hit
`score.Accuracy` percent of all double hits possible in the beatmap. If
this assumption is not upheld, then the user will have earned _less_
points than that from the accuracy portion, which means that the combo
proportion estimate will go below zero.
It is possible that this has happened on other scores before, but did
not result in the total score going negative as the accuracy portion
gained would have counteracted the effect of that due to being larger in
magnitude than the score loss incurred from the negative combo
portion. In the case of the score in question this was not the case due
to very low accuracy _and_ very low max combo.
Closes https://github.com/ppy/osu/issues/26213.
Reproduction scenario: switch audio track in editor after timeline
loads.
Happens because `beatmap.Value.Track.Length` is 0 immediately after a
track switch, until BASS computes the actual track length on the audio
thread.
Yes this is a hack. No I have no better immediate ideas how to address
this otherwise.
