-
-
Notifications
You must be signed in to change notification settings - Fork 2.3k
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
Make ctrl-up/down change speed modifier of mods #28071
Conversation
Osu lazer speedkeys
|
What is the reason for wanting this? Why should DT/HT get preferential hotkey treatment over every other mod in the game? Was this feature discussed anywhere at all before this PR? |
(Not lazer dev, just my opinion) The hotkey for this cannot be PgUp/PgDown, since those are used to scroll the carousel, and I also don't like the semantic of PgUp/PgDown being changed like that, since those keys are almost always used to scroll, and not to adjust a setting. Maybe Ctrl+Up and Ctrl+Down? |
I generally prefer all unsolicited feature requests to go through discussions first in order to reduce wasted contributor effort and to gauge interest in the feature. At this time I have no idea as to whether anyone but yourself wants this feature to exist. UX-wise I have a whole mess of doubts.
Not to mention something like wind up/down that also have speed adjustments but don't get any special hotkeys. |
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.
Missing unit tests
case GlobalAction.IncreaseSpeed: | ||
increaseSpeed(); | ||
return true; | ||
|
||
case GlobalAction.DecreaseSpeed: | ||
decreaseSpeed(); | ||
return true; |
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.
increaseSpeed()
and decreaseSpeed()
should probably be one method that accept a delta or something.
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.
Done in 3fdbd73
var rateAdjustStates = ModSelect.AllAvailableMods.Where(pair => pair.Mod is ModRateAdjust); | ||
var stateDoubleTime = ModSelect.AllAvailableMods.First(pair => pair.Mod is ModDoubleTime); | ||
bool rateModActive = ModSelect.AllAvailableMods.Count(pair => pair.Mod is ModRateAdjust && pair.Active.Value) > 0; |
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.
These should not be doing mod queries or mutations via ModSelect
. Use selectedMods
and OsuGameBase.AvailableMods
instead.
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.
Done in 3fdbd73
} | ||
|
||
// Find current active rateAdjust mod and modify speed, enable DoubleTime if necessary | ||
foreach (var state in rateAdjustStates) |
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 should be asserting that there is only one rate adjust mod present rather than doing some weird loop.
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.
Done in 3fdbd73
I created a Discussion about this feature. |
I guess the discussion seems to have gathered some support, but I'd still expect the review above to be addressed for this before this can be considered further. |
Sure, |
You can replace the entire list. |
Nightcore now switches into Daycore, keep Adjustpitch after change
|
This would already be supported if not for this early return: osu/osu.Game/Screens/Select/SongSelect.cs Lines 1138 to 1139 in 3fdbd73
Use the existing one. See usages elsewhere on how to display it. |
…Toast to nofity user what just happend
Seems like the toast implementation 148afd1 makes the test fail, locally they still run fine on my side Edit : Tests fail because the SpeedChange value of RateAdjustMods are not exactly 0.95 |
Before I gave up on attempting to fix the method.
@@ -809,6 +823,125 @@ public override bool OnBackButton() | |||
return false; | |||
} | |||
|
|||
public void ChangeSpeed(double delta) | |||
{ | |||
ModNightcore modNc = (ModNightcore)((MultiMod)game.AvailableMods.Value[ModType.DifficultyIncrease].First(mod => mod is MultiMod multiMod && multiMod.Mods.Count(modType => modType is ModNightcore) > 0)).Mods.First(mod => mod is ModNightcore); |
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 method needs a rewrite.
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.
Could you tell me what needs to change for the rewrite so i don't do the same mistake again? (more readable, more compact, ...)
{ | ||
public partial class SpeedChangeToast : Toast | ||
{ | ||
public SpeedChangeToast(OsuConfigManager config, double delta) |
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 probably should actually show the speed?
The very base class is the wrong place for it because `FreeModSelectOverlay` inherits from it, and that one has different assumptions and rules than "user" selection. In particular, in non-user selection, more than one rate adjust mod may be active, which breaks the mod speed hotkey's basic assumptions.
I've basically rewritten this.
@peppy requesting check as to whether you're willing to accept this feature or not, and if yes, on how the code looks now. |
I'm pretty happy with how this feature works (very cool) and now the code is sane. Let's 🚢 it. |
PageUp and PageDown now increase or decrease speed in song select.
Example
(Song Select screen and no mod is selected)
same goes with PageUp and DoubleTime.
If Daycore or Nightcore are currently selected it increases those.
If the minimum rate gets surpassed Half-/Double-Time get applied
Video of it in action:
https://github.com/ppy/osu/assets/28962347/e95228d2-0e77-4ec8-9e60-17472bdc4545
Im open to any suggestions for changes and i hope this feature finds its way into osu!lazer