From d9295619cb1271236602405c206dddf4f55f4c43 Mon Sep 17 00:00:00 2001 From: Seven_Sec <1260493648@qq.com> Date: Tue, 4 Aug 2026 16:40:59 +0900 Subject: [PATCH 1/6] fix: optimize beatmap set update handling in carousel --- osu.Game/Screens/Select/BeatmapCarousel.cs | 81 +++++++++++++++------- 1 file changed, 57 insertions(+), 24 deletions(-) diff --git a/osu.Game/Screens/Select/BeatmapCarousel.cs b/osu.Game/Screens/Select/BeatmapCarousel.cs index 2cf8c7d2d041..81fd16915265 100644 --- a/osu.Game/Screens/Select/BeatmapCarousel.cs +++ b/osu.Game/Screens/Select/BeatmapCarousel.cs @@ -248,44 +248,72 @@ bool attemptSelection(CarouselItem item) // In the case of difficulty reprocessing, this will trigger multiple times per beatmap as it's always triggering a set update. // We may want to look to improve this in the future either here or at the source (only trigger an update after all difficulties // have been processed) if it becomes an issue for animation or performance reasons. + // + // A set's difficulties always occupy a contiguous range in `Items` (they are only ever added or replaced as a whole set), + // so the entire diff is applied as a single replace operation below rather than one per difficulty. This avoids running + // the carousel's change handling / relayout (and a linear `IndexOf` lookup) once per difficulty. + List newBeatmaps = new List(oldSetBeatmaps.Count); + foreach (var beatmap in oldSetBeatmaps) { - int previousIndex = Items.IndexOf(beatmap); - Debug.Assert(previousIndex >= 0); - // we're intentionally being lenient with there being two difficulties with equal online ID or difficulty name. // this can be the case when the user modifies the beatmap using the editor's "external edit" feature. BeatmapInfo? matchingNewBeatmap = newSetBeatmaps.FirstOrDefault(b => b.OnlineID > 0 && b.OnlineID == beatmap.OnlineID) ?? newSetBeatmaps.FirstOrDefault(b => b.DifficultyName == beatmap.DifficultyName && b.Ruleset.Equals(beatmap.Ruleset)); - // The matching beatmap may have been deleted or invalidated in some way since this event was fired. - // Let's make sure we have the most up-to-date realm state. - if (matchingNewBeatmap?.ID is Guid matchingID) - matchingNewBeatmap = realm.Run(r => r.FindWithRefresh(matchingID)?.Detach()); + if (matchingNewBeatmap == null) + continue; - if (matchingNewBeatmap != null) - { - // TODO: should this exist in song select instead of here? - // we need to ensure the global beatmap is also updated alongside changes. - if (CurrentBeatmap != null && beatmap.Equals(CurrentBeatmap)) - // we don't know in which group the matching new beatmap is, but that's fine - we can keep the previous one for now. - // we are about to modify `Items`, which - if required - will trigger a re-filter, - // which will pick a correct group - if one is present - via `HandleFilterCompleted()`. - RequestSelection(new GroupedBeatmap(CurrentGroupedBeatmap?.Group, matchingNewBeatmap)); - - Items.ReplaceRange(previousIndex, 1, [matchingNewBeatmap]); - newSetBeatmaps.Remove(matchingNewBeatmap); - } - else + // The matched beatmap may have been deleted since the snapshot was taken, as multiple updates to the same set can be queued + // before the carousel processes them. Replacing an item with a stale beatmap converges via the follow-up update queued for the + // deletion, but selecting one would load a beatmap that no longer exists in realm. Only the selection path needs a freshness + // check, which limits this to a single realm round-trip per replace event. + if (CurrentBeatmap != null && beatmap.Equals(CurrentBeatmap) && matchingNewBeatmap.ID is Guid matchingID) { - Items.RemoveAt(previousIndex); + var refreshedBeatmap = realm.Run(r => r.FindWithRefresh(matchingID)?.Detach()); + + if (refreshedBeatmap == null) + { + // The matched beatmap was deleted since the snapshot was taken. Retain the stale match in the list (it will be + // removed by the queued follow-up update) and leave the current selection untouched rather than selecting a beatmap + // that no longer exists in realm. + newBeatmaps.Add(matchingNewBeatmap); + newSetBeatmaps.Remove(matchingNewBeatmap); + continue; + } + + matchingNewBeatmap = refreshedBeatmap; } + + // TODO: should this exist in song select instead of here? + // we need to ensure the global beatmap is also updated alongside changes. + if (CurrentBeatmap != null && beatmap.Equals(CurrentBeatmap)) + // we don't know in which group the matching new beatmap is, but that's fine - we can keep the previous one for now. + // we are about to modify `Items`, which - if required - will trigger a re-filter, + // which will pick a correct group - if one is present - via `HandleFilterCompleted()`. + RequestSelection(new GroupedBeatmap(CurrentGroupedBeatmap?.Group, matchingNewBeatmap)); + + newBeatmaps.Add(matchingNewBeatmap); + newSetBeatmaps.Remove(matchingNewBeatmap); } // Add any items which weren't found in the previous pass (difficulty names didn't match). - foreach (var beatmap in newSetBeatmaps) - Items.Add(beatmap); + newBeatmaps.AddRange(newSetBeatmaps); + + if (oldSetBeatmaps.Count == 0) + { + foreach (var beatmap in newBeatmaps) + Items.Add(beatmap); + + break; + } + + int previousIndex = Items.IndexOf(oldSetBeatmaps[0]); + Debug.Assert(previousIndex >= 0); + Debug.Assert(Items.Skip(previousIndex).Take(oldSetBeatmaps.Count).SequenceEqual(oldSetBeatmaps), "the set's difficulties should occupy a contiguous range in the carousel items"); + + Items.ReplaceRange(previousIndex, oldSetBeatmaps.Count, newBeatmaps); break; @@ -426,6 +454,11 @@ protected override bool HandleItemsChanged(NotifyCollectionChangedEventArgs args var oldBeatmaps = args.OldItems!.OfType().ToList(); var newBeatmaps = args.NewItems!.OfType().ToList(); + // A replace may change the number of items, as the carousel replaces a whole set's difficulties in one operation + // (see `beatmapSetsChanged`). Any count change requires a re-filter; only equal-sized replaces can be skipped. + if (oldBeatmaps.Count != newBeatmaps.Count) + return true; + for (int i = 0; i < oldBeatmaps.Count; i++) { var oldBeatmap = oldBeatmaps[i]; From 6c607d1775b37014e7bb3dc89527f3dba88bab7f Mon Sep 17 00:00:00 2001 From: Seven_Sec <1260493648@qq.com> Date: Tue, 4 Aug 2026 16:41:32 +0900 Subject: [PATCH 2/6] test: add tests for beatmap set replacement scenarios --- .../TestSceneBeatmapCarouselUpdateHandling.cs | 145 ++++++++++++++++++ 1 file changed, 145 insertions(+) diff --git a/osu.Game.Tests/Visual/SongSelect/TestSceneBeatmapCarouselUpdateHandling.cs b/osu.Game.Tests/Visual/SongSelect/TestSceneBeatmapCarouselUpdateHandling.cs index 1033a17e05cf..c86f54b85e4c 100644 --- a/osu.Game.Tests/Visual/SongSelect/TestSceneBeatmapCarouselUpdateHandling.cs +++ b/osu.Game.Tests/Visual/SongSelect/TestSceneBeatmapCarouselUpdateHandling.cs @@ -430,6 +430,151 @@ public void TestSortingStabilityWithNewItems() AddAssert("Order didn't change", () => Carousel.PostFilterBeatmaps.Select(b => b.ID), () => Is.EqualTo(originalOrder)); } + /// + /// Replicates a whole-set replace as applied by the carousel when a set is updated: some difficulties are + /// matched-and-replaced (by online ID), some are removed, and some new ones are added. + /// The replace handling applies the entire diff as a single range replace (including a count change), + /// so this guards both the splice itself and the count-change path of the carousel's change handling. + /// + [Test] + public void TestBeatmapSetReplacedWithMixedDifficultyMutations() + { + List expectedIds = null!; + Guid removedId = default; + + AddStep("update set with mixed difficulty mutations", () => + { + removedId = baseTestBeatmap.Beatmaps[1].ID; + + var updatedSet = new BeatmapSetInfo + { + ID = baseTestBeatmap.ID, + OnlineID = baseTestBeatmap.OnlineID, + DateAdded = baseTestBeatmap.DateAdded, + DateSubmitted = baseTestBeatmap.DateSubmitted, + DateRanked = baseTestBeatmap.DateRanked, + Status = baseTestBeatmap.Status, + StatusInt = baseTestBeatmap.StatusInt, + DeletePending = baseTestBeatmap.DeletePending, + Hash = baseTestBeatmap.Hash, + Protected = baseTestBeatmap.Protected, + }; + + // keep the first difficulty (matched by online ID, but with changed metadata => valid replace); + // drop the second difficulty entirely; and introduce a brand new third difficulty. + var keptDifficulty = baseTestBeatmap.Beatmaps[0]; + var kept = new BeatmapInfo + { + ID = keptDifficulty.ID, + Metadata = new BeatmapMetadata { Artist = "updated test", Title = "updated title" }, + Ruleset = keptDifficulty.Ruleset, + DifficultyName = keptDifficulty.DifficultyName, + BeatmapSet = updatedSet, + Status = keptDifficulty.Status, + OnlineID = keptDifficulty.OnlineID, + Length = keptDifficulty.Length, + BPM = keptDifficulty.BPM, + Hash = "new hash", + StarRating = keptDifficulty.StarRating, + MD5Hash = keptDifficulty.MD5Hash, + OnlineMD5Hash = keptDifficulty.OnlineMD5Hash, + }; + + var added = createBeatmap(updatedSet); + added.ID = Guid.NewGuid(); + added.OnlineID = -2; + added.DifficultyName = "new difficulty"; + + updatedSet.Beatmaps.Add(kept); + updatedSet.Beatmaps.Add(added); + + expectedIds = updatedSet.Beatmaps.Select(b => b.ID).ToList(); + + int originalIndex = BeatmapSets.IndexOf(baseTestBeatmap); + + Realm.Write(r => r.Add(updatedSet, update: true)); + BeatmapSets.ReplaceRange(originalIndex, 1, [updatedSet.Detach()]); + }); + + WaitForFiltering(); + + AddAssert("updated set has exactly two difficulties", () => Carousel.PostFilterBeatmaps.Count(b => expectedIds.Contains(b.ID)), () => Is.EqualTo(2)); + AddAssert("kept difficulty present", () => Carousel.PostFilterBeatmaps.Any(b => b.ID == expectedIds[0]), () => Is.True); + AddAssert("added difficulty present", () => Carousel.PostFilterBeatmaps.Any(b => b.ID == expectedIds[1]), () => Is.True); + AddAssert("removed difficulty is gone", () => Carousel.PostFilterBeatmaps.Any(b => b.ID == removedId), () => Is.False); + } + + /// + /// Replicates the #34826 scenario: the matched beatmap is present in the replace snapshot, but has been + /// deleted from realm by the time the replace is processed (multiple updates to the same set can be queued + /// before the carousel processes them). The replace handling must not request selection of a beatmap that + /// no longer exists in realm — song select would otherwise load it as the global beatmap (NRE in + /// , see https://github.com/ppy/osu/issues/34826). + /// + /// + /// Regression guard: this test fails if the per-difficulty realm check from #34914 is removed outright, + /// and passes with both the #34914 check and the current selection-path-only check. + /// + [Test] + public void TestBeatmapSetReplacedWithDeletedCurrentBeatmap() + { + BeatmapInfo selectedBeatmap = null!; + BeatmapInfo kept = null!; + + AddStep("select first difficulty", () => + { + selectedBeatmap = baseTestBeatmap.Beatmaps[0]; + Carousel.CurrentBeatmap = selectedBeatmap; + }); + + AddStep("update set with a matched difficulty no longer in realm", () => + { + var updatedSet = new BeatmapSetInfo + { + ID = baseTestBeatmap.ID, + OnlineID = baseTestBeatmap.OnlineID, + DateAdded = baseTestBeatmap.DateAdded, + DateSubmitted = baseTestBeatmap.DateSubmitted, + DateRanked = baseTestBeatmap.DateRanked, + Status = baseTestBeatmap.Status, + StatusInt = baseTestBeatmap.StatusInt, + DeletePending = baseTestBeatmap.DeletePending, + Hash = baseTestBeatmap.Hash, + Protected = baseTestBeatmap.Protected, + }; + + // The matched difficulty carries an ID which was never written to realm, simulating a beatmap deleted + // since the replace snapshot was taken. The set itself is intentionally not added to realm either. + kept = new BeatmapInfo + { + ID = Guid.NewGuid(), + Metadata = new BeatmapMetadata { Artist = "updated test", Title = "updated title" }, + Ruleset = selectedBeatmap.Ruleset, + DifficultyName = selectedBeatmap.DifficultyName, + BeatmapSet = updatedSet, + Status = selectedBeatmap.Status, + OnlineID = selectedBeatmap.OnlineID, + Length = selectedBeatmap.Length, + BPM = selectedBeatmap.BPM, + Hash = "new hash", + StarRating = selectedBeatmap.StarRating, + MD5Hash = selectedBeatmap.MD5Hash, + OnlineMD5Hash = selectedBeatmap.OnlineMD5Hash, + }; + + updatedSet.Beatmaps.Add(kept); + + int originalIndex = BeatmapSets.IndexOf(baseTestBeatmap); + + BeatmapSets.ReplaceRange(originalIndex, 1, [updatedSet.Detach()]); + }); + + WaitForFiltering(); + + AddAssert("selection unchanged", () => Carousel.CurrentBeatmap, () => Is.EqualTo(selectedBeatmap)); + AddAssert("deleted match never requested for selection", () => BeatmapRequestedSelections.Contains(kept), () => Is.False); + } + private void assertDidFilter(int count = 1) => AddAssert("did filter", () => Carousel.FilterCount, () => Is.EqualTo(initial_filter_count + count)); private void assertDidNotFilter() => AddAssert("did not filter", () => Carousel.FilterCount, () => Is.EqualTo(initial_filter_count)); From a11454691706be0cb53b2137c0d8f40891df1858 Mon Sep 17 00:00:00 2001 From: Seven_Sec <1260493648@qq.com> Date: Tue, 4 Aug 2026 17:51:11 +0900 Subject: [PATCH 3/6] fix: code analysis warnings --- .../SongSelect/TestSceneBeatmapCarouselUpdateHandling.cs | 2 +- osu.Game/Screens/Select/BeatmapCarousel.cs | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/osu.Game.Tests/Visual/SongSelect/TestSceneBeatmapCarouselUpdateHandling.cs b/osu.Game.Tests/Visual/SongSelect/TestSceneBeatmapCarouselUpdateHandling.cs index c86f54b85e4c..5433edb79874 100644 --- a/osu.Game.Tests/Visual/SongSelect/TestSceneBeatmapCarouselUpdateHandling.cs +++ b/osu.Game.Tests/Visual/SongSelect/TestSceneBeatmapCarouselUpdateHandling.cs @@ -440,7 +440,7 @@ public void TestSortingStabilityWithNewItems() public void TestBeatmapSetReplacedWithMixedDifficultyMutations() { List expectedIds = null!; - Guid removedId = default; + Guid removedId = Guid.Empty; AddStep("update set with mixed difficulty mutations", () => { diff --git a/osu.Game/Screens/Select/BeatmapCarousel.cs b/osu.Game/Screens/Select/BeatmapCarousel.cs index 81fd16915265..46c24115fdcc 100644 --- a/osu.Game/Screens/Select/BeatmapCarousel.cs +++ b/osu.Game/Screens/Select/BeatmapCarousel.cs @@ -269,9 +269,9 @@ bool attemptSelection(CarouselItem item) // before the carousel processes them. Replacing an item with a stale beatmap converges via the follow-up update queued for the // deletion, but selecting one would load a beatmap that no longer exists in realm. Only the selection path needs a freshness // check, which limits this to a single realm round-trip per replace event. - if (CurrentBeatmap != null && beatmap.Equals(CurrentBeatmap) && matchingNewBeatmap.ID is Guid matchingID) + if (CurrentBeatmap != null && beatmap.Equals(CurrentBeatmap)) { - var refreshedBeatmap = realm.Run(r => r.FindWithRefresh(matchingID)?.Detach()); + var refreshedBeatmap = realm.Run(r => r.FindWithRefresh(matchingNewBeatmap.ID)?.Detach()); if (refreshedBeatmap == null) { From 864ca2ef9a1d14c31d286e3c8c55381252bf321c Mon Sep 17 00:00:00 2001 From: Seven_Sec <1260493648@qq.com> Date: Tue, 4 Aug 2026 20:42:33 +0900 Subject: [PATCH 4/6] fix: flip the logic to match batched replaces --- osu.Game/Screens/Select/BeatmapCarousel.cs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/osu.Game/Screens/Select/BeatmapCarousel.cs b/osu.Game/Screens/Select/BeatmapCarousel.cs index 46c24115fdcc..a00d649be0e5 100644 --- a/osu.Game/Screens/Select/BeatmapCarousel.cs +++ b/osu.Game/Screens/Select/BeatmapCarousel.cs @@ -487,11 +487,11 @@ protected override bool HandleItemsChanged(NotifyCollectionChangedEventArgs args // might be used for grouping, returning from gameplay oldBeatmap.LastPlayed == newBeatmap.LastPlayed; - if (equalForDisplayPurposes) - return false; + if (!equalForDisplayPurposes) + return true; } - return true; + return false; default: throw new ArgumentOutOfRangeException(); From 8c4079e0b218cb0f52b4b27cdf3f33836473f9fb Mon Sep 17 00:00:00 2001 From: Seven_Sec <1260493648@qq.com> Date: Wed, 5 Aug 2026 19:52:25 +0900 Subject: [PATCH 5/6] refactor: check current beatmap only from realm to avoid a huge refilter (revert) --- osu.Game/Screens/Select/BeatmapCarousel.cs | 87 ++++++++-------------- 1 file changed, 29 insertions(+), 58 deletions(-) diff --git a/osu.Game/Screens/Select/BeatmapCarousel.cs b/osu.Game/Screens/Select/BeatmapCarousel.cs index a00d649be0e5..dc6cdb62e2fd 100644 --- a/osu.Game/Screens/Select/BeatmapCarousel.cs +++ b/osu.Game/Screens/Select/BeatmapCarousel.cs @@ -248,72 +248,48 @@ bool attemptSelection(CarouselItem item) // In the case of difficulty reprocessing, this will trigger multiple times per beatmap as it's always triggering a set update. // We may want to look to improve this in the future either here or at the source (only trigger an update after all difficulties // have been processed) if it becomes an issue for animation or performance reasons. - // - // A set's difficulties always occupy a contiguous range in `Items` (they are only ever added or replaced as a whole set), - // so the entire diff is applied as a single replace operation below rather than one per difficulty. This avoids running - // the carousel's change handling / relayout (and a linear `IndexOf` lookup) once per difficulty. - List newBeatmaps = new List(oldSetBeatmaps.Count); - foreach (var beatmap in oldSetBeatmaps) { + int previousIndex = Items.IndexOf(beatmap); + Debug.Assert(previousIndex >= 0); + // we're intentionally being lenient with there being two difficulties with equal online ID or difficulty name. // this can be the case when the user modifies the beatmap using the editor's "external edit" feature. BeatmapInfo? matchingNewBeatmap = newSetBeatmaps.FirstOrDefault(b => b.OnlineID > 0 && b.OnlineID == beatmap.OnlineID) ?? newSetBeatmaps.FirstOrDefault(b => b.DifficultyName == beatmap.DifficultyName && b.Ruleset.Equals(beatmap.Ruleset)); - if (matchingNewBeatmap == null) - continue; - - // The matched beatmap may have been deleted since the snapshot was taken, as multiple updates to the same set can be queued - // before the carousel processes them. Replacing an item with a stale beatmap converges via the follow-up update queued for the - // deletion, but selecting one would load a beatmap that no longer exists in realm. Only the selection path needs a freshness - // check, which limits this to a single realm round-trip per replace event. - if (CurrentBeatmap != null && beatmap.Equals(CurrentBeatmap)) + if (matchingNewBeatmap != null) { - var refreshedBeatmap = realm.Run(r => r.FindWithRefresh(matchingNewBeatmap.ID)?.Detach()); - - if (refreshedBeatmap == null) + // TODO: should this exist in song select instead of here? + // we need to ensure the global beatmap is also updated alongside changes. + if (CurrentBeatmap != null && beatmap.Equals(CurrentBeatmap)) { - // The matched beatmap was deleted since the snapshot was taken. Retain the stale match in the list (it will be - // removed by the queued follow-up update) and leave the current selection untouched rather than selecting a beatmap - // that no longer exists in realm. - newBeatmaps.Add(matchingNewBeatmap); - newSetBeatmaps.Remove(matchingNewBeatmap); - continue; + // The matching beatmap may have been deleted or invalidated in some way since this event was fired. + // Let's make sure we have the most up-to-date realm state of the current beatmap. + var refreshedNewBeatmap = realm.Run(r => r.FindWithRefresh(matchingNewBeatmap.ID)?.Detach()); + if (refreshedNewBeatmap != null) + { + matchingNewBeatmap = refreshedNewBeatmap; + // we don't know in which group the matching new beatmap is, but that's fine - we can keep the previous one for now. + // we are about to modify `Items`, which - if required - will trigger a re-filter, + // which will pick a correct group - if one is present - via `HandleFilterCompleted()`. + RequestSelection(new GroupedBeatmap(CurrentGroupedBeatmap?.Group, matchingNewBeatmap)); + } } - matchingNewBeatmap = refreshedBeatmap; + Items.ReplaceRange(previousIndex, 1, [matchingNewBeatmap]); + newSetBeatmaps.Remove(matchingNewBeatmap); + } + else + { + Items.RemoveAt(previousIndex); } - - // TODO: should this exist in song select instead of here? - // we need to ensure the global beatmap is also updated alongside changes. - if (CurrentBeatmap != null && beatmap.Equals(CurrentBeatmap)) - // we don't know in which group the matching new beatmap is, but that's fine - we can keep the previous one for now. - // we are about to modify `Items`, which - if required - will trigger a re-filter, - // which will pick a correct group - if one is present - via `HandleFilterCompleted()`. - RequestSelection(new GroupedBeatmap(CurrentGroupedBeatmap?.Group, matchingNewBeatmap)); - - newBeatmaps.Add(matchingNewBeatmap); - newSetBeatmaps.Remove(matchingNewBeatmap); } // Add any items which weren't found in the previous pass (difficulty names didn't match). - newBeatmaps.AddRange(newSetBeatmaps); - - if (oldSetBeatmaps.Count == 0) - { - foreach (var beatmap in newBeatmaps) - Items.Add(beatmap); - - break; - } - - int previousIndex = Items.IndexOf(oldSetBeatmaps[0]); - Debug.Assert(previousIndex >= 0); - Debug.Assert(Items.Skip(previousIndex).Take(oldSetBeatmaps.Count).SequenceEqual(oldSetBeatmaps), "the set's difficulties should occupy a contiguous range in the carousel items"); - - Items.ReplaceRange(previousIndex, oldSetBeatmaps.Count, newBeatmaps); + foreach (var beatmap in newSetBeatmaps) + Items.Add(beatmap); break; @@ -454,11 +430,6 @@ protected override bool HandleItemsChanged(NotifyCollectionChangedEventArgs args var oldBeatmaps = args.OldItems!.OfType().ToList(); var newBeatmaps = args.NewItems!.OfType().ToList(); - // A replace may change the number of items, as the carousel replaces a whole set's difficulties in one operation - // (see `beatmapSetsChanged`). Any count change requires a re-filter; only equal-sized replaces can be skipped. - if (oldBeatmaps.Count != newBeatmaps.Count) - return true; - for (int i = 0; i < oldBeatmaps.Count; i++) { var oldBeatmap = oldBeatmaps[i]; @@ -487,11 +458,11 @@ protected override bool HandleItemsChanged(NotifyCollectionChangedEventArgs args // might be used for grouping, returning from gameplay oldBeatmap.LastPlayed == newBeatmap.LastPlayed; - if (!equalForDisplayPurposes) - return true; + if (equalForDisplayPurposes) + return false; } - return false; + return true; default: throw new ArgumentOutOfRangeException(); From 39323556864f163695474834d4627d30c7180f2b Mon Sep 17 00:00:00 2001 From: Seven_Sec <1260493648@qq.com> Date: Thu, 6 Aug 2026 05:20:23 +0900 Subject: [PATCH 6/6] test: optimize test script and drop unnecessary one --- .../TestSceneBeatmapCarouselUpdateHandling.cs | 149 ++++-------------- osu.Game/Screens/Select/BeatmapCarousel.cs | 2 + 2 files changed, 29 insertions(+), 122 deletions(-) diff --git a/osu.Game.Tests/Visual/SongSelect/TestSceneBeatmapCarouselUpdateHandling.cs b/osu.Game.Tests/Visual/SongSelect/TestSceneBeatmapCarouselUpdateHandling.cs index 5433edb79874..e8ade5c63bd6 100644 --- a/osu.Game.Tests/Visual/SongSelect/TestSceneBeatmapCarouselUpdateHandling.cs +++ b/osu.Game.Tests/Visual/SongSelect/TestSceneBeatmapCarouselUpdateHandling.cs @@ -430,149 +430,54 @@ public void TestSortingStabilityWithNewItems() AddAssert("Order didn't change", () => Carousel.PostFilterBeatmaps.Select(b => b.ID), () => Is.EqualTo(originalOrder)); } - /// - /// Replicates a whole-set replace as applied by the carousel when a set is updated: some difficulties are - /// matched-and-replaced (by online ID), some are removed, and some new ones are added. - /// The replace handling applies the entire diff as a single range replace (including a count change), - /// so this guards both the splice itself and the count-change path of the carousel's change handling. - /// - [Test] - public void TestBeatmapSetReplacedWithMixedDifficultyMutations() - { - List expectedIds = null!; - Guid removedId = Guid.Empty; - - AddStep("update set with mixed difficulty mutations", () => - { - removedId = baseTestBeatmap.Beatmaps[1].ID; - - var updatedSet = new BeatmapSetInfo - { - ID = baseTestBeatmap.ID, - OnlineID = baseTestBeatmap.OnlineID, - DateAdded = baseTestBeatmap.DateAdded, - DateSubmitted = baseTestBeatmap.DateSubmitted, - DateRanked = baseTestBeatmap.DateRanked, - Status = baseTestBeatmap.Status, - StatusInt = baseTestBeatmap.StatusInt, - DeletePending = baseTestBeatmap.DeletePending, - Hash = baseTestBeatmap.Hash, - Protected = baseTestBeatmap.Protected, - }; - - // keep the first difficulty (matched by online ID, but with changed metadata => valid replace); - // drop the second difficulty entirely; and introduce a brand new third difficulty. - var keptDifficulty = baseTestBeatmap.Beatmaps[0]; - var kept = new BeatmapInfo - { - ID = keptDifficulty.ID, - Metadata = new BeatmapMetadata { Artist = "updated test", Title = "updated title" }, - Ruleset = keptDifficulty.Ruleset, - DifficultyName = keptDifficulty.DifficultyName, - BeatmapSet = updatedSet, - Status = keptDifficulty.Status, - OnlineID = keptDifficulty.OnlineID, - Length = keptDifficulty.Length, - BPM = keptDifficulty.BPM, - Hash = "new hash", - StarRating = keptDifficulty.StarRating, - MD5Hash = keptDifficulty.MD5Hash, - OnlineMD5Hash = keptDifficulty.OnlineMD5Hash, - }; - - var added = createBeatmap(updatedSet); - added.ID = Guid.NewGuid(); - added.OnlineID = -2; - added.DifficultyName = "new difficulty"; - - updatedSet.Beatmaps.Add(kept); - updatedSet.Beatmaps.Add(added); - - expectedIds = updatedSet.Beatmaps.Select(b => b.ID).ToList(); - - int originalIndex = BeatmapSets.IndexOf(baseTestBeatmap); - - Realm.Write(r => r.Add(updatedSet, update: true)); - BeatmapSets.ReplaceRange(originalIndex, 1, [updatedSet.Detach()]); - }); - - WaitForFiltering(); - - AddAssert("updated set has exactly two difficulties", () => Carousel.PostFilterBeatmaps.Count(b => expectedIds.Contains(b.ID)), () => Is.EqualTo(2)); - AddAssert("kept difficulty present", () => Carousel.PostFilterBeatmaps.Any(b => b.ID == expectedIds[0]), () => Is.True); - AddAssert("added difficulty present", () => Carousel.PostFilterBeatmaps.Any(b => b.ID == expectedIds[1]), () => Is.True); - AddAssert("removed difficulty is gone", () => Carousel.PostFilterBeatmaps.Any(b => b.ID == removedId), () => Is.False); - } - /// /// Replicates the #34826 scenario: the matched beatmap is present in the replace snapshot, but has been - /// deleted from realm by the time the replace is processed (multiple updates to the same set can be queued - /// before the carousel processes them). The replace handling must not request selection of a beatmap that - /// no longer exists in realm — song select would otherwise load it as the global beatmap (NRE in - /// , see https://github.com/ppy/osu/issues/34826). + /// deleted from realm by the time the replace is processed, see https://github.com/ppy/osu/issues/34826. /// - /// - /// Regression guard: this test fails if the per-difficulty realm check from #34914 is removed outright, - /// and passes with both the #34914 check and the current selection-path-only check. - /// [Test] public void TestBeatmapSetReplacedWithDeletedCurrentBeatmap() { - BeatmapInfo selectedBeatmap = null!; - BeatmapInfo kept = null!; + int targetSetIndex = 0; AddStep("select first difficulty", () => { - selectedBeatmap = baseTestBeatmap.Beatmaps[0]; - Carousel.CurrentBeatmap = selectedBeatmap; + Carousel.CurrentBeatmap = baseTestBeatmap.Beatmaps[0]; + BeatmapRequestedSelections.Clear(); }); - AddStep("update set with a matched difficulty no longer in realm", () => + AddStep("delete current beatmap from realm and replace set", () => { - var updatedSet = new BeatmapSetInfo + targetSetIndex = BeatmapSets.IndexOf(baseTestBeatmap); + var detachedSet = BeatmapSets[targetSetIndex]; + var selectedBeatmap = detachedSet.Beatmaps[0]; + + Realm.Write(r => { - ID = baseTestBeatmap.ID, - OnlineID = baseTestBeatmap.OnlineID, - DateAdded = baseTestBeatmap.DateAdded, - DateSubmitted = baseTestBeatmap.DateSubmitted, - DateRanked = baseTestBeatmap.DateRanked, - Status = baseTestBeatmap.Status, - StatusInt = baseTestBeatmap.StatusInt, - DeletePending = baseTestBeatmap.DeletePending, - Hash = baseTestBeatmap.Hash, - Protected = baseTestBeatmap.Protected, - }; + var toDelete = r.Find(selectedBeatmap.ID); + if (toDelete != null) + r.Remove(toDelete); + }); - // The matched difficulty carries an ID which was never written to realm, simulating a beatmap deleted - // since the replace snapshot was taken. The set itself is intentionally not added to realm either. - kept = new BeatmapInfo + // Trigger the Replace action with a beatmap that is not in realm. + var staleSet = new BeatmapSetInfo { - ID = Guid.NewGuid(), - Metadata = new BeatmapMetadata { Artist = "updated test", Title = "updated title" }, - Ruleset = selectedBeatmap.Ruleset, - DifficultyName = selectedBeatmap.DifficultyName, - BeatmapSet = updatedSet, - Status = selectedBeatmap.Status, - OnlineID = selectedBeatmap.OnlineID, - Length = selectedBeatmap.Length, - BPM = selectedBeatmap.BPM, - Hash = "new hash", - StarRating = selectedBeatmap.StarRating, - MD5Hash = selectedBeatmap.MD5Hash, - OnlineMD5Hash = selectedBeatmap.OnlineMD5Hash, + ID = detachedSet.ID, + OnlineID = detachedSet.OnlineID, + DateAdded = detachedSet.DateAdded, + DateSubmitted = detachedSet.DateSubmitted, + Status = detachedSet.Status, + Hash = detachedSet.Hash, + Protected = detachedSet.Protected, }; - updatedSet.Beatmaps.Add(kept); - - int originalIndex = BeatmapSets.IndexOf(baseTestBeatmap); - - BeatmapSets.ReplaceRange(originalIndex, 1, [updatedSet.Detach()]); + var staleBeatmap = createBeatmap(staleSet, selectedBeatmap); + staleSet.Beatmaps.Add(staleBeatmap); + BeatmapSets.ReplaceRange(targetSetIndex, 1, [staleSet]); }); WaitForFiltering(); - AddAssert("selection unchanged", () => Carousel.CurrentBeatmap, () => Is.EqualTo(selectedBeatmap)); - AddAssert("deleted match never requested for selection", () => BeatmapRequestedSelections.Contains(kept), () => Is.False); + AddAssert("deleted match never requested for selection", () => BeatmapRequestedSelections, () => Is.Empty); } private void assertDidFilter(int count = 1) => AddAssert("did filter", () => Carousel.FilterCount, () => Is.EqualTo(initial_filter_count + count)); diff --git a/osu.Game/Screens/Select/BeatmapCarousel.cs b/osu.Game/Screens/Select/BeatmapCarousel.cs index dc6cdb62e2fd..f2dcb1e93f3c 100644 --- a/osu.Game/Screens/Select/BeatmapCarousel.cs +++ b/osu.Game/Screens/Select/BeatmapCarousel.cs @@ -268,9 +268,11 @@ bool attemptSelection(CarouselItem item) // The matching beatmap may have been deleted or invalidated in some way since this event was fired. // Let's make sure we have the most up-to-date realm state of the current beatmap. var refreshedNewBeatmap = realm.Run(r => r.FindWithRefresh(matchingNewBeatmap.ID)?.Detach()); + if (refreshedNewBeatmap != null) { matchingNewBeatmap = refreshedNewBeatmap; + // we don't know in which group the matching new beatmap is, but that's fine - we can keep the previous one for now. // we are about to modify `Items`, which - if required - will trigger a re-filter, // which will pick a correct group - if one is present - via `HandleFilterCompleted()`.