Merge pull request #19105 from peppy/fix-editor-disposed-track

Fix audio and background file equality incorrectly comparing `BeatmapSet.Hash`

osu.Game.Tests/NonVisual/BeatmapSetInfoEqualityTest.cs

@@ -4,9 +4,12 @@
 #nullable disable
 
 using System;
+using System.Linq;
 using NUnit.Framework;
 using osu.Game.Beatmaps;
 using osu.Game.Extensions;
+using osu.Game.Models;
+using osu.Game.Tests.Resources;
 
 namespace osu.Game.Tests.NonVisual
 {
@@ -23,6 +26,47 @@ namespace osu.Game.Tests.NonVisual
             Assert.IsTrue(ourInfo.MatchesOnlineID(otherInfo));
         }
 
+        [Test]
+        public void TestAudioEqualityNoFile()
+        {
+            var beatmapSetA = TestResources.CreateTestBeatmapSetInfo(1);
+            var beatmapSetB = TestResources.CreateTestBeatmapSetInfo(1);
+
+            Assert.AreNotEqual(beatmapSetA, beatmapSetB);
+            Assert.IsTrue(beatmapSetA.Beatmaps.Single().AudioEquals(beatmapSetB.Beatmaps.Single()));
+        }
+
+        [Test]
+        public void TestAudioEqualitySameHash()
+        {
+            var beatmapSetA = TestResources.CreateTestBeatmapSetInfo(1);
+            var beatmapSetB = TestResources.CreateTestBeatmapSetInfo(1);
+
+            addAudioFile(beatmapSetA, "abc");
+            addAudioFile(beatmapSetB, "abc");
+
+            Assert.AreNotEqual(beatmapSetA, beatmapSetB);
+            Assert.IsTrue(beatmapSetA.Beatmaps.Single().AudioEquals(beatmapSetB.Beatmaps.Single()));
+        }
+
+        [Test]
+        public void TestAudioEqualityDifferentHash()
+        {
+            var beatmapSetA = TestResources.CreateTestBeatmapSetInfo(1);
+            var beatmapSetB = TestResources.CreateTestBeatmapSetInfo(1);
+
+            addAudioFile(beatmapSetA);
+            addAudioFile(beatmapSetB);
+
+            Assert.AreNotEqual(beatmapSetA, beatmapSetB);
+            Assert.IsTrue(beatmapSetA.Beatmaps.Single().AudioEquals(beatmapSetB.Beatmaps.Single()));
+        }
+
+        private static void addAudioFile(BeatmapSetInfo beatmapSetInfo, string hash = null)
+        {
+            beatmapSetInfo.Files.Add(new RealmNamedFileUsage(new RealmFile { Hash = hash ?? Guid.NewGuid().ToString() }, "audio.mp3"));
+        }
+
         [Test]
         public void TestDatabasedWithDatabased()
         {

osu.Game.Tests/Resources/TestResources.cs

@@ -134,6 +134,7 @@ namespace osu.Game.Tests.Resources
                         DifficultyName = $"{version} {beatmapId} (length {TimeSpan.FromMilliseconds(length):m\\:ss}, bpm {bpm:0.#})",
                         StarRating = diff,
                         Length = length,
+                        BeatmapSet = beatmapSet,
                         BPM = bpm,
                         Hash = Guid.NewGuid().ToString().ComputeMD5Hash(),
                         Ruleset = rulesetInfo,

osu.Game.Tests/Visual/Editing/TestSceneEditorBeatmapCreation.cs

@@ -6,6 +6,7 @@ using System.IO;
 using System.Linq;
 using NUnit.Framework;
 using osu.Framework.Allocation;
+using osu.Framework.Audio.Track;
 using osu.Framework.Extensions.ObjectExtensions;
 using osu.Framework.Graphics;
 using osu.Framework.Screens;
@@ -103,6 +104,8 @@ namespace osu.Game.Tests.Visual.Editing
          */
         public void TestAddAudioTrack()
         {
+            AddAssert("track is virtual", () => Beatmap.Value.Track is TrackVirtual);
+
             AddAssert("switch track to real track", () =>
             {
                 var setup = Editor.ChildrenOfType<SetupScreen>().First();
@@ -131,6 +134,7 @@ namespace osu.Game.Tests.Visual.Editing
                 }
             });
 
+            AddAssert("track is not virtual", () => Beatmap.Value.Track is not TrackVirtual);
             AddAssert("track length changed", () => Beatmap.Value.Track.Length > 60000);
         }
 

osu.Game/Beatmaps/BeatmapInfo.cs

@@ -2,6 +2,7 @@
 // See the LICENCE file in the repository root for full licence text.
 
 using System;
+using System.Diagnostics;
 using System.Linq;
 using JetBrains.Annotations;
 using Newtonsoft.Json;
@@ -151,14 +152,23 @@ namespace osu.Game.Beatmaps
         public bool AudioEquals(BeatmapInfo? other) => other != null
                                                        && BeatmapSet != null
                                                        && other.BeatmapSet != null
-                                                       && BeatmapSet.Hash == other.BeatmapSet.Hash
+                                                       && compareFiles(this, other, m => m.AudioFile);
-                                                       && Metadata.AudioFile == other.Metadata.AudioFile;
 
         public bool BackgroundEquals(BeatmapInfo? other) => other != null
                                                             && BeatmapSet != null
                                                             && other.BeatmapSet != null
-                                                            && BeatmapSet.Hash == other.BeatmapSet.Hash
+                                                            && compareFiles(this, other, m => m.BackgroundFile);
-                                                            && Metadata.BackgroundFile == other.Metadata.BackgroundFile;
+
+        private static bool compareFiles(BeatmapInfo x, BeatmapInfo y, Func<IBeatmapMetadataInfo, string> getFilename)
+        {
+            Debug.Assert(x.BeatmapSet != null);
+            Debug.Assert(y.BeatmapSet != null);
+
+            string? fileHashX = x.BeatmapSet.Files.FirstOrDefault(f => f.Filename == getFilename(x.BeatmapSet.Metadata))?.File.Hash;
+            string? fileHashY = y.BeatmapSet.Files.FirstOrDefault(f => f.Filename == getFilename(y.BeatmapSet.Metadata))?.File.Hash;
+
+            return fileHashX == fileHashY;
+        }
 
         IBeatmapMetadataInfo IBeatmapInfo.Metadata => Metadata;
         IBeatmapSetInfo? IBeatmapInfo.BeatmapSet => BeatmapSet;

osu.Game/Screens/Edit/Editor.cs

@@ -186,7 +186,7 @@ namespace osu.Game.Screens.Edit
                 loadableBeatmap = beatmapManager.CreateNew(Ruleset.Value, api.LocalUser.Value);
 
                 // required so we can get the track length in EditorClock.
-                // this is safe as nothing has yet got a reference to this new beatmap.
+                // this is ONLY safe because the track being provided is a `TrackVirtual` which we don't really care about disposing.
                 loadableBeatmap.LoadTrack();
 
                 // this is a bit haphazard, but guards against setting the lease Beatmap bindable if