From bd1f38cc3ef41c0ca8bbd586c81b1cbf8b6a6d9f Mon Sep 17 00:00:00 2001 From: smoogipoo Date: Fri, 26 Jun 2020 23:21:44 +0900 Subject: [PATCH 1/4] Fix crash due to unsafe mod deserialisation --- .../Online/TestAPIModSerialization.cs | 59 ++++++++++++++++++- osu.Game/Rulesets/Mods/ModTimeRamp.cs | 4 +- 2 files changed, 60 insertions(+), 3 deletions(-) diff --git a/osu.Game.Tests/Online/TestAPIModSerialization.cs b/osu.Game.Tests/Online/TestAPIModSerialization.cs index d9318aa822..5948582d77 100644 --- a/osu.Game.Tests/Online/TestAPIModSerialization.cs +++ b/osu.Game.Tests/Online/TestAPIModSerialization.cs @@ -49,9 +49,32 @@ namespace osu.Game.Tests.Online Assert.That(converted.TestSetting.Value, Is.EqualTo(2)); } + [Test] + public void TestDeserialiseTimeRampMod() + { + // Create the mod with values different from default. + var apiMod = new APIMod(new TestModTimeRamp + { + AdjustPitch = { Value = false }, + InitialRate = { Value = 1.25 }, + FinalRate = { Value = 0.25 } + }); + + var deserialised = JsonConvert.DeserializeObject(JsonConvert.SerializeObject(apiMod)); + var converted = (TestModTimeRamp)deserialised.ToMod(new TestRuleset()); + + Assert.That(converted.AdjustPitch.Value, Is.EqualTo(false)); + Assert.That(converted.InitialRate.Value, Is.EqualTo(1.25)); + Assert.That(converted.FinalRate.Value, Is.EqualTo(0.25)); + } + private class TestRuleset : Ruleset { - public override IEnumerable GetModsFor(ModType type) => new[] { new TestMod() }; + public override IEnumerable GetModsFor(ModType type) => new Mod[] + { + new TestMod(), + new TestModTimeRamp(), + }; public override DrawableRuleset CreateDrawableRulesetWith(IBeatmap beatmap, IReadOnlyList mods = null) => throw new System.NotImplementedException(); @@ -78,5 +101,39 @@ namespace osu.Game.Tests.Online Precision = 0.01, }; } + + private class TestModTimeRamp : ModTimeRamp + { + public override string Name => "Test Mod"; + public override string Acronym => "TMTR"; + public override double ScoreMultiplier => 1; + + [SettingSource("Initial rate", "The starting speed of the track")] + public override BindableNumber InitialRate { get; } = new BindableDouble + { + MinValue = 1, + MaxValue = 2, + Default = 1.5, + Value = 1.5, + Precision = 0.01, + }; + + [SettingSource("Final rate", "The speed increase to ramp towards")] + public override BindableNumber FinalRate { get; } = new BindableDouble + { + MinValue = 0, + MaxValue = 1, + Default = 0.5, + Value = 0.5, + Precision = 0.01, + }; + + [SettingSource("Adjust pitch", "Should pitch be adjusted with speed")] + public override BindableBool AdjustPitch { get; } = new BindableBool + { + Default = true, + Value = true + }; + } } } diff --git a/osu.Game/Rulesets/Mods/ModTimeRamp.cs b/osu.Game/Rulesets/Mods/ModTimeRamp.cs index cbd07efa97..839d97f04e 100644 --- a/osu.Game/Rulesets/Mods/ModTimeRamp.cs +++ b/osu.Game/Rulesets/Mods/ModTimeRamp.cs @@ -89,9 +89,9 @@ namespace osu.Game.Rulesets.Mods private void applyPitchAdjustment(ValueChangedEvent adjustPitchSetting) { // remove existing old adjustment - track.RemoveAdjustment(adjustmentForPitchSetting(adjustPitchSetting.OldValue), SpeedChange); + track?.RemoveAdjustment(adjustmentForPitchSetting(adjustPitchSetting.OldValue), SpeedChange); - track.AddAdjustment(adjustmentForPitchSetting(adjustPitchSetting.NewValue), SpeedChange); + track?.AddAdjustment(adjustmentForPitchSetting(adjustPitchSetting.NewValue), SpeedChange); } private AdjustableProperty adjustmentForPitchSetting(bool adjustPitchSettingValue) From c233dc476800e7df39ee242a7515cc6bc8beb5e9 Mon Sep 17 00:00:00 2001 From: smoogipoo Date: Sat, 27 Jun 2020 00:16:16 +0900 Subject: [PATCH 2/4] Add some global error handling --- osu.Game/Screens/Multi/RoomManager.cs | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/osu.Game/Screens/Multi/RoomManager.cs b/osu.Game/Screens/Multi/RoomManager.cs index 4d6ac46c84..b8c969a845 100644 --- a/osu.Game/Screens/Multi/RoomManager.cs +++ b/osu.Game/Screens/Multi/RoomManager.cs @@ -166,8 +166,16 @@ namespace osu.Game.Screens.Multi var r = listing[i]; r.Position.Value = i; - update(r, r); - addRoom(r); + try + { + update(r, r); + addRoom(r); + } + catch (Exception ex) + { + Logger.Error(ex, $"Failed to update room: {r.Name.Value}."); + rooms.Remove(r); + } } RoomsUpdated?.Invoke(); From e8d36bc3cbb5c6aa2c0f0fd6dde7d18d34799590 Mon Sep 17 00:00:00 2001 From: smoogipoo Date: Sat, 27 Jun 2020 00:19:22 +0900 Subject: [PATCH 3/4] Don't trigger the same exception multiple times --- osu.Game/Screens/Multi/RoomManager.cs | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/osu.Game/Screens/Multi/RoomManager.cs b/osu.Game/Screens/Multi/RoomManager.cs index b8c969a845..5083fb2ee3 100644 --- a/osu.Game/Screens/Multi/RoomManager.cs +++ b/osu.Game/Screens/Multi/RoomManager.cs @@ -3,6 +3,7 @@ using System; using System.Collections.Generic; +using System.Diagnostics; using System.Linq; using System.Threading.Tasks; using osu.Framework.Allocation; @@ -142,6 +143,8 @@ namespace osu.Game.Screens.Multi joinedRoom = null; } + private readonly List roomsFailedUpdate = new List(); + /// /// Invoked when the listing of all s is received from the server. /// @@ -173,7 +176,14 @@ namespace osu.Game.Screens.Multi } catch (Exception ex) { - Logger.Error(ex, $"Failed to update room: {r.Name.Value}."); + Debug.Assert(r.RoomID.Value != null); + + if (!roomsFailedUpdate.Contains(r.RoomID.Value.Value)) + { + Logger.Error(ex, $"Failed to update room: {r.Name.Value}."); + roomsFailedUpdate.Add(r.RoomID.Value.Value); + } + rooms.Remove(r); } } From 006adf0fb50a903c67c7317b3c721ff653615506 Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Sun, 28 Jun 2020 22:45:13 +0900 Subject: [PATCH 4/4] Change logic to ignore rooms completely after first error --- osu.Game/Screens/Multi/RoomManager.cs | 27 ++++++++++++++------------- 1 file changed, 14 insertions(+), 13 deletions(-) diff --git a/osu.Game/Screens/Multi/RoomManager.cs b/osu.Game/Screens/Multi/RoomManager.cs index 5083fb2ee3..642378d8d5 100644 --- a/osu.Game/Screens/Multi/RoomManager.cs +++ b/osu.Game/Screens/Multi/RoomManager.cs @@ -143,7 +143,7 @@ namespace osu.Game.Screens.Multi joinedRoom = null; } - private readonly List roomsFailedUpdate = new List(); + private readonly HashSet ignoredRooms = new HashSet(); /// /// Invoked when the listing of all s is received from the server. @@ -166,25 +166,26 @@ namespace osu.Game.Screens.Multi continue; } - var r = listing[i]; - r.Position.Value = i; + var room = listing[i]; + + Debug.Assert(room.RoomID.Value != null); + + if (ignoredRooms.Contains(room.RoomID.Value.Value)) + continue; + + room.Position.Value = i; try { - update(r, r); - addRoom(r); + update(room, room); + addRoom(room); } catch (Exception ex) { - Debug.Assert(r.RoomID.Value != null); + Logger.Error(ex, $"Failed to update room: {room.Name.Value}."); - if (!roomsFailedUpdate.Contains(r.RoomID.Value.Value)) - { - Logger.Error(ex, $"Failed to update room: {r.Name.Value}."); - roomsFailedUpdate.Add(r.RoomID.Value.Value); - } - - rooms.Remove(r); + ignoredRooms.Add(room.RoomID.Value.Value); + rooms.Remove(room); } }