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

Implement a bunch of rhythm difficulty calculation fixes #28871

Merged
merged 34 commits into from
Oct 7, 2024

Conversation

stanriders
Copy link
Member

@stanriders stanriders commented Jul 15, 2024

This PR addresses a couple of rhythm evaluation issues in osu!:

  1. Removes island length size bonus completely and uncaps island size (was limited to 7 before)
  2. Heavily nerf repeated islands such as triples in slowmotion (https://www.desmos.com/calculator/w9dtmvku2t)
  3. Changes delta difference comparisons to use hitwindow-based epsilon
  4. Buffs initial rhythm bonus
  5. Fixes island comparisons to use delta as well as size
  6. Considers doubletappness of islands
  7. Refactor

Values can be checked here

@smoogipoo
Copy link
Contributor

!diffcalc

Copy link

github-actions bot commented Jul 16, 2024

@smoogipoo
Copy link
Contributor

!diffcalc

Copy link

github-actions bot commented Jul 20, 2024

Copy link
Contributor

@apollo-dw apollo-dw left a comment

Choose a reason for hiding this comment

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

LGTM

@stanriders
Copy link
Member Author

@smoogipoo this change is good to go

@peppy peppy added the next release Pull requests which are almost there. We'll aim to get them in the next release, but no guarantees! label Oct 4, 2024
@bdach
Copy link
Collaborator

bdach commented Oct 7, 2024

@ppy/osu-pp-committee please advise how to fix merge conflict with #29980

1728283285

...also #29998 touches this same thing again? what is going on here???

@stanriders
Copy link
Member Author

stanriders commented Oct 7, 2024

@ppy/osu-pp-committee please advise how to fix merge conflict with #29980

1728283285

Can use any value here, you can ignore any merge conflicts in Aim skillMultiplier as long as #29998 is the final pr that's getting merged. It was adjusted in half of prs to make individual changes more balanced in testing

@bdach
Copy link
Collaborator

bdach commented Oct 7, 2024

?_?

i guess i'm going to merge #29998 into this instead if that is the case, after #29980 (comment) sheet is done

@smoogipoo
Copy link
Contributor

Any reason for waiting on the sheet? I only requested a sheet to be queued, but is not required for merge.

@bdach
Copy link
Collaborator

bdach commented Oct 7, 2024

well i mean if i run a sheet for the sake of running the sheet i can, but given that this touches the same code the preceding pull touched, it would make sense to verify that one doesn't do anything obviously broken before going forward? or not? but in that case we might as well run one sheet at the end of everything?

this is a complete mess to manage.

@smoogipoo
Copy link
Contributor

smoogipoo commented Oct 7, 2024

If you're expecting branches to be working 100% as expected from the get-go, then sheets must look good before merge with no exceptions.

But, as we discussed, I'm fine not doing that in order to keep things moving at a rapid pace. My only requirement is for a sheet to be queued for each PR. It leaves a bit of a paper trail, if not to figure out where things went wrong / if a change has some edge case that was not known during rapid-merge, then to potentially be referenced if numbers are required to understand the change.

I will check every sheet myself, it's just for my own sanity :)

@bdach
Copy link
Collaborator

bdach commented Oct 7, 2024

Alright well I'm merging master into this, then merging #29998 into this, then fixing tests, then queueing sheet. Means we can skip one sheet for #29998.

@bdach
Copy link
Collaborator

bdach commented Oct 7, 2024

!diffcalc

@bdach
Copy link
Collaborator

bdach commented Oct 7, 2024

Queueing again with explicit OSU_A spec since I realised this might not work as intended if I do it as above.

!diffcalc
OSU_A=cf66d40

@bdach bdach merged commit fe97f15 into ppy:master Oct 7, 2024
9 of 13 checks passed
Copy link

github-actions bot commented Oct 7, 2024

@stanriders stanriders deleted the rhythm-fixes branch October 31, 2024 19:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:difficulty next release Pull requests which are almost there. We'll aim to get them in the next release, but no guarantees! ruleset/osu! size/L
Projects
Status: Deployed
Development

Successfully merging this pull request may close these issues.

7 participants