h1or4dme8/osu-lazer
1
0
Fork 0
mirror of https://github.com/ppy/osu.git synced 2025-01-26 18:52:55 +08:00
Code Issues Releases Wiki Activity

Merge pull request #28769 from peppy/carousel-realm-simplification

Browse Source 
Simplify realm model tracking in `BeatmapCarousel` (and fix hard delete handling)
This commit is contained in:
Dean Herbert 2024-07-09 01:40:10 +09:00 committed by GitHub
commit c100d1ab65
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
2 changed files with 85 additions and 109 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

14
osu.Game.Tests/Visual/SongSelect/TestScenePlaySongSelect.cs
View File

@ -1211,6 +1211,20 @@ namespace osu.Game.Tests.Visual.SongSelect
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()
{

180
osu.Game/Screens/Select/BeatmapCarousel.cs
View File

@ -197,9 +197,7 @@ namespace osu.Game.Screens.Select
private CarouselRoot root;
private IDisposable? subscriptionSets;
private IDisposable? subscriptionDeletedSets;
private IDisposable? subscriptionBeatmaps;
private IDisposable? subscriptionHiddenBeatmaps;
private readonly DrawablePool<DrawableCarouselBeatmapSet> setPool = new DrawablePool<DrawableCarouselBeatmapSet>(100);
@ -246,6 +244,9 @@ namespace osu.Game.Screens.Select
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)));
}
}
@ -253,53 +254,21 @@ namespace osu.Game.Screens.Select
[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)
{
@ -307,96 +276,91 @@ namespace osu.Game.Screens.Select
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));
setsRequiringRemoval.Clear();
setsRequiringUpdate.Clear();
for (int i = 0; i < sender.Count; i++)
realmSets.Add(sender[i].ID);
foreach (var id in realmSets)
{
if (!root.BeatmapSetsByID.ContainsKey(id))
setsRequiringUpdate.Add(realm.Realm.Find<BeatmapSetInfo>(id)!.Detach());
}
foreach (var id in root.BeatmapSetsByID.Keys)
{
if (!realmSets.Contains(id))
setsRequiringRemoval.Add(id);
}
loadBeatmapSets(sender);
}
else
{
foreach (int i in changes.NewModifiedIndices)
setsRequiringUpdate.Add(sender[i].Detach());
foreach (int i in changes.DeletedIndices.OrderDescending())
{
setsRequiringRemoval.Add(realmBeatmapSets[i]);
realmBeatmapSets.RemoveAt(i);
}
foreach (int i in changes.InsertedIndices)
{
realmBeatmapSets.Insert(i, sender[i].ID);
setsRequiringUpdate.Add(sender[i].Detach());
}
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 setsRequiringUpdate) updateBeatmapSet(set);
if (setsRequiringRemoval.Count > 0 && SelectedBeatmapInfo != null)
{
foreach (var set in setsRequiringRemoval)
removeBeatmapSet(set);
// If SelectedBeatmapInfo is non-null, the set should also be non-null.
Debug.Assert(SelectedBeatmapSet != null);
foreach (var set in setsRequiringUpdate)
updateBeatmapSet(set);
// 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 (changes?.DeletedIndices.Length > 0 && SelectedBeatmapInfo != null)
if (selectedSetMarkedDeleted && setsRequiringUpdate.Any())
{
// 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);
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;
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;
}
// 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;
}
}
// 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)
@ -1312,9 +1276,7 @@ namespace osu.Game.Screens.Select
base.Dispose(isDisposing);
subscriptionSets?.Dispose();
subscriptionDeletedSets?.Dispose();
subscriptionBeatmaps?.Dispose();
subscriptionHiddenBeatmaps?.Dispose();
}
}
}