From b9ec860cf2c7a5a3eae50cc06c6301e7ed5e5802 Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Tue, 18 Jan 2022 12:20:52 +0900 Subject: [PATCH 1/6] Ensure global beatmap/ruleset are always mutated from the update thread This came up while testing the new realm thread, where `MusicController` would fall over when `OsuTestScene` changes the global beatmap from an async load thread (causing a cross-thread realm access). We don't want to have to schedule every usage of these bindables, so this seems like a good constraint to put in place. --- osu.Game/OsuGameBase.cs | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/osu.Game/OsuGameBase.cs b/osu.Game/OsuGameBase.cs index 9256514a0a..8126b74418 100644 --- a/osu.Game/OsuGameBase.cs +++ b/osu.Game/OsuGameBase.cs @@ -327,6 +327,7 @@ namespace osu.Game dependencies.CacheAs(MusicController); Ruleset.BindValueChanged(onRulesetChanged); + Beatmap.BindValueChanged(onBeatmapChanged); } protected virtual void InitialiseFonts() @@ -448,8 +449,17 @@ namespace osu.Game protected override Storage CreateStorage(GameHost host, Storage defaultStorage) => new OsuStorage(host, defaultStorage); + private void onBeatmapChanged(ValueChangedEvent valueChangedEvent) + { + if (!ThreadSafety.IsUpdateThread) + throw new InvalidOperationException("Global beatmap bindable must be changed from update thread."); + } + private void onRulesetChanged(ValueChangedEvent r) { + if (!ThreadSafety.IsUpdateThread) + throw new InvalidOperationException("Global ruleset bindable must be changed from update thread."); + if (r.NewValue?.Available != true) { // reject the change if the ruleset is not available. From 014c840d807e00ff2b35bcce9d096c0396d309a9 Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Tue, 18 Jan 2022 12:24:42 +0900 Subject: [PATCH 2/6] Fix incorrect thread usage of ruleset in tournament `DataLoadTest` --- osu.Game.Tournament.Tests/NonVisual/DataLoadTest.cs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/osu.Game.Tournament.Tests/NonVisual/DataLoadTest.cs b/osu.Game.Tournament.Tests/NonVisual/DataLoadTest.cs index db019f9242..65753bfe00 100644 --- a/osu.Game.Tournament.Tests/NonVisual/DataLoadTest.cs +++ b/osu.Game.Tournament.Tests/NonVisual/DataLoadTest.cs @@ -35,9 +35,9 @@ namespace osu.Game.Tournament.Tests.NonVisual public class TestTournament : TournamentGameBase { - [BackgroundDependencyLoader] - private void load() + protected override void LoadComplete() { + base.LoadComplete(); Ruleset.Value = new RulesetInfo(); // not available } } From b9aae5569f75ce9fd62d0cb187806f0917b009db Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Tue, 18 Jan 2022 13:22:37 +0900 Subject: [PATCH 3/6] Fix `OsuTestScene` potentially mutating global bindables --- osu.Game/Tests/Visual/OsuTestScene.cs | 45 +++++++++++++++++---------- 1 file changed, 28 insertions(+), 17 deletions(-) diff --git a/osu.Game/Tests/Visual/OsuTestScene.cs b/osu.Game/Tests/Visual/OsuTestScene.cs index 6b029729ea..be811dd54b 100644 --- a/osu.Game/Tests/Visual/OsuTestScene.cs +++ b/osu.Game/Tests/Visual/OsuTestScene.cs @@ -28,7 +28,6 @@ using osu.Game.Rulesets; using osu.Game.Rulesets.Mods; using osu.Game.Rulesets.Objects; using osu.Game.Rulesets.UI; -using osu.Game.Screens; using osu.Game.Storyboards; using osu.Game.Tests.Beatmaps; using osu.Game.Tests.Rulesets; @@ -38,13 +37,16 @@ namespace osu.Game.Tests.Visual [ExcludeFromDynamicCompile] public abstract class OsuTestScene : TestScene { - protected Bindable Beatmap { get; private set; } + [Cached] + protected Bindable Beatmap { get; } = new Bindable(); - protected Bindable Ruleset; + [Cached] + protected Bindable Ruleset { get; } = new Bindable(); - protected Bindable> SelectedMods; + [Cached] + protected Bindable> SelectedMods { get; } = new Bindable>(Array.Empty()); - protected new OsuScreenDependencies Dependencies { get; private set; } + protected new DependencyContainer Dependencies { get; private set; } protected IResourceStore Resources; @@ -139,17 +141,15 @@ namespace osu.Game.Tests.Visual var providedRuleset = CreateRuleset(); if (providedRuleset != null) - baseDependencies = rulesetDependencies = new DrawableRulesetDependencies(providedRuleset, baseDependencies); + isolatedBaseDependencies = rulesetDependencies = new DrawableRulesetDependencies(providedRuleset, baseDependencies); - Dependencies = new OsuScreenDependencies(false, baseDependencies); + Dependencies = isolatedBaseDependencies; - Beatmap = Dependencies.Beatmap; + Beatmap.Default = parent.Get>().Default; Beatmap.SetDefault(); - Ruleset = Dependencies.Ruleset; - Ruleset.SetDefault(); + Ruleset.Value = CreateRuleset()?.RulesetInfo ?? parent.Get().AvailableRulesets.First(); - SelectedMods = Dependencies.Mods; SelectedMods.SetDefault(); if (!UseOnlineAPI) @@ -162,6 +162,23 @@ namespace osu.Game.Tests.Visual return Dependencies; } + protected override void LoadComplete() + { + base.LoadComplete(); + + var parentBeatmap = Parent.Dependencies.Get>(); + parentBeatmap.Value = Beatmap.Value; + Beatmap.BindTo(parentBeatmap); + + var parentRuleset = Parent.Dependencies.Get>(); + parentRuleset.Value = Ruleset.Value; + Ruleset.BindTo(parentRuleset); + + var parentMods = Parent.Dependencies.Get>>(); + parentMods.Value = SelectedMods.Value; + SelectedMods.BindTo(parentMods); + } + protected override Container Content => content ?? base.Content; private readonly Container content; @@ -286,12 +303,6 @@ namespace osu.Game.Tests.Visual protected virtual WorkingBeatmap CreateWorkingBeatmap(IBeatmap beatmap, Storyboard storyboard = null) => new ClockBackedTestWorkingBeatmap(beatmap, storyboard, Clock, Audio); - [BackgroundDependencyLoader] - private void load(RulesetStore rulesets) - { - Ruleset.Value = CreateRuleset()?.RulesetInfo ?? rulesets.AvailableRulesets.First(); - } - protected override void Dispose(bool isDisposing) { base.Dispose(isDisposing); From da9a60a6954c5b194e9e1331fae92792fa9090c8 Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Tue, 18 Jan 2022 13:22:55 +0900 Subject: [PATCH 4/6] Update broken test scenes to match new `OsuTestScene` logic --- .../Visual/Editing/TestSceneComposeScreen.cs | 5 ++-- .../Visual/Editing/TestSceneEditorClock.cs | 6 ++--- .../Editing/TestSceneEditorSeekSnapping.cs | 6 ++--- .../Visual/Editing/TestSceneTimingScreen.cs | 5 ++-- osu.Game/Tests/Visual/EditorClockTestScene.cs | 4 +-- osu.Game/Tests/Visual/EditorTestScene.cs | 25 +++++++++++++------ 6 files changed, 31 insertions(+), 20 deletions(-) diff --git a/osu.Game.Tests/Visual/Editing/TestSceneComposeScreen.cs b/osu.Game.Tests/Visual/Editing/TestSceneComposeScreen.cs index 9b8567e853..d100fba8d6 100644 --- a/osu.Game.Tests/Visual/Editing/TestSceneComposeScreen.cs +++ b/osu.Game.Tests/Visual/Editing/TestSceneComposeScreen.cs @@ -29,9 +29,10 @@ namespace osu.Game.Tests.Visual.Editing [Cached] private EditorClipboard clipboard = new EditorClipboard(); - [BackgroundDependencyLoader] - private void load() + protected override void LoadComplete() { + base.LoadComplete(); + Beatmap.Value = CreateWorkingBeatmap(editorBeatmap.PlayableBeatmap); Child = new ComposeScreen diff --git a/osu.Game.Tests/Visual/Editing/TestSceneEditorClock.cs b/osu.Game.Tests/Visual/Editing/TestSceneEditorClock.cs index 0abf0c47f8..4b9be77471 100644 --- a/osu.Game.Tests/Visual/Editing/TestSceneEditorClock.cs +++ b/osu.Game.Tests/Visual/Editing/TestSceneEditorClock.cs @@ -2,7 +2,6 @@ // See the LICENCE file in the repository root for full licence text. using NUnit.Framework; -using osu.Framework.Allocation; using osu.Framework.Graphics; using osu.Framework.Graphics.Containers; using osu.Game.Rulesets.Osu; @@ -37,9 +36,10 @@ namespace osu.Game.Tests.Visual.Editing }); } - [BackgroundDependencyLoader] - private void load() + protected override void LoadComplete() { + base.LoadComplete(); + Beatmap.Value = CreateWorkingBeatmap(new OsuRuleset().RulesetInfo); // ensure that music controller does not change this beatmap due to it // completing naturally as part of the test. diff --git a/osu.Game.Tests/Visual/Editing/TestSceneEditorSeekSnapping.cs b/osu.Game.Tests/Visual/Editing/TestSceneEditorSeekSnapping.cs index 3a19eabe81..863f42520b 100644 --- a/osu.Game.Tests/Visual/Editing/TestSceneEditorSeekSnapping.cs +++ b/osu.Game.Tests/Visual/Editing/TestSceneEditorSeekSnapping.cs @@ -2,7 +2,6 @@ // See the LICENCE file in the repository root for full licence text. using NUnit.Framework; -using osu.Framework.Allocation; using osu.Framework.Extensions.Color4Extensions; using osu.Framework.Graphics; using osu.Framework.Graphics.Containers; @@ -23,9 +22,10 @@ namespace osu.Game.Tests.Visual.Editing BeatDivisor.Value = 4; } - [BackgroundDependencyLoader] - private void load() + protected override void LoadComplete() { + base.LoadComplete(); + var testBeatmap = new Beatmap { ControlPointInfo = new ControlPointInfo(), diff --git a/osu.Game.Tests/Visual/Editing/TestSceneTimingScreen.cs b/osu.Game.Tests/Visual/Editing/TestSceneTimingScreen.cs index 4bbffbdc7a..17b8189fc7 100644 --- a/osu.Game.Tests/Visual/Editing/TestSceneTimingScreen.cs +++ b/osu.Game.Tests/Visual/Editing/TestSceneTimingScreen.cs @@ -29,9 +29,10 @@ namespace osu.Game.Tests.Visual.Editing editorBeatmap = new EditorBeatmap(CreateBeatmap(new OsuRuleset().RulesetInfo)); } - [BackgroundDependencyLoader] - private void load() + protected override void LoadComplete() { + base.LoadComplete(); + Beatmap.Value = CreateWorkingBeatmap(editorBeatmap.PlayableBeatmap); Beatmap.Disabled = true; diff --git a/osu.Game/Tests/Visual/EditorClockTestScene.cs b/osu.Game/Tests/Visual/EditorClockTestScene.cs index c2e9892735..66ab427565 100644 --- a/osu.Game/Tests/Visual/EditorClockTestScene.cs +++ b/osu.Game/Tests/Visual/EditorClockTestScene.cs @@ -35,9 +35,9 @@ namespace osu.Game.Tests.Visual return dependencies; } - [BackgroundDependencyLoader] - private void load() + protected override void LoadComplete() { + base.LoadComplete(); Beatmap.BindValueChanged(beatmapChanged, true); } diff --git a/osu.Game/Tests/Visual/EditorTestScene.cs b/osu.Game/Tests/Visual/EditorTestScene.cs index 07152b5a3e..d9dd6e4fa8 100644 --- a/osu.Game/Tests/Visual/EditorTestScene.cs +++ b/osu.Game/Tests/Visual/EditorTestScene.cs @@ -42,17 +42,27 @@ namespace osu.Game.Tests.Visual Alpha = 0 }; + private TestBeatmapManager testBeatmapManager; + private WorkingBeatmap working; + [BackgroundDependencyLoader] private void load(GameHost host, AudioManager audio, RulesetStore rulesets) { Add(logo); - var working = CreateWorkingBeatmap(Ruleset.Value); - - Beatmap.Value = working; + working = CreateWorkingBeatmap(Ruleset.Value); if (IsolateSavingFromDatabase) - Dependencies.CacheAs(new TestBeatmapManager(LocalStorage, ContextFactory, rulesets, null, audio, Resources, host, Beatmap.Default, working)); + Dependencies.CacheAs(testBeatmapManager = new TestBeatmapManager(LocalStorage, ContextFactory, rulesets, null, audio, Resources, host, Beatmap.Default)); + } + + protected override void LoadComplete() + { + base.LoadComplete(); + + Beatmap.Value = working; + if (testBeatmapManager != null) + testBeatmapManager.TestBeatmap = working; } protected virtual bool EditorComponentsReady => Editor.ChildrenOfType().FirstOrDefault()?.IsLoaded == true @@ -114,12 +124,11 @@ namespace osu.Game.Tests.Visual private class TestBeatmapManager : BeatmapManager { - private readonly WorkingBeatmap testBeatmap; + public WorkingBeatmap TestBeatmap; - public TestBeatmapManager(Storage storage, IDatabaseContextFactory contextFactory, RulesetStore rulesets, IAPIProvider api, [NotNull] AudioManager audioManager, IResourceStore resources, GameHost host, WorkingBeatmap defaultBeatmap, WorkingBeatmap testBeatmap) + public TestBeatmapManager(Storage storage, IDatabaseContextFactory contextFactory, RulesetStore rulesets, IAPIProvider api, [NotNull] AudioManager audioManager, IResourceStore resources, GameHost host, WorkingBeatmap defaultBeatmap) : base(storage, contextFactory, rulesets, api, audioManager, resources, host, defaultBeatmap) { - this.testBeatmap = testBeatmap; } protected override BeatmapModelManager CreateBeatmapModelManager(Storage storage, IDatabaseContextFactory contextFactory, RulesetStore rulesets, IAPIProvider api, GameHost host) @@ -143,7 +152,7 @@ namespace osu.Game.Tests.Visual } public override WorkingBeatmap GetWorkingBeatmap(BeatmapInfo beatmapInfo) - => testBeatmapManager.testBeatmap; + => testBeatmapManager.TestBeatmap; } internal class TestBeatmapModelManager : BeatmapModelManager From 8978b88f69300d868635d295104d093b0aafe76b Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Tue, 18 Jan 2022 16:22:11 +0900 Subject: [PATCH 5/6] Add test coverage of startup ruleset being non-default --- .../Navigation/TestSceneStartupRuleset.cs | 32 +++++++++++++++++++ 1 file changed, 32 insertions(+) create mode 100644 osu.Game.Tests/Visual/Navigation/TestSceneStartupRuleset.cs diff --git a/osu.Game.Tests/Visual/Navigation/TestSceneStartupRuleset.cs b/osu.Game.Tests/Visual/Navigation/TestSceneStartupRuleset.cs new file mode 100644 index 0000000000..85dd501fd3 --- /dev/null +++ b/osu.Game.Tests/Visual/Navigation/TestSceneStartupRuleset.cs @@ -0,0 +1,32 @@ +// Copyright (c) ppy Pty Ltd . Licensed under the MIT Licence. +// See the LICENCE file in the repository root for full licence text. + +using NUnit.Framework; +using osu.Framework.Development; +using osu.Game.Configuration; + +namespace osu.Game.Tests.Visual.Navigation +{ + [TestFixture] + public class TestSceneStartupRuleset : OsuGameTestScene + { + protected override TestOsuGame CreateTestGame() + { + // Must be done in this function due to the RecycleLocalStorage call just before. + var config = DebugUtils.IsDebugBuild + ? new DevelopmentOsuConfigManager(LocalStorage) + : new OsuConfigManager(LocalStorage); + + config.SetValue(OsuSetting.Ruleset, "mania"); + config.Save(); + + return base.CreateTestGame(); + } + + [Test] + public void TestRulesetConsumed() + { + AddUntilStep("ruleset correct", () => Game.Ruleset.Value.ShortName == "mania"); + } + } +} From 8b8940439e770184fe80ce0c87d4387850575cc1 Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Tue, 18 Jan 2022 16:11:03 +0900 Subject: [PATCH 6/6] Fix starting game with non-default ruleset failing --- osu.Game/OsuGameBase.cs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/osu.Game/OsuGameBase.cs b/osu.Game/OsuGameBase.cs index 8126b74418..4c6e9689d3 100644 --- a/osu.Game/OsuGameBase.cs +++ b/osu.Game/OsuGameBase.cs @@ -451,13 +451,13 @@ namespace osu.Game private void onBeatmapChanged(ValueChangedEvent valueChangedEvent) { - if (!ThreadSafety.IsUpdateThread) + if (IsLoaded && !ThreadSafety.IsUpdateThread) throw new InvalidOperationException("Global beatmap bindable must be changed from update thread."); } private void onRulesetChanged(ValueChangedEvent r) { - if (!ThreadSafety.IsUpdateThread) + if (IsLoaded && !ThreadSafety.IsUpdateThread) throw new InvalidOperationException("Global ruleset bindable must be changed from update thread."); if (r.NewValue?.Available != true)