From 952d97c66e98de0f5e90cc285b329b2abf26463b Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Wed, 13 Jul 2022 19:02:21 +0900 Subject: [PATCH 1/4] Update comment regarding `LoadTrack` safety --- osu.Game/Screens/Edit/Editor.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/osu.Game/Screens/Edit/Editor.cs b/osu.Game/Screens/Edit/Editor.cs index 2b763415cd..48576b81e2 100644 --- a/osu.Game/Screens/Edit/Editor.cs +++ b/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 From c6b6f41b717be33f1595b9effe4a6a4f8845d038 Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Wed, 13 Jul 2022 19:40:57 +0900 Subject: [PATCH 2/4] Add test coverage of `AudioEquals` --- .../NonVisual/BeatmapSetInfoEqualityTest.cs | 44 +++++++++++++++++++ osu.Game.Tests/Resources/TestResources.cs | 1 + 2 files changed, 45 insertions(+) diff --git a/osu.Game.Tests/NonVisual/BeatmapSetInfoEqualityTest.cs b/osu.Game.Tests/NonVisual/BeatmapSetInfoEqualityTest.cs index de23b012c1..c887105da6 100644 --- a/osu.Game.Tests/NonVisual/BeatmapSetInfoEqualityTest.cs +++ b/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() { diff --git a/osu.Game.Tests/Resources/TestResources.cs b/osu.Game.Tests/Resources/TestResources.cs index 91d4eb70e8..41404b2636 100644 --- a/osu.Game.Tests/Resources/TestResources.cs +++ b/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, From 1cfdea911b7aec4ce7a354480b1818d61a5cbd33 Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Wed, 13 Jul 2022 19:13:18 +0900 Subject: [PATCH 3/4] Fix audio and background file equality incorrectly comparing `BeatmapSet.Hash` --- osu.Game/Beatmaps/BeatmapInfo.cs | 18 ++++++++++++++---- 1 file changed, 14 insertions(+), 4 deletions(-) diff --git a/osu.Game/Beatmaps/BeatmapInfo.cs b/osu.Game/Beatmaps/BeatmapInfo.cs index 346bf86818..531dc7deca 100644 --- a/osu.Game/Beatmaps/BeatmapInfo.cs +++ b/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 - && Metadata.AudioFile == other.Metadata.AudioFile; + && compareFiles(this, other, m => m.AudioFile); public bool BackgroundEquals(BeatmapInfo? other) => other != null && BeatmapSet != null && other.BeatmapSet != null - && BeatmapSet.Hash == other.BeatmapSet.Hash - && Metadata.BackgroundFile == other.Metadata.BackgroundFile; + && compareFiles(this, other, m => m.BackgroundFile); + + private static bool compareFiles(BeatmapInfo x, BeatmapInfo y, Func 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; From 2e86e7ccee2e802c01e17182f15145ec436988e6 Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Wed, 13 Jul 2022 19:31:53 +0900 Subject: [PATCH 4/4] Add extra steps to `TestExitWithoutSave` to guarantee track type --- .../Visual/Editing/TestSceneEditorBeatmapCreation.cs | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/osu.Game.Tests/Visual/Editing/TestSceneEditorBeatmapCreation.cs b/osu.Game.Tests/Visual/Editing/TestSceneEditorBeatmapCreation.cs index f565ca3ef4..6ad6f0b299 100644 --- a/osu.Game.Tests/Visual/Editing/TestSceneEditorBeatmapCreation.cs +++ b/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().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); }