SongSelectV2: Refine random selection to currently open group (and support difficulty split panels better) #33773
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…aying individual difficulties Previously, random selection would always be done at a *set* level. The final operation of a random action would be "select the user's recommended difficulty from this randomly selected set". This makes no sense when sets are not grouped together at song select. In fact, it is completely broken with the previous commit which adds group-isolated random support – if we're grouping by difficulty and the user's recommendation is not in the current group it would throw the user into another group unexpectedly. This fixes the issue by splitting out the random implementation into two separate pathways depending on the carousel display mode.
Noticed in a flaky test with the changes to random, where the debounce may be delayed to the point of thinking the selection is invalid even though it's valid (timing woes, I cannot explain in words but I would highly recommend smiling and nodding approach).
peppy
force-pushed
the
fix-carousel-random-not-respecting-group
branch
from
2362c2a to
873d622
Compare
bdach
requested changes
Jun 24, 2025
| // | ||
| // If this becomes an issue, we could either store a mapping, or run the random algorithm many times | ||
| // using the `SetItems` method until we get a group HIT. | ||
| ? grouping.GroupItems[ExpandedGroup].Select(i => i.Model).OfType<BeatmapInfo>().ToArray() |
There was a problem hiding this comment.
This can hard crash:
- Start with any grouping mode chosen (as long as it's not off)
- Change to any other grouping mode (or turn grouping off)
- Press random
Unhandled exception. System.Collections.Generic.KeyNotFoundException: The given key 'GroupDefinition { Order = 2, Title = C }' was not present in the dictionary.
at System.Collections.Generic.Dictionary`2.get_Item(TKey key)
at osu.Game.Screens.SelectV2.BeatmapCarousel.nextRandomSet() in /Users/bdach/Documents/Work/open-source/osu/osu.Game/Screens/SelectV2/BeatmapCarousel.cs:line 724
at osu.Game.Screens.SelectV2.BeatmapCarousel.NextRandom() in /Users/bdach/Documents/Work/open-source/osu/osu.Game/Screens/SelectV2/BeatmapCarousel.cs:line 656
at osu.Game.Screens.SelectV2.SongSelect.<CreateFooterButtons>b__60_1() in /Users/bdach/Documents/Work/open-source/osu/osu.Game/Screens/SelectV2/SongSelect.cs:line 289
at osu.Game.Screens.SelectV2.FooterButtonRandom.<load>b__12_0() in /Users/bdach/Documents/Work/open-source/osu/osu.Game/Screens/SelectV2/FooterButtonRandom.cs:line 91
There was a problem hiding this comment.
Good catch. I believe this (and the second reported point of contention) should be fixed now.
There was a problem hiding this comment.
a1ec71e fixes the crash, sure. But in testing that same scenario of changing the grouping mode, I can now produce a situation wherein the random key can now switch to a completely different group after changing grouping modes, even if the selection hasn't changed:
Screen.Recording.2025-06-24.at.11.02.05.mov
There was a problem hiding this comment.
Sorry, made one more attempt.
bdach
approved these changes
Jun 24, 2025
|
Yeah looks good, send it. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The goal of this pull request was to fix the random button not respecting groups. But in the process of adding test coverage I noticed that I'd also need to fix a larger issue with random selection. The group fix change doesn't work well on its own so I've bundled these together.
Fix random button not respecting open group
Closes #33569.
Fix random selection not working as expected when displaying individual difficulties
Previously, random selection would always be done at a set level. The final operation of a random action would be "select the user's recommended difficulty from this randomly selected set".
This makes no sense when sets are not grouped together at song select. In fact, it is completely broken with the previous commit which adds group-isolated random support – if we're grouping by difficulty and the user's recommendation is not in the current group it would throw the user into another group unexpectedly.
This fixes the issue by splitting out the random implementation into two separate pathways depending on the carousel display mode.