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 ability to cycle slider control point types via keyboard #28509

Merged
merged 18 commits into from
Jul 4, 2024

Conversation

bdach
Copy link
Collaborator

@bdach bdach commented Jun 17, 2024

RFC. Supersedes / closes #27035.

2024-06-17.15-58-20.mp4

Recent feedback has indicated that users deem having to right-click-and-context-menu to change slider control point type too cumbersome. While the original stable of shift-click can't currently work well, here's an alternative proposal.

  • During slider placement, press Tab and Shift-Tab to change the type of the first control point in the currently-placed segment.
  • During slider placement, press S to start a new segment of the path. While this was previously possible via double-clicking, I added this because I realised I didn't know how a user would do this before I looked into source, and the double-click flow seemed really janky.
  • After the slider has been placed, select a single anchor and press Tab and Shift-Tab to change the type of its anchor. In contrast to slider placement, if selecting a non-first control point, inherit is an available option when cycling.
  • Additionally, the following bindings are available for using specific curve types:
    • Alt-1 is linear
    • Alt-2 is bezier
    • Alt-3 is perfect curve
    • Alt-4 is B-spline
    • Alt-5 is inherited (only applicable in selection mode)

The bindings are not rebindable because I would have to put an osu!-specific editor binding in the main project which feels wrong.

@OliBomby
Copy link
Contributor

OliBomby commented Jun 17, 2024

Double-click to create a new segment is something that blew over from stable, so mappers coming from stable should be familiar with that flow. That being said I'm not against adding a hotkey for this. It saves clicks in case you want to make a lot of segments, but it might as well go unnoticed until you look into the source.

I tried out the changes and I like the ability to press Tab to cycle anchor type over the context menu already, but I did find myself never using the Shift-Tab to cycle backwards and instead just pressed Tab until I got the type I'm looking for. I don't think you can reasonably optimize your keypresses by using the Shift-Tab to cycle backwards because its hard to predict the what control point types are in the cycle and how many keypresses is going to get you to the type you want. It might be preferrable to have specific hotkeys for each control point type, for example Tab+number keys.

@bdach
Copy link
Collaborator Author

bdach commented Jun 17, 2024

It might be preferrable to have specific hotkeys for each control point type, for example Tab+number keys

Considered this but I dunno. Q~P row is taken by tool selection, Z~M row is taken by playback hotkeys, so pretty much only the A~L row or the numbers are left. Tab + number seems like a very weird combination and will be annoying to support properly, would rather do an actual modifier like Shift, Ctrl or Alt w/ a number if anything.

@OliBomby
Copy link
Contributor

OliBomby commented Jun 17, 2024

Considered this but I dunno. QP row is taken by tool selection, ZM row is taken by playback hotkeys, so pretty much only the A~L row or the numbers are left. Tab + number seems like a very weird combination and will be annoying to support properly, would rather do an actual modifier like Shift, Ctrl or Alt w/ a number if anything.

I agree. Shift, Ctrl or Alt w/ a number are all reasonable options. Shift+number seems to already control the beat snap divisor but it might be fine to overwrite if a slider control point is selected because slider control points can not be beat snapped. Otherwise Alt+number seems like the most comfortable option, because you can reach Alt quite easily with your thumb.

@bdach
Copy link
Collaborator Author

bdach commented Jun 17, 2024

Otherwise Alt+number seems like the most comfortable option

I've implemented this.

  • Alt-1 is linear
  • Alt-2 is bezier
  • Alt-3 is perfect curve
  • Alt-4 is B-spline
  • Alt-0 is inherited (only applicable in selection mode)

@OliBomby
Copy link
Contributor

I've implemented this.

  • Alt-1 is linear
  • Alt-2 is bezier
  • Alt-3 is perfect curve
  • Alt-4 is B-spline
  • Alt-0 is inherited (only applicable in selection mode)

Yeah this is awesome. It's really easy to change control point types now.
The Alt-0 is a bit far away though. Maybe it's better to have:

  • Alt-1 is inherited
  • Alt-2 is linear
  • Alt-3 is bezier
  • Alt-4 is perfect curve
  • Alt-5 is B-spline

Also these hotkeys should work when multiple control points are selected, mirroring functionality of using the context menu.

@peppy
Copy link
Member

peppy commented Jun 18, 2024

Could we show the bindings somewhere on the screen to make the user aware of them? Especially the "S for new segment". Either:

  • Tooltip of slider tool button
  • Tooltip of the placement node (so it follows the cursor around). Might work well, might be cumbersome
  • Somewhere floating in the editor during slider placement (top centre? bottom centre?)

@bdach
Copy link
Collaborator Author

bdach commented Jun 18, 2024

The Alt-0 is a bit far away though

Addressed this but in a slightly different way (inherited is alt-5 now). Rationale is that I really wanna keep these consistent between placement & selection and I don't like starting the placement ones at alt-2.

Also these hotkeys should work when multiple control points are selected, mirroring functionality of using the context menu.

Is fixed.

Could we show the bindings somewhere on the screen to make the user aware of them?

I did a jank tooltip to avoid inventing new UI.

osu_2024-06-18_08-15-40

I would have liked to split lines on the tooltip but that is... difficult, for structural reasons (that tooltip there is not easily customisable without deriving 3 other components).

@peppy peppy self-requested a review June 18, 2024 08:29
@bdach bdach self-assigned this Jun 19, 2024
@pull-request-size pull-request-size bot added size/XL and removed size/L labels Jul 3, 2024
@peppy
Copy link
Member

peppy commented Jul 3, 2024

I made a couple of changes here:

  • Context menu now matches the order of hotkeys. I feel this does a lot to improve the UX as you now have a visual listing which you can refer to to figure out the hotkeys.
  • Context menu now updates when using hotkeys, giving further feedback as to what is actually happening when you are using them.
2024-07-04.00.23.00.mp4

@bdach requesting a check of the last two commits since I changed a considerable amount of code.

peppy
peppy previously approved these changes Jul 3, 2024
peppy
peppy previously approved these changes Jul 3, 2024
@bdach
Copy link
Collaborator Author

bdach commented Jul 4, 2024

Changes look good to me 👍

@peppy peppy merged commit 3f13848 into ppy:master Jul 4, 2024
13 of 17 checks passed
@bdach bdach deleted the slider-anchor-type-switching branch July 4, 2024 07:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants