Skip to content

Commit

Permalink
Merge pull request #25838 from peppy/fix-song-select-realm-refresh-pe…
Browse files Browse the repository at this point in the history 
…rformance

Fix song select realm refresh performance
  • Loading branch information
@bdach
bdach authored Dec 19, 2023
2 parents 1161e0a + e306cc3 commit b869973
Show file tree
Hide file tree
Showing 4 changed files with 84 additions and 40 deletions.
4 changes: 2 additions & 2 deletions osu.Game.Tests/Visual/SongSelect/TestScenePlaySongSelect.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -464,7 +464,7 @@ public void TestSelectionRetainedOnBeatmapUpdate()
manager.Import(testBeatmapSetInfo);
}, 10);

AddUntilStep("has selection", () => songSelect!.Carousel.SelectedBeatmapInfo?.BeatmapSet?.OnlineID == originalOnlineSetID);
AddUntilStep("has selection", () => songSelect!.Carousel.SelectedBeatmapInfo?.BeatmapSet?.OnlineID, () => Is.EqualTo(originalOnlineSetID));

Task<Live<BeatmapSetInfo>?> updateTask = null!;

Expand All @@ -476,7 +476,7 @@ public void TestSelectionRetainedOnBeatmapUpdate()
});
AddUntilStep("wait for update completion", () => updateTask.IsCompleted);

AddUntilStep("retained selection", () => songSelect!.Carousel.SelectedBeatmapInfo?.BeatmapSet?.OnlineID == originalOnlineSetID);
AddUntilStep("retained selection", () => songSelect!.Carousel.SelectedBeatmapInfo?.BeatmapSet?.OnlineID, () => Is.EqualTo(originalOnlineSetID));
}

[Test]
Expand Down
96 changes: 62 additions & 34 deletions osu.Game/Screens/Select/BeatmapCarousel.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,7 @@ public partial class BeatmapCarousel : CompositeDrawable, IKeyBindingHandler<Glo
/// <summary>
/// The total count of non-filtered beatmaps displayed.
/// </summary>
public int CountDisplayed => beatmapSets.Where(s => !s.Filtered.Value).Sum(s => s.Beatmaps.Count(b => !b.Filtered.Value));
public int CountDisplayed => beatmapSets.Where(s => !s.Filtered.Value).Sum(s => s.TotalItemsNotFiltered);

/// <summary>
/// The currently selected beatmap set.
Expand Down Expand Up @@ -168,7 +168,10 @@ private void loadBeatmapSets(IEnumerable<BeatmapSetInfo> beatmapSets)
applyActiveCriteria(false);

if (loadedTestBeatmaps)
signalBeatmapsLoaded();
{
invalidateAfterChange();
BeatmapSetsLoaded = true;
}

// Restore selection
if (selectedBeatmapBefore != null && newRoot.BeatmapSetsByID.TryGetValue(selectedBeatmapBefore.BeatmapSet!.ID, out var newSelectionCandidates))
Expand Down Expand Up @@ -266,8 +269,30 @@ private void deletedBeatmapSetsChanged(IRealmCollection<BeatmapSetInfo> sender,
if (changes == null)
return;

foreach (int i in changes.InsertedIndices)
removeBeatmapSet(sender[i].ID);
var removeableSets = changes.InsertedIndices.Select(i => sender[i].ID).ToHashSet();

// This schedule is required to retain selection of beatmaps over an ImportAsUpdate operation.
// This is covered by TestPlaySongSelect.TestSelectionRetainedOnBeatmapUpdate.
//
// In short, we have specialised logic in `beatmapSetsChanged` (directly below) to infer that an
// update operation has occurred. For this to work, we need to confirm the `DeletePending` flag
// of the current selection.
//
// If we don't schedule the following code, it is possible for the `deleteBeatmapSetsChanged` handler
// to be invoked before the `beatmapSetsChanged` handler (realm call order seems non-deterministic)
// which will lead to the currently selected beatmap changing via `CarouselGroupEagerSelect`.
//
// We need a better path forward here. A few ideas:
// - Avoid the necessity of having realm subscriptions on deleted/hidden items, maybe by storing all guids in realm
// to a local list so we can better look them up on receiving `DeletedIndices`.
// - Add a new property on `BeatmapSetInfo` to link to the pre-update set, and use that to handle the update case.
Schedule(() =>
{
foreach (var set in removeableSets)
removeBeatmapSet(set);

invalidateAfterChange();
});
}

private void beatmapSetsChanged(IRealmCollection<BeatmapSetInfo> sender, ChangeSet? changes)
Expand All @@ -289,7 +314,7 @@ private void beatmapSetsChanged(IRealmCollection<BeatmapSetInfo> sender, ChangeS
foreach (var id in realmSets)
{
if (!root.BeatmapSetsByID.ContainsKey(id))
UpdateBeatmapSet(realm.Realm.Find<BeatmapSetInfo>(id)!.Detach());
updateBeatmapSet(realm.Realm.Find<BeatmapSetInfo>(id)!.Detach());
}

foreach (var id in root.BeatmapSetsByID.Keys)
Expand All @@ -298,15 +323,16 @@ private void beatmapSetsChanged(IRealmCollection<BeatmapSetInfo> sender, ChangeS
removeBeatmapSet(id);
}

signalBeatmapsLoaded();
invalidateAfterChange();
BeatmapSetsLoaded = true;
return;
}

foreach (int i in changes.NewModifiedIndices)
UpdateBeatmapSet(sender[i].Detach());
updateBeatmapSet(sender[i].Detach());

foreach (int i in changes.InsertedIndices)
UpdateBeatmapSet(sender[i].Detach());
updateBeatmapSet(sender[i].Detach());

if (changes.DeletedIndices.Length > 0 && SelectedBeatmapInfo != null)
{
Expand All @@ -316,7 +342,7 @@ private void beatmapSetsChanged(IRealmCollection<BeatmapSetInfo> sender, ChangeS
// To handle the beatmap update flow, attempt to track selection changes across delete-insert transactions.
// When an update occurs, the previous beatmap set is either soft or hard deleted.
// Check if the current selection was potentially deleted by re-querying its validity.
bool selectedSetMarkedDeleted = realm.Run(r => r.Find<BeatmapSetInfo>(SelectedBeatmapSet.ID))?.DeletePending != false;
bool selectedSetMarkedDeleted = sender.Realm.Find<BeatmapSetInfo>(SelectedBeatmapSet.ID)?.DeletePending != false;

int[] modifiedAndInserted = changes.NewModifiedIndices.Concat(changes.InsertedIndices).ToArray();

Expand Down Expand Up @@ -347,6 +373,8 @@ private void beatmapSetsChanged(IRealmCollection<BeatmapSetInfo> sender, ChangeS
SelectBeatmap(sender[modifiedAndInserted.First()].Beatmaps.First());
}
}

invalidateAfterChange();
}

private void beatmapsChanged(IRealmCollection<BeatmapInfo> sender, ChangeSet? changes)
Expand All @@ -355,6 +383,8 @@ private void beatmapsChanged(IRealmCollection<BeatmapInfo> sender, ChangeSet? ch
if (changes == null)
return;

bool changed = false;

foreach (int i in changes.InsertedIndices)
{
var beatmapInfo = sender[i];
Expand All @@ -367,17 +397,24 @@ private void beatmapsChanged(IRealmCollection<BeatmapInfo> sender, ChangeSet? ch
if (root.BeatmapSetsByID.TryGetValue(beatmapSet.ID, out var existingSets)
&& existingSets.SelectMany(s => s.Beatmaps).All(b => b.BeatmapInfo.ID != beatmapInfo.ID))
{
UpdateBeatmapSet(beatmapSet.Detach());
updateBeatmapSet(beatmapSet.Detach());
changed = true;
}
}

if (changed)
invalidateAfterChange();
}

private IQueryable<BeatmapSetInfo> getBeatmapSets(Realm realm) => realm.All<BeatmapSetInfo>().Where(s => !s.DeletePending && !s.Protected);

public void RemoveBeatmapSet(BeatmapSetInfo beatmapSet) =>
public void RemoveBeatmapSet(BeatmapSetInfo beatmapSet) => Schedule(() =>
{
removeBeatmapSet(beatmapSet.ID);
invalidateAfterChange();
});

private void removeBeatmapSet(Guid beatmapSetID) => Schedule(() =>
private void removeBeatmapSet(Guid beatmapSetID)
{
if (!root.BeatmapSetsByID.TryGetValue(beatmapSetID, out var existingSets))
return;
Expand All @@ -392,16 +429,15 @@ private void removeBeatmapSet(Guid beatmapSetID) => Schedule(() =>

root.RemoveItem(set);
}
}

itemsCache.Invalidate();

if (!Scroll.UserScrolling)
ScrollToSelected(true);

BeatmapSetsChanged?.Invoke();
public void UpdateBeatmapSet(BeatmapSetInfo beatmapSet) => Schedule(() =>
{
updateBeatmapSet(beatmapSet);
invalidateAfterChange();
});

public void UpdateBeatmapSet(BeatmapSetInfo beatmapSet) => Schedule(() =>
private void updateBeatmapSet(BeatmapSetInfo beatmapSet)
{
Guid? previouslySelectedID = null;

Expand Down Expand Up @@ -464,14 +500,7 @@ public void UpdateBeatmapSet(BeatmapSetInfo beatmapSet) => Schedule(() =>
select((CarouselItem?)newSet.Beatmaps.FirstOrDefault(b => b.BeatmapInfo.ID == previouslySelectedID) ?? newSet);
}
}

itemsCache.Invalidate();

if (!Scroll.UserScrolling)
ScrollToSelected(true);

BeatmapSetsChanged?.Invoke();
});
}

/// <summary>
/// Selects a given beatmap on the carousel.
Expand Down Expand Up @@ -748,15 +777,14 @@ void perform()
}
}

private void signalBeatmapsLoaded()
private void invalidateAfterChange()
{
if (!BeatmapSetsLoaded)
{
BeatmapSetsChanged?.Invoke();
BeatmapSetsLoaded = true;
}

itemsCache.Invalidate();

if (!Scroll.UserScrolling)
ScrollToSelected(true);

BeatmapSetsChanged?.Invoke();
}

private float? scrollTarget;
Expand Down
17 changes: 16 additions & 1 deletion osu.Game/Screens/Select/Carousel/CarouselGroup.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,8 @@ public class CarouselGroup : CarouselItem

public IReadOnlyList<CarouselItem> Items => items;

public int TotalItemsNotFiltered { get; private set; }

private readonly List<CarouselItem> items = new List<CarouselItem>();

/// <summary>
Expand All @@ -31,6 +33,9 @@ public virtual void RemoveItem(CarouselItem i)
{
items.Remove(i);

if (!i.Filtered.Value)
TotalItemsNotFiltered--;

// it's important we do the deselection after removing, so any further actions based on
// State.ValueChanged make decisions post-removal.
i.State.Value = CarouselItemState.Collapsed;
Expand All @@ -55,6 +60,9 @@ public virtual void AddItem(CarouselItem i)
// criteria may be null for initial population. the filtering will be applied post-add.
items.Add(i);
}

if (!i.Filtered.Value)
TotalItemsNotFiltered++;
}

public CarouselGroup(List<CarouselItem>? items = null)
Expand Down Expand Up @@ -84,7 +92,14 @@ public override void Filter(FilterCriteria criteria)
{
base.Filter(criteria);

items.ForEach(c => c.Filter(criteria));
TotalItemsNotFiltered = 0;

foreach (var c in items)
{
c.Filter(criteria);
if (!c.Filtered.Value)
TotalItemsNotFiltered++;
}

// Sorting is expensive, so only perform if it's actually changed.
if (lastCriteria?.RequiresSorting(criteria) != false)
Expand Down
7 changes: 4 additions & 3 deletions osu.Game/Screens/Select/SongSelect.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -162,7 +162,7 @@ private void load(AudioManager audio, OsuColour colours, ManageCollectionsDialog
BleedBottom = Footer.HEIGHT,
SelectionChanged = updateSelectedBeatmap,
BeatmapSetsChanged = carouselBeatmapsLoaded,
FilterApplied = updateVisibleBeatmapCount,
FilterApplied = () => Scheduler.AddOnce(updateVisibleBeatmapCount),
GetRecommendedBeatmap = s => recommender?.GetRecommendedBeatmap(s),
}, c => carouselContainer.Child = c);

Expand Down Expand Up @@ -843,7 +843,7 @@ private void ensurePlayingSelected()
private void carouselBeatmapsLoaded()
{
bindBindables();
updateVisibleBeatmapCount();
Scheduler.AddOnce(updateVisibleBeatmapCount);

Carousel.AllowSelection = true;

Expand Down Expand Up @@ -877,7 +877,8 @@ private void updateVisibleBeatmapCount()
{
// Intentionally not localised until we have proper support for this (see https://github.com/ppy/osu-framework/pull/4918
// but also in this case we want support for formatting a number within a string).
FilterControl.InformationalText = Carousel.CountDisplayed != 1 ? $"{Carousel.CountDisplayed:#,0} matches" : $"{Carousel.CountDisplayed:#,0} match";
int carouselCountDisplayed = Carousel.CountDisplayed;
FilterControl.InformationalText = carouselCountDisplayed != 1 ? $"{carouselCountDisplayed:#,0} matches" : $"{carouselCountDisplayed:#,0} match";
}

private bool boundLocalBindables;
Expand Down

0 comments on commit b869973

Please sign in to comment.