Introduce UserStatisticsProvider component and add support for respecting selected ruleset
#27128
Conversation
| [Resolved] | ||
| private IBindable<RulesetInfo> ruleset { get; set; } = null!; |
There was a problem hiding this comment.
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".
There was a problem hiding this comment.
#27128 (comment) is relevant.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
@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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Sounds good, will apply as proposed.
| 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); | ||
| } |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
both
SoloStatisticsWatcherandDifficultyRecommenderdo 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.
|
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. |
|
@frenzibyte whenever you get back into the flow it might be good to revisit this one (and at least fix conflicts). |
…pendency optional
Should be addressed in 663b769 |
|
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.
frenzibyte
force-pushed
the
user-statistics-provider
branch
from
1b0a5f9
to
caf56af
Compare
| /// <remarks> | ||
| /// This returns null when accessed from <see cref="IAPIProvider.LocalUser"/>. Use <see cref="LocalUserStatisticsProvider"/> instead. | ||
| /// </remarks> |
There was a problem hiding this comment.
oh how i love this model class (heavy sarcasm)
| /// <summary> | ||
| /// The current user's online statistics. | ||
| /// </summary> | ||
| IBindable<UserStatistics?> Statistics { get; } |
There was a problem hiding this comment.
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.
|
Will take one final look at this. |
|
Statistics don't load for me from a fresh startup:
Is getting here:
|
|
Here's the failing flow: [runtime] 2024-11-26 03:48:47 [verbose]: STATS: update from 1 to 2 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 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 The new component likely wants to be checking |
|
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. |
|
Sounds like the Perfect Isolated Hacky Fix. |
|
Oh yeah sure let's reinvent the whole |
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.
|
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. |
|
@bdach please check you're okay with the schedule usage. |
|
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.
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 |
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 🤷 |
I was initially leaning towards rewriting
SoloStatisticsWatcherto 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.