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 imported osu!stable scores being scaled by the classic scoring mode #23817

Merged
merged 7 commits into from
Jun 9, 2023

Conversation

smoogipoo
Copy link
Contributor

Temporary fix for #23782 / #23756, preventing stable scores going into the billions when using the classic scoring mode.

It does NOT fix old lazer scores (which likely won't/can't be fixed), and does NOT convert SV1 to SV2 yet.

@peppy peppy self-requested a review June 9, 2023 08:25
if (replayFilename == null)
continue;

using (var stream = files.Store.GetStream(replayFilename))
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think that this should probably be under the try-catch too, to catch cases like possible missing files.

Copy link
Member

Choose a reason for hiding this comment

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

Have expanded the scope slightly. Apart from that, I think this is okay to go with. As I mentioned on discord, I don't think it's going to be a huge issue that this takes some time to run. It should be in the realm of seconds, and the startup time is already pretty long so I think users will be willing to wait.

Copy link
Contributor Author

@smoogipoo smoogipoo Jun 9, 2023

Choose a reason for hiding this comment

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

Shouldn't that return the null stream? If not this function isn't properly documented.

Copy link
Member

@peppy peppy Jun 9, 2023

Choose a reason for hiding this comment

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

Not sure what kind of documentation you're expecting, but the underlying NativeStorage doesn't do much apart from pass through stream functions from .NET, so best to play it safe:

https://github.com/ppy/osu-framework/blob/bd4ac6890a1ac7b2ec492b80975a0a2e61d9fcb0/osu.Framework/Platform/NativeStorage.cs#L88-L111

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It needs to document that it both may return null if the path doesn't exist (also needs to be nullable annotated but that's a given), which is the case that @bdach was mentioning, and also use <exception> tags to describe that it also throws via the native calls.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Okay well I may have used a bad example. Non-existent files may not be an issue with this method but after seeing the number of dumb errors on sentry that are related to people's weird and corrupt storages I just feel like it's way safer to guard everything defensively.

peppy
peppy previously approved these changes Jun 9, 2023
@peppy peppy requested a review from bdach June 9, 2023 08:38
Copy link
Collaborator

@bdach bdach left a comment

Choose a reason for hiding this comment

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

Let's see how this pans out

@bdach bdach enabled auto-merge June 9, 2023 09:25
@peppy peppy disabled auto-merge June 9, 2023 09:47
@peppy peppy merged commit 631090d into ppy:master Jun 9, 2023
@peppy peppy self-requested a review June 9, 2023 16:47
@smoogipoo smoogipoo deleted the scoring-hotfix branch September 11, 2023 02:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants