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

Calculate hit windows in performance calculator instead of databased difficulty attributes #31735

Merged
merged 9 commits into from
Feb 7, 2025

Conversation

tsunyoku
Copy link
Member

See #31595 (comment).

Depends on ppy/osu-queue-score-statistics#321 so that score.BeatmapInfo.Difficulty can be used in the calculator server-side.

@tsunyoku
Copy link
Member Author

!diffcalc
RULESET=osu
OSU_A=https://github.com/ppy/osu/tree/pp-dev
OSU_B=#31735
SCORE_PROCESSOR_B=ppy/osu-queue-score-statistics#321

@smoogipoo
Copy link
Contributor

Can the great/ok attribute definitions be removed too?

@stanriders
Copy link
Member

You should do the same for other rulesets too then

@tsunyoku
Copy link
Member Author

tsunyoku commented Jan 30, 2025

Yes we can, but it's worth noting these other usages are not new. Note the following:

  • osu!mania's GreatHitWindow difficulty attribute can be removed with no consideration. It isn't even used in the first place, so useless storage.
  • osu!taiko's GreatHitWindow and OkHitWindow difficulty attributes can be removed in the same way this PR already does for osu!, and replace it with calculations in the performance calculator.

Following from https://discord.com/channels/188630481301012481/380598781432823815/1334524621625491507, I need to push a fix for the osu! changes to ensure that any mods which affect the base overall difficulty are applied to the beatmap difficulty. However, I'm unsure if this is going to result in a double application of mods in the case the beatmap provided already had them applied... I'm thinking if this is coming from the PP counter, or osu-tools, it's going to use a beatmap which already has the beatmap difficulty modified in context of the score. Maybe this could be done on the server-side so we can trust the beatmap difficulty object in the performance calculator and it will work in either case.

In doing this, I noticed that we may be able to go an extra step and remove the OverallDifficulty attribute from osu!'s difficulty attributes entirely.

Copy link

@smoogipoo
Copy link
Contributor

I guess it depends on what your goals are here. If the goal is only to remove the newly added attribute, then sure, but I was intending to go the full way and remove every single one to reduce DB storage requirements. Good catch on OD, perhaps the AR attribute too?

@tsunyoku
Copy link
Member Author

Yeah, I intend on getting rid of as many as possible. AR seems doable too.

I'm still unsure if the application of difficulty from the score mods should be happening on server-side or in the performance calculator, though. It seems that some places are applying difficulty (i.e BeatmapDifficultyCache calls WorkingBeatmap which applies difficulty), so it probably should be a responsibility of osu-queue-score-statistics to pass the beatmap with mods being applied to difficulty. What do you think?

@peppy
Copy link
Member

peppy commented Feb 5, 2025

I'm still unsure if the application of difficulty from the score mods should be happening on server-side or in the performance calculator, though

What does "application of difficulty" mean here?

It's a bit hard to get a sense of what is happening here (and hard to really picture what ppy/osu-queue-score-statistics#321 is intending to achieve) so a summary/overview of what is using what data would be appreciated.

@smoogipoo
Copy link
Contributor

should be a responsibility of osu-queue-score-statistics to pass the beatmap with mods being applied to difficulty

Thought I replied before, but I disagree with this just to keep everything uniform. We never deal with the base beatmap containing post-modded values - only after WorkingBeatmap.GetPlayableBeatmap(). Other processors like medals are likely going to start falling over if we break this, so it should continue being applied in the performance calculator.

@pull-request-size pull-request-size bot added size/L and removed size/M labels Feb 5, 2025
@tsunyoku
Copy link
Member Author

tsunyoku commented Feb 5, 2025

I'm not sure if I didn't word my question well or not but I was more asking what the game actually passes rather the design that was preferred. I ran some scenarios with a debugger locally (results screen calculation, PP counter skin element during gameplay) and it looks like the performance calculator is passed the pre-modded values which solves my concerns. Note that I did have to add a Clone() on the BeatmapInfo.Difficulty in the calculators as places such as the results screen were doing multiple calculations using the same BeatmapInfo reference and it was trying to apply mod-related changes on each invocation.

Latest commits remove all difficulty attribute usage for hit windows, OD and AR.

@tsunyoku
Copy link
Member Author

tsunyoku commented Feb 5, 2025

@peppy A summary of what's going on, so you're in the loop.

Currently, each ruleset's performance calculations rely on hit windows, approach rate or overall difficulty with consideration to the mods played. These are stored as difficulty attributes in the database, and then used in performance calculations. However, we're able to calculate these in the performance calculator on-the-fly as we have all the data required (assuming the osu-queue-score-statistics PR is merged - it gives us access to score.BeatmapInfo.Difficulty). This PR moves all of those calculations into performance calculation using the score.BeatmapInfo.Difficulty to do it. This allows us to remove quite a few attributes, thus saving a lot of DB storage.

@ppy ppy deleted a comment from github-actions bot Feb 5, 2025
@ppy ppy deleted a comment from github-actions bot Feb 5, 2025
@ppy ppy deleted a comment from github-actions bot Feb 5, 2025
@ppy ppy deleted a comment from github-actions bot Feb 5, 2025
@ppy ppy deleted a comment from github-actions bot Feb 5, 2025
@ppy ppy deleted a comment from github-actions bot Feb 5, 2025
@tsunyoku
Copy link
Member Author

tsunyoku commented Feb 6, 2025

!diffcalc
RULESET=osu
OSU_A=https://github.com/ppy/osu/tree/pp-dev
OSU_B=#31735
SCORE_PROCESSOR_B=ppy/osu-queue-score-statistics#321

@tsunyoku
Copy link
Member Author

tsunyoku commented Feb 6, 2025

!diffcalc
RULESET=taiko
OSU_A=https://github.com/ppy/osu/tree/pp-dev
OSU_B=#31735
SCORE_PROCESSOR_B=ppy/osu-queue-score-statistics#321

@tsunyoku
Copy link
Member Author

tsunyoku commented Feb 6, 2025

!diffcalc
RULESET=catch
OSU_A=https://github.com/ppy/osu/tree/pp-dev
OSU_B=#31735
SCORE_PROCESSOR_B=ppy/osu-queue-score-statistics#321

@tsunyoku
Copy link
Member Author

tsunyoku commented Feb 6, 2025

!diffcalc
RULESET=mania
OSU_A=https://github.com/ppy/osu/tree/pp-dev
OSU_B=#31735
SCORE_PROCESSOR_B=ppy/osu-queue-score-statistics#321

Copy link

github-actions bot commented Feb 6, 2025

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

Copy link

github-actions bot commented Feb 6, 2025

@ppy ppy deleted a comment from github-actions bot Feb 6, 2025
@tsunyoku
Copy link
Member Author

tsunyoku commented Feb 6, 2025

!diffcalc
RULESET=taiko
OSU_A=https://github.com/ppy/osu/tree/pp-dev
OSU_B=#31735
SCORE_PROCESSOR_B=ppy/osu-queue-score-statistics#321

@tsunyoku
Copy link
Member Author

tsunyoku commented Feb 6, 2025

!diffcalc
RULESET=osu
OSU_A=https://github.com/ppy/osu/tree/pp-dev
OSU_B=#31735
SCORE_PROCESSOR_B=ppy/osu-queue-score-statistics#321

Copy link

github-actions bot commented Feb 6, 2025

Copy link

github-actions bot commented Feb 6, 2025

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

Copy link

github-actions bot commented Feb 7, 2025

Copy link

github-actions bot commented Feb 7, 2025

@tsunyoku
Copy link
Member Author

tsunyoku commented Feb 7, 2025

!diffcalc
RULESET=mania
OSU_A=https://github.com/ppy/osu/tree/pp-dev
OSU_B=#31735
SCORE_PROCESSOR_B=ppy/osu-queue-score-statistics#321

@peppy
Copy link
Member

peppy commented Feb 7, 2025

@peppy A summary of what's going on, so you're in the loop.

Currently, each ruleset's performance calculations rely on hit windows, approach rate or overall difficulty with consideration to the mods played. These are stored as difficulty attributes in the database, and then used in performance calculations. However, we're able to calculate these in the performance calculator on-the-fly as we have all the data required (assuming the osu-queue-score-statistics PR is merged - it gives us access to score.BeatmapInfo.Difficulty). This PR moves all of those calculations into performance calculation using the score.BeatmapInfo.Difficulty to do it. This allows us to remove quite a few attributes, thus saving a lot of DB storage.

Thanks I finally get it. I was trying to figure out the cross-project dependency, and thought there was some data ferrying happening but it turns out that score-statistics just called a special method GetLegacyBeatmapConversionDifficultyInfo and that's all that's really required.

@peppy peppy merged commit 9f90ebb into ppy:pp-dev Feb 7, 2025
8 of 9 checks passed
Copy link

github-actions bot commented Feb 7, 2025

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

@smoogipoo
Copy link
Contributor

Noticed this one failed to do the osu! calc above...

!diffcalc
RULESET=osu
OSU_A=6c069b3
OSU_B=https://github.com/tsunyoku/osu/tree/no-databased-hit-windows

@tsunyoku
Copy link
Member Author

Isn't it mania which failed? Which should be fine anyways, the only change to mania here was removing an unused property in the first place.

osu sheet: #31735 (comment)

@smoogipoo
Copy link
Contributor

Oh, you're right. Seems fine then. The error is quite concerning though:

2025-02-07T11:49:10.3807545Z verify-1     | Unhandled exception. MySqlConnector.MySqlException (0x80004005): You have an error in your SQL syntax; check the manual that corresponds to your MySQL server version for the right syntax to use near 'WHERE id = 4276110462;DELETE FROM scores WHERE id = 4276110973;DELETE FROM score' at line 1
2025-02-07T11:49:10.3809464Z verify-1     |    at MySqlConnector.Core.ServerSession.ReceiveReplyAsync(IOBehavior ioBehavior, CancellationToken cancellationToken) in /_/src/MySqlConnector/Core/ServerSession.cs:line 885
2025-02-07T11:49:10.3810760Z verify-1     |    at MySqlConnector.Core.ResultSet.ReadResultSetHeaderAsync(IOBehavior ioBehavior) in /_/src/MySqlConnector/Core/ResultSet.cs:line 37
2025-02-07T11:49:10.3811934Z verify-1     |    at MySqlConnector.MySqlDataReader.ActivateResultSet(CancellationToken cancellationToken) in /_/src/MySqlConnector/MySqlDataReader.cs:line 130
2025-02-07T11:49:10.3814167Z verify-1     |    at MySqlConnector.MySqlDataReader.InitAsync(CommandListPosition commandListPosition, ICommandPayloadCreator payloadCreator, IDictionary`2 cachedProcedures, IMySqlCommand command, CommandBehavior behavior, Activity activity, IOBehavior ioBehavior, CancellationToken cancellationToken) in /_/src/MySqlConnector/MySqlDataReader.cs:line 482
2025-02-07T11:49:10.3816845Z verify-1     |    at MySqlConnector.Core.CommandExecutor.ExecuteReaderAsync(CommandListPosition commandListPosition, ICommandPayloadCreator payloadCreator, CommandBehavior behavior, Activity activity, IOBehavior ioBehavior, CancellationToken cancellationToken) in /_/src/MySqlConnector/Core/CommandExecutor.cs:line 56
2025-02-07T11:49:10.3818820Z verify-1     |    at MySqlConnector.MySqlCommand.ExecuteNonQueryAsync(IOBehavior ioBehavior, CancellationToken cancellationToken) in /_/src/MySqlConnector/MySqlCommand.cs:line 309
2025-02-07T11:49:10.3819930Z verify-1     |    at MySqlConnector.MySqlCommand.ExecuteNonQuery() in /_/src/MySqlConnector/MySqlCommand.cs:line 108
2025-02-07T11:49:10.3836281Z verify-1     |    at Dapper.SqlMapper.ExecuteCommand(IDbConnection cnn, CommandDefinition& command, Action`2 paramReader) in /_/Dapper/SqlMapper.cs:line 2976
2025-02-07T11:49:10.3837555Z verify-1     |    at Dapper.SqlMapper.ExecuteImpl(IDbConnection cnn, CommandDefinition& command) in /_/Dapper/SqlMapper.cs:line 667
2025-02-07T11:49:10.3839153Z verify-1     |    at Dapper.SqlMapper.Execute(IDbConnection cnn, String sql, Object param, IDbTransaction transaction, Nullable`1 commandTimeout, Nullable`1 commandType) in /_/Dapper/SqlMapper.cs:line 538
2025-02-07T11:49:10.3841206Z verify-1     |    at osu.Server.Queues.ScoreStatisticsProcessor.Commands.Maintenance.VerifyImportedScoresCommand.flush(MySqlConnection conn, Boolean force) in /tmp/tmp.OXbP7t1aks/osu-queue-score-statistics/osu.Server.Queues.ScoreStatisticsProcessor/Commands/Maintenance/VerifyImportedScoresCommand.cs:line 297
2025-02-07T11:49:10.3843920Z verify-1     |    at osu.Server.Queues.ScoreStatisticsProcessor.Commands.Maintenance.VerifyImportedScoresCommand.OnExecuteAsync(CancellationToken cancellationToken) in /tmp/tmp.OXbP7t1aks/osu-queue-score-statistics/osu.Server.Queues.ScoreStatisticsProcessor/Commands/Maintenance/VerifyImportedScoresCommand.cs:line 270
2025-02-07T11:49:10.3845982Z verify-1     |    at McMaster.Extensions.CommandLineUtils.Conventions.ExecuteMethodConvention.InvokeAsync(MethodInfo method, Object instance, Object[] arguments)
2025-02-07T11:49:10.3847324Z verify-1     |    at McMaster.Extensions.CommandLineUtils.Conventions.ExecuteMethodConvention.OnExecute(ConventionContext context, CancellationToken cancellationToken)
2025-02-07T11:49:10.3848586Z verify-1     |    at McMaster.Extensions.CommandLineUtils.Conventions.ExecuteMethodConvention.<>c__DisplayClass0_0.<<Apply>b__0>d.MoveNext()
2025-02-07T11:49:10.3849356Z verify-1     | --- End of stack trace from previous location ---
2025-02-07T11:49:10.3850141Z verify-1     |    at McMaster.Extensions.CommandLineUtils.CommandLineApplication.ExecuteAsync(String[] args, CancellationToken cancellationToken)
2025-02-07T11:49:10.3851906Z verify-1     |    at McMaster.Extensions.CommandLineUtils.CommandLineApplication.ExecuteAsync[TApp](CommandLineContext context, CancellationToken cancellationToken)
2025-02-07T11:49:10.3853352Z verify-1     |    at osu.Server.Queues.ScoreStatisticsProcessor.Program.Main(String[] args) in /tmp/tmp.OXbP7t1aks/osu-queue-score-statistics/osu.Server.Queues.ScoreStatisticsProcessor/Program.cs:line 27
2025-02-07T11:49:10.3854461Z verify-1     |    at osu.Server.Queues.ScoreStatisticsProcessor.Program.<Main>(String[] args)

KitsunIvy pushed a commit to KitsunIvy/osu that referenced this pull request Feb 18, 2025
…difficulty attributes (ppy#31735)

* Calculate hit windows in performance calculator instead of databased difficulty attributes

* Apply mods to beatmap difficulty in osu! performance calculator

* Remove `GreatHitWindow` difficulty attribute for osu!mania

* Remove use of approach rate and overall difficulty attributes for osu!

* Remove use of hit window difficulty attributes in osu!taiko

* Remove use of approach rate attribute in osu!catch

* Remove unused attribute IDs

* Code quality

* Fix `computeDeviationUpperBound` being called before `greatHitWindow` is set
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