From 043235fed25bf00eadc42caa0d2611dfbc86cdd8 Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Wed, 27 Aug 2025 18:03:37 +0900 Subject: [PATCH 1/5] Add test coverage ensuring filtering does not occur on unnecessary updates --- .../TestSceneBeatmapCarouselUpdateHandling.cs | 81 +++++++++++++++++-- osu.Game/Graphics/Carousel/Carousel.cs | 2 +- 2 files changed, 75 insertions(+), 8 deletions(-) diff --git a/osu.Game.Tests/Visual/SongSelectV2/TestSceneBeatmapCarouselUpdateHandling.cs b/osu.Game.Tests/Visual/SongSelectV2/TestSceneBeatmapCarouselUpdateHandling.cs index eb610a40f1..3638c8eeec 100644 --- a/osu.Game.Tests/Visual/SongSelectV2/TestSceneBeatmapCarouselUpdateHandling.cs +++ b/osu.Game.Tests/Visual/SongSelectV2/TestSceneBeatmapCarouselUpdateHandling.cs @@ -22,12 +22,16 @@ namespace osu.Game.Tests.Visual.SongSelectV2 { private BeatmapSetInfo baseTestBeatmap = null!; + private const int initial_filter_count = 3; + [SetUpSteps] public void SetUpSteps() { RemoveAllBeatmaps(); CreateCarousel(); + WaitForFiltering(); AddBeatmaps(1, 3); + WaitForFiltering(); AddStep("generate and add test beatmap", () => { baseTestBeatmap = TestResources.CreateTestBeatmapSetInfo(3); @@ -42,8 +46,9 @@ namespace osu.Game.Tests.Visual.SongSelectV2 b.Metadata = metadata; BeatmapSets.Add(baseTestBeatmap); }); - WaitForFiltering(); + + AddAssert("filter count correct", () => Carousel.FilterCount, () => Is.EqualTo(initial_filter_count)); } [Test] @@ -81,12 +86,18 @@ namespace osu.Game.Tests.Visual.SongSelectV2 AddUntilStep("is scrolled to end", () => Carousel.ChildrenOfType().Single().IsScrolledToEnd()); - updateBeatmap(b => b.Metadata = new BeatmapMetadata + updateBeatmap(b => { - Artist = "updated test", - Title = $"beatmap {RNG.Next().ToString()}" + // hash will be updated when important metadata changes, such as title, difficulty, author etc. + b.Hash = "new hash"; + b.Metadata = new BeatmapMetadata + { + Artist = "updated test", + Title = $"beatmap {RNG.Next().ToString()}" + }; }); + assertDidFilter(); WaitForFiltering(); AddAssert("scroll is still at end", () => Carousel.ChildrenOfType().Single().IsScrolledToEnd()); @@ -113,8 +124,14 @@ namespace osu.Game.Tests.Visual.SongSelectV2 AddStep("find panel", () => panel = Carousel.ChildrenOfType().Single(p => p.ChildrenOfType().Any(t => t.Text.ToString() == "beatmap"))); - updateBeatmap(b => b.Metadata = metadata); + updateBeatmap(b => + { + b.Metadata = metadata; + // hash will be updated when important metadata changes, such as title, difficulty, author etc. + b.Hash = "new hash"; + }); + assertDidFilter(); WaitForFiltering(); AddAssert("drawables unchanged", () => Carousel.ChildrenOfType(), () => Is.EqualTo(originalDrawables)); @@ -123,7 +140,41 @@ namespace osu.Game.Tests.Visual.SongSelectV2 } [Test] - public void TestSelectionHeld() + public void TestOnlineStatusUpdated() + { + List originalDrawables = new List(); + + AddStep("store drawable references", () => + { + originalDrawables.Clear(); + originalDrawables.AddRange(Carousel.ChildrenOfType().ToList()); + }); + + updateBeatmap(b => b.Status = BeatmapOnlineStatus.Graveyard); + + assertDidFilter(); + WaitForFiltering(); + + AddAssert("drawables unchanged", () => Carousel.ChildrenOfType(), () => Is.EqualTo(originalDrawables)); + } + + [Test] + public void TestNoUpdateTriggeredOnUserTagsChange() + { + var metadata = new BeatmapMetadata + { + Artist = "updated test", + Title = "new beatmap title", + UserTags = { "hi" } + }; + + updateBeatmap(b => b.Metadata = metadata); + assertDidNotFilter(); + } + + [TestCase(false)] + [TestCase(true)] + public void TestSelectionHeld(bool hashChanged) { SelectNextSet(); @@ -131,7 +182,17 @@ namespace osu.Game.Tests.Visual.SongSelectV2 AddAssert("selection is updateable beatmap", () => Carousel.CurrentSelection, () => Is.EqualTo(baseTestBeatmap.Beatmaps[0])); AddAssert("visible panel is updateable beatmap", () => GetSelectedPanel()?.Item?.Model, () => Is.EqualTo(baseTestBeatmap.Beatmaps[0])); - updateBeatmap(); + updateBeatmap(b => + { + if (hashChanged) + b.Hash = "new hash"; + }); + + if (hashChanged) + assertDidFilter(); + else + assertDidNotFilter(); + WaitForFiltering(); AddAssert("selection is updateable beatmap", () => Carousel.CurrentSelection, () => Is.EqualTo(baseTestBeatmap.Beatmaps[0])); @@ -148,6 +209,7 @@ namespace osu.Game.Tests.Visual.SongSelectV2 AddAssert("visible panel is updateable beatmap", () => GetSelectedPanel()?.Item?.Model, () => Is.EqualTo(baseTestBeatmap.Beatmaps[0])); updateBeatmap(b => b.DifficultyName = "new name"); + assertDidFilter(); WaitForFiltering(); AddAssert("selection is updateable beatmap", () => Carousel.CurrentSelection, () => Is.EqualTo(baseTestBeatmap.Beatmaps[0])); @@ -164,6 +226,7 @@ namespace osu.Game.Tests.Visual.SongSelectV2 AddAssert("visible panel is updateable beatmap", () => GetSelectedPanel()?.Item?.Model, () => Is.EqualTo(baseTestBeatmap.Beatmaps[0])); updateBeatmap(b => b.OnlineID = b.OnlineID + 1); + assertDidFilter(); WaitForFiltering(); AddAssert("selection is updateable beatmap", () => Carousel.CurrentSelection, () => Is.EqualTo(baseTestBeatmap.Beatmaps[0])); @@ -339,6 +402,10 @@ namespace osu.Game.Tests.Visual.SongSelectV2 AddAssert("Order didn't change", () => Carousel.PostFilterBeatmaps.Select(b => b.ID), () => Is.EqualTo(originalOrder)); } + private void assertDidFilter() => AddAssert("did filter", () => Carousel.FilterCount, () => Is.EqualTo(initial_filter_count + 1)); + + private void assertDidNotFilter() => AddAssert("did not filter", () => Carousel.FilterCount, () => Is.EqualTo(initial_filter_count)); + private void updateBeatmap(Action? updateBeatmap = null, Action? updateSet = null) { AddStep("update beatmap with different reference", () => diff --git a/osu.Game/Graphics/Carousel/Carousel.cs b/osu.Game/Graphics/Carousel/Carousel.cs index 7b5aea08b6..c9a3c7f723 100644 --- a/osu.Game/Graphics/Carousel/Carousel.cs +++ b/osu.Game/Graphics/Carousel/Carousel.cs @@ -79,7 +79,7 @@ namespace osu.Game.Graphics.Carousel /// /// The number of times filter operations have been triggered. /// - internal int FilterCount { get; private set; } + public int FilterCount { get; private set; } /// /// The number of displayable items currently being tracked (before filtering). From 0e57ee9ba68f0fb7b58b8f11efd4611ff4e9d936 Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Wed, 27 Aug 2025 18:04:19 +0900 Subject: [PATCH 2/5] Avoid triggering changes when add operations are empty Only seems to happen in tests. I think. --- osu.Game/Screens/SelectV2/BeatmapCarousel.cs | 3 +++ 1 file changed, 3 insertions(+) diff --git a/osu.Game/Screens/SelectV2/BeatmapCarousel.cs b/osu.Game/Screens/SelectV2/BeatmapCarousel.cs index da841aa361..10ce578562 100644 --- a/osu.Game/Screens/SelectV2/BeatmapCarousel.cs +++ b/osu.Game/Screens/SelectV2/BeatmapCarousel.cs @@ -143,6 +143,9 @@ namespace osu.Game.Screens.SelectV2 switch (changed.Action) { case NotifyCollectionChangedAction.Add: + if (!newItems!.Any()) + return; + Items.AddRange(newItems!.SelectMany(s => s.Beatmaps)); break; From be6fb9aa776f83e1b9560329f6a5033a5b1476b2 Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Wed, 27 Aug 2025 18:04:58 +0900 Subject: [PATCH 3/5] Fix beatmap carousel re-filtering when it doesn't need to Local rules ensure we only handle callbacks when we need to. --- osu.Game/Graphics/Carousel/Carousel.cs | 13 +++++- osu.Game/Screens/SelectV2/BeatmapCarousel.cs | 46 ++++++++++++++++++++ 2 files changed, 58 insertions(+), 1 deletion(-) diff --git a/osu.Game/Graphics/Carousel/Carousel.cs b/osu.Game/Graphics/Carousel/Carousel.cs index c9a3c7f723..b81df0a7eb 100644 --- a/osu.Game/Graphics/Carousel/Carousel.cs +++ b/osu.Game/Graphics/Carousel/Carousel.cs @@ -3,6 +3,7 @@ using System; using System.Collections.Generic; +using System.Collections.Specialized; using System.Diagnostics; using System.Linq; using System.Threading; @@ -210,6 +211,12 @@ namespace osu.Game.Graphics.Carousel return filterTask; } + /// + /// Called when changes in any way. + /// + /// Whether a re-filter is required. + protected virtual bool HandleItemsChanged(NotifyCollectionChangedEventArgs args) => true; + /// /// Fired after a filter operation completed. /// @@ -301,7 +308,11 @@ namespace osu.Game.Graphics.Carousel RelativeSizeAxes = Axes.Both, }; - Items.BindCollectionChanged((_, _) => filterAfterItemsChanged.Invalidate()); + Items.BindCollectionChanged((_, args) => + { + if (HandleItemsChanged(args)) + filterAfterItemsChanged.Invalidate(); + }); } [BackgroundDependencyLoader] diff --git a/osu.Game/Screens/SelectV2/BeatmapCarousel.cs b/osu.Game/Screens/SelectV2/BeatmapCarousel.cs index 10ce578562..ad691d34c0 100644 --- a/osu.Game/Screens/SelectV2/BeatmapCarousel.cs +++ b/osu.Game/Screens/SelectV2/BeatmapCarousel.cs @@ -356,6 +356,52 @@ namespace osu.Game.Screens.SelectV2 } } + protected override bool HandleItemsChanged(NotifyCollectionChangedEventArgs args) + { + switch (args.Action) + { + case NotifyCollectionChangedAction.Add: + case NotifyCollectionChangedAction.Remove: + case NotifyCollectionChangedAction.Move: + case NotifyCollectionChangedAction.Reset: + return true; + + case NotifyCollectionChangedAction.Replace: + var oldBeatmaps = args.OldItems!.OfType().ToList(); + var newBeatmaps = args.NewItems!.OfType().ToList(); + + for (int i = 0; i < oldBeatmaps.Count; i++) + { + var oldBeatmap = oldBeatmaps[i]; + var newBeatmap = newBeatmaps[i]; + + // Ignore changes which don't concern us. + // + // Here are some examples of things that can go wrong: + // - Background difficulty calculation runs and causes a realm update. + // We use `BeatmapDifficultyCache` and don't want to know about these. + // - Background user tag population runs and causes a realm update. + // We don't display user tags so want to ignore this. + if ( + // covers metadata changes + oldBeatmap.Hash == newBeatmap.Hash && + // displayed + oldBeatmap.Status == newBeatmap.Status && + // displayed + oldBeatmap.DifficultyName == newBeatmap.DifficultyName && + // sanity + oldBeatmap.OnlineID == newBeatmap.OnlineID + ) + return false; + } + + return true; + + default: + throw new ArgumentOutOfRangeException(); + } + } + protected override void HandleFilterCompleted() { base.HandleFilterCompleted(); From 8a6c8577192e61aa475be2e461a0529d2501fbfd Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Thu, 28 Aug 2025 02:33:18 +0900 Subject: [PATCH 4/5] Fix hidden beatmap state not being reflected immediately --- osu.Game/Screens/SelectV2/BeatmapCarousel.cs | 15 +++++++++------ 1 file changed, 9 insertions(+), 6 deletions(-) diff --git a/osu.Game/Screens/SelectV2/BeatmapCarousel.cs b/osu.Game/Screens/SelectV2/BeatmapCarousel.cs index ad691d34c0..5b0ad1ae29 100644 --- a/osu.Game/Screens/SelectV2/BeatmapCarousel.cs +++ b/osu.Game/Screens/SelectV2/BeatmapCarousel.cs @@ -382,16 +382,19 @@ namespace osu.Game.Screens.SelectV2 // We use `BeatmapDifficultyCache` and don't want to know about these. // - Background user tag population runs and causes a realm update. // We don't display user tags so want to ignore this. - if ( + bool equalForDisplayPurposes = // covers metadata changes oldBeatmap.Hash == newBeatmap.Hash && - // displayed + // sanity check + oldBeatmap.OnlineID == newBeatmap.OnlineID && + // displayed on panel oldBeatmap.Status == newBeatmap.Status && - // displayed + // displayed on panel oldBeatmap.DifficultyName == newBeatmap.DifficultyName && - // sanity - oldBeatmap.OnlineID == newBeatmap.OnlineID - ) + // hidden changed, needs re-filter + oldBeatmap.Hidden == newBeatmap.Hidden; + + if (equalForDisplayPurposes) return false; } From 7e109add9618bf3a59d47e999fa864fc23ed11a9 Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Thu, 28 Aug 2025 19:10:20 +0900 Subject: [PATCH 5/5] Ensure filtering also runs after local gameplay `LastPlayed` changes --- .../SongSelectV2/TestSceneSongSelect.cs | 24 +++++++++++++++++++ osu.Game/Screens/SelectV2/BeatmapCarousel.cs | 4 +++- 2 files changed, 27 insertions(+), 1 deletion(-) diff --git a/osu.Game.Tests/Visual/SongSelectV2/TestSceneSongSelect.cs b/osu.Game.Tests/Visual/SongSelectV2/TestSceneSongSelect.cs index 69bdb97617..895f148965 100644 --- a/osu.Game.Tests/Visual/SongSelectV2/TestSceneSongSelect.cs +++ b/osu.Game.Tests/Visual/SongSelectV2/TestSceneSongSelect.cs @@ -23,7 +23,9 @@ using osu.Game.Screens.Ranking; using osu.Game.Screens.Select; using osu.Game.Screens.Select.Leaderboards; using osu.Game.Screens.SelectV2; +using osu.Game.Tests.Resources; using osuTK.Input; +using BeatmapCarousel = osu.Game.Screens.SelectV2.BeatmapCarousel; using FooterButtonMods = osu.Game.Screens.SelectV2.FooterButtonMods; using FooterButtonOptions = osu.Game.Screens.SelectV2.FooterButtonOptions; using FooterButtonRandom = osu.Game.Screens.SelectV2.FooterButtonRandom; @@ -302,6 +304,28 @@ namespace osu.Game.Tests.Visual.SongSelectV2 }); } + /// + /// Last played and rank achieved may have changed, so we want to make sure filtering runs on resume to song select. + /// + [Test] + public void TestFilteringRunsAfterReturningFromGameplay() + { + AddStep("import actual beatmap", () => Beatmaps.Import(TestResources.GetQuickTestBeatmapForImport())); + LoadSongSelect(); + + AddUntilStep("wait for filtered", () => SongSelect.ChildrenOfType().Single().FilterCount, () => Is.EqualTo(1)); + + AddStep("enter gameplay", () => InputManager.Key(Key.Enter)); + + AddUntilStep("wait for player", () => Stack.CurrentScreen is Player); + AddUntilStep("wait for fail", () => ((Player)Stack.CurrentScreen).GameplayState.HasFailed); + + AddStep("exit gameplay", () => InputManager.Key(Key.Escape)); + AddStep("exit gameplay", () => InputManager.Key(Key.Escape)); + + AddUntilStep("wait for filtered", () => SongSelect.ChildrenOfType().Single().FilterCount, () => Is.EqualTo(2)); + } + [Test] public void TestAutoplayShortcut() { diff --git a/osu.Game/Screens/SelectV2/BeatmapCarousel.cs b/osu.Game/Screens/SelectV2/BeatmapCarousel.cs index 5b0ad1ae29..a4e957a1bf 100644 --- a/osu.Game/Screens/SelectV2/BeatmapCarousel.cs +++ b/osu.Game/Screens/SelectV2/BeatmapCarousel.cs @@ -392,7 +392,9 @@ namespace osu.Game.Screens.SelectV2 // displayed on panel oldBeatmap.DifficultyName == newBeatmap.DifficultyName && // hidden changed, needs re-filter - oldBeatmap.Hidden == newBeatmap.Hidden; + oldBeatmap.Hidden == newBeatmap.Hidden && + // might be used for grouping, returning from gameplay + oldBeatmap.LastPlayed == newBeatmap.LastPlayed; if (equalForDisplayPurposes) return false;