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

Allow adjusting volume controls via a drag #26564

Merged
merged 5 commits into from Jan 17, 2024

Conversation

peppy
Copy link
Sponsor Member

@peppy peppy commented Jan 16, 2024

Just a quick QoL one I've also been after for a while.

Addresses #26016.

@frenzibyte
Copy link
Member

UX-wise I find the logic not being relative to the size of the volume meter feeling sort-of weird, especially when adjusting the small meters:

CleanShot.2024-01-16.at.14.57.19.mp4

I've adjusted the code around to make it relative, and here's how it behaves now:

CleanShot.2024-01-16.at.14.51.58.mp4
diff
diff --git a/osu.Game.Tests/Visual/UserInterface/TestSceneVolumePieces.cs b/osu.Game.Tests/Visual/UserInterface/TestSceneVolumePieces.cs
index 311bae0d50..b85e4c19d1 100644
--- a/osu.Game.Tests/Visual/UserInterface/TestSceneVolumePieces.cs
+++ b/osu.Game.Tests/Visual/UserInterface/TestSceneVolumePieces.cs
@@ -14,7 +14,7 @@ protected override void LoadComplete()
         {
             VolumeMeter meter;
             MuteButton mute;
-            Add(meter = new VolumeMeter("MASTER", 125, Color4.Blue) { Position = new Vector2(10) });
+            Add(meter = new VolumeMeter("MASTER", 125, Color4.Green) { Position = new Vector2(10) });
             AddSliderStep("master volume", 0, 10, 0, i => meter.Bindable.Value = i * 0.1);
 
             Add(new VolumeMeter("BIG", 250, Color4.Red)
@@ -22,6 +22,15 @@ protected override void LoadComplete()
                 Anchor = Anchor.Centre,
                 Origin = Anchor.Centre,
                 Position = new Vector2(10),
+                Margin = new MarginPadding { Left = 250 },
+            });
+
+            Add(new VolumeMeter("SML", 125, Color4.Blue)
+            {
+                Anchor = Anchor.Centre,
+                Origin = Anchor.Centre,
+                Position = new Vector2(10),
+                Margin = new MarginPadding { Right = 500 },
             });
 
             Add(mute = new MuteButton
diff --git a/osu.Game/Overlays/Volume/VolumeMeter.cs b/osu.Game/Overlays/Volume/VolumeMeter.cs
index 6ad314c601..8962ef86c8 100644
--- a/osu.Game/Overlays/Volume/VolumeMeter.cs
+++ b/osu.Game/Overlays/Volume/VolumeMeter.cs
@@ -309,7 +309,7 @@ public double Volume
         private const double max_acceleration = 5;
         private const double acceleration_multiplier = 1.8;
 
-        private const float mouse_drag_divisor = 20;
+        private const float mouse_drag_multiplier = 15;
 
         private ScheduledDelegate accelerationDebounce;
 
@@ -317,13 +317,13 @@ public double Volume
 
         protected override bool OnDragStart(DragStartEvent e)
         {
-            adjust(-e.Delta.Y / mouse_drag_divisor, true);
+            adjust(-(e.Delta.Y / DrawSize.Y) * mouse_drag_multiplier, true);
             return true;
         }
 
         protected override void OnDrag(DragEvent e)
         {
-            adjust(-e.Delta.Y / mouse_drag_divisor, true);
+            adjust(-(e.Delta.Y / DrawSize.Y) * mouse_drag_multiplier, true);
             base.OnDrag(e);
         }
 

Does this look better to you? or do you feel the original behaviour making more sense instead.

Copy link
Member

@frenzibyte frenzibyte left a comment

Choose a reason for hiding this comment

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

^

@peppy
Copy link
Sponsor Member Author

peppy commented Jan 16, 2024

Hard to say because I'm testing with a keyboard right now. Will wait for another opinion on what feels most natural. Fine with either.

@bdach
Copy link
Collaborator

bdach commented Jan 16, 2024

Don't agree with relative. Seems arbitrary and like change for the sake of change. It's not like scrolling via mouse wheel scrolls more on the small meters is it? Also why should it be more difficult to do precise adjustments on the small meters via this flow?

@frenzibyte
Copy link
Member

frenzibyte commented Jan 16, 2024

At very least, am I the only one that finds the current divisor too high? It takes over half the screen's height to adjust from zero to maximum:

CleanShot.2024-01-16.at.16.00.44.mp4

Making it relative would at least make it easier to reach maximum for the small wheels, but if everyone thinks it's normal, I'm fine with moving on.

@bdach
Copy link
Collaborator

bdach commented Jan 16, 2024

I'd say the input handling should be the same for all metres. Not necessarily opposed to tweaking the divisor some though.

@peppy
Copy link
Sponsor Member Author

peppy commented Jan 16, 2024

Hmm, now that I look at the code, I believe the reason it feels so off to you is that it may be frame rate dependent. The underlying function accelerates based on number of times it's called, so for higher sampling rates it may accelerate faster.

@peppy
Copy link
Sponsor Member Author

peppy commented Jan 17, 2024

@frenzibyte please give things another try.

@pull-request-size pull-request-size bot added size/M and removed size/S labels Jan 17, 2024
@frenzibyte frenzibyte merged commit 0adfe5f into ppy:master Jan 17, 2024
10 of 11 checks passed
@peppy peppy deleted the drag-volume-controls branch January 18, 2024 11:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants