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.

Sign up for GitHub

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

Always show control points in timing mode #29196

Closed
wants to merge 2 commits into from
Closed

Always show control points in timing mode #29196

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
7237af6
Always show control points in timing view
kadambishreyas Jul 30, 2024
3e9317e
Add back accidentally removed delays
kadambishreyas Jul 30, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
34 changes: 24 additions & 10 deletions osu.Game/Screens/Edit/Compose/Components/Timeline/Timeline.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -82,15 +82,16 @@ public Timeline(Drawable userContent)
private Container mainContent;

private Bindable<float> waveformOpacity;
private Bindable<bool> controlPointsVisible;
private Bindable<bool> alwaysShowControlPoints;
private readonly Bindable<bool> controlPointsVisible = new Bindable<bool>();
Comment on lines +85 to +86
Copy link
Member

Choose a reason for hiding this comment

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

I don't think you need this new bindable, and can revert basically all your changes.

In the original controlPointsVisible.BindValueChanged you can just check the current editor mode and react there. Although one may argue this is too much local knowledge and the editor should be setting the value on the timeline, not vice-versa.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i'm unclear on how this would work, do you mind explaining to me why this is the case?
suppose we have the case of:

actions    | 1. start with config hidden -> 2. enable config -> 3. change to timing mode -> 4. disable config -> 5. go to compose mode
visibility | 1. hidden -> 2. shown -> 3. shown -> 4. shown -> 5. hidden
height     | 1. normal -> 2. expanded -> 3. expanded -> 4. normal -> 5. normal

if we don't create a bindable on mode state, at what point can code trigger at action (5) to correctly hide the config? is there a lifecycle thing here i'm missing..?

Copy link
Member

Choose a reason for hiding this comment

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

Check #29211, I've redone this feature in a slightly simpler way.

private Bindable<bool> ticksVisible;

private double trackLengthForZoom;

private readonly IBindable<Track> track = new Bindable<Track>();

[BackgroundDependencyLoader]
private void load(IBindable<WorkingBeatmap> beatmap, OsuColour colours, OsuConfigManager config)
private void load(IBindable<WorkingBeatmap> beatmap, OsuColour colours, OsuConfigManager config, Editor editor)
Copy link
Member

Choose a reason for hiding this comment

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

Editor has to be nullable for some tests to pass, as can be seen from CI failures.

{
CentreMarker centreMarker;

Expand Down Expand Up @@ -141,9 +142,13 @@ private void load(IBindable<WorkingBeatmap> beatmap, OsuColour colours, OsuConfi
});

waveformOpacity = config.GetBindable<float>(OsuSetting.EditorWaveformOpacity);
controlPointsVisible = config.GetBindable<bool>(OsuSetting.EditorTimelineShowTimingChanges);
ticksVisible = config.GetBindable<bool>(OsuSetting.EditorTimelineShowTicks);

alwaysShowControlPoints = config.GetBindable<bool>(OsuSetting.EditorTimelineShowTimingChanges);
alwaysShowControlPoints.BindValueChanged(_ => updateControlPointsVisible(editor));
editor.Mode.BindValueChanged(_ => updateControlPointsVisible(editor));
updateControlPointsVisible(editor);

track.BindTo(editorClock.Track);
track.BindValueChanged(_ =>
{
Expand All @@ -154,6 +159,11 @@ private void load(IBindable<WorkingBeatmap> beatmap, OsuColour colours, OsuConfi
Zoom = (float)(defaultTimelineZoom * editorBeatmap.BeatmapInfo.TimelineZoom);
}

private void updateControlPointsVisible(Editor editor)
{
controlPointsVisible.Value = alwaysShowControlPoints.Value || editor.Mode.Value == EditorScreenMode.Timing;
}

private void applyVisualOffset(IBindable<WorkingBeatmap> beatmap)
{
waveform.RelativePositionAxes = Axes.X;
Expand All @@ -176,25 +186,29 @@ protected override void LoadComplete()

ticksVisible.BindValueChanged(visible => ticks.FadeTo(visible.NewValue ? 1 : 0, 200, Easing.OutQuint), true);

controlPointsVisible.BindValueChanged(visible =>
alwaysShowControlPoints.BindValueChanged(visible =>
{
if (visible.NewValue)
{
this.ResizeHeightTo(timeline_expanded_height, 200, Easing.OutQuint);
mainContent.MoveToY(15, 200, Easing.OutQuint);

// delay the fade in else masking looks weird.
controlPoints.Delay(180).FadeIn(400, Easing.OutQuint);
}
else
{
controlPoints.FadeOut(200, Easing.OutQuint);

// likewise, delay the resize until the fade is complete.
// delay the resize until the fade is complete.
this.Delay(180).ResizeHeightTo(timeline_height, 200, Easing.OutQuint);
mainContent.Delay(180).MoveToY(0, 200, Easing.OutQuint);
}
}, true);

controlPointsVisible.BindValueChanged(visible =>
{
if (visible.NewValue)
// delay the fade in else masking looks weird.
controlPoints.Delay(180).FadeIn(400, Easing.OutQuint);
else
controlPoints.FadeOut(200, Easing.OutQuint);
}, true);
}

private void updateWaveformOpacity() =>
Expand Down
Loading