From aae98e6906450bbd4518e4ec33af8049a0508df4 Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Wed, 18 Sep 2024 16:11:47 +0900 Subject: [PATCH 1/4] Add failing test showing crash at song select on selection edge case --- .../Navigation/TestSceneScreenNavigation.cs | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/osu.Game.Tests/Visual/Navigation/TestSceneScreenNavigation.cs b/osu.Game.Tests/Visual/Navigation/TestSceneScreenNavigation.cs index f02c2fd4f0..6cd89dcd0c 100644 --- a/osu.Game.Tests/Visual/Navigation/TestSceneScreenNavigation.cs +++ b/osu.Game.Tests/Visual/Navigation/TestSceneScreenNavigation.cs @@ -144,6 +144,22 @@ namespace osu.Game.Tests.Visual.Navigation exitViaEscapeAndConfirm(); } + [Test] + public void TestEnterGameplayWhileFilteringToNoSelection() + { + TestPlaySongSelect songSelect = null; + + PushAndConfirm(() => songSelect = new TestPlaySongSelect()); + AddStep("import beatmap", () => BeatmapImportHelper.LoadQuickOszIntoOsu(Game).WaitSafely()); + AddUntilStep("wait for selected", () => !Game.Beatmap.IsDefault); + + AddStep("force selection", () => + { + songSelect.FinaliseSelection(); + songSelect.FilterControl.CurrentTextSearch.Value = "test"; + }); + } + [Test] public void TestSongSelectBackActionHandling() { From c192a6a1d54b69936c6e741e49c7b0524c0a2c8e Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Wed, 18 Sep 2024 15:50:55 +0900 Subject: [PATCH 2/4] Fix song select crashes due to attempting to clear selection after load has already begun --- osu.Game/Screens/Select/BeatmapCarousel.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/osu.Game/Screens/Select/BeatmapCarousel.cs b/osu.Game/Screens/Select/BeatmapCarousel.cs index 525884c413..57978b7bbd 100644 --- a/osu.Game/Screens/Select/BeatmapCarousel.cs +++ b/osu.Game/Screens/Select/BeatmapCarousel.cs @@ -1036,7 +1036,7 @@ namespace osu.Game.Screens.Select itemsCache.Validate(); // update and let external consumers know about selection loss. - if (BeatmapSetsLoaded) + if (BeatmapSetsLoaded && AllowSelection) { bool selectionLost = selectedBeatmapSet != null && selectedBeatmapSet.State.Value != CarouselItemState.Selected; From 743d50924105a557a0d08424e7c274ee1996d580 Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Wed, 18 Sep 2024 16:40:27 +0900 Subject: [PATCH 3/4] Also ensure filter is applied when returning to song select --- .../Navigation/TestSceneScreenNavigation.cs | 6 +++++ osu.Game/Screens/Select/SongSelect.cs | 23 ++++++++++++++++++- 2 files changed, 28 insertions(+), 1 deletion(-) diff --git a/osu.Game.Tests/Visual/Navigation/TestSceneScreenNavigation.cs b/osu.Game.Tests/Visual/Navigation/TestSceneScreenNavigation.cs index 6cd89dcd0c..eda7ce925a 100644 --- a/osu.Game.Tests/Visual/Navigation/TestSceneScreenNavigation.cs +++ b/osu.Game.Tests/Visual/Navigation/TestSceneScreenNavigation.cs @@ -150,6 +150,7 @@ namespace osu.Game.Tests.Visual.Navigation TestPlaySongSelect songSelect = null; PushAndConfirm(() => songSelect = new TestPlaySongSelect()); + AddUntilStep("wait for song select", () => songSelect.BeatmapSetsLoaded); AddStep("import beatmap", () => BeatmapImportHelper.LoadQuickOszIntoOsu(Game).WaitSafely()); AddUntilStep("wait for selected", () => !Game.Beatmap.IsDefault); @@ -158,6 +159,11 @@ namespace osu.Game.Tests.Visual.Navigation songSelect.FinaliseSelection(); songSelect.FilterControl.CurrentTextSearch.Value = "test"; }); + + AddUntilStep("wait for player", () => !songSelect.IsCurrentScreen()); + AddStep("return to song select", () => songSelect.MakeCurrent()); + + AddUntilStep("wait for selection lost", () => songSelect.Beatmap.IsDefault); } [Test] diff --git a/osu.Game/Screens/Select/SongSelect.cs b/osu.Game/Screens/Select/SongSelect.cs index 6da72ee660..18608d61e9 100644 --- a/osu.Game/Screens/Select/SongSelect.cs +++ b/osu.Game/Screens/Select/SongSelect.cs @@ -127,6 +127,8 @@ namespace osu.Game.Screens.Select private Sample sampleChangeDifficulty = null!; private Sample sampleChangeBeatmap = null!; + private bool pendingFilterApplication; + private Container carouselContainer = null!; protected BeatmapDetailArea BeatmapDetails { get; private set; } = null!; @@ -328,7 +330,20 @@ namespace osu.Game.Screens.Select GetRecommendedBeatmap = s => recommender?.GetRecommendedBeatmap(s), }, c => carouselContainer.Child = c); - FilterControl.FilterChanged = Carousel.Filter; + FilterControl.FilterChanged = criteria => + { + // If a filter operation is applied when we're in a state that doesn't allow selection, + // we might end up in an unexpected state. This is because currently carousel panels are in charge + // of updating the global selection (which is very hard to deal with). + // + // For now let's just avoid filtering when selection isn't allowed locally. + // This should be nuked from existence when we get around to fixing the complexity of song select <-> beatmap carousel. + // The debounce part of BeatmapCarousel's filtering should probably also be removed and handled locally. + if (Carousel.AllowSelection) + Carousel.Filter(criteria); + else + pendingFilterApplication = true; + }; if (ShowSongSelectFooter) { @@ -701,6 +716,12 @@ namespace osu.Game.Screens.Select Carousel.AllowSelection = true; + if (pendingFilterApplication) + { + Carousel.Filter(FilterControl.CreateCriteria()); + pendingFilterApplication = false; + } + BeatmapDetails.Refresh(); beginLooping(); From ac507a3ba568e40396a642d13032dbc1e8d6c314 Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Wed, 18 Sep 2024 17:21:43 +0900 Subject: [PATCH 4/4] Remove unused parameter in `applyActiveCriteria` --- osu.Game/Screens/Select/BeatmapCarousel.cs | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/osu.Game/Screens/Select/BeatmapCarousel.cs b/osu.Game/Screens/Select/BeatmapCarousel.cs index 57978b7bbd..d9359cfec3 100644 --- a/osu.Game/Screens/Select/BeatmapCarousel.cs +++ b/osu.Game/Screens/Select/BeatmapCarousel.cs @@ -706,7 +706,7 @@ namespace osu.Game.Screens.Select private bool beatmapsSplitOut; - private void applyActiveCriteria(bool debounce, bool alwaysResetScrollPosition = true) + private void applyActiveCriteria(bool debounce) { PendingFilter?.Cancel(); PendingFilter = null; @@ -735,8 +735,7 @@ namespace osu.Game.Screens.Select root.Filter(activeCriteria); itemsCache.Invalidate(); - if (alwaysResetScrollPosition || !Scroll.UserScrolling) - ScrollToSelected(true); + ScrollToSelected(true); FilterApplied?.Invoke(); }