Add ability to change slider length by dragging slider tail

[OliBomby]

This PR adds the ability to drag the slider tail with shift+click to change the expected distance of the slider path.
This behaviour mimics the ability to change slider duration in the timeline by shift+dragging the slider tail.
I added a little bit of hover brightness on the yellow circle (similar to path control point) to indicate it is draggable.

Benefits:

• Easier to set the slider tail exactly where you want without having to fiddle with slider velocity.
• Easier to create smaller sliders which wouldn't be long enough at the current SV.
• Easier to restore your drawn slider shape after the slider tail position moved due to snapping.
• The slider selection blueprint now always has a yellow circle on the side opposite to the slider start.

chrome_KpYVVrtQQV.mp4

[bdach]

Just on a very quick check I'm not super convinced by some of the interactions that this feature produces.

2024-05-24.14-24-24.mp4

• I'm not sure how to feel about the fact that path segments that essentially do nothing anymore (because the path is too short for them to matter in any capacity) still remain there. If you ask me they should be getting trimmed at some point, once the path is too short for them to matter, if only to reduce various overheads everywhere (parsing, path approximation, etc.)
• After this shift-drag operation, deleting any anchor causes the expected distance to seemingly revert to what it used to be at the very start, but the slider velocity continues to be the same as after the shift-drag, which makes the slider duration balloon to probably like five minutes.
• UX nit but the end circle should only get brighter once shift is held, because it feels misleading that it gets brighter when you hover it even without shift but dragging it does nothing unless you have shift held.

Not sure. I'm gonna need a @peppy opinion that we want this in the editor to begin with before spending more review time on this. And possibly fixes to the bad interactions described above.

[OliBomby]

• I'm not sure how to feel about the fact that path segments that essentially do nothing anymore (because the path is too short for them to matter in any capacity) still remain there. If you ask me they should be getting trimmed at some point, once the path is too short for them to matter, if only to reduce various overheads everywhere (parsing, path approximation, etc.)

Trimming unnecessary path segments could be part of some future map cleaning tool. I agree they should be trimmed at some point, probably before map upload/ready for rank. My proposed feature is also not the only way to create those trailing path segments, as beat snapping can do the same.

• After this shift-drag operation, deleting any anchor causes the expected distance to seemingly revert to what it used to be at the very start, but the slider velocity continues to be the same as after the shift-drag, which makes the slider duration balloon to probably like five minutes.

This interaction essentially sets the SV of the original slider to 0.10x which has the same duration balloon effect you describe. Its not hard to fix manually, just set the SV back to a reasonable value, and I doubt it will occur often in practice because you wouldn't need to shorten sliders that extremely in most cases.

I gave it some thought but I can't come up with a simple clean solution for this interaction.

One possible solution would be to trigger some kind of mode-switch in the slider once you manually set the pixel length, which prevents changes to the slider path from resnapping the slider duration. It seems kind of unintuitive though, because mappers expect sliders to resnap when they change the shape.

Another solution would be to automatically trim the slider path control points when you shorten the slider like this. This would work for sliders with multiple segments like in your example. However that would make the operation kind of destructive, and it prevents you from shortening a slider and then extending it again.

• UX nit but the end circle should only get brighter once shift is held, because it feels misleading that it gets brighter when you hover it even without shift but dragging it does nothing unless you have shift held.

Sure, I agree.

[peppy]

I think the excess points definitely need to be trimmed after the operation (on mouse up) for this to feel correct. Either that, or shortening should only be allowed for the last segment?

[bdach]

Should be annotated as [CanBeNull], because contrary to {Head,Tail}Circle, this can feasibly be null on any given slider (if it has no repeats).

And yes I'm saying to use jetbrains attributes for now because changing this to not use jetbrains attributes likely bloats this diff with 50 files of unrelated NRT changes.

[bdach]

My question at this point would be - why does only the last repeat get this treatment? Surely all of them should?

[OliBomby]

Because we only need to expose the last repeat with an updated position. This is so the SliderCircleOverlay can have a hit object to update from which is placed opposite from the sliderhead position. This is either the TailCircle or the LastRepeat based on whether the repeat count is even.

We have nothing that requires the positions of all of the repeats to be immediately updated, so I don't see a reason to do so.

[bdach]

What is all of this shiftPressed / lastShiftPressed tracking for? It doesn't really look like it's doing much useful to me.

[OliBomby]

It is to make sure updateCirclePieceColour only triggers on changes in the shift key state. I'll simplify the logic.

[bdach]

The UX is still lacking. The case I'm attempting to solve is holdng shift and accidentally dragging the last slider anchor rather than the entirety of the tail circle outside of the slider anchor (only applies when the slider anchor is inside the tail circle I guess). This causes the path to change in a different way than expected (i.e. rather than adjusting length, it begins to snap to the grid):

2024-06-03.11-34-26.mp4

I wrote this as an attempt to fix it:

diff --git a/osu.Game.Rulesets.Osu/Edit/Blueprints/Sliders/Components/SliderTailPiece.cs b/osu.Game.Rulesets.Osu/Edit/Blueprints/Sliders/Components/SliderTailPiece.cs
index a0f4401ca7..f0ec8bb129 100644
--- a/osu.Game.Rulesets.Osu/Edit/Blueprints/Sliders/Components/SliderTailPiece.cs
+++ b/osu.Game.Rulesets.Osu/Edit/Blueprints/Sliders/Components/SliderTailPiece.cs
@@ -50,10 +50,12 @@ protected override void LoadComplete()
 
         public override bool ReceivePositionalInputAt(Vector2 screenSpacePos) => CirclePiece.ReceivePositionalInputAt(screenSpacePos);
 
+        protected override bool OnMouseDown(MouseDownEvent e) => e.ShiftPressed;
+
         protected override bool OnHover(HoverEvent e)
         {
             updateCirclePieceColour();
-            return false;
+            return e.ShiftPressed;
         }
 
         protected override void OnHoverLost(HoverLostEvent e)
diff --git a/osu.Game.Rulesets.Osu/Edit/Blueprints/Sliders/SliderSelectionBlueprint.cs b/osu.Game.Rulesets.Osu/Edit/Blueprints/Sliders/SliderSelectionBlueprint.cs
index 2c239a40c8..6f0aaec2b1 100644
--- a/osu.Game.Rulesets.Osu/Edit/Blueprints/Sliders/SliderSelectionBlueprint.cs
+++ b/osu.Game.Rulesets.Osu/Edit/Blueprints/Sliders/SliderSelectionBlueprint.cs
@@ -9,6 +9,7 @@
 using osu.Framework.Allocation;
 using osu.Framework.Bindables;
 using osu.Framework.Graphics;
+using osu.Framework.Graphics.Containers;
 using osu.Framework.Graphics.Primitives;
 using osu.Framework.Graphics.UserInterface;
 using osu.Framework.Input.Events;
@@ -72,6 +73,10 @@ private void load()
             {
                 BodyPiece = new SliderBodyPiece(),
                 HeadOverlay = CreateCircleOverlay(HitObject, SliderPosition.Start),
+                pathVisualiserContainer = new Container
+                {
+                    RelativeSizeAxes = Axes.Both,
+                },
                 TailPiece = CreateTailPiece(HitObject, SliderPosition.End),
             };
         }
@@ -146,7 +151,7 @@ private void updateVisualDefinition()
             {
                 if (ControlPointVisualiser == null)
                 {
-                    AddInternal(ControlPointVisualiser = new PathControlPointVisualiser<Slider>(HitObject, true)
+                    pathVisualiserContainer.Add(ControlPointVisualiser = new PathControlPointVisualiser<Slider>(HitObject, true)
                     {
                         RemoveControlPointsRequested = removeControlPoints,
                         SplitControlPointsRequested = splitControlPoints
@@ -189,6 +194,8 @@ protected override bool OnMouseDown(MouseDownEvent e)
         [CanBeNull]
         private PathControlPoint placementControlPoint;
 
+        private Container pathVisualiserContainer;
+
         protected override bool OnDragStart(DragStartEvent e)
         {
             if (placementControlPoint == null)

Applying the patch above makes the behaviours exclusive (e.g. with shift held, you can't move the last slider anchor at all, as input will be handled by the end circle exclusively), but I'm not sure whether that is an acceptable trade-off or not. It kinda makes it not possible to momentary grid-snap the last anchor (for that you would have to start dragging the last anchor and only then start holding shift).

[OliBomby]

The UX is still lacking. The case I'm attempting to solve is holdng shift and accidentally dragging the last slider anchor rather than the entirety of the tail circle outside of the slider anchor (only applies when the slider anchor is inside the tail circle I guess). This causes the path to change in a different way than expected (i.e. rather than adjusting length, it begins to snap to the grid):

Applying the patch above makes the behaviours exclusive (e.g. with shift held, you can't move the last slider anchor at all, as input will be handled by the end circle exclusively), but I'm not sure whether that is an acceptable trade-off or not. It kinda makes it not possible to momentary grid-snap the last anchor (for that you would have to start dragging the last anchor and only then start holding shift).

While it is a valid solution to the initial UX problem, I don't think it is an acceptable trade-off. Holding shift first and then dragging the anchor seems like the more natural way to momentarily grid-snap the anchor. The last anchor is also layered ontop of the tail piece so its expected to handle input with priority.
Also according to you, changing behaviour based on the timing of the shift press is not acceptable UX (#27035 (comment))

I think its fine to keep the UX as is because the user can just click around the last anchor to reach the tail piece. The area to click the tail piece is always bigger than the anchor, on all circle size values.

To help with this we could prevent the tail piece from becoming brighter if the user is hovering an anchor. That way the user has the correct feedback to indicate whether they can drag the slider tail. I'd need some help implementing this because I don't know how the tail piece can check if the user is currently hovering an anchor.

[peppy]

I think we may need a separate anchor/icon visible for this. I'm not comfortable with such hidden behaviours anymore (have to know to click in basically-nowhere and also hold shift).

[OliBomby]

I think we may need a separate anchor/icon visible for this. I'm not comfortable with such hidden behaviours anymore (have to know to click in basically-nowhere and also hold shift).

I don't want another extra button added, the buttons we have right now are annoying enough as is.
Imo this is more of a niche feature so I don't think it needs to be as prominent as let's say the reverse selection button.

It's also not clicking in 'basically-nowhere' because you're clicking on exactly the thing you want to move, which is the slider end. If there was some separate anchor/icon, then that would be detached from the slider end.

[bdach]

For whatever it's worth I also don't think the current proposed UX is acceptable and would not really be looking to try and get this in until that is somehow improved.

[OliBomby]

I changed the UX. Now the length adjustment is initiated through the context menu. The slider end will then move to your cursor until you click again.

I discussed with @minetoblend and went through a few iterations to reach this design and I think it's an improvement.

osu.Game.Rulesets.Osu.Tests_PmxvA4ja8E.mp4

[bdach]

I don't think that works. It's very jank, the jarring jumpy movement of everything after triggering the flow just feels quite bad IMO. (Maybe if the cursor warped to the slider circle it'd be better, but not sure how easy that is to achieve.) And mouse down ending the movement is also unlike anything we've done elsewhere.

If I were to suggest anything constructive it would be something like the following.

2024-06-20.09-19-44.mp4

diff (very beelined as a poc, likely needs cleanup)

diff --git a/osu.Game.Rulesets.Osu/Edit/Blueprints/Sliders/SliderCircleOverlay.cs b/osu.Game.Rulesets.Osu/Edit/Blueprints/Sliders/SliderCircleOverlay.cs
index 55ea131dab..9b645d4f62 100644
--- a/osu.Game.Rulesets.Osu/Edit/Blueprints/Sliders/SliderCircleOverlay.cs
+++ b/osu.Game.Rulesets.Osu/Edit/Blueprints/Sliders/SliderCircleOverlay.cs
@@ -1,20 +1,46 @@
 // Copyright (c) ppy Pty Ltd <[email protected]>. Licensed under the MIT Licence.
 // See the LICENCE file in the repository root for full licence text.
 
+using System;
+using osu.Framework.Allocation;
 using osu.Framework.Graphics;
 using osu.Framework.Graphics.Containers;
+using osu.Framework.Graphics.Lines;
+using osu.Framework.Graphics.Primitives;
+using osu.Framework.Input.Events;
+using osu.Framework.Utils;
+using osu.Game.Graphics;
 using osu.Game.Rulesets.Osu.Edit.Blueprints.HitCircles.Components;
 using osu.Game.Rulesets.Osu.Objects;
+using osuTK;
 
 namespace osu.Game.Rulesets.Osu.Edit.Blueprints.Sliders
 {
     public partial class SliderCircleOverlay : CompositeDrawable
     {
-        protected readonly HitCirclePiece CirclePiece;
+        public RectangleF VisibleQuad
+        {
+            get
+            {
+                var result = CirclePiece.ScreenSpaceDrawQuad.AABBFloat;
+
+                if (endDragMarkerContainer != null)
+                {
+                    var size = result.Size * 1.4f;
+                    var location = result.TopLeft - result.Size * 0.2f;
+                    return new RectangleF(location, size);
+                }
+
+                return result;
+            }
+        }
+
+        protected HitCirclePiece CirclePiece = null!;
         protected readonly Slider Slider;
 
-        private readonly HitCircleOverlapMarker marker;
+        private HitCircleOverlapMarker marker = null!;
         private readonly SliderPosition position;
+        private Container? endDragMarkerContainer;
 
         public SliderCircleOverlay(Slider slider, SliderPosition position)
         {
@@ -26,8 +52,22 @@ public SliderCircleOverlay(Slider slider, SliderPosition position)
                 marker = new HitCircleOverlapMarker(),
                 CirclePiece = new HitCirclePiece(),
             };
+
+            if (position == SliderPosition.End)
+            {
+                AddInternal(endDragMarkerContainer = new Container
+                {
+                    AutoSizeAxes = Axes.Both,
+                    Anchor = Anchor.CentreLeft,
+                    Origin = Anchor.CentreLeft,
+                    Padding = new MarginPadding(-2.5f),
+                    Child = EndDragMarker = new SliderEndDragMarker(Slider)
+                });
+            }
         }
 
+        public SliderEndDragMarker? EndDragMarker { get; }
+
         protected override void Update()
         {
             base.Update();
@@ -37,16 +77,103 @@ protected override void Update()
 
             CirclePiece.UpdateFrom(circle);
             marker.UpdateFrom(circle);
+
+            if (endDragMarkerContainer != null)
+            {
+                endDragMarkerContainer.Position = circle.Position;
+                endDragMarkerContainer.Scale = CirclePiece.Scale * 1.2f;
+                var diff = Slider.Path.PositionAt(1) - Slider.Path.PositionAt(0.99f);
+                endDragMarkerContainer.Rotation = float.RadiansToDegrees(MathF.Atan2(diff.Y, diff.X));
+            }
         }
 
         public override void Hide()
         {
             CirclePiece.Hide();
+            endDragMarkerContainer?.Hide();
         }
 
         public override void Show()
         {
             CirclePiece.Show();
+            endDragMarkerContainer?.Show();
+        }
+
+        public partial class SliderEndDragMarker : SmoothPath
+        {
+            private readonly Slider slider;
+
+            public Action? StartDrag { get; set; }
+            public Action<DragEvent>? Drag { get; set; }
+            public Action? EndDrag { get; set; }
+
+            public SliderEndDragMarker(Slider slider)
+            {
+                this.slider = slider;
+            }
+
+            [Resolved]
+            private OsuColour colours { get; set; } = null!;
+
+            [BackgroundDependencyLoader]
+            private void load()
+            {
+                var path = PathApproximator.CircularArcToPiecewiseLinear([
+                    new Vector2(0, OsuHitObject.OBJECT_RADIUS),
+                    new Vector2(OsuHitObject.OBJECT_RADIUS, 0),
+                    new Vector2(0, -OsuHitObject.OBJECT_RADIUS)
+                ]);
+
+                Anchor = Anchor.CentreLeft;
+                Origin = Anchor.CentreLeft;
+                PathRadius = 5;
+                Vertices = path;
+            }
+
+            protected override void LoadComplete()
+            {
+                base.LoadComplete();
+
+                updateState();
+            }
+
+            protected override bool OnHover(HoverEvent e)
+            {
+                updateState();
+                return true;
+            }
+
+            protected override void OnHoverLost(HoverLostEvent e)
+            {
+                updateState();
+                base.OnHoverLost(e);
+            }
+
+            protected override bool OnDragStart(DragStartEvent e)
+            {
+                updateState();
+                StartDrag?.Invoke();
+                return true;
+            }
+
+            protected override void OnDrag(DragEvent e)
+            {
+                updateState();
+                base.OnDrag(e);
+                Drag?.Invoke(e);
+            }
+
+            protected override void OnDragEnd(DragEndEvent e)
+            {
+                updateState();
+                EndDrag?.Invoke();
+                base.OnDragEnd(e);
+            }
+
+            private void updateState()
+            {
+                Colour = IsHovered || IsDragged ? colours.Red : colours.Yellow;
+            }
         }
     }
 }
diff --git a/osu.Game.Rulesets.Osu/Edit/Blueprints/Sliders/SliderSelectionBlueprint.cs b/osu.Game.Rulesets.Osu/Edit/Blueprints/Sliders/SliderSelectionBlueprint.cs
index eb269ba680..cbbd3197b7 100644
--- a/osu.Game.Rulesets.Osu/Edit/Blueprints/Sliders/SliderSelectionBlueprint.cs
+++ b/osu.Game.Rulesets.Osu/Edit/Blueprints/Sliders/SliderSelectionBlueprint.cs
@@ -55,7 +55,18 @@ public partial class SliderSelectionBlueprint : OsuSelectionBlueprint<Slider>
         [Resolved(CanBeNull = true)]
         private BindableBeatDivisor beatDivisor { get; set; }
 
-        public override Quad SelectionQuad => BodyPiece.ScreenSpaceDrawQuad;
+        public override Quad SelectionQuad
+        {
+            get
+            {
+                var result = BodyPiece.ScreenSpaceDrawQuad.AABBFloat;
+
+                result = RectangleF.Union(result, HeadOverlay.VisibleQuad);
+                result = RectangleF.Union(result, TailOverlay.VisibleQuad);
+
+                return result;
+            }
+        }
 
         private readonly BindableList<PathControlPoint> controlPoints = new BindableList<PathControlPoint>();
         private readonly IBindable<int> pathVersion = new Bindable<int>();
@@ -173,12 +184,6 @@ private void updateVisualDefinition()
 
         protected override bool OnMouseDown(MouseDownEvent e)
         {
-            if (isAdjustingLength)
-            {
-                endAdjustLength();
-                return true;
-            }
-
             switch (e.Button)
             {
                 case MouseButton.Right:
@@ -209,11 +214,8 @@ private void endAdjustLength()
             changeHandler?.EndChange();
         }
 
-        protected override bool OnMouseMove(MouseMoveEvent e)
+        private bool adjustLength(MouseEvent e)
         {
-            if (!isAdjustingLength)
-                return base.OnMouseMove(e);
-
             double oldDistance = HitObject.Path.Distance;
             double proposedDistance = findClosestPathDistance(e);
 
@@ -262,12 +264,12 @@ private void trimExcessControlPoints(SliderPath sliderPath)
         /// <summary>
         /// Finds the expected distance value for which the slider end is closest to the mouse position.
         /// </summary>
-        private double findClosestPathDistance(MouseMoveEvent e)
+        private double findClosestPathDistance(MouseEvent e)
         {
             const double step1 = 10;
             const double step2 = 0.1;
 
-            var desiredPosition = e.MousePosition - HitObject.Position;
+            var desiredPosition = ToLocalSpace(e.ScreenSpaceMousePosition) - HitObject.Position;
 
             if (!fullPathCache.IsValid)
                 fullPathCache.Value = new SliderPath(HitObject.Path.ControlPoints.ToArray());
@@ -525,14 +527,16 @@ private void convertToStream()
                 addControlPoint(rightClickPosition);
                 changeHandler?.EndChange();
             }),
-            new OsuMenuItem("Adjust length", MenuItemType.Standard, () =>
-            {
-                isAdjustingLength = true;
-                changeHandler?.BeginChange();
-            }),
+            new OsuMenuItem("Adjust length", MenuItemType.Standard, startAdjustingLength),
             new OsuMenuItem("Convert to stream", MenuItemType.Destructive, convertToStream),
         };
 
+        private void startAdjustingLength()
+        {
+            isAdjustingLength = true;
+            changeHandler?.BeginChange();
+        }
+
         // Always refer to the drawable object's slider body so subsequent movement deltas are calculated with updated positions.
         public override Vector2 ScreenSpaceSelectionPoint => DrawableObject.SliderBody?.ToScreenSpace(DrawableObject.SliderBody.PathOffset)
                                                              ?? BodyPiece.ToScreenSpace(BodyPiece.PathStartLocation);
@@ -562,6 +566,19 @@ public override bool ReceivePositionalInputAt(Vector2 screenSpacePos)
             return false;
         }
 
-        protected virtual SliderCircleOverlay CreateCircleOverlay(Slider slider, SliderPosition position) => new SliderCircleOverlay(slider, position);
+        protected virtual SliderCircleOverlay CreateCircleOverlay(Slider slider, SliderPosition position)
+        {
+            return position == SliderPosition.End
+                ? new SliderCircleOverlay(slider, position)
+                {
+                    EndDragMarker =
+                    {
+                        StartDrag = startAdjustingLength,
+                        Drag = e => adjustLength(e),
+                        EndDrag = endAdjustLength,
+                    }
+                }
+                : new SliderCircleOverlay(slider, position);
+        }
     }
 }

[OliBomby]

If I were to suggest anything constructive it would be something like the following.

I couldn't come up with a good design for a draggable handle, but this is good, I like it. I'll try to get this in.

[peppy]

My initial thoughts on this is that I was expecting it to actually make the slider shorter in duration, not adjust the velocity.

Like doesn't this feel really weird:

osu.Game.Tests.2024-07-18.at.10.34.04.mp4

[OliBomby]

The interaction wouldn't really make sense if it changed duration instead of velocity.
Either:

• A) The slider end becomes unsnapped any time you change the length by dragging. You'd have to follow it up by shift-dragging the slider end in the timeline to resnap it, so you'd end up changing the slider velocity anyways.
• B) The slider end stays snapped but your length changing drag will not be smooth. You'd be unable to drag it to the exact length you want.

[OliBomby]

I just had a good idea:
I could make the drag handle change duration by default and have it change sv if you hold shift. That way you still have the interaction that I want and by default it produces the expected result and it follows convention of timeline interaction.

[bdach]

The idea of supporting both behaviours with shift seems good to me, but I'd probably want normal drag of the tail to snap to the beat. Any other mutation of sliders I am aware of aside from this respects beat snapping, so this one should too.

[bdach]

Seems ok to me

[peppy]

This is a bit hand-wavey but I guess it's okay.