From 02689a1b60269c6fe3952cfb35231d2e7ffd4f03 Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Wed, 13 Oct 2021 14:34:31 +0900 Subject: [PATCH 1/8] Use actual `BeatmapInfo` rather than `PlayableBeatmap.BeatmapInfo` for editor writes --- .../Visual/Editing/TestSceneEditorSaving.cs | 2 ++ osu.Game/Beatmaps/BeatmapModelManager.cs | 8 +++++++- osu.Game/Screens/Edit/Editor.cs | 6 +++--- osu.Game/Screens/Edit/EditorBeatmap.cs | 12 ++++++++---- 4 files changed, 20 insertions(+), 8 deletions(-) diff --git a/osu.Game.Tests/Visual/Editing/TestSceneEditorSaving.cs b/osu.Game.Tests/Visual/Editing/TestSceneEditorSaving.cs index f0aa3e2350..49f7bdf997 100644 --- a/osu.Game.Tests/Visual/Editing/TestSceneEditorSaving.cs +++ b/osu.Game.Tests/Visual/Editing/TestSceneEditorSaving.cs @@ -33,6 +33,7 @@ namespace osu.Game.Tests.Visual.Editing AddUntilStep("wait for editor load", () => editor != null); AddStep("Set overall difficulty", () => editorBeatmap.Difficulty.OverallDifficulty = 7); + AddStep("Set difficulty name", () => editorBeatmap.BeatmapInfo.Version = "diffname"); AddStep("Add timing point", () => editorBeatmap.ControlPointInfo.Add(0, new TimingControlPoint())); @@ -60,6 +61,7 @@ namespace osu.Game.Tests.Visual.Editing AddUntilStep("Wait for editor load", () => editor != null); AddAssert("Beatmap contains single hitcircle", () => editorBeatmap.HitObjects.Count == 1); AddAssert("Beatmap has correct overall difficulty", () => editorBeatmap.Difficulty.OverallDifficulty == 7); + AddAssert("Beatmap has correct difficulty name", () => editorBeatmap.BeatmapInfo.Version == "diffname"); } } } diff --git a/osu.Game/Beatmaps/BeatmapModelManager.cs b/osu.Game/Beatmaps/BeatmapModelManager.cs index 9c0fc5ef8a..76019a15ae 100644 --- a/osu.Game/Beatmaps/BeatmapModelManager.cs +++ b/osu.Game/Beatmaps/BeatmapModelManager.cs @@ -192,6 +192,13 @@ namespace osu.Game.Beatmaps { var setInfo = beatmapInfo.BeatmapSet; + // Difficulty settings must be copied first due to the clone in `Beatmap<>.BeatmapInfo_Set`. + // This should hopefully be temporary, assuming said clone is eventually removed. + beatmapInfo.BaseDifficulty.CopyFrom(beatmapContent.Difficulty); + + // All changes to metadata are made in the provided beatmapInfo, so this should be copied to the `IBeatmap` before encoding. + beatmapContent.BeatmapInfo = beatmapInfo; + using (var stream = new MemoryStream()) { using (var sw = new StreamWriter(stream, Encoding.UTF8, 1024, true)) @@ -202,7 +209,6 @@ namespace osu.Game.Beatmaps using (ContextFactory.GetForWrite()) { beatmapInfo = setInfo.Beatmaps.Single(b => b.ID == beatmapInfo.ID); - beatmapInfo.BaseDifficulty.CopyFrom(beatmapContent.Difficulty); var metadata = beatmapInfo.Metadata ?? setInfo.Metadata; diff --git a/osu.Game/Screens/Edit/Editor.cs b/osu.Game/Screens/Edit/Editor.cs index 1170658abb..88bed109fe 100644 --- a/osu.Game/Screens/Edit/Editor.cs +++ b/osu.Game/Screens/Edit/Editor.cs @@ -162,7 +162,7 @@ namespace osu.Game.Screens.Edit // todo: remove caching of this and consume via editorBeatmap? dependencies.Cache(beatDivisor); - AddInternal(editorBeatmap = new EditorBeatmap(playableBeatmap, loadableBeatmap.GetSkin())); + AddInternal(editorBeatmap = new EditorBeatmap(playableBeatmap, loadableBeatmap.GetSkin(), loadableBeatmap.BeatmapInfo)); dependencies.CacheAs(editorBeatmap); changeHandler = new EditorChangeHandler(editorBeatmap); dependencies.CacheAs(changeHandler); @@ -333,10 +333,10 @@ namespace osu.Game.Screens.Edit isNewBeatmap = false; // apply any set-level metadata changes. - beatmapManager.Update(playableBeatmap.BeatmapInfo.BeatmapSet); + beatmapManager.Update(editorBeatmap.BeatmapInfo.BeatmapSet); // save the loaded beatmap's data stream. - beatmapManager.Save(playableBeatmap.BeatmapInfo, editorBeatmap, editorBeatmap.BeatmapSkin); + beatmapManager.Save(editorBeatmap.BeatmapInfo, editorBeatmap.PlayableBeatmap, editorBeatmap.BeatmapSkin); updateLastSavedHash(); } diff --git a/osu.Game/Screens/Edit/EditorBeatmap.cs b/osu.Game/Screens/Edit/EditorBeatmap.cs index 64eb6225fa..c96c17326d 100644 --- a/osu.Game/Screens/Edit/EditorBeatmap.cs +++ b/osu.Game/Screens/Edit/EditorBeatmap.cs @@ -44,6 +44,7 @@ namespace osu.Game.Screens.Edit /// public readonly Bindable PlacementObject = new Bindable(); + private readonly BeatmapInfo beatmapInfo; public readonly IBeatmap PlayableBeatmap; /// @@ -66,9 +67,12 @@ namespace osu.Game.Screens.Edit private readonly Dictionary> startTimeBindables = new Dictionary>(); - public EditorBeatmap(IBeatmap playableBeatmap, ISkin beatmapSkin = null) + public EditorBeatmap(IBeatmap playableBeatmap, ISkin beatmapSkin = null, BeatmapInfo beatmapInfo = null) { PlayableBeatmap = playableBeatmap; + + this.beatmapInfo = beatmapInfo ?? playableBeatmap.BeatmapInfo; + if (beatmapSkin is Skin skin) BeatmapSkin = new EditorBeatmapSkin(skin); @@ -80,11 +84,11 @@ namespace osu.Game.Screens.Edit public BeatmapInfo BeatmapInfo { - get => PlayableBeatmap.BeatmapInfo; - set => PlayableBeatmap.BeatmapInfo = value; + get => beatmapInfo; + set => throw new InvalidOperationException(); } - public BeatmapMetadata Metadata => PlayableBeatmap.Metadata; + public BeatmapMetadata Metadata => beatmapInfo.Metadata; public BeatmapDifficulty Difficulty { From 09536cd7339b9f2ebb2bf55b2ac12d3e6e9ba088 Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Thu, 14 Oct 2021 13:58:36 +0900 Subject: [PATCH 2/8] Add logging of `WorkingBeatmapCache.Invalidate` calls --- osu.Game/Beatmaps/WorkingBeatmapCache.cs | 5 +++++ osu.Game/Screens/Edit/Editor.cs | 3 +++ 2 files changed, 8 insertions(+) diff --git a/osu.Game/Beatmaps/WorkingBeatmapCache.cs b/osu.Game/Beatmaps/WorkingBeatmapCache.cs index ad3e890b3a..cf83345e2a 100644 --- a/osu.Game/Beatmaps/WorkingBeatmapCache.cs +++ b/osu.Game/Beatmaps/WorkingBeatmapCache.cs @@ -66,8 +66,12 @@ namespace osu.Game.Beatmaps lock (workingCache) { var working = workingCache.FirstOrDefault(w => w.BeatmapInfo?.ID == info.ID); + if (working != null) + { + Logger.Log($"Invalidating working beatmap cache for {info}"); workingCache.Remove(working); + } } } @@ -86,6 +90,7 @@ namespace osu.Game.Beatmaps lock (workingCache) { var working = workingCache.FirstOrDefault(w => w.BeatmapInfo?.ID == beatmapInfo.ID); + if (working != null) return working; diff --git a/osu.Game/Screens/Edit/Editor.cs b/osu.Game/Screens/Edit/Editor.cs index 88bed109fe..512226413b 100644 --- a/osu.Game/Screens/Edit/Editor.cs +++ b/osu.Game/Screens/Edit/Editor.cs @@ -523,7 +523,10 @@ namespace osu.Game.Screens.Edit var refetchedBeatmap = beatmapManager.GetWorkingBeatmap(Beatmap.Value.BeatmapInfo); if (!(refetchedBeatmap is DummyWorkingBeatmap)) + { + Logger.Log("Editor providing re-fetched beatmap post edit session"); Beatmap.Value = refetchedBeatmap; + } return base.OnExiting(next); } From e0babe4b792d2e041b8c224ca854ffd4dcc28242 Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Thu, 14 Oct 2021 14:07:43 +0900 Subject: [PATCH 3/8] Add global logging of `WorkingBeatmap` changes --- osu.Game/OsuGame.cs | 1 + osu.Game/Screens/Select/SongSelect.cs | 2 +- 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/osu.Game/OsuGame.cs b/osu.Game/OsuGame.cs index 020cdebab6..5960451c9c 100644 --- a/osu.Game/OsuGame.cs +++ b/osu.Game/OsuGame.cs @@ -554,6 +554,7 @@ namespace osu.Game { beatmap.OldValue?.CancelAsyncLoad(); beatmap.NewValue?.BeginAsyncLoad(); + Logger.Log($"Game-wide working beatmap updated to {beatmap.NewValue}"); } private void modsChanged(ValueChangedEvent> mods) diff --git a/osu.Game/Screens/Select/SongSelect.cs b/osu.Game/Screens/Select/SongSelect.cs index 6cafcb9d16..a2dea355ac 100644 --- a/osu.Game/Screens/Select/SongSelect.cs +++ b/osu.Game/Screens/Select/SongSelect.cs @@ -410,7 +410,7 @@ namespace osu.Game.Screens.Select { if (e.NewValue is DummyWorkingBeatmap || !this.IsCurrentScreen()) return; - Logger.Log($"working beatmap updated to {e.NewValue}"); + Logger.Log($"Song select working beatmap updated to {e.NewValue}"); if (!Carousel.SelectBeatmap(e.NewValue.BeatmapInfo, false)) { From 2e0a2a28ab1809efdb6da5182dacd5c4e7c20120 Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Thu, 14 Oct 2021 14:40:20 +0900 Subject: [PATCH 4/8] Check mutations at each point in the process (and also check artist/title) --- .../Visual/Editing/TestSceneEditorSaving.cs | 22 +++++++++++++++---- 1 file changed, 18 insertions(+), 4 deletions(-) diff --git a/osu.Game.Tests/Visual/Editing/TestSceneEditorSaving.cs b/osu.Game.Tests/Visual/Editing/TestSceneEditorSaving.cs index 49f7bdf997..1404aa1103 100644 --- a/osu.Game.Tests/Visual/Editing/TestSceneEditorSaving.cs +++ b/osu.Game.Tests/Visual/Editing/TestSceneEditorSaving.cs @@ -30,10 +30,15 @@ namespace osu.Game.Tests.Visual.Editing PushAndConfirm(() => new EditorLoader()); - AddUntilStep("wait for editor load", () => editor != null); + AddUntilStep("wait for editor load", () => editor?.IsLoaded == true); AddStep("Set overall difficulty", () => editorBeatmap.Difficulty.OverallDifficulty = 7); - AddStep("Set difficulty name", () => editorBeatmap.BeatmapInfo.Version = "diffname"); + AddStep("Set artist and title", () => + { + editorBeatmap.BeatmapInfo.Metadata.Artist = "artist"; + editorBeatmap.BeatmapInfo.Metadata.Title = "title"; + }); + AddStep("Set difficulty name", () => editorBeatmap.BeatmapInfo.Version = "difficulty"); AddStep("Add timing point", () => editorBeatmap.ControlPointInfo.Add(0, new TimingControlPoint())); @@ -44,10 +49,12 @@ namespace osu.Game.Tests.Visual.Editing AddStep("Move to playfield", () => InputManager.MoveMouseTo(Game.ScreenSpaceDrawQuad.Centre)); AddStep("Place single hitcircle", () => InputManager.Click(MouseButton.Left)); - AddAssert("Beatmap contains single hitcircle", () => editorBeatmap.HitObjects.Count == 1); + checkMutations(); AddStep("Save", () => InputManager.Keys(PlatformAction.Save)); + checkMutations(); + AddStep("Exit", () => InputManager.Key(Key.Escape)); AddUntilStep("Wait for main menu", () => Game.ScreenStack.CurrentScreen is MainMenu); @@ -59,9 +66,16 @@ namespace osu.Game.Tests.Visual.Editing AddStep("Enter editor", () => InputManager.Key(Key.Number5)); AddUntilStep("Wait for editor load", () => editor != null); + + checkMutations(); + } + + private void checkMutations() + { AddAssert("Beatmap contains single hitcircle", () => editorBeatmap.HitObjects.Count == 1); AddAssert("Beatmap has correct overall difficulty", () => editorBeatmap.Difficulty.OverallDifficulty == 7); - AddAssert("Beatmap has correct difficulty name", () => editorBeatmap.BeatmapInfo.Version == "diffname"); + AddAssert("Beatmap has correct metadata", () => editorBeatmap.BeatmapInfo.Metadata.Artist == "artist" && editorBeatmap.BeatmapInfo.Metadata.Title == "title"); + AddAssert("Beatmap has correct difficulty name", () => editorBeatmap.BeatmapInfo.Version == "difficulty"); } } } From 060bb1afbd129db70c5f2132bb781c725fd69105 Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Thu, 14 Oct 2021 15:39:08 +0900 Subject: [PATCH 5/8] Add locking around async beatmap (task) retrieval --- osu.Game/Beatmaps/WorkingBeatmap.cs | 26 +++++++++++++++++--------- 1 file changed, 17 insertions(+), 9 deletions(-) diff --git a/osu.Game/Beatmaps/WorkingBeatmap.cs b/osu.Game/Beatmaps/WorkingBeatmap.cs index 18adecb7aa..177678789d 100644 --- a/osu.Game/Beatmaps/WorkingBeatmap.cs +++ b/osu.Game/Beatmaps/WorkingBeatmap.cs @@ -205,19 +205,27 @@ namespace osu.Game.Beatmaps return new CancellationTokenSource(timeout ?? TimeSpan.FromSeconds(10)); } - private Task loadBeatmapAsync() => beatmapLoadTask ??= Task.Factory.StartNew(() => + private readonly object beatmapFetchLock = new object(); + + private Task loadBeatmapAsync() { - // Todo: Handle cancellation during beatmap parsing - var b = GetBeatmap() ?? new Beatmap(); + 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; + // 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; + // 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); + return b; + }, loadCancellation.Token, TaskCreationOptions.LongRunning, TaskScheduler.Default); + } + } public override string ToString() => BeatmapInfo.ToString(); From 8a4c0c0ac82094868e396e997ed6d45fce2b9e1d Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Thu, 14 Oct 2021 16:22:43 +0900 Subject: [PATCH 6/8] Lock one more case of usage --- osu.Game/Beatmaps/WorkingBeatmap.cs | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/osu.Game/Beatmaps/WorkingBeatmap.cs b/osu.Game/Beatmaps/WorkingBeatmap.cs index 177678789d..d2c0f7de0f 100644 --- a/osu.Game/Beatmaps/WorkingBeatmap.cs +++ b/osu.Game/Beatmaps/WorkingBeatmap.cs @@ -189,11 +189,14 @@ namespace osu.Game.Beatmaps /// public void CancelAsyncLoad() { - loadCancellation?.Cancel(); - loadCancellation = new CancellationTokenSource(); + lock (beatmapFetchLock) + { + loadCancellation?.Cancel(); + loadCancellation = new CancellationTokenSource(); - if (beatmapLoadTask?.IsCompleted != true) - beatmapLoadTask = null; + if (beatmapLoadTask?.IsCompleted != true) + beatmapLoadTask = null; + } } private CancellationTokenSource createCancellationTokenSource(TimeSpan? timeout) From ad0732484ff19e8854af7663d4631d950c15eae2 Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Thu, 14 Oct 2021 22:12:38 +0900 Subject: [PATCH 7/8] Just wait for metadata section to be loaded --- osu.Game.Tests/Visual/Editing/TestSceneEditorSaving.cs | 3 +++ osu.Game/Screens/Edit/Setup/MetadataSection.cs | 2 +- 2 files changed, 4 insertions(+), 1 deletion(-) diff --git a/osu.Game.Tests/Visual/Editing/TestSceneEditorSaving.cs b/osu.Game.Tests/Visual/Editing/TestSceneEditorSaving.cs index 1404aa1103..49136e6019 100644 --- a/osu.Game.Tests/Visual/Editing/TestSceneEditorSaving.cs +++ b/osu.Game.Tests/Visual/Editing/TestSceneEditorSaving.cs @@ -8,6 +8,7 @@ using osu.Framework.Testing; using osu.Game.Beatmaps.ControlPoints; using osu.Game.Rulesets.Edit; using osu.Game.Screens.Edit; +using osu.Game.Screens.Edit.Setup; using osu.Game.Screens.Menu; using osu.Game.Screens.Select; using osuTK.Input; @@ -32,6 +33,8 @@ namespace osu.Game.Tests.Visual.Editing AddUntilStep("wait for editor load", () => editor?.IsLoaded == true); + AddUntilStep("wait for metadata screen load", () => editor.ChildrenOfType().FirstOrDefault()?.IsLoaded == true); + AddStep("Set overall difficulty", () => editorBeatmap.Difficulty.OverallDifficulty = 7); AddStep("Set artist and title", () => { diff --git a/osu.Game/Screens/Edit/Setup/MetadataSection.cs b/osu.Game/Screens/Edit/Setup/MetadataSection.cs index 9e93b0b038..5bb40c09a5 100644 --- a/osu.Game/Screens/Edit/Setup/MetadataSection.cs +++ b/osu.Game/Screens/Edit/Setup/MetadataSection.cs @@ -10,7 +10,7 @@ using osu.Game.Graphics.UserInterfaceV2; namespace osu.Game.Screens.Edit.Setup { - internal class MetadataSection : SetupSection + public class MetadataSection : SetupSection { protected LabelledTextBox ArtistTextBox; protected LabelledTextBox RomanisedArtistTextBox; From 02d29097a23300b19d6b8174285a00fd845176a1 Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Fri, 15 Oct 2021 11:14:03 +0900 Subject: [PATCH 8/8] Switch away from metadata screen before making any changes in test logic --- osu.Game.Tests/Visual/Editing/TestSceneEditorSaving.cs | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/osu.Game.Tests/Visual/Editing/TestSceneEditorSaving.cs b/osu.Game.Tests/Visual/Editing/TestSceneEditorSaving.cs index 49136e6019..ab2bc4649a 100644 --- a/osu.Game.Tests/Visual/Editing/TestSceneEditorSaving.cs +++ b/osu.Game.Tests/Visual/Editing/TestSceneEditorSaving.cs @@ -35,6 +35,11 @@ namespace osu.Game.Tests.Visual.Editing AddUntilStep("wait for metadata screen load", () => editor.ChildrenOfType().FirstOrDefault()?.IsLoaded == true); + // We intentionally switch away from the metadata screen, else there is a feedback loop with the textbox handling which causes metadata changes below to get overwritten. + + AddStep("Enter compose mode", () => InputManager.Key(Key.F1)); + AddUntilStep("Wait for compose mode load", () => editor.ChildrenOfType().FirstOrDefault()?.IsLoaded == true); + AddStep("Set overall difficulty", () => editorBeatmap.Difficulty.OverallDifficulty = 7); AddStep("Set artist and title", () => { @@ -45,9 +50,6 @@ namespace osu.Game.Tests.Visual.Editing AddStep("Add timing point", () => editorBeatmap.ControlPointInfo.Add(0, new TimingControlPoint())); - AddStep("Enter compose mode", () => InputManager.Key(Key.F1)); - AddUntilStep("Wait for compose mode load", () => editor.ChildrenOfType().FirstOrDefault()?.IsLoaded == true); - AddStep("Change to placement mode", () => InputManager.Key(Key.Number2)); AddStep("Move to playfield", () => InputManager.MoveMouseTo(Game.ScreenSpaceDrawQuad.Centre)); AddStep("Place single hitcircle", () => InputManager.Click(MouseButton.Left));