Merge pull request #16213 from peppy/working-beatmap-live

Tidy up `WorkingBeatmap`

osu.Game.Tests/Visual/Gameplay/TestSceneStoryboardSamplePlayback.cs

@@ -57,7 +57,7 @@ namespace osu.Game.Tests.Visual.Gameplay
 
         private void checkForFirstSamplePlayback()
         {
-            AddUntilStep("storyboard loaded", () => Player.Beatmap.Value.StoryboardLoaded);
+            AddAssert("storyboard loaded", () => Player.Beatmap.Value.Storyboard != null);
             AddUntilStep("any storyboard samples playing", () => allStoryboardSamples.Any(sound => sound.IsPlaying));
         }
 

osu.Game.Tests/Visual/Navigation/TestSceneScreenNavigation.cs

@@ -157,12 +157,12 @@ namespace osu.Game.Tests.Visual.Navigation
             AddUntilStep("wait for fail", () => player.HasFailed);
 
             AddUntilStep("wait for track stop", () => !Game.MusicController.IsPlaying);
-            AddAssert("Ensure time before preview point", () => Game.MusicController.CurrentTrack.CurrentTime < beatmap().Metadata.PreviewTime);
+            AddAssert("Ensure time before preview point", () => Game.MusicController.CurrentTrack.CurrentTime < beatmap().BeatmapInfo.Metadata.PreviewTime);
 
             pushEscape();
 
             AddUntilStep("wait for track playing", () => Game.MusicController.IsPlaying);
-            AddAssert("Ensure time wasn't reset to preview point", () => Game.MusicController.CurrentTrack.CurrentTime < beatmap().Metadata.PreviewTime);
+            AddAssert("Ensure time wasn't reset to preview point", () => Game.MusicController.CurrentTrack.CurrentTime < beatmap().BeatmapInfo.Metadata.PreviewTime);
         }
 
         [Test]

osu.Game/Beatmaps/IWorkingBeatmap.cs

@@ -15,39 +15,18 @@ using osu.Game.Storyboards;
 
 namespace osu.Game.Beatmaps
 {
+    /// <summary>
+    /// Provides access to the multiple resources offered by a beatmap model (textures, skins, playable beatmaps etc.)
+    /// </summary>
     public interface IWorkingBeatmap
     {
         IBeatmapInfo BeatmapInfo { get; }
 
-        IBeatmapSetInfo BeatmapSetInfo { get; }
-
-        IBeatmapMetadataInfo Metadata { get; }
-
         /// <summary>
         /// Whether the Beatmap has finished loading.
         ///</summary>
         public bool BeatmapLoaded { get; }
 
-        /// <summary>
-        /// Whether the Background has finished loading.
-        ///</summary>
-        public bool BackgroundLoaded { get; }
-
-        /// <summary>
-        /// Whether the Waveform has finished loading.
-        ///</summary>
-        public bool WaveformLoaded { get; }
-
-        /// <summary>
-        /// Whether the Storyboard has finished loading.
-        ///</summary>
-        public bool StoryboardLoaded { get; }
-
-        /// <summary>
-        /// Whether the Skin has finished loading.
-        ///</summary>
-        public bool SkinLoaded { get; }
-
         /// <summary>
         /// Whether the Track has finished loading.
         ///</summary>

osu.Game/Beatmaps/WorkingBeatmap.cs

@@ -28,24 +28,126 @@ namespace osu.Game.Beatmaps
     {
         public readonly BeatmapInfo BeatmapInfo;
         public readonly BeatmapSetInfo BeatmapSetInfo;
-        public readonly BeatmapMetadata Metadata;
 
-        protected AudioManager AudioManager { get; }
+        // TODO: remove once the fallback lookup is not required (and access via `working.BeatmapInfo.Metadata` directly).
+        public BeatmapMetadata Metadata => BeatmapInfo.Metadata ?? BeatmapSetInfo?.Metadata ?? new BeatmapMetadata();
+
+        public Waveform Waveform => waveform.Value;
+
+        public Storyboard Storyboard => storyboard.Value;
+
+        public Texture Background => GetBackground(); // Texture uses ref counting, so we want to return a new instance every usage.
+
+        public ISkin Skin => skin.Value;
+
+        private AudioManager audioManager { get; }
+
+        private CancellationTokenSource loadCancellationSource = new CancellationTokenSource();
+
+        private readonly object beatmapFetchLock = new object();
+
+        private readonly Lazy<Waveform> waveform;
+        private readonly Lazy<Storyboard> storyboard;
+        private readonly Lazy<ISkin> skin;
+        private Track track; // track is not Lazy as we allow transferring and loading multiple times.
 
         protected WorkingBeatmap(BeatmapInfo beatmapInfo, AudioManager audioManager)
         {
-            AudioManager = audioManager;
+            this.audioManager = audioManager;
+
             BeatmapInfo = beatmapInfo;
             BeatmapSetInfo = beatmapInfo.BeatmapSet;
-            Metadata = beatmapInfo.Metadata ?? BeatmapSetInfo?.Metadata ?? new BeatmapMetadata();
 
-            background = new RecyclableLazy<Texture>(GetBackground, BackgroundStillValid);
+            waveform = new Lazy<Waveform>(GetWaveform);
-            waveform = new RecyclableLazy<Waveform>(GetWaveform);
+            storyboard = new Lazy<Storyboard>(GetStoryboard);
-            storyboard = new RecyclableLazy<Storyboard>(GetStoryboard);
+            skin = new Lazy<ISkin>(GetSkin);
-            skin = new RecyclableLazy<ISkin>(GetSkin);
         }
 
-        protected virtual Track GetVirtualTrack(double emptyLength = 0)
+        #region Resource getters
+
+        protected virtual Waveform GetWaveform() => new Waveform(null);
+        protected virtual Storyboard GetStoryboard() => new Storyboard { BeatmapInfo = BeatmapInfo };
+
+        protected abstract IBeatmap GetBeatmap();
+        protected abstract Texture GetBackground();
+        protected abstract Track GetBeatmapTrack();
+
+        /// <summary>
+        /// Creates a new skin instance for this beatmap.
+        /// </summary>
+        /// <remarks>
+        /// This should only be called externally in scenarios where it is explicitly desired to get a new instance of a skin
+        /// (e.g. for editing purposes, to avoid state pollution).
+        /// For standard reading purposes, <see cref="Skin"/> should always be used directly.
+        /// </remarks>
+        protected internal abstract ISkin GetSkin();
+
+        #endregion
+
+        #region Async load control
+
+        public void BeginAsyncLoad() => loadBeatmapAsync();
+
+        public void CancelAsyncLoad()
+        {
+            lock (beatmapFetchLock)
+            {
+                loadCancellationSource?.Cancel();
+                loadCancellationSource = new CancellationTokenSource();
+
+                if (beatmapLoadTask?.IsCompleted != true)
+                    beatmapLoadTask = null;
+            }
+        }
+
+        #endregion
+
+        #region Track
+
+        public virtual bool TrackLoaded => track != null;
+
+        public Track LoadTrack() => track = GetBeatmapTrack() ?? GetVirtualTrack(1000);
+
+        public void PrepareTrackForPreviewLooping()
+        {
+            Track.Looping = true;
+            Track.RestartPoint = Metadata.PreviewTime;
+
+            if (Track.RestartPoint == -1)
+            {
+                if (!Track.IsLoaded)
+                {
+                    // force length to be populated (https://github.com/ppy/osu-framework/issues/4202)
+                    Track.Seek(Track.CurrentTime);
+                }
+
+                Track.RestartPoint = 0.4f * Track.Length;
+            }
+        }
+
+        /// <summary>
+        /// Transfer a valid audio track into this working beatmap. Used as an optimisation to avoid reload / track swap
+        /// across difficulties in the same beatmap set.
+        /// </summary>
+        /// <param name="track">The track to transfer.</param>
+        public void TransferTrack([NotNull] Track track) => this.track = track ?? throw new ArgumentNullException(nameof(track));
+
+        /// <summary>
+        /// Get the loaded audio track instance. <see cref="LoadTrack"/> must have first been called.
+        /// This generally happens via MusicController when changing the global beatmap.
+        /// </summary>
+        public Track Track
+        {
+            get
+            {
+                if (!TrackLoaded)
+                    throw new InvalidOperationException($"Cannot access {nameof(Track)} without first calling {nameof(LoadTrack)}.");
+
+                return track;
+            }
+        }
+
+        protected Track GetVirtualTrack(double emptyLength = 0)
         {
             const double excess_length = 1000;
 
@@ -68,18 +170,67 @@ namespace osu.Game.Beatmaps
                     break;
             }
 
-            return AudioManager.Tracks.GetVirtual(length);
+            return audioManager.Tracks.GetVirtual(length);
         }
 
-        /// <summary>
+        #endregion
-        /// Creates a <see cref="IBeatmapConverter"/> to convert a <see cref="IBeatmap"/> for a specified <see cref="Ruleset"/>.
-        /// </summary>
-        /// <param name="beatmap">The <see cref="IBeatmap"/> to be converted.</param>
-        /// <param name="ruleset">The <see cref="Ruleset"/> for which <paramref name="beatmap"/> should be converted.</param>
-        /// <returns>The applicable <see cref="IBeatmapConverter"/>.</returns>
-        protected virtual IBeatmapConverter CreateBeatmapConverter(IBeatmap beatmap, Ruleset ruleset) => ruleset.CreateBeatmapConverter(beatmap);
 
-        public IBeatmap GetPlayableBeatmap([NotNull] IRulesetInfo ruleset, IReadOnlyList<Mod> mods = null)
+        #region Beatmap
+
+        public virtual bool BeatmapLoaded => beatmapLoadTask?.IsCompleted ?? false;
+
+        public IBeatmap Beatmap
+        {
+            get
+            {
+                try
+                {
+                    return loadBeatmapAsync().Result;
+                }
+                catch (AggregateException ae)
+                {
+                    // This is the exception that is generally expected here, which occurs via natural cancellation of the asynchronous load
+                    if (ae.InnerExceptions.FirstOrDefault() is TaskCanceledException)
+                        return null;
+
+                    Logger.Error(ae, "Beatmap failed to load");
+                    return null;
+                }
+                catch (Exception e)
+                {
+                    Logger.Error(e, "Beatmap failed to load");
+                    return null;
+                }
+            }
+        }
+
+        private Task<IBeatmap> beatmapLoadTask;
+
+        private Task<IBeatmap> loadBeatmapAsync()
+        {
+            lock (beatmapFetchLock)
+            {
+                return beatmapLoadTask ??= Task.Factory.StartNew(() =>
+                {
+                    // Todo: Handle cancellation during beatmap parsing
+                    var b = GetBeatmap() ?? new Beatmap();
+
+                    // The original beatmap version needs to be preserved as the database doesn't contain it
+                    BeatmapInfo.BeatmapVersion = b.BeatmapInfo.BeatmapVersion;
+
+                    // Use the database-backed info for more up-to-date values (beatmap id, ranked status, etc)
+                    b.BeatmapInfo = BeatmapInfo;
+
+                    return b;
+                }, loadCancellationSource.Token, TaskCreationOptions.LongRunning, TaskScheduler.Default);
+            }
+        }
+
+        #endregion
+
+        #region Playable beatmap
+
+        public IBeatmap GetPlayableBeatmap(IRulesetInfo ruleset, IReadOnlyList<Mod> mods = null)
         {
             try
             {
@@ -95,7 +246,7 @@ namespace osu.Game.Beatmaps
             }
         }
 
-        public virtual IBeatmap GetPlayableBeatmap([NotNull] IRulesetInfo ruleset, [NotNull] IReadOnlyList<Mod> mods, CancellationToken token)
+        public virtual IBeatmap GetPlayableBeatmap(IRulesetInfo ruleset, IReadOnlyList<Mod> mods, CancellationToken token)
         {
             var rulesetInstance = ruleset.CreateInstance();
 
@@ -169,207 +320,21 @@ namespace osu.Game.Beatmaps
             return converted;
         }
 
-        private CancellationTokenSource loadCancellation = new CancellationTokenSource();
+        /// <summary>
+        /// Creates a <see cref="IBeatmapConverter"/> to convert a <see cref="IBeatmap"/> for a specified <see cref="Ruleset"/>.
+        /// </summary>
+        /// <param name="beatmap">The <see cref="IBeatmap"/> to be converted.</param>
+        /// <param name="ruleset">The <see cref="Ruleset"/> for which <paramref name="beatmap"/> should be converted.</param>
+        /// <returns>The applicable <see cref="IBeatmapConverter"/>.</returns>
+        protected virtual IBeatmapConverter CreateBeatmapConverter(IBeatmap beatmap, Ruleset ruleset) => ruleset.CreateBeatmapConverter(beatmap);
 
-        public void BeginAsyncLoad() => loadBeatmapAsync();
+        #endregion
-
-        public void CancelAsyncLoad()
-        {
-            lock (beatmapFetchLock)
-            {
-                loadCancellation?.Cancel();
-                loadCancellation = new CancellationTokenSource();
-
-                if (beatmapLoadTask?.IsCompleted != true)
-                    beatmapLoadTask = null;
-            }
-        }
-
-        private readonly object beatmapFetchLock = new object();
-
-        private Task<IBeatmap> loadBeatmapAsync()
-        {
-            lock (beatmapFetchLock)
-            {
-                return beatmapLoadTask ??= Task.Factory.StartNew(() =>
-                {
-                    // Todo: Handle cancellation during beatmap parsing
-                    var b = GetBeatmap() ?? new Beatmap();
-
-                    // The original beatmap version needs to be preserved as the database doesn't contain it
-                    BeatmapInfo.BeatmapVersion = b.BeatmapInfo.BeatmapVersion;
-
-                    // Use the database-backed info for more up-to-date values (beatmap id, ranked status, etc)
-                    b.BeatmapInfo = BeatmapInfo;
-
-                    return b;
-                }, loadCancellation.Token, TaskCreationOptions.LongRunning, TaskScheduler.Default);
-            }
-        }
 
         public override string ToString() => BeatmapInfo.ToString();
 
-        public virtual bool BeatmapLoaded => beatmapLoadTask?.IsCompleted ?? false;
-
-        IBeatmapInfo IWorkingBeatmap.BeatmapInfo => BeatmapInfo;
-        IBeatmapMetadataInfo IWorkingBeatmap.Metadata => Metadata;
-        IBeatmapSetInfo IWorkingBeatmap.BeatmapSetInfo => BeatmapSetInfo;
-
-        public IBeatmap Beatmap
-        {
-            get
-            {
-                try
-                {
-                    return loadBeatmapAsync().Result;
-                }
-                catch (AggregateException ae)
-                {
-                    // This is the exception that is generally expected here, which occurs via natural cancellation of the asynchronous load
-                    if (ae.InnerExceptions.FirstOrDefault() is TaskCanceledException)
-                        return null;
-
-                    Logger.Error(ae, "Beatmap failed to load");
-                    return null;
-                }
-                catch (Exception e)
-                {
-                    Logger.Error(e, "Beatmap failed to load");
-                    return null;
-                }
-            }
-        }
-
-        protected abstract IBeatmap GetBeatmap();
-        private Task<IBeatmap> beatmapLoadTask;
-
-        public bool BackgroundLoaded => background.IsResultAvailable;
-        public Texture Background => background.Value;
-        protected virtual bool BackgroundStillValid(Texture b) => b == null || b.Available;
-        protected abstract Texture GetBackground();
-        private readonly RecyclableLazy<Texture> background;
-
-        private Track loadedTrack;
-
-        [NotNull]
-        public Track LoadTrack() => loadedTrack = GetBeatmapTrack() ?? GetVirtualTrack(1000);
-
-        public void PrepareTrackForPreviewLooping()
-        {
-            Track.Looping = true;
-            Track.RestartPoint = Metadata.PreviewTime;
-
-            if (Track.RestartPoint == -1)
-            {
-                if (!Track.IsLoaded)
-                {
-                    // force length to be populated (https://github.com/ppy/osu-framework/issues/4202)
-                    Track.Seek(Track.CurrentTime);
-                }
-
-                Track.RestartPoint = 0.4f * Track.Length;
-            }
-        }
-
-        /// <summary>
-        /// Transfer a valid audio track into this working beatmap. Used as an optimisation to avoid reload / track swap
-        /// across difficulties in the same beatmap set.
-        /// </summary>
-        /// <param name="track">The track to transfer.</param>
-        public void TransferTrack([NotNull] Track track) => loadedTrack = track ?? throw new ArgumentNullException(nameof(track));
-
-        /// <summary>
-        /// Whether this beatmap's track has been loaded via <see cref="LoadTrack"/>.
-        /// </summary>
-        public virtual bool TrackLoaded => loadedTrack != null;
-
-        /// <summary>
-        /// Get the loaded audio track instance. <see cref="LoadTrack"/> must have first been called.
-        /// This generally happens via MusicController when changing the global beatmap.
-        /// </summary>
-        public Track Track
-        {
-            get
-            {
-                if (!TrackLoaded)
-                    throw new InvalidOperationException($"Cannot access {nameof(Track)} without first calling {nameof(LoadTrack)}.");
-
-                return loadedTrack;
-            }
-        }
-
-        protected abstract Track GetBeatmapTrack();
-
-        public bool WaveformLoaded => waveform.IsResultAvailable;
-        public Waveform Waveform => waveform.Value;
-        protected virtual Waveform GetWaveform() => new Waveform(null);
-        private readonly RecyclableLazy<Waveform> waveform;
-
-        public bool StoryboardLoaded => storyboard.IsResultAvailable;
-        public Storyboard Storyboard => storyboard.Value;
-        protected virtual Storyboard GetStoryboard() => new Storyboard { BeatmapInfo = BeatmapInfo };
-        private readonly RecyclableLazy<Storyboard> storyboard;
-
-        public bool SkinLoaded => skin.IsResultAvailable;
-        public ISkin Skin => skin.Value;
-
-        /// <summary>
-        /// Creates a new skin instance for this beatmap.
-        /// </summary>
-        /// <remarks>
-        /// This should only be called externally in scenarios where it is explicitly desired to get a new instance of a skin
-        /// (e.g. for editing purposes, to avoid state pollution).
-        /// For standard reading purposes, <see cref="Skin"/> should always be used directly.
-        /// </remarks>
-        protected internal abstract ISkin GetSkin();
-
-        private readonly RecyclableLazy<ISkin> skin;
-
         public abstract Stream GetStream(string storagePath);
 
-        public class RecyclableLazy<T>
+        IBeatmapInfo IWorkingBeatmap.BeatmapInfo => BeatmapInfo;
-        {
-            private Lazy<T> lazy;
-            private readonly Func<T> valueFactory;
-            private readonly Func<T, bool> stillValidFunction;
-
-            private readonly object fetchLock = new object();
-
-            public RecyclableLazy(Func<T> valueFactory, Func<T, bool> stillValidFunction = null)
-            {
-                this.valueFactory = valueFactory;
-                this.stillValidFunction = stillValidFunction;
-
-                recreate();
-            }
-
-            public void Recycle()
-            {
-                if (!IsResultAvailable) return;
-
-                (lazy.Value as IDisposable)?.Dispose();
-                recreate();
-            }
-
-            public bool IsResultAvailable => stillValid;
-
-            public T Value
-            {
-                get
-                {
-                    lock (fetchLock)
-                    {
-                        if (!stillValid)
-                            recreate();
-                        return lazy.Value;
-                    }
-                }
-            }
-
-            private bool stillValid => lazy.IsValueCreated && (stillValidFunction?.Invoke(lazy.Value) ?? true);
-
-            private void recreate() => lazy = new Lazy<T>(valueFactory, LazyThreadSafetyMode.ExecutionAndPublication);
-        }
 
         private class BeatmapLoadTimeoutException : TimeoutException
         {

osu.Game/Beatmaps/WorkingBeatmapCache.cs

@@ -145,8 +145,6 @@ namespace osu.Game.Beatmaps
                 }
             }
 
-            protected override bool BackgroundStillValid(Texture b) => false; // bypass lazy logic. we want to return a new background each time for refcounting purposes.
-
             protected override Texture GetBackground()
             {
                 if (string.IsNullOrEmpty(Metadata?.BackgroundFile))