Fix Score V1 simulation in scoring test scene incorrectly applying multiplier #24792
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…ltiplier
`TestSceneScoring` included a local simulation of stable's Score V1
algorithm. One of the parts of said algorithm is a mysterious
"score multiplier", influenced by - among others - the beatmap's drain
rate, overall difficulty, circle size, object count, drain length,
and active mods. (An implementation of this already exists in lazer
source, in `OsuLegacyScoreSimulator`, but more on this later.)
However, `TestSceneScoring` had this multiplier in _two_ places, with
_two_ distinct values, one of which being 1 (i.e. basically off).
Unfortunately, the place that had 1 as the multiplier was the wrong one.
Stable calculates the score increase for every hit in two stages;
first, it takes the raw numerical value of the judgement, but then
applies a combo-based bonus on top of it:
scoreIncrease += (int)(Math.Max(0, ComboCounter.HitCombo - 1) * (scoreIncrease / 25 * ScoreMultiplier));
On the face of it, it may appear that the `ScoreMultiplier` factor
can be factored out and applied at the end only when returning total
score. However, once the above formula is rewritten as:
scoreIncrease = scoreIncrease + (int)(Math.Max(0, ComboCounter.HitCombo - 1) * (scoreIncrease / 25 * ScoreMultiplier));
= scoreIncrease * (1 + (Math.Max(0, ComboCounter.HitCombo - 1) / 25 * ScoreMultiplier))
it becomes clear that that assumption is actually _incorrect_,
and the `ScoreMultiplier` _must_ be applied to every score increase
individually.
The above was cross-checked experimentally against stable source
on an example test map with 100 objects, and a replay hitting them
perfectly.
peppy
approved these changes
Sep 13, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
TestSceneScoringincluded a local simulation of stable's Score V1 algorithm. One of the parts of said algorithm is a mysterious "score multiplier", influenced by - among others - the beatmap's drain rate, overall difficulty, circle size, object count, drain length, and active mods. (An implementation of this already exists in lazer source, inOsuLegacyScoreSimulator, but more on this later.)However,
TestSceneScoringhad this multiplier in two places, with two distinct values, one of which being 1 (i.e. basically off). Unfortunately, the place that had 1 as the multiplier was the wrong one.Stable calculates the score increase for every hit in two stages; first, it takes the raw numerical value of the judgement, but then applies a combo-based bonus on top of it:
On the face of it, it may appear that the
ScoreMultiplierfactor can be factored out and applied at the end only when returning total score. However, once the above formula is rewritten as:it becomes clear that that assumption is actually incorrect, and the
ScoreMultipliermust be applied to every score increase individually.This was spurred by a cross-check of
TestSceneScoringfor bugs after this discord message. It's the only issue I found, so not sure what that report was about.Everything above was cross-checked experimentally against stable source on an example test map with 100 objects, and a replay hitting them perfectly.
The following archive contains the aforementioned test beatmap and replay: ScoreV1PerfectTest.zip
To test against stable, apply:
to extract the intermediate score totals. You should get something like the following:
To cross-check this PR against the above, adjust the
score_multiplierin the test scene.For what it's worth, I'm not sure the score V1 simulation code in the test scene is going to live for much longer, as with each passing minute I'm eyeing the
ILegacyScoreSimulators for reuse. But I'm not willing to make a concrete judgement on this yet, it's a one-liner fix, and I already put the time in, so I figure what's the harm?