Merge pull request #20178 from smoogipoo/fix-match-creation-beatmap-select

Fix several weird scenarios with online play song selection

osu.Game.Tests/Visual/Multiplayer/TestSceneMultiplayerMatchSongSelect.cs

@@ -19,7 +19,6 @@ using osu.Game.Database;
 using osu.Game.Online.Rooms;
 using osu.Game.Overlays.Mods;
 using osu.Game.Rulesets;
-using osu.Game.Rulesets.Catch;
 using osu.Game.Rulesets.Mods;
 using osu.Game.Rulesets.Osu;
 using osu.Game.Rulesets.Osu.Mods;
@@ -68,37 +67,6 @@ namespace osu.Game.Tests.Visual.Multiplayer
             AddUntilStep("wait for present", () => songSelect.IsCurrentScreen() && songSelect.BeatmapSetsLoaded);
         }
 
-        [Test]
-        public void TestBeatmapRevertedOnExitIfNoSelection()
-        {
-            BeatmapInfo selectedBeatmap = null;
-
-            AddStep("select beatmap",
-                () => songSelect.Carousel.SelectBeatmap(selectedBeatmap = beatmaps.Where(beatmap => beatmap.Ruleset.OnlineID == new OsuRuleset().LegacyID).ElementAt(1)));
-            AddUntilStep("wait for selection", () => Beatmap.Value.BeatmapInfo.Equals(selectedBeatmap));
-
-            AddStep("exit song select", () => songSelect.Exit());
-            AddAssert("beatmap reverted", () => Beatmap.IsDefault);
-        }
-
-        [Test]
-        public void TestModsRevertedOnExitIfNoSelection()
-        {
-            AddStep("change mods", () => SelectedMods.Value = new[] { new OsuModDoubleTime() });
-
-            AddStep("exit song select", () => songSelect.Exit());
-            AddAssert("mods reverted", () => SelectedMods.Value.Count == 0);
-        }
-
-        [Test]
-        public void TestRulesetRevertedOnExitIfNoSelection()
-        {
-            AddStep("change ruleset", () => Ruleset.Value = new CatchRuleset().RulesetInfo);
-
-            AddStep("exit song select", () => songSelect.Exit());
-            AddAssert("ruleset reverted", () => Ruleset.Value.Equals(new OsuRuleset().RulesetInfo));
-        }
-
         [Test]
         public void TestBeatmapConfirmed()
         {
@@ -152,8 +120,8 @@ namespace osu.Game.Tests.Visual.Multiplayer
 
             public new BeatmapCarousel Carousel => base.Carousel;
 
-            public TestMultiplayerMatchSongSelect(Room room, WorkingBeatmap beatmap = null, RulesetInfo ruleset = null)
+            public TestMultiplayerMatchSongSelect(Room room)
-                : base(room, null, beatmap, ruleset)
+                : base(room)
             {
             }
         }

osu.Game/Screens/OnlinePlay/Match/RoomSubScreen.cs

@@ -433,6 +433,9 @@ namespace osu.Game.Screens.OnlinePlay.Match
 
         private void updateWorkingBeatmap()
         {
+            if (SelectedItem.Value == null || !this.IsCurrentScreen())
+                return;
+
             var beatmap = SelectedItem.Value?.Beatmap;
 
             // Retrieve the corresponding local beatmap, since we can't directly use the playlist's beatmap info

osu.Game/Screens/OnlinePlay/Multiplayer/Match/MultiplayerMatchSettingsOverlay.cs

@@ -1,12 +1,9 @@
 // Copyright (c) ppy Pty Ltd <contact@ppy.sh>. Licensed under the MIT Licence.
 // See the LICENCE file in the repository root for full licence text.
 
-#nullable disable
-
 using System;
 using System.ComponentModel;
 using System.Diagnostics;
-using JetBrains.Annotations;
 using osu.Framework.Allocation;
 using osu.Framework.Bindables;
 using osu.Framework.Extensions;
@@ -30,12 +27,12 @@ namespace osu.Game.Screens.OnlinePlay.Multiplayer.Match
 {
     public class MultiplayerMatchSettingsOverlay : RoomSettingsOverlay
     {
-        private MatchSettings settings;
+        private MatchSettings settings = null!;
 
         protected override OsuButton SubmitButton => settings.ApplyButton;
 
         [Resolved]
-        private OngoingOperationTracker ongoingOperationTracker { get; set; }
+        private OngoingOperationTracker ongoingOperationTracker { get; set; } = null!;
 
         protected override bool IsLoading => ongoingOperationTracker.InProgress.Value;
 
@@ -57,20 +54,21 @@ namespace osu.Game.Screens.OnlinePlay.Multiplayer.Match
         {
             private const float disabled_alpha = 0.2f;
 
-            public Action SettingsApplied;
+            public Action? SettingsApplied;
 
-            public OsuTextBox NameField, MaxParticipantsField;
+            public OsuTextBox NameField = null!;
-            public MatchTypePicker TypePicker;
+            public OsuTextBox MaxParticipantsField = null!;
-            public OsuEnumDropdown<QueueMode> QueueModeDropdown;
+            public MatchTypePicker TypePicker = null!;
-            public OsuTextBox PasswordTextBox;
+            public OsuEnumDropdown<QueueMode> QueueModeDropdown = null!;
-            public OsuCheckbox AutoSkipCheckbox;
+            public OsuTextBox PasswordTextBox = null!;
-            public TriangleButton ApplyButton;
+            public OsuCheckbox AutoSkipCheckbox = null!;
+            public TriangleButton ApplyButton = null!;
 
-            public OsuSpriteText ErrorText;
+            public OsuSpriteText ErrorText = null!;
 
-            private OsuEnumDropdown<StartMode> startModeDropdown;
+            private OsuEnumDropdown<StartMode> startModeDropdown = null!;
-            private OsuSpriteText typeLabel;
+            private OsuSpriteText typeLabel = null!;
-            private LoadingLayer loadingLayer;
+            private LoadingLayer loadingLayer = null!;
 
             public void SelectBeatmap()
             {
@@ -79,26 +77,23 @@ namespace osu.Game.Screens.OnlinePlay.Multiplayer.Match
             }
 
             [Resolved]
-            private MultiplayerMatchSubScreen matchSubScreen { get; set; }
+            private MultiplayerMatchSubScreen matchSubScreen { get; set; } = null!;
 
             [Resolved]
-            private IRoomManager manager { get; set; }
+            private IRoomManager manager { get; set; } = null!;
 
             [Resolved]
-            private MultiplayerClient client { get; set; }
+            private MultiplayerClient client { get; set; } = null!;
 
             [Resolved]
-            private OngoingOperationTracker ongoingOperationTracker { get; set; }
+            private OngoingOperationTracker ongoingOperationTracker { get; set; } = null!;
 
             private readonly IBindable<bool> operationInProgress = new BindableBool();
-
-            [CanBeNull]
-            private IDisposable applyingSettingsOperation;
-
             private readonly Room room;
 
-            private Drawable playlistContainer;
+            private IDisposable? applyingSettingsOperation;
-            private DrawableRoomPlaylist drawablePlaylist;
+            private Drawable playlistContainer = null!;
+            private DrawableRoomPlaylist drawablePlaylist = null!;
 
             public MatchSettings(Room room)
             {
@@ -423,7 +418,7 @@ namespace osu.Game.Screens.OnlinePlay.Multiplayer.Match
                     else
                         room.MaxParticipants.Value = null;
 
-                    manager?.CreateRoom(room, onSuccess, onError);
+                    manager.CreateRoom(room, onSuccess, onError);
                 }
             }
 
@@ -466,7 +461,7 @@ namespace osu.Game.Screens.OnlinePlay.Multiplayer.Match
         public class CreateOrUpdateButton : TriangleButton
         {
             [Resolved(typeof(Room), nameof(Room.RoomID))]
-            private Bindable<long?> roomId { get; set; }
+            private Bindable<long?> roomId { get; set; } = null!;
 
             protected override void LoadComplete()
             {

osu.Game/Screens/OnlinePlay/Multiplayer/MultiplayerMatchSongSelect.cs

@@ -8,11 +8,9 @@ using osu.Framework.Allocation;
 using osu.Framework.Bindables;
 using osu.Framework.Logging;
 using osu.Framework.Screens;
-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.Rulesets.Mods;
 using osu.Game.Screens.Select;
 
@@ -27,7 +25,7 @@ namespace osu.Game.Screens.OnlinePlay.Multiplayer
         private OngoingOperationTracker operationTracker { get; set; } = null!;
 
         private readonly IBindable<bool> operationInProgress = new Bindable<bool>();
-        private readonly long? itemToEdit;
+        private readonly PlaylistItem? itemToEdit;
 
         private LoadingLayer loadingLayer = null!;
         private IDisposable? selectionOperation;
@@ -37,21 +35,10 @@ namespace osu.Game.Screens.OnlinePlay.Multiplayer
         /// </summary>
         /// <param name="room">The room.</param>
         /// <param name="itemToEdit">The item to be edited. May be null, in which case a new item will be added to the playlist.</param>
-        /// <param name="beatmap">An optional initial beatmap selection to perform.</param>
+        public MultiplayerMatchSongSelect(Room room, PlaylistItem? itemToEdit = null)
-        /// <param name="ruleset">An optional initial ruleset selection to perform.</param>
+            : base(room, itemToEdit)
-        public MultiplayerMatchSongSelect(Room room, long? itemToEdit = null, WorkingBeatmap? beatmap = null, RulesetInfo? ruleset = null)
-            : base(room)
         {
             this.itemToEdit = itemToEdit;
-
-            if (beatmap != null || ruleset != null)
-            {
-                Schedule(() =>
-                {
-                    if (beatmap != null) Beatmap.Value = beatmap;
-                    if (ruleset != null) Ruleset.Value = ruleset;
-                });
-            }
         }
 
         [BackgroundDependencyLoader]
@@ -80,7 +67,7 @@ namespace osu.Game.Screens.OnlinePlay.Multiplayer
         {
             if (operationInProgress.Value)
             {
-                Logger.Log($"{nameof(SelectedItem)} aborted due to {nameof(operationInProgress)}");
+                Logger.Log($"{nameof(SelectItem)} aborted due to {nameof(operationInProgress)}");
                 return false;
             }
 
@@ -92,7 +79,7 @@ namespace osu.Game.Screens.OnlinePlay.Multiplayer
 
                 var multiplayerItem = new MultiplayerPlaylistItem
                 {
-                    ID = itemToEdit ?? 0,
+                    ID = itemToEdit?.ID ?? 0,
                     BeatmapID = item.Beatmap.OnlineID,
                     BeatmapChecksum = item.Beatmap.MD5Hash,
                     RulesetID = item.RulesetID,

osu.Game/Screens/OnlinePlay/Multiplayer/MultiplayerMatchSubScreen.cs

@@ -49,11 +49,6 @@ namespace osu.Game.Screens.OnlinePlay.Multiplayer
         [Resolved]
         private MultiplayerClient client { get; set; }
 
-        [Resolved]
-        private BeatmapManager beatmapManager { get; set; }
-
-        private readonly IBindable<bool> isConnected = new Bindable<bool>();
-
         private AddItemButton addItemButton;
 
         public MultiplayerMatchSubScreen(Room room)
@@ -227,12 +222,7 @@ namespace osu.Game.Screens.OnlinePlay.Multiplayer
             if (!this.IsCurrentScreen())
                 return;
 
-            int id = itemToEdit?.Beatmap.OnlineID ?? Room.Playlist.Last().Beatmap.OnlineID;
+            this.Push(new MultiplayerMatchSongSelect(Room, itemToEdit));
-            var localBeatmap = beatmapManager.QueryBeatmap(b => b.OnlineID == id);
-
-            var workingBeatmap = localBeatmap == null ? null : beatmapManager.GetWorkingBeatmap(localBeatmap);
-
-            this.Push(new MultiplayerMatchSongSelect(Room, itemToEdit?.ID, workingBeatmap));
         }
 
         protected override Drawable CreateFooter() => new MultiplayerMatchFooter();
@@ -424,7 +414,7 @@ namespace osu.Game.Screens.OnlinePlay.Multiplayer
                 return;
             }
 
-            this.Push(new MultiplayerMatchSongSelect(Room, client.Room.Settings.PlaylistItemId, beatmap, ruleset));
+            this.Push(new MultiplayerMatchSongSelect(Room, Room.Playlist.Single(item => item.ID == client.Room.Settings.PlaylistItemId)));
         }
 
         protected override void Dispose(bool isDisposing)

osu.Game/Screens/OnlinePlay/OnlinePlaySongSelect.cs

@@ -1,13 +1,11 @@
 // Copyright (c) ppy Pty Ltd <contact@ppy.sh>. Licensed under the MIT Licence.
 // See the LICENCE file in the repository root for full licence text.
 
-#nullable disable
-
 using System;
 using System.Collections.Generic;
+using System.Diagnostics;
 using System.Linq;
 using Humanizer;
-using JetBrains.Annotations;
 using osu.Framework.Allocation;
 using osu.Framework.Bindables;
 using osu.Framework.Graphics;
@@ -35,32 +33,34 @@ namespace osu.Game.Screens.OnlinePlay
         public override bool AllowEditing => false;
 
         [Resolved(typeof(Room), nameof(Room.Playlist))]
-        protected BindableList<PlaylistItem> Playlist { get; private set; }
+        protected BindableList<PlaylistItem> Playlist { get; private set; } = null!;
-
-        [CanBeNull]
-        [Resolved(CanBeNull = true)]
-        protected IBindable<PlaylistItem> SelectedItem { get; private set; }
 
         [Resolved]
-        private RulesetStore rulesets { get; set; }
+        private RulesetStore rulesets { get; set; } = null!;
+
+        [Resolved]
+        private BeatmapManager beatmapManager { get; set; } = null!;
 
         protected override UserActivity InitialActivity => new UserActivity.InLobby(room);
 
         protected readonly Bindable<IReadOnlyList<Mod>> FreeMods = new Bindable<IReadOnlyList<Mod>>(Array.Empty<Mod>());
 
         private readonly Room room;
-
+        private readonly PlaylistItem? initialItem;
-        private WorkingBeatmap initialBeatmap;
-        private RulesetInfo initialRuleset;
-        private IReadOnlyList<Mod> initialMods;
-        private bool itemSelected;
-
         private readonly FreeModSelectOverlay freeModSelectOverlay;
-        private IDisposable freeModSelectOverlayRegistration;
 
-        protected OnlinePlaySongSelect(Room room)
+        private IDisposable? freeModSelectOverlayRegistration;
+
+        /// <summary>
+        /// Creates a new <see cref="OnlinePlaySongSelect"/>.
+        /// </summary>
+        /// <param name="room">The room.</param>
+        /// <param name="initialItem">An optional initial <see cref="PlaylistItem"/> to use for the initial beatmap/ruleset/mods.
+        /// If <c>null</c>, the last <see cref="PlaylistItem"/> in the room will be used.</param>
+        protected OnlinePlaySongSelect(Room room, PlaylistItem? initialItem = null)
         {
             this.room = room;
+            this.initialItem = initialItem ?? room.Playlist.LastOrDefault();
 
             Padding = new MarginPadding { Horizontal = HORIZONTAL_OVERFLOW_PADDING };
 
@@ -75,11 +75,6 @@ namespace osu.Game.Screens.OnlinePlay
         private void load()
         {
             LeftArea.Padding = new MarginPadding { Top = Header.HEIGHT };
-
-            initialBeatmap = Beatmap.Value;
-            initialRuleset = Ruleset.Value;
-            initialMods = Mods.Value.ToList();
-
             LoadComponent(freeModSelectOverlay);
         }
 
@@ -87,14 +82,35 @@ namespace osu.Game.Screens.OnlinePlay
         {
             base.LoadComplete();
 
-            var rulesetInstance = SelectedItem?.Value?.RulesetID == null ? null : rulesets.GetRuleset(SelectedItem.Value.RulesetID)?.CreateInstance();
+            if (initialItem != null)
-
-            if (rulesetInstance != null)
             {
+                // Prefer using a local databased beatmap lookup since OnlineId may be -1 for an invalid beatmap selection.
+                BeatmapInfo? beatmapInfo = initialItem.Beatmap as BeatmapInfo;
+
+                // And in the case that this isn't a local databased beatmap, query by online ID.
+                if (beatmapInfo == null)
+                {
+                    int onlineId = initialItem.Beatmap.OnlineID;
+                    beatmapInfo = beatmapManager.QueryBeatmap(b => b.OnlineID == onlineId);
+                }
+
+                if (beatmapInfo != null)
+                    Beatmap.Value = beatmapManager.GetWorkingBeatmap(beatmapInfo);
+
+                RulesetInfo? ruleset = rulesets.GetRuleset(initialItem.RulesetID);
+
+                if (ruleset != null)
+                {
+                    Ruleset.Value = ruleset;
+
+                    var rulesetInstance = ruleset.CreateInstance();
+                    Debug.Assert(rulesetInstance != null);
+
                     // At this point, Mods contains both the required and allowed mods. For selection purposes, it should only contain the required mods.
                     // Similarly, freeMods is currently empty but should only contain the allowed mods.
-                Mods.Value = SelectedItem.Value.RequiredMods.Select(m => m.ToMod(rulesetInstance)).ToArray();
+                    Mods.Value = initialItem.RequiredMods.Select(m => m.ToMod(rulesetInstance)).ToArray();
-                FreeMods.Value = SelectedItem.Value.AllowedMods.Select(m => m.ToMod(rulesetInstance)).ToArray();
+                    FreeMods.Value = initialItem.AllowedMods.Select(m => m.ToMod(rulesetInstance)).ToArray();
+                }
             }
 
             Mods.BindValueChanged(onModsChanged);
@@ -125,13 +141,7 @@ namespace osu.Game.Screens.OnlinePlay
                 AllowedMods = FreeMods.Value.Select(m => new APIMod(m)).ToArray()
             };
 
-            if (SelectItem(item))
+            return SelectItem(item);
-            {
-                itemSelected = true;
-                return true;
-            }
-
-            return false;
         }
 
         /// <summary>
@@ -154,15 +164,7 @@ namespace osu.Game.Screens.OnlinePlay
 
         public override bool OnExiting(ScreenExitEvent e)
         {
-            if (!itemSelected)
-            {
-                Beatmap.Value = initialBeatmap;
-                Ruleset.Value = initialRuleset;
-                Mods.Value = initialMods;
-            }
-
             freeModSelectOverlay.Hide();
-
             return base.OnExiting(e);
         }
 
@@ -199,7 +201,6 @@ namespace osu.Game.Screens.OnlinePlay
         protected override void Dispose(bool isDisposing)
         {
             base.Dispose(isDisposing);
-
             freeModSelectOverlayRegistration?.Dispose();
         }
     }

osu.Game/Screens/OnlinePlay/Playlists/PlaylistsSongSelect.cs

@@ -1,8 +1,6 @@
 // Copyright (c) ppy Pty Ltd <contact@ppy.sh>. Licensed under the MIT Licence.
 // See the LICENCE file in the repository root for full licence text.
 
-#nullable disable
-
 using System.Linq;
 using osu.Framework.Screens;
 using osu.Game.Online.API;