-
-
Notifications
You must be signed in to change notification settings - Fork 2.6k
Change song select grouping to be divided into 10 BPM groups #34381
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
Conversation
f8bb3b2 to
e34f181
Compare
e34f181 to
c6cbda5
Compare
| if (bpm < 60) | ||
| return new GroupDefinition(60, "Under 60 BPM"); | ||
|
|
||
| for (int i = 60; i < 300; i += 10) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| for (int i = 60; i < 300; i += 10) | |
| for (int i = 70; i < 300; i += 10) |
Starting at i = 60 means there's an extra dead iteration of the loop every time.
| private GroupDefinition defineGroupByBPM(double bpm) | ||
| { | ||
| for (int i = 1; i < 6; i++) | ||
| if (bpm < 60) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is sorta orthogonal to the issue here but I do wonder about using raw unrounded BPM, because on displays we use
osu/osu.Game/Utils/FormatUtils.cs
Lines 70 to 75 in 0d388cc
| /// <summary> | |
| /// Applies rounding to the given BPM value. | |
| /// </summary> | |
| /// <param name="baseBpm">The base BPM to round.</param> | |
| /// <param name="rate">Rate adjustment, if applicable.</param> | |
| public static int RoundBPM(double baseBpm, double rate = 1) => (int)Math.Round(baseBpm * rate); |
and therefore stuff can be wonky (i.e. a 59.6 BPM beatmap being grouped into <60 BPM group but displaying as 60 BPM).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, seems like a good opportunity to fix / test coverage this.
As proposed in #34152.