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

Migrate pp system implementation to osuAkatsuki/akatsuki-pp-rs #315

Merged
merged 24 commits into from
Dec 28, 2022

Conversation

7mochi
Copy link
Contributor

@7mochi 7mochi commented Dec 24, 2022

Closes #283, closes #184, closes #101

@7mochi 7mochi requested a review from cmyui as a code owner December 24, 2022 17:31
@minisbett
Copy link
Contributor

Why would you make the AR factor 0 on relax

@minisbett
Copy link
Contributor

minisbett commented Dec 24, 2022

This is the worst relax implementation I've ever seen can't we just implement the Akatsuki aim crosscheck instead

@minisbett
Copy link
Contributor

Nvm it's the bancho implementation I forgot it sucks

@minisbett
Copy link
Contributor

We will definitely have to fork rosupp i don't think anyone here wants 1k pp plays getting nerfed to 200-300pp

@tsunyoku
Copy link
Contributor

tbh just use https://github.com/osuAkatsuki/akatsuki-pp-rs since it's rosu-pp too.
just note the relax system lies in osu_2019 and not osu, because adapting 2022 for relax was awful

i'd also like to note that rosu_pp_py is not up to date with the latest rosu-pp version (due to rewrite stuff), so i don't think making this change is a good idea right now

@minisbett
Copy link
Contributor

Also better for customization so for this PR I would suggest putting in rosupp-py as a submodule instead

@minisbett
Copy link
Contributor

tbh just use https://github.com/osuAkatsuki/akatsuki-pp-rs since it's rosu-pp too. just note the relax system lies in osu_2019 and not osu, because adapting 2022 for relax was awful

i'd also like to note that rosu_pp_py is not up to date with the latest rosu-pp version (due to rewrite stuff), so i don't think making this change is a good idea right now

Or that would work

@7ez
Copy link
Contributor

7ez commented Dec 24, 2022

just replace vn pp with rosu-pp, I don't see a sense in replacing the whole std calc with it

@tsunyoku
Copy link
Contributor

just replace vn pp with rosu-pp, I don't see a sense in replacing the whole std calc with it

because we get to use a single system across the board which makes life much easier
also oppai is problematic & peace is outdated and doesn't support converts

however, we need to wait for rosu-pp-py to be updated at the very least; unless we decide to just use akatsuki's version before then

@7mochi
Copy link
Contributor Author

7mochi commented Dec 24, 2022

tbh just use https://github.com/osuAkatsuki/akatsuki-pp-rs since it's rosu-pp too. just note the relax system lies in osu_2019 and not osu, because adapting 2022 for relax was awful

i'd also like to note that rosu_pp_py is not up to date with the latest rosu-pp version (due to rewrite stuff), so i don't think making this change is a good idea right now

I could try adapting that version with the latest rosu-pp changes. Would that work?

@tsunyoku
Copy link
Contributor

tbh just use https://github.com/osuAkatsuki/akatsuki-pp-rs since it's rosu-pp too. just note the relax system lies in osu_2019 and not osu, because adapting 2022 for relax was awful
i'd also like to note that rosu_pp_py is not up to date with the latest rosu-pp version (due to rewrite stuff), so i don't think making this change is a good idea right now

I could try adapting that version with the latest rosu-pp changes. Would that work?

it already includes them, it’s up to date with rosu-pp’s main branch

@minisbett
Copy link
Contributor

Either way, I still suggest putting rosupp-py in as a submodule so it can be modified

@cmyui
Copy link
Member

cmyui commented Dec 24, 2022

nice work so far, thanks for taking this responsibility on!

@cmyui cmyui added enhancement New feature or request code quality Something is implemented poorly labels Dec 24, 2022
@cmyui
Copy link
Member

cmyui commented Dec 24, 2022

also, looks like i must've broke pre-commit ci at some point 🤔

@7mochi
Copy link
Contributor Author

7mochi commented Dec 24, 2022

Which python bindings would I use with akatsuki-pp-rs?

however, we need to wait for rosu-pp-py to be updated at the very least;

I was using rosu-pp-py in my fork and it was working without problems D:

@cmyui
Copy link
Member

cmyui commented Dec 25, 2022

Which python bindings would I use with akatsuki-pp-rs?

@tsunyoku if you could help him out a bit here would be very epic 11/10

@7mochi
Copy link
Contributor Author

7mochi commented Dec 27, 2022

Which python bindings would I use with akatsuki-pp-rs?

@tsunyoku if you could help him out a bit here would be very epic 11/10

akatsuki-pp-rs doesn't have python bindings yet so idk know how I should proceed 💀
I guess I'll wait until there is one because I haven't been able to adapt those changes to rosu-pp-py on my own

@minisbett
Copy link
Contributor

Which python bindings would I use with akatsuki-pp-rs?

@tsunyoku if you could help him out a bit here would be very epic 11/10

akatsuki-pp-rs doesn't have python bindings yet so idk know how I should proceed 💀 I guess I'll wait until there is one because I haven't been able to adapt those changes to rosu-pp-py on my own

Since akatsuki-pp-rs is based on rosu-pp shouldn't the normal rosu-pp bindings work? Idk how python bindings work but if I imagine it correct it's basically just saying access these and these methods and the methods should have the same signatures

@7mochi
Copy link
Contributor Author

7mochi commented Dec 27, 2022

I think I got it to work, but there's no bindings repo in osuAkatsuki so i can't make a PR yet, anyways, i've tested it in my fork and it works. Could you guys check it? @cmyui @tsunyoku

Changes:

if score["mods"] and Mods.RELAX & score["mods"]:
    calculator.set_acc_2019(acc)
    result = calculator.performance_2019(map)
else:
    result = calculator.performance(map)

@cmyui
Copy link
Member

cmyui commented Dec 27, 2022

nice, @tsunyoku is working on making bindings for akatsuki's pp system

@cmyui
Copy link
Member

cmyui commented Dec 27, 2022

i'll review this shortly

@cmyui cmyui requested a review from tsunyoku December 27, 2022 22:36
@cmyui cmyui added the documentation Improvements or additions to documentation label Dec 27, 2022
@cmyui
Copy link
Member

cmyui commented Dec 27, 2022

@7mochi hey, do you have any uncommitted changes? i'd like to take over the pr for a bit when you're ready

app/commands.py Outdated Show resolved Hide resolved
app/commands.py Outdated Show resolved Hide resolved
app/commands.py Outdated Show resolved Hide resolved
app/commands.py Outdated Show resolved Hide resolved
app/constants/gamemodes.py Show resolved Hide resolved
app/commands.py Outdated Show resolved Hide resolved
app/commands.py Outdated Show resolved Hide resolved
app/commands.py Outdated Show resolved Hide resolved
app/commands.py Outdated Show resolved Hide resolved
app/commands.py Outdated Show resolved Hide resolved
@cmyui
Copy link
Member

cmyui commented Dec 28, 2022

going to refactor the calculate_performances_x functions into a singular interface, now that we can

@cmyui cmyui added the dependencies Pull requests that update a dependency file label Dec 28, 2022
@cmyui cmyui changed the title Migrate pp system implementation to rosu-pp-py Migrate pp system implementation to rosu-pp Dec 28, 2022
@cmyui
Copy link
Member

cmyui commented Dec 28, 2022

I think we're good to go on this.

Can I get a re-review @tsunyoku, @7mochi please!

@cmyui cmyui requested a review from tsunyoku December 28, 2022 04:47
@cmyui cmyui requested a review from a team December 28, 2022 04:47
@cmyui
Copy link
Member

cmyui commented Dec 28, 2022

also @alowave223 you should take a look, i think

@cmyui
Copy link
Member

cmyui commented Dec 28, 2022

ah, found some bugs with callers of calculate_performances still using params that were moved into the scores param

@cmyui
Copy link
Member

cmyui commented Dec 28, 2022

ok, things look good now. feel free to review away!

@cmyui cmyui changed the title Migrate pp system implementation to rosu-pp Migrate pp system implementation to osuAkatsuki/akatsuki-pp-rs Dec 28, 2022
@cmyui cmyui merged commit 1f0249e into osuAkatsuki:master Dec 28, 2022
@cmyui cmyui deleted the rosupp branch December 28, 2022 16:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
code quality Something is implemented poorly dependencies Pull requests that update a dependency file documentation Improvements or additions to documentation enhancement New feature or request
Projects
None yet
5 participants