From e91d3dd8b4b5dfaba5adf41f649dcb0fd4802949 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bart=C5=82omiej=20Dach?= Date: Tue, 11 Jan 2022 21:21:04 +0100 Subject: [PATCH 1/4] Add failing test for incorrect sample control point time after paste --- .../Visual/Editing/TestSceneEditorClipboard.cs | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/osu.Game.Tests/Visual/Editing/TestSceneEditorClipboard.cs b/osu.Game.Tests/Visual/Editing/TestSceneEditorClipboard.cs index 3aff74a0a8..e41f8372b4 100644 --- a/osu.Game.Tests/Visual/Editing/TestSceneEditorClipboard.cs +++ b/osu.Game.Tests/Visual/Editing/TestSceneEditorClipboard.cs @@ -4,6 +4,7 @@ using System.Linq; using NUnit.Framework; using osu.Framework.Testing; +using osu.Framework.Utils; using osu.Game.Beatmaps; using osu.Game.Rulesets; using osu.Game.Rulesets.Edit; @@ -85,11 +86,17 @@ namespace osu.Game.Tests.Visual.Editing AddAssert("is one object", () => EditorBeatmap.HitObjects.Count == 1); + Slider slider = null; + AddStep("retrieve slider", () => slider = (Slider)EditorBeatmap.HitObjects.Single()); AddAssert("path matches", () => { - var path = ((Slider)EditorBeatmap.HitObjects.Single()).Path; + var path = slider.Path; return path.ControlPoints.Count == 2 && path.ControlPoints.SequenceEqual(addedObject.Path.ControlPoints); }); + + // see `HitObject.control_point_leniency`. + AddAssert("sample control point has correct time", () => Precision.AlmostEquals(slider.SampleControlPoint.Time, slider.GetEndTime(), 1)); + AddAssert("difficulty control point has correct time", () => slider.DifficultyControlPoint.Time == slider.StartTime); } [Test] From 7a25fe79b71c8c6ca7f08d9934ca4107a1b0fb36 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bart=C5=82omiej=20Dach?= Date: Tue, 11 Jan 2022 21:27:22 +0100 Subject: [PATCH 2/4] Fix sample control point time being calculated before defaults applied In editor contexts, the `StartTimeBindable` subscription in `HitObject` was firing before defaults were applied, which in the case of sliders manifested in an infinite end time. `ApplyDefaults()` also did not always set the time of the control point to the correct value, which matters when the beatmap is encoded. Ensure that the control points receive the correct time values during default application, and only register the `StartTimeBindable` change callback after defaults have been successfully applied. --- osu.Game/Rulesets/Objects/HitObject.cs | 44 ++++++++++++-------------- 1 file changed, 21 insertions(+), 23 deletions(-) diff --git a/osu.Game/Rulesets/Objects/HitObject.cs b/osu.Game/Rulesets/Objects/HitObject.cs index a80b3d0fa5..e63ccbc0d3 100644 --- a/osu.Game/Rulesets/Objects/HitObject.cs +++ b/osu.Game/Rulesets/Objects/HitObject.cs @@ -87,23 +87,6 @@ namespace osu.Game.Rulesets.Objects [JsonIgnore] public SlimReadOnlyListWrapper NestedHitObjects => nestedHitObjects.AsSlimReadOnly(); - public HitObject() - { - StartTimeBindable.ValueChanged += time => - { - double offset = time.NewValue - time.OldValue; - - foreach (var nested in nestedHitObjects) - nested.StartTime += offset; - - if (DifficultyControlPoint != DifficultyControlPoint.DEFAULT) - DifficultyControlPoint.Time = time.NewValue; - - if (SampleControlPoint != SampleControlPoint.DEFAULT) - SampleControlPoint.Time = this.GetEndTime() + control_point_leniency; - }; - } - /// /// Applies default values to this HitObject. /// @@ -115,24 +98,22 @@ namespace osu.Game.Rulesets.Objects var legacyInfo = controlPointInfo as LegacyControlPointInfo; if (legacyInfo != null) - { DifficultyControlPoint = (DifficultyControlPoint)legacyInfo.DifficultyPointAt(StartTime).DeepClone(); - DifficultyControlPoint.Time = StartTime; - } else if (DifficultyControlPoint == DifficultyControlPoint.DEFAULT) DifficultyControlPoint = new DifficultyControlPoint(); + DifficultyControlPoint.Time = StartTime; + ApplyDefaultsToSelf(controlPointInfo, difficulty); // This is done here after ApplyDefaultsToSelf as we may require custom defaults to be applied to have an accurate end time. if (legacyInfo != null) - { SampleControlPoint = (SampleControlPoint)legacyInfo.SamplePointAt(this.GetEndTime() + control_point_leniency).DeepClone(); - SampleControlPoint.Time = this.GetEndTime() + control_point_leniency; - } else if (SampleControlPoint == SampleControlPoint.DEFAULT) SampleControlPoint = new SampleControlPoint(); + SampleControlPoint.Time = this.GetEndTime() + control_point_leniency; + nestedHitObjects.Clear(); CreateNestedHitObjects(cancellationToken); @@ -155,6 +136,23 @@ namespace osu.Game.Rulesets.Objects foreach (var h in nestedHitObjects) h.ApplyDefaults(controlPointInfo, difficulty, cancellationToken); + // importantly, this callback is only registered after default application + // to ensure that the read of `this.GetEndTime()` within doesn't return an invalid value + // if `StartTimeBindable` is changed prior to default application. + StartTimeBindable.ValueChanged += time => + { + double offset = time.NewValue - time.OldValue; + + foreach (var nested in nestedHitObjects) + nested.StartTime += offset; + + if (DifficultyControlPoint != DifficultyControlPoint.DEFAULT) + DifficultyControlPoint.Time = time.NewValue; + + if (SampleControlPoint != SampleControlPoint.DEFAULT) + SampleControlPoint.Time = this.GetEndTime() + control_point_leniency; + }; + DefaultsApplied?.Invoke(this); } From 80ccff90689601ce449ac574ab777666ae953678 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bart=C5=82omiej=20Dach?= Date: Tue, 11 Jan 2022 22:12:23 +0100 Subject: [PATCH 3/4] Remove no longer necessary guards against default control points The subscription in which the guards were present was moved from constructor to `ApplyDefaults()`, and at that point neither the sample control point or the difficulty point can be the default point, because there are explicit paths that overwrite those with blank points in the same methods, prior to the subscription's registration. The only worry would be that someone would set the default point on the object themselves, but at that point that is most likely programmer error anyhow. --- osu.Game/Rulesets/Objects/HitObject.cs | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/osu.Game/Rulesets/Objects/HitObject.cs b/osu.Game/Rulesets/Objects/HitObject.cs index e63ccbc0d3..7d08261035 100644 --- a/osu.Game/Rulesets/Objects/HitObject.cs +++ b/osu.Game/Rulesets/Objects/HitObject.cs @@ -146,11 +146,8 @@ namespace osu.Game.Rulesets.Objects foreach (var nested in nestedHitObjects) nested.StartTime += offset; - if (DifficultyControlPoint != DifficultyControlPoint.DEFAULT) - DifficultyControlPoint.Time = time.NewValue; - - if (SampleControlPoint != SampleControlPoint.DEFAULT) - SampleControlPoint.Time = this.GetEndTime() + control_point_leniency; + DifficultyControlPoint.Time = time.NewValue; + SampleControlPoint.Time = this.GetEndTime() + control_point_leniency; }; DefaultsApplied?.Invoke(this); From afce976f086bc8aabe3b4d5f4ee6bcf63c91c5f7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bart=C5=82omiej=20Dach?= Date: Wed, 12 Jan 2022 19:24:59 +0100 Subject: [PATCH 4/4] Fix oversubscription to `StartTimeBindable.ValueChanged` --- osu.Game/Rulesets/Objects/HitObject.cs | 19 +++++++++++++------ 1 file changed, 13 insertions(+), 6 deletions(-) diff --git a/osu.Game/Rulesets/Objects/HitObject.cs b/osu.Game/Rulesets/Objects/HitObject.cs index 7d08261035..c590cc302f 100644 --- a/osu.Game/Rulesets/Objects/HitObject.cs +++ b/osu.Game/Rulesets/Objects/HitObject.cs @@ -136,10 +136,19 @@ namespace osu.Game.Rulesets.Objects foreach (var h in nestedHitObjects) h.ApplyDefaults(controlPointInfo, difficulty, cancellationToken); - // importantly, this callback is only registered after default application - // to ensure that the read of `this.GetEndTime()` within doesn't return an invalid value + // `ApplyDefaults()` may be called multiple times on a single hitobject. + // to prevent subscribing to `StartTimeBindable.ValueChanged` multiple times with the same callback, + // remove the previous subscription (if present) before (re-)registering. + StartTimeBindable.ValueChanged -= onStartTimeChanged; + + // this callback must be (re-)registered after default application + // to ensure that the read of `this.GetEndTime()` within `onStartTimeChanged` doesn't return an invalid value // if `StartTimeBindable` is changed prior to default application. - StartTimeBindable.ValueChanged += time => + StartTimeBindable.ValueChanged += onStartTimeChanged; + + DefaultsApplied?.Invoke(this); + + void onStartTimeChanged(ValueChangedEvent time) { double offset = time.NewValue - time.OldValue; @@ -148,9 +157,7 @@ namespace osu.Game.Rulesets.Objects DifficultyControlPoint.Time = time.NewValue; SampleControlPoint.Time = this.GetEndTime() + control_point_leniency; - }; - - DefaultsApplied?.Invoke(this); + } } protected virtual void ApplyDefaultsToSelf(ControlPointInfo controlPointInfo, IBeatmapDifficultyInfo difficulty)