Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add alternative for random beatmap selection #888

Merged
merged 5 commits into from Jun 1, 2017

Conversation

4 participants
@Drenata
Copy link
Contributor

commented May 31, 2017

"Never repeat" will not repeat until all songs have been seen by repeatedly pressing F2/Random button

Drenata added some commits May 31, 2017

Add alternative for random beatmap selection
"Never repeat" will not repeat until all songs have been seen by repeatedly pressing F2/Random button
BeatmapGroup group;
if (randomType == SelectionRandomType.RandomPermutation)
{
List<BeatmapGroup> notSeenGroups = visibleGroups.Except(seenGroups).ToList();

This comment has been minimized.

Copy link
@Tom94

Tom94 May 31, 2017

Contributor

Actually, nevermind. Revisiting the surrounding code I think I prefer your solution quite a bit. :)

This comment has been minimized.

Copy link
@smoogipoo

smoogipoo May 31, 2017

Contributor

Is it possible to use IEnumerable here instead of ToList() (replacing the below notSeenGroups[...] with notSeenGroups.ElementAt(...). This is going to be a pretty chunky allocation with a lot of beatmaps.

@@ -20,6 +20,8 @@ protected override void InitialiseDefaults()
Set(OsuSetting.DisplayStarsMinimum, 0.0, 0, 10);
Set(OsuSetting.DisplayStarsMaximum, 10.0, 0, 10);

Set(OsuSetting.SelectionRandomType, SelectionRandomType.Random);

This comment has been minimized.

Copy link
@Tom94

Tom94 May 31, 2017

Contributor

I think it's totally fine to just make your permutation-based randomness the default (and only) random selection method. @peppy ?

This comment has been minimized.

Copy link
@peppy

peppy Jun 1, 2017

Member

I think it's a better default, but let's leave the setting for the time being. Since we can easily add more settings (and will add more filtering/grouping of the settings menu in the future) I see no harm in doing so.

Please change the default to this algorithm, though.

@@ -70,6 +72,9 @@ public IEnumerable<BeatmapSetInfo> Beatmaps

private readonly List<BeatmapGroup> groups = new List<BeatmapGroup>();

private Bindable<SelectionRandomType> randomType;
private readonly HashSet<BeatmapGroup> seenGroups = new HashSet<BeatmapGroup>();

This comment has been minimized.

Copy link
@smoogipoo

smoogipoo May 31, 2017

Contributor

Any reason why this is a HashSet instead of a List?

This comment has been minimized.

Copy link
@Drenata

Drenata Jun 1, 2017

Author Contributor

HashSet in order to better show that the order in which groups have been shown is not important and that there should be no duplicates. It is not strictly a necessity though, C# should be able to run O(M+N) set difference on lists too, right?

This comment has been minimized.

Copy link
@smoogipoo

smoogipoo Jun 1, 2017

Contributor

The existence of duplicates doesn't matter in the first place given that Except() is used, and shouldn't be added to the list anyway. This is less efficient than using List because .NET internally allocates a HashSet with the elements in the parameter passed to Except() (so really, you're allocating and filling two hashsets).

@peppy

peppy approved these changes Jun 1, 2017

@peppy peppy merged commit 900e3ce into ppy:master Jun 1, 2017

1 check passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.