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.

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

Fix incorrect score conversion on selected beatmaps due to incorrect difficultyPeppyStars rounding #26471

Merged
merged 1 commit into from
Jan 12, 2024

Conversation

bdach
Copy link
Collaborator

@bdach bdach commented Jan 10, 2024

Fixes issue that occurs on about 245 beatmaps1 and was first described by me on discord and then rediscovered again during work on #26405.

Explanation

It so happens that in stable, due to .NET Framework internals, float math would be performed using x87 registers and opcodes. .NET (Core) however uses SSE instructions on 32- and 64-bit words. x87 registers are 80 bits wide. Which is notably wider than both float and double. Therefore, on a significant number of beatmaps, the final rounding of difficultyPeppyStars would not produce correct values due to insufficient precision. See following gist for corroboration of the above (compare dump of JIT asm calculating the value for lazer and stable).

Thus, to crudely - but, seemingly mostly accurately, after checking across all ranked maps - emulate this, use decimal, which is slow, but has bigger precision than double. The single known exception beatmap in whose case this results in an incorrect result is https://osu.ppy.sh/beatmapsets/1156087#osu/2625853, which is considered an "acceptable casualty" of sorts (read: after spending a whole full day of dev on this, I do not consider a single beatmap's stable scores getting buffed by 10% significant enough to matter).

Doing this requires some fooling of the compiler / runtime (see second inline comment in new method). To corroborate that this is required, you can try the following code snippet:

Console.WriteLine(string.Join(' ', BitConverter.GetBytes(1.3f).Select(x => x.ToString("X2"))));
Console.WriteLine(string.Join(' ', BitConverter.GetBytes(1.3).Select(x => x.ToString("X2"))));
Console.WriteLine();

decimal d1 = (decimal)1.3f;
decimal d2 = (decimal)1.3;
decimal d3 = (decimal)(double)1.3f;

Console.WriteLine(string.Join(' ', decimal.GetBits(d1).SelectMany(BitConverter.GetBytes).Select(x => x.ToString("X2"))));
Console.WriteLine(string.Join(' ', decimal.GetBits(d2).SelectMany(BitConverter.GetBytes).Select(x => x.ToString("X2"))));
Console.WriteLine(string.Join(' ', decimal.GetBits(d3).SelectMany(BitConverter.GetBytes).Select(x => x.ToString("X2"))));

which will print

66 66 A6 3F
CD CC CC CC CC CC F4 3F

0D 00 00 00 00 00 00 00 00 00 00 00 00 00 01 00
0D 00 00 00 00 00 00 00 00 00 00 00 00 00 01 00
8C 5D 89 FB 3B 76 00 00 00 00 00 00 00 00 0E 00

Note that despite d1 being converted from a less-precise floating-point value than d2, it still is represented 100% accurately as a decimal number.

Yes, this is a hack. Yes, it doesn't work completely. I think it's better to start with this rather than consider the alternatives, though (which is having something hard-stuck on windows+x86 just to compute this one value for correct score conversion until stable can't submit scores anymore, i.e. forevermore).

Implications

A catch sheet showing the effects of this change is available for preview here, although this really impacts all rulesets. Some sampling of scores from there shows the change having the desired effect:

  • Most gains, except for the one known-broken beatmap mentioned above (https://osu.ppy.sh/beatmapsets/1156087#osu/2625853), are happening because the combo score was previously overestimated, thus capping the following expression

    double bonusProportion = Math.Max(0, ((long)score.LegacyTotalScore - maximumLegacyBaseScore) * maximumLegacyBonusRatio);

    to 0, and thus underestimating the bonus portion.

  • The losses are 200% expected and what this PR is primarily intending to fix (scores way in excess of 1M).

Warning

Note that after applying this change, recomputation of legacy scoring attributes for all rulesets will be required for the change to take the desired effect.

Footnotes

  1. An approximate list of affected maps can be found here. The list is approximate because the source of data used to compare computation of difficultyPeppyStars between .NET Framework and .NET (Core) was a data dump provided by @peppy, which unfortunately was not 100% accurate due to drainTime sometimes being off by one.

…`difficultyPeppyStars` rounding

Fixes issue that occurs on *about* 246 beatmaps and was first described
by me on discord:

    https://discord.com/channels/188630481301012481/188630652340404224/1154367700378865715

and then rediscovered again during work on
ppy#26405:

    https://gist.github.com/bdach/414d5289f65b0399fa8f9732245a4f7c#venenog-on-ultmate-end-by-blacky-overdose-631

It so happens that in stable, due to .NET Framework internals, float
math would be performed using x87 registers and opcodes.
.NET (Core) however uses SSE instructions on 32- and 64-bit words.
x87 registers are _80 bits_ wide. Which is notably wider than _both_
float and double. Therefore, on a significant number of beatmaps,
the rounding would not produce correct values due to insufficient
precision.

See following gist for corroboration of the above:

    https://gist.github.com/bdach/dcde58d5a3607b0408faa3aa2b67bf10

Thus, to crudely - but, seemingly accurately, after checking across
all ranked maps - emulate this, use `decimal`, which is slow, but has
bigger precision than `double`. The single known exception beatmap
in whose case this results in an incorrect result is

    https://osu.ppy.sh/beatmapsets/1156087#osu/2625853

which is considered an "acceptable casualty" of sorts.

Doing this requires some fooling of the compiler / runtime (see second
inline comment in new method). To corroborate that this is required,
you can try the following code snippet:

    Console.WriteLine(string.Join(' ', BitConverter.GetBytes(1.3f).Select(x => x.ToString("X2"))));
    Console.WriteLine(string.Join(' ', BitConverter.GetBytes(1.3).Select(x => x.ToString("X2"))));
    Console.WriteLine();

    decimal d1 = (decimal)1.3f;
    decimal d2 = (decimal)1.3;
    decimal d3 = (decimal)(double)1.3f;

    Console.WriteLine(string.Join(' ', decimal.GetBits(d1).SelectMany(BitConverter.GetBytes).Select(x => x.ToString("X2"))));
    Console.WriteLine(string.Join(' ', decimal.GetBits(d2).SelectMany(BitConverter.GetBytes).Select(x => x.ToString("X2"))));
    Console.WriteLine(string.Join(' ', decimal.GetBits(d3).SelectMany(BitConverter.GetBytes).Select(x => x.ToString("X2"))));

which will print

    66 66 A6 3F
    CD CC CC CC CC CC F4 3F

    0D 00 00 00 00 00 00 00 00 00 00 00 00 00 01 00
    0D 00 00 00 00 00 00 00 00 00 00 00 00 00 01 00
    8C 5D 89 FB 3B 76 00 00 00 00 00 00 00 00 0E 00

Note that despite `d1` being converted from a less-precise floating-
-point value than `d2`, it still is represented 100% accurately as
a decimal number.

After applying this change, recomputation of legacy scoring attributes
for *all* rulesets will be required.
@bdach bdach added area:scoring next release Pull requests which are almost there. We'll aim to get them in the next release, but no guarantees! labels Jan 10, 2024
@bdach bdach requested a review from a team January 10, 2024 18:56
@bdach bdach self-assigned this Jan 10, 2024
@peppy peppy merged commit 02975b9 into ppy:master Jan 12, 2024
13 of 17 checks passed
@bdach bdach deleted the fix-incorrect-difficulty-peppy-stars branch January 13, 2024 09:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:scoring next release Pull requests which are almost there. We'll aim to get them in the next release, but no guarantees! size/M
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

2 participants