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

Redesign editor timelines to address most frequent user complaints #28788

Merged
merged 15 commits into from
Jul 12, 2024

Conversation

bdach
Copy link
Collaborator

@bdach bdach commented Jul 9, 2024

master this PR
osu! (timing changes on) osu_2024-07-09_09-26-47 osu_2024-07-09_09-29-49
osu! (timing changes off) osu_2024-07-09_09-26-57 osu_2024-07-09_09-30-04
mania (timing changes off) osu_2024-07-09_09-27-25 osu_2024-07-09_09-30-22

The main user complaints in question are:

  • Slider velocity currently covers up the tick behind it #28667
  • Bottom timeline is too large for the value it provides
  • BPM indicator in bottom left is useless
  • Bottom timeline dots are not readable (thin lines preferred)
  • Bottom timeline colours changing around is screwing up user visual parsing (this is mostly fixed, "green lines" still aren't green on scrolling rulesets, but let's treat that as a separate concern since scrolling rulesets have a way to go to make adding scroll speed changes more user-friendly, and also preview point colour is not the same - remains green).

bdach added 3 commits July 9, 2024 09:25
This removes the BPM display, which is commonly cited to have
no functional purpose by users, and reduces the height of the bottom bar
in exchange for more space for the playfield.
@bdach bdach self-assigned this Jul 9, 2024
@peppy peppy self-requested a review July 11, 2024 07:56
@@ -31,7 +31,7 @@ private void load(Editor editor)

RelativeSizeAxes = Axes.X;

Height = 60;
Height = 40;
Copy link
Member

Choose a reason for hiding this comment

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

Hmm, I really liked the BPM display. Also feel like the new height is way too cramped. Can we settle on this?

diff --git a/osu.Game/Screens/Edit/BottomBar.cs b/osu.Game/Screens/Edit/BottomBar.cs
index a11f40c8fd..9f04d80e65 100644
--- a/osu.Game/Screens/Edit/BottomBar.cs
+++ b/osu.Game/Screens/Edit/BottomBar.cs
@@ -31,7 +31,7 @@ private void load(Editor editor)
 
             RelativeSizeAxes = Axes.X;
 
-            Height = 40;
+            Height = 50;
 
             Masking = true;
             EdgeEffect = new EdgeEffectParameters
diff --git a/osu.Game/Screens/Edit/Components/TimeInfoContainer.cs b/osu.Game/Screens/Edit/Components/TimeInfoContainer.cs
index 37facb3b95..25dc9b9552 100644
--- a/osu.Game/Screens/Edit/Components/TimeInfoContainer.cs
+++ b/osu.Game/Screens/Edit/Components/TimeInfoContainer.cs
@@ -17,6 +17,8 @@ namespace osu.Game.Screens.Edit.Components
 {
     public partial class TimeInfoContainer : BottomBarContainer
     {
+        private OsuSpriteText bpm = null!;
+
         [Resolved]
         private EditorBeatmap editorBeatmap { get; set; } = null!;
 
@@ -24,16 +26,41 @@ public partial class TimeInfoContainer : BottomBarContainer
         private EditorClock editorClock { get; set; } = null!;
 
         [BackgroundDependencyLoader]
-        private void load(OverlayColourProvider colourProvider)
+        private void load(OsuColour colours, OverlayColourProvider colourProvider)
         {
             Background.Colour = colourProvider.Background5;
 
             Children = new Drawable[]
             {
-                new TimestampControl(),
+                new TimestampControl
+                {
+                    Position = new Vector2(0, -2),
+                },
+                bpm = new OsuSpriteText
+                {
+                    Colour = colours.Orange1,
+                    Anchor = Anchor.CentreLeft,
+                    Font = OsuFont.Torus.With(size: 18, weight: FontWeight.SemiBold),
+                    Position = new Vector2(2, 6),
+                }
             };
         }
 
+        private double? lastBPM;
+
+        protected override void Update()
+        {
+            base.Update();
+
+            double newBPM = editorBeatmap.ControlPointInfo.TimingPointAt(editorClock.CurrentTime).BPM;
+
+            if (lastBPM != newBPM)
+            {
+                lastBPM = newBPM;
+                bpm.Text = @$"{newBPM:0} BPM";
+            }
+        }
+
         private partial class TimestampControl : OsuClickableContainer
         {
             private Container hoverLayer = null!;
@@ -87,8 +114,9 @@ private void load()
                     },
                     inputTextBox = new OsuTextBox
                     {
+                        Position = new Vector2(-4, 4),
                         Width = 150,
-                        Height = 36,
+                        Height = 32,
                         Alpha = 0,
                         CommitOnFocusLost = true,
                     },

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

As stated in OP, I've had the bpm display described as "useless" to me (on multiple occasions, I wanna say), but I'm not wholly opposed to bringing it back. Although I do find complaints about "cramped" a little funny given that with the bpm display back the left side of the bottom bar is more cramped than I have it here.

I think if you want to proceed with that we'll need to reduce some font sizes. The timestamp uses a humongous text size for no real reason. The BPM text could probably also be smaller too.

Copy link
Member

Choose a reason for hiding this comment

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

Although I do find complaints about "cramped" a little funny given that with the bpm display back the left side of the bottom bar is more cramped than I have it here.

It's more the bar as a whole than that area of the bar.

I'd recommend against reducing font sizes as the current time is one of the most (if not the most) important visual displays in the editor.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Something like this maybe if I were to give an actual counterproposal.

diff --git a/osu.Game/Screens/Edit/BottomBar.cs b/osu.Game/Screens/Edit/BottomBar.cs
index a11f40c8fd..514a06f1c5 100644
--- a/osu.Game/Screens/Edit/BottomBar.cs
+++ b/osu.Game/Screens/Edit/BottomBar.cs
@@ -31,7 +31,7 @@ private void load(Editor editor)
 
             RelativeSizeAxes = Axes.X;
 
-            Height = 40;
+            Height = 50;
 
             Masking = true;
             EdgeEffect = new EdgeEffectParameters
@@ -48,7 +48,7 @@ private void load(Editor editor)
                     RelativeSizeAxes = Axes.Both,
                     ColumnDimensions = new[]
                     {
-                        new Dimension(GridSizeMode.Absolute, 170),
+                        new Dimension(GridSizeMode.Absolute, 150),
                         new Dimension(),
                         new Dimension(GridSizeMode.Absolute, 220),
                         new Dimension(GridSizeMode.Absolute, HitObjectComposer.TOOLBOX_CONTRACTED_SIZE_RIGHT),
diff --git a/osu.Game/Screens/Edit/Components/TimeInfoContainer.cs b/osu.Game/Screens/Edit/Components/TimeInfoContainer.cs
index 37facb3b95..ddbb652821 100644
--- a/osu.Game/Screens/Edit/Components/TimeInfoContainer.cs
+++ b/osu.Game/Screens/Edit/Components/TimeInfoContainer.cs
@@ -17,6 +17,8 @@ namespace osu.Game.Screens.Edit.Components
 {
     public partial class TimeInfoContainer : BottomBarContainer
     {
+        private OsuSpriteText bpm = null!;
+
         [Resolved]
         private EditorBeatmap editorBeatmap { get; set; } = null!;
 
@@ -24,16 +26,38 @@ public partial class TimeInfoContainer : BottomBarContainer
         private EditorClock editorClock { get; set; } = null!;
 
         [BackgroundDependencyLoader]
-        private void load(OverlayColourProvider colourProvider)
+        private void load(OsuColour colours, OverlayColourProvider colourProvider)
         {
             Background.Colour = colourProvider.Background5;
 
             Children = new Drawable[]
             {
                 new TimestampControl(),
+                bpm = new OsuSpriteText
+                {
+                    Colour = colours.Orange1,
+                    Anchor = Anchor.CentreLeft,
+                    Font = OsuFont.Torus.With(size: 14, weight: FontWeight.SemiBold),
+                    Position = new Vector2(2, 4),
+                }
             };
         }
 
+        private double? lastBPM;
+
+        protected override void Update()
+        {
+            base.Update();
+
+            double newBPM = editorBeatmap.ControlPointInfo.TimingPointAt(editorClock.CurrentTime).BPM;
+
+            if (lastBPM != newBPM)
+            {
+                lastBPM = newBPM;
+                bpm.Text = @$"{newBPM:0} BPM";
+            }
+        }
+
         private partial class TimestampControl : OsuClickableContainer
         {
             private Container hoverLayer = null!;
@@ -83,12 +107,13 @@ private void load()
                         Anchor = Anchor.CentreLeft,
                         Origin = Anchor.CentreLeft,
                         Spacing = new Vector2(-2, 0),
-                        Font = OsuFont.Torus.With(size: 36, fixedWidth: true, weight: FontWeight.Light),
+                        Font = OsuFont.Torus.With(size: 32, fixedWidth: true, weight: FontWeight.Light),
                     },
-                    inputTextBox = new OsuTextBox
+                    inputTextBox = new TimestampTextBox
                     {
-                        Width = 150,
-                        Height = 36,
+                        Position = new Vector2(-4, 4),
+                        Width = 130,
+                        Height = 24,
                         Alpha = 0,
                         CommitOnFocusLost = true,
                     },
@@ -136,6 +161,14 @@ protected override void Update()
                     showingHoverLayer = shouldShowHoverLayer;
                 }
             }
+
+            private partial class TimestampTextBox : OsuTextBox
+            {
+                public TimestampTextBox()
+                {
+                    TextContainer.Height = 0.8f;
+                }
+            }
         }
     }
 }

Copy link
Member

Choose a reason for hiding this comment

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

Seems good, I've applied this and done some further realignment of the hover and textbox.

@peppy
Copy link
Member

peppy commented Jul 11, 2024

The changes to the summary timeline are visually unappealing to me. I'll see what I can come up with. The lines probably need to be adjusted for screen resolution, and maybe the corner rounding removed (or a minimum width enforced).

@bdach
Copy link
Collaborator Author

bdach commented Jul 11, 2024

The changes to the summary timeline are visually unappealing to me (...) The lines probably need to be adjusted for screen resolution

Might be a hidpi thing. They seemed fine here.

@peppy peppy self-requested a review July 11, 2024 16:23
@peppy
Copy link
Member

peppy commented Jul 12, 2024

I've pushed enough changes to be on board with this now. I get that you're probably not willing to give a subjective opinion on this, but please check that you don't strongly disagree with anything.

Things I have started on but will push as separate efforts today:

  • Tooltips for all summary timeline displays
  • Stopping kiai start / ends from showing as effect point dots (they already show as a duration)

@peppy
Copy link
Member

peppy commented Jul 12, 2024

I'm done making changes here now, FWIW.

Will wait for re-approval from @bdach or someone else before merging.

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

bdach commented Jul 12, 2024

On a technical level changes seem fine. I've made most of my functional concerns with the changes clear internally already, but will restate here again for the sake of transparency.

  • Slider velocity currently covers up the tick behind it #28667 could be (on an uncharitable read) considered not resolved with this PR anymore because cases like this are now possible (while they weren't with my initial changes):
    1720770778
    They require hiding the timing indicators, yes, but they are possible.
  • BPM indicator is back but smaller, unsure what the user reaction will be.
  • Bottom timeline indicators are not thin anymore, unsure what the user reaction will be. The rationale given internally was that keeping them wide but giving them transparency instead was going to cause a similar effect to stable's thin lines of conveying density more properly.

Copy link
Member

@peppy peppy left a comment

Choose a reason for hiding this comment

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

Let's get these changes out and see what the feedback is like.

@bdach bdach merged commit 518dc17 into ppy:master Jul 12, 2024
14 of 17 checks passed
@bdach bdach deleted the timeline-redesign branch July 12, 2024 08:59
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.

2 participants