From 8241fee4a8aab8853d11853ca0757e08e13a3664 Mon Sep 17 00:00:00 2001 From: Salman Ahmed Date: Fri, 25 Jun 2021 09:22:34 +0300 Subject: [PATCH 1/9] Add failing test case --- .../TestSceneRulesetSkinProvidingContainer.cs | 100 ++++++++++++++++++ .../Testing/TestSceneRulesetDependencies.cs | 2 +- 2 files changed, 101 insertions(+), 1 deletion(-) create mode 100644 osu.Game.Tests/Rulesets/TestSceneRulesetSkinProvidingContainer.cs diff --git a/osu.Game.Tests/Rulesets/TestSceneRulesetSkinProvidingContainer.cs b/osu.Game.Tests/Rulesets/TestSceneRulesetSkinProvidingContainer.cs new file mode 100644 index 0000000000..dd78f28351 --- /dev/null +++ b/osu.Game.Tests/Rulesets/TestSceneRulesetSkinProvidingContainer.cs @@ -0,0 +1,100 @@ +// Copyright (c) ppy Pty Ltd . Licensed under the MIT Licence. + // See the LICENCE file in the repository root for full licence text. + +using System; +using System.Linq; +using JetBrains.Annotations; +using NUnit.Framework; +using osu.Framework.Allocation; +using osu.Framework.Audio.Sample; +using osu.Framework.Bindables; +using osu.Framework.Graphics; +using osu.Framework.Graphics.OpenGL.Textures; +using osu.Framework.Graphics.Textures; +using osu.Game.Audio; +using osu.Game.Beatmaps; +using osu.Game.Rulesets; +using osu.Game.Skinning; +using osu.Game.Tests.Testing; +using osu.Game.Tests.Visual; + +namespace osu.Game.Tests.Rulesets +{ + public class TestSceneRulesetSkinProvidingContainer : OsuTestScene + { + [Resolved] + private SkinManager skins { get; set; } + + private SkinRequester requester; + + protected override Ruleset CreateRuleset() => new TestRuleset(); + + [Test] + public void TestEarlyAddedSkinRequester() + { + ISample transformerSampleOnBdl = null; + + // need a legacy skin to plug the TestRuleset's legacy transformer, which is required for testing this. + AddStep("set legacy skin", () => skins.CurrentSkinInfo.Value = DefaultLegacySkin.Info); + + AddStep("setup provider", () => + { + var rulesetSkinProvider = new RulesetSkinProvidingContainer(Ruleset.Value.CreateInstance(), Beatmap.Value.Beatmap, Beatmap.Value.Skin); + + rulesetSkinProvider.Add(requester = new SkinRequester()); + + requester.OnBdl += () => transformerSampleOnBdl = requester.GetSample(new SampleInfo(TestLegacySkinTransformer.VIRTUAL_SAMPLE_NAME)); + + Child = rulesetSkinProvider; + }); + + AddAssert("requester got correct initial sample", () => transformerSampleOnBdl != null); + } + + private class SkinRequester : Drawable, ISkin + { + private ISkinSource skin; + + public event Action OnBdl; + + [BackgroundDependencyLoader] + private void load(ISkinSource skin) + { + this.skin = skin; + + OnBdl?.Invoke(); + } + + public Drawable GetDrawableComponent(ISkinComponent component) => skin.GetDrawableComponent(component); + + public Texture GetTexture(string componentName, WrapMode wrapModeS = default, WrapMode wrapModeT = default) => skin.GetTexture(componentName); + + public ISample GetSample(ISampleInfo sampleInfo) => skin.GetSample(sampleInfo); + + public IBindable GetConfig(TLookup lookup) => skin.GetConfig(lookup); + } + + private class TestRuleset : TestSceneRulesetDependencies.TestRuleset + { + public override ISkin CreateLegacySkinProvider(ISkin skin, IBeatmap beatmap) => new TestLegacySkinTransformer(skin); + } + + private class TestLegacySkinTransformer : LegacySkinTransformer + { + public const string VIRTUAL_SAMPLE_NAME = "virtual-test-sample"; + + public TestLegacySkinTransformer([NotNull] ISkin skin) + : base(skin) + { + } + + public override ISample GetSample(ISampleInfo sampleInfo) + { + if (sampleInfo.LookupNames.Single() == VIRTUAL_SAMPLE_NAME) + return new SampleVirtual(); + + return base.GetSample(sampleInfo); + } + } + } +} diff --git a/osu.Game.Tests/Testing/TestSceneRulesetDependencies.cs b/osu.Game.Tests/Testing/TestSceneRulesetDependencies.cs index 97087e31ab..fb50da32f3 100644 --- a/osu.Game.Tests/Testing/TestSceneRulesetDependencies.cs +++ b/osu.Game.Tests/Testing/TestSceneRulesetDependencies.cs @@ -52,7 +52,7 @@ namespace osu.Game.Tests.Testing Dependencies.Get() != null); } - private class TestRuleset : Ruleset + public class TestRuleset : Ruleset { public override string Description => string.Empty; public override string ShortName => string.Empty; From f07008a0a284023e50f63e7de3c2392d2de0e626 Mon Sep 17 00:00:00 2001 From: Salman Ahmed Date: Fri, 25 Jun 2021 09:55:29 +0300 Subject: [PATCH 2/9] Fix `RulesetSkinProvidingContainer` potentially late in setting up skin sources --- .../Skinning/RulesetSkinProvidingContainer.cs | 25 +++++++++++-------- 1 file changed, 14 insertions(+), 11 deletions(-) diff --git a/osu.Game/Skinning/RulesetSkinProvidingContainer.cs b/osu.Game/Skinning/RulesetSkinProvidingContainer.cs index c48aeca99a..abf5cb040a 100644 --- a/osu.Game/Skinning/RulesetSkinProvidingContainer.cs +++ b/osu.Game/Skinning/RulesetSkinProvidingContainer.cs @@ -42,27 +42,30 @@ namespace osu.Game.Skinning }; } - [Resolved] - private ISkinSource skinSource { get; set; } + private ISkinSource parentSource; - [BackgroundDependencyLoader] - private void load() + protected override IReadOnlyDependencyContainer CreateChildDependencies(IReadOnlyDependencyContainer parent) { - UpdateSkins(); - skinSource.SourceChanged += OnSourceChanged; + parentSource = parent.Get(); + + UpdateSkinSources(); + + parentSource.SourceChanged += OnSourceChanged; + + return base.CreateChildDependencies(parent); } protected override void OnSourceChanged() { - UpdateSkins(); + UpdateSkinSources(); base.OnSourceChanged(); } - protected virtual void UpdateSkins() + protected virtual void UpdateSkinSources() { SkinSources.Clear(); - foreach (var skin in skinSource.AllSources) + foreach (var skin in parentSource.AllSources) { switch (skin) { @@ -93,8 +96,8 @@ namespace osu.Game.Skinning { base.Dispose(isDisposing); - if (skinSource != null) - skinSource.SourceChanged -= OnSourceChanged; + if (parentSource != null) + parentSource.SourceChanged -= OnSourceChanged; } } } From 06e357647abce9f383d0dee2c66bfc85440a41e7 Mon Sep 17 00:00:00 2001 From: Salman Ahmed Date: Fri, 25 Jun 2021 10:40:07 +0300 Subject: [PATCH 3/9] OnBdl -> OnLoadAsync --- .../Rulesets/TestSceneRulesetSkinProvidingContainer.cs | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/osu.Game.Tests/Rulesets/TestSceneRulesetSkinProvidingContainer.cs b/osu.Game.Tests/Rulesets/TestSceneRulesetSkinProvidingContainer.cs index dd78f28351..3072c466e0 100644 --- a/osu.Game.Tests/Rulesets/TestSceneRulesetSkinProvidingContainer.cs +++ b/osu.Game.Tests/Rulesets/TestSceneRulesetSkinProvidingContainer.cs @@ -32,7 +32,7 @@ namespace osu.Game.Tests.Rulesets [Test] public void TestEarlyAddedSkinRequester() { - ISample transformerSampleOnBdl = null; + ISample transformerSampleOnLoad = null; // need a legacy skin to plug the TestRuleset's legacy transformer, which is required for testing this. AddStep("set legacy skin", () => skins.CurrentSkinInfo.Value = DefaultLegacySkin.Info); @@ -43,26 +43,26 @@ namespace osu.Game.Tests.Rulesets rulesetSkinProvider.Add(requester = new SkinRequester()); - requester.OnBdl += () => transformerSampleOnBdl = requester.GetSample(new SampleInfo(TestLegacySkinTransformer.VIRTUAL_SAMPLE_NAME)); + requester.OnLoadAsync += () => transformerSampleOnLoad = requester.GetSample(new SampleInfo(TestLegacySkinTransformer.VIRTUAL_SAMPLE_NAME)); Child = rulesetSkinProvider; }); - AddAssert("requester got correct initial sample", () => transformerSampleOnBdl != null); + AddAssert("requester got correct initial sample", () => transformerSampleOnLoad != null); } private class SkinRequester : Drawable, ISkin { private ISkinSource skin; - public event Action OnBdl; + public event Action OnLoadAsync; [BackgroundDependencyLoader] private void load(ISkinSource skin) { this.skin = skin; - OnBdl?.Invoke(); + OnLoadAsync?.Invoke(); } public Drawable GetDrawableComponent(ISkinComponent component) => skin.GetDrawableComponent(component); From 8d7705dc923b04ea037803260fc40fbc02f55933 Mon Sep 17 00:00:00 2001 From: Salman Ahmed Date: Fri, 25 Jun 2021 10:55:23 +0300 Subject: [PATCH 4/9] Test using a simple `GetTexture` lookup instead Presumes that `RulesetSkinProvidingContainer` doesn't allow falling back to parent skins, whatsoever. --- .../TestSceneRulesetSkinProvidingContainer.cs | 37 +++---------------- 1 file changed, 5 insertions(+), 32 deletions(-) diff --git a/osu.Game.Tests/Rulesets/TestSceneRulesetSkinProvidingContainer.cs b/osu.Game.Tests/Rulesets/TestSceneRulesetSkinProvidingContainer.cs index 3072c466e0..50b75ea0c5 100644 --- a/osu.Game.Tests/Rulesets/TestSceneRulesetSkinProvidingContainer.cs +++ b/osu.Game.Tests/Rulesets/TestSceneRulesetSkinProvidingContainer.cs @@ -2,8 +2,6 @@ // See the LICENCE file in the repository root for full licence text. using System; -using System.Linq; -using JetBrains.Annotations; using NUnit.Framework; using osu.Framework.Allocation; using osu.Framework.Audio.Sample; @@ -12,7 +10,6 @@ using osu.Framework.Graphics; using osu.Framework.Graphics.OpenGL.Textures; using osu.Framework.Graphics.Textures; using osu.Game.Audio; -using osu.Game.Beatmaps; using osu.Game.Rulesets; using osu.Game.Skinning; using osu.Game.Tests.Testing; @@ -27,15 +24,14 @@ namespace osu.Game.Tests.Rulesets private SkinRequester requester; - protected override Ruleset CreateRuleset() => new TestRuleset(); + protected override Ruleset CreateRuleset() => new TestSceneRulesetDependencies.TestRuleset(); [Test] public void TestEarlyAddedSkinRequester() { - ISample transformerSampleOnLoad = null; + Texture textureOnLoad = null; - // need a legacy skin to plug the TestRuleset's legacy transformer, which is required for testing this. - AddStep("set legacy skin", () => skins.CurrentSkinInfo.Value = DefaultLegacySkin.Info); + AddStep("set skin", () => skins.CurrentSkinInfo.Value = DefaultLegacySkin.Info); AddStep("setup provider", () => { @@ -43,12 +39,12 @@ namespace osu.Game.Tests.Rulesets rulesetSkinProvider.Add(requester = new SkinRequester()); - requester.OnLoadAsync += () => transformerSampleOnLoad = requester.GetSample(new SampleInfo(TestLegacySkinTransformer.VIRTUAL_SAMPLE_NAME)); + requester.OnLoadAsync += () => textureOnLoad = requester.GetTexture("hitcircle"); Child = rulesetSkinProvider; }); - AddAssert("requester got correct initial sample", () => transformerSampleOnLoad != null); + AddAssert("requester got correct initial texture", () => textureOnLoad != null); } private class SkinRequester : Drawable, ISkin @@ -73,28 +69,5 @@ namespace osu.Game.Tests.Rulesets public IBindable GetConfig(TLookup lookup) => skin.GetConfig(lookup); } - - private class TestRuleset : TestSceneRulesetDependencies.TestRuleset - { - public override ISkin CreateLegacySkinProvider(ISkin skin, IBeatmap beatmap) => new TestLegacySkinTransformer(skin); - } - - private class TestLegacySkinTransformer : LegacySkinTransformer - { - public const string VIRTUAL_SAMPLE_NAME = "virtual-test-sample"; - - public TestLegacySkinTransformer([NotNull] ISkin skin) - : base(skin) - { - } - - public override ISample GetSample(ISampleInfo sampleInfo) - { - if (sampleInfo.LookupNames.Single() == VIRTUAL_SAMPLE_NAME) - return new SampleVirtual(); - - return base.GetSample(sampleInfo); - } - } } } From 13ed52a990cc4f1b53b3a1f1b37694859b5427cf Mon Sep 17 00:00:00 2001 From: Salman Ahmed Date: Fri, 25 Jun 2021 11:16:26 +0300 Subject: [PATCH 5/9] Fix weird license misindent No idea how the hell that happened... R# silent about it, of course. --- .../Rulesets/TestSceneRulesetSkinProvidingContainer.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/osu.Game.Tests/Rulesets/TestSceneRulesetSkinProvidingContainer.cs b/osu.Game.Tests/Rulesets/TestSceneRulesetSkinProvidingContainer.cs index 50b75ea0c5..b6800e40e4 100644 --- a/osu.Game.Tests/Rulesets/TestSceneRulesetSkinProvidingContainer.cs +++ b/osu.Game.Tests/Rulesets/TestSceneRulesetSkinProvidingContainer.cs @@ -1,5 +1,5 @@ // Copyright (c) ppy Pty Ltd . Licensed under the MIT Licence. - // See the LICENCE file in the repository root for full licence text. +// See the LICENCE file in the repository root for full licence text. using System; using NUnit.Framework; From ff5e590d323ce868cce4eeaed71280645e6ae729 Mon Sep 17 00:00:00 2001 From: Salman Ahmed Date: Fri, 25 Jun 2021 12:00:46 +0300 Subject: [PATCH 6/9] Add local source for testing --- .../TestSceneRulesetSkinProvidingContainer.cs | 23 ++++++++++++++----- 1 file changed, 17 insertions(+), 6 deletions(-) diff --git a/osu.Game.Tests/Rulesets/TestSceneRulesetSkinProvidingContainer.cs b/osu.Game.Tests/Rulesets/TestSceneRulesetSkinProvidingContainer.cs index b6800e40e4..b058cc3694 100644 --- a/osu.Game.Tests/Rulesets/TestSceneRulesetSkinProvidingContainer.cs +++ b/osu.Game.Tests/Rulesets/TestSceneRulesetSkinProvidingContainer.cs @@ -19,11 +19,11 @@ namespace osu.Game.Tests.Rulesets { public class TestSceneRulesetSkinProvidingContainer : OsuTestScene { - [Resolved] - private SkinManager skins { get; set; } - private SkinRequester requester; + [Cached(typeof(ISkin))] + private readonly TestSkinProvider testSkin = new TestSkinProvider(); + protected override Ruleset CreateRuleset() => new TestSceneRulesetDependencies.TestRuleset(); [Test] @@ -31,15 +31,13 @@ namespace osu.Game.Tests.Rulesets { Texture textureOnLoad = null; - AddStep("set skin", () => skins.CurrentSkinInfo.Value = DefaultLegacySkin.Info); - AddStep("setup provider", () => { var rulesetSkinProvider = new RulesetSkinProvidingContainer(Ruleset.Value.CreateInstance(), Beatmap.Value.Beatmap, Beatmap.Value.Skin); rulesetSkinProvider.Add(requester = new SkinRequester()); - requester.OnLoadAsync += () => textureOnLoad = requester.GetTexture("hitcircle"); + requester.OnLoadAsync += () => textureOnLoad = requester.GetTexture(TestSkinProvider.TEXTURE_NAME); Child = rulesetSkinProvider; }); @@ -69,5 +67,18 @@ namespace osu.Game.Tests.Rulesets public IBindable GetConfig(TLookup lookup) => skin.GetConfig(lookup); } + + private class TestSkinProvider : ISkin + { + public const string TEXTURE_NAME = "some-texture"; + + public Drawable GetDrawableComponent(ISkinComponent component) => throw new NotImplementedException(); + + public Texture GetTexture(string componentName, WrapMode wrapModeS, WrapMode wrapModeT) => componentName == TEXTURE_NAME ? Texture.WhitePixel : null; + + public ISample GetSample(ISampleInfo sampleInfo) => throw new NotImplementedException(); + + public IBindable GetConfig(TLookup lookup) => throw new NotImplementedException(); + } } } From 84c9ede966af09a0b4b1337ce7750d01b14d7c03 Mon Sep 17 00:00:00 2001 From: Salman Ahmed Date: Fri, 25 Jun 2021 13:17:11 +0300 Subject: [PATCH 7/9] Fix incorrect pushed changes This should've been in the original commit, but for some reason got deleted out. --- .../TestSceneRulesetSkinProvidingContainer.cs | 23 +++++++++++++++---- 1 file changed, 19 insertions(+), 4 deletions(-) diff --git a/osu.Game.Tests/Rulesets/TestSceneRulesetSkinProvidingContainer.cs b/osu.Game.Tests/Rulesets/TestSceneRulesetSkinProvidingContainer.cs index b058cc3694..0dde0a8194 100644 --- a/osu.Game.Tests/Rulesets/TestSceneRulesetSkinProvidingContainer.cs +++ b/osu.Game.Tests/Rulesets/TestSceneRulesetSkinProvidingContainer.cs @@ -2,6 +2,7 @@ // See the LICENCE file in the repository root for full licence text. using System; +using System.Collections.Generic; using NUnit.Framework; using osu.Framework.Allocation; using osu.Framework.Audio.Sample; @@ -21,11 +22,15 @@ namespace osu.Game.Tests.Rulesets { private SkinRequester requester; - [Cached(typeof(ISkin))] - private readonly TestSkinProvider testSkin = new TestSkinProvider(); - protected override Ruleset CreateRuleset() => new TestSceneRulesetDependencies.TestRuleset(); + protected override IReadOnlyDependencyContainer CreateChildDependencies(IReadOnlyDependencyContainer parent) + { + var dependencies = new DependencyContainer(base.CreateChildDependencies(parent)); + dependencies.CacheAs(new TestSkinProvider()); + return dependencies; + } + [Test] public void TestEarlyAddedSkinRequester() { @@ -68,7 +73,7 @@ namespace osu.Game.Tests.Rulesets public IBindable GetConfig(TLookup lookup) => skin.GetConfig(lookup); } - private class TestSkinProvider : ISkin + private class TestSkinProvider : ISkinSource { public const string TEXTURE_NAME = "some-texture"; @@ -79,6 +84,16 @@ namespace osu.Game.Tests.Rulesets public ISample GetSample(ISampleInfo sampleInfo) => throw new NotImplementedException(); public IBindable GetConfig(TLookup lookup) => throw new NotImplementedException(); + + public event Action SourceChanged + { + add { } + remove { } + } + + public ISkin FindProvider(Func lookupFunction) => lookupFunction(this) ? this : null; + + public IEnumerable AllSources => new[] { this }; } } } From e387feb1d6261b491246e014de199e921a290103 Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Mon, 28 Jun 2021 14:39:55 +0900 Subject: [PATCH 8/9] Add inline comment mentioning why `CreateChildDependencies` is being used in this instance --- osu.Game/Skinning/RulesetSkinProvidingContainer.cs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/osu.Game/Skinning/RulesetSkinProvidingContainer.cs b/osu.Game/Skinning/RulesetSkinProvidingContainer.cs index abf5cb040a..cb8b0fb3c8 100644 --- a/osu.Game/Skinning/RulesetSkinProvidingContainer.cs +++ b/osu.Game/Skinning/RulesetSkinProvidingContainer.cs @@ -47,11 +47,11 @@ namespace osu.Game.Skinning protected override IReadOnlyDependencyContainer CreateChildDependencies(IReadOnlyDependencyContainer parent) { parentSource = parent.Get(); - - UpdateSkinSources(); - parentSource.SourceChanged += OnSourceChanged; + // ensure sources are populated and ready for use before childrens' asynchronous load flow. + UpdateSkinSources(); + return base.CreateChildDependencies(parent); } From 73bd88cb318ae1c8b4690a8398587bb87f83b3ee Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Mon, 28 Jun 2021 14:44:52 +0900 Subject: [PATCH 9/9] Simplify caching in test --- .../Rulesets/TestSceneRulesetSkinProvidingContainer.cs | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) diff --git a/osu.Game.Tests/Rulesets/TestSceneRulesetSkinProvidingContainer.cs b/osu.Game.Tests/Rulesets/TestSceneRulesetSkinProvidingContainer.cs index 0dde0a8194..25619de323 100644 --- a/osu.Game.Tests/Rulesets/TestSceneRulesetSkinProvidingContainer.cs +++ b/osu.Game.Tests/Rulesets/TestSceneRulesetSkinProvidingContainer.cs @@ -24,12 +24,8 @@ namespace osu.Game.Tests.Rulesets protected override Ruleset CreateRuleset() => new TestSceneRulesetDependencies.TestRuleset(); - protected override IReadOnlyDependencyContainer CreateChildDependencies(IReadOnlyDependencyContainer parent) - { - var dependencies = new DependencyContainer(base.CreateChildDependencies(parent)); - dependencies.CacheAs(new TestSkinProvider()); - return dependencies; - } + [Cached(typeof(ISkinSource))] + private readonly ISkinSource testSource = new TestSkinProvider(); [Test] public void TestEarlyAddedSkinRequester()