From 0aaa62efc2d9f2d1e8f6d46632661b0e6ef8db03 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bart=C5=82omiej=20Dach?= Date: Sat, 30 Jan 2021 20:55:56 +0100 Subject: [PATCH 01/18] Add failing test case --- .../NonVisual/OngoingOperationTrackerTest.cs | 44 +++++++++++++++++++ 1 file changed, 44 insertions(+) diff --git a/osu.Game.Tests/NonVisual/OngoingOperationTrackerTest.cs b/osu.Game.Tests/NonVisual/OngoingOperationTrackerTest.cs index eef9582af9..10216c3339 100644 --- a/osu.Game.Tests/NonVisual/OngoingOperationTrackerTest.cs +++ b/osu.Game.Tests/NonVisual/OngoingOperationTrackerTest.cs @@ -3,8 +3,12 @@ using System; using NUnit.Framework; +using osu.Framework.Allocation; using osu.Framework.Bindables; +using osu.Framework.Graphics; +using osu.Framework.Screens; using osu.Framework.Testing; +using osu.Game.Screens; using osu.Game.Screens.OnlinePlay; using osu.Game.Tests.Visual; @@ -58,5 +62,45 @@ namespace osu.Game.Tests.NonVisual AddStep("end operation", () => operation.Dispose()); AddAssert("operation is ended", () => !operationInProgress.Value); } + + [Test] + public void TestOperationDisposalAfterScreenExit() + { + TestScreenWithTracker screen = null; + OsuScreenStack stack; + IDisposable operation = null; + + AddStep("create screen with tracker", () => + { + Child = stack = new OsuScreenStack + { + RelativeSizeAxes = Axes.Both + }; + + stack.Push(screen = new TestScreenWithTracker()); + }); + AddUntilStep("wait for loaded", () => screen.IsLoaded); + + AddStep("begin operation", () => operation = screen.OngoingOperationTracker.BeginOperation()); + AddAssert("operation in progress", () => screen.OngoingOperationTracker.InProgress.Value); + + AddStep("dispose after screen exit", () => + { + screen.Exit(); + operation.Dispose(); + }); + AddAssert("operation ended", () => !screen.OngoingOperationTracker.InProgress.Value); + } + + private class TestScreenWithTracker : OsuScreen + { + public OngoingOperationTracker OngoingOperationTracker { get; private set; } + + [BackgroundDependencyLoader] + private void load() + { + InternalChild = OngoingOperationTracker = new OngoingOperationTracker(); + } + } } } From 96f56d1c942505797f20cf1c166b5452c60ea592 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bart=C5=82omiej=20Dach?= Date: Sat, 30 Jan 2021 21:00:13 +0100 Subject: [PATCH 02/18] Return tracker lease via UnbindAll() Improves reliability by being fail-safe in case of multiple returns, which can happen if the operation tracker is part of a screen being exited (as is the case with its current primary usage in multiplayer). --- .../Screens/OnlinePlay/OngoingOperationTracker.cs | 15 +++++---------- 1 file changed, 5 insertions(+), 10 deletions(-) diff --git a/osu.Game/Screens/OnlinePlay/OngoingOperationTracker.cs b/osu.Game/Screens/OnlinePlay/OngoingOperationTracker.cs index b7ee84eb9e..9e88fabb3d 100644 --- a/osu.Game/Screens/OnlinePlay/OngoingOperationTracker.cs +++ b/osu.Game/Screens/OnlinePlay/OngoingOperationTracker.cs @@ -53,20 +53,15 @@ namespace osu.Game.Screens.OnlinePlay // for extra safety, marshal the end of operation back to the update thread if necessary. Scheduler.Add(() => { - leasedInProgress?.Return(); + // UnbindAll() is purposefully used instead of Return() - the two do roughly the same thing, with one difference: + // the former won't throw if the lease has already been returned before. + // this matters because framework can unbind the lease via the internal UnbindAllBindables(), which is not always detectable + // (it is in the case of disposal, but not in the case of screen exit - at least not cleanly). + leasedInProgress?.UnbindAll(); leasedInProgress = null; }, false); } - protected override void Dispose(bool isDisposing) - { - base.Dispose(isDisposing); - - // base call does an UnbindAllBindables(). - // clean up the leased reference here so that it doesn't get returned twice. - leasedInProgress = null; - } - private class OngoingOperation : IDisposable { private readonly OngoingOperationTracker tracker; From 5f320cd4264ad444fe5cb3f7aa7bef74d530e932 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bart=C5=82omiej=20Dach?= Date: Sat, 30 Jan 2021 21:03:09 +0100 Subject: [PATCH 03/18] Move lease check inside schedule Theoretically safer due to avoiding a potential data race (change in `leasedInProgress` between the time of the check and start of schedule execution). --- osu.Game/Screens/OnlinePlay/OngoingOperationTracker.cs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/osu.Game/Screens/OnlinePlay/OngoingOperationTracker.cs b/osu.Game/Screens/OnlinePlay/OngoingOperationTracker.cs index 9e88fabb3d..aabeafe460 100644 --- a/osu.Game/Screens/OnlinePlay/OngoingOperationTracker.cs +++ b/osu.Game/Screens/OnlinePlay/OngoingOperationTracker.cs @@ -47,12 +47,12 @@ namespace osu.Game.Screens.OnlinePlay private void endOperationWithKnownLease(LeasedBindable lease) { - if (lease != leasedInProgress) - return; - // for extra safety, marshal the end of operation back to the update thread if necessary. Scheduler.Add(() => { + if (lease != leasedInProgress) + return; + // UnbindAll() is purposefully used instead of Return() - the two do roughly the same thing, with one difference: // the former won't throw if the lease has already been returned before. // this matters because framework can unbind the lease via the internal UnbindAllBindables(), which is not always detectable From ead8262257ed0749b9b81670aa76b534aa73874f Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Mon, 1 Feb 2021 20:20:10 +0900 Subject: [PATCH 04/18] Add function to check for (and return) invalid mods --- osu.Game/Utils/ModUtils.cs | 42 ++++++++++++++++++++++++++++++++++++++ 1 file changed, 42 insertions(+) diff --git a/osu.Game/Utils/ModUtils.cs b/osu.Game/Utils/ModUtils.cs index 808dba2900..9e638d4f2f 100644 --- a/osu.Game/Utils/ModUtils.cs +++ b/osu.Game/Utils/ModUtils.cs @@ -83,6 +83,48 @@ namespace osu.Game.Utils .All(m => allowedSet.Contains(m.GetType())); } + /// + /// Check the provided combination of mods are valid for a local gameplay session. + /// + /// The mods to check. + /// Invalid mods, if any where found. Can be null if all mods were valid. + /// Whether the input mods were all valid. If false, will contain all invalid entries. + public static bool CheckValidForGameplay(IEnumerable mods, out Mod[]? invalidMods) + { + mods = mods.ToArray(); + + List? foundInvalid = null; + + void addInvalid(Mod mod) + { + foundInvalid ??= new List(); + foundInvalid.Add(mod); + } + + foreach (var mod in mods) + { + bool valid = mod.Type != ModType.System + && mod.HasImplementation + && !(mod is MultiMod); + + if (!valid) + { + // if this mod was found as invalid, we can exclude it before potentially excluding more incompatible types. + addInvalid(mod); + continue; + } + + foreach (var type in mod.IncompatibleMods) + { + foreach (var invalid in mods.Where(m => type.IsInstanceOfType(m))) + addInvalid(invalid); + } + } + + invalidMods = foundInvalid?.ToArray(); + return foundInvalid == null; + } + /// /// Flattens a set of s, returning a new set with all s removed. /// From 425dc8a210d69c853c58f4d2a10ce0347914261f Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Mon, 1 Feb 2021 20:20:19 +0900 Subject: [PATCH 05/18] Ensure mods are always in a valid state at a game level --- osu.Game/OsuGame.cs | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/osu.Game/OsuGame.cs b/osu.Game/OsuGame.cs index 5acd6bc73d..a00cd5e6a0 100644 --- a/osu.Game/OsuGame.cs +++ b/osu.Game/OsuGame.cs @@ -468,6 +468,12 @@ namespace osu.Game private void modsChanged(ValueChangedEvent> mods) { updateModDefaults(); + + if (!ModUtils.CheckValidForGameplay(mods.NewValue, out var invalid)) + { + // ensure we always have a valid set of mods. + SelectedMods.Value = mods.NewValue.Except(invalid).ToArray(); + } } private void updateModDefaults() From 80d88024d6bbca6db1f9ca2765340bb765f5ae07 Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Tue, 2 Feb 2021 14:13:50 +0900 Subject: [PATCH 06/18] Add basic test coverage of CheckValidForGameplay function --- osu.Game.Tests/Mods/ModUtilsTest.cs | 28 ++++++++++++++++++++++++++++ 1 file changed, 28 insertions(+) diff --git a/osu.Game.Tests/Mods/ModUtilsTest.cs b/osu.Game.Tests/Mods/ModUtilsTest.cs index fdb441343a..88eee5449c 100644 --- a/osu.Game.Tests/Mods/ModUtilsTest.cs +++ b/osu.Game.Tests/Mods/ModUtilsTest.cs @@ -1,9 +1,13 @@ // 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.Collections.Generic; +using System.Linq; using Moq; using NUnit.Framework; using osu.Game.Rulesets.Mods; +using osu.Game.Rulesets.Osu.Mods; using osu.Game.Utils; namespace osu.Game.Tests.Mods @@ -60,5 +64,29 @@ namespace osu.Game.Tests.Mods var mod = new Mock(); Assert.That(ModUtils.CheckAllowed(new[] { mod.Object }, new[] { typeof(Mod) }), Is.False); } + + // test incompatible pair. + [TestCase(new[] { typeof(OsuModDoubleTime), typeof(OsuModHalfTime) }, new[] { typeof(OsuModDoubleTime), typeof(OsuModHalfTime) })] + // test incompatible pair with derived class. + [TestCase(new[] { typeof(OsuModNightcore), typeof(OsuModHalfTime) }, new[] { typeof(OsuModNightcore), typeof(OsuModHalfTime) })] + // test system mod. + [TestCase(new[] { typeof(OsuModDoubleTime), typeof(OsuModTouchDevice) }, new[] { typeof(OsuModTouchDevice) })] + // test valid. + [TestCase(new[] { typeof(OsuModDoubleTime), typeof(OsuModHardRock) }, null)] + public void TestInvalidModScenarios(Type[] input, Type[] expectedInvalid) + { + List inputMods = new List(); + foreach (var t in input) + inputMods.Add((Mod)Activator.CreateInstance(t)); + + bool isValid = ModUtils.CheckValidForGameplay(inputMods, out var invalid); + + Assert.That(isValid, Is.EqualTo(expectedInvalid == null)); + + if (isValid) + Assert.IsNull(invalid); + else + Assert.That(invalid?.Select(t => t.GetType()), Is.EquivalentTo(expectedInvalid)); + } } } From 1c645601d41491004c96903231c61ce5a163c7b2 Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Tue, 2 Feb 2021 14:14:31 +0900 Subject: [PATCH 07/18] Fix typo in xmldoc --- 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 9e638d4f2f..2146abacb6 100644 --- a/osu.Game/Utils/ModUtils.cs +++ b/osu.Game/Utils/ModUtils.cs @@ -87,7 +87,7 @@ namespace osu.Game.Utils /// Check the provided combination of mods are valid for a local gameplay session. /// /// The mods to check. - /// Invalid mods, if any where found. Can be null if all mods were valid. + /// Invalid mods, if any were found. Can be null if all mods were valid. /// Whether the input mods were all valid. If false, will contain all invalid entries. public static bool CheckValidForGameplay(IEnumerable mods, out Mod[]? invalidMods) { From fb52ac8c69792e0101c7f612904cb562b3d1e8c2 Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Tue, 2 Feb 2021 16:57:08 +0900 Subject: [PATCH 08/18] Share remove from playlist button design with adjacent download button --- .../Graphics/UserInterface/DownloadButton.cs | 60 +++++++------------ osu.Game/Graphics/UserInterface/GrayButton.cs | 48 +++++++++++++++ .../OnlinePlay/DrawableRoomPlaylistItem.cs | 26 +++++++- 3 files changed, 93 insertions(+), 41 deletions(-) create mode 100644 osu.Game/Graphics/UserInterface/GrayButton.cs diff --git a/osu.Game/Graphics/UserInterface/DownloadButton.cs b/osu.Game/Graphics/UserInterface/DownloadButton.cs index 5168ff646b..7a8db158c1 100644 --- a/osu.Game/Graphics/UserInterface/DownloadButton.cs +++ b/osu.Game/Graphics/UserInterface/DownloadButton.cs @@ -4,54 +4,38 @@ using osu.Framework.Allocation; using osu.Framework.Bindables; using osu.Framework.Graphics; -using osu.Framework.Graphics.Shapes; using osu.Framework.Graphics.Sprites; using osu.Game.Online; using osuTK; namespace osu.Game.Graphics.UserInterface { - public class DownloadButton : OsuAnimatedButton + public class DownloadButton : GrayButton { - public readonly Bindable State = new Bindable(); - - private readonly SpriteIcon icon; - private readonly SpriteIcon checkmark; - private readonly Box background; - [Resolved] private OsuColour colours { get; set; } + public readonly Bindable State = new Bindable(); + + private SpriteIcon checkmark; + public DownloadButton() + : base(FontAwesome.Solid.Download) { - Children = new Drawable[] - { - background = new Box - { - RelativeSizeAxes = Axes.Both, - Depth = float.MaxValue - }, - icon = new SpriteIcon - { - Anchor = Anchor.Centre, - Origin = Anchor.Centre, - Size = new Vector2(13), - Icon = FontAwesome.Solid.Download, - }, - checkmark = new SpriteIcon - { - Anchor = Anchor.Centre, - Origin = Anchor.Centre, - X = 8, - Size = Vector2.Zero, - Icon = FontAwesome.Solid.Check, - } - }; } [BackgroundDependencyLoader] private void load() { + AddInternal(checkmark = new SpriteIcon + { + Anchor = Anchor.Centre, + Origin = Anchor.Centre, + X = 8, + Size = Vector2.Zero, + Icon = FontAwesome.Solid.Check, + }); + State.BindValueChanged(updateState, true); } @@ -60,27 +44,27 @@ namespace osu.Game.Graphics.UserInterface switch (state.NewValue) { case DownloadState.NotDownloaded: - background.FadeColour(colours.Gray4, 500, Easing.InOutExpo); - icon.MoveToX(0, 500, Easing.InOutExpo); + Background.FadeColour(colours.Gray4, 500, Easing.InOutExpo); + Icon.MoveToX(0, 500, Easing.InOutExpo); checkmark.ScaleTo(Vector2.Zero, 500, Easing.InOutExpo); TooltipText = "Download"; break; case DownloadState.Downloading: - background.FadeColour(colours.Blue, 500, Easing.InOutExpo); - icon.MoveToX(0, 500, Easing.InOutExpo); + Background.FadeColour(colours.Blue, 500, Easing.InOutExpo); + Icon.MoveToX(0, 500, Easing.InOutExpo); checkmark.ScaleTo(Vector2.Zero, 500, Easing.InOutExpo); TooltipText = "Downloading..."; break; case DownloadState.Importing: - background.FadeColour(colours.Yellow, 500, Easing.InOutExpo); + Background.FadeColour(colours.Yellow, 500, Easing.InOutExpo); TooltipText = "Importing"; break; case DownloadState.LocallyAvailable: - background.FadeColour(colours.Green, 500, Easing.InOutExpo); - icon.MoveToX(-8, 500, Easing.InOutExpo); + Background.FadeColour(colours.Green, 500, Easing.InOutExpo); + Icon.MoveToX(-8, 500, Easing.InOutExpo); checkmark.ScaleTo(new Vector2(13), 500, Easing.InOutExpo); break; } diff --git a/osu.Game/Graphics/UserInterface/GrayButton.cs b/osu.Game/Graphics/UserInterface/GrayButton.cs new file mode 100644 index 0000000000..dd05701545 --- /dev/null +++ b/osu.Game/Graphics/UserInterface/GrayButton.cs @@ -0,0 +1,48 @@ +// Copyright (c) ppy Pty Ltd . Licensed under the MIT Licence. +// See the LICENCE file in the repository root for full licence text. + +using osu.Framework.Allocation; +using osu.Framework.Graphics; +using osu.Framework.Graphics.Shapes; +using osu.Framework.Graphics.Sprites; +using osuTK; + +namespace osu.Game.Graphics.UserInterface +{ + public class GrayButton : OsuAnimatedButton + { + protected SpriteIcon Icon; + protected Box Background; + + private readonly IconUsage icon; + + [Resolved] + private OsuColour colours { get; set; } + + public GrayButton(IconUsage icon) + { + this.icon = icon; + } + + [BackgroundDependencyLoader] + private void load() + { + Children = new Drawable[] + { + Background = new Box + { + Colour = colours.Gray4, + RelativeSizeAxes = Axes.Both, + Depth = float.MaxValue + }, + Icon = new SpriteIcon + { + Anchor = Anchor.Centre, + Origin = Anchor.Centre, + Size = new Vector2(13), + Icon = icon, + }, + }; + } + } +} diff --git a/osu.Game/Screens/OnlinePlay/DrawableRoomPlaylistItem.cs b/osu.Game/Screens/OnlinePlay/DrawableRoomPlaylistItem.cs index f8982582d5..4316a9508e 100644 --- a/osu.Game/Screens/OnlinePlay/DrawableRoomPlaylistItem.cs +++ b/osu.Game/Screens/OnlinePlay/DrawableRoomPlaylistItem.cs @@ -214,7 +214,8 @@ namespace osu.Game.Screens.OnlinePlay Origin = Anchor.CentreRight, Direction = FillDirection.Horizontal, AutoSizeAxes = Axes.Both, - X = -18, + Spacing = new Vector2(5), + X = -10, ChildrenEnumerable = CreateButtons() } } @@ -225,16 +226,35 @@ namespace osu.Game.Screens.OnlinePlay { new PlaylistDownloadButton(Item) { + Anchor = Anchor.Centre, + Origin = Anchor.Centre, Size = new Vector2(50, 30) }, - new IconButton + new PlaylistRemoveButton { - Icon = FontAwesome.Solid.MinusSquare, + Anchor = Anchor.Centre, + Origin = Anchor.Centre, + Size = new Vector2(30, 30), Alpha = allowEdit ? 1 : 0, Action = () => RequestDeletion?.Invoke(Model), }, }; + public class PlaylistRemoveButton : GrayButton + { + public PlaylistRemoveButton() + : base(FontAwesome.Solid.MinusSquare) + { + TooltipText = "Remove from playlist"; + } + + [BackgroundDependencyLoader] + private void load() + { + Icon.Scale = new Vector2(0.8f); + } + } + protected override bool OnClick(ClickEvent e) { if (allowSelection) From 6d9ac4d0f01bfefccd38b399ca9158bd4415ce23 Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Tue, 2 Feb 2021 16:57:27 +0900 Subject: [PATCH 09/18] Increase darkness of gradient on buttons to make text readability (slightly) better --- .../TestSceneDrawableRoomPlaylist.cs | 17 ++++++++++++++++- .../OnlinePlay/DrawableRoomPlaylistItem.cs | 14 ++++---------- 2 files changed, 20 insertions(+), 11 deletions(-) diff --git a/osu.Game.Tests/Visual/Multiplayer/TestSceneDrawableRoomPlaylist.cs b/osu.Game.Tests/Visual/Multiplayer/TestSceneDrawableRoomPlaylist.cs index 874c1694eb..16f6723e2d 100644 --- a/osu.Game.Tests/Visual/Multiplayer/TestSceneDrawableRoomPlaylist.cs +++ b/osu.Game.Tests/Visual/Multiplayer/TestSceneDrawableRoomPlaylist.cs @@ -20,6 +20,7 @@ using osu.Game.Rulesets.Osu; using osu.Game.Rulesets.Osu.Mods; using osu.Game.Screens.OnlinePlay; using osu.Game.Tests.Beatmaps; +using osu.Game.Users; using osuTK; using osuTK.Input; @@ -278,7 +279,21 @@ namespace osu.Game.Tests.Visual.Multiplayer playlist.Items.Add(new PlaylistItem { ID = i, - Beatmap = { Value = new TestBeatmap(new OsuRuleset().RulesetInfo).BeatmapInfo }, + Beatmap = + { + Value = i % 2 == 1 + ? new TestBeatmap(new OsuRuleset().RulesetInfo).BeatmapInfo + : new BeatmapInfo + { + Metadata = new BeatmapMetadata + { + Artist = "Artist", + Author = new User { Username = "Creator name here" }, + Title = "Long title used to check background colour", + }, + BeatmapSet = new BeatmapSetInfo() + } + }, Ruleset = { Value = new OsuRuleset().RulesetInfo }, RequiredMods = { diff --git a/osu.Game/Screens/OnlinePlay/DrawableRoomPlaylistItem.cs b/osu.Game/Screens/OnlinePlay/DrawableRoomPlaylistItem.cs index 4316a9508e..844758b262 100644 --- a/osu.Game/Screens/OnlinePlay/DrawableRoomPlaylistItem.cs +++ b/osu.Game/Screens/OnlinePlay/DrawableRoomPlaylistItem.cs @@ -342,20 +342,14 @@ namespace osu.Game.Screens.OnlinePlay new Box { RelativeSizeAxes = Axes.Both, - Colour = ColourInfo.GradientHorizontal(Color4.Black, new Color4(0f, 0f, 0f, 0.9f)), - Width = 0.05f, + Colour = ColourInfo.GradientHorizontal(Color4.Black, new Color4(0f, 0f, 0f, 0.7f)), + Width = 0.4f, }, new Box { RelativeSizeAxes = Axes.Both, - Colour = ColourInfo.GradientHorizontal(new Color4(0f, 0f, 0f, 0.9f), new Color4(0f, 0f, 0f, 0.1f)), - Width = 0.2f, - }, - new Box - { - RelativeSizeAxes = Axes.Both, - Colour = ColourInfo.GradientHorizontal(new Color4(0f, 0f, 0f, 0.1f), new Color4(0, 0, 0, 0)), - Width = 0.05f, + Colour = ColourInfo.GradientHorizontal(new Color4(0f, 0f, 0f, 0.7f), new Color4(0, 0, 0, 0.4f)), + Width = 0.4f, }, } } From 40233fb47cba94b9b48eafdf28944036c301e347 Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Tue, 2 Feb 2021 17:09:59 +0900 Subject: [PATCH 10/18] Make font bolder --- .../OnlinePlay/DrawableRoomPlaylistItem.cs | 155 +++++++++--------- 1 file changed, 80 insertions(+), 75 deletions(-) diff --git a/osu.Game/Screens/OnlinePlay/DrawableRoomPlaylistItem.cs b/osu.Game/Screens/OnlinePlay/DrawableRoomPlaylistItem.cs index 844758b262..a7015ba1c4 100644 --- a/osu.Game/Screens/OnlinePlay/DrawableRoomPlaylistItem.cs +++ b/osu.Game/Screens/OnlinePlay/DrawableRoomPlaylistItem.cs @@ -124,102 +124,107 @@ namespace osu.Game.Screens.OnlinePlay modDisplay.Current.Value = requiredMods.ToArray(); } - protected override Drawable CreateContent() => maskingContainer = new Container + protected override Drawable CreateContent() { - RelativeSizeAxes = Axes.X, - Height = 50, - Masking = true, - CornerRadius = 10, - Children = new Drawable[] + Action fontParameters = s => s.Font = OsuFont.Default.With(weight: FontWeight.SemiBold); + + return maskingContainer = new Container { - new Box // A transparent box that forces the border to be drawn if the panel background is opaque + RelativeSizeAxes = Axes.X, + Height = 50, + Masking = true, + CornerRadius = 10, + Children = new Drawable[] { - RelativeSizeAxes = Axes.Both, - Alpha = 0, - AlwaysPresent = true - }, - new PanelBackground - { - RelativeSizeAxes = Axes.Both, - Beatmap = { BindTarget = beatmap } - }, - new FillFlowContainer - { - RelativeSizeAxes = Axes.Both, - Padding = new MarginPadding { Left = 8 }, - Spacing = new Vector2(8, 0), - Direction = FillDirection.Horizontal, - Children = new Drawable[] + new Box // A transparent box that forces the border to be drawn if the panel background is opaque { - difficultyIconContainer = new Container + RelativeSizeAxes = Axes.Both, + Alpha = 0, + AlwaysPresent = true + }, + new PanelBackground + { + RelativeSizeAxes = Axes.Both, + Beatmap = { BindTarget = beatmap } + }, + new FillFlowContainer + { + RelativeSizeAxes = Axes.Both, + Padding = new MarginPadding { Left = 8 }, + Spacing = new Vector2(8, 0), + Direction = FillDirection.Horizontal, + Children = new Drawable[] { - Anchor = Anchor.CentreLeft, - Origin = Anchor.CentreLeft, - AutoSizeAxes = Axes.Both, - }, - new FillFlowContainer - { - Anchor = Anchor.CentreLeft, - Origin = Anchor.CentreLeft, - AutoSizeAxes = Axes.Both, - Direction = FillDirection.Vertical, - Children = new Drawable[] + difficultyIconContainer = new Container { - beatmapText = new LinkFlowContainer { AutoSizeAxes = Axes.Both }, - new FillFlowContainer + Anchor = Anchor.CentreLeft, + Origin = Anchor.CentreLeft, + AutoSizeAxes = Axes.Both, + }, + new FillFlowContainer + { + Anchor = Anchor.CentreLeft, + Origin = Anchor.CentreLeft, + AutoSizeAxes = Axes.Both, + Direction = FillDirection.Vertical, + Children = new Drawable[] { - AutoSizeAxes = Axes.Both, - Direction = FillDirection.Horizontal, - Spacing = new Vector2(10f, 0), - Children = new Drawable[] + beatmapText = new LinkFlowContainer(fontParameters) { AutoSizeAxes = Axes.Both }, + new FillFlowContainer { - new FillFlowContainer + AutoSizeAxes = Axes.Both, + Direction = FillDirection.Horizontal, + Spacing = new Vector2(10f, 0), + Children = new Drawable[] { - AutoSizeAxes = Axes.Both, - Direction = FillDirection.Horizontal, - Spacing = new Vector2(10f, 0), - Children = new Drawable[] + new FillFlowContainer { - authorText = new LinkFlowContainer { AutoSizeAxes = Axes.Both }, - explicitContentPill = new ExplicitContentBeatmapPill + AutoSizeAxes = Axes.Both, + Direction = FillDirection.Horizontal, + Spacing = new Vector2(10f, 0), + Children = new Drawable[] { - Alpha = 0f, - Anchor = Anchor.CentreLeft, - Origin = Anchor.CentreLeft, - Margin = new MarginPadding { Top = 3f }, - } + authorText = new LinkFlowContainer(fontParameters) { AutoSizeAxes = Axes.Both }, + explicitContentPill = new ExplicitContentBeatmapPill + { + Alpha = 0f, + Anchor = Anchor.CentreLeft, + Origin = Anchor.CentreLeft, + Margin = new MarginPadding { Top = 3f }, + } + }, }, - }, - new Container - { - Anchor = Anchor.CentreLeft, - Origin = Anchor.CentreLeft, - AutoSizeAxes = Axes.Both, - Child = modDisplay = new ModDisplay + new Container { - Scale = new Vector2(0.4f), - DisplayUnrankedText = false, - ExpansionMode = ExpansionMode.AlwaysExpanded + Anchor = Anchor.CentreLeft, + Origin = Anchor.CentreLeft, + AutoSizeAxes = Axes.Both, + Child = modDisplay = new ModDisplay + { + Scale = new Vector2(0.4f), + DisplayUnrankedText = false, + ExpansionMode = ExpansionMode.AlwaysExpanded + } } } } } } } + }, + new FillFlowContainer + { + Anchor = Anchor.CentreRight, + Origin = Anchor.CentreRight, + Direction = FillDirection.Horizontal, + AutoSizeAxes = Axes.Both, + Spacing = new Vector2(5), + X = -10, + ChildrenEnumerable = CreateButtons() } - }, - new FillFlowContainer - { - Anchor = Anchor.CentreRight, - Origin = Anchor.CentreRight, - Direction = FillDirection.Horizontal, - AutoSizeAxes = Axes.Both, - Spacing = new Vector2(5), - X = -10, - ChildrenEnumerable = CreateButtons() } - } - }; + }; + } protected virtual IEnumerable CreateButtons() => new Drawable[] From bc8a4f411159140371596546144fd56e34bc63d2 Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Tue, 2 Feb 2021 17:21:46 +0900 Subject: [PATCH 11/18] Update test handling --- .../Visual/Multiplayer/TestSceneDrawableRoomPlaylist.cs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/osu.Game.Tests/Visual/Multiplayer/TestSceneDrawableRoomPlaylist.cs b/osu.Game.Tests/Visual/Multiplayer/TestSceneDrawableRoomPlaylist.cs index 16f6723e2d..960aad10c6 100644 --- a/osu.Game.Tests/Visual/Multiplayer/TestSceneDrawableRoomPlaylist.cs +++ b/osu.Game.Tests/Visual/Multiplayer/TestSceneDrawableRoomPlaylist.cs @@ -11,8 +11,8 @@ using osu.Framework.Graphics.Containers; using osu.Framework.Platform; using osu.Framework.Testing; using osu.Game.Beatmaps; +using osu.Game.Beatmaps.Drawables; using osu.Game.Graphics.Containers; -using osu.Game.Graphics.UserInterface; using osu.Game.Online.Rooms; using osu.Game.Overlays; using osu.Game.Rulesets; @@ -242,7 +242,7 @@ namespace osu.Game.Tests.Visual.Multiplayer } private void moveToItem(int index, Vector2? offset = null) - => AddStep($"move mouse to item {index}", () => InputManager.MoveMouseTo(playlist.ChildrenOfType>().ElementAt(index), offset)); + => AddStep($"move mouse to item {index}", () => InputManager.MoveMouseTo(playlist.ChildrenOfType().ElementAt(index), offset)); private void moveToDragger(int index, Vector2? offset = null) => AddStep($"move mouse to dragger {index}", () => { @@ -253,7 +253,7 @@ namespace osu.Game.Tests.Visual.Multiplayer private void moveToDeleteButton(int index, Vector2? offset = null) => AddStep($"move mouse to delete button {index}", () => { var item = playlist.ChildrenOfType>().ElementAt(index); - InputManager.MoveMouseTo(item.ChildrenOfType().ElementAt(0), offset); + InputManager.MoveMouseTo(item.ChildrenOfType().ElementAt(0), offset); }); private void assertHandleVisibility(int index, bool visible) @@ -261,7 +261,7 @@ namespace osu.Game.Tests.Visual.Multiplayer () => (playlist.ChildrenOfType.PlaylistItemHandle>().ElementAt(index).Alpha > 0) == visible); private void assertDeleteButtonVisibility(int index, bool visible) - => AddAssert($"delete button {index} {(visible ? "is" : "is not")} visible", () => (playlist.ChildrenOfType().ElementAt(2 + index * 2).Alpha > 0) == visible); + => AddAssert($"delete button {index} {(visible ? "is" : "is not")} visible", () => (playlist.ChildrenOfType().ElementAt(2 + index * 2).Alpha > 0) == visible); private void createPlaylist(bool allowEdit, bool allowSelection) { From 052cf1abaedec23cf44022aedc3324dacd5a8168 Mon Sep 17 00:00:00 2001 From: smoogipoo Date: Tue, 2 Feb 2021 18:42:02 +0900 Subject: [PATCH 12/18] Reuse existing method --- osu.Game/Utils/ModUtils.cs | 30 ++++++------------------------ 1 file changed, 6 insertions(+), 24 deletions(-) diff --git a/osu.Game/Utils/ModUtils.cs b/osu.Game/Utils/ModUtils.cs index 05a07f0459..0eb30cbe36 100644 --- a/osu.Game/Utils/ModUtils.cs +++ b/osu.Game/Utils/ModUtils.cs @@ -88,40 +88,22 @@ namespace osu.Game.Utils /// The mods to check. /// Invalid mods, if any were found. Can be null if all mods were valid. /// Whether the input mods were all valid. If false, will contain all invalid entries. - public static bool CheckValidForGameplay(IEnumerable mods, out Mod[]? invalidMods) + public static bool CheckValidForGameplay(IEnumerable mods, out List? invalidMods) { mods = mods.ToArray(); - List? foundInvalid = null; - - void addInvalid(Mod mod) - { - foundInvalid ??= new List(); - foundInvalid.Add(mod); - } + CheckCompatibleSet(mods, out invalidMods); foreach (var mod in mods) { - bool valid = mod.Type != ModType.System - && mod.HasImplementation - && !(mod is MultiMod); - - if (!valid) + if (mod.Type == ModType.System || !mod.HasImplementation || mod is MultiMod) { - // if this mod was found as invalid, we can exclude it before potentially excluding more incompatible types. - addInvalid(mod); - continue; - } - - foreach (var type in mod.IncompatibleMods) - { - foreach (var invalid in mods.Where(m => type.IsInstanceOfType(m))) - addInvalid(invalid); + invalidMods ??= new List(); + invalidMods.Add(mod); } } - invalidMods = foundInvalid?.ToArray(); - return foundInvalid == null; + return invalidMods == null; } /// From 0a9861d0abe19879a3d9772c248df7ed2b0c2e74 Mon Sep 17 00:00:00 2001 From: smoogipoo Date: Tue, 2 Feb 2021 18:51:13 +0900 Subject: [PATCH 13/18] Use TestCaseSource and add multi-mod test --- osu.Game.Tests/Mods/ModUtilsTest.cs | 47 +++++++++++++++++++++-------- 1 file changed, 35 insertions(+), 12 deletions(-) diff --git a/osu.Game.Tests/Mods/ModUtilsTest.cs b/osu.Game.Tests/Mods/ModUtilsTest.cs index ff1af88bac..fbdb1e2f3d 100644 --- a/osu.Game.Tests/Mods/ModUtilsTest.cs +++ b/osu.Game.Tests/Mods/ModUtilsTest.cs @@ -103,20 +103,43 @@ namespace osu.Game.Tests.Mods Assert.That(ModUtils.CheckAllowed(new[] { mod.Object }, new[] { typeof(Mod) }), Is.False); } - // test incompatible pair. - [TestCase(new[] { typeof(OsuModDoubleTime), typeof(OsuModHalfTime) }, new[] { typeof(OsuModDoubleTime), typeof(OsuModHalfTime) })] - // test incompatible pair with derived class. - [TestCase(new[] { typeof(OsuModNightcore), typeof(OsuModHalfTime) }, new[] { typeof(OsuModNightcore), typeof(OsuModHalfTime) })] - // test system mod. - [TestCase(new[] { typeof(OsuModDoubleTime), typeof(OsuModTouchDevice) }, new[] { typeof(OsuModTouchDevice) })] - // test valid. - [TestCase(new[] { typeof(OsuModDoubleTime), typeof(OsuModHardRock) }, null)] - public void TestInvalidModScenarios(Type[] input, Type[] expectedInvalid) + private static readonly object[] invalid_mod_test_scenarios = { - List inputMods = new List(); - foreach (var t in input) - inputMods.Add((Mod)Activator.CreateInstance(t)); + // incompatible pair. + new object[] + { + new Mod[] { new OsuModDoubleTime(), new OsuModHalfTime() }, + new[] { typeof(OsuModDoubleTime), typeof(OsuModHalfTime) } + }, + // incompatible pair with derived class. + new object[] + { + new Mod[] { new OsuModNightcore(), new OsuModHalfTime() }, + new[] { typeof(OsuModNightcore), typeof(OsuModHalfTime) } + }, + // system mod. + new object[] + { + new Mod[] { new OsuModDoubleTime(), new OsuModTouchDevice() }, + new[] { typeof(OsuModTouchDevice) } + }, + // multi mod. + new object[] + { + new Mod[] { new MultiMod(new OsuModHalfTime()), new OsuModHalfTime() }, + new[] { typeof(MultiMod) } + }, + // valid pair. + new object[] + { + new Mod[] { new OsuModDoubleTime(), new OsuModHardRock() }, + null + } + }; + [TestCaseSource(nameof(invalid_mod_test_scenarios))] + public void TestInvalidModScenarios(Mod[] inputMods, Type[] expectedInvalid) + { bool isValid = ModUtils.CheckValidForGameplay(inputMods, out var invalid); Assert.That(isValid, Is.EqualTo(expectedInvalid == null)); From 41593ff09e4ef0ac405cbd0fdcdba0beff9a29b5 Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Tue, 2 Feb 2021 19:14:44 +0900 Subject: [PATCH 14/18] Privatise protected property setters --- osu.Game/Graphics/UserInterface/GrayButton.cs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/osu.Game/Graphics/UserInterface/GrayButton.cs b/osu.Game/Graphics/UserInterface/GrayButton.cs index dd05701545..88c46f29e0 100644 --- a/osu.Game/Graphics/UserInterface/GrayButton.cs +++ b/osu.Game/Graphics/UserInterface/GrayButton.cs @@ -11,8 +11,8 @@ namespace osu.Game.Graphics.UserInterface { public class GrayButton : OsuAnimatedButton { - protected SpriteIcon Icon; - protected Box Background; + protected SpriteIcon Icon { get; private set; } + protected Box Background { get; private set; } private readonly IconUsage icon; From 8e70a50af0836af6fb821c69a7239c7cad9e1eec Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Tue, 2 Feb 2021 19:22:13 +0900 Subject: [PATCH 15/18] Remove unused using statement --- osu.Game.Tests/Mods/ModUtilsTest.cs | 1 - 1 file changed, 1 deletion(-) diff --git a/osu.Game.Tests/Mods/ModUtilsTest.cs b/osu.Game.Tests/Mods/ModUtilsTest.cs index fbdb1e2f3d..7dcaabca3d 100644 --- a/osu.Game.Tests/Mods/ModUtilsTest.cs +++ b/osu.Game.Tests/Mods/ModUtilsTest.cs @@ -2,7 +2,6 @@ // See the LICENCE file in the repository root for full licence text. using System; -using System.Collections.Generic; using System.Linq; using Moq; using NUnit.Framework; From 921f008217719bb6768b54c7746eef2d7d0f0ba7 Mon Sep 17 00:00:00 2001 From: smoogipoo Date: Tue, 2 Feb 2021 21:35:08 +0900 Subject: [PATCH 16/18] Fix ModIcon not updating background colour correctly --- .../Visual/UserInterface/TestSceneModIcon.cs | 21 +++++++++ osu.Game/Rulesets/UI/ModIcon.cs | 46 +++++++++++-------- 2 files changed, 47 insertions(+), 20 deletions(-) create mode 100644 osu.Game.Tests/Visual/UserInterface/TestSceneModIcon.cs diff --git a/osu.Game.Tests/Visual/UserInterface/TestSceneModIcon.cs b/osu.Game.Tests/Visual/UserInterface/TestSceneModIcon.cs new file mode 100644 index 0000000000..e7fa7d9235 --- /dev/null +++ b/osu.Game.Tests/Visual/UserInterface/TestSceneModIcon.cs @@ -0,0 +1,21 @@ +// 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.Game.Rulesets.Osu.Mods; +using osu.Game.Rulesets.UI; + +namespace osu.Game.Tests.Visual.UserInterface +{ + public class TestSceneModIcon : OsuTestScene + { + [Test] + public void TestChangeModType() + { + ModIcon icon = null; + + AddStep("create mod icon", () => Child = icon = new ModIcon(new OsuModDoubleTime())); + AddStep("change mod", () => icon.Mod = new OsuModEasy()); + } + } +} diff --git a/osu.Game/Rulesets/UI/ModIcon.cs b/osu.Game/Rulesets/UI/ModIcon.cs index 04a2e052fa..cae5da3d16 100644 --- a/osu.Game/Rulesets/UI/ModIcon.cs +++ b/osu.Game/Rulesets/UI/ModIcon.cs @@ -29,8 +29,6 @@ namespace osu.Game.Rulesets.UI private const float size = 80; - private readonly ModType type; - public virtual string TooltipText => showTooltip ? mod.IconTooltip : null; private Mod mod; @@ -42,10 +40,18 @@ namespace osu.Game.Rulesets.UI set { mod = value; - updateMod(value); + + if (IsLoaded) + updateMod(value); } } + [Resolved] + private OsuColour colours { get; set; } + + private Color4 backgroundColour; + private Color4 highlightedColour; + /// /// Construct a new instance. /// @@ -56,8 +62,6 @@ namespace osu.Game.Rulesets.UI this.mod = mod ?? throw new ArgumentNullException(nameof(mod)); this.showTooltip = showTooltip; - type = mod.Type; - Size = new Vector2(size); Children = new Drawable[] @@ -89,6 +93,13 @@ namespace osu.Game.Rulesets.UI Icon = FontAwesome.Solid.Question }, }; + } + + protected override void LoadComplete() + { + base.LoadComplete(); + + Selected.BindValueChanged(_ => updateColour()); updateMod(mod); } @@ -102,20 +113,14 @@ namespace osu.Game.Rulesets.UI { modIcon.FadeOut(); modAcronym.FadeIn(); - return; + } + else + { + modIcon.FadeIn(); + modAcronym.FadeOut(); } - modIcon.FadeIn(); - modAcronym.FadeOut(); - } - - private Color4 backgroundColour; - private Color4 highlightedColour; - - [BackgroundDependencyLoader] - private void load(OsuColour colours) - { - switch (type) + switch (value.Type) { default: case ModType.DifficultyIncrease: @@ -149,12 +154,13 @@ namespace osu.Game.Rulesets.UI modIcon.Colour = colours.Yellow; break; } + + updateColour(); } - protected override void LoadComplete() + private void updateColour() { - base.LoadComplete(); - Selected.BindValueChanged(selected => background.Colour = selected.NewValue ? highlightedColour : backgroundColour, true); + background.Colour = Selected.Value ? highlightedColour : backgroundColour; } } } From 181d2c672b5c93c1dfb84929b78a413dfe326db3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bart=C5=82omiej=20Dach?= Date: Tue, 2 Feb 2021 22:05:25 +0100 Subject: [PATCH 17/18] Fix outdated comment --- osu.Game/Screens/OnlinePlay/DrawableRoomPlaylistItem.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/osu.Game/Screens/OnlinePlay/DrawableRoomPlaylistItem.cs b/osu.Game/Screens/OnlinePlay/DrawableRoomPlaylistItem.cs index a7015ba1c4..2d438bd96e 100644 --- a/osu.Game/Screens/OnlinePlay/DrawableRoomPlaylistItem.cs +++ b/osu.Game/Screens/OnlinePlay/DrawableRoomPlaylistItem.cs @@ -343,7 +343,7 @@ namespace osu.Game.Screens.OnlinePlay Colour = Color4.Black, Width = 0.4f, }, - // Piecewise-linear gradient with 3 segments to make it appear smoother + // Piecewise-linear gradient with 2 segments to make it appear smoother new Box { RelativeSizeAxes = Axes.Both, From fc84ec131347ee220c9c9df63561b3e6a099156f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bart=C5=82omiej=20Dach?= Date: Tue, 2 Feb 2021 22:18:14 +0100 Subject: [PATCH 18/18] Move anchor specification to central place --- .../Screens/OnlinePlay/DrawableRoomPlaylistItem.cs | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/osu.Game/Screens/OnlinePlay/DrawableRoomPlaylistItem.cs b/osu.Game/Screens/OnlinePlay/DrawableRoomPlaylistItem.cs index 2d438bd96e..23c713a2c1 100644 --- a/osu.Game/Screens/OnlinePlay/DrawableRoomPlaylistItem.cs +++ b/osu.Game/Screens/OnlinePlay/DrawableRoomPlaylistItem.cs @@ -220,7 +220,11 @@ namespace osu.Game.Screens.OnlinePlay AutoSizeAxes = Axes.Both, Spacing = new Vector2(5), X = -10, - ChildrenEnumerable = CreateButtons() + ChildrenEnumerable = CreateButtons().Select(button => button.With(b => + { + b.Anchor = Anchor.Centre; + b.Origin = Anchor.Centre; + })) } } }; @@ -231,14 +235,10 @@ namespace osu.Game.Screens.OnlinePlay { new PlaylistDownloadButton(Item) { - Anchor = Anchor.Centre, - Origin = Anchor.Centre, Size = new Vector2(50, 30) }, new PlaylistRemoveButton { - Anchor = Anchor.Centre, - Origin = Anchor.Centre, Size = new Vector2(30, 30), Alpha = allowEdit ? 1 : 0, Action = () => RequestDeletion?.Invoke(Model),