Implemented functionality to calculate all local scores using Realm #213

Givikap120 · 2024-08-13T17:20:23Z

Now Profile page will have a switch that can change mode of calculation to local.
Local mode has additional setting menu.
As local mode doesn't have deltas - ExtendedProfileScore was divided into two: ProfileScore and ExtendedProfileScore, where only second has delta display.
Calculation of 50k scores will be slow, that's why new caching algorithm was implemented. This caching will be also used in BeatmapLeaderboardScreen, what will speed-up calculation of leaderboard on long maps in around 10 times.

Thanks to dani211e for "calculation from Realm" function

Natelytle · 2024-08-13T19:47:14Z

Parenthesis are useless imo (+ the wording could be better, "for example" and "anyway" isn't wording you should use in a context like this.)
Scorev1'd scores and unsubmitted scores should be combined into one option. If you do decide to keep it, fix the wording, something like "Only calculate highest legacy score".
I don't get why RX is excluded? Autopilot I understand, but RX is a very weird choice since it is handled in PP now.

As well, the calculation seems to fail for me. Not sure why, no error message.

Givikap120 · 2024-08-14T08:10:13Z

Parenthesis are useless imo (+ the wording could be better, "for example" and "anyway" isn't wording you should use in a context like this.) Scorev1'd scores and unsubmitted scores should be combined into one option. If you do decide to keep it, fix the wording, something like "Only calculate highest legacy score". I don't get why RX is excluded? Autopilot I understand, but RX is a very weird choice since it is handled in PP now.

As well, the calculation seems to fail for me. Not sure why, no error message.

Scorev1-ed scores are submitted (as well as scores on graveyard maps are submitted too). So it doesn't makes sense to filter-out those scores if we ignore not submitted scores.
RX is excluded because RX was overweight with CSR, making my #1 unranked score to be 300pp on 8 star with RX. But that's silly reason, ye, I should probably remove it.
Also >14 star unranked maps was excluded in the code because aspire map sliderend miss penalty is super bad. Don't sure should I keep this or no.
And your screenshot seems to have old text, scorev1-ning option name is "Enable Scorev1 score overwrite for legacy scores".
About linux - idk how to deal with it. I will let StanR to look into this issue.

smoogipoo · 2024-08-15T07:05:59Z

What is the use-case of this - why compute from local scores rather than online profile?

stanriders · 2024-08-15T08:02:13Z

What is the use-case of this - why compute from local scores rather than online profile?

The idea is to be able to compute more scores per profile since API is limited to 100 only and database dumps are limited to top 10k players

Givikap120 · 2024-08-15T09:46:39Z

What is the use-case of this - why compute from local scores rather than online profile?

checking your real profile with CSR is pretty much impossible now (if you're not 4 digit and higher) - this thing fixes it
also it's useful for players who often play graveryard maps and have a lot of top plays on them (example - Plasma, Lolu)
+ players like me, who have a lot of RA scores - I can finally see where they should be
it's just funny in general to see different statistics like "amount of 200pp plays you have" or "how much pp you would have only on unranked maps"
in future I will also add mod filter, so you will be able to calculate your top plays only for NM for example
precedent for having Realm access, meaning that you now can have simulate score on lazer beatmap without having jump through hoops with exporting it back to stable. Also it means proper search in the simulate menu. Also it means you can use beatmaps directly from lazer, without having to download it into local storage, costing time and space.

I hope this is enough reasons to understand why this PR is useful

minisbett · 2024-08-31T01:26:31Z

Selecting maps from lazer instead of having to bother with .osu files? yes please merge this that'd be sooo useful

stanriders · 2024-08-31T08:21:14Z

Selecting maps from lazer instead of having to bother with .osu files? yes please merge this that'd be sooo useful

This is not what it does. It's only allowing you to calculate already existing scores from your lazer scores database. You can also just use beatmap ids instead of .osu files already

stanriders · 2024-08-31T08:24:39Z

PerformanceCalculatorGUI/Screens/BeatmapLeaderboardScreen.cs


-                    Mod[] mods = score.Mods.Select(x => x.ToMod(rulesetInstance)).ToArray();
+                    if (!attributesCache.TryGetValue(modsHash, out var difficultyAttributes))


Realistically one player wouldn't have more than like ~10 scores on average with the same mod combination on one map so it seems redundant to me.

Caching reduced calculation time in 10 times for me. 10 times performance difference, especially in case of 15 seconds vs 3 minutes is definitely not redundant.

Calculating my own database (4410 scores) took 3 minutes 30 seconds with caching and 2 minutes 8 seconds without. I'm not saying that caching is making it slower (its probably just filesystem being faster when accessing the same files again), but it's definitely isn't making it faster

And that's with osu being on an old HDD which probably contributes a lot to the calculation being slow because of map parsing

After running it couple more times with and without caching it settled at about the same time per calculation with non-cached being ~15 seconds slower which I think is negligible considering you already have to wait a couple of minutes per run

After running it couple more times with and without caching it settled at about the same time per calculation with non-cached being ~15 seconds slower which I think is negligible considering you already have to wait a couple of minutes per run

Did you used scorev1-ning?
Because if all your scores are stable scores and they're filtered during scorev1 simulation - there will be no benefit from caching.

After running it couple more times with and without caching it settled at about the same time per calculation with non-cached being ~15 seconds slower which I think is negligible considering you already have to wait a couple of minutes per run

I don't know how you tested it, but you definitely messed something up, because you can't get this low speed-up from just simply accessing array of attributes instead of calculating it each time
https://www.youtube.com/watch?v=KEYa7Y-UJCw
cached: 0:36 - 3:17 = 161s = 2m 41s
not cached: 4:24 - 11:09 = 405s = 6m 45s
the difference is 2.5 times (not 10, but I haven't tested it before, I just noticed that it was giga slow without caching)
this is definitely not minor nor negliable, especially when we are talking about more than 4 minutes of saved time for each calculation

stanriders · 2024-08-31T08:26:21Z

PerformanceCalculatorGUI/RulesetHelper.cs

+        /// Generates the unique hash of mods combo that affect difficulty calculation
+        /// Needs to be updated if list of difficulty adjusting mods changes
+        /// </summary>
+        public static int GenerateModsHash(Mod[] mods, BeatmapDifficulty difficulty, RulesetInfo ruleset)


I really want this to not exist - not only is it one more thing to keep updated, its also redundant and confusing to look at

Calculating big amount of scores without caching is pretty much impossible
Having to update one function once new mod affect Star Rating (happens once in a decade) is much better than having to wait for 30 minutes till your recalc will finish

This cannot exist. You cannot combine 50 million different flags like this into a hash function.

The most that should be done is akin to https://github.com/ppy/osu/blob/62f737d8de0944d5b278d4ab72844af6dbc67ace/osu.Game/Beatmaps/BeatmapDifficultyCache.cs#L295-L305

PerformanceCalculatorGUI/Components/LazerCalculationSettings.cs

stanriders · 2024-08-31T08:28:26Z

PerformanceCalculatorGUI/Components/LazerCalculationSettings.cs

+
+namespace PerformanceCalculatorGUI.Components
+{
+    public partial class LazerCalculationSettings : ToolbarButton, IHasPopover


Why is it a toolbar button? It doesn't seem to be on the toolbar

Because I needed to make "options" button and ToolbarButton class was the easiest way to do it.

stanriders · 2024-08-31T08:29:48Z

PerformanceCalculatorGUI/Screens/ProfileScreen.cs

@@ -6,22 +6,27 @@
 using System.Linq;


Please move database calculation into a separate screen

It initially was "RealmScreen", but first - it have big amount of code duplication, second - it's much more convenient to use as a switch for profile calculation (the same way as simulation screen have options for import by ID and import by .osu file).
But if it's important to have separate screens for this - I will revert the merge of 2 screens.

stanriders · 2024-08-31T08:33:52Z

PerformanceCalculatorGUI/Screens/ProfileScreen.cs

+                }
+
+                var storage = gameHost.GetStorage(lazerPath);
+                var realmAccess = new RealmAccess(storage, @"client.realm");


Tools should never use lazer's database directly - please copy to a local directory and use that instead

Then please add the least add buttons to copy lazers data into a local copy and be able to sync it

Please don't, it should be syncronized automatically at the start of profile calculation.

oh thats an option too, but why should it be cloned instead of accessing Lazer directly in the first place? Issues if Lazer is running at the same time?

Tools should never use lazer's database directly - please copy to a local directory and use that instead

copying 30+ gigs of data? sounds very bad

Either that or maybe have the user select the client.db?

Furthermore, I wonder if you'd be better off splitting into two processes:

Select a db file, copy it locally, recalculate scores given whatever parameters, write values back to the DB.

Select a db file, display scores.

You could implement (1) as a CLI command that writes the DB to an output path.

dotnet run -- recalc-lazer-db /path/to/input/client.ream /path/to/output/client.realm [--options]

Is there a good way to make copied client.realm to contain ONLY replay files
not only this will be more convenient - it will be also faster

Faster how? You said it's a 50MB file? That takes 2 seconds on a 2000s era HDD, and is practically instantaneous on anything faster.

I suggest you don't go pre-optimising.

This is not about copying
The first 28 seconds of 3 minutes calculation was spent to collect all scores from the database, what is quite big portion of the time
Source:
https://www.youtube.com/watch?v=KEYa7Y-UJCw

That is a problem with the structure of your code, because you're using Detach(). You should restructure so you don't have to do that, such as using the Live<> class or otherwise.

stanriders · 2024-08-31T08:35:46Z

PerformanceCalculatorGUI/Screens/ProfileScreen.cs

+                }
+
+                var storage = gameHost.GetStorage(lazerPath);
+                var realmAccess = new RealmAccess(storage, @"client.realm");


RealmAccess definitely should not be created here - having it in a Task is bound to have threading-related issues

It should be fine as long as it's constrained to a single thread. If you're passing the realm or objects retrieved within it that are not .Detach()ed between threads, then there'll be an issue.

It already crashes on someone's linux machine because of cross-thread access, I think it'd just be safer to use it through safeguards instead of hoping that TPL wouldn't do something funny

PerformanceCalculatorGUI/Screens/ProfileScreen.cs

stanriders · 2024-08-31T08:39:24Z

PerformanceCalculatorGUI/Components/ExtendedProfileScore.cs


 namespace PerformanceCalculatorGUI.Components
 {
-    public class ExtendedScore
+    public class ExtendedProfileScore : ProfileScore


What's the reasoning for this split?

Because I need to have Profile Score class that display only pp (as it doesn't have delta). And Extended Profile Score (child of Profile Score) also displays delta.

minisbett · 2024-08-31T09:26:40Z

Selecting maps from lazer instead of having to bother with .osu files? yes please merge this that'd be sooo useful

This is not what it does. It's only allowing you to calculate already existing scores from your lazer scores database. You can also just use beatmap ids instead of .osu files already

Oh, it sounded like there's a proper selection for a beatmap with a search function instead of having to find the beatmap id, but seems like i misunderstood that

Givikap120 · 2024-08-31T10:14:04Z

Selecting maps from lazer instead of having to bother with .osu files? yes please merge this that'd be sooo useful

This is not what it does. It's only allowing you to calculate already existing scores from your lazer scores database. You can also just use beatmap ids instead of .osu files already

Oh, it sounded like there's a proper selection for a beatmap with a search function instead of having to find the beatmap id, but seems like i misunderstood that

If Realm Access of this PR is merged - then making proper selection for a beatmap is 10 times easier as it's the main problem that's stopping search in lazer db.

smoogipoo

I honestly don't know if this even belongs in this repo... This is a lot of code and the dev team (especially myself) is pretty constrained review-wise, so I'm only doing a very broad review and will entrust StanR with general code quality and what is/isn't acceptable.

PerformanceCalculatorGUI/Components/LazerCalculationSettings.cs

PerformanceCalculatorGUI/Components/ProfileScore.cs

smoogipoo · 2024-08-31T18:03:34Z

PerformanceCalculatorGUI/RulesetHelper.cs

+        /// Generates the unique hash of mods combo that affect difficulty calculation
+        /// Needs to be updated if list of difficulty adjusting mods changes
+        /// </summary>
+        public static int GenerateModsHash(Mod[] mods, BeatmapDifficulty difficulty, RulesetInfo ruleset)


This cannot exist. You cannot combine 50 million different flags like this into a hash function.

The most that should be done is akin to https://github.com/ppy/osu/blob/62f737d8de0944d5b278d4ab72844af6dbc67ace/osu.Game/Beatmaps/BeatmapDifficultyCache.cs#L295-L305

smoogipoo · 2024-08-31T18:10:24Z

PerformanceCalculatorGUI/Components/LazerCalculationSettings.cs

+                            },
+                            new OsuCheckbox
+                            {
+                                LabelText = "Enable Scorev1 score overwrite for legacy scores",


What does this even mean? I'm struggling to understand what this option does.

In stable for each unique mod combination only score with the highest scorev1 is calculated. This switch is turning this mechanism on and off.

What does that have to do with lazer? It doesn't sound like you've properly thought through what this PR is set to achieve and are just throwing flags around the place in the name of optimisation...

Like... why not just calculate all scores in the db all the time? If you want to only calculate the online scores then there already is a screen for that.

this is not about optimization
it's about giving user a choice between "accurate simulation" and "what if all my scores was weighted fairly"
for now it's not making big difference, but when combining with CSR - the difference in total pp can be quite big

If you want to only calculate the online scores then there already is a screen for that

online profile doesn't account for score outside of the top100 (and they can be severely buffed in reworks like CSR)
also online profile doesn't have option to calculate mods like DT rates and DA

smoogipoo · 2024-08-31T18:12:02Z

PerformanceCalculatorGUI/Screens/ProfileScreen.cs

+
+                var player = await apiManager.GetJsonFromApi<APIUser>($"users/{username}/{ruleset.Value.ShortName}");
+
+                currentUser = [player.Username, .. player.PreviousUsernames, player.Id.ToString()];


I have never seen this syntax ever before, please don't use it.

https://learn.microsoft.com/en-us/dotnet/csharp/whats-new/csharp-12#collection-expressions

Collection expressions introduce a new terse syntax to create common collection values. Inlining other collections into these values is possible using a spread element ..e.

I found spread operators pretty useful to work with in other languages so I'd be a little bummed to see them banished.

Maybe it's the usage that's confusing here, because it's assigning an array to something called "currentUser". If it were "currentUserNames" it'd probably be more understandable.

I'll trade you this for literally every other feature I've pushed over the last couple of years but get strong pushback every time because it's some newspangled wankjizz... No doubt I'm going to be the only one of us core devs who sees this feature and questions it for its one esoteric use. :(

So I'm gonna just rename currentUser?

Givikap120 added 5 commits August 13, 2024 20:14

ported from master

62294c9

Update LazerCalculationSettings.cs

8ec4ed2

fixed CI

1c81765

CI fix again

ce15637

Update ProfileScreen.cs

ed1d7f6

updated the text and disabled RX filtering

eef465b

stanriders self-requested a review August 15, 2024 08:02

Givikap120 added 2 commits August 23, 2024 02:11

fixed scorev1 simulation not working

dd2123e

added HR check

9acd859

stanriders requested changes Aug 31, 2024

View reviewed changes

removed arbitrary filtering

5f912bf

smoogipoo requested changes Aug 31, 2024

View reviewed changes

Givikap120 added 5 commits September 1, 2024 01:41

removed bindables array

a2b4f3b

changed error to unknown

6d082ac

changed hashing

417c6de

renamed currentUser

2b28193

added functionality to calculate offline profile

dc560e3


		Mod[] mods = score.Mods.Select(x => x.ToMod(rulesetInstance)).ToArray();
		if (!attributesCache.TryGetValue(modsHash, out var difficultyAttributes))


		var player = await apiManager.GetJsonFromApi<APIUser>($"users/{username}/{ruleset.Value.ShortName}");

		currentUser = [player.Username, .. player.PreviousUsernames, player.Id.ToString()];

Implemented functionality to calculate all local scores using Realm #213

Are you sure you want to change the base?

Implemented functionality to calculate all local scores using Realm #213

Conversation

Givikap120 commented Aug 13, 2024 • edited Loading

Natelytle commented Aug 13, 2024

Givikap120 commented Aug 14, 2024

smoogipoo commented Aug 15, 2024

stanriders commented Aug 15, 2024

Givikap120 commented Aug 15, 2024 • edited Loading

minisbett commented Aug 31, 2024

stanriders commented Aug 31, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stanriders Aug 31, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Givikap120 Aug 31, 2024 • edited Loading

Choose a reason for hiding this comment

Givikap120 Aug 31, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Givikap120 Aug 31, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Givikap120 Sep 1, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

minisbett commented Aug 31, 2024

Givikap120 commented Aug 31, 2024

smoogipoo left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

smoogipoo Aug 31, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

smoogipoo Sep 1, 2024 • edited Loading

Choose a reason for hiding this comment

Givikap120 Sep 1, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Givikap120 commented Aug 13, 2024 •

edited

Loading

Givikap120 commented Aug 15, 2024 •

edited

Loading

stanriders Aug 31, 2024 •

edited

Loading

Givikap120 Aug 31, 2024 •

edited

Loading

Givikap120 Aug 31, 2024 •

edited

Loading

Givikap120 Aug 31, 2024 •

edited

Loading

Givikap120 Sep 1, 2024 •

edited

Loading

smoogipoo left a comment •

edited

Loading

smoogipoo Aug 31, 2024 •

edited

Loading

smoogipoo Sep 1, 2024 •

edited

Loading

Givikap120 Sep 1, 2024 •

edited

Loading