Restore previous beatmap when leaving scoped mode#36582
Restore previous beatmap when leaving scoped mode#36582LiquidPL wants to merge 7 commits intoppy:masterfrom
Conversation
|
Just in case, I'll give my thoughts on why I suggested this UX in the issue post, specifically this part:
There are 2 solutions: either always return to the difficulty which was selected before entering scoped mode, or only return to it if the user selects a difficulty inside scoped mode that's outside filter range. By the way, with this PR, if you enter scoped mode, then select a difficulty that's outside filter range, then exit to main menu, then enter song select, the scrolling will still be lost. I'll leave it to you if this is worth fixing (I don't know code). |
There was a problem hiding this comment.
In general I am not a fan of how this restore behaviour has smeared itself all over BeatmapCarousel. I think it's going to end badly because I tried similar once before and it resulted in carnage.
If this sort of thing is to be entertained then the "scoping" API probably has to change. It probably should not be a plain bindable anymore and instead be a method you call, something like (DISCLAIMER: I have not tried this, just spitballing):
IDisposable ScopeToBeatmap(IBeatmapSetInfo beatmapSet);The IDisposable would be an InvokeOnDisposal() that the caller should dispose of when they wish to stop showing the scoped beatmap. This disposable would be responsible for restoring the beatmap, hopefully in a way that centralises this logic to one confined operation.
The way this is currently written just gives me little to no confidence that this flow won't fire in the wrong circumstances accidentally, will fire in all right applicable circumstances, etc.
Either way any solution here probably needs some further testing with things like changing ruleset inside scoped mode, etc. to give better guarantees that this doesn't fire in some unwanted circumstance.
This is out of scope to fix even on a UX / user expectation level. Not sure why exiting out of song select should always preserve scroll position now. |
It would be better if the scroll position always got preserved; why would anyone want their scrolling to randomly be at the top? But it's fine, I personally don't care about it and it's a very minor issue, I just noticed it while testing the PR and thought I'd mention it in case it's a simple fix which might as well be included. |
|
I've added a bunch of tests for cases where the selection would be changed through some other way on top of the revert-on-unscope behavior implemented here (changing ruleset, filters. etc.). Ultimately I didn't end up using |
|
|
||
| /// <summary> | ||
| /// Contains the currently scoped beatmapset. Used by external consumers for displaying its state. | ||
| /// Cannot be used to change the value, any changes must be done through <see cref="ScopeToBeatmapSet"/> |
There was a problem hiding this comment.
If this "cannot be used to change the value" anymore then it should not be exposed as Bindable anymore. Use IBindable which is immutable (doesn't have an exposed setter on Value).
| FilterCompleted?.Invoke(); | ||
| HandleFilterCompleted(); |
There was a problem hiding this comment.
FilterCompleted should not exist. Definitely not in Carousel, possibly inside HandleFilterCompleted() in BeatmapCarousel, and hopefully not at all (though I am not holding my breath for that last one).
As is this looks turbo weird with how Carousel has essentially two ways of hooking into the same operation for little reason.
|
|
||
| showConvertedBeatmaps = config.GetBindable<bool>(OsuSetting.ShowConvertedBeatmaps); | ||
|
|
||
| leasedScopedBeatmapSet = ScopedBeatmapSet.BeginLease(false); |
There was a problem hiding this comment.
What's with the lease? I'd hope we don't have to do that at all.
There was a problem hiding this comment.
I misunderstood how to make a bindable read only. Will nuke this and use IBindable instead.
| leasedScopedBeatmapSet.Value = null; | ||
| } | ||
|
|
||
| private void filterCompleted() |
There was a problem hiding this comment.
Does this need to hook into the filtering flow there? Can't we just do
diff --git a/osu.Game/Screens/SelectV2/SongSelect.cs b/osu.Game/Screens/SelectV2/SongSelect.cs
index 1e40877666..6964c5034d 100644
--- a/osu.Game/Screens/SelectV2/SongSelect.cs
+++ b/osu.Game/Screens/SelectV2/SongSelect.cs
@@ -271,7 +271,6 @@ private void load(AudioManager audio, OsuConfigManager config)
RequestPresentBeatmap = b => SelectAndRun(b, OnStart),
RequestSelection = queueBeatmapSelection,
RequestRecommendedSelection = requestRecommendedSelection,
- FilterCompleted = filterCompleted,
NewItemsPresented = newItemsPresented,
},
noResultsPlaceholder = new NoResultsPlaceholder
@@ -1264,18 +1263,9 @@ public void UnscopeBeatmapSet()
return;
leasedScopedBeatmapSet.Value = null;
- }
-
- private void filterCompleted()
- {
- if (carousel.CurrentGroupedBeatmap == null)
- return;
-
- if (beforeScopedSelection == null)
- return;
-
- if (!carousel.GetCarouselItems()!.Any(i => i.Model is GroupedBeatmap g && g.Equals(carousel.CurrentGroupedBeatmap)))
+ if (beforeScopedSelection != null)
queueBeatmapSelection(beforeScopedSelection);
+ beforeScopedSelection = null;
}
#endregion
I guess the concern might have been that if it's done that way then the selection will change before the filter completes which looks kinda twitchy? But I dunno, I think that's fine, I'd rather have it look a little twitchy than be worse code. (Which maybe betrays that my priorities are backwards, but I'd be running this past @peppy anyway before a merge.)
There was a problem hiding this comment.
That's what I did initially, but this causes the selection to be always reverted, even if the current one is still visible after unscoping. This might be fine but I feel that is something that could break expectations.
The selection changing before the filter completing didn't look that bad actually when I was testing it.
There was a problem hiding this comment.
I think always reverting the selection to the one before scoping is probably OK? The reporter of the issue thought it was acceptable as well.
I'd personally say let's start there and we can complicate later as required.
| } | ||
|
|
||
| public Bindable<BeatmapSetInfo?> ScopedBeatmapSet => filterControl.ScopedBeatmapSet; | ||
| private GroupedBeatmap? beforeScopedSelection; |
There was a problem hiding this comment.
Also incidentally does anything ever clear this to null? I don't see that being done, and that spooks me. It should be cleared when scoping is off just to decrease possibility of Hijinks™.
There was a problem hiding this comment.
I had a line that's supposed to clear it, but I must have accidentally remove it when I was experimenting with the whole "wait until filters complete to figure out if current selection will be visible after unscoping" thing.
|
Here's how the transition looks with the selection being reverted before the filtering happens: 2026-02-13.16-47-00.mp4I think it actually telegraphs the intention of reverting to the pre-scope selection pretty well. |
Resolves #36288.
If the current selection is still available after leaving scoped mode, it's left as is. If it's not, the selection from before entering scoped mode is restored.
2026-02-04.14-07-31.mp4