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.

Sign up for GitHub

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

Jump to bottom

Simplify convertible hitobject parsing and add IHasLegacyHitObjectType #30578

Merged
merged 5 commits into from
Nov 14, 2024
Merged

Simplify convertible hitobject parsing and add IHasLegacyHitObjectType #30578

merged 5 commits into from
Nov 14, 2024

Conversation

smoogipoo
Copy link
Contributor

@smoogipoo smoogipoo commented Nov 11, 2024 •
edited
Loading

The main task I wanted to solve here is to add IHasLegacyHitObjectType so that I can update the databased osu_beatmap counts from osu-difficulty-calculator (ppy/osu-difficulty-calculator#233).

In the process, I thought I'd refactor this whole thing and remove the duplicate ConvertX hitobject types.

@smoogipoo
Merge implementations of ConvertHitObjectParser
e1d93a7 
Having these be separate implementations sounded awesome at the time,
but it only ever led to confusion. There's no practical difference if,
for example, catch sees hitobjects with `IHasPosition` instead of
`IHasXPosition`.
@smoogipoo
Add IHasLegacyHitObjectType to ConvertHitObject
7206e97
@smoogipoo
Remove unused params
8c68db0
@smoogipoo smoogipoo added the area:beatmap-parsing .osu file format parsing label Nov 11, 2024
@smoogipoo smoogipoo mentioned this pull request Nov 11, 2024
Merged
1 task
@smoogipoo
Copy link
Contributor Author

!diffcalc
RULESET=osu

@smoogipoo
Copy link
Contributor Author

!diffcalc
RULESET=taiko

@smoogipoo
Copy link
Contributor Author

!diffcalc
RULESET=catch

@smoogipoo
Copy link
Contributor Author

!diffcalc
RULESET=mania

@smoogipoo
Copy link
Contributor Author

!diffcalc
RULESET=osu
DIFFICULTY_CALCULATOR_B=ppy/osu-difficulty-calculator#233

@smoogipoo
Copy link
Contributor Author

!diffcalc
RULESET=taiko
DIFFICULTY_CALCULATOR_B=ppy/osu-difficulty-calculator#233

@smoogipoo
Copy link
Contributor Author

!diffcalc
RULESET=catch
DIFFICULTY_CALCULATOR_B=ppy/osu-difficulty-calculator#233

@smoogipoo
Copy link
Contributor Author

!diffcalc
RULESET=mania
DIFFICULTY_CALCULATOR_B=ppy/osu-difficulty-calculator#233

@bdach
Copy link
Collaborator

bdach commented Nov 11, 2024

Test compile failures here

@bdach bdach self-requested a review November 11, 2024 08:27
bdach
bdach reviewed Nov 11, 2024
View reviewed changes
Copy link
Collaborator

@bdach bdach left a comment

Choose a reason for hiding this comment

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

Source looks correct to me, but will wait for the diffcalc runs

@github-actions GitHub Actions
Copy link

Target: #30578
Spreadsheet: https://docs.google.com/spreadsheets/d/1YpYGWlET1PnYPVRuyS6D2-iHa0KODwaX3MJFPuRefrw/edit

@github-actions GitHub Actions
Copy link

Target: #30578
Spreadsheet: https://docs.google.com/spreadsheets/d/1TqUBxmm7eYPaJtDim1cAehSX39xTcTvrYH83ws9lwiw/edit

@github-actions GitHub Actions
Copy link

Target: #30578
Spreadsheet: https://docs.google.com/spreadsheets/d/1pyfSZUn8_3JdwojUCRkvMm2hURVKwrqNFKsDBRlumBI/edit

@github-actions GitHub Actions
Copy link

Target: #30578
Spreadsheet: https://docs.google.com/spreadsheets/d/1kWJlTQNRBFZ0Y5HnSLl-Ha_eiUyMHCMoB38K-6iqW1U/edit

@github-actions GitHub Actions
Copy link

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

@github-actions GitHub Actions
Copy link

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

@github-actions GitHub Actions
Copy link

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

@github-actions GitHub Actions
Copy link

Target: #30578
Spreadsheet: https://docs.google.com/spreadsheets/d/19cKPfM38bHiwU06_4QKLv8ALi21baEF7WiakK9IXiP0/edit

@github-actions GitHub Actions
Copy link

Target: #30578
Spreadsheet: https://docs.google.com/spreadsheets/d/1hYaynwHHb_rxxYElkPd5HOGqM7_jL6zoxhJhja7cu7s/edit

@github-actions GitHub Actions
Copy link

Target: #30578
Spreadsheet: https://docs.google.com/spreadsheets/d/1kVenDECmkQgDtL6fppEXlQSqbyH0xHcpzLWFnSWXIbM/edit

@github-actions GitHub Actions
Copy link

Target: #30578
Spreadsheet: https://docs.google.com/spreadsheets/d/1q0-GNedfQXs0CJ5GpU2SCFYwtdZjTC0QbkddVulJ4lo/edit

@smoogipoo
Copy link
Contributor Author

Not sure what happened with the first set of runs that have had spreadsheets deleted 🤔

https://docs.google.com/spreadsheets/d/19cKPfM38bHiwU06_4QKLv8ALi21baEF7WiakK9IXiP0/edit

The change here is intended, and is likely so small scale due to the hotfix #30544

Regardless, I'm pretty confident in this one now. Can rerun the previous sheets if you feel necessary.

@peppy peppy self-requested a review November 14, 2024 07:12
peppy
peppy approved these changes Nov 14, 2024
View reviewed changes
Copy link
Member

@peppy peppy left a comment

Choose a reason for hiding this comment

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

Amazing improvement 💯

@peppy peppy merged commit 7f8eebc into ppy:master Nov 14, 2024
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bdach bdach bdach left review comments

@peppy peppy peppy approved these changes

Assignees
No one assigned
Labels
area:beatmap-parsing .osu file format parsing size/XL
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@smoogipoo @bdach @peppy