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.

Sign up for GitHub

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

Jump to bottom

Simplify realm model tracking in BeatmapCarousel (and fix hard delete handling) #28769

Merged
merged 5 commits into from
Jul 8, 2024
Merged

Simplify realm model tracking in BeatmapCarousel (and fix hard delete handling) #28769

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
e51d510
Add failing test for beatmap set hard deletion
peppy Jul 8, 2024
151c448
Simplify tracking of beatmap sets in `BeatmapCarousel`
peppy Jul 8, 2024
1095137
Simplify tracking of hidden beatmaps
peppy Jul 8, 2024
8f27117
Coalesce beatmap updates
peppy Jul 8, 2024
9a61adc
Ensure other lists are cleared when realm is reset
peppy Jul 8, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 14 additions & 0 deletions osu.Game.Tests/Visual/SongSelect/TestScenePlaySongSelect.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1211,6 +1211,20 @@ public void TestTextBoxBeatmapDifficultyCount()
AddAssert("0 matching shown", () => songSelect.ChildrenOfType<FilterControl>().Single().InformationalText == "0 matches");
}

[Test]
[Solo]
public void TestHardDeleteHandledCorrectly()
{
createSongSelect();

addRulesetImportStep(0);
AddAssert("3 matching shown", () => songSelect.ChildrenOfType<FilterControl>().Single().InformationalText == "3 matches");

AddStep("hard delete beatmap", () => Realm.Write(r => r.RemoveRange(r.All<BeatmapSetInfo>().Where(s => !s.Protected))));

AddUntilStep("0 matching shown", () => songSelect.ChildrenOfType<FilterControl>().Single().InformationalText == "0 matches");
}

[Test]
public void TestDeleteHotkey()
{
Expand Down
178 changes: 70 additions & 108 deletions osu.Game/Screens/Select/BeatmapCarousel.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -197,9 +197,7 @@ private void loadBeatmapSets(IEnumerable<BeatmapSetInfo> beatmapSets)
private CarouselRoot root;

private IDisposable? subscriptionSets;
private IDisposable? subscriptionDeletedSets;
private IDisposable? subscriptionBeatmaps;
private IDisposable? subscriptionHiddenBeatmaps;

private readonly DrawablePool<DrawableCarouselBeatmapSet> setPool = new DrawablePool<DrawableCarouselBeatmapSet>(100);

Expand Down Expand Up @@ -246,157 +244,123 @@ private void load(OsuConfigManager config, AudioManager audio)

if (!loadedTestBeatmaps)
{
// This is performing an unnecessary second lookup on realm (in addition to the subscription), but for performance reasons
// we require it to be separate: the subscription's initial callback (with `ChangeSet` of `null`) will run on the update
// thread. If we attempt to detach beatmaps in this callback the game will fall over (it takes time).
realm.Run(r => loadBeatmapSets(getBeatmapSets(r)));
}
}

[Resolved]
private RealmAccess realm { get; set; } = null!;

/// <summary>
/// Track GUIDs of all sets in realm to allow handling deletions.
/// </summary>
private readonly List<Guid> realmBeatmapSets = new List<Guid>();

protected override void LoadComplete()
{
base.LoadComplete();

subscriptionSets = realm.RegisterForNotifications(getBeatmapSets, beatmapSetsChanged);
subscriptionBeatmaps = realm.RegisterForNotifications(r => r.All<BeatmapInfo>().Where(b => !b.Hidden), beatmapsChanged);

// Can't use main subscriptions because we can't lookup deleted indices.
// https://github.com/realm/realm-dotnet/discussions/2634#discussioncomment-1605595.
subscriptionDeletedSets = realm.RegisterForNotifications(r => r.All<BeatmapSetInfo>().Where(s => s.DeletePending && !s.Protected), deletedBeatmapSetsChanged);
subscriptionHiddenBeatmaps = realm.RegisterForNotifications(r => r.All<BeatmapInfo>().Where(b => b.Hidden), beatmapsChanged);
}

private void deletedBeatmapSetsChanged(IRealmCollection<BeatmapSetInfo> sender, ChangeSet? changes)
{
// If loading test beatmaps, avoid overwriting with realm subscription callbacks.
if (loadedTestBeatmaps)
return;

if (changes == null)
return;

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 readonly HashSet<BeatmapSetInfo> setsRequiringUpdate = new HashSet<BeatmapSetInfo>();
private readonly HashSet<Guid> setsRequiringRemoval = new HashSet<Guid>();

private void beatmapSetsChanged(IRealmCollection<BeatmapSetInfo> sender, ChangeSet? changes)
{
// If loading test beatmaps, avoid overwriting with realm subscription callbacks.
if (loadedTestBeatmaps)
return;

var setsRequiringUpdate = new HashSet<BeatmapSetInfo>();
var setsRequiringRemoval = new HashSet<Guid>();

if (changes == null)
{
// 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 realmSets = new HashSet<Guid>();
realmBeatmapSets.Clear();
realmBeatmapSets.AddRange(sender.Select(r => r.ID));
bdach marked this conversation as resolved.
Show resolved Hide resolved
setsRequiringRemoval.Clear();
setsRequiringUpdate.Clear();

for (int i = 0; i < sender.Count; i++)
realmSets.Add(sender[i].ID);

foreach (var id in realmSets)
loadBeatmapSets(sender);
}
else
{
foreach (int i in changes.DeletedIndices.OrderDescending())
{
if (!root.BeatmapSetsByID.ContainsKey(id))
setsRequiringUpdate.Add(realm.Realm.Find<BeatmapSetInfo>(id)!.Detach());
setsRequiringRemoval.Add(realmBeatmapSets[i]);
realmBeatmapSets.RemoveAt(i);
}

foreach (var id in root.BeatmapSetsByID.Keys)
foreach (int i in changes.InsertedIndices)
{
if (!realmSets.Contains(id))
setsRequiringRemoval.Add(id);
}
}
else
{
foreach (int i in changes.NewModifiedIndices)
realmBeatmapSets.Insert(i, sender[i].ID);
setsRequiringUpdate.Add(sender[i].Detach());
}

foreach (int i in changes.InsertedIndices)
foreach (int i in changes.NewModifiedIndices)
setsRequiringUpdate.Add(sender[i].Detach());
}

// All local operations must be scheduled.
//
// If we don't schedule, beatmaps getting changed while song select is suspended (ie. last played being updated)
// will cause unexpected sounds and operations to occur in the background.
Schedule(() =>
Scheduler.AddOnce(processBeatmapChanges);
}

// All local operations must be scheduled.
//
// If we don't schedule, beatmaps getting changed while song select is suspended (ie. last played being updated)
// will cause unexpected sounds and operations to occur in the background.
private void processBeatmapChanges()
{
try
{
try
{
foreach (var set in setsRequiringRemoval)
removeBeatmapSet(set);
foreach (var set in setsRequiringRemoval) removeBeatmapSet(set);

foreach (var set in setsRequiringUpdate)
updateBeatmapSet(set);
foreach (var set in setsRequiringUpdate) updateBeatmapSet(set);

if (changes?.DeletedIndices.Length > 0 && SelectedBeatmapInfo != null)
{
// If SelectedBeatmapInfo is non-null, the set should also be non-null.
Debug.Assert(SelectedBeatmapSet != null);
if (setsRequiringRemoval.Count > 0 && SelectedBeatmapInfo != null)
{
// If SelectedBeatmapInfo is non-null, the set should also be non-null.
Debug.Assert(SelectedBeatmapSet != null);

// 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);
// 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);

if (selectedSetMarkedDeleted && setsRequiringUpdate.Any())
if (selectedSetMarkedDeleted && setsRequiringUpdate.Any())
{
// If it is no longer valid, make the bold assumption that an updated version will be available in the modified/inserted indices.
// This relies on the full update operation being in a single transaction, so please don't change that.
foreach (var set in setsRequiringUpdate)
{
// If it is no longer valid, make the bold assumption that an updated version will be available in the modified/inserted indices.
// This relies on the full update operation being in a single transaction, so please don't change that.
foreach (var set in setsRequiringUpdate)
foreach (var beatmapInfo in set.Beatmaps)
{
foreach (var beatmapInfo in set.Beatmaps)
if (!((IBeatmapMetadataInfo)beatmapInfo.Metadata).Equals(SelectedBeatmapInfo.Metadata)) continue;

// Best effort matching. We can't use ID because in the update flow a new version will get its own GUID.
if (beatmapInfo.DifficultyName == SelectedBeatmapInfo.DifficultyName)
{
if (!((IBeatmapMetadataInfo)beatmapInfo.Metadata).Equals(SelectedBeatmapInfo.Metadata))
continue;

// Best effort matching. We can't use ID because in the update flow a new version will get its own GUID.
if (beatmapInfo.DifficultyName == SelectedBeatmapInfo.DifficultyName)
{
SelectBeatmap(beatmapInfo);
return;
}
SelectBeatmap(beatmapInfo);
return;
}
}

// If a direct selection couldn't be made, it's feasible that the difficulty name (or beatmap metadata) changed.
// Let's attempt to follow set-level selection anyway.
SelectBeatmap(setsRequiringUpdate.First().Beatmaps.First());
}

// If a direct selection couldn't be made, it's feasible that the difficulty name (or beatmap metadata) changed.
// Let's attempt to follow set-level selection anyway.
SelectBeatmap(setsRequiringUpdate.First().Beatmaps.First());
}
}
finally
{
BeatmapSetsLoaded = true;
invalidateAfterChange();
}
});
}
finally
{
BeatmapSetsLoaded = true;
invalidateAfterChange();
}

setsRequiringRemoval.Clear();
setsRequiringUpdate.Clear();
}

private void beatmapsChanged(IRealmCollection<BeatmapInfo> sender, ChangeSet? changes)
Expand Down Expand Up @@ -1312,9 +1276,7 @@ protected override void Dispose(bool isDisposing)
base.Dispose(isDisposing);

subscriptionSets?.Dispose();
subscriptionDeletedSets?.Dispose();
subscriptionBeatmaps?.Dispose();
subscriptionHiddenBeatmaps?.Dispose();
}
}
}
Loading