From 133a1bc1fada7b2ca93cbcd9c4e9e7385a7907c8 Mon Sep 17 00:00:00 2001 From: Dan Balasescu Date: Fri, 28 Mar 2025 21:26:14 +0900 Subject: [PATCH 01/12] Add test for changing item in list Brought forward from https://github.com/ppy/osu/pull/32250. --- .../TestSceneMultiplayerQueueList.cs | 18 ++++++++++++++++-- 1 file changed, 16 insertions(+), 2 deletions(-) diff --git a/osu.Game.Tests/Visual/Multiplayer/TestSceneMultiplayerQueueList.cs b/osu.Game.Tests/Visual/Multiplayer/TestSceneMultiplayerQueueList.cs index 7283e3a1fe..d7659351bb 100644 --- a/osu.Game.Tests/Visual/Multiplayer/TestSceneMultiplayerQueueList.cs +++ b/osu.Game.Tests/Visual/Multiplayer/TestSceneMultiplayerQueueList.cs @@ -49,13 +49,15 @@ namespace osu.Game.Tests.Visual.Multiplayer AddStep("create playlist", () => { - Child = playlist = new MultiplayerQueueList(room) + Child = playlist = new MultiplayerQueueList { Anchor = Anchor.Centre, Origin = Anchor.Centre, - Size = new Vector2(500, 300), + Size = new Vector2(500, 300) }; + playlist.Items.ReplaceRange(0, playlist.Items.Count, MultiplayerClient.ClientAPIRoom!.Playlist); + MultiplayerClient.ClientAPIRoom!.PropertyChanged += (_, e) => { if (e.PropertyName == nameof(Room.Playlist)) @@ -132,6 +134,18 @@ namespace osu.Game.Tests.Visual.Multiplayer assertDeleteButtonVisibility(1, false); } + [Test] + public void TestChangeExistingItem() + { + AddStep("change beatmap", () => MultiplayerClient.EditPlaylistItem(new MultiplayerPlaylistItem + { + ID = playlist.Items[0].ID, + BeatmapID = 1337 + }).WaitSafely()); + + AddUntilStep("first playlist item has new beatmap", () => playlist.Items[0].Beatmap.OnlineID, () => Is.EqualTo(1337)); + } + private void addPlaylistItem(Func userId) { long itemId = -1; From 67eb691c8310620b629211626ec5425de9b2538f Mon Sep 17 00:00:00 2001 From: Dan Balasescu Date: Fri, 28 Mar 2025 21:49:32 +0900 Subject: [PATCH 02/12] Fix room ids not being incremented in tests Tests will [preserve](https://github.com/ppy/osu/blob/f4c96ecb54ff7650e617597faa583f1f40ce323b/osu.Game/Tests/Visual/OnlinePlay/TestRoomRequestsHandler.cs#L270) the incoming `RoomId` value if non-null. But since now everything goes through `MultiplayerClient.CreateRoom(MultiplayerRoom)` which internally creates a representative `Room` to return in requests, this would previously return a `Room` with `RoomId == 0` and not the expected `null` value. The only consumer of this outside of tests is [server-spectator](https://github.com/ppy/osu-server-spectator/blob/c1f33672cc2245614bdcfeb3109801b20a89c709/osu.Server.Spectator/Services/SharedInterop.cs#L147), but only in the context of creating rooms at which point RoomId is irrelevant. --- osu.Game/Online/Rooms/Room.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/osu.Game/Online/Rooms/Room.cs b/osu.Game/Online/Rooms/Room.cs index e965f9c187..b93917eff6 100644 --- a/osu.Game/Online/Rooms/Room.cs +++ b/osu.Game/Online/Rooms/Room.cs @@ -348,7 +348,7 @@ namespace osu.Game.Online.Rooms public Room(MultiplayerRoom room) { - RoomID = room.RoomID; + RoomID = room.RoomID > 0 ? room.RoomID : null; Name = room.Settings.Name; Password = room.Settings.Password; Type = room.Settings.MatchType; From b93e21c6667acfbf51e88ac2a0f62da58daf20a3 Mon Sep 17 00:00:00 2001 From: Dan Balasescu Date: Fri, 28 Mar 2025 21:51:30 +0900 Subject: [PATCH 03/12] Refactor `MultiplayerPlaylist` to not have a selected item bindable --- .../TestSceneMultiplayerPlaylist.cs | 6 +- .../Match/Playlist/MultiplayerPlaylist.cs | 61 ++++++++----------- .../Match/Playlist/MultiplayerQueueList.cs | 35 +---------- .../Multiplayer/MultiplayerMatchSubScreen.cs | 5 +- 4 files changed, 31 insertions(+), 76 deletions(-) diff --git a/osu.Game.Tests/Visual/Multiplayer/TestSceneMultiplayerPlaylist.cs b/osu.Game.Tests/Visual/Multiplayer/TestSceneMultiplayerPlaylist.cs index 1affa08813..1bde02762e 100644 --- a/osu.Game.Tests/Visual/Multiplayer/TestSceneMultiplayerPlaylist.cs +++ b/osu.Game.Tests/Visual/Multiplayer/TestSceneMultiplayerPlaylist.cs @@ -6,7 +6,6 @@ using System.Linq; using NUnit.Framework; using osu.Framework.Allocation; using osu.Framework.Audio; -using osu.Framework.Bindables; using osu.Framework.Extensions; using osu.Framework.Graphics; using osu.Framework.Platform; @@ -53,13 +52,12 @@ namespace osu.Game.Tests.Visual.Multiplayer AddStep("create list", () => { - Child = list = new MultiplayerPlaylist(room) + Child = list = new MultiplayerPlaylist { Anchor = Anchor.Centre, Origin = Anchor.Centre, RelativeSizeAxes = Axes.Both, - Size = new Vector2(0.4f, 0.8f), - SelectedItem = new Bindable() + Size = new Vector2(0.4f, 0.8f) }; }); diff --git a/osu.Game/Screens/OnlinePlay/Multiplayer/Match/Playlist/MultiplayerPlaylist.cs b/osu.Game/Screens/OnlinePlay/Multiplayer/Match/Playlist/MultiplayerPlaylist.cs index 9feee0ae41..8f59db467d 100644 --- a/osu.Game/Screens/OnlinePlay/Multiplayer/Match/Playlist/MultiplayerPlaylist.cs +++ b/osu.Game/Screens/OnlinePlay/Multiplayer/Match/Playlist/MultiplayerPlaylist.cs @@ -7,6 +7,7 @@ using osu.Framework.Allocation; using osu.Framework.Bindables; using osu.Framework.Graphics; using osu.Framework.Graphics.Containers; +using osu.Game.Online.API.Requests.Responses; using osu.Game.Online.Multiplayer; using osu.Game.Online.Rooms; @@ -19,12 +20,6 @@ namespace osu.Game.Screens.OnlinePlay.Multiplayer.Match.Playlist { public readonly Bindable DisplayMode = new Bindable(); - public required Bindable SelectedItem - { - get => selectedItem; - set => selectedItem.Current = value; - } - /// /// Invoked when an item requests to be edited. /// @@ -33,18 +28,11 @@ namespace osu.Game.Screens.OnlinePlay.Multiplayer.Match.Playlist [Resolved] private MultiplayerClient client { get; set; } = null!; - private readonly Room room; - private readonly BindableWithCurrent selectedItem = new BindableWithCurrent(); private MultiplayerPlaylistTabControl playlistTabControl = null!; private MultiplayerQueueList queueList = null!; private MultiplayerHistoryList historyList = null!; private bool firstPopulation = true; - public MultiplayerPlaylist(Room room) - { - this.room = room; - } - [BackgroundDependencyLoader] private void load() { @@ -65,17 +53,15 @@ namespace osu.Game.Screens.OnlinePlay.Multiplayer.Match.Playlist Masking = true, Children = new Drawable[] { - queueList = new MultiplayerQueueList(room) + queueList = new MultiplayerQueueList { RelativeSizeAxes = Axes.Both, - SelectedItem = { BindTarget = selectedItem }, RequestEdit = item => RequestEdit?.Invoke(item) }, historyList = new MultiplayerHistoryList { RelativeSizeAxes = Axes.Both, Alpha = 0, - SelectedItem = { BindTarget = selectedItem } } } } @@ -89,10 +75,12 @@ namespace osu.Game.Screens.OnlinePlay.Multiplayer.Match.Playlist base.LoadComplete(); DisplayMode.BindValueChanged(onDisplayModeChanged, true); + client.ItemAdded += playlistItemAdded; client.ItemRemoved += playlistItemRemoved; client.ItemChanged += playlistItemChanged; client.RoomUpdated += onRoomUpdated; + updateState(); } @@ -121,28 +109,29 @@ namespace osu.Game.Screens.OnlinePlay.Multiplayer.Match.Playlist firstPopulation = false; } + + // As a small optimisation, only the ID is required to match the selected item. + PlaylistItem? selectedItem = client.Room == null ? null : new PlaylistItem(new APIBeatmap()) { ID = client.Room.Settings.PlaylistItemId }; + queueList.SelectedItem.Value = selectedItem; + historyList.SelectedItem.Value = selectedItem; } - private void playlistItemAdded(MultiplayerPlaylistItem item) => Schedule(() => addItemToLists(item)); + private void playlistItemAdded(MultiplayerPlaylistItem item) => Scheduler.Add(() => addItemToLists(item)); - private void playlistItemRemoved(long item) => Schedule(() => removeItemFromLists(item)); + private void playlistItemRemoved(long item) => Scheduler.Add(() => removeItemFromLists(item)); - private void playlistItemChanged(MultiplayerPlaylistItem item) => Schedule(() => + private void playlistItemChanged(MultiplayerPlaylistItem item) => Scheduler.Add(() => { if (client.Room == null) return; - var newApiItem = new PlaylistItem(item); - var existingApiItemInQueue = queueList.Items.SingleOrDefault(i => i.ID == item.ID); + var existingItem = queueList.Items.SingleOrDefault(i => i.ID == item.ID); // Test if the only change between the two playlist items is the order. - if (existingApiItemInQueue != null && existingApiItemInQueue.With(playlistOrder: newApiItem.PlaylistOrder).Equals(newApiItem)) + if (existingItem != null && existingItem.With(playlistOrder: item.PlaylistOrder).Equals(new PlaylistItem(item))) { - // Set the new playlist order directly without refreshing the DrawablePlaylistItem. - existingApiItemInQueue.PlaylistOrder = newApiItem.PlaylistOrder; - - // The following isn't really required, but is here for safety and explicitness. - // MultiplayerQueueList internally binds to changes in Playlist to invalidate its own layout, which is mutated on every playlist operation. + // Set the new order directly and refresh the flow layout as an optimisation to avoid refreshing the items' visual state. + existingItem.PlaylistOrder = item.PlaylistOrder; queueList.Invalidate(); } else @@ -154,22 +143,22 @@ namespace osu.Game.Screens.OnlinePlay.Multiplayer.Match.Playlist private void addItemToLists(MultiplayerPlaylistItem item) { - var apiItem = client.Room?.Playlist.SingleOrDefault(i => i.ID == item.ID); - - // Item could have been removed from the playlist while the local player was in gameplay. - if (apiItem == null) + if (client.Room == null) return; if (item.Expired) - historyList.Items.Add(new PlaylistItem(apiItem)); + historyList.Items.Add(new PlaylistItem(item)); else - queueList.Items.Add(new PlaylistItem(apiItem)); + queueList.Items.Add(new PlaylistItem(item)); } - private void removeItemFromLists(long item) + private void removeItemFromLists(long itemId) { - queueList.Items.RemoveAll(i => i.ID == item); - historyList.Items.RemoveAll(i => i.ID == item); + if (client.Room == null) + return; + + queueList.Items.RemoveAll(i => i.ID == itemId); + historyList.Items.RemoveAll(i => i.ID == itemId); } } } diff --git a/osu.Game/Screens/OnlinePlay/Multiplayer/Match/Playlist/MultiplayerQueueList.cs b/osu.Game/Screens/OnlinePlay/Multiplayer/Match/Playlist/MultiplayerQueueList.cs index 04bb9b69e6..dc6a713908 100644 --- a/osu.Game/Screens/OnlinePlay/Multiplayer/Match/Playlist/MultiplayerQueueList.cs +++ b/osu.Game/Screens/OnlinePlay/Multiplayer/Match/Playlist/MultiplayerQueueList.cs @@ -2,7 +2,6 @@ // See the LICENCE file in the repository root for full licence text. using System.Collections.Generic; -using System.ComponentModel; using System.Linq; using osu.Framework.Allocation; using osu.Framework.Extensions.ObjectExtensions; @@ -20,50 +19,20 @@ namespace osu.Game.Screens.OnlinePlay.Multiplayer.Match.Playlist /// public partial class MultiplayerQueueList : DrawableRoomPlaylist { - private readonly Room room; - - private QueueFillFlowContainer flow = null!; - - public MultiplayerQueueList(Room room) + public MultiplayerQueueList() { - this.room = room; ShowItemOwners = true; } - protected override void LoadComplete() - { - base.LoadComplete(); - - room.PropertyChanged += onRoomPropertyChanged; - updateRoomPlaylist(); - } - - private void onRoomPropertyChanged(object? sender, PropertyChangedEventArgs e) - { - if (e.PropertyName == nameof(Room.Playlist)) - updateRoomPlaylist(); - } - - private void updateRoomPlaylist() - => flow.InvalidateLayout(); - - protected override FillFlowContainer> CreateListFillFlowContainer() => flow = new QueueFillFlowContainer + protected override FillFlowContainer> CreateListFillFlowContainer() => new QueueFillFlowContainer { Spacing = new Vector2(0, 2) }; protected override DrawableRoomPlaylistItem CreateDrawablePlaylistItem(PlaylistItem item) => new QueuePlaylistItem(item); - protected override void Dispose(bool isDisposing) - { - base.Dispose(isDisposing); - room.PropertyChanged -= onRoomPropertyChanged; - } - private partial class QueueFillFlowContainer : FillFlowContainer> { - public new void InvalidateLayout() => base.InvalidateLayout(); - public override IEnumerable FlowingChildren => base.FlowingChildren.OfType>().OrderBy(item => item.Model.PlaylistOrder); } diff --git a/osu.Game/Screens/OnlinePlay/Multiplayer/MultiplayerMatchSubScreen.cs b/osu.Game/Screens/OnlinePlay/Multiplayer/MultiplayerMatchSubScreen.cs index 6c6932f479..08a469fa03 100644 --- a/osu.Game/Screens/OnlinePlay/Multiplayer/MultiplayerMatchSubScreen.cs +++ b/osu.Game/Screens/OnlinePlay/Multiplayer/MultiplayerMatchSubScreen.cs @@ -145,11 +145,10 @@ namespace osu.Game.Screens.OnlinePlay.Multiplayer null, new Drawable[] { - new MultiplayerPlaylist(Room) + new MultiplayerPlaylist { RelativeSizeAxes = Axes.Both, - RequestEdit = OpenSongSelection, - SelectedItem = SelectedItem + RequestEdit = OpenSongSelection } }, new[] From 1a0432ce3a17aaf8da1bf84e0526a0aca8fa7145 Mon Sep 17 00:00:00 2001 From: Dan Balasescu Date: Fri, 28 Mar 2025 22:01:23 +0900 Subject: [PATCH 04/12] Re-enable ignored playlist test --- .../TestSceneMultiplayerPlaylist.cs | 45 +++++++++---------- 1 file changed, 21 insertions(+), 24 deletions(-) diff --git a/osu.Game.Tests/Visual/Multiplayer/TestSceneMultiplayerPlaylist.cs b/osu.Game.Tests/Visual/Multiplayer/TestSceneMultiplayerPlaylist.cs index 1bde02762e..c6a203c77a 100644 --- a/osu.Game.Tests/Visual/Multiplayer/TestSceneMultiplayerPlaylist.cs +++ b/osu.Game.Tests/Visual/Multiplayer/TestSceneMultiplayerPlaylist.cs @@ -31,7 +31,6 @@ namespace osu.Game.Tests.Visual.Multiplayer private BeatmapManager beatmaps = null!; private BeatmapSetInfo importedSet = null!; private BeatmapInfo importedBeatmap = null!; - private Room room = null!; [BackgroundDependencyLoader] private void load(GameHost host, AudioManager audio) @@ -46,8 +45,7 @@ namespace osu.Game.Tests.Visual.Multiplayer { base.SetUpSteps(); - AddStep("create room", () => room = CreateDefaultRoom()); - AddStep("join room", () => JoinRoom(room)); + AddStep("join room", () => JoinRoom(CreateDefaultRoom())); WaitForJoined(); AddStep("create list", () => @@ -156,37 +154,36 @@ namespace osu.Game.Tests.Visual.Multiplayer assertQueueTabCount(0); } - [Ignore("Expired items are initially removed from the room.")] [Test] public void TestJoinRoomWithMixedItemsAddedInCorrectLists() { AddStep("leave room", () => MultiplayerClient.LeaveRoom()); AddUntilStep("wait for room part", () => !RoomJoined); - AddStep("join room with items", () => + AddStep("join room with expired items", () => { - API.Queue(new CreateRoomRequest(new Room - { - Name = "test name", - Playlist = - [ - new PlaylistItem(new TestBeatmap(Ruleset.Value).BeatmapInfo) - { - RulesetID = Ruleset.Value.OnlineID - }, - new PlaylistItem(new TestBeatmap(Ruleset.Value).BeatmapInfo) - { - RulesetID = Ruleset.Value.OnlineID, - Expired = true - } - ] - })); + Room room = CreateDefaultRoom(); + room.Playlist = + [ + new PlaylistItem(new TestBeatmap(Ruleset.Value).BeatmapInfo) + { + RulesetID = Ruleset.Value.OnlineID + }, + new PlaylistItem(new TestBeatmap(Ruleset.Value).BeatmapInfo) + { + RulesetID = Ruleset.Value.OnlineID, + Expired = true + } + ]; + + JoinRoom(room); }); - AddUntilStep("wait for room join", () => RoomJoined); + WaitForJoined(); - assertItemInQueueListStep(1, 0); - assertItemInHistoryListStep(2, 0); + // IDs are offset by 1 because we've joined two rooms in this test. + assertItemInQueueListStep(2, 0); + assertItemInHistoryListStep(3, 0); } [Test] From 36e1077f99a6545b1540e306b2c4e3279a8202a5 Mon Sep 17 00:00:00 2001 From: Dan Balasescu Date: Fri, 28 Mar 2025 22:07:59 +0900 Subject: [PATCH 05/12] Add missing disposal unbinds This is an issue on `master` and have likely been causing some leaks. --- .../Match/Playlist/MultiplayerPlaylist.cs | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/osu.Game/Screens/OnlinePlay/Multiplayer/Match/Playlist/MultiplayerPlaylist.cs b/osu.Game/Screens/OnlinePlay/Multiplayer/Match/Playlist/MultiplayerPlaylist.cs index 8f59db467d..d44cb1dde8 100644 --- a/osu.Game/Screens/OnlinePlay/Multiplayer/Match/Playlist/MultiplayerPlaylist.cs +++ b/osu.Game/Screens/OnlinePlay/Multiplayer/Match/Playlist/MultiplayerPlaylist.cs @@ -5,6 +5,7 @@ using System; using System.Linq; using osu.Framework.Allocation; using osu.Framework.Bindables; +using osu.Framework.Extensions.ObjectExtensions; using osu.Framework.Graphics; using osu.Framework.Graphics.Containers; using osu.Game.Online.API.Requests.Responses; @@ -160,5 +161,18 @@ namespace osu.Game.Screens.OnlinePlay.Multiplayer.Match.Playlist queueList.Items.RemoveAll(i => i.ID == itemId); historyList.Items.RemoveAll(i => i.ID == itemId); } + + protected override void Dispose(bool isDisposing) + { + base.Dispose(isDisposing); + + if (client.IsNotNull()) + { + client.ItemAdded -= playlistItemAdded; + client.ItemRemoved -= playlistItemRemoved; + client.ItemChanged -= playlistItemChanged; + client.RoomUpdated -= onRoomUpdated; + } + } } } From 117e91bfabc343f467b98fea80ea9d9cff017ff2 Mon Sep 17 00:00:00 2001 From: Dan Balasescu Date: Mon, 31 Mar 2025 16:21:49 +0900 Subject: [PATCH 06/12] Remove local optimisation, use `CurrentPlaylistItem` --- .../Multiplayer/Match/Playlist/MultiplayerPlaylist.cs | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/osu.Game/Screens/OnlinePlay/Multiplayer/Match/Playlist/MultiplayerPlaylist.cs b/osu.Game/Screens/OnlinePlay/Multiplayer/Match/Playlist/MultiplayerPlaylist.cs index d44cb1dde8..fba3acc32a 100644 --- a/osu.Game/Screens/OnlinePlay/Multiplayer/Match/Playlist/MultiplayerPlaylist.cs +++ b/osu.Game/Screens/OnlinePlay/Multiplayer/Match/Playlist/MultiplayerPlaylist.cs @@ -8,7 +8,6 @@ using osu.Framework.Bindables; using osu.Framework.Extensions.ObjectExtensions; using osu.Framework.Graphics; using osu.Framework.Graphics.Containers; -using osu.Game.Online.API.Requests.Responses; using osu.Game.Online.Multiplayer; using osu.Game.Online.Rooms; @@ -111,10 +110,9 @@ namespace osu.Game.Screens.OnlinePlay.Multiplayer.Match.Playlist firstPopulation = false; } - // As a small optimisation, only the ID is required to match the selected item. - PlaylistItem? selectedItem = client.Room == null ? null : new PlaylistItem(new APIBeatmap()) { ID = client.Room.Settings.PlaylistItemId }; - queueList.SelectedItem.Value = selectedItem; - historyList.SelectedItem.Value = selectedItem; + PlaylistItem? currentItem = client.Room == null ? null : new PlaylistItem(client.Room.CurrentPlaylistItem); + queueList.SelectedItem.Value = currentItem; + historyList.SelectedItem.Value = currentItem; } private void playlistItemAdded(MultiplayerPlaylistItem item) => Scheduler.Add(() => addItemToLists(item)); From 74d234a7debd6be49a8d6eba8045368ab65d1b07 Mon Sep 17 00:00:00 2001 From: Dan Balasescu Date: Mon, 31 Mar 2025 16:32:32 +0900 Subject: [PATCH 07/12] Replace RoomID change with test-local fixes --- .../Visual/DailyChallenge/TestSceneDailyChallenge.cs | 8 ++------ .../DailyChallenge/TestSceneDailyChallengeIntro.cs | 11 +++++------ osu.Game/Online/Rooms/Room.cs | 2 +- .../Visual/OnlinePlay/TestRoomRequestsHandler.cs | 2 +- 4 files changed, 9 insertions(+), 14 deletions(-) diff --git a/osu.Game.Tests/Visual/DailyChallenge/TestSceneDailyChallenge.cs b/osu.Game.Tests/Visual/DailyChallenge/TestSceneDailyChallenge.cs index c974a852f3..185ebc1d39 100644 --- a/osu.Game.Tests/Visual/DailyChallenge/TestSceneDailyChallenge.cs +++ b/osu.Game.Tests/Visual/DailyChallenge/TestSceneDailyChallenge.cs @@ -43,7 +43,6 @@ namespace osu.Game.Tests.Visual.DailyChallenge { var room = new Room { - RoomID = 1234, Name = "Daily Challenge: June 4, 2024", Playlist = [ @@ -66,7 +65,6 @@ namespace osu.Game.Tests.Visual.DailyChallenge { var room = new Room { - RoomID = 1234, Name = "Daily Challenge: June 4, 2024", Playlist = [ @@ -99,7 +97,6 @@ namespace osu.Game.Tests.Visual.DailyChallenge { var room = new Room { - RoomID = 1234, Name = "Daily Challenge: June 4, 2024", Playlist = [ @@ -114,7 +111,7 @@ namespace osu.Game.Tests.Visual.DailyChallenge }; AddStep("add room", () => API.Perform(new CreateRoomRequest(room))); - AddStep("set daily challenge info", () => metadataClient.DailyChallengeInfo.Value = new DailyChallengeInfo { RoomID = 1234 }); + AddStep("set daily challenge info", () => metadataClient.DailyChallengeInfo.Value = new DailyChallengeInfo { RoomID = room.RoomID!.Value }); Screens.OnlinePlay.DailyChallenge.DailyChallenge screen = null!; AddStep("push screen", () => LoadScreen(screen = new Screens.OnlinePlay.DailyChallenge.DailyChallenge(room))); @@ -128,7 +125,6 @@ namespace osu.Game.Tests.Visual.DailyChallenge { var room = new Room { - RoomID = 1234, Name = "Daily Challenge: June 4, 2024", Playlist = [ @@ -143,7 +139,7 @@ namespace osu.Game.Tests.Visual.DailyChallenge }; AddStep("add room", () => API.Perform(new CreateRoomRequest(room))); - AddStep("set daily challenge info", () => metadataClient.DailyChallengeInfo.Value = new DailyChallengeInfo { RoomID = 1234 }); + AddStep("set daily challenge info", () => metadataClient.DailyChallengeInfo.Value = new DailyChallengeInfo { RoomID = room.RoomID!.Value }); Screens.OnlinePlay.DailyChallenge.DailyChallenge screen = null!; AddStep("push screen", () => LoadScreen(screen = new Screens.OnlinePlay.DailyChallenge.DailyChallenge(room))); diff --git a/osu.Game.Tests/Visual/DailyChallenge/TestSceneDailyChallengeIntro.cs b/osu.Game.Tests/Visual/DailyChallenge/TestSceneDailyChallengeIntro.cs index d6665e24a4..97b957df43 100644 --- a/osu.Game.Tests/Visual/DailyChallenge/TestSceneDailyChallengeIntro.cs +++ b/osu.Game.Tests/Visual/DailyChallenge/TestSceneDailyChallengeIntro.cs @@ -44,17 +44,17 @@ namespace osu.Game.Tests.Visual.DailyChallenge [Test] public void TestDailyChallenge() { - startChallenge(1234); + startChallenge(); AddStep("push screen", () => LoadScreen(new DailyChallengeIntro(room))); } [Test] public void TestPlayIntroOnceFlag() { - startChallenge(1234); + startChallenge(); AddStep("set intro played flag", () => Dependencies.Get().SetValue(Static.DailyChallengeIntroPlayed, true)); - startChallenge(1235); + startChallenge(); AddAssert("intro played flag reset", () => Dependencies.Get().Get(Static.DailyChallengeIntroPlayed), () => Is.False); @@ -62,13 +62,12 @@ namespace osu.Game.Tests.Visual.DailyChallenge AddUntilStep("intro played flag set", () => Dependencies.Get().Get(Static.DailyChallengeIntroPlayed), () => Is.True); } - private void startChallenge(int roomId) + private void startChallenge() { AddStep("add room", () => { API.Perform(new CreateRoomRequest(room = new Room { - RoomID = roomId, Name = "Daily Challenge: June 4, 2024", Playlist = [ @@ -83,7 +82,7 @@ namespace osu.Game.Tests.Visual.DailyChallenge Category = RoomCategory.DailyChallenge })); }); - AddStep("signal client", () => metadataClient.DailyChallengeUpdated(new DailyChallengeInfo { RoomID = roomId })); + AddStep("signal client", () => metadataClient.DailyChallengeUpdated(new DailyChallengeInfo { RoomID = room.RoomID!.Value })); } } } diff --git a/osu.Game/Online/Rooms/Room.cs b/osu.Game/Online/Rooms/Room.cs index b93917eff6..e965f9c187 100644 --- a/osu.Game/Online/Rooms/Room.cs +++ b/osu.Game/Online/Rooms/Room.cs @@ -348,7 +348,7 @@ namespace osu.Game.Online.Rooms public Room(MultiplayerRoom room) { - RoomID = room.RoomID > 0 ? room.RoomID : null; + RoomID = room.RoomID; Name = room.Settings.Name; Password = room.Settings.Password; Type = room.Settings.MatchType; diff --git a/osu.Game/Tests/Visual/OnlinePlay/TestRoomRequestsHandler.cs b/osu.Game/Tests/Visual/OnlinePlay/TestRoomRequestsHandler.cs index 46c1251d42..08f61f3ddc 100644 --- a/osu.Game/Tests/Visual/OnlinePlay/TestRoomRequestsHandler.cs +++ b/osu.Game/Tests/Visual/OnlinePlay/TestRoomRequestsHandler.cs @@ -267,7 +267,7 @@ namespace osu.Game.Tests.Visual.OnlinePlay /// The room host. public void AddServerSideRoom(Room room, APIUser host) { - room.RoomID ??= currentRoomId++; + room.RoomID = currentRoomId++; room.Host = host; for (int i = 0; i < room.Playlist.Count; i++) From 79375eccb7e0d3717c5ca10ff4688f2d9a015f3a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bart=C5=82omiej=20Dach?= Date: Mon, 31 Mar 2025 14:23:56 +0200 Subject: [PATCH 08/12] Add failing test case Co-authored-by: Rodrigo Correia --- .../Visual/Gameplay/TestSceneSkinEditor.cs | 57 +++++++++++++++++++ 1 file changed, 57 insertions(+) diff --git a/osu.Game.Tests/Visual/Gameplay/TestSceneSkinEditor.cs b/osu.Game.Tests/Visual/Gameplay/TestSceneSkinEditor.cs index 91188f5bac..97889eea4d 100644 --- a/osu.Game.Tests/Visual/Gameplay/TestSceneSkinEditor.cs +++ b/osu.Game.Tests/Visual/Gameplay/TestSceneSkinEditor.cs @@ -15,6 +15,7 @@ using osu.Framework.Graphics.UserInterface; using osu.Framework.Input; using osu.Framework.Testing; using osu.Game.Database; +using osu.Game.Graphics.UserInterface; using osu.Game.Overlays; using osu.Game.Overlays.Settings; using osu.Game.Overlays.SkinEditor; @@ -458,6 +459,62 @@ namespace osu.Game.Tests.Visual.Gameplay AddAssert("combo placed in ruleset target", () => rulesetHUDTarget.Components.OfType().Count() == 1); } + [Test] + public void TestAnchorRadioButtonBehavior() + { + ISerialisableDrawable? selectedComponent = null; + + AddStep("Select first component", () => + { + var blueprint = skinEditor.ChildrenOfType().First(); + skinEditor.SelectedComponents.Clear(); + skinEditor.SelectedComponents.Add(blueprint.Item); + selectedComponent = blueprint.Item; + }); + + AddStep("Right-click to open context menu", () => + { + if (selectedComponent != null) + InputManager.MoveMouseTo(((Drawable)selectedComponent).ScreenSpaceDrawQuad.Centre); + InputManager.Click(MouseButton.Right); + }); + + AddStep("Click on Anchor menu", () => + { + InputManager.MoveMouseTo(getMenuItemByText("Anchor")); + InputManager.Click(MouseButton.Left); + }); + + AddStep("Right-click TopLeft anchor", () => + { + InputManager.MoveMouseTo(getMenuItemByText("TopLeft")); + InputManager.Click(MouseButton.Right); + }); + + AddAssert("TopLeft item checked", () => (getMenuItemByText("TopLeft").Item as TernaryStateRadioMenuItem)?.State.Value == TernaryState.True); + + AddStep("Right-click Centre anchor", () => + { + InputManager.MoveMouseTo(getMenuItemByText("Centre")); + InputManager.Click(MouseButton.Right); + }); + + AddAssert("Centre item checked", () => (getMenuItemByText("Centre").Item as TernaryStateRadioMenuItem)?.State.Value == TernaryState.True); + AddAssert("TopLeft item unchecked", () => (getMenuItemByText("TopLeft").Item as TernaryStateRadioMenuItem)?.State.Value == TernaryState.False); + + AddStep("Right-click Closest anchor", () => + { + InputManager.MoveMouseTo(getMenuItemByText("Closest")); + InputManager.Click(MouseButton.Right); + }); + + AddAssert("Closest item checked", () => (getMenuItemByText("Closest").Item as TernaryStateRadioMenuItem)?.State.Value == TernaryState.True); + AddAssert("Centre item unchecked", () => (getMenuItemByText("Centre").Item as TernaryStateRadioMenuItem)?.State.Value == TernaryState.False); + + Menu.DrawableMenuItem getMenuItemByText(string text) + => this.ChildrenOfType().First(m => m.Item.Text.ToString() == text); + } + private Skin importSkinFromArchives(string filename) { var imported = skins.Import(new ImportTask(TestResources.OpenResource($@"Archives/{filename}"), filename)).GetResultSafely(); From 736f2af1e04f7e4c588483874f9e55035a7f077d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bart=C5=82omiej=20Dach?= Date: Mon, 31 Mar 2025 14:24:42 +0200 Subject: [PATCH 09/12] Fix skin editor anchor/origin context menu ternary states not updating properly - Closes https://github.com/ppy/osu/issues/31797 - Supersedes / closes https://github.com/ppy/osu/pull/32611 --- osu.Game/Overlays/SkinEditor/SkinEditor.cs | 9 +- .../SkinEditor/SkinSelectionHandler.cs | 107 ++++++++++++------ 2 files changed, 76 insertions(+), 40 deletions(-) diff --git a/osu.Game/Overlays/SkinEditor/SkinEditor.cs b/osu.Game/Overlays/SkinEditor/SkinEditor.cs index 534abd1ab3..c1c64cac1f 100644 --- a/osu.Game/Overlays/SkinEditor/SkinEditor.cs +++ b/osu.Game/Overlays/SkinEditor/SkinEditor.cs @@ -760,11 +760,7 @@ namespace osu.Game.Overlays.SkinEditor #region Delegation of IEditorChangeHandler - public event Action? OnStateChange - { - add => throw new NotImplementedException(); - remove => throw new NotImplementedException(); - } + public event Action? OnStateChange; private IEditorChangeHandler? beginChangeHandler; @@ -773,6 +769,9 @@ namespace osu.Game.Overlays.SkinEditor // Change handler may change between begin and end, which can cause unbalanced operations. // Let's track the one that was used when beginning the change so we can call EndChange on it specifically. (beginChangeHandler = changeHandler)?.BeginChange(); + + if (beginChangeHandler != null) + beginChangeHandler.OnStateChange += OnStateChange; } public void EndChange() => beginChangeHandler?.EndChange(); diff --git a/osu.Game/Overlays/SkinEditor/SkinSelectionHandler.cs b/osu.Game/Overlays/SkinEditor/SkinSelectionHandler.cs index bc878b9214..23270a1097 100644 --- a/osu.Game/Overlays/SkinEditor/SkinSelectionHandler.cs +++ b/osu.Game/Overlays/SkinEditor/SkinSelectionHandler.cs @@ -22,7 +22,10 @@ namespace osu.Game.Overlays.SkinEditor { public partial class SkinSelectionHandler : SelectionHandler { - private OsuMenuItem originMenu = null!; + private OsuMenuItem? originMenu; + + private TernaryStateRadioMenuItem? closestAnchor; + private AnchorMenuItem[]? fixedAnchors; [Resolved] private SkinEditor skinEditor { get; set; } = null!; @@ -44,6 +47,38 @@ namespace osu.Game.Overlays.SkinEditor return scaleHandler; } + protected override void LoadComplete() + { + base.LoadComplete(); + + if (ChangeHandler != null) + ChangeHandler.OnStateChange += updateTernaryStates; + SelectedItems.BindCollectionChanged((_, _) => updateTernaryStates()); + } + + private void updateTernaryStates() + { + var usingClosestAnchor = GetStateFromSelection(SelectedBlueprints, c => !c.Item.UsesFixedAnchor); + + if (closestAnchor != null) + closestAnchor.State.Value = usingClosestAnchor; + + if (fixedAnchors != null) + { + foreach (var fixedAnchor in fixedAnchors) + fixedAnchor.State.Value = GetStateFromSelection(SelectedBlueprints, c => c.Item.UsesFixedAnchor && ((Drawable)c.Item).Anchor == fixedAnchor.Anchor); + } + + if (originMenu != null) + { + foreach (var origin in originMenu.Items.OfType()) + { + origin.State.Value = GetStateFromSelection(SelectedBlueprints, c => ((Drawable)c.Item).Origin == origin.Anchor); + origin.Action.Disabled = usingClosestAnchor == TernaryState.True; + } + } + } + public override bool HandleFlip(Direction direction, bool flipOverOrigin) { var selectionQuad = getSelectionQuad(); @@ -102,27 +137,19 @@ namespace osu.Game.Overlays.SkinEditor protected override IEnumerable GetContextMenuItemsForSelection(IEnumerable> selection) { - var closestItem = new TernaryStateRadioMenuItem(SkinEditorStrings.Closest, MenuItemType.Standard, _ => applyClosestAnchors()) - { - State = { Value = GetStateFromSelection(selection, c => !c.Item.UsesFixedAnchor) } - }; + closestAnchor = new TernaryStateRadioMenuItem(SkinEditorStrings.Closest, MenuItemType.Standard, _ => applyClosestAnchors()); + fixedAnchors = createAnchorItems(applyFixedAnchors).ToArray(); yield return new OsuMenuItem(SkinEditorStrings.Anchor) { - Items = createAnchorItems((d, a) => d.UsesFixedAnchor && ((Drawable)d).Anchor == a, applyFixedAnchors) - .Prepend(closestItem) - .ToArray() + Items = fixedAnchors.Prepend(closestAnchor).ToArray() }; yield return originMenu = new OsuMenuItem(SkinEditorStrings.Origin); - closestItem.State.BindValueChanged(s => - { - // For UX simplicity, origin should only be user-editable when "closest" anchor mode is disabled. - originMenu.Items = s.NewValue == TernaryState.True - ? Array.Empty() - : createAnchorItems((d, o) => ((Drawable)d).Origin == o, applyOrigins).ToArray(); - }, true); + var usingClosestAnchor = GetStateFromSelection(SelectedBlueprints, c => !c.Item.UsesFixedAnchor); + + originMenu.Items = createAnchorItems(applyOrigins).ToArray(); yield return new OsuMenuItemSpacer(); @@ -163,27 +190,37 @@ namespace osu.Game.Overlays.SkinEditor foreach (var item in base.GetContextMenuItemsForSelection(selection)) yield return item; - IEnumerable createAnchorItems(Func checkFunction, Action applyFunction) + updateTernaryStates(); + } + + private IEnumerable createAnchorItems(Action applyFunction) + { + var displayableAnchors = new[] { - var displayableAnchors = new[] - { - Anchor.TopLeft, - Anchor.TopCentre, - Anchor.TopRight, - Anchor.CentreLeft, - Anchor.Centre, - Anchor.CentreRight, - Anchor.BottomLeft, - Anchor.BottomCentre, - Anchor.BottomRight, - }; - return displayableAnchors.Select(a => - { - return new TernaryStateRadioMenuItem(a.ToString(), MenuItemType.Standard, _ => applyFunction(a)) - { - State = { Value = GetStateFromSelection(selection, c => checkFunction(c.Item, a)) } - }; - }); + Anchor.TopLeft, + Anchor.TopCentre, + Anchor.TopRight, + Anchor.CentreLeft, + Anchor.Centre, + Anchor.CentreRight, + Anchor.BottomLeft, + Anchor.BottomCentre, + Anchor.BottomRight, + }; + return displayableAnchors.Select(a => + { + return new AnchorMenuItem(a, _ => applyFunction(a)); + }); + } + + private partial class AnchorMenuItem : TernaryStateRadioMenuItem + { + public readonly Anchor Anchor; + + public AnchorMenuItem(Anchor anchor, Action applyFunction) + : base(anchor.ToString(), MenuItemType.Standard, _ => applyFunction(anchor)) + { + Anchor = anchor; } } From 005b204f0a6be4cd83ba379e413c9f17092f7e92 Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Mon, 31 Mar 2025 23:43:04 +0900 Subject: [PATCH 10/12] Remove unused local --- osu.Game/Overlays/SkinEditor/SkinSelectionHandler.cs | 2 -- 1 file changed, 2 deletions(-) diff --git a/osu.Game/Overlays/SkinEditor/SkinSelectionHandler.cs b/osu.Game/Overlays/SkinEditor/SkinSelectionHandler.cs index 23270a1097..838b5ff2f0 100644 --- a/osu.Game/Overlays/SkinEditor/SkinSelectionHandler.cs +++ b/osu.Game/Overlays/SkinEditor/SkinSelectionHandler.cs @@ -147,8 +147,6 @@ namespace osu.Game.Overlays.SkinEditor yield return originMenu = new OsuMenuItem(SkinEditorStrings.Origin); - var usingClosestAnchor = GetStateFromSelection(SelectedBlueprints, c => !c.Item.UsesFixedAnchor); - originMenu.Items = createAnchorItems(applyOrigins).ToArray(); yield return new OsuMenuItemSpacer(); From 22f2c6f7b97fe748844726adf5b914dab6feb42e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bart=C5=82omiej=20Dach?= Date: Wed, 2 Apr 2025 12:39:45 +0200 Subject: [PATCH 11/12] Add support for ruleset-specific user tags Follow-up from https://github.com/ppy/osu-web/pull/12059 --- osu.Game/Online/API/Requests/Responses/APITag.cs | 3 +++ osu.Game/Screens/Ranking/UserTagControl.cs | 8 +++++--- 2 files changed, 8 insertions(+), 3 deletions(-) diff --git a/osu.Game/Online/API/Requests/Responses/APITag.cs b/osu.Game/Online/API/Requests/Responses/APITag.cs index 4dd18663af..b0454fdb1d 100644 --- a/osu.Game/Online/API/Requests/Responses/APITag.cs +++ b/osu.Game/Online/API/Requests/Responses/APITag.cs @@ -15,5 +15,8 @@ namespace osu.Game.Online.API.Requests.Responses [JsonProperty("description")] public string Description { get; set; } = string.Empty; + + [JsonProperty("ruleset_id")] + public int? RulesetId { get; set; } } } diff --git a/osu.Game/Screens/Ranking/UserTagControl.cs b/osu.Game/Screens/Ranking/UserTagControl.cs index ae4a918ae5..80d487112b 100644 --- a/osu.Game/Screens/Ranking/UserTagControl.cs +++ b/osu.Game/Screens/Ranking/UserTagControl.cs @@ -148,12 +148,14 @@ namespace osu.Game.Screens.Ranking if (allTags.Value == null || apiBeatmap.Value?.TopTags == null) return; - var allTagsById = allTags.Value.ToDictionary(t => t.Id); + var relevantTagsById = allTags.Value + .Where(tag => tag.RulesetId == null || tag.RulesetId == beatmapInfo.Ruleset.OnlineID) + .ToDictionary(t => t.Id); var ownTagIds = apiBeatmap.Value.OwnTagIds?.ToHashSet() ?? new HashSet(); foreach (var topTag in apiBeatmap.Value.TopTags) { - if (allTagsById.Remove(topTag.TagId, out var tag)) + if (relevantTagsById.Remove(topTag.TagId, out var tag)) { displayedTags.Add(new UserTag(tag) { @@ -163,7 +165,7 @@ namespace osu.Game.Screens.Ranking } } - extraTags.AddRange(allTagsById.Select(t => new UserTag(t.Value))); + extraTags.AddRange(relevantTagsById.Select(t => new UserTag(t.Value))); loadingLayer.Hide(); } From db0f1895fb056e00efaacb3896fc44502518c6bb Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bart=C5=82omiej=20Dach?= Date: Wed, 2 Apr 2025 13:25:18 +0200 Subject: [PATCH 12/12] Add rudimentary test coverage --- .../Visual/Ranking/TestSceneUserTagControl.cs | 44 ++++++++++++------- 1 file changed, 29 insertions(+), 15 deletions(-) diff --git a/osu.Game.Tests/Visual/Ranking/TestSceneUserTagControl.cs b/osu.Game.Tests/Visual/Ranking/TestSceneUserTagControl.cs index d622df8d76..9174b2a3db 100644 --- a/osu.Game.Tests/Visual/Ranking/TestSceneUserTagControl.cs +++ b/osu.Game.Tests/Visual/Ranking/TestSceneUserTagControl.cs @@ -9,6 +9,8 @@ using osu.Game.Beatmaps; using osu.Game.Online.API; using osu.Game.Online.API.Requests; using osu.Game.Online.API.Requests.Responses; +using osu.Game.Rulesets.Osu; +using osu.Game.Rulesets.Taiko; using osu.Game.Screens.Ranking; namespace osu.Game.Tests.Visual.Ranking @@ -20,10 +22,6 @@ namespace osu.Game.Tests.Visual.Ranking [SetUpSteps] public void SetUpSteps() { - AddStep("set up working beatmap", () => - { - Beatmap.Value.BeatmapInfo.OnlineID = 42; - }); AddStep("set up network requests", () => { dummyAPI.HandleRequest = request => @@ -40,6 +38,7 @@ namespace osu.Game.Tests.Visual.Ranking new APITag { Id = 2, Name = "alt", Description = "Colloquial term for maps which use rhythms that encourage the player to alternate notes. Typically distinct from burst or stream maps.", }, new APITag { Id = 3, Name = "aim", Description = "Category for difficulty relating to cursor movement.", }, new APITag { Id = 4, Name = "tap", Description = "Category for difficulty relating to tapping input.", }, + new APITag { Id = 5, Name = "mono-heavy", Description = "Features monos used in large amounts.", RulesetId = 1, }, ] }), 500); return true; @@ -67,19 +66,34 @@ namespace osu.Game.Tests.Visual.Ranking return false; }; }); - AddStep("create control", () => + AddStep("show for osu! beatmap", () => { - Child = new PopoverContainer - { - RelativeSizeAxes = Axes.Both, - Child = new UserTagControl(Beatmap.Value.BeatmapInfo) - { - Width = 500, - Anchor = Anchor.Centre, - Origin = Anchor.Centre, - } - }; + var working = CreateWorkingBeatmap(new OsuRuleset().RulesetInfo); + working.BeatmapInfo.OnlineID = 42; + Beatmap.Value = working; + recreateControl(); }); + AddStep("show for taiko beatmap", () => + { + var working = CreateWorkingBeatmap(new TaikoRuleset().RulesetInfo); + working.BeatmapInfo.OnlineID = 44; + Beatmap.Value = working; + recreateControl(); + }); + } + + private void recreateControl() + { + Child = new PopoverContainer + { + RelativeSizeAxes = Axes.Both, + Child = new UserTagControl(Beatmap.Value.BeatmapInfo) + { + Width = 500, + Anchor = Anchor.Centre, + Origin = Anchor.Centre, + } + }; } } }