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

Refactor HitResult generation to make ruleset-specific behavior more explicit #231

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

Givikap120
Copy link
Contributor

@Givikap120 Givikap120 commented Nov 15, 2024

Before you had function GenerateHitResults(double accuracy, IBeatmap beatmap, int countMiss, int? countMeh, int? countGood), where you pass countMiss, countMeh and countGood.
So by function signature you may assume that it's using those 3 judgements to generate hitresults.
In reality - osu also used largeTickMisses and sliderTailMisses from class itself, mania used oks and greats from class, when taiko just ignored countMeh because it doesn't exist in taiko. What is quite confusing and unintuitive.

This PR removes those params from basic GenerateHitResults, so it would be intuitive that all values are taken from attributes of the class.
And then this function delegates calculation to ruleset-specific static function that takes correct parameters.

Prerequisite for #232

@Givikap120
Copy link
Contributor Author

This PR, similar to #232, seems to complicate code at no gain. Nothing is wrong with the current state, and imo splitting this up is unnecessary

I explain the gain in detail in PRs description. Please respond more precisely about what is trying to be addressed instead of "PR is bad".

@smoogipoo
Copy link
Contributor

@Givikap120 you're going to need to fix this PR.

@smoogipoo
Copy link
Contributor

Will also await on further refactoring as per https://discord.com/channels/188630481301012481/380598781432823815/1307180578906312724 before merging.

@minisbett
Copy link
Contributor

Will also await on further refactoring as per https://discord.com/channels/188630481301012481/380598781432823815/1307180578906312724 before merging.

This PR is not the one requiring refactoring I believe, this PR should be fine as-is. The accuracy calculation PR does though.

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