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 lazy path calculation in OsuDifficultyHitObject processing #30233

Open
wants to merge 2 commits into
base: pp-dev
Choose a base branch
from

Conversation

Givikap120
Copy link
Contributor

@Givikap120 Givikap120 commented Oct 11, 2024

  1. For some reason there was movement variable as well as movementLength variable, when only length was used. This was fixed.
  2. Movement length was calculated by subtraction and calling .Length method. It can be calculated in one line with Vector2.Distance, making it more readable.
  3. Some very scary multiplication formula was replaced with simple Vector2.Lerp call.
  4. To represent changed functions variables was renamed respectively

This can trigger some floating-point precision errors
EDIT: ye, that triggers some very small errors around 8E-5. Even tho I didn't noticed difference on Star Rating in game. Even on Kaede.

tsunyoku
tsunyoku previously approved these changes Nov 7, 2024
Copy link
Member

@tsunyoku tsunyoku left a comment

Choose a reason for hiding this comment

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

I'm not convinced that this really matters, but there's no harm in it either as long as sheets show that SR/PP differences are negligible.

@smoogipoo
Copy link
Contributor

smoogipoo commented Nov 7, 2024

Tests will need adjustments.

@Givikap120
Copy link
Contributor Author

I'm not convinced that this really matters, but there's no harm in it either as long as sheets show that SR/PP differences are negligible.

the whole sliders calculation looks way more intimidating than it should
so maybe some person who will read the pp code will have more willingness to research how it works

I don't sure this is necessary too
but after digging in the whole pp code I tried to change the things that was the most confusing

@smoogipoo
Copy link
Contributor

!diffcalc

Copy link

@Givikap120
Copy link
Contributor Author

oops, this has outdated diffcalc version

@smoogipoo
Copy link
Contributor

!diffcalc

Copy link

Difficulty calculation failed: https://github.com/ppy/osu/actions/runs/11791198882

Copy link

@Givikap120
Copy link
Contributor Author

Target: #30233 Spreadsheet: https://docs.google.com/spreadsheets/d/15lVwe2IEO5xiB95Oow1vClW6Mv5W1qSLyfvotHgRHh4/edit

hm yea, this is definitely big changes, I will look into it

@smoogipoo smoogipoo changed the base branch from master to pp-dev December 18, 2024 14:08
@smoogipoo smoogipoo dismissed tsunyoku’s stale review December 18, 2024 14:08

The base branch was changed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: In Progress
Development

Successfully merging this pull request may close these issues.

3 participants