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

Account for CL mod when generating HitResults #232

Merged
merged 3 commits into from
Nov 18, 2024

Conversation

Givikap120
Copy link
Contributor

@Givikap120 Givikap120 commented Nov 15, 2024

Depends on #231

Adds mods parameter GenerateHitResults so you can generate correct hitresults for CL scores.

@minisbett
Copy link
Contributor

This PR seems to make things more complicated at no gain. The accuracy calculation already works fine with CL and non-CL, even when exclusively generating non-CL hit statistics and using the Lazer accuracy formula.

@Givikap120
Copy link
Contributor Author

This PR seems to make things more complicated at no gain. The accuracy calculation already works fine with CL and non-CL, even when exclusively generating non-CL hit statistics and using the Lazer accuracy formula.

It's literally incorrect for CL scores

@minisbett
Copy link
Contributor

minisbett commented Nov 15, 2024

Oh I think you're right. Can you maybe close #231? I think it makes more sense to have all of this in a single PR, easier to review

@Givikap120
Copy link
Contributor Author

Oh I think you're right. Can you maybe close #231? I think it makes more sense to have all of this in a single PR, easier to review

I split it exactly for reason of being easier to review.
Because adding mods param to the function without refactor makes confusing way of calculating hitresults even more confusing.

@minisbett
Copy link
Contributor

Oh I think you're right. Can you maybe close #231? I think it makes more sense to have all of this in a single PR, easier to review

I split it exactly for reason of being easier to review. Because adding mods param to the function without refactor makes confusing way of calculating hitresults even more confusing.

ok yeah fair, after looking at the code for longer maybe

@smoogipoo smoogipoo merged commit 80e8017 into ppy:master Nov 18, 2024
3 checks passed
@smoogipoo
Copy link
Contributor

Ah, I didn't notice the dependency here...

@minisbett
Copy link
Contributor

Wasn't it discussed in #difficulty-osu that the complicated accuracy calculation here can be simplified here? https://discord.com/channels/188630481301012481/380598781432823815/1307180578906312724

@smoogipoo
Copy link
Contributor

Yeah, use that 👍

@minisbett
Copy link
Contributor

So maybe revert this PR until #231 is merged and a more proper accuracy calculation implemented?

smoogipoo added a commit that referenced this pull request Nov 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants