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 more ways to seek to sample points #28737

Merged
merged 24 commits into from
Aug 30, 2024
Merged
Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
24 commits
Select commit Hold shift + click to select a range
b9c6674
Allow seeking to sample point on double-click
OliBomby Jul 4, 2024
00f7a34
Add test coverage
OliBomby Jul 4, 2024
98610f4
alt left/right or scroll to seek to neighbouring hit objects
OliBomby Jul 5, 2024
7d6ade7
shift alt seek to open next sample edit popover
OliBomby Jul 5, 2024
8d46d6c
always seek on click
OliBomby Jul 5, 2024
c05f489
fix naming violation
OliBomby Jul 5, 2024
9013c11
update tests
OliBomby Jul 5, 2024
ba44757
clarify logic
OliBomby Jul 5, 2024
5da8bb5
prevent volume control from eating inputs
OliBomby Jul 7, 2024
f36321a
allow alt scroll for volume in editor
OliBomby Jul 7, 2024
306dc37
Make hit object and sample point seek keybinds configurable
OliBomby Jul 9, 2024
ae6d855
Merge remote-tracking branch 'upstream/master' into doubleclick
OliBomby Aug 20, 2024
1ce9e97
add arrow indicator
OliBomby Aug 20, 2024
423fead
Revert "add arrow indicator"
OliBomby Aug 21, 2024
0db068e
allow repeating on seek actions
OliBomby Aug 22, 2024
adbdb39
move public member to top of file
OliBomby Aug 22, 2024
71044a0
fix difference in sample time calculation
OliBomby Aug 27, 2024
daad476
Add float comparison leniency just in case
OliBomby Aug 27, 2024
1117fd5
change default seek hotkeys
OliBomby Aug 27, 2024
b5b4f91
Automatic seek to sample point on right-click
OliBomby Aug 27, 2024
cadbb0f
change sample seek keybind to ctrl shift
OliBomby Aug 28, 2024
8fe7ab1
dont seek on right-click, only on keyboard request
OliBomby Aug 29, 2024
ba0c4df
Merge remote-tracking branch 'upstream/master' into doubleclick
OliBomby Aug 29, 2024
3a1afda
fix test
OliBomby Aug 29, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
using NUnit.Framework;
using osu.Framework.Input;
using osu.Framework.Testing;
using osu.Framework.Utils;
using osu.Game.Audio;
using osu.Game.Beatmaps;
using osu.Game.Graphics.UserInterface;
Expand Down Expand Up @@ -307,6 +308,55 @@ public void TestNodeSamplePopover()
hitObjectNodeHasSampleVolume(0, 1, 10);
}

[Test]
public void TestSamplePointSeek()
{
AddStep("add slider", () =>
{
EditorBeatmap.Clear();
EditorBeatmap.Add(new Slider
{
Position = new Vector2(256, 256),
StartTime = 0,
Path = new SliderPath(new[] { new PathControlPoint(Vector2.Zero), new PathControlPoint(new Vector2(250, 0)) }),
Samples =
{
new HitSampleInfo(HitSampleInfo.HIT_NORMAL)
},
NodeSamples =
{
new List<HitSampleInfo> { new HitSampleInfo(HitSampleInfo.HIT_NORMAL) },
new List<HitSampleInfo> { new HitSampleInfo(HitSampleInfo.HIT_NORMAL) },
},
RepeatCount = 1
});
});

clickNodeSamplePiece(0, 0);
editorTimeIs(0);
clickNodeSamplePiece(0, 1);
editorTimeIs(813);
clickNodeSamplePiece(0, 2);
editorTimeIs(1627);
clickSamplePiece(0);
editorTimeIs(406);

seekSamplePiece(-1);
editorTimeIs(0);
samplePopoverIsOpen();
seekSamplePiece(-1);
editorTimeIs(0);
samplePopoverIsOpen();
seekSamplePiece(1);
editorTimeIs(406);
seekSamplePiece(1);
editorTimeIs(813);
seekSamplePiece(1);
editorTimeIs(1627);
seekSamplePiece(1);
editorTimeIs(1627);
}

[Test]
public void TestHotkeysMultipleSelectionWithSameSampleBank()
{
Expand Down Expand Up @@ -486,7 +536,7 @@ public void TestHotkeysAffectNodeSamples()

private void clickSamplePiece(int objectIndex) => AddStep($"click {objectIndex.ToOrdinalWords()} sample piece", () =>
{
var samplePiece = this.ChildrenOfType<SamplePointPiece>().Single(piece => piece.HitObject == EditorBeatmap.HitObjects.ElementAt(objectIndex));
var samplePiece = this.ChildrenOfType<SamplePointPiece>().Single(piece => piece is not NodeSamplePointPiece && piece.HitObject == EditorBeatmap.HitObjects.ElementAt(objectIndex));

InputManager.MoveMouseTo(samplePiece);
InputManager.Click(MouseButton.Left);
Expand All @@ -500,6 +550,21 @@ private void clickNodeSamplePiece(int objectIndex, int nodeIndex) => AddStep($"c
InputManager.Click(MouseButton.Left);
});

private void seekSamplePiece(int direction) => AddStep($"seek sample piece {direction}", () =>
{
InputManager.PressKey(Key.ShiftLeft);
InputManager.PressKey(Key.AltLeft);
InputManager.Key(direction < 1 ? Key.Left : Key.Right);
InputManager.ReleaseKey(Key.AltLeft);
InputManager.ReleaseKey(Key.ShiftLeft);
});

private void samplePopoverIsOpen() => AddUntilStep("sample popover is open", () =>
{
var popover = this.ChildrenOfType<SamplePointPiece.SampleEditPopover>().SingleOrDefault(o => o.IsPresent);
return popover != null;
});

private void samplePopoverHasNoFocus() => AddUntilStep("sample popover textbox not focused", () =>
{
var popover = this.ChildrenOfType<SamplePointPiece.SampleEditPopover>().SingleOrDefault();
Expand Down Expand Up @@ -644,5 +709,7 @@ private void hitObjectNodeHasSampleAdditionBank(int objectIndex, int nodeIndex,
var h = EditorBeatmap.HitObjects.ElementAt(objectIndex) as IHasRepeats;
return h is not null && h.NodeSamples[nodeIndex].Where(o => o.Name != HitSampleInfo.HIT_NORMAL).All(o => o.Bank == bank);
});

private void editorTimeIs(double time) => AddAssert($"editor time is {time}", () => Precision.AlmostEquals(EditorClock.CurrentTimeAccurate, time, 1));
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,12 @@ public NodeSamplePointPiece(HitObject hitObject, int nodeIndex)
NodeIndex = nodeIndex;
}

protected override double GetTime()
{
var hasRepeats = (IHasRepeats)HitObject;
return HitObject.StartTime + hasRepeats.Duration * NodeIndex / hasRepeats.SpanCount();
}

protected override IList<HitSampleInfo> GetSamples()
{
var hasRepeats = (IHasRepeats)HitObject;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@
using osu.Game.Rulesets.Objects;
using osu.Game.Screens.Edit.Components.TernaryButtons;
using osu.Game.Rulesets.Objects.Drawables;
using osu.Game.Rulesets.Objects.Types;
using osu.Game.Screens.Edit.Timing;
using osuTK;
using osuTK.Graphics;
Expand All @@ -32,6 +33,12 @@ public partial class SamplePointPiece : HitObjectPointPiece, IHasPopover
{
public readonly HitObject HitObject;

[Resolved]
private EditorClock? editorClock { get; set; }

[Resolved]
private Editor? editor { get; set; }

public SamplePointPiece(HitObject hitObject)
{
HitObject = hitObject;
Expand All @@ -41,15 +48,35 @@ public SamplePointPiece(HitObject hitObject)

protected override Color4 GetRepresentingColour(OsuColour colours) => AlternativeColor ? colours.Pink2 : colours.Pink1;

protected virtual double GetTime() => HitObject is IHasRepeats r ? HitObject.StartTime + r.Duration / r.SpanCount() / 2 : HitObject.StartTime;
peppy marked this conversation as resolved.
Show resolved Hide resolved

[BackgroundDependencyLoader]
private void load()
{
HitObject.DefaultsApplied += _ => updateText();
updateText();

if (editor != null)
editor.ShowSampleEditPopoverRequested += onShowSampleEditPopoverRequested;
}

protected override void Dispose(bool isDisposing)
{
base.Dispose(isDisposing);

if (editor != null)
editor.ShowSampleEditPopoverRequested -= onShowSampleEditPopoverRequested;
}

private void onShowSampleEditPopoverRequested(double time)
{
if (time == GetTime())
this.ShowPopover();
}

protected override bool OnClick(ClickEvent e)
{
editorClock?.SeekSmoothlyTo(GetTime());
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@OliBomby OliBomby Aug 27, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

@peppy peppy Aug 29, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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()

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

this.ShowPopover();
return true;
}
Expand Down
77 changes: 75 additions & 2 deletions osu.Game/Screens/Edit/Editor.cs
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,7 @@
using osu.Game.Rulesets;
using osu.Game.Rulesets.Edit;
using osu.Game.Rulesets.Objects;
using osu.Game.Rulesets.Objects.Types;
using osu.Game.Screens.Edit.Components.Menus;
using osu.Game.Screens.Edit.Compose;
using osu.Game.Screens.Edit.Compose.Components.Timeline;
Expand Down Expand Up @@ -593,7 +594,7 @@ public void OnReleased(KeyBindingReleaseEvent<PlatformAction> e)

protected override bool OnKeyDown(KeyDownEvent e)
{
if (e.ControlPressed || e.AltPressed || e.SuperPressed) return false;
if (e.ControlPressed || e.SuperPressed) return false;

switch (e.Key)
{
Expand Down Expand Up @@ -674,7 +675,7 @@ protected override bool OnKeyDown(KeyDownEvent e)

protected override bool OnScroll(ScrollEvent e)
{
if (e.ControlPressed || e.AltPressed || e.SuperPressed)
if (e.ControlPressed || e.SuperPressed)
return false;

const double precision = 1;
Expand Down Expand Up @@ -1064,8 +1065,80 @@ private void seekControlPoint(int direction)
clock.Seek(found.Time);
}

private void seekHitObject(int direction)
{
var found = direction < 1
? editorBeatmap.HitObjects.LastOrDefault(p => p.StartTime < clock.CurrentTimeAccurate)
: editorBeatmap.HitObjects.FirstOrDefault(p => p.StartTime > clock.CurrentTimeAccurate);

if (found != null)
clock.SeekSmoothlyTo(found.StartTime);
}

[CanBeNull]
public event Action<double> ShowSampleEditPopoverRequested;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Move public things to top of file and document though.


private void seekSamplePoint(int direction)
{
double currentTime = clock.CurrentTimeAccurate;

// Check if we are currently inside a hit object with node samples, if so seek to the next node sample point
var current = direction < 1
? editorBeatmap.HitObjects.LastOrDefault(p => p is IHasRepeats r && p.StartTime < currentTime && r.EndTime >= currentTime)
: editorBeatmap.HitObjects.LastOrDefault(p => p is IHasRepeats r && p.StartTime <= currentTime && r.EndTime > currentTime);

if (current != null)
{
// Find the next node sample point
var r = (IHasRepeats)current;
double[] nodeSamplePointTimes = new double[r.RepeatCount + 3];

nodeSamplePointTimes[0] = current.StartTime;
// The sample point for the main samples is sandwiched between the head and the first repeat
nodeSamplePointTimes[1] = current.StartTime + r.Duration / r.SpanCount() / 2;

for (int i = 0; i < r.SpanCount(); i++)
{
nodeSamplePointTimes[i + 2] = current.StartTime + r.Duration / r.SpanCount() * (i + 1);
}

double found = direction < 1
? nodeSamplePointTimes.Last(p => p < currentTime)
: nodeSamplePointTimes.First(p => p > currentTime);

clock.SeekSmoothlyTo(found);
}
else
{
if (direction < 1)
{
current = editorBeatmap.HitObjects.LastOrDefault(p => p.StartTime < currentTime);
if (current != null)
clock.SeekSmoothlyTo(current is IHasRepeats r ? r.EndTime : current.StartTime);
}
else
{
current = editorBeatmap.HitObjects.FirstOrDefault(p => p.StartTime > currentTime);
if (current != null)
clock.SeekSmoothlyTo(current.StartTime);
}
}

// Show the sample edit popover at the current time
ShowSampleEditPopoverRequested?.Invoke(clock.CurrentTimeAccurate);
}

private void seek(UIEvent e, int direction)
{
if (e.AltPressed)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

{
if (e.ShiftPressed)
seekSamplePoint(direction);
else
seekHitObject(direction);
return;
}

double amount = e.ShiftPressed ? 4 : 1;

bool trackPlaying = clock.IsRunning;
Expand Down
Loading