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

Reduce number of redundant control points displayed on summary timeline #16446

Merged
@@ -1,6 +1,7 @@
// Copyright (c) ppy Pty Ltd <contact@ppy.sh>. Licensed under the MIT Licence.
// See the LICENCE file in the repository root for full licence text.

using System;
using System.Collections.Specialized;
using System.Linq;
using osu.Framework.Bindables;
Expand Down Expand Up @@ -31,15 +32,37 @@ protected override void LoadBeatmap(EditorBeatmap beatmap)

case NotifyCollectionChangedAction.Add:
foreach (var group in args.NewItems.OfType<ControlPointGroup>())
{
// as an optimisation, don't add a visualisation if there are already groups with the same types in close proximity.
// for newly added control points (ie. lazer editor first where group is added empty) we always skip for simplicity.
// that is fine, because cases where this is causing a performance issue are mostly where external tools were used to create an insane number of points.
if (Children.Any(g => Math.Abs(g.Group.Time - group.Time) < 1000 && g.IsRedundant(group)))
continue;

Add(new GroupVisualisation(group));
}

break;

case NotifyCollectionChangedAction.Remove:
foreach (var group in args.OldItems.OfType<ControlPointGroup>())
{
var matching = Children.SingleOrDefault(gv => gv.Group == group);

matching?.Expire();
if (matching != null)
matching.Expire();
else
{
// due to the add optimisation above, if a point is deleted which wasn't being displayed we need to recreate all points
// to guarantee an accurate representation.
//
// note that the case where control point (type) is added or removed from a non-displayed group is not handled correctly.
// this is an edge case which shouldn't affect the user too badly. we may flatted control point groups in the future
bdach marked this conversation as resolved.
Show resolved Hide resolved
// which would allow this to be handled better.
Clear();
foreach (var g in controlPointGroups)
Add(new GroupVisualisation(g));
}
}

break;
Expand Down
Expand Up @@ -9,7 +9,7 @@

namespace osu.Game.Screens.Edit.Components.Timelines.Summary.Parts
{
public class ControlPointVisualisation : PointVisualisation
public class ControlPointVisualisation : PointVisualisation, IControlPointVisualisationRedundant
{
protected readonly ControlPoint Point;

Expand All @@ -26,5 +26,7 @@ private void load(OsuColour colours)
{
Colour = Point.GetRepresentingColour(colours);
}

public bool IsRedundant(ControlPoint other) => other.GetType() == Point.GetType();
}
}
Expand Up @@ -13,7 +13,7 @@

namespace osu.Game.Screens.Edit.Components.Timelines.Summary.Parts
{
public class EffectPointVisualisation : CompositeDrawable
public class EffectPointVisualisation : CompositeDrawable, IControlPointVisualisationRedundant
{
private readonly EffectControlPoint effect;
private Bindable<bool> kiai;
Expand Down Expand Up @@ -68,5 +68,8 @@ private void load()
}
}, true);
}

// kiai sections display duration, so are required to be visualised.
public bool IsRedundant(ControlPoint other) => (other as EffectControlPoint)?.KiaiMode == effect.KiaiMode;
Copy link
Collaborator

Choose a reason for hiding this comment

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

no type check? so any other point type can hide this? may be especially relevant for scroll speed changes, which are handled by effect points on scrolling rulesets now

Copy link
Sponsor Member Author

Choose a reason for hiding this comment

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

I don't quite follow. The existing as check will cause a null/false if the type doesn't match.

Copy link
Collaborator

Choose a reason for hiding this comment

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

oh. i was defeated by nullable tri-state boolean logic

}
}
@@ -1,6 +1,7 @@
// Copyright (c) ppy Pty Ltd <contact@ppy.sh>. Licensed under the MIT Licence.
// See the LICENCE file in the repository root for full licence text.

using System.Linq;
using osu.Framework.Bindables;
using osu.Framework.Graphics;
using osu.Framework.Graphics.Containers;
Expand All @@ -23,12 +24,8 @@ public GroupVisualisation(ControlPointGroup group)

Group = group;
X = (float)group.Time;
}

protected override void LoadComplete()
{
base.LoadComplete();

// Run in constructor so IsRedundant calls can work correctly.
controlPoints.BindTo(Group.ControlPoints);
controlPoints.BindCollectionChanged((_, __) =>
{
Expand Down Expand Up @@ -60,5 +57,13 @@ protected override void LoadComplete()
}
}, true);
}

/// <summary>
/// For display purposes, check whether the proposed group is made redundant by this visualisation group.
/// </summary>
/// <param name="other"></param>
/// <returns></returns>
bdach marked this conversation as resolved.
Show resolved Hide resolved
public bool IsRedundant(ControlPointGroup other) =>
other.ControlPoints.Any(c => InternalChildren.OfType<IControlPointVisualisationRedundant>().Any(c2 => c2.IsRedundant(c)));
}
}
@@ -0,0 +1,12 @@
// Copyright (c) ppy Pty Ltd <contact@ppy.sh>. Licensed under the MIT Licence.
// See the LICENCE file in the repository root for full licence text.

using osu.Game.Beatmaps.ControlPoints;

namespace osu.Game.Screens.Edit.Components.Timelines.Summary.Parts
{
public interface IControlPointVisualisationRedundant
bdach marked this conversation as resolved.
Show resolved Hide resolved
{
bool IsRedundant(ControlPoint other);
}
}