Introduce `UserStatisticsProvider` component and add support for respecting selected ruleset #27128

frenzibyte · 2024-02-11T05:35:02Z

Closes Rank on player card don't show the other rulesets #26802
Depends on full-stack testing

I was initially leaning towards rewriting SoloStatisticsWatcher to become a general-purpose component surrounding around providing user statistics, but after brief discussion yesterday, I've settled on making a separate component for that instead, to keep complexity of each component to a minimum.

osu.Game/OsuGameBase.cs

bdach · 2024-02-12T16:33:08Z

osu.Game/Online/LocalUserStatisticsProvider.cs

+        [Resolved]
+        private IBindable<RulesetInfo> ruleset { get; set; } = null!;


i don't get what's with the insistence of having this bindable. i'd rather it didn't exist and this component didn't do any "ruleset tracking".

#27128 (comment) is relevant.

I don't really think that comment is relevant. The question here is more of an architectural one no? Like you can still cache the per-ruleset statistics, but instead of exposing the ruleset-specific bindable it would be up to the consumers (DiscordRichPresence and UserRankPanel).

That said, I think I'm okay with it existing as it is. At least until a use case comes up where we want to retrieve this for something that isn't the current global ruleset. Which arguable exists – the single usage you've piped through GetStatisticsFor. Is there a reason this flow doesn't do the online retrieval?

I will say it does feel weird having this exist alongside UserStatisticsWatcher with data being awkwardly ferried between the two. Like they are both basically doing the same thing and the cross-communication is only required to keep the new component in sync?

For the sake of making each component have not but a single purpose, LocalUserStatisticsProvider and UserStatisticsWatcher have been split. The first exposes local user statistics to the game codebase, while the second has a very specific purpose tied to score submission which is to listen to submission events and display a change in user statistics while updating the former component with the new statistics.

I can understand that there's still a level of awkwardness between the two components since they both look up statistics online, and I wouldn't mind merging them together, but I would have to hear @bdach's opinion on that since he doesn't like that to be the case.

I'm more focused on the fact that you added cross-talk between them, for the sake of saving one request I guess? If we're keeping both, I'd see that being removed.

The cross talk must exist for score submission to update the statistics exposed by LocalUserStatisticsProvider, the only way to avoid that is merging both components together.

@bdach Can I get your five minutes attention on the current state of this PR and make sure you're still happy with the direction as it stands? The "required" cross-talk here makes things more complicated than I'd hope, and while I haven't looked into whether this can be improved or removed, I'd like your take on it.

My position on this is unchanged: I would like the bindable to be gone.

The reasoning is extremely simple: when faced between two choices, one of which introduces ambient global state, and one of which does not, always choose the one that does not introduce the ambient global state unless you absolutely have to. It is simpler.

As I see it, LocalUserStatisticsProvider should be a dumb lookup facilitator / a cache. Same way something like DifficultyCache is. It should expose a dictionary for anyone to use in any way they see fit. Keyed by (userId, rulesetId). Maybe it could even expose bindables, like DifficultyCache does, if you need that.

As for the interaction with UserStatisticsWatcher - LocalUserStatisticsProvider should expose a method to invalidate and refetch data for a given cache key, and UserStatisticsWatcher should use that to retrieve its rank update.

Sounds good, will apply as proposed.

bdach · 2024-02-12T16:34:09Z

osu.Game/Online/LocalUserStatisticsProvider.cs

+        private void requestStatistics(RulesetInfo ruleset)
+        {
+            currentRequest = new GetUserRequest(api.LocalUser.Value.OnlineID, ruleset);
+            currentRequest.Success += u => statistics.Value = allStatistics[ruleset.ShortName] = u.Statistics;
+            api.Queue(currentRequest);
+        }


both SoloStatisticsWatcher and DifficultyRecommender do initial population of statistics for their own uses for all rulesets. so why is this doing the same in a third place?

this is why i was floating potentially exposing the internal dictionary from SoloStatisticsWatcher.

I did not emphasise too much about this, but I have mentioned in OP the following:

This could be improved by fetching statistics of all rulesets once the user is logged in, but it would require performing four consecutive GetUserRequests for each ruleset to provide the statistics in full detail (i.e. when performing a GetUsersRequest, the RulesetsStatistics dictionary does not provide information about the country ranking in each ruleset).

SoloStatisticsWatcher is initialising user statistics for all rulesets using a GetUsersRequest, the response of that request does not include country ranking and potentially other attributes.

If anything, I would foresee this new component exposing statistics for all rulesets by a method or a dictionary, and DifficultyRecommender turning into an extension method, and SoloStatisticsWatcher becoming completely dependent on LocalUserStatisticsProvider and do away with the API request logic in the watcher.

However, right now it's sort-of not pretty to achieve that, as I have to make the component perform one-time four consecutive API requests to receive full user statistics on each ruleset, and in complete honestly, I have no idea whether this is something that you would be fine with or not.

at this point i'd probably ask if it is possible for us to receive full statistics in a single request because this feels wasteful.

@ppy/team-web do you think we could change some user-fetching endpoint to return complete rank data for all rulesets in one request, including country ranking?

country ranking is rather expensive

both SoloStatisticsWatcher and DifficultyRecommender do initial population of statistics for their own uses for all rulesets. so why is this doing the same in a third place?

this is why i was floating potentially exposing the internal dictionary from SoloStatisticsWatcher.

Looking back at this, since the purpose of LocalUserStatisticsProvider is to track latest statistics and provide the latest one, I've made UserStatisticsWatcher fully rely on it and remove its local part of fetching initial statistics and tracking statistics in general.

Joehuu · 2024-08-29T04:56:25Z

Whenever this gets updated again, it should also be applied to discord rich presence as the rank is only fetched at game start / login and never updated.

peppy · 2024-09-30T10:04:34Z

@frenzibyte whenever you get back into the flow it might be good to revisit this one (and at least fix conflicts).

…pendency optional

frenzibyte · 2024-10-25T07:47:08Z

Whenever this gets updated again, it should also be applied to discord rich presence as the rank is only fetched at game start / login and never updated.

Should be addressed in 663b769

frenzibyte · 2024-10-27T03:11:35Z

Tested changes against a local full stack, works as expected.

…Provider` This also throws away the logic of updating `API.LocalUser.Value.Statistics`. Components should rely on `LocalUserStatisticsProvider` instead for proper behaviour and ability to update on statistics updates.

osu.Game/Online/UserStatisticsWatcher.cs

bdach · 2024-11-19T10:15:29Z

osu.Game/Online/API/Requests/Responses/APIUser.cs

+        /// <remarks>
+        /// This returns null when accessed from <see cref="IAPIProvider.LocalUser"/>. Use <see cref="LocalUserStatisticsProvider"/> instead.
+        /// </remarks>


oh how i love this model class (heavy sarcasm)

bdach · 2024-11-19T10:16:11Z

osu.Game/Online/API/IAPIProvider.cs

-        /// <summary>
-        /// The current user's online statistics.
-        /// </summary>
-        IBindable<UserStatistics?> Statistics { get; }


For one, very happy to see this gone from here

Also no longer cancels previous API requests as there's no actual need to do it.

bdach

added one precautionary guard (0a3f3c3) but otherwise seems fine

peppy · 2024-11-25T14:07:36Z

Will take one final look at this.

peppy · 2024-11-26T03:35:55Z

Statistics don't load for me from a fresh startup:

Is getting here:

peppy · 2024-11-26T03:51:07Z

Here's the failing flow:

[runtime] 2024-11-26 03:48:47 [verbose]: STATS: update from 1 to 2
[runtime] 2024-11-26 03:48:47 [verbose]: STATS: init
[runtime] 2024-11-26 03:48:47 [verbose]: STATS: refreshed
[network] 2024-11-26 03:48:47 [verbose]: Failing request osu.Game.Online.API.Requests.GetUserRequest (System.Net.WebException: User not logged in)
[network] 2024-11-26 03:48:47 [verbose]: Failing request osu.Game.Online.API.Requests.GetUserRequest (System.Net.WebException: User not logged in)
[network] 2024-11-26 03:48:47 [verbose]: Failing request osu.Game.Online.API.Requests.GetUserRequest (System.Net.WebException: User not logged in)
[network] 2024-11-26 03:48:47 [verbose]: Failing request osu.Game.Online.API.Requests.GetUserRequest (System.Net.WebException: User not logged in)

Should be able to reproduce by delaying login completion:

diff --git a/osu.Game/Online/API/APIAccess.cs b/osu.Game/Online/API/APIAccess.cs
index ec48fa2436..9ea4a862e0 100644
--- a/osu.Game/Online/API/APIAccess.cs
+++ b/osu.Game/Online/API/APIAccess.cs
@@ -259,6 +259,8 @@ private void attemptConnect()
                 });
             }
 
+            Thread.Sleep(10000);
+
             // save the username at this point, if the user requested for it to be.
             config.SetValue(OsuSetting.Username, config.Get<bool>(OsuSetting.SaveUsername) ? ProvidedUsername : string.Empty);

Here's a fix at APIAccess level...

diff --git a/osu.Game/Online/API/APIAccess.cs b/osu.Game/Online/API/APIAccess.cs
index ec48fa2436..45daaaf046 100644
--- a/osu.Game/Online/API/APIAccess.cs
+++ b/osu.Game/Online/API/APIAccess.cs
@@ -339,12 +339,11 @@ private void attemptConnect()
 
                     userReq.Success += me =>
                     {
-                        me.Status.Value = configStatus.Value ?? UserStatus.Online;
-
-                        setLocalUser(me);
-
                         state.Value = me.SessionVerified ? APIState.Online : APIState.RequiresSecondFactorAuth;
                         failureCount = 0;
+
+                        me.Status.Value = configStatus.Value ?? UserStatus.Online;
+                        setLocalUser(me);
                     };
 
                     if (!handleRequest(userReq))

This does fix this usage, but also may break others things, ie. if anything binds to State changes and expects LocalUser to be updated with the final user. Chicken-egg scenario with these two bindables.

The new component likely wants to be checking State rather than LocalUser. I think most other components do this. I'm not immediately sure how to avoid this clumsy failure from happening again in the future though..

peppy

🙈

frenzibyte · 2024-11-26T06:14:41Z

Sounds like the perfect solution would be to schedule. If the status is updated directly after the local user is updated, then all that should be done is to just schedule the callback so enough time is passed for other states to be updated.

peppy · 2024-11-26T06:17:55Z

Sounds like the Perfect Isolated Hacky Fix.

frenzibyte · 2024-11-26T06:21:34Z

Oh yeah sure let's reinvent the whole IAPIProvider system...; I'll push a schedule fix. It's already done in UserStatisticsWatcher matter of fact.

Queuing up requests on change to `api.LocalUser` is bad because the API state is updated after `LocalUser` is updated, therefore we have to schhhhhedullllllllleeeeeeeeeeeeeeee.

peppy · 2024-11-26T06:26:47Z

If you're adding a schedule you better have inline documentation explaining why, so the next person who actually wants to fix the issue knows the context.

peppy · 2024-11-26T09:22:25Z

@bdach please check you're okay with the schedule usage.

bdach · 2024-11-26T11:08:14Z

I can live with the schedule. It's a hacky fix but at least it's isolated and it'd be nice to actually get this PR off the list one way or another after 9 months.

It's already done in UserStatisticsWatcher matter of fact.

Baffled by this remark though, because the reason why that component schedules is completely different. There it's for basic thread safety. The primary usage of the watcher flow is in online score submission, which is async. That's not working around bindable idiosyncrasies like this is.

frenzibyte · 2024-11-26T18:13:00Z

It's already done in UserStatisticsWatcher matter of fact.

Baffled by this remark though, because the reason why that component schedules is completely different. There it's for basic thread safety. The primary usage of the watcher flow is in online score submission, which is async. That's not working around bindable idiosyncrasies like this is.

No? Check master, I'm referring to the schedule in onUserChanged, that method is used nowhere other than the api.LocalUser bindable callback.

bdach · 2024-11-26T21:07:06Z

I'm referring to the schedule in onUserChanged,

My bad I guess. I don't even remember why that one is there, and I wrote that code. I would like to think I would have mentioned if the reason for the schedule was the same in a comment, or I guess past me just sucks 🤷

frenzibyte added 3 commits February 11, 2024 08:22

Introduce LocalUserStatisticsProvider component

91fb59e

Remove IAPIProvider.Statistics in favour of the new component

3ab60b7

Update UserRankPanel implementation to use new component

633d854

frenzibyte added type:online type:behavioural labels Feb 11, 2024

pull-request-size bot added the size/L label Feb 11, 2024

frenzibyte added 2 commits February 11, 2024 11:16

Fix ImportTest.TestOsuGameBase having null ruleset

bc2b705

Fix TestSceneUserPanel tests failing

11b3fa8

bdach reviewed Feb 12, 2024

View reviewed changes

This was referenced Feb 12, 2024

Add highest rank tooltip to global rank display #27107

Merged

Add pp and playtime to the UserRankPanel #27147

Open

Watch online statistics changes after every play & display them in toolbar #27156

Merged

frenzibyte added 6 commits October 25, 2024 01:35

Merge branch 'master' into user-statistics-provider

701fb56

Fix post-merge errors

2fd4952

Move LocalUserStatisticsProvider to non-base game class and make de…

3a57b21

…pendency optional

Make UserStatisticsWatcher fully rely on LocalUserStatisticsProvider

44dd813

Merge branch 'master' into user-statistics-provider

fdeb8b9

Update DiscordRichPresence to use new statistics provider component

663b769

Reorder code slightly

979065c

peppy self-requested a review November 13, 2024 06:19

peppy and others added 5 commits November 13, 2024 15:21

Merge branch 'master' into user-statistics-provider

0760451

Merge branch 'master' into user-statistics-provider

6c8a900

Make DifficultyRecommender rely on the statistics provider

28f8740

Fix UserRankPanel not updating on ruleset change

07609b6

pull-request-size bot removed the size/L label Nov 17, 2024

Fix various test failures

caf56af

frenzibyte force-pushed the user-statistics-provider branch from 1b0a5f9 to caf56af Compare November 18, 2024 00:13

frenzibyte added 3 commits November 17, 2024 20:36

Fix more test / component breakage

b106833

Replace bindable with an event

74daf85

Handle logged out user

0b52080

bdach reviewed Nov 19, 2024

View reviewed changes

frenzibyte and others added 4 commits November 24, 2024 04:11

Replace event subscription with callback in UserStatisticsWatcher

631bfad

Also no longer cancels previous API requests as there's no actual need to do it.

Enable NRT and fix code

aa1358b

Fix failing test

53b3906

Add guard against fetching statistics for non-legacy rulesets

0a3f3c3

bdach previously approved these changes Nov 25, 2024

View reviewed changes

peppy requested changes Nov 26, 2024

View reviewed changes

Schedule the thing

b76460f

Queuing up requests on change to `api.LocalUser` is bad because the API state is updated after `LocalUser` is updated, therefore we have to schhhhhedullllllllleeeeeeeeeeeeeeee.

frenzibyte dismissed bdach’s stale review via b76460f November 26, 2024 06:26

Add inline comment

42c68ba

peppy approved these changes Nov 26, 2024

View reviewed changes

peppy mentioned this pull request Nov 27, 2024

Show results in results screen based on leaderboard selected in song select #27861

Open

peppy merged commit 573aaf6 into ppy:master Nov 27, 2024
7 of 10 checks passed

frenzibyte deleted the user-statistics-provider branch November 27, 2024 04:25

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce `UserStatisticsProvider` component and add support for respecting selected ruleset #27128

Introduce `UserStatisticsProvider` component and add support for respecting selected ruleset #27128

frenzibyte commented Feb 11, 2024 •

edited

Loading

bdach Feb 12, 2024

frenzibyte Feb 12, 2024

peppy Nov 13, 2024

peppy Nov 13, 2024

frenzibyte Nov 13, 2024

peppy Nov 13, 2024

frenzibyte Nov 13, 2024

peppy Nov 14, 2024

bdach Nov 14, 2024 •

edited

Loading

frenzibyte Nov 17, 2024

bdach Feb 12, 2024

frenzibyte Feb 12, 2024

bdach Feb 12, 2024 •

edited

Loading

nanaya Feb 12, 2024

frenzibyte Oct 25, 2024

Joehuu commented Aug 29, 2024

peppy commented Sep 30, 2024

frenzibyte commented Oct 25, 2024

frenzibyte commented Oct 27, 2024

bdach Nov 19, 2024

bdach Nov 19, 2024

bdach left a comment

peppy commented Nov 25, 2024

peppy commented Nov 26, 2024 •

edited

Loading

peppy commented Nov 26, 2024 •

edited

Loading

peppy left a comment

frenzibyte commented Nov 26, 2024

peppy commented Nov 26, 2024

frenzibyte commented Nov 26, 2024

peppy commented Nov 26, 2024

peppy commented Nov 26, 2024

bdach commented Nov 26, 2024

frenzibyte commented Nov 26, 2024

bdach commented Nov 26, 2024 •

edited

Loading

		[Resolved]
		private IBindable<RulesetInfo> ruleset { get; set; } = null!;

Introduce UserStatisticsProvider component and add support for respecting selected ruleset #27128

Introduce UserStatisticsProvider component and add support for respecting selected ruleset #27128

Conversation

frenzibyte commented Feb 11, 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

bdach Nov 14, 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

bdach Feb 12, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Joehuu commented Aug 29, 2024

peppy commented Sep 30, 2024

frenzibyte commented Oct 25, 2024

frenzibyte commented Oct 27, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bdach left a comment

Choose a reason for hiding this comment

peppy commented Nov 25, 2024

peppy commented Nov 26, 2024 • edited Loading

peppy commented Nov 26, 2024 • edited Loading

peppy left a comment

Choose a reason for hiding this comment

frenzibyte commented Nov 26, 2024

peppy commented Nov 26, 2024

frenzibyte commented Nov 26, 2024

peppy commented Nov 26, 2024

peppy commented Nov 26, 2024

bdach commented Nov 26, 2024

frenzibyte commented Nov 26, 2024

bdach commented Nov 26, 2024 • edited Loading

Introduce `UserStatisticsProvider` component and add support for respecting selected ruleset #27128

Introduce `UserStatisticsProvider` component and add support for respecting selected ruleset #27128

frenzibyte commented Feb 11, 2024 •

edited

Loading

bdach Nov 14, 2024 •

edited

Loading

bdach Feb 12, 2024 •

edited

Loading

peppy commented Nov 26, 2024 •

edited

Loading

peppy commented Nov 26, 2024 •

edited

Loading

bdach commented Nov 26, 2024 •

edited

Loading