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 timing table tracking #29532

Merged
merged 6 commits into from
Aug 22, 2024
Merged

Conversation

bdach
Copy link
Collaborator

@bdach bdach commented Aug 20, 2024

RFC. This change reverts some behaviours of the timing screen that were (in my view) causing overall jankness, and manifested in actual issues such as #23147. The general spirit of the change is "don't try to do too much for the user".

The reason why I went in this direction is that rather than layer additional silent mechanics onto this screen such as "locking rows" or whatever, I'd like to try removing automation first and seeing how that gets received. Something something, "perfection is achieved, not when there is nothing more to add, but when there is nothing left to take away".

  • On entering the screen, the timing point active at the current instant of the map is selected. This is the only time where the selected point is changed automatically for the user.

  • The ongoing automatic tracking of the relevant point after the initial selection is gone. Even knowing the fact that it was supposed to track the supposedly relevant "last selected type" of control point, I always found the tracking to be fairly arbitrary in how it works. Removing this behaviour also incidentally fixes Can't move timing point using "Use current time" #23147.

    In its stead, to indicate which timing groups are having an effect, they receive an indicator line on the left (coloured using the relevant control points' representing colours), as well as a slight highlight effect.

  • If there is no control point selected, the table will autoscroll to the latest timing group, unless the user manually scrolled the table before.

  • If the selected control point changes, the table will autoscroll to the newly selected point, regardless of whether the user manually scrolled the table before.

  • A new button is added which permits the user to select the latest timing group. As per the point above, this will autoscroll the user to that group at the same time.

Additional test coverage for these behaviours can be added once I'm sure this PR won't get immediately dumpstered on sight.

1724146059

- On entering the screen, the timing point active at the current instant
  of the map is selected. This is the *only* time where the selected
  point is changed automatically for the user.

- The ongoing automatic tracking of the relevant point after the initial
  selection is *gone*. Even knowing the fact that it was supposed to
  track the supposedly relevant "last selected type" of control point,
  I always found the tracking to be fairly arbitrary in how it works.
  Removing this behaviour also incidentally fixes
  ppy#23147.

  In its stead, to indicate which timing groups are having an effect,
  they receive an indicator line on the left (coloured using the
  relevant control points' representing colours), as well as a slight
  highlight effect.

- If there is no control point selected, the table will autoscroll to
  the latest timing group, unless the user manually scrolled the table
  before.

- If the selected control point changes, the table will autoscroll to
  the newly selected point, *regardless* of whether the user manually
  scrolled the table before.

- A new button is added which permits the user to select the latest
  timing group. As per the point above, this will autoscroll the user
  to that group at the same time.
@peppy
Copy link
Member

peppy commented Aug 21, 2024

I'm okay with these changes, but I do think the buttons for creating new points (both of them) should be a different colour to the new one which jumps around.

@bdach
Copy link
Collaborator Author

bdach commented Aug 21, 2024

Does c4f08b4 work for you?

osu_2024-08-21_08-54-02

@peppy
Copy link
Member

peppy commented Aug 21, 2024

Hmm, might be a bit too colourful now 😅 I'll have a look now.

@peppy
Copy link
Member

peppy commented Aug 21, 2024

How about this?

diff --git a/osu.Game/Screens/Edit/Timing/ControlPointList.cs b/osu.Game/Screens/Edit/Timing/ControlPointList.cs
index cbef0b9064..8699c388b3 100644
--- a/osu.Game/Screens/Edit/Timing/ControlPointList.cs
+++ b/osu.Game/Screens/Edit/Timing/ControlPointList.cs
@@ -44,6 +44,26 @@ private void load(OsuColour colours)
                     Groups = { BindTarget = Beatmap.ControlPointInfo.Groups, },
                 },
                 new FillFlowContainer
+                {
+                    AutoSizeAxes = Axes.Both,
+                    Anchor = Anchor.BottomLeft,
+                    Origin = Anchor.BottomLeft,
+                    Direction = FillDirection.Horizontal,
+                    Margin = new MarginPadding(margins),
+                    Spacing = new Vector2(5),
+                    Children = new Drawable[]
+                    {
+                        new RoundedButton
+                        {
+                            Text = "Select closest to current time",
+                            Action = goToCurrentGroup,
+                            Size = new Vector2(220, 30),
+                            Anchor = Anchor.BottomRight,
+                            Origin = Anchor.BottomRight,
+                        },
+                    }
+                },
+                new FillFlowContainer
                 {
                     AutoSizeAxes = Axes.Both,
                     Anchor = Anchor.BottomRight,
@@ -68,15 +88,6 @@ private void load(OsuColour colours)
                             Size = new Vector2(160, 30),
                             Anchor = Anchor.BottomRight,
                             Origin = Anchor.BottomRight,
-                            BackgroundColour = colours.Green3,
-                        },
-                        new RoundedButton
-                        {
-                            Text = "Go to current time",
-                            Action = goToCurrentGroup,
-                            Size = new Vector2(140, 30),
-                            Anchor = Anchor.BottomRight,
-                            Origin = Anchor.BottomRight,
                         },
                     }
                 },

In a future effort we should re-think these buttons. For instance, delete may work better inline in the table to allow for quick deletion, or even in the right control area.

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.

As commented.

@bdach
Copy link
Collaborator Author

bdach commented Aug 21, 2024

Sure, fine to apply.

@peppy peppy self-requested a review August 21, 2024 08:44
@peppy
Copy link
Member

peppy commented Aug 22, 2024

On a code and UX pass this seems okay. I still kinda liked the previous behaviour but let's see if I was alone or others ask for this to return.

@bdach I'm okay with or without tests, so up to you.

@bdach
Copy link
Collaborator Author

bdach commented Aug 22, 2024

I'm just gonna merge /shrug

@bdach bdach merged commit d33f483 into ppy:master Aug 22, 2024
13 checks passed
@bdach bdach deleted the redesign-timing-table-tracking branch August 22, 2024 06:21
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.

Can't move timing point using "Use current time"
2 participants