From 7c29386717cc7f8c96668dc9fb4d970c7b43a17a Mon Sep 17 00:00:00 2001 From: smoogipoo Date: Tue, 2 Feb 2021 18:01:33 +0900 Subject: [PATCH 1/6] Add failing tests --- osu.Game.Tests/Mods/ModUtilsTest.cs | 23 +++++++++++++++++++++++ 1 file changed, 23 insertions(+) diff --git a/osu.Game.Tests/Mods/ModUtilsTest.cs b/osu.Game.Tests/Mods/ModUtilsTest.cs index fdb441343a..b602c082bf 100644 --- a/osu.Game.Tests/Mods/ModUtilsTest.cs +++ b/osu.Game.Tests/Mods/ModUtilsTest.cs @@ -47,6 +47,29 @@ namespace osu.Game.Tests.Mods Assert.That(ModUtils.CheckCompatibleSet(new[] { mod1.Object, multiMod }), Is.False); } + [Test] + public void TestCompatibleMods() + { + var mod1 = new Mock(); + var mod2 = new Mock(); + + // Test both orderings. + Assert.That(ModUtils.CheckCompatibleSet(new[] { mod1.Object, mod2.Object }), Is.True); + Assert.That(ModUtils.CheckCompatibleSet(new[] { mod2.Object, mod1.Object }), Is.True); + } + + [Test] + public void TestIncompatibleThroughBaseType() + { + var mod1 = new Mock(); + var mod2 = new Mock(); + mod2.Setup(m => m.IncompatibleMods).Returns(new[] { mod1.Object.GetType().BaseType }); + + // Test both orderings. + Assert.That(ModUtils.CheckCompatibleSet(new[] { mod1.Object, mod2.Object }), Is.False); + Assert.That(ModUtils.CheckCompatibleSet(new[] { mod2.Object, mod1.Object }), Is.False); + } + [Test] public void TestAllowedThroughMostDerivedType() { From 8232d9d2fe667a201aded46f4df1dba44b93ae7e Mon Sep 17 00:00:00 2001 From: smoogipoo Date: Tue, 2 Feb 2021 18:01:38 +0900 Subject: [PATCH 2/6] Fix incorrect implementation --- osu.Game/Utils/ModUtils.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/osu.Game/Utils/ModUtils.cs b/osu.Game/Utils/ModUtils.cs index 808dba2900..34bc0faca4 100644 --- a/osu.Game/Utils/ModUtils.cs +++ b/osu.Game/Utils/ModUtils.cs @@ -56,7 +56,7 @@ namespace osu.Game.Utils // Add the new mod types, checking whether any match the incompatible types. foreach (var t in mod.GetType().EnumerateBaseTypes()) { - if (incomingTypes.Contains(t)) + if (incompatibleTypes.Contains(t)) return false; incomingTypes.Add(t); From d0655c21c6db7e73b006a837b276d2efd0492e27 Mon Sep 17 00:00:00 2001 From: smoogipoo Date: Tue, 2 Feb 2021 18:18:57 +0900 Subject: [PATCH 3/6] Simplify implementation of CheckCompatibleSet --- osu.Game/Utils/ModUtils.cs | 40 ++++++++++++++++++-------------------- 1 file changed, 19 insertions(+), 21 deletions(-) diff --git a/osu.Game/Utils/ModUtils.cs b/osu.Game/Utils/ModUtils.cs index 34bc0faca4..a9271db1b5 100644 --- a/osu.Game/Utils/ModUtils.cs +++ b/osu.Game/Utils/ModUtils.cs @@ -4,7 +4,6 @@ using System; using System.Collections.Generic; using System.Linq; -using osu.Framework.Extensions.TypeExtensions; using osu.Game.Rulesets.Mods; #nullable enable @@ -29,7 +28,7 @@ namespace osu.Game.Utils { // Prevent multiple-enumeration. var combinationList = combination as ICollection ?? combination.ToArray(); - return CheckCompatibleSet(combinationList) && CheckAllowed(combinationList, allowedTypes); + return CheckCompatibleSet(combinationList, out _) && CheckAllowed(combinationList, allowedTypes); } /// @@ -38,32 +37,31 @@ namespace osu.Game.Utils /// The combination to check. /// Whether all s in the combination are compatible with each-other. public static bool CheckCompatibleSet(IEnumerable combination) + => CheckCompatibleSet(combination, out _); + + /// + /// Checks that all s in a combination are compatible with each-other. + /// + /// The combination to check. + /// Any invalid mods in the set. + /// Whether all s in the combination are compatible with each-other. + public static bool CheckCompatibleSet(IEnumerable combination, out List? invalidMods) { - var incompatibleTypes = new HashSet(); - var incomingTypes = new HashSet(); + invalidMods = null; - foreach (var mod in combination.SelectMany(FlattenMod)) + foreach (var mod in combination) { - // Add the new mod incompatibilities, checking whether any match the existing mod types. - foreach (var t in mod.IncompatibleMods) + foreach (var type in mod.IncompatibleMods) { - if (incomingTypes.Contains(t)) - return false; - - incompatibleTypes.Add(t); - } - - // Add the new mod types, checking whether any match the incompatible types. - foreach (var t in mod.GetType().EnumerateBaseTypes()) - { - if (incompatibleTypes.Contains(t)) - return false; - - incomingTypes.Add(t); + foreach (var invalid in combination.Where(m => type.IsInstanceOfType(m))) + { + invalidMods ??= new List(); + invalidMods.Add(invalid); + } } } - return true; + return invalidMods == null; } /// From 1df412a03cde9c1ef9363fc1759f8942ea73ec94 Mon Sep 17 00:00:00 2001 From: smoogipoo Date: Tue, 2 Feb 2021 18:31:08 +0900 Subject: [PATCH 4/6] Fix incorrect handling of multi-mod incompatibilities --- osu.Game.Tests/Mods/ModUtilsTest.cs | 25 ++++++++++++++++++++++++- osu.Game/Utils/ModUtils.cs | 1 + 2 files changed, 25 insertions(+), 1 deletion(-) diff --git a/osu.Game.Tests/Mods/ModUtilsTest.cs b/osu.Game.Tests/Mods/ModUtilsTest.cs index b602c082bf..7d3dea7ed5 100644 --- a/osu.Game.Tests/Mods/ModUtilsTest.cs +++ b/osu.Game.Tests/Mods/ModUtilsTest.cs @@ -32,7 +32,7 @@ namespace osu.Game.Tests.Mods } [Test] - public void TestIncompatibleThroughMultiMod() + public void TestMultiModIncompatibleWithTopLevel() { var mod1 = new Mock(); @@ -47,6 +47,21 @@ namespace osu.Game.Tests.Mods Assert.That(ModUtils.CheckCompatibleSet(new[] { mod1.Object, multiMod }), Is.False); } + [Test] + public void TestTopLevelIncompatibleWithMultiMod() + { + // The nested mod. + var mod1 = new Mock(); + var multiMod = new MultiMod(new MultiMod(mod1.Object)); + + var mod2 = new Mock(); + mod2.Setup(m => m.IncompatibleMods).Returns(new[] { typeof(CustomMod1) }); + + // Test both orderings. + Assert.That(ModUtils.CheckCompatibleSet(new Mod[] { multiMod, mod2.Object }), Is.False); + Assert.That(ModUtils.CheckCompatibleSet(new Mod[] { mod2.Object, multiMod }), Is.False); + } + [Test] public void TestCompatibleMods() { @@ -83,5 +98,13 @@ namespace osu.Game.Tests.Mods var mod = new Mock(); Assert.That(ModUtils.CheckAllowed(new[] { mod.Object }, new[] { typeof(Mod) }), Is.False); } + + public abstract class CustomMod1 : Mod + { + } + + public abstract class CustomMod2 : Mod + { + } } } diff --git a/osu.Game/Utils/ModUtils.cs b/osu.Game/Utils/ModUtils.cs index a9271db1b5..41f7b1b45c 100644 --- a/osu.Game/Utils/ModUtils.cs +++ b/osu.Game/Utils/ModUtils.cs @@ -47,6 +47,7 @@ namespace osu.Game.Utils /// Whether all s in the combination are compatible with each-other. public static bool CheckCompatibleSet(IEnumerable combination, out List? invalidMods) { + combination = FlattenMods(combination); invalidMods = null; foreach (var mod in combination) From 9955e0289869be6a14cb67e762a75e9b49a599b1 Mon Sep 17 00:00:00 2001 From: smoogipoo Date: Tue, 2 Feb 2021 18:33:12 +0900 Subject: [PATCH 5/6] Make more tests use the custom mod classes For safety purposes... In implementing the previous tests, I found that using mod.Object.GetType() can lead to bad assertions since the same ModProxy class is used for all mocked classes. --- osu.Game.Tests/Mods/ModUtilsTest.cs | 40 ++++++++++++++--------------- 1 file changed, 20 insertions(+), 20 deletions(-) diff --git a/osu.Game.Tests/Mods/ModUtilsTest.cs b/osu.Game.Tests/Mods/ModUtilsTest.cs index 7d3dea7ed5..e4ded602aa 100644 --- a/osu.Game.Tests/Mods/ModUtilsTest.cs +++ b/osu.Game.Tests/Mods/ModUtilsTest.cs @@ -14,37 +14,37 @@ namespace osu.Game.Tests.Mods [Test] public void TestModIsCompatibleByItself() { - var mod = new Mock(); + var mod = new Mock(); Assert.That(ModUtils.CheckCompatibleSet(new[] { mod.Object })); } [Test] public void TestIncompatibleThroughTopLevel() { - var mod1 = new Mock(); - var mod2 = new Mock(); + var mod1 = new Mock(); + var mod2 = new Mock(); mod1.Setup(m => m.IncompatibleMods).Returns(new[] { mod2.Object.GetType() }); // Test both orderings. - Assert.That(ModUtils.CheckCompatibleSet(new[] { mod1.Object, mod2.Object }), Is.False); - Assert.That(ModUtils.CheckCompatibleSet(new[] { mod2.Object, mod1.Object }), Is.False); + Assert.That(ModUtils.CheckCompatibleSet(new Mod[] { mod1.Object, mod2.Object }), Is.False); + Assert.That(ModUtils.CheckCompatibleSet(new Mod[] { mod2.Object, mod1.Object }), Is.False); } [Test] public void TestMultiModIncompatibleWithTopLevel() { - var mod1 = new Mock(); + var mod1 = new Mock(); // The nested mod. - var mod2 = new Mock(); + var mod2 = new Mock(); mod2.Setup(m => m.IncompatibleMods).Returns(new[] { mod1.Object.GetType() }); var multiMod = new MultiMod(new MultiMod(mod2.Object)); // Test both orderings. - Assert.That(ModUtils.CheckCompatibleSet(new[] { multiMod, mod1.Object }), Is.False); - Assert.That(ModUtils.CheckCompatibleSet(new[] { mod1.Object, multiMod }), Is.False); + Assert.That(ModUtils.CheckCompatibleSet(new Mod[] { multiMod, mod1.Object }), Is.False); + Assert.That(ModUtils.CheckCompatibleSet(new Mod[] { mod1.Object, multiMod }), Is.False); } [Test] @@ -65,37 +65,37 @@ namespace osu.Game.Tests.Mods [Test] public void TestCompatibleMods() { - var mod1 = new Mock(); - var mod2 = new Mock(); + var mod1 = new Mock(); + var mod2 = new Mock(); // Test both orderings. - Assert.That(ModUtils.CheckCompatibleSet(new[] { mod1.Object, mod2.Object }), Is.True); - Assert.That(ModUtils.CheckCompatibleSet(new[] { mod2.Object, mod1.Object }), Is.True); + Assert.That(ModUtils.CheckCompatibleSet(new Mod[] { mod1.Object, mod2.Object }), Is.True); + Assert.That(ModUtils.CheckCompatibleSet(new Mod[] { mod2.Object, mod1.Object }), Is.True); } [Test] public void TestIncompatibleThroughBaseType() { - var mod1 = new Mock(); - var mod2 = new Mock(); - mod2.Setup(m => m.IncompatibleMods).Returns(new[] { mod1.Object.GetType().BaseType }); + var mod1 = new Mock(); + var mod2 = new Mock(); + mod2.Setup(m => m.IncompatibleMods).Returns(new[] { typeof(Mod) }); // Test both orderings. - Assert.That(ModUtils.CheckCompatibleSet(new[] { mod1.Object, mod2.Object }), Is.False); - Assert.That(ModUtils.CheckCompatibleSet(new[] { mod2.Object, mod1.Object }), Is.False); + Assert.That(ModUtils.CheckCompatibleSet(new Mod[] { mod1.Object, mod2.Object }), Is.False); + Assert.That(ModUtils.CheckCompatibleSet(new Mod[] { mod2.Object, mod1.Object }), Is.False); } [Test] public void TestAllowedThroughMostDerivedType() { - var mod = new Mock(); + var mod = new Mock(); Assert.That(ModUtils.CheckAllowed(new[] { mod.Object }, new[] { mod.Object.GetType() })); } [Test] public void TestNotAllowedThroughBaseType() { - var mod = new Mock(); + var mod = new Mock(); Assert.That(ModUtils.CheckAllowed(new[] { mod.Object }, new[] { typeof(Mod) }), Is.False); } From 12f52316cd2f198c0d649f41b46ef2975cfbb16d Mon Sep 17 00:00:00 2001 From: smoogipoo Date: Tue, 2 Feb 2021 18:37:11 +0900 Subject: [PATCH 6/6] Prevent multiple enumeration --- osu.Game/Utils/ModUtils.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/osu.Game/Utils/ModUtils.cs b/osu.Game/Utils/ModUtils.cs index 41f7b1b45c..9336add465 100644 --- a/osu.Game/Utils/ModUtils.cs +++ b/osu.Game/Utils/ModUtils.cs @@ -47,7 +47,7 @@ namespace osu.Game.Utils /// Whether all s in the combination are compatible with each-other. public static bool CheckCompatibleSet(IEnumerable combination, out List? invalidMods) { - combination = FlattenMods(combination); + combination = FlattenMods(combination).ToArray(); invalidMods = null; foreach (var mod in combination)