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

Show more comprehensive background processing progress notifications #25832

Merged

Conversation

peppy
Copy link
Member

@peppy peppy commented Dec 18, 2023

Addresses the visibility part of #25824. Will attempt to reduce the side-effects at song select separately.

CleanShot.2023-12-18.at.09.43.29.mp4

@peppy peppy force-pushed the more-background-processing-progress-notifications branch from 0e9e38b to 5ab3815 Compare December 18, 2023 09:45
@peppy peppy added the next release Pull requests which are almost there. We'll aim to get them in the next release, but no guarantees! label Dec 18, 2023
if (notification == null)
return;

notification.Text = notification.Text.ToString().Split('(').First().TrimEnd() + $" ({processedCount} of {totalCount})";
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is a bit of a crime but i'm not gonna split hairs for now as none of this is correctly localisable anyways (no pluralisation support)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

split hairs 😎

yeah i dunno, it's isolated and can be refactored if/when we manage to localise this.

@bdach
Copy link
Collaborator

bdach commented Dec 18, 2023

The above is a nit, but the bigger problem here is the tests. They are presumably failing because these notifications will now show up on a blank install.

In fact, this is what shows up on a completely clean lazer environment (after rm -rfing the data directory):

osu_2023-12-18_12-05-59

This is because the processing is running on the fake "triangles" beatmap.

We probably don't want that. One thing that could be done is this:

diff --git a/osu.Game/BackgroundDataStoreProcessor.cs b/osu.Game/BackgroundDataStoreProcessor.cs
index 55be7f2c9e..9d2a719957 100644
--- a/osu.Game/BackgroundDataStoreProcessor.cs
+++ b/osu.Game/BackgroundDataStoreProcessor.cs
@@ -135,12 +135,12 @@ private void processBeatmapSetsWithMissingMetrics()
                 // of other possible ways), but for now avoid queueing if the user isn't logged in at startup.
                 if (api.IsLoggedIn)
                 {
-                    foreach (var b in r.All<BeatmapInfo>().Where(b => (b.StarRating < 0 || (b.OnlineID > 0 && b.LastOnlineUpdate == null)) && b.BeatmapSet != null))
+                    foreach (var b in r.All<BeatmapInfo>().Where(b => (b.StarRating < 0 || (b.OnlineID > 0 && b.LastOnlineUpdate == null)) && b.BeatmapSet != null && !b.BeatmapSet.Protected))
                         beatmapSetIds.Add(b.BeatmapSet!.ID);
                 }
                 else
                 {
-                    foreach (var b in r.All<BeatmapInfo>().Where(b => b.StarRating < 0 && b.BeatmapSet != null))
+                    foreach (var b in r.All<BeatmapInfo>().Where(b => b.StarRating < 0 && b.BeatmapSet != null && !b.BeatmapSet.Protected))
                         beatmapSetIds.Add(b.BeatmapSet!.ID);
                 }
             });
@@ -196,7 +196,7 @@ private void processBeatmapsWithMissingObjectCounts()
 
             realmAccess.Run(r =>
             {
-                foreach (var b in r.All<BeatmapInfo>().Where(b => b.TotalObjectCount == 0))
+                foreach (var b in r.All<BeatmapInfo>().Where(b => b.TotalObjectCount == 0 && b.BeatmapSet != null && !b.BeatmapSet.Protected))
                     beatmapIds.Add(b.ID);
             });
 

but I don't know...

@peppy
Copy link
Member Author

peppy commented Dec 18, 2023

I've applied the proposal in a bit of a more central way, good catch.

@peppy peppy force-pushed the more-background-processing-progress-notifications branch from 704c203 to 9aaaa12 Compare December 18, 2023 15:01
@peppy
Copy link
Member Author

peppy commented Dec 18, 2023

Turns out that solution plays pretty bad because IQueryable doesn't work well on children objects. So I've taken the alternative approach of only showing notifications when there's enough items for it to matter.

@bdach bdach merged commit 8e8d9b2 into ppy:master Dec 18, 2023
15 of 17 checks passed
@peppy peppy deleted the more-background-processing-progress-notifications branch December 19, 2023 14:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
next release Pull requests which are almost there. We'll aim to get them in the next release, but no guarantees! size/L type:behavioural
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants