From 9e6994166c0eef43cb63b6d1d34e70e0cb324634 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bart=C5=82omiej=20Dach?= Date: Mon, 28 Dec 2020 20:59:12 +0100 Subject: [PATCH 01/33] Add helper to track ongoing operations in UI --- .../OnlinePlay/OngoingOperationTracker.cs | 47 +++++++++++++++++++ 1 file changed, 47 insertions(+) create mode 100644 osu.Game/Screens/OnlinePlay/OngoingOperationTracker.cs diff --git a/osu.Game/Screens/OnlinePlay/OngoingOperationTracker.cs b/osu.Game/Screens/OnlinePlay/OngoingOperationTracker.cs new file mode 100644 index 0000000000..f2d943e14f --- /dev/null +++ b/osu.Game/Screens/OnlinePlay/OngoingOperationTracker.cs @@ -0,0 +1,47 @@ +// 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 osu.Framework.Bindables; + +namespace osu.Game.Screens.OnlinePlay +{ + /// + /// Utility class to track ongoing online operations' progress. + /// Can be used to disable interactivity while waiting for a response from online sources. + /// + public class OngoingOperationTracker + { + /// + /// Whether there is an online operation in progress. + /// + public IBindable InProgress => inProgress; + + private readonly Bindable inProgress = new BindableBool(); + + private LeasedBindable leasedInProgress; + + /// + /// Begins tracking a new online operation. + /// + /// An operation has already been started. + public void BeginOperation() + { + if (leasedInProgress != null) + throw new InvalidOperationException("Cannot begin operation while another is in progress."); + + leasedInProgress = inProgress.BeginLease(true); + leasedInProgress.Value = true; + } + + /// + /// Ends tracking an online operation. + /// Does nothing if an operation has not been begun yet. + /// + public void EndOperation() + { + leasedInProgress?.Return(); + leasedInProgress = null; + } + } +} From 47ab7c9fd67270079d39e8ad9f45fb3e986c76df Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bart=C5=82omiej=20Dach?= Date: Mon, 28 Dec 2020 20:59:38 +0100 Subject: [PATCH 02/33] Disable ready button after host click --- .../TestSceneMultiplayerReadyButton.cs | 22 +++++++++++++++---- .../Match/MultiplayerReadyButton.cs | 13 ++++++++++- .../Multiplayer/MultiplayerMatchSubScreen.cs | 4 ++++ 3 files changed, 34 insertions(+), 5 deletions(-) diff --git a/osu.Game.Tests/Visual/Multiplayer/TestSceneMultiplayerReadyButton.cs b/osu.Game.Tests/Visual/Multiplayer/TestSceneMultiplayerReadyButton.cs index 6b11613f1c..958c6d218b 100644 --- a/osu.Game.Tests/Visual/Multiplayer/TestSceneMultiplayerReadyButton.cs +++ b/osu.Game.Tests/Visual/Multiplayer/TestSceneMultiplayerReadyButton.cs @@ -7,11 +7,14 @@ using osu.Framework.Allocation; using osu.Framework.Audio; using osu.Framework.Graphics; using osu.Framework.Platform; +using osu.Framework.Testing; using osu.Framework.Utils; using osu.Game.Beatmaps; +using osu.Game.Graphics.UserInterface; using osu.Game.Online.Multiplayer; using osu.Game.Online.Rooms; using osu.Game.Rulesets; +using osu.Game.Screens.OnlinePlay; using osu.Game.Screens.OnlinePlay.Multiplayer.Match; using osu.Game.Tests.Resources; using osu.Game.Users; @@ -27,6 +30,9 @@ namespace osu.Game.Tests.Visual.Multiplayer private BeatmapManager beatmaps; private RulesetStore rulesets; + [Cached] + private OngoingOperationTracker gameplayStartTracker = new OngoingOperationTracker(); + [BackgroundDependencyLoader] private void load(GameHost host, AudioManager audio) { @@ -89,8 +95,7 @@ namespace osu.Game.Tests.Visual.Multiplayer addClickButtonStep(); AddAssert("user is ready", () => Client.Room?.Users[0].State == MultiplayerUserState.Ready); - addClickButtonStep(); - AddAssert("match started", () => Client.Room?.Users[0].State == MultiplayerUserState.WaitingForLoad); + verifyGameplayStartFlow(); } [Test] @@ -105,8 +110,7 @@ namespace osu.Game.Tests.Visual.Multiplayer addClickButtonStep(); AddStep("make user host", () => Client.TransferHost(Client.Room?.Users[0].UserID ?? 0)); - addClickButtonStep(); - AddAssert("match started", () => Client.Room?.Users[0].State == MultiplayerUserState.WaitingForLoad); + verifyGameplayStartFlow(); } [Test] @@ -160,5 +164,15 @@ namespace osu.Game.Tests.Visual.Multiplayer InputManager.MoveMouseTo(button); InputManager.Click(MouseButton.Left); }); + + private void verifyGameplayStartFlow() + { + addClickButtonStep(); + AddAssert("user waiting for load", () => Client.Room?.Users[0].State == MultiplayerUserState.WaitingForLoad); + AddAssert("ready button disabled", () => !button.ChildrenOfType().Single().Enabled.Value); + + AddStep("transitioned to gameplay", () => gameplayStartTracker.EndOperation()); + AddAssert("ready button enabled", () => button.ChildrenOfType().Single().Enabled.Value); + } } } diff --git a/osu.Game/Screens/OnlinePlay/Multiplayer/Match/MultiplayerReadyButton.cs b/osu.Game/Screens/OnlinePlay/Multiplayer/Match/MultiplayerReadyButton.cs index 281e92404c..5009b435f4 100644 --- a/osu.Game/Screens/OnlinePlay/Multiplayer/Match/MultiplayerReadyButton.cs +++ b/osu.Game/Screens/OnlinePlay/Multiplayer/Match/MultiplayerReadyButton.cs @@ -33,11 +33,15 @@ namespace osu.Game.Screens.OnlinePlay.Multiplayer.Match [Resolved] private OsuColour colours { get; set; } + [Resolved] + private OngoingOperationTracker gameplayStartTracker { get; set; } + private SampleChannel sampleReadyCount; private readonly ButtonWithTrianglesExposed button; private int countReady; + private IBindable gameplayStartInProgress; public MultiplayerReadyButton() { @@ -54,6 +58,9 @@ namespace osu.Game.Screens.OnlinePlay.Multiplayer.Match private void load(AudioManager audio) { sampleReadyCount = audio.Samples.Get(@"SongSelect/select-difficulty"); + + gameplayStartInProgress = gameplayStartTracker.InProgress.GetBoundCopy(); + gameplayStartInProgress.BindValueChanged(_ => updateState()); } protected override void OnRoomUpdated() @@ -63,7 +70,6 @@ namespace osu.Game.Screens.OnlinePlay.Multiplayer.Match // this method is called on leaving the room, so the local user may not exist in the room any more. localUser = Room?.Users.SingleOrDefault(u => u.User?.Id == api.LocalUser.Value.Id); - button.Enabled.Value = Client.Room?.State == MultiplayerRoomState.Open; updateState(); } @@ -100,6 +106,8 @@ namespace osu.Game.Screens.OnlinePlay.Multiplayer.Match break; } + button.Enabled.Value = Client.Room?.State == MultiplayerRoomState.Open && !gameplayStartInProgress.Value; + if (newCountReady != countReady) { countReady = newCountReady; @@ -142,7 +150,10 @@ namespace osu.Game.Screens.OnlinePlay.Multiplayer.Match else { if (Room?.Host?.Equals(localUser) == true) + { + gameplayStartTracker.BeginOperation(); Client.StartMatch().CatchUnobservedExceptions(true); + } else Client.ChangeState(MultiplayerUserState.Idle).CatchUnobservedExceptions(true); } diff --git a/osu.Game/Screens/OnlinePlay/Multiplayer/MultiplayerMatchSubScreen.cs b/osu.Game/Screens/OnlinePlay/Multiplayer/MultiplayerMatchSubScreen.cs index 58314c3774..93db913ce0 100644 --- a/osu.Game/Screens/OnlinePlay/Multiplayer/MultiplayerMatchSubScreen.cs +++ b/osu.Game/Screens/OnlinePlay/Multiplayer/MultiplayerMatchSubScreen.cs @@ -31,6 +31,9 @@ namespace osu.Game.Screens.OnlinePlay.Multiplayer [Resolved] private StatefulMultiplayerClient client { get; set; } + [Cached] + private OngoingOperationTracker gameplayStartTracker = new OngoingOperationTracker(); + private MultiplayerMatchSettingsOverlay settingsOverlay; private IBindable isConnected; @@ -203,6 +206,7 @@ namespace osu.Game.Screens.OnlinePlay.Multiplayer int[] userIds = client.Room.Users.Where(u => u.State >= MultiplayerUserState.WaitingForLoad).Select(u => u.UserID).ToArray(); StartPlay(() => new MultiplayerPlayer(SelectedItem.Value, userIds)); + gameplayStartTracker.EndOperation(); } protected override void Dispose(bool isDisposing) From af66e4531196853215e8625cf858d4a0d221aaec Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bart=C5=82omiej=20Dach?= Date: Mon, 28 Dec 2020 21:39:11 +0100 Subject: [PATCH 03/33] Disable create room button after triggering join --- .../TestSceneCreateMultiplayerMatchButton.cs | 52 +++++++++++++++++++ .../OnlinePlay/Lounge/LoungeSubScreen.cs | 22 ++++---- .../CreateMultiplayerMatchButton.cs | 19 ++++++- .../Screens/OnlinePlay/OnlinePlayScreen.cs | 3 ++ 4 files changed, 83 insertions(+), 13 deletions(-) create mode 100644 osu.Game.Tests/Visual/Multiplayer/TestSceneCreateMultiplayerMatchButton.cs diff --git a/osu.Game.Tests/Visual/Multiplayer/TestSceneCreateMultiplayerMatchButton.cs b/osu.Game.Tests/Visual/Multiplayer/TestSceneCreateMultiplayerMatchButton.cs new file mode 100644 index 0000000000..2a549e5262 --- /dev/null +++ b/osu.Game.Tests/Visual/Multiplayer/TestSceneCreateMultiplayerMatchButton.cs @@ -0,0 +1,52 @@ +// 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.Framework.Allocation; +using osu.Framework.Graphics; +using osu.Game.Screens.OnlinePlay; +using osu.Game.Screens.OnlinePlay.Multiplayer; + +namespace osu.Game.Tests.Visual.Multiplayer +{ + public class TestSceneCreateMultiplayerMatchButton : MultiplayerTestScene + { + [Cached] + private OngoingOperationTracker joiningRoomTracker = new OngoingOperationTracker(); + + private CreateMultiplayerMatchButton button; + + public override void SetUpSteps() + { + base.SetUpSteps(); + AddStep("create button", () => Child = button = new CreateMultiplayerMatchButton + { + Width = 200, + Height = 100, + Anchor = Anchor.Centre, + Origin = Anchor.Centre + }); + } + + [Test] + public void TestButtonEnableStateChanges() + { + assertButtonEnableState(true); + + AddStep("begin joining room", () => joiningRoomTracker.BeginOperation()); + assertButtonEnableState(false); + + AddStep("end joining room", () => joiningRoomTracker.EndOperation()); + assertButtonEnableState(true); + + AddStep("disconnect client", () => Client.Disconnect()); + assertButtonEnableState(false); + + AddStep("re-connect client", () => Client.Connect()); + assertButtonEnableState(true); + } + + private void assertButtonEnableState(bool enabled) + => AddAssert($"button {(enabled ? "enabled" : "disabled")}", () => button.Enabled.Value == enabled); + } +} diff --git a/osu.Game/Screens/OnlinePlay/Lounge/LoungeSubScreen.cs b/osu.Game/Screens/OnlinePlay/Lounge/LoungeSubScreen.cs index 79f5dfdee1..2730693f73 100644 --- a/osu.Game/Screens/OnlinePlay/Lounge/LoungeSubScreen.cs +++ b/osu.Game/Screens/OnlinePlay/Lounge/LoungeSubScreen.cs @@ -26,6 +26,7 @@ namespace osu.Game.Screens.OnlinePlay.Lounge protected override UserActivity InitialActivity => new UserActivity.SearchingForLobby(); private readonly IBindable initialRoomsReceived = new Bindable(); + private readonly IBindable joiningRoom = new Bindable(); private FilterControl filter; private Container content; @@ -37,7 +38,8 @@ namespace osu.Game.Screens.OnlinePlay.Lounge [Resolved] private MusicController music { get; set; } - private bool joiningRoom; + [Resolved] + private OngoingOperationTracker joiningRoomTracker { get; set; } [BackgroundDependencyLoader] private void load() @@ -98,7 +100,10 @@ namespace osu.Game.Screens.OnlinePlay.Lounge base.LoadComplete(); initialRoomsReceived.BindTo(RoomManager.InitialRoomsReceived); - initialRoomsReceived.BindValueChanged(onInitialRoomsReceivedChanged, true); + initialRoomsReceived.BindValueChanged(_ => updateLoadingLayer()); + + joiningRoom.BindTo(joiningRoomTracker.InProgress); + joiningRoom.BindValueChanged(_ => updateLoadingLayer(), true); } protected override void UpdateAfterChildren() @@ -156,26 +161,21 @@ namespace osu.Game.Screens.OnlinePlay.Lounge private void joinRequested(Room room) { - joiningRoom = true; - updateLoadingLayer(); + joiningRoomTracker.BeginOperation(); RoomManager?.JoinRoom(room, r => { Open(room); - joiningRoom = false; - updateLoadingLayer(); + joiningRoomTracker.EndOperation(); }, _ => { - joiningRoom = false; - updateLoadingLayer(); + joiningRoomTracker.EndOperation(); }); } - private void onInitialRoomsReceivedChanged(ValueChangedEvent received) => updateLoadingLayer(); - private void updateLoadingLayer() { - if (joiningRoom || !initialRoomsReceived.Value) + if (joiningRoom.Value || !initialRoomsReceived.Value) loadingLayer.Show(); else loadingLayer.Hide(); diff --git a/osu.Game/Screens/OnlinePlay/Multiplayer/CreateMultiplayerMatchButton.cs b/osu.Game/Screens/OnlinePlay/Multiplayer/CreateMultiplayerMatchButton.cs index 163efd9c20..7518a4e71b 100644 --- a/osu.Game/Screens/OnlinePlay/Multiplayer/CreateMultiplayerMatchButton.cs +++ b/osu.Game/Screens/OnlinePlay/Multiplayer/CreateMultiplayerMatchButton.cs @@ -10,14 +10,29 @@ namespace osu.Game.Screens.OnlinePlay.Multiplayer { public class CreateMultiplayerMatchButton : PurpleTriangleButton { + private IBindable isConnected; + private IBindable joiningRoom; + + [Resolved] + private StatefulMultiplayerClient multiplayerClient { get; set; } + + [Resolved] + private OngoingOperationTracker joiningRoomTracker { get; set; } + [BackgroundDependencyLoader] - private void load(StatefulMultiplayerClient multiplayerClient) + private void load() { Triangles.TriangleScale = 1.5f; Text = "Create room"; - ((IBindable)Enabled).BindTo(multiplayerClient.IsConnected); + isConnected = multiplayerClient.IsConnected.GetBoundCopy(); + isConnected.BindValueChanged(_ => updateState()); + + joiningRoom = joiningRoomTracker.InProgress.GetBoundCopy(); + joiningRoom.BindValueChanged(_ => updateState(), true); } + + private void updateState() => Enabled.Value = isConnected.Value && !joiningRoom.Value; } } diff --git a/osu.Game/Screens/OnlinePlay/OnlinePlayScreen.cs b/osu.Game/Screens/OnlinePlay/OnlinePlayScreen.cs index 4074dd1573..92665c498b 100644 --- a/osu.Game/Screens/OnlinePlay/OnlinePlayScreen.cs +++ b/osu.Game/Screens/OnlinePlay/OnlinePlayScreen.cs @@ -52,6 +52,9 @@ namespace osu.Game.Screens.OnlinePlay [Cached] private readonly Bindable currentFilter = new Bindable(new FilterCriteria()); + [Cached] + private readonly OngoingOperationTracker joiningRoomTracker = new OngoingOperationTracker(); + [Resolved(CanBeNull = true)] private MusicController music { get; set; } From 6dc0f6af50a3befd707a7f9d4a5fe10cd0862413 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bart=C5=82omiej=20Dach?= Date: Mon, 28 Dec 2020 21:44:58 +0100 Subject: [PATCH 04/33] Disable setting apply button for duration of operation --- .../Match/MultiplayerMatchSettingsOverlay.cs | 19 ++++++++++++++----- 1 file changed, 14 insertions(+), 5 deletions(-) diff --git a/osu.Game/Screens/OnlinePlay/Multiplayer/Match/MultiplayerMatchSettingsOverlay.cs b/osu.Game/Screens/OnlinePlay/Multiplayer/Match/MultiplayerMatchSettingsOverlay.cs index ae03d384f6..de25cff546 100644 --- a/osu.Game/Screens/OnlinePlay/Multiplayer/Match/MultiplayerMatchSettingsOverlay.cs +++ b/osu.Game/Screens/OnlinePlay/Multiplayer/Match/MultiplayerMatchSettingsOverlay.cs @@ -68,6 +68,8 @@ namespace osu.Game.Screens.OnlinePlay.Multiplayer.Match [Resolved] private Bindable ruleset { get; set; } + private readonly OngoingOperationTracker applyingSettingsTracker = new OngoingOperationTracker(); + [BackgroundDependencyLoader] private void load(OsuColour colours) { @@ -274,13 +276,21 @@ namespace osu.Game.Screens.OnlinePlay.Multiplayer.Match Type.BindValueChanged(type => TypePicker.Current.Value = type.NewValue, true); MaxParticipants.BindValueChanged(count => MaxParticipantsField.Text = count.NewValue?.ToString(), true); RoomID.BindValueChanged(roomId => initialBeatmapControl.Alpha = roomId.NewValue == null ? 1 : 0, true); + + applyingSettingsTracker.InProgress.BindValueChanged(v => + { + if (v.NewValue) + loadingLayer.Show(); + else + loadingLayer.Hide(); + }); } protected override void Update() { base.Update(); - ApplyButton.Enabled.Value = Playlist.Count > 0 && NameField.Text.Length > 0; + ApplyButton.Enabled.Value = Playlist.Count > 0 && NameField.Text.Length > 0 && !applyingSettingsTracker.InProgress.Value; } private void apply() @@ -289,7 +299,7 @@ namespace osu.Game.Screens.OnlinePlay.Multiplayer.Match return; hideError(); - loadingLayer.Show(); + applyingSettingsTracker.BeginOperation(); // If the client is already in a room, update via the client. // Otherwise, update the room directly in preparation for it to be submitted to the API on match creation. @@ -322,7 +332,7 @@ namespace osu.Game.Screens.OnlinePlay.Multiplayer.Match private void onSuccess(Room room) { - loadingLayer.Hide(); + applyingSettingsTracker.EndOperation(); SettingsApplied?.Invoke(); } @@ -330,8 +340,7 @@ namespace osu.Game.Screens.OnlinePlay.Multiplayer.Match { ErrorText.Text = text; ErrorText.FadeIn(50); - - loadingLayer.Hide(); + applyingSettingsTracker.EndOperation(); } } From 540dec2e7ccbb96342683545d744ffa74b4011d4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bart=C5=82omiej=20Dach?= Date: Mon, 28 Dec 2020 22:54:52 +0100 Subject: [PATCH 05/33] Allow null tracker in lounge screen for tests --- .../Screens/OnlinePlay/Lounge/LoungeSubScreen.cs | 15 +++++++++------ 1 file changed, 9 insertions(+), 6 deletions(-) diff --git a/osu.Game/Screens/OnlinePlay/Lounge/LoungeSubScreen.cs b/osu.Game/Screens/OnlinePlay/Lounge/LoungeSubScreen.cs index 2730693f73..f25835f56a 100644 --- a/osu.Game/Screens/OnlinePlay/Lounge/LoungeSubScreen.cs +++ b/osu.Game/Screens/OnlinePlay/Lounge/LoungeSubScreen.cs @@ -38,7 +38,7 @@ namespace osu.Game.Screens.OnlinePlay.Lounge [Resolved] private MusicController music { get; set; } - [Resolved] + [Resolved(CanBeNull = true)] private OngoingOperationTracker joiningRoomTracker { get; set; } [BackgroundDependencyLoader] @@ -102,8 +102,11 @@ namespace osu.Game.Screens.OnlinePlay.Lounge initialRoomsReceived.BindTo(RoomManager.InitialRoomsReceived); initialRoomsReceived.BindValueChanged(_ => updateLoadingLayer()); - joiningRoom.BindTo(joiningRoomTracker.InProgress); - joiningRoom.BindValueChanged(_ => updateLoadingLayer(), true); + if (joiningRoomTracker != null) + { + joiningRoom.BindTo(joiningRoomTracker.InProgress); + joiningRoom.BindValueChanged(_ => updateLoadingLayer(), true); + } } protected override void UpdateAfterChildren() @@ -161,15 +164,15 @@ namespace osu.Game.Screens.OnlinePlay.Lounge private void joinRequested(Room room) { - joiningRoomTracker.BeginOperation(); + joiningRoomTracker?.BeginOperation(); RoomManager?.JoinRoom(room, r => { Open(room); - joiningRoomTracker.EndOperation(); + joiningRoomTracker?.EndOperation(); }, _ => { - joiningRoomTracker.EndOperation(); + joiningRoomTracker?.EndOperation(); }); } From 903dca875e7aa364277dc336e52c9c350a6f363a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bart=C5=82omiej=20Dach?= Date: Tue, 29 Dec 2020 07:46:22 +0100 Subject: [PATCH 06/33] Make localUser a client property --- .../Online/Multiplayer/StatefulMultiplayerClient.cs | 5 +++++ .../Multiplayer/Match/MultiplayerReadyButton.cs | 10 ++++------ 2 files changed, 9 insertions(+), 6 deletions(-) diff --git a/osu.Game/Online/Multiplayer/StatefulMultiplayerClient.cs b/osu.Game/Online/Multiplayer/StatefulMultiplayerClient.cs index 39d119b2a4..0f44085134 100644 --- a/osu.Game/Online/Multiplayer/StatefulMultiplayerClient.cs +++ b/osu.Game/Online/Multiplayer/StatefulMultiplayerClient.cs @@ -65,6 +65,11 @@ namespace osu.Game.Online.Multiplayer /// public readonly BindableList CurrentMatchPlayingUserIds = new BindableList(); + /// + /// The corresponding to the local player, if available. + /// + public MultiplayerRoomUser? LocalUser => Room?.Users.SingleOrDefault(u => u.User?.Id == api.LocalUser.Value.Id); + [Resolved] private UserLookupCache userLookupCache { get; set; } = null!; diff --git a/osu.Game/Screens/OnlinePlay/Multiplayer/Match/MultiplayerReadyButton.cs b/osu.Game/Screens/OnlinePlay/Multiplayer/Match/MultiplayerReadyButton.cs index 5009b435f4..9b406ff52c 100644 --- a/osu.Game/Screens/OnlinePlay/Multiplayer/Match/MultiplayerReadyButton.cs +++ b/osu.Game/Screens/OnlinePlay/Multiplayer/Match/MultiplayerReadyButton.cs @@ -27,9 +27,6 @@ namespace osu.Game.Screens.OnlinePlay.Multiplayer.Match [Resolved] private IAPIProvider api { get; set; } - [CanBeNull] - private MultiplayerRoomUser localUser; - [Resolved] private OsuColour colours { get; set; } @@ -67,14 +64,13 @@ namespace osu.Game.Screens.OnlinePlay.Multiplayer.Match { base.OnRoomUpdated(); - // this method is called on leaving the room, so the local user may not exist in the room any more. - localUser = Room?.Users.SingleOrDefault(u => u.User?.Id == api.LocalUser.Value.Id); - updateState(); } private void updateState() { + var localUser = Client.LocalUser; + if (localUser == null) return; @@ -142,6 +138,8 @@ namespace osu.Game.Screens.OnlinePlay.Multiplayer.Match private void onClick() { + var localUser = Client.LocalUser; + if (localUser == null) return; From 9ff214023297f4300a439e5c2427281a0cb37bcd Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bart=C5=82omiej=20Dach?= Date: Tue, 29 Dec 2020 07:51:46 +0100 Subject: [PATCH 07/33] Move ready-up logic to match sub-screen --- .../Match/MultiplayerMatchFooter.cs | 9 +++++- .../Match/MultiplayerReadyButton.cs | 30 ++++--------------- .../Multiplayer/MultiplayerMatchSubScreen.cs | 28 ++++++++++++++++- 3 files changed, 41 insertions(+), 26 deletions(-) diff --git a/osu.Game/Screens/OnlinePlay/Multiplayer/Match/MultiplayerMatchFooter.cs b/osu.Game/Screens/OnlinePlay/Multiplayer/Match/MultiplayerMatchFooter.cs index a52f62fe00..bd1ca1377e 100644 --- a/osu.Game/Screens/OnlinePlay/Multiplayer/Match/MultiplayerMatchFooter.cs +++ b/osu.Game/Screens/OnlinePlay/Multiplayer/Match/MultiplayerMatchFooter.cs @@ -1,6 +1,7 @@ // 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 osu.Framework.Allocation; using osu.Framework.Bindables; using osu.Framework.Extensions.Color4Extensions; @@ -19,7 +20,13 @@ namespace osu.Game.Screens.OnlinePlay.Multiplayer.Match public readonly Bindable SelectedItem = new Bindable(); + public Action OnReady + { + set => readyButton.OnReady = value; + } + private readonly Drawable background; + private readonly MultiplayerReadyButton readyButton; public MultiplayerMatchFooter() { @@ -29,7 +36,7 @@ namespace osu.Game.Screens.OnlinePlay.Multiplayer.Match InternalChildren = new[] { background = new Box { RelativeSizeAxes = Axes.Both }, - new MultiplayerReadyButton + readyButton = new MultiplayerReadyButton { Anchor = Anchor.Centre, Origin = Anchor.Centre, diff --git a/osu.Game/Screens/OnlinePlay/Multiplayer/Match/MultiplayerReadyButton.cs b/osu.Game/Screens/OnlinePlay/Multiplayer/Match/MultiplayerReadyButton.cs index 9b406ff52c..96470cf070 100644 --- a/osu.Game/Screens/OnlinePlay/Multiplayer/Match/MultiplayerReadyButton.cs +++ b/osu.Game/Screens/OnlinePlay/Multiplayer/Match/MultiplayerReadyButton.cs @@ -1,15 +1,14 @@ // 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.Diagnostics; using System.Linq; -using JetBrains.Annotations; using osu.Framework.Allocation; using osu.Framework.Audio; using osu.Framework.Audio.Sample; using osu.Framework.Bindables; using osu.Framework.Graphics; -using osu.Game.Extensions; using osu.Game.Graphics; using osu.Game.Graphics.Backgrounds; using osu.Game.Online.API; @@ -24,6 +23,11 @@ namespace osu.Game.Screens.OnlinePlay.Multiplayer.Match { public Bindable SelectedItem => button.SelectedItem; + public Action OnReady + { + set => button.Action = value; + } + [Resolved] private IAPIProvider api { get; set; } @@ -47,7 +51,6 @@ namespace osu.Game.Screens.OnlinePlay.Multiplayer.Match RelativeSizeAxes = Axes.Both, Size = Vector2.One, Enabled = { Value = true }, - Action = onClick }; } @@ -136,27 +139,6 @@ namespace osu.Game.Screens.OnlinePlay.Multiplayer.Match } } - private void onClick() - { - var localUser = Client.LocalUser; - - if (localUser == null) - return; - - if (localUser.State == MultiplayerUserState.Idle) - Client.ChangeState(MultiplayerUserState.Ready).CatchUnobservedExceptions(true); - else - { - if (Room?.Host?.Equals(localUser) == true) - { - gameplayStartTracker.BeginOperation(); - Client.StartMatch().CatchUnobservedExceptions(true); - } - else - Client.ChangeState(MultiplayerUserState.Idle).CatchUnobservedExceptions(true); - } - } - private class ButtonWithTrianglesExposed : ReadyButton { public new Triangles Triangles => base.Triangles; diff --git a/osu.Game/Screens/OnlinePlay/Multiplayer/MultiplayerMatchSubScreen.cs b/osu.Game/Screens/OnlinePlay/Multiplayer/MultiplayerMatchSubScreen.cs index 8eafd7c7df..a87471c8af 100644 --- a/osu.Game/Screens/OnlinePlay/Multiplayer/MultiplayerMatchSubScreen.cs +++ b/osu.Game/Screens/OnlinePlay/Multiplayer/MultiplayerMatchSubScreen.cs @@ -9,6 +9,7 @@ using osu.Framework.Bindables; using osu.Framework.Graphics; using osu.Framework.Graphics.Containers; using osu.Framework.Screens; +using osu.Game.Extensions; using osu.Game.Online.Multiplayer; using osu.Game.Online.Rooms; using osu.Game.Screens.OnlinePlay.Components; @@ -153,7 +154,11 @@ namespace osu.Game.Screens.OnlinePlay.Multiplayer }, new Drawable[] { - new MultiplayerMatchFooter { SelectedItem = { BindTarget = SelectedItem } } + new MultiplayerMatchFooter + { + SelectedItem = { BindTarget = SelectedItem }, + OnReady = onReady + } } }, RowDimensions = new[] @@ -199,6 +204,27 @@ namespace osu.Game.Screens.OnlinePlay.Multiplayer private void onPlaylistChanged(object sender, NotifyCollectionChangedEventArgs e) => SelectedItem.Value = Playlist.FirstOrDefault(); + private void onReady() + { + var localUser = client.LocalUser; + + if (localUser == null) + return; + + if (localUser.State == MultiplayerUserState.Idle) + client.ChangeState(MultiplayerUserState.Ready).CatchUnobservedExceptions(true); + else + { + if (client.Room?.Host?.Equals(localUser) == true) + { + gameplayStartTracker.BeginOperation(); + client.StartMatch().CatchUnobservedExceptions(true); + } + else + client.ChangeState(MultiplayerUserState.Idle).CatchUnobservedExceptions(true); + } + } + private void onLoadRequested() { Debug.Assert(client.Room != null); From f59ba799d30390f030fcac0e12ac99a1893f78f4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bart=C5=82omiej=20Dach?= Date: Tue, 29 Dec 2020 07:54:27 +0100 Subject: [PATCH 08/33] Adjust operation tracker implementation --- osu.Game/Screens/OnlinePlay/OngoingOperationTracker.cs | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/osu.Game/Screens/OnlinePlay/OngoingOperationTracker.cs b/osu.Game/Screens/OnlinePlay/OngoingOperationTracker.cs index f2d943e14f..6a340d7954 100644 --- a/osu.Game/Screens/OnlinePlay/OngoingOperationTracker.cs +++ b/osu.Game/Screens/OnlinePlay/OngoingOperationTracker.cs @@ -2,6 +2,7 @@ // See the LICENCE file in the repository root for full licence text. using System; +using osu.Framework.Allocation; using osu.Framework.Bindables; namespace osu.Game.Screens.OnlinePlay @@ -24,21 +25,26 @@ namespace osu.Game.Screens.OnlinePlay /// /// Begins tracking a new online operation. /// + /// + /// An that will automatically mark the operation as ended on disposal. + /// /// An operation has already been started. - public void BeginOperation() + public IDisposable BeginOperation() { if (leasedInProgress != null) throw new InvalidOperationException("Cannot begin operation while another is in progress."); leasedInProgress = inProgress.BeginLease(true); leasedInProgress.Value = true; + + return new InvokeOnDisposal(endOperation); } /// /// Ends tracking an online operation. /// Does nothing if an operation has not been begun yet. /// - public void EndOperation() + private void endOperation() { leasedInProgress?.Return(); leasedInProgress = null; From db52255bbec7a284b07baac967280e2cf5b3d9a2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bart=C5=82omiej=20Dach?= Date: Tue, 29 Dec 2020 08:20:43 +0100 Subject: [PATCH 09/33] Adjust tracker usages to match new API --- .../TestSceneCreateMultiplayerMatchButton.cs | 9 ++++-- .../TestSceneMultiplayerReadyButton.cs | 9 ++++-- .../OnlinePlay/Lounge/LoungeSubScreen.cs | 21 +++++++++---- .../CreateMultiplayerMatchButton.cs | 10 +++--- .../Match/MultiplayerMatchSettingsOverlay.cs | 31 +++++++++++++++---- .../Match/MultiplayerReadyButton.cs | 10 +++--- .../Multiplayer/MultiplayerMatchSubScreen.cs | 19 +++++++++--- .../Screens/OnlinePlay/OnlinePlayScreen.cs | 2 +- 8 files changed, 79 insertions(+), 32 deletions(-) diff --git a/osu.Game.Tests/Visual/Multiplayer/TestSceneCreateMultiplayerMatchButton.cs b/osu.Game.Tests/Visual/Multiplayer/TestSceneCreateMultiplayerMatchButton.cs index 2a549e5262..381270c5aa 100644 --- a/osu.Game.Tests/Visual/Multiplayer/TestSceneCreateMultiplayerMatchButton.cs +++ b/osu.Game.Tests/Visual/Multiplayer/TestSceneCreateMultiplayerMatchButton.cs @@ -1,6 +1,7 @@ // 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 NUnit.Framework; using osu.Framework.Allocation; using osu.Framework.Graphics; @@ -12,7 +13,7 @@ namespace osu.Game.Tests.Visual.Multiplayer public class TestSceneCreateMultiplayerMatchButton : MultiplayerTestScene { [Cached] - private OngoingOperationTracker joiningRoomTracker = new OngoingOperationTracker(); + private OngoingOperationTracker ongoingOperationTracker = new OngoingOperationTracker(); private CreateMultiplayerMatchButton button; @@ -31,12 +32,14 @@ namespace osu.Game.Tests.Visual.Multiplayer [Test] public void TestButtonEnableStateChanges() { + IDisposable joiningRoomOperation = null; + assertButtonEnableState(true); - AddStep("begin joining room", () => joiningRoomTracker.BeginOperation()); + AddStep("begin joining room", () => joiningRoomOperation = ongoingOperationTracker.BeginOperation()); assertButtonEnableState(false); - AddStep("end joining room", () => joiningRoomTracker.EndOperation()); + AddStep("end joining room", () => joiningRoomOperation.Dispose()); assertButtonEnableState(true); AddStep("disconnect client", () => Client.Disconnect()); diff --git a/osu.Game.Tests/Visual/Multiplayer/TestSceneMultiplayerReadyButton.cs b/osu.Game.Tests/Visual/Multiplayer/TestSceneMultiplayerReadyButton.cs index 958c6d218b..de02c6c844 100644 --- a/osu.Game.Tests/Visual/Multiplayer/TestSceneMultiplayerReadyButton.cs +++ b/osu.Game.Tests/Visual/Multiplayer/TestSceneMultiplayerReadyButton.cs @@ -1,6 +1,7 @@ // 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 NUnit.Framework; using osu.Framework.Allocation; @@ -31,7 +32,7 @@ namespace osu.Game.Tests.Visual.Multiplayer private RulesetStore rulesets; [Cached] - private OngoingOperationTracker gameplayStartTracker = new OngoingOperationTracker(); + private OngoingOperationTracker ongoingOperationTracker = new OngoingOperationTracker(); [BackgroundDependencyLoader] private void load(GameHost host, AudioManager audio) @@ -167,11 +168,15 @@ namespace osu.Game.Tests.Visual.Multiplayer private void verifyGameplayStartFlow() { + IDisposable gameplayStartOperation = null; + + AddStep("hook up tracker", () => button.OnReady = () => gameplayStartOperation = ongoingOperationTracker.BeginOperation()); + addClickButtonStep(); AddAssert("user waiting for load", () => Client.Room?.Users[0].State == MultiplayerUserState.WaitingForLoad); AddAssert("ready button disabled", () => !button.ChildrenOfType().Single().Enabled.Value); - AddStep("transitioned to gameplay", () => gameplayStartTracker.EndOperation()); + AddStep("transitioned to gameplay", () => gameplayStartOperation.Dispose()); AddAssert("ready button enabled", () => button.ChildrenOfType().Single().Enabled.Value); } } diff --git a/osu.Game/Screens/OnlinePlay/Lounge/LoungeSubScreen.cs b/osu.Game/Screens/OnlinePlay/Lounge/LoungeSubScreen.cs index f25835f56a..2a16a62714 100644 --- a/osu.Game/Screens/OnlinePlay/Lounge/LoungeSubScreen.cs +++ b/osu.Game/Screens/OnlinePlay/Lounge/LoungeSubScreen.cs @@ -1,7 +1,10 @@ // 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.Diagnostics; using System.Linq; +using JetBrains.Annotations; using osu.Framework.Allocation; using osu.Framework.Bindables; using osu.Framework.Graphics; @@ -39,7 +42,10 @@ namespace osu.Game.Screens.OnlinePlay.Lounge private MusicController music { get; set; } [Resolved(CanBeNull = true)] - private OngoingOperationTracker joiningRoomTracker { get; set; } + private OngoingOperationTracker ongoingOperationTracker { get; set; } + + [CanBeNull] + private IDisposable joiningRoomOperation { get; set; } [BackgroundDependencyLoader] private void load() @@ -102,9 +108,9 @@ namespace osu.Game.Screens.OnlinePlay.Lounge initialRoomsReceived.BindTo(RoomManager.InitialRoomsReceived); initialRoomsReceived.BindValueChanged(_ => updateLoadingLayer()); - if (joiningRoomTracker != null) + if (ongoingOperationTracker != null) { - joiningRoom.BindTo(joiningRoomTracker.InProgress); + joiningRoom.BindTo(ongoingOperationTracker.InProgress); joiningRoom.BindValueChanged(_ => updateLoadingLayer(), true); } } @@ -164,15 +170,18 @@ namespace osu.Game.Screens.OnlinePlay.Lounge private void joinRequested(Room room) { - joiningRoomTracker?.BeginOperation(); + Debug.Assert(joiningRoomOperation == null); + joiningRoomOperation = ongoingOperationTracker?.BeginOperation(); RoomManager?.JoinRoom(room, r => { Open(room); - joiningRoomTracker?.EndOperation(); + joiningRoomOperation?.Dispose(); + joiningRoomOperation = null; }, _ => { - joiningRoomTracker?.EndOperation(); + joiningRoomOperation?.Dispose(); + joiningRoomOperation = null; }); } diff --git a/osu.Game/Screens/OnlinePlay/Multiplayer/CreateMultiplayerMatchButton.cs b/osu.Game/Screens/OnlinePlay/Multiplayer/CreateMultiplayerMatchButton.cs index 7518a4e71b..3785dfc29f 100644 --- a/osu.Game/Screens/OnlinePlay/Multiplayer/CreateMultiplayerMatchButton.cs +++ b/osu.Game/Screens/OnlinePlay/Multiplayer/CreateMultiplayerMatchButton.cs @@ -11,13 +11,13 @@ namespace osu.Game.Screens.OnlinePlay.Multiplayer public class CreateMultiplayerMatchButton : PurpleTriangleButton { private IBindable isConnected; - private IBindable joiningRoom; + private IBindable operationInProgress; [Resolved] private StatefulMultiplayerClient multiplayerClient { get; set; } [Resolved] - private OngoingOperationTracker joiningRoomTracker { get; set; } + private OngoingOperationTracker ongoingOperationTracker { get; set; } [BackgroundDependencyLoader] private void load() @@ -29,10 +29,10 @@ namespace osu.Game.Screens.OnlinePlay.Multiplayer isConnected = multiplayerClient.IsConnected.GetBoundCopy(); isConnected.BindValueChanged(_ => updateState()); - joiningRoom = joiningRoomTracker.InProgress.GetBoundCopy(); - joiningRoom.BindValueChanged(_ => updateState(), true); + operationInProgress = ongoingOperationTracker.InProgress.GetBoundCopy(); + operationInProgress.BindValueChanged(_ => updateState(), true); } - private void updateState() => Enabled.Value = isConnected.Value && !joiningRoom.Value; + private void updateState() => Enabled.Value = isConnected.Value && !operationInProgress.Value; } } diff --git a/osu.Game/Screens/OnlinePlay/Multiplayer/Match/MultiplayerMatchSettingsOverlay.cs b/osu.Game/Screens/OnlinePlay/Multiplayer/Match/MultiplayerMatchSettingsOverlay.cs index de25cff546..59b138123d 100644 --- a/osu.Game/Screens/OnlinePlay/Multiplayer/Match/MultiplayerMatchSettingsOverlay.cs +++ b/osu.Game/Screens/OnlinePlay/Multiplayer/Match/MultiplayerMatchSettingsOverlay.cs @@ -2,6 +2,8 @@ // See the LICENCE file in the repository root for full licence text. using System; +using System.Diagnostics; +using JetBrains.Annotations; using osu.Framework.Allocation; using osu.Framework.Bindables; using osu.Framework.Extensions.Color4Extensions; @@ -68,7 +70,13 @@ namespace osu.Game.Screens.OnlinePlay.Multiplayer.Match [Resolved] private Bindable ruleset { get; set; } - private readonly OngoingOperationTracker applyingSettingsTracker = new OngoingOperationTracker(); + [Resolved] + private OngoingOperationTracker ongoingOperationTracker { get; set; } + + private readonly IBindable operationInProgress = new BindableBool(); + + [CanBeNull] + private IDisposable applyingSettingsOperation; [BackgroundDependencyLoader] private void load(OsuColour colours) @@ -277,7 +285,8 @@ namespace osu.Game.Screens.OnlinePlay.Multiplayer.Match MaxParticipants.BindValueChanged(count => MaxParticipantsField.Text = count.NewValue?.ToString(), true); RoomID.BindValueChanged(roomId => initialBeatmapControl.Alpha = roomId.NewValue == null ? 1 : 0, true); - applyingSettingsTracker.InProgress.BindValueChanged(v => + operationInProgress.BindTo(ongoingOperationTracker.InProgress); + operationInProgress.BindValueChanged(v => { if (v.NewValue) loadingLayer.Show(); @@ -290,7 +299,7 @@ namespace osu.Game.Screens.OnlinePlay.Multiplayer.Match { base.Update(); - ApplyButton.Enabled.Value = Playlist.Count > 0 && NameField.Text.Length > 0 && !applyingSettingsTracker.InProgress.Value; + ApplyButton.Enabled.Value = Playlist.Count > 0 && NameField.Text.Length > 0 && !operationInProgress.Value; } private void apply() @@ -299,7 +308,9 @@ namespace osu.Game.Screens.OnlinePlay.Multiplayer.Match return; hideError(); - applyingSettingsTracker.BeginOperation(); + + Debug.Assert(applyingSettingsOperation == null); + applyingSettingsOperation = ongoingOperationTracker.BeginOperation(); // If the client is already in a room, update via the client. // Otherwise, update the room directly in preparation for it to be submitted to the API on match creation. @@ -332,15 +343,23 @@ namespace osu.Game.Screens.OnlinePlay.Multiplayer.Match private void onSuccess(Room room) { - applyingSettingsTracker.EndOperation(); + Debug.Assert(applyingSettingsOperation != null); + SettingsApplied?.Invoke(); + + applyingSettingsOperation.Dispose(); + applyingSettingsOperation = null; } private void onError(string text) { + Debug.Assert(applyingSettingsOperation != null); + ErrorText.Text = text; ErrorText.FadeIn(50); - applyingSettingsTracker.EndOperation(); + + applyingSettingsOperation.Dispose(); + applyingSettingsOperation = null; } } diff --git a/osu.Game/Screens/OnlinePlay/Multiplayer/Match/MultiplayerReadyButton.cs b/osu.Game/Screens/OnlinePlay/Multiplayer/Match/MultiplayerReadyButton.cs index 96470cf070..b145cd46c1 100644 --- a/osu.Game/Screens/OnlinePlay/Multiplayer/Match/MultiplayerReadyButton.cs +++ b/osu.Game/Screens/OnlinePlay/Multiplayer/Match/MultiplayerReadyButton.cs @@ -35,14 +35,14 @@ namespace osu.Game.Screens.OnlinePlay.Multiplayer.Match private OsuColour colours { get; set; } [Resolved] - private OngoingOperationTracker gameplayStartTracker { get; set; } + private OngoingOperationTracker ongoingOperationTracker { get; set; } private SampleChannel sampleReadyCount; private readonly ButtonWithTrianglesExposed button; private int countReady; - private IBindable gameplayStartInProgress; + private IBindable operationInProgress; public MultiplayerReadyButton() { @@ -59,8 +59,8 @@ namespace osu.Game.Screens.OnlinePlay.Multiplayer.Match { sampleReadyCount = audio.Samples.Get(@"SongSelect/select-difficulty"); - gameplayStartInProgress = gameplayStartTracker.InProgress.GetBoundCopy(); - gameplayStartInProgress.BindValueChanged(_ => updateState()); + operationInProgress = ongoingOperationTracker.InProgress.GetBoundCopy(); + operationInProgress.BindValueChanged(_ => updateState()); } protected override void OnRoomUpdated() @@ -105,7 +105,7 @@ namespace osu.Game.Screens.OnlinePlay.Multiplayer.Match break; } - button.Enabled.Value = Client.Room?.State == MultiplayerRoomState.Open && !gameplayStartInProgress.Value; + button.Enabled.Value = Client.Room?.State == MultiplayerRoomState.Open && !operationInProgress.Value; if (newCountReady != countReady) { diff --git a/osu.Game/Screens/OnlinePlay/Multiplayer/MultiplayerMatchSubScreen.cs b/osu.Game/Screens/OnlinePlay/Multiplayer/MultiplayerMatchSubScreen.cs index a87471c8af..b82bf508ce 100644 --- a/osu.Game/Screens/OnlinePlay/Multiplayer/MultiplayerMatchSubScreen.cs +++ b/osu.Game/Screens/OnlinePlay/Multiplayer/MultiplayerMatchSubScreen.cs @@ -1,9 +1,11 @@ // 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.Specialized; using System.Diagnostics; using System.Linq; +using JetBrains.Annotations; using osu.Framework.Allocation; using osu.Framework.Bindables; using osu.Framework.Graphics; @@ -32,13 +34,16 @@ namespace osu.Game.Screens.OnlinePlay.Multiplayer [Resolved] private StatefulMultiplayerClient client { get; set; } - [Cached] - private OngoingOperationTracker gameplayStartTracker = new OngoingOperationTracker(); + [Resolved] + private OngoingOperationTracker ongoingOperationTracker { get; set; } private MultiplayerMatchSettingsOverlay settingsOverlay; private IBindable isConnected; + [CanBeNull] + private IDisposable gameplayStartOperation; + public MultiplayerMatchSubScreen(Room room) { Title = room.RoomID.Value == null ? "New room" : room.Name.Value; @@ -217,7 +222,9 @@ namespace osu.Game.Screens.OnlinePlay.Multiplayer { if (client.Room?.Host?.Equals(localUser) == true) { - gameplayStartTracker.BeginOperation(); + Debug.Assert(gameplayStartOperation == null); + gameplayStartOperation = ongoingOperationTracker.BeginOperation(); + client.StartMatch().CatchUnobservedExceptions(true); } else @@ -232,7 +239,11 @@ namespace osu.Game.Screens.OnlinePlay.Multiplayer int[] userIds = client.CurrentMatchPlayingUserIds.ToArray(); StartPlay(() => new MultiplayerPlayer(SelectedItem.Value, userIds)); - gameplayStartTracker.EndOperation(); + + Debug.Assert(gameplayStartOperation != null); + + gameplayStartOperation.Dispose(); + gameplayStartOperation = null; } protected override void Dispose(bool isDisposing) diff --git a/osu.Game/Screens/OnlinePlay/OnlinePlayScreen.cs b/osu.Game/Screens/OnlinePlay/OnlinePlayScreen.cs index 92665c498b..13ebc1912e 100644 --- a/osu.Game/Screens/OnlinePlay/OnlinePlayScreen.cs +++ b/osu.Game/Screens/OnlinePlay/OnlinePlayScreen.cs @@ -53,7 +53,7 @@ namespace osu.Game.Screens.OnlinePlay private readonly Bindable currentFilter = new Bindable(new FilterCriteria()); [Cached] - private readonly OngoingOperationTracker joiningRoomTracker = new OngoingOperationTracker(); + private readonly OngoingOperationTracker ongoingOperationTracker = new OngoingOperationTracker(); [Resolved(CanBeNull = true)] private MusicController music { get; set; } From e9b0652359e9bcffed8c4e84f95a35f3397eef45 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bart=C5=82omiej=20Dach?= Date: Tue, 29 Dec 2020 09:09:47 +0100 Subject: [PATCH 10/33] Move ready-up operation logic again to client To salvage ready up button tests. --- .../TestSceneMultiplayerReadyButton.cs | 16 ++++++--- .../Multiplayer/StatefulMultiplayerClient.cs | 33 +++++++++++++++++++ .../Match/MultiplayerMatchFooter.cs | 4 +-- .../Match/MultiplayerReadyButton.cs | 2 +- .../Multiplayer/MultiplayerMatchSubScreen.cs | 33 ++++++++----------- 5 files changed, 61 insertions(+), 27 deletions(-) diff --git a/osu.Game.Tests/Visual/Multiplayer/TestSceneMultiplayerReadyButton.cs b/osu.Game.Tests/Visual/Multiplayer/TestSceneMultiplayerReadyButton.cs index de02c6c844..86398b205d 100644 --- a/osu.Game.Tests/Visual/Multiplayer/TestSceneMultiplayerReadyButton.cs +++ b/osu.Game.Tests/Visual/Multiplayer/TestSceneMultiplayerReadyButton.cs @@ -34,6 +34,8 @@ namespace osu.Game.Tests.Visual.Multiplayer [Cached] private OngoingOperationTracker ongoingOperationTracker = new OngoingOperationTracker(); + private IDisposable toggleReadyOperation; + [BackgroundDependencyLoader] private void load(GameHost host, AudioManager audio) { @@ -61,6 +63,14 @@ namespace osu.Game.Tests.Visual.Multiplayer Beatmap = { Value = beatmap }, Ruleset = { Value = beatmap.Ruleset } } + }, + OnToggleReady = async () => + { + toggleReadyOperation = ongoingOperationTracker.BeginOperation(); + + bool gameplayStarted = await Client.ToggleReady(); + if (!gameplayStarted) + toggleReadyOperation.Dispose(); } }; }); @@ -168,15 +178,11 @@ namespace osu.Game.Tests.Visual.Multiplayer private void verifyGameplayStartFlow() { - IDisposable gameplayStartOperation = null; - - AddStep("hook up tracker", () => button.OnReady = () => gameplayStartOperation = ongoingOperationTracker.BeginOperation()); - addClickButtonStep(); AddAssert("user waiting for load", () => Client.Room?.Users[0].State == MultiplayerUserState.WaitingForLoad); AddAssert("ready button disabled", () => !button.ChildrenOfType().Single().Enabled.Value); - AddStep("transitioned to gameplay", () => gameplayStartOperation.Dispose()); + AddStep("transitioned to gameplay", () => toggleReadyOperation.Dispose()); AddAssert("ready button enabled", () => button.ChildrenOfType().Single().Enabled.Value); } } diff --git a/osu.Game/Online/Multiplayer/StatefulMultiplayerClient.cs b/osu.Game/Online/Multiplayer/StatefulMultiplayerClient.cs index 0f44085134..ee42e56d75 100644 --- a/osu.Game/Online/Multiplayer/StatefulMultiplayerClient.cs +++ b/osu.Game/Online/Multiplayer/StatefulMultiplayerClient.cs @@ -183,6 +183,39 @@ namespace osu.Game.Online.Multiplayer }); } + /// + /// Toggles the 's ready state. + /// + /// true if this toggle triggered a gameplay start; false otherwise. + /// If a toggle of ready state is not valid at this time. + public async Task ToggleReady() + { + var localUser = LocalUser; + + if (localUser == null) + return false; + + switch (localUser.State) + { + case MultiplayerUserState.Idle: + await ChangeState(MultiplayerUserState.Ready); + return false; + + case MultiplayerUserState.Ready: + if (Room?.Host?.Equals(localUser) == true) + { + await StartMatch(); + return true; + } + + await ChangeState(MultiplayerUserState.Idle); + return false; + + default: + throw new InvalidOperationException($"Cannot toggle ready when in {localUser.State}"); + } + } + public abstract Task TransferHost(int userId); public abstract Task ChangeSettings(MultiplayerRoomSettings settings); diff --git a/osu.Game/Screens/OnlinePlay/Multiplayer/Match/MultiplayerMatchFooter.cs b/osu.Game/Screens/OnlinePlay/Multiplayer/Match/MultiplayerMatchFooter.cs index bd1ca1377e..a5e7650e31 100644 --- a/osu.Game/Screens/OnlinePlay/Multiplayer/Match/MultiplayerMatchFooter.cs +++ b/osu.Game/Screens/OnlinePlay/Multiplayer/Match/MultiplayerMatchFooter.cs @@ -20,9 +20,9 @@ namespace osu.Game.Screens.OnlinePlay.Multiplayer.Match public readonly Bindable SelectedItem = new Bindable(); - public Action OnReady + public Action OnToggleReady { - set => readyButton.OnReady = value; + set => readyButton.OnToggleReady = value; } private readonly Drawable background; diff --git a/osu.Game/Screens/OnlinePlay/Multiplayer/Match/MultiplayerReadyButton.cs b/osu.Game/Screens/OnlinePlay/Multiplayer/Match/MultiplayerReadyButton.cs index b145cd46c1..251b5b30ae 100644 --- a/osu.Game/Screens/OnlinePlay/Multiplayer/Match/MultiplayerReadyButton.cs +++ b/osu.Game/Screens/OnlinePlay/Multiplayer/Match/MultiplayerReadyButton.cs @@ -23,7 +23,7 @@ namespace osu.Game.Screens.OnlinePlay.Multiplayer.Match { public Bindable SelectedItem => button.SelectedItem; - public Action OnReady + public Action OnToggleReady { set => button.Action = value; } diff --git a/osu.Game/Screens/OnlinePlay/Multiplayer/MultiplayerMatchSubScreen.cs b/osu.Game/Screens/OnlinePlay/Multiplayer/MultiplayerMatchSubScreen.cs index b82bf508ce..88b4c8a1bf 100644 --- a/osu.Game/Screens/OnlinePlay/Multiplayer/MultiplayerMatchSubScreen.cs +++ b/osu.Game/Screens/OnlinePlay/Multiplayer/MultiplayerMatchSubScreen.cs @@ -162,7 +162,7 @@ namespace osu.Game.Screens.OnlinePlay.Multiplayer new MultiplayerMatchFooter { SelectedItem = { BindTarget = SelectedItem }, - OnReady = onReady + OnToggleReady = onToggleReady } } }, @@ -209,27 +209,22 @@ namespace osu.Game.Screens.OnlinePlay.Multiplayer private void onPlaylistChanged(object sender, NotifyCollectionChangedEventArgs e) => SelectedItem.Value = Playlist.FirstOrDefault(); - private void onReady() + private void onToggleReady() { - var localUser = client.LocalUser; + Debug.Assert(gameplayStartOperation == null); + gameplayStartOperation = ongoingOperationTracker.BeginOperation(); - if (localUser == null) - return; + client.ToggleReady() + .ContinueWith(t => + { + // if gameplay was started, the button will be unblocked on load requested. + if (t.Result) return; - if (localUser.State == MultiplayerUserState.Idle) - client.ChangeState(MultiplayerUserState.Ready).CatchUnobservedExceptions(true); - else - { - if (client.Room?.Host?.Equals(localUser) == true) - { - Debug.Assert(gameplayStartOperation == null); - gameplayStartOperation = ongoingOperationTracker.BeginOperation(); - - client.StartMatch().CatchUnobservedExceptions(true); - } - else - client.ChangeState(MultiplayerUserState.Idle).CatchUnobservedExceptions(true); - } + // gameplay was not started; unblock button. + gameplayStartOperation?.Dispose(); + gameplayStartOperation = null; + }) + .CatchUnobservedExceptions(); } private void onLoadRequested() From 274730de34336b646ca57027c6820922cd9fc0ff Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bart=C5=82omiej=20Dach?= Date: Tue, 29 Dec 2020 09:15:33 +0100 Subject: [PATCH 11/33] Cache tracker in test scene to resolve test fails --- .../Visual/Multiplayer/TestSceneMultiplayerMatchSubScreen.cs | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/osu.Game.Tests/Visual/Multiplayer/TestSceneMultiplayerMatchSubScreen.cs b/osu.Game.Tests/Visual/Multiplayer/TestSceneMultiplayerMatchSubScreen.cs index 8869718fd1..2344ebea0e 100644 --- a/osu.Game.Tests/Visual/Multiplayer/TestSceneMultiplayerMatchSubScreen.cs +++ b/osu.Game.Tests/Visual/Multiplayer/TestSceneMultiplayerMatchSubScreen.cs @@ -3,10 +3,12 @@ using System.Linq; using NUnit.Framework; +using osu.Framework.Allocation; using osu.Framework.Screens; using osu.Framework.Testing; using osu.Game.Online.Rooms; using osu.Game.Rulesets.Osu; +using osu.Game.Screens.OnlinePlay; using osu.Game.Screens.OnlinePlay.Multiplayer; using osu.Game.Screens.OnlinePlay.Multiplayer.Match; using osu.Game.Tests.Beatmaps; @@ -18,6 +20,9 @@ namespace osu.Game.Tests.Visual.Multiplayer { private MultiplayerMatchSubScreen screen; + [Cached] + private OngoingOperationTracker ongoingOperationTracker = new OngoingOperationTracker(); + public TestSceneMultiplayerMatchSubScreen() : base(false) { From cafa241ef3e7d73952956fda2ec83b04ef519da4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bart=C5=82omiej=20Dach?= Date: Tue, 29 Dec 2020 09:41:32 +0100 Subject: [PATCH 12/33] Fix ready-up button getting stuck if server operation fails --- .../OnlinePlay/Multiplayer/MultiplayerMatchSubScreen.cs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/osu.Game/Screens/OnlinePlay/Multiplayer/MultiplayerMatchSubScreen.cs b/osu.Game/Screens/OnlinePlay/Multiplayer/MultiplayerMatchSubScreen.cs index 88b4c8a1bf..5676bb22b4 100644 --- a/osu.Game/Screens/OnlinePlay/Multiplayer/MultiplayerMatchSubScreen.cs +++ b/osu.Game/Screens/OnlinePlay/Multiplayer/MultiplayerMatchSubScreen.cs @@ -11,7 +11,6 @@ using osu.Framework.Bindables; using osu.Framework.Graphics; using osu.Framework.Graphics.Containers; using osu.Framework.Screens; -using osu.Game.Extensions; using osu.Game.Online.Multiplayer; using osu.Game.Online.Rooms; using osu.Game.Screens.OnlinePlay.Components; @@ -218,13 +217,14 @@ namespace osu.Game.Screens.OnlinePlay.Multiplayer .ContinueWith(t => { // if gameplay was started, the button will be unblocked on load requested. - if (t.Result) return; + // accessing Exception here also silences any potential errors from the antecedent task + // (we still want to unblock the button if the ready-up fails). + if (t.Exception == null && t.Result) return; // gameplay was not started; unblock button. gameplayStartOperation?.Dispose(); gameplayStartOperation = null; - }) - .CatchUnobservedExceptions(); + }); } private void onLoadRequested() From 59f2017a13672be09b527873470d528717d4ae70 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bart=C5=82omiej=20Dach?= Date: Wed, 30 Dec 2020 16:22:11 +0100 Subject: [PATCH 13/33] Move BindValueChanged subscriptions to LoadComplete --- .../Multiplayer/CreateMultiplayerMatchButton.cs | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/osu.Game/Screens/OnlinePlay/Multiplayer/CreateMultiplayerMatchButton.cs b/osu.Game/Screens/OnlinePlay/Multiplayer/CreateMultiplayerMatchButton.cs index 3785dfc29f..87b0e49b5b 100644 --- a/osu.Game/Screens/OnlinePlay/Multiplayer/CreateMultiplayerMatchButton.cs +++ b/osu.Game/Screens/OnlinePlay/Multiplayer/CreateMultiplayerMatchButton.cs @@ -27,9 +27,14 @@ namespace osu.Game.Screens.OnlinePlay.Multiplayer Text = "Create room"; isConnected = multiplayerClient.IsConnected.GetBoundCopy(); - isConnected.BindValueChanged(_ => updateState()); - operationInProgress = ongoingOperationTracker.InProgress.GetBoundCopy(); + } + + protected override void LoadComplete() + { + base.LoadComplete(); + + isConnected.BindValueChanged(_ => updateState()); operationInProgress.BindValueChanged(_ => updateState(), true); } From dd87478690096232e4833dc69116006b1bf1e57e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bart=C5=82omiej=20Dach?= Date: Wed, 30 Dec 2020 16:29:19 +0100 Subject: [PATCH 14/33] Add helper IsHost property to Client --- .../Online/Multiplayer/StatefulMultiplayerClient.cs | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/osu.Game/Online/Multiplayer/StatefulMultiplayerClient.cs b/osu.Game/Online/Multiplayer/StatefulMultiplayerClient.cs index ee42e56d75..290765dc35 100644 --- a/osu.Game/Online/Multiplayer/StatefulMultiplayerClient.cs +++ b/osu.Game/Online/Multiplayer/StatefulMultiplayerClient.cs @@ -70,6 +70,18 @@ namespace osu.Game.Online.Multiplayer /// public MultiplayerRoomUser? LocalUser => Room?.Users.SingleOrDefault(u => u.User?.Id == api.LocalUser.Value.Id); + /// + /// Whether the is the host in . + /// + public bool IsHost + { + get + { + var localUser = LocalUser; + return localUser != null && Room?.Host != null && localUser.Equals(Room.Host); + } + } + [Resolved] private UserLookupCache userLookupCache { get; set; } = null!; From d34609b98ed925ccb181f2fc6d47101e3c1623a4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bart=C5=82omiej=20Dach?= Date: Wed, 30 Dec 2020 16:29:36 +0100 Subject: [PATCH 15/33] Rename On{ToggleReady -> ReadyClick} --- .../Visual/Multiplayer/TestSceneMultiplayerReadyButton.cs | 2 +- .../OnlinePlay/Multiplayer/Match/MultiplayerMatchFooter.cs | 4 ++-- .../OnlinePlay/Multiplayer/Match/MultiplayerReadyButton.cs | 2 +- .../OnlinePlay/Multiplayer/MultiplayerMatchSubScreen.cs | 2 +- 4 files changed, 5 insertions(+), 5 deletions(-) diff --git a/osu.Game.Tests/Visual/Multiplayer/TestSceneMultiplayerReadyButton.cs b/osu.Game.Tests/Visual/Multiplayer/TestSceneMultiplayerReadyButton.cs index 86398b205d..1d82b29a91 100644 --- a/osu.Game.Tests/Visual/Multiplayer/TestSceneMultiplayerReadyButton.cs +++ b/osu.Game.Tests/Visual/Multiplayer/TestSceneMultiplayerReadyButton.cs @@ -64,7 +64,7 @@ namespace osu.Game.Tests.Visual.Multiplayer Ruleset = { Value = beatmap.Ruleset } } }, - OnToggleReady = async () => + OnReadyClick = async () => { toggleReadyOperation = ongoingOperationTracker.BeginOperation(); diff --git a/osu.Game/Screens/OnlinePlay/Multiplayer/Match/MultiplayerMatchFooter.cs b/osu.Game/Screens/OnlinePlay/Multiplayer/Match/MultiplayerMatchFooter.cs index a5e7650e31..bbf861fac3 100644 --- a/osu.Game/Screens/OnlinePlay/Multiplayer/Match/MultiplayerMatchFooter.cs +++ b/osu.Game/Screens/OnlinePlay/Multiplayer/Match/MultiplayerMatchFooter.cs @@ -20,9 +20,9 @@ namespace osu.Game.Screens.OnlinePlay.Multiplayer.Match public readonly Bindable SelectedItem = new Bindable(); - public Action OnToggleReady + public Action OnReadyClick { - set => readyButton.OnToggleReady = value; + set => readyButton.OnReadyClick = value; } private readonly Drawable background; diff --git a/osu.Game/Screens/OnlinePlay/Multiplayer/Match/MultiplayerReadyButton.cs b/osu.Game/Screens/OnlinePlay/Multiplayer/Match/MultiplayerReadyButton.cs index 251b5b30ae..c6018a79e9 100644 --- a/osu.Game/Screens/OnlinePlay/Multiplayer/Match/MultiplayerReadyButton.cs +++ b/osu.Game/Screens/OnlinePlay/Multiplayer/Match/MultiplayerReadyButton.cs @@ -23,7 +23,7 @@ namespace osu.Game.Screens.OnlinePlay.Multiplayer.Match { public Bindable SelectedItem => button.SelectedItem; - public Action OnToggleReady + public Action OnReadyClick { set => button.Action = value; } diff --git a/osu.Game/Screens/OnlinePlay/Multiplayer/MultiplayerMatchSubScreen.cs b/osu.Game/Screens/OnlinePlay/Multiplayer/MultiplayerMatchSubScreen.cs index 5676bb22b4..b00ab065fb 100644 --- a/osu.Game/Screens/OnlinePlay/Multiplayer/MultiplayerMatchSubScreen.cs +++ b/osu.Game/Screens/OnlinePlay/Multiplayer/MultiplayerMatchSubScreen.cs @@ -161,7 +161,7 @@ namespace osu.Game.Screens.OnlinePlay.Multiplayer new MultiplayerMatchFooter { SelectedItem = { BindTarget = SelectedItem }, - OnToggleReady = onToggleReady + OnReadyClick = onToggleReady } } }, From f800448c87f163346a2ce4284ace782646816823 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bart=C5=82omiej=20Dach?= Date: Wed, 30 Dec 2020 18:00:57 +0100 Subject: [PATCH 16/33] Move game start logic to a higher level --- .../TestSceneMultiplayerReadyButton.cs | 17 ++++--- .../Multiplayer/StatefulMultiplayerClient.cs | 15 ++---- .../Multiplayer/MultiplayerMatchSubScreen.cs | 51 ++++++++++++------- 3 files changed, 48 insertions(+), 35 deletions(-) diff --git a/osu.Game.Tests/Visual/Multiplayer/TestSceneMultiplayerReadyButton.cs b/osu.Game.Tests/Visual/Multiplayer/TestSceneMultiplayerReadyButton.cs index 1d82b29a91..a6037dcbf2 100644 --- a/osu.Game.Tests/Visual/Multiplayer/TestSceneMultiplayerReadyButton.cs +++ b/osu.Game.Tests/Visual/Multiplayer/TestSceneMultiplayerReadyButton.cs @@ -34,7 +34,7 @@ namespace osu.Game.Tests.Visual.Multiplayer [Cached] private OngoingOperationTracker ongoingOperationTracker = new OngoingOperationTracker(); - private IDisposable toggleReadyOperation; + private IDisposable readyClickOperation; [BackgroundDependencyLoader] private void load(GameHost host, AudioManager audio) @@ -66,11 +66,16 @@ namespace osu.Game.Tests.Visual.Multiplayer }, OnReadyClick = async () => { - toggleReadyOperation = ongoingOperationTracker.BeginOperation(); + readyClickOperation = ongoingOperationTracker.BeginOperation(); - bool gameplayStarted = await Client.ToggleReady(); - if (!gameplayStarted) - toggleReadyOperation.Dispose(); + if (Client.IsHost && Client.LocalUser?.State == MultiplayerUserState.Ready) + { + await Client.StartMatch(); + return; + } + + await Client.ToggleReady(); + readyClickOperation.Dispose(); } }; }); @@ -182,7 +187,7 @@ namespace osu.Game.Tests.Visual.Multiplayer AddAssert("user waiting for load", () => Client.Room?.Users[0].State == MultiplayerUserState.WaitingForLoad); AddAssert("ready button disabled", () => !button.ChildrenOfType().Single().Enabled.Value); - AddStep("transitioned to gameplay", () => toggleReadyOperation.Dispose()); + AddStep("transitioned to gameplay", () => readyClickOperation.Dispose()); AddAssert("ready button enabled", () => button.ChildrenOfType().Single().Enabled.Value); } } diff --git a/osu.Game/Online/Multiplayer/StatefulMultiplayerClient.cs b/osu.Game/Online/Multiplayer/StatefulMultiplayerClient.cs index 290765dc35..5c6baa4fea 100644 --- a/osu.Game/Online/Multiplayer/StatefulMultiplayerClient.cs +++ b/osu.Game/Online/Multiplayer/StatefulMultiplayerClient.cs @@ -198,30 +198,23 @@ namespace osu.Game.Online.Multiplayer /// /// Toggles the 's ready state. /// - /// true if this toggle triggered a gameplay start; false otherwise. /// If a toggle of ready state is not valid at this time. - public async Task ToggleReady() + public async Task ToggleReady() { var localUser = LocalUser; if (localUser == null) - return false; + return; switch (localUser.State) { case MultiplayerUserState.Idle: await ChangeState(MultiplayerUserState.Ready); - return false; + return; case MultiplayerUserState.Ready: - if (Room?.Host?.Equals(localUser) == true) - { - await StartMatch(); - return true; - } - await ChangeState(MultiplayerUserState.Idle); - return false; + return; default: throw new InvalidOperationException($"Cannot toggle ready when in {localUser.State}"); diff --git a/osu.Game/Screens/OnlinePlay/Multiplayer/MultiplayerMatchSubScreen.cs b/osu.Game/Screens/OnlinePlay/Multiplayer/MultiplayerMatchSubScreen.cs index b00ab065fb..1bef8841f5 100644 --- a/osu.Game/Screens/OnlinePlay/Multiplayer/MultiplayerMatchSubScreen.cs +++ b/osu.Game/Screens/OnlinePlay/Multiplayer/MultiplayerMatchSubScreen.cs @@ -11,6 +11,7 @@ using osu.Framework.Bindables; using osu.Framework.Graphics; using osu.Framework.Graphics.Containers; using osu.Framework.Screens; +using osu.Game.Extensions; using osu.Game.Online.Multiplayer; using osu.Game.Online.Rooms; using osu.Game.Screens.OnlinePlay.Components; @@ -41,7 +42,7 @@ namespace osu.Game.Screens.OnlinePlay.Multiplayer private IBindable isConnected; [CanBeNull] - private IDisposable gameplayStartOperation; + private IDisposable readyClickOperation; public MultiplayerMatchSubScreen(Room room) { @@ -161,7 +162,7 @@ namespace osu.Game.Screens.OnlinePlay.Multiplayer new MultiplayerMatchFooter { SelectedItem = { BindTarget = SelectedItem }, - OnReadyClick = onToggleReady + OnReadyClick = onReadyClick } } }, @@ -208,23 +209,37 @@ namespace osu.Game.Screens.OnlinePlay.Multiplayer private void onPlaylistChanged(object sender, NotifyCollectionChangedEventArgs e) => SelectedItem.Value = Playlist.FirstOrDefault(); - private void onToggleReady() + private void onReadyClick() { - Debug.Assert(gameplayStartOperation == null); - gameplayStartOperation = ongoingOperationTracker.BeginOperation(); + Debug.Assert(readyClickOperation == null); + readyClickOperation = ongoingOperationTracker.BeginOperation(); + + if (client.IsHost && client.LocalUser?.State == MultiplayerUserState.Ready) + { + client.StartMatch() + .ContinueWith(t => + { + // accessing Exception here silences any potential errors from the antecedent task + if (t.Exception != null) + { + // gameplay was not started due to an exception; unblock button. + endOperation(); + } + + // gameplay is starting, the button will be unblocked on load requested. + }); + return; + } client.ToggleReady() - .ContinueWith(t => - { - // if gameplay was started, the button will be unblocked on load requested. - // accessing Exception here also silences any potential errors from the antecedent task - // (we still want to unblock the button if the ready-up fails). - if (t.Exception == null && t.Result) return; + .ContinueWith(_ => endOperation()) + .CatchUnobservedExceptions(); - // gameplay was not started; unblock button. - gameplayStartOperation?.Dispose(); - gameplayStartOperation = null; - }); + void endOperation() + { + readyClickOperation?.Dispose(); + readyClickOperation = null; + } } private void onLoadRequested() @@ -235,10 +250,10 @@ namespace osu.Game.Screens.OnlinePlay.Multiplayer StartPlay(() => new MultiplayerPlayer(SelectedItem.Value, userIds)); - Debug.Assert(gameplayStartOperation != null); + Debug.Assert(readyClickOperation != null); - gameplayStartOperation.Dispose(); - gameplayStartOperation = null; + readyClickOperation.Dispose(); + readyClickOperation = null; } protected override void Dispose(bool isDisposing) From 7d9a61fbc1b7afad59df89d3cf13cea4cce8be8b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bart=C5=82omiej=20Dach?= Date: Thu, 31 Dec 2020 11:50:59 +0100 Subject: [PATCH 17/33] Handle unobserved exceptions from ready button properly --- .../OnlinePlay/Multiplayer/MultiplayerMatchSubScreen.cs | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/osu.Game/Screens/OnlinePlay/Multiplayer/MultiplayerMatchSubScreen.cs b/osu.Game/Screens/OnlinePlay/Multiplayer/MultiplayerMatchSubScreen.cs index 1bef8841f5..39323d9db9 100644 --- a/osu.Game/Screens/OnlinePlay/Multiplayer/MultiplayerMatchSubScreen.cs +++ b/osu.Game/Screens/OnlinePlay/Multiplayer/MultiplayerMatchSubScreen.cs @@ -222,6 +222,7 @@ namespace osu.Game.Screens.OnlinePlay.Multiplayer // accessing Exception here silences any potential errors from the antecedent task if (t.Exception != null) { + t.CatchUnobservedExceptions(true); // will run immediately. // gameplay was not started due to an exception; unblock button. endOperation(); } @@ -232,8 +233,11 @@ namespace osu.Game.Screens.OnlinePlay.Multiplayer } client.ToggleReady() - .ContinueWith(_ => endOperation()) - .CatchUnobservedExceptions(); + .ContinueWith(t => + { + t.CatchUnobservedExceptions(true); // will run immediately. + endOperation(); + }); void endOperation() { From b4df2d6d43f96659c6687badc4c739770957ebfa Mon Sep 17 00:00:00 2001 From: Salman Ahmed Date: Fri, 1 Jan 2021 03:46:09 +0300 Subject: [PATCH 18/33] Add method for copying properties from another mod --- osu.Game/Rulesets/Mods/Mod.cs | 25 +++++++++++++++++-------- 1 file changed, 17 insertions(+), 8 deletions(-) diff --git a/osu.Game/Rulesets/Mods/Mod.cs b/osu.Game/Rulesets/Mods/Mod.cs index b8dc7a2661..236bf9ff00 100644 --- a/osu.Game/Rulesets/Mods/Mod.cs +++ b/osu.Game/Rulesets/Mods/Mod.cs @@ -131,22 +131,31 @@ namespace osu.Game.Rulesets.Mods /// public virtual Mod CreateCopy() { - var copy = (Mod)Activator.CreateInstance(GetType()); + var result = (Mod)Activator.CreateInstance(GetType(), true); + result.CopyFrom(this); + return result; + } + + /// + /// Copies properties of given mod into here, through changing value. + /// + /// The mod to copy properties from. + public void CopyFrom(Mod them) + { + if (them.GetType() != GetType()) + throw new ArgumentException($"Expected mod of type {GetType()}, got {them.GetType()}.", nameof(them)); - // Copy bindable values across foreach (var (_, prop) in this.GetSettingsSourceProperties()) { - var origBindable = prop.GetValue(this); - var copyBindable = prop.GetValue(copy); + var ourBindable = prop.GetValue(this); + var theirBindable = prop.GetValue(them); // The bindables themselves are readonly, so the value must be transferred through the Bindable.Value property. - var valueProperty = origBindable.GetType().GetProperty(nameof(Bindable.Value), BindingFlags.Public | BindingFlags.Instance); + var valueProperty = theirBindable.GetType().GetProperty(nameof(Bindable.Value), BindingFlags.Public | BindingFlags.Instance); Debug.Assert(valueProperty != null); - valueProperty.SetValue(copyBindable, valueProperty.GetValue(origBindable)); + valueProperty.SetValue(ourBindable, valueProperty.GetValue(theirBindable)); } - - return copy; } public bool Equals(IMod other) => GetType() == other?.GetType(); From 2ce9599957d50f5c9b8220cf7d3105372191a6d4 Mon Sep 17 00:00:00 2001 From: Salman Ahmed Date: Fri, 1 Jan 2021 03:47:13 +0300 Subject: [PATCH 19/33] Copy selected mods properties into overlay's buttons --- osu.Game/Overlays/Mods/ModSection.cs | 24 +++++++++++++++------- osu.Game/Overlays/Mods/ModSelectOverlay.cs | 2 +- 2 files changed, 18 insertions(+), 8 deletions(-) diff --git a/osu.Game/Overlays/Mods/ModSection.cs b/osu.Game/Overlays/Mods/ModSection.cs index 0107f94dcf..86da179064 100644 --- a/osu.Game/Overlays/Mods/ModSection.cs +++ b/osu.Game/Overlays/Mods/ModSection.cs @@ -127,18 +127,28 @@ namespace osu.Game.Overlays.Mods } /// - /// Select one or more mods in this section and deselects all other ones. + /// Updates all buttons with the given list of selected mods. /// - /// The types of s which should be selected. - public void SelectTypes(IEnumerable modTypes) + /// The types of s to select. + public void UpdateSelectedMods(IReadOnlyList newSelectedMods) { foreach (var button in buttons) { - int i = Array.FindIndex(button.Mods, m => modTypes.Any(t => t == m.GetType())); + int index = -1; - if (i >= 0) - button.SelectAt(i); - else + foreach (var mod in newSelectedMods) + { + index = Array.FindIndex(button.Mods, m1 => mod.GetType() == m1.GetType()); + if (index < 0) + continue; + + var buttonMod = button.Mods[index]; + buttonMod.CopyFrom(mod); + button.SelectAt(index); + break; + } + + if (index < 0) button.Deselect(); } } diff --git a/osu.Game/Overlays/Mods/ModSelectOverlay.cs b/osu.Game/Overlays/Mods/ModSelectOverlay.cs index 34f5c70adb..c1622548f5 100644 --- a/osu.Game/Overlays/Mods/ModSelectOverlay.cs +++ b/osu.Game/Overlays/Mods/ModSelectOverlay.cs @@ -409,7 +409,7 @@ namespace osu.Game.Overlays.Mods private void selectedModsChanged(ValueChangedEvent> mods) { foreach (var section in ModSectionsContainer.Children) - section.SelectTypes(mods.NewValue.Select(m => m.GetType()).ToList()); + section.UpdateSelectedMods(mods.NewValue); updateMods(); } From ee664ad57117fbdd01dd38a3025a621e69bf7eca Mon Sep 17 00:00:00 2001 From: Salman Ahmed Date: Fri, 1 Jan 2021 03:47:18 +0300 Subject: [PATCH 20/33] Add test coverage --- .../UserInterface/TestSceneModSelectOverlay.cs | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/osu.Game.Tests/Visual/UserInterface/TestSceneModSelectOverlay.cs b/osu.Game.Tests/Visual/UserInterface/TestSceneModSelectOverlay.cs index 6f083f4ab6..0d0acbb8f4 100644 --- a/osu.Game.Tests/Visual/UserInterface/TestSceneModSelectOverlay.cs +++ b/osu.Game.Tests/Visual/UserInterface/TestSceneModSelectOverlay.cs @@ -131,6 +131,18 @@ namespace osu.Game.Tests.Visual.UserInterface AddAssert("ensure mods not selected", () => modDisplay.Current.Value.Count == 0); } + [Test] + public void TestExternallySetCustomizedMod() + { + AddStep("set customized mod externally", () => SelectedMods.Value = new[] { new OsuModDoubleTime { SpeedChange = { Value = 1.01 } } }); + + AddAssert("ensure button is selected and customized accordingly", () => + { + var button = modSelect.GetModButton(SelectedMods.Value.Single()); + return ((OsuModDoubleTime)button.SelectedMod).SpeedChange.Value == 1.01; + }); + } + private void testSingleMod(Mod mod) { selectNext(mod); From a031c8e0b62f3933e23e049dc75633f6c4603aef Mon Sep 17 00:00:00 2001 From: Salman Ahmed Date: Fri, 1 Jan 2021 15:34:09 +0300 Subject: [PATCH 21/33] Apply documentation suggestions MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Bartłomiej Dach --- osu.Game/Overlays/Mods/ModSection.cs | 2 +- osu.Game/Rulesets/Mods/Mod.cs | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/osu.Game/Overlays/Mods/ModSection.cs b/osu.Game/Overlays/Mods/ModSection.cs index 86da179064..e29ed4f5c4 100644 --- a/osu.Game/Overlays/Mods/ModSection.cs +++ b/osu.Game/Overlays/Mods/ModSection.cs @@ -129,7 +129,7 @@ namespace osu.Game.Overlays.Mods /// /// Updates all buttons with the given list of selected mods. /// - /// The types of s to select. + /// The new list of selected mods to select. public void UpdateSelectedMods(IReadOnlyList newSelectedMods) { foreach (var button in buttons) diff --git a/osu.Game/Rulesets/Mods/Mod.cs b/osu.Game/Rulesets/Mods/Mod.cs index 236bf9ff00..6e83299d63 100644 --- a/osu.Game/Rulesets/Mods/Mod.cs +++ b/osu.Game/Rulesets/Mods/Mod.cs @@ -137,7 +137,7 @@ namespace osu.Game.Rulesets.Mods } /// - /// Copies properties of given mod into here, through changing value. + /// Copies mod setting values from into this instance. /// /// The mod to copy properties from. public void CopyFrom(Mod them) From c1a1e3acc5aa199442ce7a96058e1eaae2bc7dd9 Mon Sep 17 00:00:00 2001 From: Salman Ahmed Date: Fri, 1 Jan 2021 15:40:40 +0300 Subject: [PATCH 22/33] Revert drive-by changes --- osu.Game/Rulesets/Mods/Mod.cs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/osu.Game/Rulesets/Mods/Mod.cs b/osu.Game/Rulesets/Mods/Mod.cs index 6e83299d63..3f1f16e561 100644 --- a/osu.Game/Rulesets/Mods/Mod.cs +++ b/osu.Game/Rulesets/Mods/Mod.cs @@ -131,7 +131,7 @@ namespace osu.Game.Rulesets.Mods /// public virtual Mod CreateCopy() { - var result = (Mod)Activator.CreateInstance(GetType(), true); + var result = (Mod)Activator.CreateInstance(GetType()); result.CopyFrom(this); return result; } @@ -151,7 +151,7 @@ namespace osu.Game.Rulesets.Mods var theirBindable = prop.GetValue(them); // The bindables themselves are readonly, so the value must be transferred through the Bindable.Value property. - var valueProperty = theirBindable.GetType().GetProperty(nameof(Bindable.Value), BindingFlags.Public | BindingFlags.Instance); + var valueProperty = ourBindable.GetType().GetProperty(nameof(Bindable.Value), BindingFlags.Public | BindingFlags.Instance); Debug.Assert(valueProperty != null); valueProperty.SetValue(ourBindable, valueProperty.GetValue(theirBindable)); From 988f9b98a153d95b5f6de42dc3782d08912c125f Mon Sep 17 00:00:00 2001 From: Salman Ahmed Date: Fri, 1 Jan 2021 16:16:00 +0300 Subject: [PATCH 23/33] Split button mods updating to private method --- osu.Game/Overlays/Mods/ModSection.cs | 30 ++++++++++++++-------------- 1 file changed, 15 insertions(+), 15 deletions(-) diff --git a/osu.Game/Overlays/Mods/ModSection.cs b/osu.Game/Overlays/Mods/ModSection.cs index e29ed4f5c4..573d1e5355 100644 --- a/osu.Game/Overlays/Mods/ModSection.cs +++ b/osu.Game/Overlays/Mods/ModSection.cs @@ -133,24 +133,24 @@ namespace osu.Game.Overlays.Mods public void UpdateSelectedMods(IReadOnlyList newSelectedMods) { foreach (var button in buttons) + updateButtonMods(button, newSelectedMods); + } + + private void updateButtonMods(ModButton button, IReadOnlyList newSelectedMods) + { + foreach (var mod in newSelectedMods) { - int index = -1; - - foreach (var mod in newSelectedMods) - { - index = Array.FindIndex(button.Mods, m1 => mod.GetType() == m1.GetType()); - if (index < 0) - continue; - - var buttonMod = button.Mods[index]; - buttonMod.CopyFrom(mod); - button.SelectAt(index); - break; - } - + var index = Array.FindIndex(button.Mods, m1 => mod.GetType() == m1.GetType()); if (index < 0) - button.Deselect(); + continue; + + var buttonMod = button.Mods[index]; + buttonMod.CopyFrom(mod); + button.SelectAt(index); + return; } + + button.Deselect(); } protected ModSection() From 6c3ccaddbfec4a148ad6080e2ab6e958d54cd2e0 Mon Sep 17 00:00:00 2001 From: "dependabot-preview[bot]" <27856297+dependabot-preview[bot]@users.noreply.github.com> Date: Sat, 2 Jan 2021 12:51:41 +0000 Subject: [PATCH 24/33] Bump Microsoft.CodeAnalysis.FxCopAnalyzers from 3.3.1 to 3.3.2 Bumps [Microsoft.CodeAnalysis.FxCopAnalyzers](https://github.com/dotnet/roslyn-analyzers) from 3.3.1 to 3.3.2. - [Release notes](https://github.com/dotnet/roslyn-analyzers/releases) - [Changelog](https://github.com/dotnet/roslyn-analyzers/blob/master/PostReleaseActivities.md) - [Commits](https://github.com/dotnet/roslyn-analyzers/compare/v3.3.1...v3.3.2) Signed-off-by: dependabot-preview[bot] --- Directory.Build.props | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Directory.Build.props b/Directory.Build.props index 9ec442aafa..a74b204436 100644 --- a/Directory.Build.props +++ b/Directory.Build.props @@ -18,7 +18,7 @@ - + $(MSBuildThisFileDirectory)CodeAnalysis\osu.ruleset From 26b2a065bc365fc75e04315dfe9cbc71a3b0bb57 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bart=C5=82omiej=20Dach?= Date: Sat, 2 Jan 2021 14:25:15 +0100 Subject: [PATCH 25/33] Add deprecation warning ignore with explanation --- Directory.Build.props | 16 ++++++++++++---- 1 file changed, 12 insertions(+), 4 deletions(-) diff --git a/Directory.Build.props b/Directory.Build.props index a74b204436..b55eff9df9 100644 --- a/Directory.Build.props +++ b/Directory.Build.props @@ -28,9 +28,17 @@ $(NoWarn);CS1591 - - $(NoWarn);NU1701 + + $(NoWarn);NU1701;CA9998 false @@ -43,4 +51,4 @@ Copyright (c) 2020 ppy Pty Ltd osu game - \ No newline at end of file + From edd328c8fecdb2566c8f68f087f2c8089219bb77 Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Fri, 8 Jan 2021 17:24:55 +0900 Subject: [PATCH 26/33] Move bindable closer to source class --- .../OnlinePlay/Multiplayer/Match/MultiplayerReadyButton.cs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/osu.Game/Screens/OnlinePlay/Multiplayer/Match/MultiplayerReadyButton.cs b/osu.Game/Screens/OnlinePlay/Multiplayer/Match/MultiplayerReadyButton.cs index c6018a79e9..04030cdbfd 100644 --- a/osu.Game/Screens/OnlinePlay/Multiplayer/Match/MultiplayerReadyButton.cs +++ b/osu.Game/Screens/OnlinePlay/Multiplayer/Match/MultiplayerReadyButton.cs @@ -37,12 +37,13 @@ namespace osu.Game.Screens.OnlinePlay.Multiplayer.Match [Resolved] private OngoingOperationTracker ongoingOperationTracker { get; set; } + private IBindable operationInProgress; + private SampleChannel sampleReadyCount; private readonly ButtonWithTrianglesExposed button; private int countReady; - private IBindable operationInProgress; public MultiplayerReadyButton() { From 284d30d336d0d7b851481a084c3067c45ff3cfdc Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bart=C5=82omiej=20Dach?= Date: Fri, 8 Jan 2021 21:13:36 +0100 Subject: [PATCH 27/33] Move screen activity update to LoadComplete() Fixes a potential crash when moving from main menu to editor after having previously opened the login settings overlay. Setting the activity in BDL as done before is unsafe, as that set can trigger value change callbacks, which in turn can trigger adding transforms, which should always be done on the update thread. Semantically it also makes sense, as the user activity should change once the screen they're moving to has actually loaded and displayed to the user. --- osu.Game/Screens/OsuScreen.cs | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/osu.Game/Screens/OsuScreen.cs b/osu.Game/Screens/OsuScreen.cs index e1a29946f4..9b716b323d 100644 --- a/osu.Game/Screens/OsuScreen.cs +++ b/osu.Game/Screens/OsuScreen.cs @@ -143,7 +143,11 @@ namespace osu.Game.Screens private void load(OsuGame osu, AudioManager audio) { sampleExit = audio.Samples.Get(@"UI/screen-back"); + } + protected override void LoadComplete() + { + base.LoadComplete(); Activity.Value ??= InitialActivity; } From dad5dd36676de0c965c645b2564e496adb334908 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bart=C5=82omiej=20Dach?= Date: Fri, 8 Jan 2021 22:17:37 +0100 Subject: [PATCH 28/33] Remove unnecessary permissiveness wrt null --- .../OnlinePlay/Multiplayer/MultiplayerMatchSubScreen.cs | 3 ++- osu.Game/Screens/OnlinePlay/OngoingOperationTracker.cs | 9 ++++----- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/osu.Game/Screens/OnlinePlay/Multiplayer/MultiplayerMatchSubScreen.cs b/osu.Game/Screens/OnlinePlay/Multiplayer/MultiplayerMatchSubScreen.cs index 39323d9db9..e539b315e4 100644 --- a/osu.Game/Screens/OnlinePlay/Multiplayer/MultiplayerMatchSubScreen.cs +++ b/osu.Game/Screens/OnlinePlay/Multiplayer/MultiplayerMatchSubScreen.cs @@ -241,7 +241,8 @@ namespace osu.Game.Screens.OnlinePlay.Multiplayer void endOperation() { - readyClickOperation?.Dispose(); + Debug.Assert(readyClickOperation != null); + readyClickOperation.Dispose(); readyClickOperation = null; } } diff --git a/osu.Game/Screens/OnlinePlay/OngoingOperationTracker.cs b/osu.Game/Screens/OnlinePlay/OngoingOperationTracker.cs index 6a340d7954..c34d39136e 100644 --- a/osu.Game/Screens/OnlinePlay/OngoingOperationTracker.cs +++ b/osu.Game/Screens/OnlinePlay/OngoingOperationTracker.cs @@ -40,13 +40,12 @@ namespace osu.Game.Screens.OnlinePlay return new InvokeOnDisposal(endOperation); } - /// - /// Ends tracking an online operation. - /// Does nothing if an operation has not been begun yet. - /// private void endOperation() { - leasedInProgress?.Return(); + if (leasedInProgress == null) + throw new InvalidOperationException("Cannot end operation multiple times."); + + leasedInProgress.Return(); leasedInProgress = null; } } From c2eeb822b84d007beec6190cda094ca3ea1ba062 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bart=C5=82omiej=20Dach?= Date: Fri, 8 Jan 2021 22:23:38 +0100 Subject: [PATCH 29/33] Rename {joiningRoom -> operationInProgress} --- osu.Game/Screens/OnlinePlay/Lounge/LoungeSubScreen.cs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/osu.Game/Screens/OnlinePlay/Lounge/LoungeSubScreen.cs b/osu.Game/Screens/OnlinePlay/Lounge/LoungeSubScreen.cs index 2a16a62714..9b4e78543c 100644 --- a/osu.Game/Screens/OnlinePlay/Lounge/LoungeSubScreen.cs +++ b/osu.Game/Screens/OnlinePlay/Lounge/LoungeSubScreen.cs @@ -29,7 +29,7 @@ namespace osu.Game.Screens.OnlinePlay.Lounge protected override UserActivity InitialActivity => new UserActivity.SearchingForLobby(); private readonly IBindable initialRoomsReceived = new Bindable(); - private readonly IBindable joiningRoom = new Bindable(); + private readonly IBindable operationInProgress = new Bindable(); private FilterControl filter; private Container content; @@ -110,8 +110,8 @@ namespace osu.Game.Screens.OnlinePlay.Lounge if (ongoingOperationTracker != null) { - joiningRoom.BindTo(ongoingOperationTracker.InProgress); - joiningRoom.BindValueChanged(_ => updateLoadingLayer(), true); + operationInProgress.BindTo(ongoingOperationTracker.InProgress); + operationInProgress.BindValueChanged(_ => updateLoadingLayer(), true); } } @@ -187,7 +187,7 @@ namespace osu.Game.Screens.OnlinePlay.Lounge private void updateLoadingLayer() { - if (joiningRoom.Value || !initialRoomsReceived.Value) + if (operationInProgress.Value || !initialRoomsReceived.Value) loadingLayer.Show(); else loadingLayer.Hide(); From ff60d652ed6094054125990375dc9edc8ea05313 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bart=C5=82omiej=20Dach?= Date: Fri, 8 Jan 2021 22:28:21 +0100 Subject: [PATCH 30/33] Move out test ongoing operation tracker to higher level --- .../Visual/Multiplayer/TestSceneMultiplayerReadyButton.cs | 6 +----- osu.Game/Tests/Visual/Multiplayer/MultiplayerTestScene.cs | 3 +++ 2 files changed, 4 insertions(+), 5 deletions(-) diff --git a/osu.Game.Tests/Visual/Multiplayer/TestSceneMultiplayerReadyButton.cs b/osu.Game.Tests/Visual/Multiplayer/TestSceneMultiplayerReadyButton.cs index a6037dcbf2..8b9bffcee1 100644 --- a/osu.Game.Tests/Visual/Multiplayer/TestSceneMultiplayerReadyButton.cs +++ b/osu.Game.Tests/Visual/Multiplayer/TestSceneMultiplayerReadyButton.cs @@ -15,7 +15,6 @@ using osu.Game.Graphics.UserInterface; using osu.Game.Online.Multiplayer; using osu.Game.Online.Rooms; using osu.Game.Rulesets; -using osu.Game.Screens.OnlinePlay; using osu.Game.Screens.OnlinePlay.Multiplayer.Match; using osu.Game.Tests.Resources; using osu.Game.Users; @@ -31,9 +30,6 @@ namespace osu.Game.Tests.Visual.Multiplayer private BeatmapManager beatmaps; private RulesetStore rulesets; - [Cached] - private OngoingOperationTracker ongoingOperationTracker = new OngoingOperationTracker(); - private IDisposable readyClickOperation; [BackgroundDependencyLoader] @@ -66,7 +62,7 @@ namespace osu.Game.Tests.Visual.Multiplayer }, OnReadyClick = async () => { - readyClickOperation = ongoingOperationTracker.BeginOperation(); + readyClickOperation = OngoingOperationTracker.BeginOperation(); if (Client.IsHost && Client.LocalUser?.State == MultiplayerUserState.Ready) { diff --git a/osu.Game/Tests/Visual/Multiplayer/MultiplayerTestScene.cs b/osu.Game/Tests/Visual/Multiplayer/MultiplayerTestScene.cs index da0e39d965..75ddb34685 100644 --- a/osu.Game/Tests/Visual/Multiplayer/MultiplayerTestScene.cs +++ b/osu.Game/Tests/Visual/Multiplayer/MultiplayerTestScene.cs @@ -23,6 +23,9 @@ namespace osu.Game.Tests.Visual.Multiplayer [Cached] public Bindable Filter { get; } + [Cached] + public OngoingOperationTracker OngoingOperationTracker { get; } = new OngoingOperationTracker(); + protected override Container Content => content; private readonly TestMultiplayerRoomContainer content; From 0aad0c7c6c5dd363bd5bb75d2eb4cc5c35a3804f Mon Sep 17 00:00:00 2001 From: Salman Ahmed Date: Sat, 9 Jan 2021 00:30:12 +0300 Subject: [PATCH 31/33] Target logic at `this` and adjust variables --- osu.Game/Rulesets/Mods/Mod.cs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/osu.Game/Rulesets/Mods/Mod.cs b/osu.Game/Rulesets/Mods/Mod.cs index 7fe9a06597..e72e9a004f 100644 --- a/osu.Game/Rulesets/Mods/Mod.cs +++ b/osu.Game/Rulesets/Mods/Mod.cs @@ -144,12 +144,12 @@ namespace osu.Game.Rulesets.Mods foreach (var (_, prop) in this.GetSettingsSourceProperties()) { - var origBindable = (IBindable)prop.GetValue(this); - var copyBindable = (IBindable)prop.GetValue(copy); + var targetBindable = (IBindable)prop.GetValue(this); + var sourceBindable = (IBindable)prop.GetValue(them); // we only care about changes that have been made away from defaults. - if (!origBindable.IsDefault) - copy.CopyAdjustedSetting(copyBindable, origBindable); + if (!sourceBindable.IsDefault) + CopyAdjustedSetting(targetBindable, sourceBindable); } } From 8c3955d34136026b2387864223b065c7f57b185e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bart=C5=82omiej=20Dach?= Date: Sat, 9 Jan 2021 21:38:20 +0100 Subject: [PATCH 32/33] Improve safety of ongoing operation tracker Finishing an operation started via `OngoingOperationTracker.BeginOperation()` was risky in cases where the operation ended at a callback on another thread (which, in the case of multiplayer, is *most* cases). In particular, if any consumer registered a callback that mutates transforms when the operation ends, it would result in crashes after the framework-side safety checks. Rework `OngoingOperationTracker` into an always-present component residing in the drawable hierarchy, and ensure that the `operationInProgress` bindable is always updated on the update thread. This way consumers don't have to add local schedules in multiple places. --- .../TestSceneCreateMultiplayerMatchButton.cs | 7 +------ .../Screens/OnlinePlay/OngoingOperationTracker.cs | 11 +++++++++-- osu.Game/Screens/OnlinePlay/OnlinePlayScreen.cs | 5 +++-- .../Tests/Visual/Multiplayer/MultiplayerTestScene.cs | 3 ++- .../Multiplayer/TestMultiplayerRoomContainer.cs | 4 ++++ 5 files changed, 19 insertions(+), 11 deletions(-) diff --git a/osu.Game.Tests/Visual/Multiplayer/TestSceneCreateMultiplayerMatchButton.cs b/osu.Game.Tests/Visual/Multiplayer/TestSceneCreateMultiplayerMatchButton.cs index 381270c5aa..2f0398c6ef 100644 --- a/osu.Game.Tests/Visual/Multiplayer/TestSceneCreateMultiplayerMatchButton.cs +++ b/osu.Game.Tests/Visual/Multiplayer/TestSceneCreateMultiplayerMatchButton.cs @@ -3,18 +3,13 @@ using System; using NUnit.Framework; -using osu.Framework.Allocation; using osu.Framework.Graphics; -using osu.Game.Screens.OnlinePlay; using osu.Game.Screens.OnlinePlay.Multiplayer; namespace osu.Game.Tests.Visual.Multiplayer { public class TestSceneCreateMultiplayerMatchButton : MultiplayerTestScene { - [Cached] - private OngoingOperationTracker ongoingOperationTracker = new OngoingOperationTracker(); - private CreateMultiplayerMatchButton button; public override void SetUpSteps() @@ -36,7 +31,7 @@ namespace osu.Game.Tests.Visual.Multiplayer assertButtonEnableState(true); - AddStep("begin joining room", () => joiningRoomOperation = ongoingOperationTracker.BeginOperation()); + AddStep("begin joining room", () => joiningRoomOperation = OngoingOperationTracker.BeginOperation()); assertButtonEnableState(false); AddStep("end joining room", () => joiningRoomOperation.Dispose()); diff --git a/osu.Game/Screens/OnlinePlay/OngoingOperationTracker.cs b/osu.Game/Screens/OnlinePlay/OngoingOperationTracker.cs index c34d39136e..5c9e9ce90b 100644 --- a/osu.Game/Screens/OnlinePlay/OngoingOperationTracker.cs +++ b/osu.Game/Screens/OnlinePlay/OngoingOperationTracker.cs @@ -4,6 +4,7 @@ using System; using osu.Framework.Allocation; using osu.Framework.Bindables; +using osu.Framework.Graphics; namespace osu.Game.Screens.OnlinePlay { @@ -11,7 +12,7 @@ namespace osu.Game.Screens.OnlinePlay /// Utility class to track ongoing online operations' progress. /// Can be used to disable interactivity while waiting for a response from online sources. /// - public class OngoingOperationTracker + public class OngoingOperationTracker : Component { /// /// Whether there is an online operation in progress. @@ -22,6 +23,11 @@ namespace osu.Game.Screens.OnlinePlay private LeasedBindable leasedInProgress; + public OngoingOperationTracker() + { + AlwaysPresent = true; + } + /// /// Begins tracking a new online operation. /// @@ -37,7 +43,8 @@ namespace osu.Game.Screens.OnlinePlay leasedInProgress = inProgress.BeginLease(true); leasedInProgress.Value = true; - return new InvokeOnDisposal(endOperation); + // for extra safety, marshal the end of operation back to the update thread if necessary. + return new InvokeOnDisposal(() => Scheduler.Add(endOperation, false)); } private void endOperation() diff --git a/osu.Game/Screens/OnlinePlay/OnlinePlayScreen.cs b/osu.Game/Screens/OnlinePlay/OnlinePlayScreen.cs index b73e0a7c52..71fd0d5c76 100644 --- a/osu.Game/Screens/OnlinePlay/OnlinePlayScreen.cs +++ b/osu.Game/Screens/OnlinePlay/OnlinePlayScreen.cs @@ -54,7 +54,7 @@ namespace osu.Game.Screens.OnlinePlay private readonly Bindable currentFilter = new Bindable(new FilterCriteria()); [Cached] - private readonly OngoingOperationTracker ongoingOperationTracker = new OngoingOperationTracker(); + private OngoingOperationTracker ongoingOperationTracker { get; set; } [Resolved(CanBeNull = true)] private MusicController music { get; set; } @@ -144,7 +144,8 @@ namespace osu.Game.Screens.OnlinePlay }; button.Action = () => OpenNewRoom(); }), - RoomManager = CreateRoomManager() + RoomManager = CreateRoomManager(), + ongoingOperationTracker = new OngoingOperationTracker() } }; diff --git a/osu.Game/Tests/Visual/Multiplayer/MultiplayerTestScene.cs b/osu.Game/Tests/Visual/Multiplayer/MultiplayerTestScene.cs index 75ddb34685..a87b22affe 100644 --- a/osu.Game/Tests/Visual/Multiplayer/MultiplayerTestScene.cs +++ b/osu.Game/Tests/Visual/Multiplayer/MultiplayerTestScene.cs @@ -24,7 +24,7 @@ namespace osu.Game.Tests.Visual.Multiplayer public Bindable Filter { get; } [Cached] - public OngoingOperationTracker OngoingOperationTracker { get; } = new OngoingOperationTracker(); + public OngoingOperationTracker OngoingOperationTracker { get; } protected override Container Content => content; private readonly TestMultiplayerRoomContainer content; @@ -39,6 +39,7 @@ namespace osu.Game.Tests.Visual.Multiplayer Client = content.Client; RoomManager = content.RoomManager; Filter = content.Filter; + OngoingOperationTracker = content.OngoingOperationTracker; } [SetUp] diff --git a/osu.Game/Tests/Visual/Multiplayer/TestMultiplayerRoomContainer.cs b/osu.Game/Tests/Visual/Multiplayer/TestMultiplayerRoomContainer.cs index ad3e2f7105..860caef071 100644 --- a/osu.Game/Tests/Visual/Multiplayer/TestMultiplayerRoomContainer.cs +++ b/osu.Game/Tests/Visual/Multiplayer/TestMultiplayerRoomContainer.cs @@ -25,6 +25,9 @@ namespace osu.Game.Tests.Visual.Multiplayer [Cached] public readonly Bindable Filter = new Bindable(new FilterCriteria()); + [Cached] + public readonly OngoingOperationTracker OngoingOperationTracker; + public TestMultiplayerRoomContainer() { RelativeSizeAxes = Axes.Both; @@ -33,6 +36,7 @@ namespace osu.Game.Tests.Visual.Multiplayer { Client = new TestMultiplayerClient(), RoomManager = new TestMultiplayerRoomManager(), + OngoingOperationTracker = new OngoingOperationTracker(), content = new Container { RelativeSizeAxes = Axes.Both } }); } From 4b4adc927cb49a166f9f13c093c318afbbe18f2b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bart=C5=82omiej=20Dach?= Date: Sun, 10 Jan 2021 15:35:53 +0100 Subject: [PATCH 33/33] Rename param to match method body --- osu.Game/Rulesets/Mods/Mod.cs | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/osu.Game/Rulesets/Mods/Mod.cs b/osu.Game/Rulesets/Mods/Mod.cs index e72e9a004f..3a8717e678 100644 --- a/osu.Game/Rulesets/Mods/Mod.cs +++ b/osu.Game/Rulesets/Mods/Mod.cs @@ -134,18 +134,18 @@ namespace osu.Game.Rulesets.Mods } /// - /// Copies mod setting values from into this instance. + /// Copies mod setting values from into this instance. /// - /// The mod to copy properties from. - public void CopyFrom(Mod them) + /// The mod to copy properties from. + public void CopyFrom(Mod source) { - if (them.GetType() != GetType()) - throw new ArgumentException($"Expected mod of type {GetType()}, got {them.GetType()}.", nameof(them)); + if (source.GetType() != GetType()) + throw new ArgumentException($"Expected mod of type {GetType()}, got {source.GetType()}.", nameof(source)); foreach (var (_, prop) in this.GetSettingsSourceProperties()) { var targetBindable = (IBindable)prop.GetValue(this); - var sourceBindable = (IBindable)prop.GetValue(them); + var sourceBindable = (IBindable)prop.GetValue(source); // we only care about changes that have been made away from defaults. if (!sourceBindable.IsDefault)