Split out beatmap update tasks to BeatmapUpdater and invoke from editor save flow
#18835
Conversation
| // For now, just fire off a task. | ||
| // TODO: Add actual queueing probably. | ||
| Task.Factory.StartNew(() => beatmap.PerformRead(Process)); |
There was a problem hiding this comment.
This is intended to be called from other sources, such as a server event which suggests a beatmap has been updated. I'm not sure whether this should be split out into smaller pieces (ie. in the case of an online update difficulty etc. would not need to be updated), potentially a [Flags] enum with the pieces that require update.
Can remove for now if preferred.
osu.Game/Beatmaps/BeatmapManager.cs
Outdated
| /// <param name="beatmapInfo">The beatmap to lookup.</param> | ||
| /// <param name="refetch">Whether to force a refetch from the database to ensure <see cref="BeatmapInfo"/> is up-to-date.</param> | ||
| /// <returns>A <see cref="WorkingBeatmap"/> instance correlating to the provided <see cref="BeatmapInfo"/>.</returns> | ||
| public WorkingBeatmap GetWorkingBeatmap(BeatmapInfo beatmapInfo, bool refetch = false) |
There was a problem hiding this comment.
Not sure why beatmapInfo is no longer allowed to be nullable, but this raises complaints from both the compiler (see: SongSelect), and tests (see: many failures in multiplayer tests). Dropping the nullable spec doesn't even look to be that much of an issue...?
There was a problem hiding this comment.
I removed this because it felt really awkward, but have added back for the time being, can revisit later on.
| @@ -25,5 +27,7 @@ public interface IWorkingBeatmapCache | |||
| /// </summary> | |||
| /// <param name="beatmapInfo">The beatmap info to invalidate any cached entries for.</param> | |||
| void Invalidate(BeatmapInfo beatmapInfo); | |||
|
|
|||
| event Action<WorkingBeatmap> OnInvalidated; | |||
There was a problem hiding this comment.
Probably should receive xmldoc.
osu.Game/Beatmaps/BeatmapUpdater.cs
Outdated
| #nullable enable | ||
|
|
There was a problem hiding this comment.
| #nullable enable |
| public class BeatmapUpdater | ||
| { | ||
| private readonly IWorkingBeatmapCache workingBeatmapCache; | ||
| private readonly BeatmapOnlineLookupQueue onlineLookupQueue; |
There was a problem hiding this comment.
This I find a touch awkward. BeatmapUpdater receives a reference to a BeatmapOnlineLookupQueue which is IDisposable, but the updater itself is not disposable. Currently the online lookup queue is being disposed in BeatmapManager which constructs this, but maybe it'd be better to have this use "ownership transfer" semantics, i.e. after BeatmapUpdater receives the instance, it should be responsible for cleaning it up? Not sure.
There was a problem hiding this comment.
I've moved BeatmapOnlineLookupQueue inside this component, since it's the only place it's used now.
| @@ -39,6 +40,15 @@ protected async Task<TValue> GetAsync([NotNull] TLookup lookup, CancellationToke | |||
| return computed; | |||
| } | |||
|
|
|||
| protected void Invalidate(Func<TLookup, bool> invalidationFunction) | |||
There was a problem hiding this comment.
Param needs a better name like matchKeyPredicate or something (or xmldoc). I wasn't sure what this was supposed to mean initially.
| { | ||
| // Not sure if this refresh is required. | ||
| var beatmapInfo = beatmaps.QueryBeatmap(b => b.ID == w.BeatmapInfo.ID); | ||
| Beatmap.Value = beatmaps.GetWorkingBeatmap(beatmapInfo); |
There was a problem hiding this comment.
As per test failures, looks like this is not possible to do like this if a subscreen has taken a lease on the global beatmap bindable. Will probably require a workaround like the one I recently added.
There was a problem hiding this comment.
Turns out none of this is needed since I actually got the invalidation logic setup correctly. Was a remnant of a previous attempt.
| @@ -700,6 +716,8 @@ protected override void Dispose(bool isDisposing) | |||
| music.TrackChanged -= ensureTrackLooping; | |||
|
|
|||
| modSelectOverlayRegistration?.Dispose(); | |||
|
|
|||
| beatmaps.OnInvalidated -= workingBeatmapInvalidated; | |||
There was a problem hiding this comment.
beatmaps should be null-checked in case of unfortunate disposal before dependency injection.
| @@ -630,9 +630,7 @@ private void refetchBeatmap() | |||
| // To update the game-wide beatmap with any changes, perform a re-fetch on exit/suspend. | |||
| // This is required as the editor makes its local changes via EditorBeatmap | |||
| // (which are not propagated outwards to a potentially cached WorkingBeatmap). | |||
| ((IWorkingBeatmapCache)beatmapManager).Invalidate(Beatmap.Value.BeatmapInfo); | |||
| var refetchedBeatmapInfo = beatmapManager.QueryBeatmap(b => b.ID == Beatmap.Value.BeatmapInfo.ID); | |||
There was a problem hiding this comment.
Just to confirm intent: the idea here was that the .Save() call results in a BeatmapUpdater.Process() call, which does all of the invalidating this method used to, and then updates the value of Beatmap.Value to the correct post-save-and-invalidation one? Removing the refetch from realm here initially spooked me but if the idea above is what was intended, then I think it's fine (as long as the test failures are sorted).
There was a problem hiding this comment.
The refetch is actually done via the bool parameter now.
|
This should be ready for another review now. |
|
Will review after test failures are fixed. |
|
Weird, I got that failure locally but it seemed unrelated at first (only happened when running tests in certain order). Will take another look. |
I'm opening this mainly as an RFC. I want to add a bit more test coverage (of the
DifficultyCacheinvalidation, mostly) still.Quite a few changes here, so let me know if anything doesn't look right of if you'd prefer the PR is split into more pieces.