From 01487a11859af1cd36719fe250d34dff919c66cd Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Mon, 27 Jun 2022 15:52:45 +0900 Subject: [PATCH 1/5] Add assertion on realm re-fetch being not-null --- osu.Game/Database/ModelManager.cs | 2 ++ 1 file changed, 2 insertions(+) diff --git a/osu.Game/Database/ModelManager.cs b/osu.Game/Database/ModelManager.cs index 9603412178..4224c92f2c 100644 --- a/osu.Game/Database/ModelManager.cs +++ b/osu.Game/Database/ModelManager.cs @@ -3,6 +3,7 @@ using System; using System.Collections.Generic; +using System.Diagnostics; using System.IO; using System.Linq; using osu.Framework.Platform; @@ -46,6 +47,7 @@ namespace osu.Game.Database Realm.Realm.Write(realm => { var managed = realm.Find(item.ID); + Debug.Assert(managed != null); operation(managed); item.Files.Clear(); From b70b365411ab9c820da30829b6dbcc5c42c3b15a Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Mon, 27 Jun 2022 15:53:05 +0900 Subject: [PATCH 2/5] Ensure files are cleaned up even on test step failure in `TestAddAudioTrack` --- .../Editing/TestSceneEditorBeatmapCreation.cs | 23 +++++++++++-------- 1 file changed, 14 insertions(+), 9 deletions(-) diff --git a/osu.Game.Tests/Visual/Editing/TestSceneEditorBeatmapCreation.cs b/osu.Game.Tests/Visual/Editing/TestSceneEditorBeatmapCreation.cs index a204fc5686..fcbf63da79 100644 --- a/osu.Game.Tests/Visual/Editing/TestSceneEditorBeatmapCreation.cs +++ b/osu.Game.Tests/Visual/Editing/TestSceneEditorBeatmapCreation.cs @@ -95,18 +95,23 @@ namespace osu.Game.Tests.Visual.Editing string extractedFolder = $"{temp}_extracted"; Directory.CreateDirectory(extractedFolder); - using (var zip = ZipArchive.Open(temp)) - zip.WriteToDirectory(extractedFolder); + try + { + using (var zip = ZipArchive.Open(temp)) + zip.WriteToDirectory(extractedFolder); - bool success = setup.ChildrenOfType().First().ChangeAudioTrack(new FileInfo(Path.Combine(extractedFolder, "03. Renatus - Soleily 192kbps.mp3"))); + bool success = setup.ChildrenOfType().First().ChangeAudioTrack(new FileInfo(Path.Combine(extractedFolder, "03. Renatus - Soleily 192kbps.mp3"))); - File.Delete(temp); - Directory.Delete(extractedFolder, true); + // ensure audio file is copied to beatmap as "audio.mp3" rather than original filename. + Assert.That(Beatmap.Value.Metadata.AudioFile == "audio.mp3"); - // ensure audio file is copied to beatmap as "audio.mp3" rather than original filename. - Assert.That(Beatmap.Value.Metadata.AudioFile == "audio.mp3"); - - return success; + return success; + } + finally + { + File.Delete(temp); + Directory.Delete(extractedFolder, true); + } }); AddAssert("track length changed", () => Beatmap.Value.Track.Length > 60000); From 10d1bbb1316f427598d2cd714ebc7b645061e030 Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Mon, 27 Jun 2022 15:58:51 +0900 Subject: [PATCH 3/5] Log global working beatmap updates in `OsuGameBase` instead of `OsuGame` --- osu.Game/OsuGame.cs | 1 - osu.Game/OsuGameBase.cs | 4 +++- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/osu.Game/OsuGame.cs b/osu.Game/OsuGame.cs index f51a18b36f..7724241d5a 100644 --- a/osu.Game/OsuGame.cs +++ b/osu.Game/OsuGame.cs @@ -612,7 +612,6 @@ 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/OsuGameBase.cs b/osu.Game/OsuGameBase.cs index 7d39aad226..57369419d2 100644 --- a/osu.Game/OsuGameBase.cs +++ b/osu.Game/OsuGameBase.cs @@ -490,10 +490,12 @@ namespace osu.Game } } - private void onBeatmapChanged(ValueChangedEvent valueChangedEvent) + private void onBeatmapChanged(ValueChangedEvent beatmap) { if (IsLoaded && !ThreadSafety.IsUpdateThread) throw new InvalidOperationException("Global beatmap bindable must be changed from update thread."); + + Logger.Log($"Game-wide working beatmap updated to {beatmap.NewValue}"); } private void onRulesetChanged(ValueChangedEvent r) From 5a7d339cc871aeacaf1f2623f289eb2b599e7253 Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Mon, 27 Jun 2022 16:19:31 +0900 Subject: [PATCH 4/5] Centralise and harden editor-ready-for-use check Not only does this combine the check into one location, but it also adds a check on the global `WorkingBeatmap` being updated, which is the only way I can see the following failure happening: ```csharp 05:19:07 osu.Game.Tests: osu.Game.Tests.Visual.Editing.TestSceneEditorBeatmapCreation.TestAddAudioTrack 05:19:07 Failed TestAddAudioTrack [161 ms] 05:19:07 Error Message: 05:19:07 TearDown : System.NullReferenceException : Object reference not set to an instance of an object. 05:19:07 Stack Trace: 05:19:07 --TearDown 05:19:07 at osu.Game.Database.ModelManager`1.<>c__DisplayClass7_0.b__0(TModel managed) in C:\BuildAgent\work\ecd860037212ac52\osu.Game\Database\ModelManager.cs:line 36 05:19:07 at osu.Game.Database.ModelManager`1.<>c__DisplayClass8_0.b__0(Realm realm) in C:\BuildAgent\work\ecd860037212ac52\osu.Game\Database\ModelManager.cs:line 49 05:19:07 at osu.Game.Database.RealmExtensions.Write(Realm realm, Action`1 function) in C:\BuildAgent\work\ecd860037212ac52\osu.Game\Database\RealmExtensions.cs:line 14 05:19:07 at osu.Game.Database.ModelManager`1.performFileOperation(TModel item, Action`1 operation) in C:\BuildAgent\work\ecd860037212ac52\osu.Game\Database\ModelManager.cs:line 46 05:19:07 at osu.Game.Database.ModelManager`1.AddFile(TModel item, Stream contents, String filename) in C:\BuildAgent\work\ecd860037212ac52\osu.Game\Database\ModelManager.cs:line 36 05:19:07 at osu.Game.Screens.Edit.Setup.ResourcesSection.ChangeAudioTrack(FileInfo source) in C:\BuildAgent\work\ecd860037212ac52\osu.Game\Screens\Edit\Setup\ResourcesSection.cs:line 115 05:19:07 at osu.Game.Tests.Visual.Editing.TestSceneEditorBeatmapCreation.b__13_0() in C:\BuildAgent\work\ecd860037212ac52\osu.Game.Tests\Visual\Editing\TestSceneEditorBeatmapCreation.cs:line 101 05:19:07 at osu.Framework.Testing.Drawables.Steps.AssertButton.checkAssert() 05:19:07 at osu.Framework.Testing.Drawables.Steps.StepButton.PerformStep(Boolean userTriggered) 05:19:07 at osu.Framework.Testing.TestScene.runNextStep(Action onCompletion, Action`1 onError, Func`2 stopCondition) ``` --- .../Editor/TestSceneSliderVelocityAdjust.cs | 9 ++---- .../Editing/TestSceneEditorBeatmapCreation.cs | 2 -- .../Editing/TestSceneEditorNavigation.cs | 7 +---- osu.Game/Screens/Edit/Editor.cs | 29 +++++++++++++++++++ osu.Game/Tests/Visual/EditorTestScene.cs | 8 ++--- 5 files changed, 34 insertions(+), 21 deletions(-) diff --git a/osu.Game.Rulesets.Osu.Tests/Editor/TestSceneSliderVelocityAdjust.cs b/osu.Game.Rulesets.Osu.Tests/Editor/TestSceneSliderVelocityAdjust.cs index 3d9fe37e0f..ef9e332253 100644 --- a/osu.Game.Rulesets.Osu.Tests/Editor/TestSceneSliderVelocityAdjust.cs +++ b/osu.Game.Rulesets.Osu.Tests/Editor/TestSceneSliderVelocityAdjust.cs @@ -10,7 +10,6 @@ using osu.Framework.Input; using osu.Framework.Testing; using osu.Framework.Utils; using osu.Game.Beatmaps; -using osu.Game.Rulesets.Edit; using osu.Game.Rulesets.Osu.Objects; using osu.Game.Rulesets.UI; using osu.Game.Screens.Edit; @@ -41,10 +40,6 @@ namespace osu.Game.Rulesets.Osu.Tests.Editor protected override IBeatmap CreateBeatmap(RulesetInfo ruleset) => new TestBeatmap(ruleset, false); - private bool editorComponentsReady => editor.ChildrenOfType().FirstOrDefault()?.IsLoaded == true - && editor.ChildrenOfType().FirstOrDefault()?.IsLoaded == true - && editor?.ChildrenOfType().FirstOrDefault()?.IsLoaded == true; - [TestCase(true)] [TestCase(false)] public void TestVelocityChangeSavesCorrectly(bool adjustVelocity) @@ -52,7 +47,7 @@ namespace osu.Game.Rulesets.Osu.Tests.Editor double? velocity = null; AddStep("enter editor", () => Game.ScreenStack.Push(new EditorLoader())); - AddUntilStep("wait for editor load", () => editorComponentsReady); + AddUntilStep("wait for editor load", () => editor?.ReadyForUse == true); AddStep("seek to first control point", () => editorClock.Seek(editorBeatmap.ControlPointInfo.TimingPoints.First().Time)); AddStep("enter slider placement mode", () => InputManager.Key(Key.Number3)); @@ -91,7 +86,7 @@ namespace osu.Game.Rulesets.Osu.Tests.Editor AddStep("exit", () => InputManager.Key(Key.Escape)); AddStep("enter editor (again)", () => Game.ScreenStack.Push(new EditorLoader())); - AddUntilStep("wait for editor load", () => editorComponentsReady); + AddUntilStep("wait for editor load", () => editor?.ReadyForUse == true); AddStep("seek to slider", () => editorClock.Seek(slider.StartTime)); AddAssert("slider has correct velocity", () => slider.Velocity == velocity); diff --git a/osu.Game.Tests/Visual/Editing/TestSceneEditorBeatmapCreation.cs b/osu.Game.Tests/Visual/Editing/TestSceneEditorBeatmapCreation.cs index fcbf63da79..b711d55e15 100644 --- a/osu.Game.Tests/Visual/Editing/TestSceneEditorBeatmapCreation.cs +++ b/osu.Game.Tests/Visual/Editing/TestSceneEditorBeatmapCreation.cs @@ -36,8 +36,6 @@ namespace osu.Game.Tests.Visual.Editing { protected override Ruleset CreateEditorRuleset() => new OsuRuleset(); - protected override bool EditorComponentsReady => Editor.ChildrenOfType().SingleOrDefault()?.IsLoaded == true; - protected override bool IsolateSavingFromDatabase => false; [Resolved] diff --git a/osu.Game.Tests/Visual/Editing/TestSceneEditorNavigation.cs b/osu.Game.Tests/Visual/Editing/TestSceneEditorNavigation.cs index 273e8abcd7..62ca97d5e0 100644 --- a/osu.Game.Tests/Visual/Editing/TestSceneEditorNavigation.cs +++ b/osu.Game.Tests/Visual/Editing/TestSceneEditorNavigation.cs @@ -9,12 +9,10 @@ using osu.Framework.Extensions.ObjectExtensions; using osu.Framework.Testing; using osu.Game.Beatmaps; using osu.Game.Database; -using osu.Game.Rulesets.Edit; using osu.Game.Rulesets.Mania; using osu.Game.Rulesets.Osu; using osu.Game.Screens.Edit; using osu.Game.Screens.Edit.Components.Timelines.Summary; -using osu.Game.Screens.Edit.Compose.Components.Timeline; using osu.Game.Screens.Edit.GameplayTest; using osu.Game.Screens.Select; using osu.Game.Tests.Resources; @@ -40,10 +38,7 @@ namespace osu.Game.Tests.Visual.Editing AddStep("switch ruleset", () => Game.Ruleset.Value = new ManiaRuleset().RulesetInfo); AddStep("open editor", () => ((PlaySongSelect)Game.ScreenStack.CurrentScreen).Edit(beatmapSet.Beatmaps.First(beatmap => beatmap.Ruleset.OnlineID == 0))); - AddUntilStep("wait for editor open", () => Game.ScreenStack.CurrentScreen is Editor editor - && editor.IsLoaded - && editor.ChildrenOfType().FirstOrDefault()?.IsLoaded == true - && editor.ChildrenOfType().FirstOrDefault()?.IsLoaded == true); + AddUntilStep("wait for editor open", () => Game.ScreenStack.CurrentScreen is Editor editor && editor.ReadyForUse); AddStep("test gameplay", () => { var testGameplayButton = this.ChildrenOfType().Single(); diff --git a/osu.Game/Screens/Edit/Editor.cs b/osu.Game/Screens/Edit/Editor.cs index 20bdf75b7d..9dbb5f4cfe 100644 --- a/osu.Game/Screens/Edit/Editor.cs +++ b/osu.Game/Screens/Edit/Editor.cs @@ -21,6 +21,7 @@ using osu.Framework.Input.Events; using osu.Framework.Logging; using osu.Framework.Platform; using osu.Framework.Screens; +using osu.Framework.Testing; using osu.Framework.Timing; using osu.Game.Audio; using osu.Game.Beatmaps; @@ -39,6 +40,7 @@ using osu.Game.Rulesets.Edit; using osu.Game.Rulesets.Objects; using osu.Game.Screens.Edit.Components.Menus; using osu.Game.Screens.Edit.Compose; +using osu.Game.Screens.Edit.Compose.Components.Timeline; using osu.Game.Screens.Edit.Design; using osu.Game.Screens.Edit.GameplayTest; using osu.Game.Screens.Edit.Setup; @@ -97,6 +99,32 @@ namespace osu.Game.Screens.Edit public IBindable SamplePlaybackDisabled => samplePlaybackDisabled; + /// + /// Ensure all asynchronously loading pieces of the editor are in a good state. + /// This exists here for convenience for tests, not for actual use. + /// Eventually we'd probably want a better way to signal this. + /// + public bool ReadyForUse + { + get + { + if (!workingBeatmapUpdated) + return false; + + var loadedScreen = screenContainer?.Children.SingleOrDefault(s => s.IsLoaded); + + if (loadedScreen == null) + return false; + + if (loadedScreen is EditorScreenWithTimeline) + return loadedScreen.ChildrenOfType().FirstOrDefault()?.IsLoaded == true; + + return true; + } + } + + private bool workingBeatmapUpdated; + private readonly Bindable samplePlaybackDisabled = new Bindable(); private bool canSave; @@ -219,6 +247,7 @@ namespace osu.Game.Screens.Edit // this assumes that nothing during the rest of this load() method is accessing Beatmap.Value (loadableBeatmap should be preferred). // generally this is quite safe, as the actual load of editor content comes after menuBar.Mode.ValueChanged is fired in its own LoadComplete. Beatmap.Value = loadableBeatmap; + workingBeatmapUpdated = true; }); OsuMenuItem undoMenuItem; diff --git a/osu.Game/Tests/Visual/EditorTestScene.cs b/osu.Game/Tests/Visual/EditorTestScene.cs index 3f5fee5768..31036247ab 100644 --- a/osu.Game/Tests/Visual/EditorTestScene.cs +++ b/osu.Game/Tests/Visual/EditorTestScene.cs @@ -17,9 +17,7 @@ using osu.Game.Online.API; using osu.Game.Overlays; using osu.Game.Overlays.Dialog; using osu.Game.Rulesets; -using osu.Game.Rulesets.Edit; using osu.Game.Screens.Edit; -using osu.Game.Screens.Edit.Compose.Components.Timeline; using osu.Game.Screens.Menu; using osu.Game.Skinning; @@ -58,15 +56,13 @@ namespace osu.Game.Tests.Visual Dependencies.CacheAs(testBeatmapManager = new TestBeatmapManager(LocalStorage, Realm, rulesets, null, audio, Resources, host, Beatmap.Default)); } - protected virtual bool EditorComponentsReady => Editor.ChildrenOfType().FirstOrDefault()?.IsLoaded == true - && Editor.ChildrenOfType().FirstOrDefault()?.IsLoaded == true; - public override void SetUpSteps() { base.SetUpSteps(); AddStep("load editor", LoadEditor); - AddUntilStep("wait for editor to load", () => EditorComponentsReady); + AddUntilStep("wait for editor to load", () => Editor?.ReadyForUse == true); + AddUntilStep("wait for beatmap updated", () => !Beatmap.IsDefault); } protected virtual void LoadEditor() From 0ba29b6fa64eac5a908903fbbfbe0270662879a1 Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Mon, 27 Jun 2022 18:28:00 +0900 Subject: [PATCH 5/5] Use `currentScreen` instead to make sure the screen we care about is loaded --- osu.Game/Screens/Edit/Editor.cs | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/osu.Game/Screens/Edit/Editor.cs b/osu.Game/Screens/Edit/Editor.cs index 9dbb5f4cfe..81ae08aea1 100644 --- a/osu.Game/Screens/Edit/Editor.cs +++ b/osu.Game/Screens/Edit/Editor.cs @@ -111,13 +111,11 @@ namespace osu.Game.Screens.Edit if (!workingBeatmapUpdated) return false; - var loadedScreen = screenContainer?.Children.SingleOrDefault(s => s.IsLoaded); - - if (loadedScreen == null) + if (currentScreen?.IsLoaded != true) return false; - if (loadedScreen is EditorScreenWithTimeline) - return loadedScreen.ChildrenOfType().FirstOrDefault()?.IsLoaded == true; + if (currentScreen is EditorScreenWithTimeline) + return currentScreen.ChildrenOfType().FirstOrDefault()?.IsLoaded == true; return true; }