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 TaikoDifficultyCalculator and add DifficultStrain attributes #31191

Merged
merged 7 commits into from
Dec 19, 2024

Conversation

Lawtrohux
Copy link
Member

@Lawtrohux Lawtrohux commented Dec 19, 2024

First split out of the major PR. Includes

  • Refactor HitObject creation, cleaner list as more things will be included
  • Introduce CountDifficultStrains as an attribute
  • Move p_norm into DifficultyCalculationUtils

@Lawtrohux Lawtrohux requested a review from a team December 19, 2024 03:59
Copy link

@YaniFR YaniFR left a comment

Choose a reason for hiding this comment

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

lgtm, acc scaling shift was approved by the community on huismetbenen prior

@bdach
Copy link
Collaborator

bdach commented Dec 19, 2024

Why are refactors being bundled with actual changes? Surely refactoring should be split out separately out of any actual adjustments to prevent undesirable changes?

@Lawtrohux
Copy link
Member Author

Why are refactors being bundled with actual changes? Surely refactoring should be split out separately out of any actual adjustments to prevent undesirable changes?

Specifically because in the bundling of pr's we had a separate smoogisheet created purely for the refactor values, as well as a huismet link, and because I didn't want to create a pr solely for increasing the accuracy scale shift to 400, which was it's previous value before the current deploy to provide a good base for future changes.

@bdach
Copy link
Collaborator

bdach commented Dec 19, 2024

I'm going to pretend I saw nothing and trust in your judgement. Please do not disappoint.

@tsunyoku
Copy link
Member

Similar to bdach's comments, I'm a little bit apprehensive about calling this a "refactor" PR when we're refactoring as well as removing existing penalties, adding new difficulty attributes and changing accuracy scaling. I'd be much happier if we could split this into 3:

  • Refactors (anything which doesn't change difficulty/pp values)
  • Accuracy scaling change
  • Convert penalty change

It might seem overkill but especially given the result of last deploy it'd be nice if we could easily pull these things out where necessary. It would also allow for sheets with isolated diff so we can find any points of failure more easily.

@Lawtrohux Lawtrohux changed the title Refactor taikodifficultycalculator and increase accuracy scaling Refactor TaikoDifficultyCalculator and add DifficultStrain attributes Dec 19, 2024
@tsunyoku tsunyoku self-requested a review December 19, 2024 10:20
@tsunyoku tsunyoku enabled auto-merge (squash) December 19, 2024 11:07
@tsunyoku tsunyoku merged commit 4ca88ae into ppy:pp-dev Dec 19, 2024
8 checks passed
@Lawtrohux Lawtrohux deleted the sr-refactor branch December 19, 2024 14:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Pending Deploy
Development

Successfully merging this pull request may close these issues.

4 participants