From a33294ac42717717c5fd603bea2d92fdca18ed50 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bart=C5=82omiej=20Dach?= Date: Tue, 20 Aug 2024 11:14:42 +0200 Subject: [PATCH 1/4] Redesign timing table tracking - 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 https://github.com/ppy/osu/issues/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. --- .../Screens/Edit/Timing/ControlPointList.cs | 83 +++--------- .../Screens/Edit/Timing/ControlPointTable.cs | 126 ++++++++++++++---- 2 files changed, 117 insertions(+), 92 deletions(-) diff --git a/osu.Game/Screens/Edit/Timing/ControlPointList.cs b/osu.Game/Screens/Edit/Timing/ControlPointList.cs index b7367dddda..4df52a0a3a 100644 --- a/osu.Game/Screens/Edit/Timing/ControlPointList.cs +++ b/osu.Game/Screens/Edit/Timing/ControlPointList.cs @@ -11,7 +11,6 @@ using osu.Framework.Input.Events; using osu.Game.Beatmaps.ControlPoints; using osu.Game.Graphics.UserInterface; using osu.Game.Graphics.UserInterfaceV2; -using osu.Game.Overlays; using osuTK; namespace osu.Game.Screens.Edit.Timing @@ -31,7 +30,7 @@ namespace osu.Game.Screens.Edit.Timing private Bindable selectedGroup { get; set; } = null!; [BackgroundDependencyLoader] - private void load(OverlayColourProvider colours) + private void load() { RelativeSizeAxes = Axes.Both; @@ -68,6 +67,14 @@ namespace osu.Game.Screens.Edit.Timing Anchor = Anchor.BottomRight, Origin = Anchor.BottomRight, }, + new RoundedButton + { + Text = "Go to current time", + Action = goToCurrentGroup, + Size = new Vector2(140, 30), + Anchor = Anchor.BottomRight, + Origin = Anchor.BottomRight, + }, } }, }; @@ -97,78 +104,18 @@ namespace osu.Game.Screens.Edit.Timing { base.Update(); - trackActivePoint(); - addButton.Enabled.Value = clock.CurrentTimeAccurate != selectedGroup.Value?.Time; } - private Type? trackedType; - - /// - /// Given the user has selected a control point group, we want to track any group which is - /// active at the current point in time which matches the type the user has selected. - /// - /// So if the user is currently looking at a timing point and seeks into the future, a - /// future timing point would be automatically selected if it is now the new "current" point. - /// - private void trackActivePoint() + private void goToCurrentGroup() { - // For simplicity only match on the first type of the active control point. - if (selectedGroup.Value == null) - trackedType = null; - else - { - switch (selectedGroup.Value.ControlPoints.Count) - { - // If the selected group has no control points, clear the tracked type. - // Otherwise the user will be unable to select a group with no control points. - case 0: - trackedType = null; - break; + double accurateTime = clock.CurrentTimeAccurate; - // If the selected group only has one control point, update the tracking type. - case 1: - trackedType = selectedGroup.Value?.ControlPoints[0].GetType(); - break; + var activeTimingPoint = Beatmap.ControlPointInfo.TimingPointAt(accurateTime); + var activeEffectPoint = Beatmap.ControlPointInfo.EffectPointAt(accurateTime); - // If the selected group has more than one control point, choose the first as the tracking type - // if we don't already have a singular tracked type. - default: - trackedType ??= selectedGroup.Value?.ControlPoints[0].GetType(); - break; - } - } - - if (trackedType != null) - { - double accurateTime = clock.CurrentTimeAccurate; - - // We don't have an efficient way of looking up groups currently, only individual point types. - // To improve the efficiency of this in the future, we should reconsider the overall structure of ControlPointInfo. - - // Find the next group which has the same type as the selected one. - ControlPointGroup? found = null; - - for (int i = 0; i < Beatmap.ControlPointInfo.Groups.Count; i++) - { - var g = Beatmap.ControlPointInfo.Groups[i]; - - if (g.Time > accurateTime) - continue; - - for (int j = 0; j < g.ControlPoints.Count; j++) - { - if (g.ControlPoints[j].GetType() == trackedType) - { - found = g; - break; - } - } - } - - if (found != null) - selectedGroup.Value = found; - } + double latestActiveTime = Math.Max(activeTimingPoint.Time, activeEffectPoint.Time); + selectedGroup.Value = Beatmap.ControlPointInfo.GroupAt(latestActiveTime); } private void delete() diff --git a/osu.Game/Screens/Edit/Timing/ControlPointTable.cs b/osu.Game/Screens/Edit/Timing/ControlPointTable.cs index 8dc0ced30e..501d8c0e41 100644 --- a/osu.Game/Screens/Edit/Timing/ControlPointTable.cs +++ b/osu.Game/Screens/Edit/Timing/ControlPointTable.cs @@ -6,6 +6,7 @@ using System.Linq; using osu.Framework.Allocation; using osu.Framework.Bindables; using osu.Framework.Graphics; +using osu.Framework.Graphics.Colour; using osu.Framework.Graphics.Containers; using osu.Framework.Graphics.Pooling; using osu.Framework.Graphics.Shapes; @@ -27,10 +28,27 @@ namespace osu.Game.Screens.Edit.Timing { public BindableList Groups { get; } = new BindableList(); + [Cached] + private Bindable activeTimingPoint { get; } = new Bindable(); + + [Cached] + private Bindable activeEffectPoint { get; } = new Bindable(); + + [Resolved] + private EditorBeatmap beatmap { get; set; } = null!; + + [Resolved] + private Bindable selectedGroup { get; set; } = null!; + + [Resolved] + private EditorClock editorClock { get; set; } = null!; + private const float timing_column_width = 300; private const float row_height = 25; private const float row_horizontal_padding = 20; + private ControlPointRowList list = null!; + [BackgroundDependencyLoader] private void load(OverlayColourProvider colours) { @@ -65,7 +83,7 @@ namespace osu.Game.Screens.Edit.Timing { Anchor = Anchor.CentreLeft, Origin = Anchor.CentreLeft, - Margin = new MarginPadding { Left = ControlPointTable.timing_column_width } + Margin = new MarginPadding { Left = timing_column_width } }, } }, @@ -73,7 +91,7 @@ namespace osu.Game.Screens.Edit.Timing { RelativeSizeAxes = Axes.Both, Padding = new MarginPadding { Top = row_height }, - Child = new ControlPointRowList + Child = list = new ControlPointRowList { RelativeSizeAxes = Axes.Both, RowData = { BindTarget = Groups, }, @@ -82,40 +100,63 @@ namespace osu.Game.Screens.Edit.Timing }; } + protected override void LoadComplete() + { + base.LoadComplete(); + + selectedGroup.BindValueChanged(_ => scrollToMostRelevantRow(force: true), true); + } + + protected override void Update() + { + base.Update(); + + scrollToMostRelevantRow(force: false); + } + + private void scrollToMostRelevantRow(bool force) + { + double accurateTime = editorClock.CurrentTimeAccurate; + + activeTimingPoint.Value = beatmap.ControlPointInfo.TimingPointAt(accurateTime); + activeEffectPoint.Value = beatmap.ControlPointInfo.EffectPointAt(accurateTime); + + double latestActiveTime = Math.Max(activeTimingPoint.Value?.Time ?? double.NegativeInfinity, activeEffectPoint.Value?.Time ?? double.NegativeInfinity); + var groupToShow = selectedGroup.Value ?? beatmap.ControlPointInfo.GroupAt(latestActiveTime); + list.ScrollTo(groupToShow, force); + } + private partial class ControlPointRowList : VirtualisedListContainer { - [Resolved] - private Bindable selectedGroup { get; set; } = null!; - public ControlPointRowList() : base(row_height, 50) { } - protected override ScrollContainer CreateScrollContainer() => new OsuScrollContainer(); + protected override ScrollContainer CreateScrollContainer() => new UserTrackingScrollContainer(); - protected override void LoadComplete() + protected new UserTrackingScrollContainer Scroll => (UserTrackingScrollContainer)base.Scroll; + + public void ScrollTo(ControlPointGroup group, bool force) { - base.LoadComplete(); + if (Scroll.UserScrolling && !force) + return; - selectedGroup.BindValueChanged(val => - { - // can't use `.ScrollIntoView()` here because of the list virtualisation not giving - // child items valid coordinates from the start, so ballpark something similar - // using estimated row height. - var row = Items.FlowingChildren.SingleOrDefault(item => item.Row.Equals(val.NewValue)); + // can't use `.ScrollIntoView()` here because of the list virtualisation not giving + // child items valid coordinates from the start, so ballpark something similar + // using estimated row height. + var row = Items.FlowingChildren.SingleOrDefault(item => item.Row.Equals(group)); - if (row == null) - return; + if (row == null) + return; - float minPos = row.Y; - float maxPos = minPos + row_height; + float minPos = row.Y; + float maxPos = minPos + row_height; - if (minPos < Scroll.Current) - Scroll.ScrollTo(minPos); - else if (maxPos > Scroll.Current + Scroll.DisplayableContent) - Scroll.ScrollTo(maxPos - Scroll.DisplayableContent); - }); + if (minPos < Scroll.Current) + Scroll.ScrollTo(minPos); + else if (maxPos > Scroll.Current + Scroll.DisplayableContent) + Scroll.ScrollTo(maxPos - Scroll.DisplayableContent); } } @@ -130,13 +171,23 @@ namespace osu.Game.Screens.Edit.Timing private readonly BindableWithCurrent current = new BindableWithCurrent(); private Box background = null!; + private Box currentIndicator = null!; [Resolved] private OverlayColourProvider colourProvider { get; set; } = null!; + [Resolved] + private OsuColour colours { get; set; } = null!; + [Resolved] private Bindable selectedGroup { get; set; } = null!; + [Resolved] + private Bindable activeTimingPoint { get; set; } = null!; + + [Resolved] + private Bindable activeEffectPoint { get; set; } = null!; + [Resolved] private EditorClock editorClock { get; set; } = null!; @@ -153,6 +204,12 @@ namespace osu.Game.Screens.Edit.Timing Colour = colourProvider.Background1, Alpha = 0, }, + currentIndicator = new Box + { + RelativeSizeAxes = Axes.Y, + Width = 5, + Alpha = 0, + }, new Container { RelativeSizeAxes = Axes.Both, @@ -174,7 +231,9 @@ namespace osu.Game.Screens.Edit.Timing { base.LoadComplete(); - selectedGroup.BindValueChanged(_ => updateState(), true); + selectedGroup.BindValueChanged(_ => updateState()); + activeEffectPoint.BindValueChanged(_ => updateState()); + activeTimingPoint.BindValueChanged(_ => updateState(), true); FinishTransforms(true); } @@ -213,12 +272,31 @@ namespace osu.Game.Screens.Edit.Timing { bool isSelected = selectedGroup.Value?.Equals(current.Value) == true; + bool hasCurrentTimingPoint = activeTimingPoint.Value != null && current.Value.ControlPoints.Contains(activeTimingPoint.Value); + bool hasCurrentEffectPoint = activeEffectPoint.Value != null && current.Value.ControlPoints.Contains(activeEffectPoint.Value); + if (IsHovered || isSelected) background.FadeIn(100, Easing.OutQuint); + else if (hasCurrentTimingPoint || hasCurrentEffectPoint) + background.FadeTo(0.2f, 100, Easing.OutQuint); else background.FadeOut(100, Easing.OutQuint); background.Colour = isSelected ? colourProvider.Colour3 : colourProvider.Background1; + + if (hasCurrentTimingPoint || hasCurrentEffectPoint) + { + currentIndicator.FadeIn(100, Easing.OutQuint); + + if (hasCurrentTimingPoint && hasCurrentEffectPoint) + currentIndicator.Colour = ColourInfo.GradientVertical(activeTimingPoint.Value!.GetRepresentingColour(colours), activeEffectPoint.Value!.GetRepresentingColour(colours)); + else if (hasCurrentTimingPoint) + currentIndicator.Colour = activeTimingPoint.Value!.GetRepresentingColour(colours); + else + currentIndicator.Colour = activeEffectPoint.Value!.GetRepresentingColour(colours); + } + else + currentIndicator.FadeOut(100, Easing.OutQuint); } } From 333e5b8cac7aa19afac0014732325390dbcdb323 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bart=C5=82omiej=20Dach?= Date: Tue, 20 Aug 2024 11:23:39 +0200 Subject: [PATCH 2/4] Remove outdated tests --- .../Visual/Editing/TestSceneTimingScreen.cs | 34 ------------------- 1 file changed, 34 deletions(-) diff --git a/osu.Game.Tests/Visual/Editing/TestSceneTimingScreen.cs b/osu.Game.Tests/Visual/Editing/TestSceneTimingScreen.cs index 6181024230..cf07ce2431 100644 --- a/osu.Game.Tests/Visual/Editing/TestSceneTimingScreen.cs +++ b/osu.Game.Tests/Visual/Editing/TestSceneTimingScreen.cs @@ -114,40 +114,6 @@ namespace osu.Game.Tests.Visual.Editing }); } - [Test] - public void TestTrackingCurrentTimeWhileRunning() - { - AddStep("Select first effect point", () => - { - InputManager.MoveMouseTo(Child.ChildrenOfType().First()); - InputManager.Click(MouseButton.Left); - }); - - AddUntilStep("Selection changed", () => timingScreen.SelectedGroup.Value.Time == 54670); - AddUntilStep("Ensure seeked to correct time", () => EditorClock.CurrentTimeAccurate == 54670); - - AddStep("Seek to just before next point", () => EditorClock.Seek(69000)); - AddStep("Start clock", () => EditorClock.Start()); - - AddUntilStep("Selection changed", () => timingScreen.SelectedGroup.Value.Time == 69670); - } - - [Test] - public void TestTrackingCurrentTimeWhilePaused() - { - AddStep("Select first effect point", () => - { - InputManager.MoveMouseTo(Child.ChildrenOfType().First()); - InputManager.Click(MouseButton.Left); - }); - - AddUntilStep("Selection changed", () => timingScreen.SelectedGroup.Value.Time == 54670); - AddUntilStep("Ensure seeked to correct time", () => EditorClock.CurrentTimeAccurate == 54670); - - AddStep("Seek to later", () => EditorClock.Seek(80000)); - AddUntilStep("Selection changed", () => timingScreen.SelectedGroup.Value.Time == 69670); - } - [Test] public void TestScrollControlGroupIntoView() { From c4f08b42abacb959008d35535246fae3a0cb801f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bart=C5=82omiej=20Dach?= Date: Wed, 21 Aug 2024 09:05:10 +0200 Subject: [PATCH 3/4] Use colours to distinguish buttons better --- osu.Game/Screens/Edit/Timing/ControlPointList.cs | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/osu.Game/Screens/Edit/Timing/ControlPointList.cs b/osu.Game/Screens/Edit/Timing/ControlPointList.cs index 4df52a0a3a..cbef0b9064 100644 --- a/osu.Game/Screens/Edit/Timing/ControlPointList.cs +++ b/osu.Game/Screens/Edit/Timing/ControlPointList.cs @@ -9,6 +9,7 @@ using osu.Framework.Graphics; using osu.Framework.Graphics.Containers; using osu.Framework.Input.Events; using osu.Game.Beatmaps.ControlPoints; +using osu.Game.Graphics; using osu.Game.Graphics.UserInterface; using osu.Game.Graphics.UserInterfaceV2; using osuTK; @@ -30,7 +31,7 @@ namespace osu.Game.Screens.Edit.Timing private Bindable selectedGroup { get; set; } = null!; [BackgroundDependencyLoader] - private void load() + private void load(OsuColour colours) { RelativeSizeAxes = Axes.Both; @@ -59,6 +60,7 @@ namespace osu.Game.Screens.Edit.Timing Action = delete, Anchor = Anchor.BottomRight, Origin = Anchor.BottomRight, + BackgroundColour = colours.Red3, }, addButton = new RoundedButton { @@ -66,6 +68,7 @@ namespace osu.Game.Screens.Edit.Timing Size = new Vector2(160, 30), Anchor = Anchor.BottomRight, Origin = Anchor.BottomRight, + BackgroundColour = colours.Green3, }, new RoundedButton { From 9df12e3d8750c56b0190e407c25f44db7cb6f340 Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Thu, 22 Aug 2024 14:15:36 +0900 Subject: [PATCH 4/4] Move seek button to left to differentiate mutating operations --- .../Screens/Edit/Timing/ControlPointList.cs | 29 +++++++++++++------ 1 file changed, 20 insertions(+), 9 deletions(-) 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 @@ namespace osu.Game.Screens.Edit.Timing 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 @@ namespace osu.Game.Screens.Edit.Timing 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, }, } },