From de50b725d59be235c0be5ccee68d20f648c0c045 Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Tue, 12 May 2020 20:08:35 +0900 Subject: [PATCH 1/9] Fix mod failure checks executing actual game logic --- osu.Game.Rulesets.Osu/Mods/OsuModAutopilot.cs | 3 ++- osu.Game.Tests/Visual/Gameplay/TestSceneReplay.cs | 3 ++- osu.Game/Rulesets/Mods/IApplicableFailOverride.cs | 5 +++-- osu.Game/Rulesets/Mods/ModAutoplay.cs | 3 ++- osu.Game/Rulesets/Mods/ModBlockFail.cs | 2 +- osu.Game/Rulesets/Mods/ModEasy.cs | 13 +++++-------- osu.Game/Rulesets/Mods/ModSuddenDeath.cs | 3 ++- osu.Game/Screens/Play/Player.cs | 4 ++-- osu.Game/Screens/Play/ReplayPlayer.cs | 2 +- osu.Game/Tests/Visual/ModPerfectTestScene.cs | 2 +- osu.Game/Tests/Visual/ModTestScene.cs | 6 ++++-- 11 files changed, 25 insertions(+), 21 deletions(-) diff --git a/osu.Game.Rulesets.Osu/Mods/OsuModAutopilot.cs b/osu.Game.Rulesets.Osu/Mods/OsuModAutopilot.cs index fe46876050..d75f4c70d7 100644 --- a/osu.Game.Rulesets.Osu/Mods/OsuModAutopilot.cs +++ b/osu.Game.Rulesets.Osu/Mods/OsuModAutopilot.cs @@ -24,7 +24,8 @@ namespace osu.Game.Rulesets.Osu.Mods public override double ScoreMultiplier => 1; public override Type[] IncompatibleMods => new[] { typeof(OsuModSpunOut), typeof(ModRelax), typeof(ModSuddenDeath), typeof(ModNoFail), typeof(ModAutoplay) }; - public bool AllowFail => false; + public bool PerformFail() => false; + public bool RestartOnFail => false; private OsuInputManager inputManager; diff --git a/osu.Game.Tests/Visual/Gameplay/TestSceneReplay.cs b/osu.Game.Tests/Visual/Gameplay/TestSceneReplay.cs index e82722e7a2..1908988739 100644 --- a/osu.Game.Tests/Visual/Gameplay/TestSceneReplay.cs +++ b/osu.Game.Tests/Visual/Gameplay/TestSceneReplay.cs @@ -33,7 +33,8 @@ namespace osu.Game.Tests.Visual.Gameplay { public new ScoreProcessor ScoreProcessor => base.ScoreProcessor; public new HUDOverlay HUDOverlay => base.HUDOverlay; - public new bool AllowFail => base.AllowFail; + + public bool AllowFail => base.CheckModsAllowFailure(); protected override bool PauseOnFocusLost => false; diff --git a/osu.Game/Rulesets/Mods/IApplicableFailOverride.cs b/osu.Game/Rulesets/Mods/IApplicableFailOverride.cs index 120bfc9a23..8c99d739cb 100644 --- a/osu.Game/Rulesets/Mods/IApplicableFailOverride.cs +++ b/osu.Game/Rulesets/Mods/IApplicableFailOverride.cs @@ -11,10 +11,11 @@ namespace osu.Game.Rulesets.Mods /// /// Whether we should allow failing at the current point in time. /// - bool AllowFail { get; } + /// Whether the fail should be allowed to proceed. Return false to block. + bool PerformFail(); /// - /// Whether we want to restart on fail. Only used if is true. + /// Whether we want to restart on fail. Only used if returns true. /// bool RestartOnFail { get; } } diff --git a/osu.Game/Rulesets/Mods/ModAutoplay.cs b/osu.Game/Rulesets/Mods/ModAutoplay.cs index e51b8b6457..945dd444be 100644 --- a/osu.Game/Rulesets/Mods/ModAutoplay.cs +++ b/osu.Game/Rulesets/Mods/ModAutoplay.cs @@ -27,7 +27,8 @@ namespace osu.Game.Rulesets.Mods public override string Description => "Watch a perfect automated play through the song."; public override double ScoreMultiplier => 1; - public bool AllowFail => false; + public bool PerformFail() => false; + public bool RestartOnFail => false; public override Type[] IncompatibleMods => new[] { typeof(ModRelax), typeof(ModSuddenDeath), typeof(ModNoFail) }; diff --git a/osu.Game/Rulesets/Mods/ModBlockFail.cs b/osu.Game/Rulesets/Mods/ModBlockFail.cs index 7d7ecfa416..1fde5abad4 100644 --- a/osu.Game/Rulesets/Mods/ModBlockFail.cs +++ b/osu.Game/Rulesets/Mods/ModBlockFail.cs @@ -14,7 +14,7 @@ namespace osu.Game.Rulesets.Mods /// /// We never fail, 'yo. /// - public bool AllowFail => false; + public bool PerformFail() => false; public bool RestartOnFail => false; diff --git a/osu.Game/Rulesets/Mods/ModEasy.cs b/osu.Game/Rulesets/Mods/ModEasy.cs index c1c4124b98..7cf9656810 100644 --- a/osu.Game/Rulesets/Mods/ModEasy.cs +++ b/osu.Game/Rulesets/Mods/ModEasy.cs @@ -48,17 +48,14 @@ namespace osu.Game.Rulesets.Mods retries = Retries.Value; } - public bool AllowFail + public bool PerformFail() { - get - { - if (retries == 0) return true; + if (retries == 0) return true; - health.Value = health.MaxValue; - retries--; + health.Value = health.MaxValue; + retries--; - return false; - } + return false; } public bool RestartOnFail => false; diff --git a/osu.Game/Rulesets/Mods/ModSuddenDeath.cs b/osu.Game/Rulesets/Mods/ModSuddenDeath.cs index 8799431f1d..df10262845 100644 --- a/osu.Game/Rulesets/Mods/ModSuddenDeath.cs +++ b/osu.Game/Rulesets/Mods/ModSuddenDeath.cs @@ -20,7 +20,8 @@ namespace osu.Game.Rulesets.Mods public override bool Ranked => true; public override Type[] IncompatibleMods => new[] { typeof(ModNoFail), typeof(ModRelax), typeof(ModAutoplay) }; - public bool AllowFail => true; + public bool PerformFail() => true; + public bool RestartOnFail => true; public void ApplyToHealthProcessor(HealthProcessor healthProcessor) diff --git a/osu.Game/Screens/Play/Player.cs b/osu.Game/Screens/Play/Player.cs index a2735c8c55..1ec3a69b24 100644 --- a/osu.Game/Screens/Play/Player.cs +++ b/osu.Game/Screens/Play/Player.cs @@ -105,7 +105,7 @@ namespace osu.Game.Screens.Play /// Whether failing should be allowed. /// By default, this checks whether all selected mods allow failing. /// - protected virtual bool AllowFail => Mods.Value.OfType().All(m => m.AllowFail); + protected virtual bool CheckModsAllowFailure() => Mods.Value.OfType().All(m => m.PerformFail()); private readonly bool allowPause; private readonly bool showResults; @@ -485,7 +485,7 @@ namespace osu.Game.Screens.Play private bool onFail() { - if (!AllowFail) + if (!CheckModsAllowFailure()) return false; HasFailed = true; diff --git a/osu.Game/Screens/Play/ReplayPlayer.cs b/osu.Game/Screens/Play/ReplayPlayer.cs index 0d2ddb7b01..f0c76163f1 100644 --- a/osu.Game/Screens/Play/ReplayPlayer.cs +++ b/osu.Game/Screens/Play/ReplayPlayer.cs @@ -12,7 +12,7 @@ namespace osu.Game.Screens.Play private readonly Score score; // Disallow replays from failing. (see https://github.com/ppy/osu/issues/6108) - protected override bool AllowFail => false; + protected override bool CheckModsAllowFailure() => false; public ReplayPlayer(Score score, bool allowPause = true, bool showResults = true) : base(allowPause, showResults) diff --git a/osu.Game/Tests/Visual/ModPerfectTestScene.cs b/osu.Game/Tests/Visual/ModPerfectTestScene.cs index 5948283428..c16352bead 100644 --- a/osu.Game/Tests/Visual/ModPerfectTestScene.cs +++ b/osu.Game/Tests/Visual/ModPerfectTestScene.cs @@ -41,7 +41,7 @@ namespace osu.Game.Tests.Visual { } - protected override bool AllowFail => true; + protected override bool CheckModsAllowFailure() => false; public bool CheckFailed(bool failed) { diff --git a/osu.Game/Tests/Visual/ModTestScene.cs b/osu.Game/Tests/Visual/ModTestScene.cs index 8b41fb5075..b5b3084097 100644 --- a/osu.Game/Tests/Visual/ModTestScene.cs +++ b/osu.Game/Tests/Visual/ModTestScene.cs @@ -64,12 +64,14 @@ namespace osu.Game.Tests.Visual protected class ModTestPlayer : TestPlayer { - protected override bool AllowFail { get; } + private readonly bool allowFail; + + protected override bool CheckModsAllowFailure() => allowFail; public ModTestPlayer(bool allowFail) : base(false, false) { - AllowFail = allowFail; + this.allowFail = allowFail; } } From 44319c1b719c64468de0594e22c48c357c3c62ac Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Tue, 12 May 2020 20:26:34 +0900 Subject: [PATCH 2/9] Commit missed change --- osu.Game/Tests/Visual/ModPerfectTestScene.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/osu.Game/Tests/Visual/ModPerfectTestScene.cs b/osu.Game/Tests/Visual/ModPerfectTestScene.cs index c16352bead..95a62bbf65 100644 --- a/osu.Game/Tests/Visual/ModPerfectTestScene.cs +++ b/osu.Game/Tests/Visual/ModPerfectTestScene.cs @@ -41,7 +41,7 @@ namespace osu.Game.Tests.Visual { } - protected override bool CheckModsAllowFailure() => false; + protected override bool CheckModsAllowFailure() => true; public bool CheckFailed(bool failed) { From ef8375b442d78dea8737ef91b6bb5b3ecbf6e7ee Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Thu, 14 May 2020 22:42:42 +0900 Subject: [PATCH 3/9] Add protection against migrating to a nested folder --- .../NonVisual/CustomDataDirectoryTest.cs | 24 +++++++++++++++++++ osu.Game/IO/OsuStorage.cs | 6 +++++ 2 files changed, 30 insertions(+) diff --git a/osu.Game.Tests/NonVisual/CustomDataDirectoryTest.cs b/osu.Game.Tests/NonVisual/CustomDataDirectoryTest.cs index ef2b20de64..433067ffdd 100644 --- a/osu.Game.Tests/NonVisual/CustomDataDirectoryTest.cs +++ b/osu.Game.Tests/NonVisual/CustomDataDirectoryTest.cs @@ -220,6 +220,30 @@ namespace osu.Game.Tests.NonVisual } } + [Test] + public void TestMigrationToNestedTargetFails() + { + using (HeadlessGameHost host = new CleanRunHeadlessGameHost(nameof(TestMigrationToSameTargetFails))) + { + try + { + var osu = loadOsu(host); + + Assert.DoesNotThrow(() => osu.Migrate(customPath)); + + string subFolder = Path.Combine(customPath, "sub"); + + Directory.CreateDirectory(subFolder); + + Assert.Throws(() => osu.Migrate(subFolder)); + } + finally + { + host.Exit(); + } + } + } + private OsuGameBase loadOsu(GameHost host) { var osu = new OsuGameBase(); diff --git a/osu.Game/IO/OsuStorage.cs b/osu.Game/IO/OsuStorage.cs index 71b01ce479..7c0af16a63 100644 --- a/osu.Game/IO/OsuStorage.cs +++ b/osu.Game/IO/OsuStorage.cs @@ -48,6 +48,12 @@ namespace osu.Game.IO var source = new DirectoryInfo(GetFullPath(".")); var destination = new DirectoryInfo(newLocation); + if (source.FullName == destination.FullName) + throw new ArgumentException("Destination provided is already the current location", nameof(newLocation)); + + if (destination.FullName.Contains(source.FullName)) + throw new ArgumentException("Destination provided is inside the source", nameof(newLocation)); + // ensure the new location has no files present, else hard abort if (destination.Exists) { From 827d75b152f17af984b9823dc4322a185fdaaa55 Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Thu, 14 May 2020 22:44:27 +0900 Subject: [PATCH 4/9] Revert "Add protection against migrating to a nested folder" This reverts commit ef8375b442d78dea8737ef91b6bb5b3ecbf6e7ee. --- .../NonVisual/CustomDataDirectoryTest.cs | 24 ------------------- osu.Game/IO/OsuStorage.cs | 6 ----- 2 files changed, 30 deletions(-) diff --git a/osu.Game.Tests/NonVisual/CustomDataDirectoryTest.cs b/osu.Game.Tests/NonVisual/CustomDataDirectoryTest.cs index 433067ffdd..ef2b20de64 100644 --- a/osu.Game.Tests/NonVisual/CustomDataDirectoryTest.cs +++ b/osu.Game.Tests/NonVisual/CustomDataDirectoryTest.cs @@ -220,30 +220,6 @@ namespace osu.Game.Tests.NonVisual } } - [Test] - public void TestMigrationToNestedTargetFails() - { - using (HeadlessGameHost host = new CleanRunHeadlessGameHost(nameof(TestMigrationToSameTargetFails))) - { - try - { - var osu = loadOsu(host); - - Assert.DoesNotThrow(() => osu.Migrate(customPath)); - - string subFolder = Path.Combine(customPath, "sub"); - - Directory.CreateDirectory(subFolder); - - Assert.Throws(() => osu.Migrate(subFolder)); - } - finally - { - host.Exit(); - } - } - } - private OsuGameBase loadOsu(GameHost host) { var osu = new OsuGameBase(); diff --git a/osu.Game/IO/OsuStorage.cs b/osu.Game/IO/OsuStorage.cs index 7c0af16a63..71b01ce479 100644 --- a/osu.Game/IO/OsuStorage.cs +++ b/osu.Game/IO/OsuStorage.cs @@ -48,12 +48,6 @@ namespace osu.Game.IO var source = new DirectoryInfo(GetFullPath(".")); var destination = new DirectoryInfo(newLocation); - if (source.FullName == destination.FullName) - throw new ArgumentException("Destination provided is already the current location", nameof(newLocation)); - - if (destination.FullName.Contains(source.FullName)) - throw new ArgumentException("Destination provided is inside the source", nameof(newLocation)); - // ensure the new location has no files present, else hard abort if (destination.Exists) { From 25bbb02999481de9adecaa38f51e3e6293018bcd Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Wed, 13 May 2020 22:57:41 +0900 Subject: [PATCH 5/9] Throw better exceptions from OsuStorage --- osu.Game/IO/OsuStorage.cs | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/osu.Game/IO/OsuStorage.cs b/osu.Game/IO/OsuStorage.cs index 71b01ce479..8109631ef9 100644 --- a/osu.Game/IO/OsuStorage.cs +++ b/osu.Game/IO/OsuStorage.cs @@ -48,11 +48,14 @@ namespace osu.Game.IO var source = new DirectoryInfo(GetFullPath(".")); var destination = new DirectoryInfo(newLocation); + if (source.FullName == destination.FullName) + throw new ArgumentException("Destination provided is already the current location", nameof(newLocation)); + // ensure the new location has no files present, else hard abort if (destination.Exists) { if (destination.GetFiles().Length > 0 || destination.GetDirectories().Length > 0) - throw new InvalidOperationException("Migration destination already has files present"); + throw new ArgumentException("Destination provided already has files or directories present", nameof(newLocation)); deleteRecursive(destination); } From 19f117ae53fe3058ed251bd687a7cb8266e75cf8 Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Thu, 14 May 2020 20:18:57 +0900 Subject: [PATCH 6/9] Update test logic for new exception type --- osu.Game.Tests/NonVisual/CustomDataDirectoryTest.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/osu.Game.Tests/NonVisual/CustomDataDirectoryTest.cs b/osu.Game.Tests/NonVisual/CustomDataDirectoryTest.cs index ef2b20de64..7a20bd364b 100644 --- a/osu.Game.Tests/NonVisual/CustomDataDirectoryTest.cs +++ b/osu.Game.Tests/NonVisual/CustomDataDirectoryTest.cs @@ -211,7 +211,7 @@ namespace osu.Game.Tests.NonVisual var osu = loadOsu(host); Assert.DoesNotThrow(() => osu.Migrate(customPath)); - Assert.Throws(() => osu.Migrate(customPath)); + Assert.Throws(() => osu.Migrate(customPath)); } finally { From 0690d81bbb1bc7ee969b7e07050a565138b5b5c0 Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Thu, 14 May 2020 22:42:42 +0900 Subject: [PATCH 7/9] Add protection against migrating to a nested folder --- .../NonVisual/CustomDataDirectoryTest.cs | 48 +++++++++++++++++++ osu.Game/IO/OsuStorage.cs | 9 +++- 2 files changed, 56 insertions(+), 1 deletion(-) diff --git a/osu.Game.Tests/NonVisual/CustomDataDirectoryTest.cs b/osu.Game.Tests/NonVisual/CustomDataDirectoryTest.cs index 7a20bd364b..2a98a6dbc6 100644 --- a/osu.Game.Tests/NonVisual/CustomDataDirectoryTest.cs +++ b/osu.Game.Tests/NonVisual/CustomDataDirectoryTest.cs @@ -220,6 +220,54 @@ namespace osu.Game.Tests.NonVisual } } + [Test] + public void TestMigrationToNestedTargetFails() + { + using (HeadlessGameHost host = new CleanRunHeadlessGameHost(nameof(TestMigrationToSameTargetFails))) + { + try + { + var osu = loadOsu(host); + + Assert.DoesNotThrow(() => osu.Migrate(customPath)); + + string subFolder = Path.Combine(customPath, "sub"); + + Directory.CreateDirectory(subFolder); + + Assert.Throws(() => osu.Migrate(subFolder)); + } + finally + { + host.Exit(); + } + } + } + + [Test] + public void TestMigrationToSeeminglyNestedTarget() + { + using (HeadlessGameHost host = new CleanRunHeadlessGameHost(nameof(TestMigrationToSeeminglyNestedTarget))) + { + try + { + var osu = loadOsu(host); + + Assert.DoesNotThrow(() => osu.Migrate(customPath)); + + string subFolder = customPath + "sub"; + + Directory.CreateDirectory(subFolder); + + osu.Migrate(subFolder); + } + finally + { + host.Exit(); + } + } + } + private OsuGameBase loadOsu(GameHost host) { var osu = new OsuGameBase(); diff --git a/osu.Game/IO/OsuStorage.cs b/osu.Game/IO/OsuStorage.cs index 8109631ef9..ac28a05375 100644 --- a/osu.Game/IO/OsuStorage.cs +++ b/osu.Game/IO/OsuStorage.cs @@ -48,9 +48,16 @@ namespace osu.Game.IO var source = new DirectoryInfo(GetFullPath(".")); var destination = new DirectoryInfo(newLocation); - if (source.FullName == destination.FullName) + // using Uri is the easiest way to check equality and contains (https://stackoverflow.com/a/7710620) + var sourceUri = new Uri(source.FullName + Path.DirectorySeparatorChar); + var destinationUri = new Uri(destination.FullName + Path.DirectorySeparatorChar); + + if (sourceUri == destinationUri) throw new ArgumentException("Destination provided is already the current location", nameof(newLocation)); + if (sourceUri.IsBaseOf(destinationUri)) + throw new ArgumentException("Destination provided is inside the source", nameof(newLocation)); + // ensure the new location has no files present, else hard abort if (destination.Exists) { From 7641507c90f770bab9e096dad63132001cc532d5 Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Fri, 15 May 2020 10:45:57 +0900 Subject: [PATCH 8/9] Ensure test directories are deleted before subsequent run --- osu.Game.Tests/NonVisual/CustomDataDirectoryTest.cs | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-) diff --git a/osu.Game.Tests/NonVisual/CustomDataDirectoryTest.cs b/osu.Game.Tests/NonVisual/CustomDataDirectoryTest.cs index 2a98a6dbc6..d69bf94ee2 100644 --- a/osu.Game.Tests/NonVisual/CustomDataDirectoryTest.cs +++ b/osu.Game.Tests/NonVisual/CustomDataDirectoryTest.cs @@ -233,6 +233,9 @@ namespace osu.Game.Tests.NonVisual string subFolder = Path.Combine(customPath, "sub"); + if (Directory.Exists(subFolder)) + Directory.Delete(subFolder, true); + Directory.CreateDirectory(subFolder); Assert.Throws(() => osu.Migrate(subFolder)); @@ -255,11 +258,14 @@ namespace osu.Game.Tests.NonVisual Assert.DoesNotThrow(() => osu.Migrate(customPath)); - string subFolder = customPath + "sub"; + string seeminglySubFolder = customPath + "sub"; - Directory.CreateDirectory(subFolder); + if (Directory.Exists(seeminglySubFolder)) + Directory.Delete(seeminglySubFolder, true); - osu.Migrate(subFolder); + Directory.CreateDirectory(seeminglySubFolder); + + osu.Migrate(seeminglySubFolder); } finally { From aea192080a51f014d79da5caad8ce70507d65a74 Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Fri, 15 May 2020 13:02:46 +0900 Subject: [PATCH 9/9] Fix incorrect storage name --- osu.Game.Tests/NonVisual/CustomDataDirectoryTest.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/osu.Game.Tests/NonVisual/CustomDataDirectoryTest.cs b/osu.Game.Tests/NonVisual/CustomDataDirectoryTest.cs index d69bf94ee2..743c924bbd 100644 --- a/osu.Game.Tests/NonVisual/CustomDataDirectoryTest.cs +++ b/osu.Game.Tests/NonVisual/CustomDataDirectoryTest.cs @@ -223,7 +223,7 @@ namespace osu.Game.Tests.NonVisual [Test] public void TestMigrationToNestedTargetFails() { - using (HeadlessGameHost host = new CleanRunHeadlessGameHost(nameof(TestMigrationToSameTargetFails))) + using (HeadlessGameHost host = new CleanRunHeadlessGameHost(nameof(TestMigrationToNestedTargetFails))) { try {