From a8ce2c5edf15f5af96cb8e14183e89757c650ae9 Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Thu, 20 Jan 2022 21:14:10 +0900 Subject: [PATCH 1/6] Detach before sending `BeatmapSetInfo` to any handling method --- osu.Game/Screens/Select/BeatmapCarousel.cs | 12 +++++------- 1 file changed, 5 insertions(+), 7 deletions(-) diff --git a/osu.Game/Screens/Select/BeatmapCarousel.cs b/osu.Game/Screens/Select/BeatmapCarousel.cs index 75ad0511e6..0ee59f7f04 100644 --- a/osu.Game/Screens/Select/BeatmapCarousel.cs +++ b/osu.Game/Screens/Select/BeatmapCarousel.cs @@ -114,7 +114,7 @@ namespace osu.Game.Screens.Select { CarouselRoot newRoot = new CarouselRoot(this); - newRoot.AddChildren(beatmapSets.Select(createCarouselSet).Where(g => g != null)); + newRoot.AddChildren(beatmapSets.Select(s => createCarouselSet(s.Detach())).Where(g => g != null)); root = newRoot; if (selectedBeatmapSet != null && !beatmapSets.Contains(selectedBeatmapSet.BeatmapSet)) @@ -209,7 +209,7 @@ namespace osu.Game.Screens.Select return; foreach (int i in changes.InsertedIndices) - RemoveBeatmapSet(sender[i]); + RemoveBeatmapSet(sender[i].Detach()); } private void beatmapSetsChanged(IRealmCollection sender, ChangeSet changes, Exception error) @@ -248,10 +248,10 @@ namespace osu.Game.Screens.Select } foreach (int i in changes.NewModifiedIndices) - UpdateBeatmapSet(sender[i]); + UpdateBeatmapSet(sender[i].Detach()); foreach (int i in changes.InsertedIndices) - UpdateBeatmapSet(sender[i]); + UpdateBeatmapSet(sender[i].Detach()); } private void beatmapsChanged(IRealmCollection sender, ChangeSet changes, Exception error) @@ -261,7 +261,7 @@ namespace osu.Game.Screens.Select return; foreach (int i in changes.InsertedIndices) - UpdateBeatmapSet(sender[i].BeatmapSet); + UpdateBeatmapSet(sender[i].BeatmapSet?.Detach()); } private IRealmCollection getBeatmapSets(Realm realm) => realm.All().Where(s => !s.DeletePending && !s.Protected).AsRealmCollection(); @@ -711,8 +711,6 @@ namespace osu.Game.Screens.Select private CarouselBeatmapSet createCarouselSet(BeatmapSetInfo beatmapSet) { - beatmapSet = beatmapSet.Detach(); - // This can be moved to the realm query if required using: // .Filter("DeletePending == false && Protected == false && ANY Beatmaps.Hidden == false") // From 9a864267d2054cb48fe06db37ccbdd9e4b8f26fa Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Thu, 20 Jan 2022 21:57:16 +0900 Subject: [PATCH 2/6] Fix `CarouselGroupEagerSelect` not invoking subclassed `AddChild` from `AddChildren` calls --- .../Select/Carousel/CarouselGroupEagerSelect.cs | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/osu.Game/Screens/Select/Carousel/CarouselGroupEagerSelect.cs b/osu.Game/Screens/Select/Carousel/CarouselGroupEagerSelect.cs index 9e8aad4b6f..aac0e4ed82 100644 --- a/osu.Game/Screens/Select/Carousel/CarouselGroupEagerSelect.cs +++ b/osu.Game/Screens/Select/Carousel/CarouselGroupEagerSelect.cs @@ -55,10 +55,16 @@ namespace osu.Game.Screens.Select.Carousel updateSelectedIndex(); } + private bool addingChildren; + public void AddChildren(IEnumerable items) { + addingChildren = true; + foreach (var i in items) - base.AddChild(i); + AddChild(i); + + addingChildren = false; attemptSelection(); } @@ -66,7 +72,8 @@ namespace osu.Game.Screens.Select.Carousel public override void AddChild(CarouselItem i) { base.AddChild(i); - attemptSelection(); + if (!addingChildren) + attemptSelection(); } protected override void ChildItemStateChanged(CarouselItem item, CarouselItemState value) From 0b93f3c88f2f1d2a6ae7b3e2300d5b77f8606cfb Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Thu, 20 Jan 2022 21:58:16 +0900 Subject: [PATCH 3/6] Add `` dictionary to speed up update operations in carousel --- osu.Game/Screens/Select/BeatmapCarousel.cs | 99 ++++++++++++++-------- 1 file changed, 64 insertions(+), 35 deletions(-) diff --git a/osu.Game/Screens/Select/BeatmapCarousel.cs b/osu.Game/Screens/Select/BeatmapCarousel.cs index 0ee59f7f04..458a987130 100644 --- a/osu.Game/Screens/Select/BeatmapCarousel.cs +++ b/osu.Game/Screens/Select/BeatmapCarousel.cs @@ -95,7 +95,7 @@ namespace osu.Game.Screens.Select protected readonly CarouselScrollContainer Scroll; - private IEnumerable beatmapSets => root.Children.OfType(); + private IEnumerable beatmapSets => root.BeatmapSetsByID.Values; // todo: only used for testing, maybe remove. private bool loadedTestBeatmaps; @@ -117,6 +117,7 @@ namespace osu.Game.Screens.Select newRoot.AddChildren(beatmapSets.Select(s => createCarouselSet(s.Detach())).Where(g => g != null)); root = newRoot; + if (selectedBeatmapSet != null && !beatmapSets.Contains(selectedBeatmapSet.BeatmapSet)) selectedBeatmapSet = null; @@ -209,7 +210,7 @@ namespace osu.Game.Screens.Select return; foreach (int i in changes.InsertedIndices) - RemoveBeatmapSet(sender[i].Detach()); + removeBeatmapSet(sender[i].ID); } private void beatmapSetsChanged(IRealmCollection sender, ChangeSet changes, Exception error) @@ -223,24 +224,20 @@ namespace osu.Game.Screens.Select // During initial population, we must manually account for the fact that our original query was done on an async thread. // Since then, there may have been imports or deletions. // Here we manually catch up on any changes. - var populatedSets = new HashSet(); - foreach (var s in beatmapSets) - populatedSets.Add(s.BeatmapSet.ID); - var realmSets = new HashSet(); foreach (var s in sender) realmSets.Add(s.ID); - foreach (var s in realmSets) + foreach (var id in realmSets) { - if (!populatedSets.Contains(s)) - UpdateBeatmapSet(realmFactory.Context.Find(s)); + if (!root.BeatmapSetsByID.ContainsKey(id)) + UpdateBeatmapSet(realmFactory.Context.Find(id).Detach()); } - foreach (var s in populatedSets) + foreach (var id in root.BeatmapSetsByID.Keys) { - if (!realmSets.Contains(s)) - RemoveBeatmapSet(realmFactory.Context.Find(s)); + if (!realmSets.Contains(id)) + removeBeatmapSet(id); } signalBeatmapsLoaded(); @@ -261,16 +258,30 @@ namespace osu.Game.Screens.Select return; foreach (int i in changes.InsertedIndices) - UpdateBeatmapSet(sender[i].BeatmapSet?.Detach()); + { + var beatmapInfo = sender[i]; + var beatmapSet = beatmapInfo.BeatmapSet; + + Debug.Assert(beatmapSet != null); + + // Only require to action here if the beatmap is missing. + // This avoids processing these events unnecessarily when new beatmaps are imported, for example. + if (root.BeatmapSetsByID.TryGetValue(beatmapSet.ID, out var existingSet) + && existingSet.BeatmapSet.Beatmaps.All(b => b.ID != beatmapInfo.ID)) + { + UpdateBeatmapSet(beatmapSet.Detach()); + } + } } private IRealmCollection getBeatmapSets(Realm realm) => realm.All().Where(s => !s.DeletePending && !s.Protected).AsRealmCollection(); - public void RemoveBeatmapSet(BeatmapSetInfo beatmapSet) => Schedule(() => - { - var existingSet = beatmapSets.FirstOrDefault(b => b.BeatmapSet.Equals(beatmapSet)); + public void RemoveBeatmapSet(BeatmapSetInfo beatmapSet) => + removeBeatmapSet(beatmapSet.ID); - if (existingSet == null) + private void removeBeatmapSet(Guid beatmapSetID) => Schedule(() => + { + if (!root.BeatmapSetsByID.TryGetValue(beatmapSetID, out var existingSet)) return; root.RemoveChild(existingSet); @@ -281,33 +292,27 @@ namespace osu.Game.Screens.Select { Guid? previouslySelectedID = null; - CarouselBeatmapSet existingSet = beatmapSets.FirstOrDefault(b => b.BeatmapSet.Equals(beatmapSet)); - // If the selected beatmap is about to be removed, store its ID so it can be re-selected if required - if (existingSet?.State?.Value == CarouselItemState.Selected) + if (selectedBeatmapSet?.BeatmapSet.ID == beatmapSet.ID) previouslySelectedID = selectedBeatmap?.BeatmapInfo.ID; var newSet = createCarouselSet(beatmapSet); - if (existingSet != null) - root.RemoveChild(existingSet); + root.RemoveChild(beatmapSet.ID); - if (newSet == null) + if (newSet != null) { - itemsCache.Invalidate(); - return; + root.AddChild(newSet); + + // only reset scroll position if already near the scroll target. + // without this, during a large beatmap import it is impossible to navigate the carousel. + applyActiveCriteria(false, alwaysResetScrollPosition: false); + + // check if we can/need to maintain our current selection. + if (previouslySelectedID != null) + select((CarouselItem)newSet.Beatmaps.FirstOrDefault(b => b.BeatmapInfo.ID == previouslySelectedID) ?? newSet); } - root.AddChild(newSet); - - // only reset scroll position if already near the scroll target. - // without this, during a large beatmap import it is impossible to navigate the carousel. - applyActiveCriteria(false, alwaysResetScrollPosition: false); - - // check if we can/need to maintain our current selection. - if (previouslySelectedID != null) - select((CarouselItem)newSet.Beatmaps.FirstOrDefault(b => b.BeatmapInfo.ID == previouslySelectedID) ?? newSet); - itemsCache.Invalidate(); Schedule(() => BeatmapSetsChanged?.Invoke()); }); @@ -911,6 +916,8 @@ namespace osu.Game.Screens.Select { private readonly BeatmapCarousel carousel; + public readonly Dictionary BeatmapSetsByID = new Dictionary(); + public CarouselRoot(BeatmapCarousel carousel) { // root should always remain selected. if not, PerformSelection will not be called. @@ -920,6 +927,28 @@ namespace osu.Game.Screens.Select this.carousel = carousel; } + public override void AddChild(CarouselItem i) + { + CarouselBeatmapSet set = (CarouselBeatmapSet)i; + BeatmapSetsByID[set.BeatmapSet.ID] = set; + + base.AddChild(i); + } + + public void RemoveChild(Guid beatmapSetID) + { + if (BeatmapSetsByID.TryGetValue(beatmapSetID, out var carouselBeatmapSet)) + RemoveChild(carouselBeatmapSet); + } + + public override void RemoveChild(CarouselItem i) + { + CarouselBeatmapSet set = (CarouselBeatmapSet)i; + BeatmapSetsByID.Remove(set.BeatmapSet.ID); + + base.RemoveChild(i); + } + protected override void PerformSelection() { if (LastSelected == null || LastSelected.Filtered.Value) From 80f3a67876adcad87ba910e41e7bc82fd46cf820 Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Thu, 20 Jan 2022 22:21:00 +0900 Subject: [PATCH 4/6] Use `for` instead of `foreach` to avoid enumerator overhead --- osu.Game/Screens/Select/BeatmapCarousel.cs | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/osu.Game/Screens/Select/BeatmapCarousel.cs b/osu.Game/Screens/Select/BeatmapCarousel.cs index 458a987130..a5704b3b2e 100644 --- a/osu.Game/Screens/Select/BeatmapCarousel.cs +++ b/osu.Game/Screens/Select/BeatmapCarousel.cs @@ -225,8 +225,9 @@ namespace osu.Game.Screens.Select // Since then, there may have been imports or deletions. // Here we manually catch up on any changes. var realmSets = new HashSet(); - foreach (var s in sender) - realmSets.Add(s.ID); + + for (int i = 0; i < sender.Count; i++) + realmSets.Add(sender[i].ID); foreach (var id in realmSets) { From ba31ddee017a950be2c8b50c4c9750ebeb575af6 Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Fri, 21 Jan 2022 00:33:46 +0900 Subject: [PATCH 5/6] Revert `beatmapSets` reference to fix tests New version changed order. --- osu.Game/Screens/Select/BeatmapCarousel.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/osu.Game/Screens/Select/BeatmapCarousel.cs b/osu.Game/Screens/Select/BeatmapCarousel.cs index a5704b3b2e..961dac9856 100644 --- a/osu.Game/Screens/Select/BeatmapCarousel.cs +++ b/osu.Game/Screens/Select/BeatmapCarousel.cs @@ -95,7 +95,7 @@ namespace osu.Game.Screens.Select protected readonly CarouselScrollContainer Scroll; - private IEnumerable beatmapSets => root.BeatmapSetsByID.Values; + private IEnumerable beatmapSets => root.Children.OfType(); // todo: only used for testing, maybe remove. private bool loadedTestBeatmaps; From 3bcdce128c5927a90f718b0ef54d76678272648a Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Fri, 21 Jan 2022 15:29:21 +0900 Subject: [PATCH 6/6] Use dictionary add for safety --- osu.Game/Screens/Select/BeatmapCarousel.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/osu.Game/Screens/Select/BeatmapCarousel.cs b/osu.Game/Screens/Select/BeatmapCarousel.cs index 3cd9253eff..e8171d1512 100644 --- a/osu.Game/Screens/Select/BeatmapCarousel.cs +++ b/osu.Game/Screens/Select/BeatmapCarousel.cs @@ -934,7 +934,7 @@ namespace osu.Game.Screens.Select public override void AddChild(CarouselItem i) { CarouselBeatmapSet set = (CarouselBeatmapSet)i; - BeatmapSetsByID[set.BeatmapSet.ID] = set; + BeatmapSetsByID.Add(set.BeatmapSet.ID, set); base.AddChild(i); }