Add more ways to seek to sample points #28737

OliBomby · 2024-07-04T09:51:14Z

This feature unlocks a powerful UX that allows users to quickly edit multiple samples by double-clicking and using hotkeys, without having to move the mouse cursor.

bdach · 2024-07-04T13:49:01Z

I'm gonna be frank, I'm not sure I see what is "powerful" in this.

For adding additional sounds, W~T keys are faster.
For changing bank of all sounds at once, W~R keys are faster.
For setting volume or setting separate normal/addition banks, clicking once is faster than clicking twice. And then if you want to change another sound, you have to move the mouse again anyway, albeit maybe less so than without the seek.

Is it the seek that's supposed to be making this "powerful"? Then let's maybe just have the popover seek every time?

In general I'm not sure this is going to be winning many favours. I've already heard from multiple feedback sources that the sample pieces on the timeline are difficult to work with when the timeline has high object density.

I'm honestly thinking we will inevitably have to go to the stable model of having clickable slider nodes themselves on playfield and/or timeline. That said I would want @peppy buy-in on this, because it has severe implications, such as killing the "all rulesets get a single unchanging implementation of timeline" premise, but I personally think that premise is dead in the water already if we wanna support everyone's use cases properly (case in point: scrolling rulesets and recurring complaints that you can't see scroll speed changes on timeline - which is because we moved them to timing screen).

OliBomby · 2024-07-04T16:29:32Z

I'm gonna be frank, I'm not sure I see what is "powerful" in this.

For adding additional sounds, W~T keys are faster.

For changing bank of all sounds at once, W~R keys are faster.

It's powerful in combination with these features. Mappers usually have a specific pattern of hitsounds in mind and need to apply different hitsounds to each object. The automatic seek allows them to: click - press keys for hs - click - press keys for hs - ...
All without moving the mouse if its on the same snap. It optimizes the flow ever so slightly.

Is it the seek that's supposed to be making this "powerful"? Then let's maybe just have the popover seek every time?

I think this is also acceptable UX.

I'm honestly thinking we will inevitably have to go to the stable model of having clickable slider nodes themselves on playfield and/or timeline.

Does this imply removing the sample point pieces entirely? Would be good to know before I try redesigning them again to hopefully make them bearable to use.

I'm not against going to the stable model, it works pretty well and we can potentially have a separate hitsounding tab for more advanced hitsounds or modding.

it has severe implications, such as killing the "all rulesets get a single unchanging implementation of timeline" premise, but I personally think that premise is dead in the water already if we wanna support everyone's use cases properly (case in point: scrolling rulesets and recurring complaints that you can't see scroll speed changes on timeline - which is because we moved them to timing screen).

I'm also in favour of allowing ruleset-specific implementations of the timeline. I ran into issues with this back when #21566

bdach · 2024-07-05T06:10:57Z

Does this imply removing the sample point pieces entirely? Would be good to know before I try redesigning them again to hopefully make them bearable to use.

I'm honestly not sure. I haven't really tried to come up with what the alternative would be yet. Maybe they would stay, but as display-only and not be directly interactable.

peppy · 2024-07-05T09:50:50Z

I don't know, this feels really awkward with its current implementation:

osu.2024-07-05.at.09.48.11.mp4

Rather than this, would it not be better to have something like Alt+Left/Right skip to the next object (generally in editor) and making Shift+Alt+Left/Right bring up the samples edit box or similar?

peppy · 2024-07-05T09:53:41Z

As for a better path forward, I think there should be a way to expand the timeline out and reveal a hitsounding section (can take up as much height as required) or even a separate hitsounding mode as suggested by @OliBomby where using left/right arrows seeks between hitsound nodes (similar to keyframe skipping in a video editor).

bdach · 2024-07-05T09:59:05Z

I think there should be a way to expand the timeline out and reveal a hitsounding section (can take up as much height as required) or even a separate hitsounding mode

I would generally be on board with this (have floated the same previously) but I'm not super sure all users will be happy with just that. Some are used to being able to seamlessly hitsound while mapping and as such, these types of modality changes where you need to go into and out of a special hitsounding mode are not likely to work super well for them.

OliBomby · 2024-07-05T12:37:23Z

I've implemented both of your suggestions.

It now seeks on click instead of double-click. I think this makes it a lot less jank.
I've added alt + left/right to skip to the next object.
I've added shift + alt + left/right to skip to the next sample point and automatically open it. This will be very useful for accessing those very densely packed sample points.

peppy · 2024-07-07T04:20:28Z

I'm not 100% sure about the auto scroll on single click.

peppy · 2024-07-07T04:21:53Z

osu.Game/Screens/Edit/Editor.cs

        private void seek(UIEvent e, int direction)
        {
+            if (e.AltPressed)


To make this work, I need to apply this patch:

diff --git a/osu.Game/Overlays/Volume/VolumeControlReceptor.cs b/osu.Game/Overlays/Volume/VolumeControlReceptor.cs index 4ddbc9dd48..2e8d86d4c7 100644 --- a/osu.Game/Overlays/Volume/VolumeControlReceptor.cs +++ b/osu.Game/Overlays/Volume/VolumeControlReceptor.cs @@ -23,15 +23,15 @@ public bool OnPressed(KeyBindingPressEvent<GlobalAction> e) { case GlobalAction.DecreaseVolume: case GlobalAction.IncreaseVolume: - ActionRequested?.Invoke(e.Action); - return true; + return ActionRequested?.Invoke(e.Action) == true; case GlobalAction.ToggleMute: case GlobalAction.NextVolumeMeter: case GlobalAction.PreviousVolumeMeter: if (!e.Repeat) - ActionRequested?.Invoke(e.Action); - return true; + return ActionRequested?.Invoke(e.Action) == true; + + return false; } return false; diff --git a/osu.Game/Overlays/VolumeOverlay.cs b/osu.Game/Overlays/VolumeOverlay.cs index 5470c70400..fa6e797c9c 100644 --- a/osu.Game/Overlays/VolumeOverlay.cs +++ b/osu.Game/Overlays/VolumeOverlay.cs @@ -120,14 +120,18 @@ public bool Adjust(GlobalAction action, float amount = 1, bool isPrecise = false return true; case GlobalAction.NextVolumeMeter: - if (State.Value == Visibility.Visible) - volumeMeters.SelectNext(); + if (State.Value != Visibility.Visible) + return false; + + volumeMeters.SelectNext(); Show(); return true; case GlobalAction.PreviousVolumeMeter: - if (State.Value == Visibility.Visible) - volumeMeters.SelectPrevious(); + if (State.Value != Visibility.Visible) + return false; + + volumeMeters.SelectPrevious(); Show(); return true;

I think this is probably fine though.

OliBomby · 2024-07-07T19:56:51Z

I'm not 100% sure about the auto scroll on single click.

I'll ask some mappers for feedback about this. I'm also not 100% sure.

Next to your patch I also disabled Alt+Scroll to seek, which I had previously enabled. We probably don't want to disable the ability to change volume in the editor with Alt+Scroll.

Edit:
I got feedback from DeviousPanda. He seems most excited about the ability to seek to nearby sample points with the keybinds. Particularly he liked the scroll functionality for seeking to nearby sample points, so I think I'll turn that into a configurable keybind so he can rebind it to the scrollwheel if he wants.

About the auto seek on single click. He said it brings lazer closer to stable functionality, so it might be a good change. I think its fine to leave it in then and see if people complain.

OliBomby · 2024-08-20T21:21:48Z

Ah I see the problem now. It seems I already fixed this in #28772 but this PR wasn't updated since.

Sometimes the popover doesn't go to the seeked object. Also I feel like adding the arrow is a good UX improvement because before I wouldn't know what I'm editing.

I like that triangle. I'll add that as well.

bdach · 2024-08-21T09:30:00Z

The arrow key to move around thing seems pretty cool for whatever it's worth.

Adding the arrow to the popover though, not sure. Weird to be adding it only in one place. The reason why I didn't originally do this even though Popover supports it is that it doesn't work with shadows:

Note that the shadow draws over the arrow. It's not super trivial to fix.

Maybe we need selection state to show on the sample/velocity pieces themselves. Probably with something like a yellow border or whatever, to match blueprints. It's something that I had in mind before, in that it's kinda difficult to tell right now which pieces you're editing specifically at any given time, especially when multiple objects are selected at a time.

This reverts commit 1ce9e97.

OliBomby · 2024-08-21T12:30:49Z

Adding the arrow to the popover though, not sure. Weird to be adding it only in one place. The reason why I didn't originally do this even though Popover supports it is that it doesn't work with shadows:

Note that the shadow draws over the arrow. It's not super trivial to fix.

Maybe we need selection state to show on the sample/velocity pieces themselves. Probably with something like a yellow border or whatever, to match blueprints. It's something that I had in mind before, in that it's kinda difficult to tell right now which pieces you're editing specifically at any given time, especially when multiple objects are selected at a time.

Seems like those solutions are all not trivial, so I'll keep it out of this PR.

I think the yellow border on the sample pieces makes sense if they are also individually selectable like hitobjects in the timeline.

peppy · 2024-08-21T14:44:48Z

I was also very tempted to add the speech bubble marker. I wouldn't mind if it was just applied here for now, but either or I guess.

peppy · 2024-08-22T06:36:36Z

osu.Game/Screens/Edit/Editor.cs

@@ -751,6 +752,22 @@ public bool OnPressed(KeyBindingPressEvent<GlobalAction> e)
                    bottomBar.TestGameplayButton.TriggerClick();
                    return true;

+                case GlobalAction.EditorSeekToPreviousHitObject:


I feel like these should allow key repeat, feels better?

diff --git a/osu.Game/Screens/Edit/Editor.cs b/osu.Game/Screens/Edit/Editor.cs index 2933c89cd8..355d724434 100644 --- a/osu.Game/Screens/Edit/Editor.cs +++ b/osu.Game/Screens/Edit/Editor.cs @@ -714,6 +714,26 @@ protected override bool OnScroll(ScrollEvent e) public bool OnPressed(KeyBindingPressEvent<GlobalAction> e) { + // Repeatable actions + switch (e.Action) + { + case GlobalAction.EditorSeekToPreviousHitObject: + seekHitObject(-1); + return true; + + case GlobalAction.EditorSeekToNextHitObject: + seekHitObject(1); + return true; + + case GlobalAction.EditorSeekToPreviousSamplePoint: + seekSamplePoint(-1); + return true; + + case GlobalAction.EditorSeekToNextSamplePoint: + seekSamplePoint(1); + return true; + } + if (e.Repeat) return false; @@ -751,26 +771,9 @@ public bool OnPressed(KeyBindingPressEvent<GlobalAction> e) case GlobalAction.EditorTestGameplay: bottomBar.TestGameplayButton.TriggerClick(); return true; - - case GlobalAction.EditorSeekToPreviousHitObject: - seekHitObject(-1); - return true; - - case GlobalAction.EditorSeekToNextHitObject: - seekHitObject(1); - return true; - - case GlobalAction.EditorSeekToPreviousSamplePoint: - seekSamplePoint(-1); - return true; - - case GlobalAction.EditorSeekToNextSamplePoint: - seekSamplePoint(1); - return true; - - default: - return false; } + + return false; } public void OnReleased(KeyBindingReleaseEvent<GlobalAction> e)

peppy · 2024-08-22T06:41:44Z

osu.Game/Screens/Edit/Editor.cs

+        }
+
+        [CanBeNull]
+        public event Action<double> ShowSampleEditPopoverRequested;


This flow is a bit ugly. Probably fine maybe but still ugly.

Move public things to top of file and document though.

peppy · 2024-08-27T05:14:57Z

@OliBomby I'm thinking we should change this to be Ctrl instead of Alt to match user expectations (ie. matches traversing text by words in a text box). What say you?

osu.Game/Screens/Edit/Compose/Components/Timeline/SamplePointPiece.cs

peppy · 2024-08-27T05:21:16Z

osu.Game/Screens/Edit/Compose/Components/Timeline/SamplePointPiece.cs

        }

        protected override bool OnClick(ClickEvent e)
        {
+            editorClock?.SeekSmoothlyTo(GetTime());


I'm still not sure about this UX. Maybe best to only trigger this seek when using the seek keys?

Results in big reach:

osu.2024-08-27.at.05.20.36.mp4

I see your point. I'm not sure what you mean by the seek keys. Should it only seek when you click it while Control is held?

I think it might be best to trigger the seek on a right click maybe, or some other mouse button, because I expect the mapper to be already using the hotkeys for assigning hitsounds when using this feature, so adding modifiers on click makes the flow more complicated.
Also right click currently does nothing on this drawable.

I meant to only seek when using the new keyboard navigation you've added. If you click on them directly it wouldn't seek.

diff --git a/osu.Game/Screens/Edit/Compose/Components/Timeline/SamplePointPiece.cs b/osu.Game/Screens/Edit/Compose/Components/Timeline/SamplePointPiece.cs index 488cd288e4..a8cf8723f2 100644 --- a/osu.Game/Screens/Edit/Compose/Components/Timeline/SamplePointPiece.cs +++ b/osu.Game/Screens/Edit/Compose/Components/Timeline/SamplePointPiece.cs @@ -72,22 +72,16 @@ protected override void Dispose(bool isDisposing) private void onShowSampleEditPopoverRequested(double time) { - if (Precision.AlmostEquals(time, GetTime())) - this.ShowPopover(); - } + if (!Precision.AlmostEquals(time, GetTime())) return; - protected override bool OnClick(ClickEvent e) - { + editorClock?.SeekSmoothlyTo(GetTime()); this.ShowPopover(); - return true; } - protected override void OnMouseUp(MouseUpEvent e) + protected override bool OnClick(ClickEvent e) { - if (e.Button != MouseButton.Right) return; - - editorClock?.SeekSmoothlyTo(GetTime()); this.ShowPopover(); + return true; } private void updateText()

I would've liked to keep the right-click navigation but I assume that's a peppy-no-go

I dunno. It's too many random UX behaviours. I want to avoid this unless it's well defined (ie. something that matches other apps, or something implemented everywhere in lazer).

It's a no-go for this PR.

OliBomby · 2024-08-27T17:21:24Z

@OliBomby I'm thinking we should change this to be Ctrl instead of Alt to match user expectations (ie. matches traversing text by words in a text box). What say you?

That makes sense yeah.
I'll go with Ctrl+arrow keys for hitobject seek and Alt+arrow keys for sample point seek.

The only pain is that if you want to change they hotkeys from arrow keys to scroll wheel there will not be an obvious conversion for the sample point seek since alt+scroll is already used for the volume control, so you'll have to go for the only remaining combination which is Alt+Shift+scroll. But I guess its not a big deal since you can also change the hotkeys for beat snap and distance spacing to free up hotkeys.

peppy · 2024-08-28T03:00:35Z

Can we not keep the ctrl-shift for sample points? I feel like we may want to keep alt for other usages in the future. Or leave it unused.

OliBomby · 2024-08-28T07:57:40Z

Can we not keep the ctrl-shift for sample points? I feel like we may want to keep alt for other usages in the future. Or leave it unused.

That also works I guess.

peppy

As proposed.

Allow seeking to sample point on double-click

b9c6674

pull-request-size bot added the size/S label Jul 4, 2024

Add test coverage

00f7a34

pull-request-size bot added size/M and removed size/S labels Jul 4, 2024

bdach added the area:editor label Jul 4, 2024

peppy self-requested a review July 5, 2024 09:46

OliBomby added 3 commits July 5, 2024 12:41

alt left/right or scroll to seek to neighbouring hit objects

98610f4

shift alt seek to open next sample edit popover

7d6ade7

always seek on click

8d46d6c

pull-request-size bot added size/L and removed size/M labels Jul 5, 2024

OliBomby added 2 commits July 5, 2024 14:33

fix naming violation

c05f489

update tests

9013c11

OliBomby changed the title ~~Allow seeking to sample point on double-click~~ Add more ways to seek to sample points Jul 5, 2024

clarify logic

ba44757

peppy reviewed Jul 7, 2024

View reviewed changes

OliBomby added 2 commits July 7, 2024 21:33

prevent volume control from eating inputs

5da8bb5

allow alt scroll for volume in editor

f36321a

Make hit object and sample point seek keybinds configurable

306dc37

peppy self-requested a review August 19, 2024 04:15

Merge remote-tracking branch 'upstream/master' into doubleclick

ae6d855

add arrow indicator

1ce9e97

Revert "add arrow indicator"

423fead

This reverts commit 1ce9e97.

peppy reviewed Aug 22, 2024

View reviewed changes

OliBomby added 2 commits August 22, 2024 19:15

allow repeating on seek actions

0db068e

move public member to top of file

adbdb39

peppy self-requested a review August 27, 2024 05:09

peppy reviewed Aug 27, 2024

View reviewed changes

OliBomby added 2 commits August 27, 2024 19:02

fix difference in sample time calculation

71044a0

Add float comparison leniency just in case

daad476

OliBomby added 2 commits August 27, 2024 19:40

change default seek hotkeys

1117fd5

Automatic seek to sample point on right-click

b5b4f91

change sample seek keybind to ctrl shift

cadbb0f

peppy self-requested a review August 29, 2024 09:38

peppy requested changes Aug 29, 2024

View reviewed changes

OliBomby added 3 commits August 29, 2024 19:34

dont seek on right-click, only on keyboard request

8fe7ab1

Merge remote-tracking branch 'upstream/master' into doubleclick

ba0c4df

fix test

3a1afda

peppy self-requested a review August 30, 2024 06:47

peppy approved these changes Aug 30, 2024

View reviewed changes

peppy merged commit a71bc3a into ppy:master Aug 30, 2024
10 of 13 checks passed

OliBomby deleted the doubleclick branch August 30, 2024 07:28

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add more ways to seek to sample points #28737

Add more ways to seek to sample points #28737

OliBomby commented Jul 4, 2024

bdach commented Jul 4, 2024 •

edited

Loading

OliBomby commented Jul 4, 2024

bdach commented Jul 5, 2024

peppy commented Jul 5, 2024

peppy commented Jul 5, 2024

bdach commented Jul 5, 2024

OliBomby commented Jul 5, 2024

peppy commented Jul 7, 2024

peppy Jul 7, 2024

OliBomby commented Jul 7, 2024 •

edited

Loading

OliBomby commented Aug 20, 2024 •

edited

Loading

bdach commented Aug 21, 2024 •

edited

Loading

OliBomby commented Aug 21, 2024

peppy commented Aug 21, 2024

peppy Aug 22, 2024

peppy Aug 22, 2024

peppy commented Aug 27, 2024

peppy Aug 27, 2024

OliBomby Aug 27, 2024 •

edited

Loading

peppy Aug 29, 2024 •

edited

Loading

OliBomby Aug 29, 2024

peppy Aug 29, 2024

OliBomby commented Aug 27, 2024

peppy commented Aug 28, 2024

OliBomby commented Aug 28, 2024

peppy left a comment

Add more ways to seek to sample points #28737

Add more ways to seek to sample points #28737

Conversation

OliBomby commented Jul 4, 2024

bdach commented Jul 4, 2024 • edited Loading

OliBomby commented Jul 4, 2024

bdach commented Jul 5, 2024

peppy commented Jul 5, 2024

peppy commented Jul 5, 2024

bdach commented Jul 5, 2024

OliBomby commented Jul 5, 2024

peppy commented Jul 7, 2024

peppy Jul 7, 2024

Choose a reason for hiding this comment

OliBomby commented Jul 7, 2024 • edited Loading

OliBomby commented Aug 20, 2024 • edited Loading

bdach commented Aug 21, 2024 • edited Loading

OliBomby commented Aug 21, 2024

peppy commented Aug 21, 2024

peppy Aug 22, 2024

Choose a reason for hiding this comment

peppy Aug 22, 2024

Choose a reason for hiding this comment

peppy commented Aug 27, 2024

peppy Aug 27, 2024

Choose a reason for hiding this comment

OliBomby Aug 27, 2024 • edited Loading

Choose a reason for hiding this comment

peppy Aug 29, 2024 • edited Loading

Choose a reason for hiding this comment

OliBomby Aug 29, 2024

Choose a reason for hiding this comment

peppy Aug 29, 2024

Choose a reason for hiding this comment

OliBomby commented Aug 27, 2024

peppy commented Aug 28, 2024

OliBomby commented Aug 28, 2024

peppy left a comment

Choose a reason for hiding this comment

bdach commented Jul 4, 2024 •

edited

Loading

OliBomby commented Jul 7, 2024 •

edited

Loading

OliBomby commented Aug 20, 2024 •

edited

Loading

bdach commented Aug 21, 2024 •

edited

Loading

OliBomby Aug 27, 2024 •

edited

Loading

peppy Aug 29, 2024 •

edited

Loading