From ea0683adb2369489c0684a11f2e6c1437538aa31 Mon Sep 17 00:00:00 2001 From: smoogipoo Date: Mon, 16 Apr 2018 20:18:33 +0900 Subject: [PATCH 1/6] Fix hitobject lengths not being calculated for overlapping speed changes Fixes #2359 --- .../OverlappingSpeedChangeVisualiser.cs | 28 +++++++++++++++---- 1 file changed, 23 insertions(+), 5 deletions(-) diff --git a/osu.Game/Rulesets/UI/Scrolling/Visualisers/OverlappingSpeedChangeVisualiser.cs b/osu.Game/Rulesets/UI/Scrolling/Visualisers/OverlappingSpeedChangeVisualiser.cs index 2365582645..c5e91c6929 100644 --- a/osu.Game/Rulesets/UI/Scrolling/Visualisers/OverlappingSpeedChangeVisualiser.cs +++ b/osu.Game/Rulesets/UI/Scrolling/Visualisers/OverlappingSpeedChangeVisualiser.cs @@ -4,6 +4,7 @@ using System.Collections.Generic; using osu.Framework.Lists; using osu.Game.Rulesets.Objects.Drawables; +using osu.Game.Rulesets.Objects.Types; using osu.Game.Rulesets.Timing; using OpenTK; @@ -22,8 +23,24 @@ namespace osu.Game.Rulesets.UI.Scrolling.Visualisers { foreach (var obj in hitObjects) { - var controlPoint = controlPointAt(obj.HitObject.StartTime); - obj.LifetimeStart = obj.HitObject.StartTime - timeRange / controlPoint.Multiplier; + obj.LifetimeStart = obj.HitObject.StartTime - timeRange / controlPointAt(obj.HitObject.StartTime).Multiplier; + + if (obj.HitObject is IHasEndTime endTime) + { + var diff = -positionAt(endTime.EndTime, obj, timeRange); + + switch (direction) + { + case ScrollingDirection.Up: + case ScrollingDirection.Down: + obj.Height = (float)(diff * length.Y); + break; + case ScrollingDirection.Left: + case ScrollingDirection.Right: + obj.Width = (float)(diff * length.X); + break; + } + } if (obj.HasNestedHitObjects) { @@ -37,9 +54,7 @@ namespace osu.Game.Rulesets.UI.Scrolling.Visualisers { foreach (var obj in hitObjects) { - var controlPoint = controlPointAt(obj.HitObject.StartTime); - - var position = (obj.HitObject.StartTime - currentTime) * controlPoint.Multiplier / timeRange; + var position = positionAt(currentTime, obj, timeRange); switch (direction) { @@ -59,6 +74,9 @@ namespace osu.Game.Rulesets.UI.Scrolling.Visualisers } } + private double positionAt(double time, DrawableHitObject obj, double timeRange) + => (obj.HitObject.StartTime - time) * controlPointAt(obj.HitObject.StartTime).Multiplier / timeRange; + private readonly MultiplierControlPoint searchPoint = new MultiplierControlPoint(); private MultiplierControlPoint controlPointAt(double time) { From 1bab601cbc85318eff49efc6ca049ec9fab4198a Mon Sep 17 00:00:00 2001 From: smoogipoo Date: Fri, 20 Apr 2018 13:51:36 +0900 Subject: [PATCH 2/6] Comments + xmldocs --- .../OverlappingSpeedChangeVisualiser.cs | 31 ++++++++++++++++--- 1 file changed, 26 insertions(+), 5 deletions(-) diff --git a/osu.Game/Rulesets/UI/Scrolling/Visualisers/OverlappingSpeedChangeVisualiser.cs b/osu.Game/Rulesets/UI/Scrolling/Visualisers/OverlappingSpeedChangeVisualiser.cs index c5e91c6929..6045ab50e7 100644 --- a/osu.Game/Rulesets/UI/Scrolling/Visualisers/OverlappingSpeedChangeVisualiser.cs +++ b/osu.Game/Rulesets/UI/Scrolling/Visualisers/OverlappingSpeedChangeVisualiser.cs @@ -23,21 +23,24 @@ namespace osu.Game.Rulesets.UI.Scrolling.Visualisers { foreach (var obj in hitObjects) { + // For optimal lifetimes, the speed of the hitobject is factored into the time range obj.LifetimeStart = obj.HitObject.StartTime - timeRange / controlPointAt(obj.HitObject.StartTime).Multiplier; if (obj.HitObject is IHasEndTime endTime) { - var diff = -positionAt(endTime.EndTime, obj, timeRange); + // At the hitobject's end time, the hitobject will be positioned such that its end rests at the origin. + // This results in a negative-position value, and the absolute of it indicates the length of the hitobject. + var hitObjectLength = -hitObjectPositionAt(obj, endTime.EndTime, timeRange); switch (direction) { case ScrollingDirection.Up: case ScrollingDirection.Down: - obj.Height = (float)(diff * length.Y); + obj.Height = (float)(hitObjectLength * length.Y); break; case ScrollingDirection.Left: case ScrollingDirection.Right: - obj.Width = (float)(diff * length.X); + obj.Width = (float)(hitObjectLength * length.X); break; } } @@ -45,6 +48,8 @@ namespace osu.Game.Rulesets.UI.Scrolling.Visualisers if (obj.HasNestedHitObjects) { ComputeInitialStates(obj.NestedHitObjects, direction, timeRange, length); + + // Nested hitobjects don't need to scroll, but they do need accurate positions ComputePositions(obj.NestedHitObjects, direction, obj.HitObject.StartTime, timeRange, length); } } @@ -54,7 +59,7 @@ namespace osu.Game.Rulesets.UI.Scrolling.Visualisers { foreach (var obj in hitObjects) { - var position = positionAt(currentTime, obj, timeRange); + var position = hitObjectPositionAt(obj, currentTime, timeRange); switch (direction) { @@ -74,10 +79,26 @@ namespace osu.Game.Rulesets.UI.Scrolling.Visualisers } } - private double positionAt(double time, DrawableHitObject obj, double timeRange) + /// + /// Computes the position of a at a point in time.
+ /// At t < startTime, position > 0.
+ /// At t = startTime, position = 0.
+ /// At t > startTime, position < 0. + /// + /// The . + /// The time to find the position of at. + /// The amount of time visualised by the scrolling area. + /// The position of in the scrolling area at time = . + private double hitObjectPositionAt(DrawableHitObject obj, double time, double timeRange) => (obj.HitObject.StartTime - time) * controlPointAt(obj.HitObject.StartTime).Multiplier / timeRange; private readonly MultiplierControlPoint searchPoint = new MultiplierControlPoint(); + + /// + /// Finds the which affects the speed of hitobjects at a specific time. + /// + /// The time which the should affect. + /// The . private MultiplierControlPoint controlPointAt(double time) { if (controlPoints.Count == 0) From 48b421b4b417ec2113980b8561c76482bc66fbbf Mon Sep 17 00:00:00 2001 From: smoogipoo Date: Fri, 20 Apr 2018 14:16:30 +0900 Subject: [PATCH 3/6] Add comments to SequentialSpeedChangeVisualiser --- .../OverlappingSpeedChangeVisualiser.cs | 4 ++- .../SequentialSpeedChangeVisualiser.cs | 29 ++++++++++++++++--- 2 files changed, 28 insertions(+), 5 deletions(-) diff --git a/osu.Game/Rulesets/UI/Scrolling/Visualisers/OverlappingSpeedChangeVisualiser.cs b/osu.Game/Rulesets/UI/Scrolling/Visualisers/OverlappingSpeedChangeVisualiser.cs index 6045ab50e7..e45e391dbc 100644 --- a/osu.Game/Rulesets/UI/Scrolling/Visualisers/OverlappingSpeedChangeVisualiser.cs +++ b/osu.Game/Rulesets/UI/Scrolling/Visualisers/OverlappingSpeedChangeVisualiser.cs @@ -80,10 +80,12 @@ namespace osu.Game.Rulesets.UI.Scrolling.Visualisers } /// - /// Computes the position of a at a point in time.
+ /// Computes the position of a at a point in time. + /// /// At t < startTime, position > 0.
/// At t = startTime, position = 0.
/// At t > startTime, position < 0. + /// /// /// The . /// The time to find the position of at. diff --git a/osu.Game/Rulesets/UI/Scrolling/Visualisers/SequentialSpeedChangeVisualiser.cs b/osu.Game/Rulesets/UI/Scrolling/Visualisers/SequentialSpeedChangeVisualiser.cs index 708a2f173b..c8725fab56 100644 --- a/osu.Game/Rulesets/UI/Scrolling/Visualisers/SequentialSpeedChangeVisualiser.cs +++ b/osu.Game/Rulesets/UI/Scrolling/Visualisers/SequentialSpeedChangeVisualiser.cs @@ -25,23 +25,25 @@ namespace osu.Game.Rulesets.UI.Scrolling.Visualisers { foreach (var obj in hitObjects) { + // To reduce iterations when updating hitobject positions later on, their initial positions are cached var startPosition = hitObjectPositions[obj] = positionAt(obj.HitObject.StartTime, timeRange); + // Todo: This is approximate and will be incorrect in the case of extreme speed changes obj.LifetimeStart = obj.HitObject.StartTime - timeRange - 1000; if (obj.HitObject is IHasEndTime endTime) { - var diff = positionAt(endTime.EndTime, timeRange) - startPosition; + var hitObjectLength = positionAt(endTime.EndTime, timeRange) - startPosition; switch (direction) { case ScrollingDirection.Up: case ScrollingDirection.Down: - obj.Height = (float)(diff * length.Y); + obj.Height = (float)(hitObjectLength * length.Y); break; case ScrollingDirection.Left: case ScrollingDirection.Right: - obj.Width = (float)(diff * length.X); + obj.Width = (float)(hitObjectLength * length.X); break; } } @@ -49,6 +51,8 @@ namespace osu.Game.Rulesets.UI.Scrolling.Visualisers if (obj.HasNestedHitObjects) { ComputeInitialStates(obj.NestedHitObjects, direction, timeRange, length); + + // Nested hitobjects don't need to scroll, but they do need accurate positions ComputePositions(obj.NestedHitObjects, direction, obj.HitObject.StartTime, timeRange, length); } } @@ -80,21 +84,38 @@ namespace osu.Game.Rulesets.UI.Scrolling.Visualisers } } + /// + /// Finds the position which corresponds to a point in time. + /// This is a non-linear operation that depends on all the control points up to and including the one active at the time value. + /// + /// The time to find the position at. + /// The amount of time visualised by the scrolling area. + /// A positive value indicating the position at . private double positionAt(double time, double timeRange) { double length = 0; + + // We need to consider all timing points until the specified time and not just the currently-active one, + // since each timing point individually affects the positions of _all_ hitobjects after its start time for (int i = 0; i < controlPoints.Count; i++) { var current = controlPoints[i]; var next = i < controlPoints.Count - 1 ? controlPoints[i + 1] : null; + // We don't need to consider any control points beyond the current time, since it will not yet + // affect any hitobjects if (i > 0 && current.StartTime > time) continue; // Duration of the current control point var currentDuration = (next?.StartTime ?? double.PositiveInfinity) - current.StartTime; - length += Math.Min(currentDuration, time - current.StartTime) * current.Multiplier / timeRange; + // We want to consider the minimal amount of time that this control point has affected, + // which may be either its duration, or the amount of time that has passed within it + var durationInCurrent = Math.Min(currentDuration, time - current.StartTime); + + // Figure out how much of the time range the duration represents, and adjust it by the speed multiplier + length += durationInCurrent / timeRange * current.Multiplier; } return length; From f3fddcc82cf3dbb92f3290fba90792d2522a56da Mon Sep 17 00:00:00 2001 From: smoogipoo Date: Fri, 20 Apr 2018 14:20:04 +0900 Subject: [PATCH 4/6] Reorder parameter for consistency --- .../Scrolling/Visualisers/OverlappingSpeedChangeVisualiser.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/osu.Game/Rulesets/UI/Scrolling/Visualisers/OverlappingSpeedChangeVisualiser.cs b/osu.Game/Rulesets/UI/Scrolling/Visualisers/OverlappingSpeedChangeVisualiser.cs index e45e391dbc..eaac10873e 100644 --- a/osu.Game/Rulesets/UI/Scrolling/Visualisers/OverlappingSpeedChangeVisualiser.cs +++ b/osu.Game/Rulesets/UI/Scrolling/Visualisers/OverlappingSpeedChangeVisualiser.cs @@ -92,7 +92,7 @@ namespace osu.Game.Rulesets.UI.Scrolling.Visualisers /// The amount of time visualised by the scrolling area. /// The position of in the scrolling area at time = . private double hitObjectPositionAt(DrawableHitObject obj, double time, double timeRange) - => (obj.HitObject.StartTime - time) * controlPointAt(obj.HitObject.StartTime).Multiplier / timeRange; + => (obj.HitObject.StartTime - time) / timeRange * controlPointAt(obj.HitObject.StartTime).Multiplier; private readonly MultiplierControlPoint searchPoint = new MultiplierControlPoint(); From 52e3ffff303327052c0ab962682c2574c7f3a572 Mon Sep 17 00:00:00 2001 From: smoogipoo Date: Fri, 20 Apr 2018 14:20:16 +0900 Subject: [PATCH 5/6] Add some more commenting to lifetime calculation --- .../Visualisers/OverlappingSpeedChangeVisualiser.cs | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/osu.Game/Rulesets/UI/Scrolling/Visualisers/OverlappingSpeedChangeVisualiser.cs b/osu.Game/Rulesets/UI/Scrolling/Visualisers/OverlappingSpeedChangeVisualiser.cs index eaac10873e..d0dc65fe33 100644 --- a/osu.Game/Rulesets/UI/Scrolling/Visualisers/OverlappingSpeedChangeVisualiser.cs +++ b/osu.Game/Rulesets/UI/Scrolling/Visualisers/OverlappingSpeedChangeVisualiser.cs @@ -23,8 +23,10 @@ namespace osu.Game.Rulesets.UI.Scrolling.Visualisers { foreach (var obj in hitObjects) { - // For optimal lifetimes, the speed of the hitobject is factored into the time range - obj.LifetimeStart = obj.HitObject.StartTime - timeRange / controlPointAt(obj.HitObject.StartTime).Multiplier; + // The total amount of time that the hitobject will remain visible within the timeRange, which decreases as the speed multiplier increases + double visibleDuration = timeRange / controlPointAt(obj.HitObject.StartTime).Multiplier; + + obj.LifetimeStart = obj.HitObject.StartTime - visibleDuration; if (obj.HitObject is IHasEndTime endTime) { From 11b943c820280282b276dcd3b923abaf9376f072 Mon Sep 17 00:00:00 2001 From: smoogipoo Date: Fri, 20 Apr 2018 14:22:48 +0900 Subject: [PATCH 6/6] ComputePositions -> UpdatePositions --- .../UI/Scrolling/ScrollingHitObjectContainer.cs | 2 +- .../Scrolling/Visualisers/ISpeedChangeVisualiser.cs | 13 ++++++------- .../Visualisers/OverlappingSpeedChangeVisualiser.cs | 4 ++-- .../Visualisers/SequentialSpeedChangeVisualiser.cs | 4 ++-- 4 files changed, 11 insertions(+), 12 deletions(-) diff --git a/osu.Game/Rulesets/UI/Scrolling/ScrollingHitObjectContainer.cs b/osu.Game/Rulesets/UI/Scrolling/ScrollingHitObjectContainer.cs index edfea57e94..36c6b07f54 100644 --- a/osu.Game/Rulesets/UI/Scrolling/ScrollingHitObjectContainer.cs +++ b/osu.Game/Rulesets/UI/Scrolling/ScrollingHitObjectContainer.cs @@ -110,7 +110,7 @@ namespace osu.Game.Rulesets.UI.Scrolling base.UpdateAfterChildrenLife(); // We need to calculate this as soon as possible after lifetimes so that hitobjects get the final say in their positions - speedChangeVisualiser.ComputePositions(AliveObjects, direction, Time.Current, TimeRange, DrawSize); + speedChangeVisualiser.UpdatePositions(AliveObjects, direction, Time.Current, TimeRange, DrawSize); } } } diff --git a/osu.Game/Rulesets/UI/Scrolling/Visualisers/ISpeedChangeVisualiser.cs b/osu.Game/Rulesets/UI/Scrolling/Visualisers/ISpeedChangeVisualiser.cs index 02791e0517..097e28b2dc 100644 --- a/osu.Game/Rulesets/UI/Scrolling/Visualisers/ISpeedChangeVisualiser.cs +++ b/osu.Game/Rulesets/UI/Scrolling/Visualisers/ISpeedChangeVisualiser.cs @@ -10,24 +10,23 @@ namespace osu.Game.Rulesets.UI.Scrolling.Visualisers public interface ISpeedChangeVisualiser { /// - /// Computes the states of s that are constant, such as lifetime and spatial length. + /// Computes the states of s that remain constant while scrolling, such as lifetime and spatial length. /// This is invoked once whenever or changes. /// /// The s whose states should be computed. /// The scrolling direction. - /// The duration required to scroll through one length of the screen before any control point adjustments. + /// The duration required to scroll through one length of the screen before any speed adjustments. /// The length of the screen that is scrolled through. void ComputeInitialStates(IEnumerable hitObjects, ScrollingDirection direction, double timeRange, Vector2 length); /// - /// Computes the states of s that change depending on , such as position. - /// This is invoked once per frame. + /// Updates the positions of s, depending on the current time. This is invoked once per frame. /// - /// The s whose states should be computed. + /// The s whose positions should be computed. /// The scrolling direction. /// The current time. - /// The duration required to scroll through one length of the screen before any control point adjustments. + /// The duration required to scroll through one length of the screen before any speed adjustments. /// The length of the screen that is scrolled through. - void ComputePositions(IEnumerable hitObjects, ScrollingDirection direction, double currentTime, double timeRange, Vector2 length); + void UpdatePositions(IEnumerable hitObjects, ScrollingDirection direction, double currentTime, double timeRange, Vector2 length); } } diff --git a/osu.Game/Rulesets/UI/Scrolling/Visualisers/OverlappingSpeedChangeVisualiser.cs b/osu.Game/Rulesets/UI/Scrolling/Visualisers/OverlappingSpeedChangeVisualiser.cs index d0dc65fe33..35a91275a7 100644 --- a/osu.Game/Rulesets/UI/Scrolling/Visualisers/OverlappingSpeedChangeVisualiser.cs +++ b/osu.Game/Rulesets/UI/Scrolling/Visualisers/OverlappingSpeedChangeVisualiser.cs @@ -52,12 +52,12 @@ namespace osu.Game.Rulesets.UI.Scrolling.Visualisers ComputeInitialStates(obj.NestedHitObjects, direction, timeRange, length); // Nested hitobjects don't need to scroll, but they do need accurate positions - ComputePositions(obj.NestedHitObjects, direction, obj.HitObject.StartTime, timeRange, length); + UpdatePositions(obj.NestedHitObjects, direction, obj.HitObject.StartTime, timeRange, length); } } } - public void ComputePositions(IEnumerable hitObjects, ScrollingDirection direction, double currentTime, double timeRange, Vector2 length) + public void UpdatePositions(IEnumerable hitObjects, ScrollingDirection direction, double currentTime, double timeRange, Vector2 length) { foreach (var obj in hitObjects) { diff --git a/osu.Game/Rulesets/UI/Scrolling/Visualisers/SequentialSpeedChangeVisualiser.cs b/osu.Game/Rulesets/UI/Scrolling/Visualisers/SequentialSpeedChangeVisualiser.cs index c8725fab56..e353c07e9f 100644 --- a/osu.Game/Rulesets/UI/Scrolling/Visualisers/SequentialSpeedChangeVisualiser.cs +++ b/osu.Game/Rulesets/UI/Scrolling/Visualisers/SequentialSpeedChangeVisualiser.cs @@ -53,12 +53,12 @@ namespace osu.Game.Rulesets.UI.Scrolling.Visualisers ComputeInitialStates(obj.NestedHitObjects, direction, timeRange, length); // Nested hitobjects don't need to scroll, but they do need accurate positions - ComputePositions(obj.NestedHitObjects, direction, obj.HitObject.StartTime, timeRange, length); + UpdatePositions(obj.NestedHitObjects, direction, obj.HitObject.StartTime, timeRange, length); } } } - public void ComputePositions(IEnumerable hitObjects, ScrollingDirection direction, double currentTime, double timeRange, Vector2 length) + public void UpdatePositions(IEnumerable hitObjects, ScrollingDirection direction, double currentTime, double timeRange, Vector2 length) { var timelinePosition = positionAt(currentTime, timeRange);