From 2bef9312d9c88f4424ee0896b38b240bb5a3801d Mon Sep 17 00:00:00 2001 From: smoogipoo Date: Fri, 6 Nov 2020 22:11:49 +0900 Subject: [PATCH 01/19] Make SkinReloadableDrawable poolable --- osu.Game/Skinning/SkinReloadableDrawable.cs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/osu.Game/Skinning/SkinReloadableDrawable.cs b/osu.Game/Skinning/SkinReloadableDrawable.cs index 4a1aaa62bf..d0531c46c5 100644 --- a/osu.Game/Skinning/SkinReloadableDrawable.cs +++ b/osu.Game/Skinning/SkinReloadableDrawable.cs @@ -4,13 +4,14 @@ using System; using osu.Framework.Allocation; using osu.Framework.Graphics.Containers; +using osu.Framework.Graphics.Pooling; namespace osu.Game.Skinning { /// /// A drawable which has a callback when the skin changes. /// - public abstract class SkinReloadableDrawable : CompositeDrawable + public abstract class SkinReloadableDrawable : PoolableDrawable { /// /// Invoked when has changed. From 248d342a2f0e487c6b5c000aee604b5337c25c57 Mon Sep 17 00:00:00 2001 From: smoogipoo Date: Fri, 6 Nov 2020 22:15:00 +0900 Subject: [PATCH 02/19] Initial Apply()/FreeAfterUse() DHO implementation --- .../Objects/Drawables/DrawableHitObject.cs | 169 ++++++++++++------ osu.Game/Rulesets/Objects/HitObject.cs | 6 + osu.Game/Rulesets/UI/HitObjectContainer.cs | 89 ++++----- osu.Game/Skinning/SkinReloadableDrawable.cs | 1 - 4 files changed, 168 insertions(+), 97 deletions(-) diff --git a/osu.Game/Rulesets/Objects/Drawables/DrawableHitObject.cs b/osu.Game/Rulesets/Objects/Drawables/DrawableHitObject.cs index 5c3f57c2d0..43688a7332 100644 --- a/osu.Game/Rulesets/Objects/Drawables/DrawableHitObject.cs +++ b/osu.Game/Rulesets/Objects/Drawables/DrawableHitObject.cs @@ -3,6 +3,7 @@ using System; using System.Collections.Generic; +using System.Collections.Specialized; using System.Linq; using JetBrains.Annotations; using osu.Framework.Allocation; @@ -27,7 +28,10 @@ namespace osu.Game.Rulesets.Objects.Drawables { public event Action DefaultsApplied; - public readonly HitObject HitObject; + /// + /// The currently represented by this . + /// + public HitObject HitObject { get; private set; } /// /// The colour used for various elements of this DrawableHitObject. @@ -96,10 +100,10 @@ namespace osu.Game.Rulesets.Objects.Drawables /// protected virtual float SamplePlaybackPosition => 0.5f; - private BindableList samplesBindable; - private Bindable startTimeBindable; - private Bindable userPositionalHitSounds; - private Bindable comboIndexBindable; + public readonly Bindable StartTimeBindable = new Bindable(); + private readonly BindableList samplesBindable = new BindableList(); + private readonly Bindable userPositionalHitSounds = new Bindable(); + private readonly Bindable comboIndexBindable = new Bindable(); public override bool RemoveWhenNotAlive => false; public override bool RemoveCompletedTransforms => false; @@ -111,52 +115,120 @@ namespace osu.Game.Rulesets.Objects.Drawables public IBindable State => state; - protected DrawableHitObject([NotNull] HitObject hitObject) + /// + /// Creates a new . + /// + /// + /// The to be initially applied to this . + /// If null, a hitobject is expected to be later applied via (or automatically via pooling). + /// + protected DrawableHitObject([CanBeNull] HitObject initialHitObject = null) { - HitObject = hitObject ?? throw new ArgumentNullException(nameof(hitObject)); + HitObject = initialHitObject; } [BackgroundDependencyLoader] private void load(OsuConfigManager config) { - userPositionalHitSounds = config.GetBindable(OsuSetting.PositionalHitSounds); - var judgement = HitObject.CreateJudgement(); - - Result = CreateResult(judgement); - if (Result == null) - throw new InvalidOperationException($"{GetType().ReadableName()} must provide a {nameof(JudgementResult)} through {nameof(CreateResult)}."); + config.BindWith(OsuSetting.PositionalHitSounds, userPositionalHitSounds); } protected override void LoadAsyncComplete() { base.LoadAsyncComplete(); - LoadSamples(); - - HitObject.DefaultsApplied += onDefaultsApplied; - - startTimeBindable = HitObject.StartTimeBindable.GetBoundCopy(); - startTimeBindable.BindValueChanged(_ => updateState(State.Value, true)); - - if (HitObject is IHasComboInformation combo) - { - comboIndexBindable = combo.ComboIndexBindable.GetBoundCopy(); - comboIndexBindable.BindValueChanged(_ => updateComboColour(), true); - } - - samplesBindable = HitObject.SamplesBindable.GetBoundCopy(); - samplesBindable.CollectionChanged += (_, __) => LoadSamples(); - - apply(HitObject); + if (HitObject != null) + Apply(HitObject); } protected override void LoadComplete() { base.LoadComplete(); + StartTimeBindable.BindValueChanged(_ => updateState(State.Value, true)); + comboIndexBindable.BindValueChanged(_ => updateComboColour(), true); + updateState(ArmedState.Idle, true); } + /// + /// Removes the currently applied to this , + /// + protected override void FreeAfterUse() + { + StartTimeBindable.UnbindFrom(HitObject.StartTimeBindable); + if (HitObject is IHasComboInformation combo) + comboIndexBindable.UnbindFrom(combo.ComboIndexBindable); + + samplesBindable.UnbindFrom(HitObject.SamplesBindable); + + // When a new hitobject is applied, the samples will be cleared before re-populating. + // In order to stop this needless update, the event is unbound and re-bound as late as possible in Apply(). + samplesBindable.CollectionChanged -= onSamplesChanged; + + if (nestedHitObjects.IsValueCreated) + { + foreach (var obj in nestedHitObjects.Value) + { + obj.OnNewResult -= onNewResult; + obj.OnRevertResult -= onRevertResult; + obj.ApplyCustomUpdateState -= onApplyCustomUpdateState; + } + + nestedHitObjects.Value.Clear(); + ClearNestedHitObjects(); + } + + HitObject.DefaultsApplied -= onDefaultsApplied; + HitObject = null; + + base.FreeAfterUse(); + } + + /// + /// Applies a new to be represented by this . + /// + /// + public virtual void Apply(HitObject hitObject) + { + HitObject = hitObject; + + // Copy any existing result from the hitobject (required for rewind / judgement revert). + Result = HitObject.Result; + + // Ensure this DHO has a result. + Result ??= CreateResult(HitObject.CreateJudgement()) + ?? throw new InvalidOperationException($"{GetType().ReadableName()} must provide a {nameof(JudgementResult)} through {nameof(CreateResult)}."); + + // Ensure the hitobject has a result. + HitObject.Result = Result; + + foreach (var h in HitObject.NestedHitObjects) + { + var drawableNested = CreateNestedHitObject(h) ?? throw new InvalidOperationException($"{nameof(CreateNestedHitObject)} returned null for {h.GetType().ReadableName()}."); + + drawableNested.OnNewResult += (d, r) => OnNewResult?.Invoke(d, r); + drawableNested.OnRevertResult += (d, r) => OnRevertResult?.Invoke(d, r); + drawableNested.ApplyCustomUpdateState += (d, j) => ApplyCustomUpdateState?.Invoke(d, j); + + nestedHitObjects.Value.Add(drawableNested); + AddNestedHitObject(drawableNested); + } + + StartTimeBindable.BindTo(HitObject.StartTimeBindable); + if (HitObject is IHasComboInformation combo) + comboIndexBindable.BindTo(combo.ComboIndexBindable); + + samplesBindable.BindTo(HitObject.SamplesBindable); + samplesBindable.BindCollectionChanged(onSamplesChanged, true); + + HitObject.DefaultsApplied += onDefaultsApplied; + + // If not loaded, the state update happens in LoadComplete(). Otherwise, the update is scheduled to allow for lifetime updates. + if (IsLoaded) + Schedule(() => updateState(ArmedState.Idle, true)); + } + /// /// Invoked by the base to populate samples, once on initial load and potentially again on any change to the samples collection. /// @@ -183,34 +255,21 @@ namespace osu.Game.Rulesets.Objects.Drawables AddInternal(Samples); } + private void onSamplesChanged(object sender, NotifyCollectionChangedEventArgs e) => LoadSamples(); + + private void onNewResult(DrawableHitObject drawableHitObject, JudgementResult result) => OnNewResult?.Invoke(drawableHitObject, result); + + private void onRevertResult(DrawableHitObject drawableHitObject, JudgementResult result) => OnRevertResult?.Invoke(drawableHitObject, result); + + private void onApplyCustomUpdateState(DrawableHitObject drawableHitObject, ArmedState state) => ApplyCustomUpdateState?.Invoke(drawableHitObject, state); + private void onDefaultsApplied(HitObject hitObject) { - apply(hitObject); - updateState(state.Value, true); + FreeAfterUse(); + Apply(hitObject); DefaultsApplied?.Invoke(this); } - private void apply(HitObject hitObject) - { - if (nestedHitObjects.IsValueCreated) - { - nestedHitObjects.Value.Clear(); - ClearNestedHitObjects(); - } - - foreach (var h in hitObject.NestedHitObjects) - { - var drawableNested = CreateNestedHitObject(h) ?? throw new InvalidOperationException($"{nameof(CreateNestedHitObject)} returned null for {h.GetType().ReadableName()}."); - - drawableNested.OnNewResult += (d, r) => OnNewResult?.Invoke(d, r); - drawableNested.OnRevertResult += (d, r) => OnRevertResult?.Invoke(d, r); - drawableNested.ApplyCustomUpdateState += (d, j) => ApplyCustomUpdateState?.Invoke(d, j); - - nestedHitObjects.Value.Add(drawableNested); - AddNestedHitObject(drawableNested); - } - } - /// /// Invoked by the base to add nested s to the hierarchy. /// @@ -600,7 +659,9 @@ namespace osu.Game.Rulesets.Objects.Drawables protected override void Dispose(bool isDisposing) { base.Dispose(isDisposing); - HitObject.DefaultsApplied -= onDefaultsApplied; + + if (HitObject != null) + HitObject.DefaultsApplied -= onDefaultsApplied; } } diff --git a/osu.Game/Rulesets/Objects/HitObject.cs b/osu.Game/Rulesets/Objects/HitObject.cs index 826d411822..9ef3ff9c4a 100644 --- a/osu.Game/Rulesets/Objects/HitObject.cs +++ b/osu.Game/Rulesets/Objects/HitObject.cs @@ -12,6 +12,7 @@ using osu.Game.Audio; using osu.Game.Beatmaps; using osu.Game.Beatmaps.ControlPoints; using osu.Game.Rulesets.Judgements; +using osu.Game.Rulesets.Objects.Drawables; using osu.Game.Rulesets.Objects.Types; using osu.Game.Rulesets.Scoring; @@ -168,6 +169,11 @@ namespace osu.Game.Rulesets.Objects /// [NotNull] protected virtual HitWindows CreateHitWindows() => new HitWindows(); + + /// + /// The result this was judged with. Used internally for rewinding within . + /// + internal JudgementResult Result; } public static class HitObjectExtensions diff --git a/osu.Game/Rulesets/UI/HitObjectContainer.cs b/osu.Game/Rulesets/UI/HitObjectContainer.cs index 4cadfa9ad4..4666aa9211 100644 --- a/osu.Game/Rulesets/UI/HitObjectContainer.cs +++ b/osu.Game/Rulesets/UI/HitObjectContainer.cs @@ -16,7 +16,7 @@ namespace osu.Game.Rulesets.UI public IEnumerable Objects => InternalChildren.Cast().OrderBy(h => h.HitObject.StartTime); public IEnumerable AliveObjects => AliveInternalChildren.Cast().OrderBy(h => h.HitObject.StartTime); - private readonly Dictionary bindable, double timeAtAdd)> startTimeMap = new Dictionary, double)>(); + private readonly Dictionary startTimeMap = new Dictionary(); public HitObjectContainer() { @@ -25,10 +25,7 @@ namespace osu.Game.Rulesets.UI public virtual void Add(DrawableHitObject hitObject) { - // Added first for the comparer to remain ordered during AddInternal - startTimeMap[hitObject] = (hitObject.HitObject.StartTimeBindable.GetBoundCopy(), hitObject.HitObject.StartTime); - startTimeMap[hitObject].bindable.BindValueChanged(_ => onStartTimeChanged(hitObject)); - + bindStartTime(hitObject); AddInternal(hitObject); } @@ -37,54 +34,19 @@ namespace osu.Game.Rulesets.UI if (!RemoveInternal(hitObject)) return false; - // Removed last for the comparer to remain ordered during RemoveInternal - startTimeMap[hitObject].bindable.UnbindAll(); - startTimeMap.Remove(hitObject); + unbindStartTime(hitObject); return true; } - protected override void Dispose(bool isDisposing) - { - base.Dispose(isDisposing); - unbindStartTimeMap(); - } - public virtual void Clear(bool disposeChildren = true) { ClearInternal(disposeChildren); - unbindStartTimeMap(); - } - - private void unbindStartTimeMap() - { - foreach (var kvp in startTimeMap) - kvp.Value.bindable.UnbindAll(); - startTimeMap.Clear(); + unbindAllStartTimes(); } public int IndexOf(DrawableHitObject hitObject) => IndexOfInternal(hitObject); - private void onStartTimeChanged(DrawableHitObject hitObject) - { - if (!RemoveInternal(hitObject)) - return; - - // Update the stored time, preserving the existing bindable - startTimeMap[hitObject] = (startTimeMap[hitObject].bindable, hitObject.HitObject.StartTime); - AddInternal(hitObject); - } - - protected override int Compare(Drawable x, Drawable y) - { - if (!(x is DrawableHitObject xObj) || !(y is DrawableHitObject yObj)) - return base.Compare(x, y); - - // Put earlier hitobjects towards the end of the list, so they handle input first - int i = startTimeMap[yObj].timeAtAdd.CompareTo(startTimeMap[xObj].timeAtAdd); - return i == 0 ? CompareReverseChildID(x, y) : i; - } - protected override void OnChildLifetimeBoundaryCrossed(LifetimeBoundaryCrossedEvent e) { if (!(e.Child is DrawableHitObject hitObject)) @@ -96,5 +58,48 @@ namespace osu.Game.Rulesets.UI hitObject.OnKilled(); } } + + #region Comparator + StartTime tracking + + private void bindStartTime(DrawableHitObject hitObject) + { + var bindable = hitObject.StartTimeBindable.GetBoundCopy(); + bindable.BindValueChanged(_ => onStartTimeChanged(hitObject)); + + startTimeMap[hitObject] = bindable; + } + + private void unbindStartTime(DrawableHitObject hitObject) + { + startTimeMap[hitObject].UnbindAll(); + startTimeMap.Remove(hitObject); + } + + private void unbindAllStartTimes() + { + foreach (var kvp in startTimeMap) + kvp.Value.UnbindAll(); + startTimeMap.Clear(); + } + + private void onStartTimeChanged(DrawableHitObject hitObject) => SortInternal(); + + protected override int Compare(Drawable x, Drawable y) + { + if (!(x is DrawableHitObject xObj) || !(y is DrawableHitObject yObj)) + return base.Compare(x, y); + + // Put earlier hitobjects towards the end of the list, so they handle input first + int i = yObj.HitObject.StartTime.CompareTo(xObj.HitObject.StartTime); + return i == 0 ? CompareReverseChildID(x, y) : i; + } + + #endregion + + protected override void Dispose(bool isDisposing) + { + base.Dispose(isDisposing); + unbindAllStartTimes(); + } } } diff --git a/osu.Game/Skinning/SkinReloadableDrawable.cs b/osu.Game/Skinning/SkinReloadableDrawable.cs index d0531c46c5..cc9cbf7b59 100644 --- a/osu.Game/Skinning/SkinReloadableDrawable.cs +++ b/osu.Game/Skinning/SkinReloadableDrawable.cs @@ -3,7 +3,6 @@ using System; using osu.Framework.Allocation; -using osu.Framework.Graphics.Containers; using osu.Framework.Graphics.Pooling; namespace osu.Game.Skinning From 2d892c74079cc5b379ab5aabd750fce127907fe0 Mon Sep 17 00:00:00 2001 From: smoogipoo Date: Fri, 6 Nov 2020 23:03:29 +0900 Subject: [PATCH 03/19] Allow Apply() to be called multiple times sequentially --- osu.Game/Rulesets/Objects/Drawables/DrawableHitObject.cs | 3 +++ 1 file changed, 3 insertions(+) diff --git a/osu.Game/Rulesets/Objects/Drawables/DrawableHitObject.cs b/osu.Game/Rulesets/Objects/Drawables/DrawableHitObject.cs index 43688a7332..1e304f3d7d 100644 --- a/osu.Game/Rulesets/Objects/Drawables/DrawableHitObject.cs +++ b/osu.Game/Rulesets/Objects/Drawables/DrawableHitObject.cs @@ -191,6 +191,9 @@ namespace osu.Game.Rulesets.Objects.Drawables /// public virtual void Apply(HitObject hitObject) { + if (HitObject != null) + FreeAfterUse(); + HitObject = hitObject; // Copy any existing result from the hitobject (required for rewind / judgement revert). From 7eceda242bf40887d340d0388e2886e3fe6082e7 Mon Sep 17 00:00:00 2001 From: smoogipoo Date: Fri, 6 Nov 2020 23:04:28 +0900 Subject: [PATCH 04/19] Change derived class to use property --- osu.Game/Rulesets/Objects/Drawables/DrawableHitObject.cs | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/osu.Game/Rulesets/Objects/Drawables/DrawableHitObject.cs b/osu.Game/Rulesets/Objects/Drawables/DrawableHitObject.cs index 1e304f3d7d..6e2967062c 100644 --- a/osu.Game/Rulesets/Objects/Drawables/DrawableHitObject.cs +++ b/osu.Game/Rulesets/Objects/Drawables/DrawableHitObject.cs @@ -671,12 +671,11 @@ namespace osu.Game.Rulesets.Objects.Drawables public abstract class DrawableHitObject : DrawableHitObject where TObject : HitObject { - public new readonly TObject HitObject; + public new TObject HitObject => (TObject)base.HitObject; protected DrawableHitObject(TObject hitObject) : base(hitObject) { - HitObject = hitObject; } } } From 33b629a87a86b571d3cc94ffaa02949d13bfd7f1 Mon Sep 17 00:00:00 2001 From: smoogipoo Date: Fri, 6 Nov 2020 23:09:23 +0900 Subject: [PATCH 05/19] Make top-level osu! objects use new methods --- .../Objects/Drawables/DrawableHitCircle.cs | 3 ++- .../Objects/Drawables/DrawableOsuHitObject.cs | 16 +++++++++++++ .../Objects/Drawables/DrawableSlider.cs | 24 +++++++++++++++---- .../Objects/Drawables/DrawableSpinner.cs | 3 ++- 4 files changed, 40 insertions(+), 6 deletions(-) diff --git a/osu.Game.Rulesets.Osu/Objects/Drawables/DrawableHitCircle.cs b/osu.Game.Rulesets.Osu/Objects/Drawables/DrawableHitCircle.cs index b0c4e3758d..90a25e9625 100644 --- a/osu.Game.Rulesets.Osu/Objects/Drawables/DrawableHitCircle.cs +++ b/osu.Game.Rulesets.Osu/Objects/Drawables/DrawableHitCircle.cs @@ -3,6 +3,7 @@ using System; using System.Diagnostics; +using JetBrains.Annotations; using osu.Framework.Allocation; using osu.Framework.Graphics; using osu.Framework.Graphics.Containers; @@ -30,7 +31,7 @@ namespace osu.Game.Rulesets.Osu.Objects.Drawables private Container scaleContainer; private InputManager inputManager; - public DrawableHitCircle(HitCircle h) + public DrawableHitCircle([CanBeNull] HitCircle h = null) : base(h) { } diff --git a/osu.Game.Rulesets.Osu/Objects/Drawables/DrawableOsuHitObject.cs b/osu.Game.Rulesets.Osu/Objects/Drawables/DrawableOsuHitObject.cs index 208f79f165..2d0b939e30 100644 --- a/osu.Game.Rulesets.Osu/Objects/Drawables/DrawableOsuHitObject.cs +++ b/osu.Game.Rulesets.Osu/Objects/Drawables/DrawableOsuHitObject.cs @@ -9,6 +9,7 @@ using osu.Framework.Graphics; using osu.Game.Rulesets.Judgements; using osu.Game.Rulesets.Osu.Judgements; using osu.Game.Graphics.Containers; +using osu.Game.Rulesets.Objects; using osu.Game.Rulesets.Osu.UI; using osuTK; @@ -49,6 +50,21 @@ namespace osu.Game.Rulesets.Osu.Objects.Drawables ShakeDuration = 30, RelativeSizeAxes = Axes.Both }); + } + + protected override void FreeAfterUse() + { + IndexInCurrentComboBindable.UnbindFrom(HitObject.IndexInCurrentComboBindable); + PositionBindable.UnbindFrom(HitObject.PositionBindable); + StackHeightBindable.UnbindFrom(HitObject.StackHeightBindable); + ScaleBindable.UnbindFrom(HitObject.ScaleBindable); + + base.FreeAfterUse(); + } + + public override void Apply(HitObject hitObject) + { + base.Apply(hitObject); IndexInCurrentComboBindable.BindTo(HitObject.IndexInCurrentComboBindable); PositionBindable.BindTo(HitObject.PositionBindable); diff --git a/osu.Game.Rulesets.Osu/Objects/Drawables/DrawableSlider.cs b/osu.Game.Rulesets.Osu/Objects/Drawables/DrawableSlider.cs index d8dd0d7471..5a47e3bebe 100644 --- a/osu.Game.Rulesets.Osu/Objects/Drawables/DrawableSlider.cs +++ b/osu.Game.Rulesets.Osu/Objects/Drawables/DrawableSlider.cs @@ -3,6 +3,7 @@ using System; using System.Linq; +using JetBrains.Annotations; using osuTK; using osu.Framework.Graphics; using osu.Game.Rulesets.Objects.Drawables; @@ -32,14 +33,15 @@ namespace osu.Game.Rulesets.Osu.Objects.Drawables private PlaySliderBody sliderBody => Body.Drawable as PlaySliderBody; - public readonly IBindable PathVersion = new Bindable(); + public IBindable PathVersion => pathVersion; + private readonly Bindable pathVersion = new Bindable(); private Container headContainer; private Container tailContainer; private Container tickContainer; private Container repeatContainer; - public DrawableSlider(Slider s) + public DrawableSlider([CanBeNull] Slider s = null) : base(s) { } @@ -63,8 +65,6 @@ namespace osu.Game.Rulesets.Osu.Objects.Drawables headContainer = new Container { RelativeSizeAxes = Axes.Both }, }; - PathVersion.BindTo(HitObject.Path.Version); - PositionBindable.BindValueChanged(_ => Position = HitObject.StackedPosition, true); StackHeightBindable.BindValueChanged(_ => Position = HitObject.StackedPosition, true); ScaleBindable.BindValueChanged(scale => Ball.Scale = new Vector2(scale.NewValue), true); @@ -78,6 +78,22 @@ namespace osu.Game.Rulesets.Osu.Objects.Drawables Tracking.BindValueChanged(updateSlidingSample); } + protected override void FreeAfterUse() + { + PathVersion.UnbindFrom(HitObject.Path.Version); + + base.FreeAfterUse(); + } + + public override void Apply(HitObject hitObject) + { + base.Apply(hitObject); + + // Ensure that the version will change after the upcoming BindTo(). + pathVersion.Value = int.MaxValue; + PathVersion.BindTo(HitObject.Path.Version); + } + private PausableSkinnableSound slidingSample; protected override void LoadSamples() diff --git a/osu.Game.Rulesets.Osu/Objects/Drawables/DrawableSpinner.cs b/osu.Game.Rulesets.Osu/Objects/Drawables/DrawableSpinner.cs index 77fbff9c51..d564a76e5e 100644 --- a/osu.Game.Rulesets.Osu/Objects/Drawables/DrawableSpinner.cs +++ b/osu.Game.Rulesets.Osu/Objects/Drawables/DrawableSpinner.cs @@ -3,6 +3,7 @@ using System; using System.Linq; +using JetBrains.Annotations; using osu.Framework.Allocation; using osu.Framework.Audio; using osu.Framework.Bindables; @@ -32,7 +33,7 @@ namespace osu.Game.Rulesets.Osu.Objects.Drawables private Bindable isSpinning; private bool spinnerFrequencyModulate; - public DrawableSpinner(Spinner s) + public DrawableSpinner([CanBeNull] Spinner s = null) : base(s) { } From e190afbfed30d6ad0d12b289209d8173f92a5985 Mon Sep 17 00:00:00 2001 From: smoogipoo Date: Fri, 6 Nov 2020 23:35:47 +0900 Subject: [PATCH 06/19] Remove initial value changed invocations --- .../Objects/Drawables/DrawableHitCircle.cs | 8 ++++---- osu.Game.Rulesets.Osu/Objects/Drawables/DrawableSlider.cs | 6 +++--- .../Objects/Drawables/DrawableSpinner.cs | 2 +- 3 files changed, 8 insertions(+), 8 deletions(-) diff --git a/osu.Game.Rulesets.Osu/Objects/Drawables/DrawableHitCircle.cs b/osu.Game.Rulesets.Osu/Objects/Drawables/DrawableHitCircle.cs index 90a25e9625..77d24db084 100644 --- a/osu.Game.Rulesets.Osu/Objects/Drawables/DrawableHitCircle.cs +++ b/osu.Game.Rulesets.Osu/Objects/Drawables/DrawableHitCircle.cs @@ -73,10 +73,10 @@ namespace osu.Game.Rulesets.Osu.Objects.Drawables Size = HitArea.DrawSize; - PositionBindable.BindValueChanged(_ => Position = HitObject.StackedPosition, true); - StackHeightBindable.BindValueChanged(_ => Position = HitObject.StackedPosition, true); - ScaleBindable.BindValueChanged(scale => scaleContainer.Scale = new Vector2(scale.NewValue), true); - AccentColour.BindValueChanged(accent => ApproachCircle.Colour = accent.NewValue, true); + PositionBindable.BindValueChanged(_ => Position = HitObject.StackedPosition); + StackHeightBindable.BindValueChanged(_ => Position = HitObject.StackedPosition); + ScaleBindable.BindValueChanged(scale => scaleContainer.Scale = new Vector2(scale.NewValue)); + AccentColour.BindValueChanged(accent => ApproachCircle.Colour = accent.NewValue); } protected override void LoadComplete() diff --git a/osu.Game.Rulesets.Osu/Objects/Drawables/DrawableSlider.cs b/osu.Game.Rulesets.Osu/Objects/Drawables/DrawableSlider.cs index 5a47e3bebe..72b932ea36 100644 --- a/osu.Game.Rulesets.Osu/Objects/Drawables/DrawableSlider.cs +++ b/osu.Game.Rulesets.Osu/Objects/Drawables/DrawableSlider.cs @@ -65,9 +65,9 @@ namespace osu.Game.Rulesets.Osu.Objects.Drawables headContainer = new Container { RelativeSizeAxes = Axes.Both }, }; - PositionBindable.BindValueChanged(_ => Position = HitObject.StackedPosition, true); - StackHeightBindable.BindValueChanged(_ => Position = HitObject.StackedPosition, true); - ScaleBindable.BindValueChanged(scale => Ball.Scale = new Vector2(scale.NewValue), true); + PositionBindable.BindValueChanged(_ => Position = HitObject.StackedPosition); + StackHeightBindable.BindValueChanged(_ => Position = HitObject.StackedPosition); + ScaleBindable.BindValueChanged(scale => Ball.Scale = new Vector2(scale.NewValue)); AccentColour.BindValueChanged(colour => { diff --git a/osu.Game.Rulesets.Osu/Objects/Drawables/DrawableSpinner.cs b/osu.Game.Rulesets.Osu/Objects/Drawables/DrawableSpinner.cs index d564a76e5e..eb125969b0 100644 --- a/osu.Game.Rulesets.Osu/Objects/Drawables/DrawableSpinner.cs +++ b/osu.Game.Rulesets.Osu/Objects/Drawables/DrawableSpinner.cs @@ -73,7 +73,7 @@ namespace osu.Game.Rulesets.Osu.Objects.Drawables } }; - PositionBindable.BindValueChanged(pos => Position = pos.NewValue, true); + PositionBindable.BindValueChanged(pos => Position = pos.NewValue); } protected override void LoadComplete() From 1c8d68676eeaa0d93eff7bd5a99d573e7a05bc63 Mon Sep 17 00:00:00 2001 From: smoogipoo Date: Fri, 6 Nov 2020 23:56:02 +0900 Subject: [PATCH 07/19] Add tests --- .../TestSceneHitCircleApplication.cs | 44 +++++++++++++ .../TestSceneSliderApplication.cs | 61 +++++++++++++++++++ .../TestSceneSpinnerApplication.cs | 46 ++++++++++++++ 3 files changed, 151 insertions(+) create mode 100644 osu.Game.Rulesets.Osu.Tests/TestSceneHitCircleApplication.cs create mode 100644 osu.Game.Rulesets.Osu.Tests/TestSceneSliderApplication.cs create mode 100644 osu.Game.Rulesets.Osu.Tests/TestSceneSpinnerApplication.cs diff --git a/osu.Game.Rulesets.Osu.Tests/TestSceneHitCircleApplication.cs b/osu.Game.Rulesets.Osu.Tests/TestSceneHitCircleApplication.cs new file mode 100644 index 0000000000..8b3fead366 --- /dev/null +++ b/osu.Game.Rulesets.Osu.Tests/TestSceneHitCircleApplication.cs @@ -0,0 +1,44 @@ +// Copyright (c) ppy Pty Ltd . Licensed under the MIT Licence. +// See the LICENCE file in the repository root for full licence text. + +using NUnit.Framework; +using osu.Framework.Timing; +using osu.Game.Beatmaps; +using osu.Game.Beatmaps.ControlPoints; +using osu.Game.Rulesets.Osu.Objects; +using osu.Game.Rulesets.Osu.Objects.Drawables; +using osu.Game.Tests.Visual; +using osuTK; + +namespace osu.Game.Rulesets.Osu.Tests +{ + public class TestSceneHitCircleApplication : OsuTestScene + { + [Test] + public void TestApplyNewCircle() + { + DrawableHitCircle dho = null; + + AddStep("create circle", () => Child = dho = new DrawableHitCircle(prepareObject(new HitCircle + { + Position = new Vector2(256, 192), + IndexInCurrentCombo = 0 + })) + { + Clock = new FramedClock(new StopwatchClock()) + }); + + AddStep("apply new circle", () => dho.Apply(prepareObject(new HitCircle + { + Position = new Vector2(128, 128), + ComboIndex = 1, + }))); + } + + private HitCircle prepareObject(HitCircle circle) + { + circle.ApplyDefaults(new ControlPointInfo(), new BeatmapDifficulty()); + return circle; + } + } +} diff --git a/osu.Game.Rulesets.Osu.Tests/TestSceneSliderApplication.cs b/osu.Game.Rulesets.Osu.Tests/TestSceneSliderApplication.cs new file mode 100644 index 0000000000..e0898dc95d --- /dev/null +++ b/osu.Game.Rulesets.Osu.Tests/TestSceneSliderApplication.cs @@ -0,0 +1,61 @@ +// Copyright (c) ppy Pty Ltd . Licensed under the MIT Licence. +// See the LICENCE file in the repository root for full licence text. + +using NUnit.Framework; +using osu.Framework.Timing; +using osu.Game.Beatmaps; +using osu.Game.Beatmaps.ControlPoints; +using osu.Game.Rulesets.Objects; +using osu.Game.Rulesets.Objects.Types; +using osu.Game.Rulesets.Osu.Objects; +using osu.Game.Rulesets.Osu.Objects.Drawables; +using osu.Game.Tests.Visual; +using osuTK; + +namespace osu.Game.Rulesets.Osu.Tests +{ + public class TestSceneSliderApplication : OsuTestScene + { + [Test] + public void TestApplyNewSlider() + { + DrawableSlider dho = null; + + AddStep("create slider", () => Child = dho = new DrawableSlider(prepareObject(new Slider + { + Position = new Vector2(256, 192), + IndexInCurrentCombo = 0, + Path = new SliderPath(PathType.Linear, new[] + { + Vector2.Zero, + new Vector2(150, 100), + new Vector2(300, 0), + }) + })) + { + Clock = new FramedClock(new StopwatchClock(true)) + }); + + AddWaitStep("wait for progression", 1); + + AddStep("apply new slider", () => dho.Apply(prepareObject(new Slider + { + Position = new Vector2(256, 192), + ComboIndex = 1, + Path = new SliderPath(PathType.Bezier, new[] + { + Vector2.Zero, + new Vector2(150, 100), + new Vector2(300, 0), + }), + RepeatCount = 1 + }))); + } + + private Slider prepareObject(Slider slider) + { + slider.ApplyDefaults(new ControlPointInfo(), new BeatmapDifficulty()); + return slider; + } + } +} diff --git a/osu.Game.Rulesets.Osu.Tests/TestSceneSpinnerApplication.cs b/osu.Game.Rulesets.Osu.Tests/TestSceneSpinnerApplication.cs new file mode 100644 index 0000000000..5951574079 --- /dev/null +++ b/osu.Game.Rulesets.Osu.Tests/TestSceneSpinnerApplication.cs @@ -0,0 +1,46 @@ +// Copyright (c) ppy Pty Ltd . Licensed under the MIT Licence. +// See the LICENCE file in the repository root for full licence text. + +using NUnit.Framework; +using osu.Framework.Timing; +using osu.Game.Beatmaps; +using osu.Game.Beatmaps.ControlPoints; +using osu.Game.Rulesets.Osu.Objects; +using osu.Game.Rulesets.Osu.Objects.Drawables; +using osu.Game.Tests.Visual; +using osuTK; + +namespace osu.Game.Rulesets.Osu.Tests +{ + public class TestSceneSpinnerApplication : OsuTestScene + { + [Test] + public void TestApplyNewCircle() + { + DrawableSpinner dho = null; + + AddStep("create spinner", () => Child = dho = new DrawableSpinner(prepareObject(new Spinner + { + Position = new Vector2(256, 192), + IndexInCurrentCombo = 0, + Duration = 0, + })) + { + Clock = new FramedClock(new StopwatchClock()) + }); + + AddStep("apply new spinner", () => dho.Apply(prepareObject(new Spinner + { + Position = new Vector2(256, 192), + ComboIndex = 1, + Duration = 1000, + }))); + } + + private Spinner prepareObject(Spinner circle) + { + circle.ApplyDefaults(new ControlPointInfo(), new BeatmapDifficulty()); + return circle; + } + } +} From 3a4bd73823c9e884de908591d8784a63eae69bbc Mon Sep 17 00:00:00 2001 From: smoogipoo Date: Sat, 7 Nov 2020 00:25:26 +0900 Subject: [PATCH 08/19] Fix DHOs being freed when not expected --- .../Objects/Drawables/DrawableOsuHitObject.cs | 4 +- .../Objects/Drawables/DrawableSlider.cs | 4 +- .../Objects/Drawables/DrawableHitObject.cs | 95 +++++++++++-------- 3 files changed, 60 insertions(+), 43 deletions(-) diff --git a/osu.Game.Rulesets.Osu/Objects/Drawables/DrawableOsuHitObject.cs b/osu.Game.Rulesets.Osu/Objects/Drawables/DrawableOsuHitObject.cs index 2d0b939e30..0326f38439 100644 --- a/osu.Game.Rulesets.Osu/Objects/Drawables/DrawableOsuHitObject.cs +++ b/osu.Game.Rulesets.Osu/Objects/Drawables/DrawableOsuHitObject.cs @@ -52,14 +52,14 @@ namespace osu.Game.Rulesets.Osu.Objects.Drawables }); } - protected override void FreeAfterUse() + public override void Free() { IndexInCurrentComboBindable.UnbindFrom(HitObject.IndexInCurrentComboBindable); PositionBindable.UnbindFrom(HitObject.PositionBindable); StackHeightBindable.UnbindFrom(HitObject.StackHeightBindable); ScaleBindable.UnbindFrom(HitObject.ScaleBindable); - base.FreeAfterUse(); + base.Free(); } public override void Apply(HitObject hitObject) diff --git a/osu.Game.Rulesets.Osu/Objects/Drawables/DrawableSlider.cs b/osu.Game.Rulesets.Osu/Objects/Drawables/DrawableSlider.cs index 72b932ea36..844e95aa7a 100644 --- a/osu.Game.Rulesets.Osu/Objects/Drawables/DrawableSlider.cs +++ b/osu.Game.Rulesets.Osu/Objects/Drawables/DrawableSlider.cs @@ -78,11 +78,11 @@ namespace osu.Game.Rulesets.Osu.Objects.Drawables Tracking.BindValueChanged(updateSlidingSample); } - protected override void FreeAfterUse() + public override void Free() { PathVersion.UnbindFrom(HitObject.Path.Version); - base.FreeAfterUse(); + base.Free(); } public override void Apply(HitObject hitObject) diff --git a/osu.Game/Rulesets/Objects/Drawables/DrawableHitObject.cs b/osu.Game/Rulesets/Objects/Drawables/DrawableHitObject.cs index 6e2967062c..8e56b07420 100644 --- a/osu.Game/Rulesets/Objects/Drawables/DrawableHitObject.cs +++ b/osu.Game/Rulesets/Objects/Drawables/DrawableHitObject.cs @@ -115,6 +115,11 @@ namespace osu.Game.Rulesets.Objects.Drawables public IBindable State => state; + /// + /// Whether is currently applied. + /// + private bool hasHitObjectApplied; + /// /// Creates a new . /// @@ -151,50 +156,16 @@ namespace osu.Game.Rulesets.Objects.Drawables updateState(ArmedState.Idle, true); } - /// - /// Removes the currently applied to this , - /// - protected override void FreeAfterUse() - { - StartTimeBindable.UnbindFrom(HitObject.StartTimeBindable); - if (HitObject is IHasComboInformation combo) - comboIndexBindable.UnbindFrom(combo.ComboIndexBindable); - - samplesBindable.UnbindFrom(HitObject.SamplesBindable); - - // When a new hitobject is applied, the samples will be cleared before re-populating. - // In order to stop this needless update, the event is unbound and re-bound as late as possible in Apply(). - samplesBindable.CollectionChanged -= onSamplesChanged; - - if (nestedHitObjects.IsValueCreated) - { - foreach (var obj in nestedHitObjects.Value) - { - obj.OnNewResult -= onNewResult; - obj.OnRevertResult -= onRevertResult; - obj.ApplyCustomUpdateState -= onApplyCustomUpdateState; - } - - nestedHitObjects.Value.Clear(); - ClearNestedHitObjects(); - } - - HitObject.DefaultsApplied -= onDefaultsApplied; - HitObject = null; - - base.FreeAfterUse(); - } - /// /// Applies a new to be represented by this . /// - /// + /// The to apply. public virtual void Apply(HitObject hitObject) { - if (HitObject != null) - FreeAfterUse(); + if (hasHitObjectApplied) + Free(); - HitObject = hitObject; + HitObject = hitObject ?? throw new InvalidOperationException($"Cannot apply a null {nameof(HitObject)}."); // Copy any existing result from the hitobject (required for rewind / judgement revert). Result = HitObject.Result; @@ -230,6 +201,52 @@ namespace osu.Game.Rulesets.Objects.Drawables // If not loaded, the state update happens in LoadComplete(). Otherwise, the update is scheduled to allow for lifetime updates. if (IsLoaded) Schedule(() => updateState(ArmedState.Idle, true)); + + hasHitObjectApplied = true; + } + + /// + /// Removes the currently applied + /// + public virtual void Free() + { + StartTimeBindable.UnbindFrom(HitObject.StartTimeBindable); + if (HitObject is IHasComboInformation combo) + comboIndexBindable.UnbindFrom(combo.ComboIndexBindable); + + samplesBindable.UnbindFrom(HitObject.SamplesBindable); + + // When a new hitobject is applied, the samples will be cleared before re-populating. + // In order to stop this needless update, the event is unbound and re-bound as late as possible in Apply(). + samplesBindable.CollectionChanged -= onSamplesChanged; + + if (nestedHitObjects.IsValueCreated) + { + foreach (var obj in nestedHitObjects.Value) + { + obj.OnNewResult -= onNewResult; + obj.OnRevertResult -= onRevertResult; + obj.ApplyCustomUpdateState -= onApplyCustomUpdateState; + } + + nestedHitObjects.Value.Clear(); + ClearNestedHitObjects(); + } + + HitObject.DefaultsApplied -= onDefaultsApplied; + HitObject = null; + + hasHitObjectApplied = false; + } + + protected sealed override void FreeAfterUse() + { + base.FreeAfterUse(); + + if (!IsInPool) + return; + + Free(); } /// @@ -268,7 +285,7 @@ namespace osu.Game.Rulesets.Objects.Drawables private void onDefaultsApplied(HitObject hitObject) { - FreeAfterUse(); + Free(); Apply(hitObject); DefaultsApplied?.Invoke(this); } From b1e039bcec1c45c66f7e622d20b1dbb8c104fea9 Mon Sep 17 00:00:00 2001 From: smoogipoo Date: Sat, 7 Nov 2020 00:40:26 +0900 Subject: [PATCH 09/19] Prevent overrides from messing with application/freeing --- .../Objects/Drawables/DrawableOsuHitObject.cs | 24 ++++++------- .../Objects/Drawables/DrawableSlider.cs | 18 +++++----- .../Objects/Drawables/DrawableHitObject.cs | 35 +++++++++++++++---- 3 files changed, 49 insertions(+), 28 deletions(-) diff --git a/osu.Game.Rulesets.Osu/Objects/Drawables/DrawableOsuHitObject.cs b/osu.Game.Rulesets.Osu/Objects/Drawables/DrawableOsuHitObject.cs index 0326f38439..d17bf93fa0 100644 --- a/osu.Game.Rulesets.Osu/Objects/Drawables/DrawableOsuHitObject.cs +++ b/osu.Game.Rulesets.Osu/Objects/Drawables/DrawableOsuHitObject.cs @@ -52,19 +52,9 @@ namespace osu.Game.Rulesets.Osu.Objects.Drawables }); } - public override void Free() + protected override void OnApply(HitObject hitObject) { - IndexInCurrentComboBindable.UnbindFrom(HitObject.IndexInCurrentComboBindable); - PositionBindable.UnbindFrom(HitObject.PositionBindable); - StackHeightBindable.UnbindFrom(HitObject.StackHeightBindable); - ScaleBindable.UnbindFrom(HitObject.ScaleBindable); - - base.Free(); - } - - public override void Apply(HitObject hitObject) - { - base.Apply(hitObject); + base.OnApply(hitObject); IndexInCurrentComboBindable.BindTo(HitObject.IndexInCurrentComboBindable); PositionBindable.BindTo(HitObject.PositionBindable); @@ -72,6 +62,16 @@ namespace osu.Game.Rulesets.Osu.Objects.Drawables ScaleBindable.BindTo(HitObject.ScaleBindable); } + protected override void OnFree(HitObject hitObject) + { + base.OnFree(hitObject); + + IndexInCurrentComboBindable.UnbindFrom(HitObject.IndexInCurrentComboBindable); + PositionBindable.UnbindFrom(HitObject.PositionBindable); + StackHeightBindable.UnbindFrom(HitObject.StackHeightBindable); + ScaleBindable.UnbindFrom(HitObject.ScaleBindable); + } + // Forward all internal management to shakeContainer. // This is a bit ugly but we don't have the concept of InternalContent so it'll have to do for now. (https://github.com/ppy/osu-framework/issues/1690) protected override void AddInternal(Drawable drawable) => shakeContainer.Add(drawable); diff --git a/osu.Game.Rulesets.Osu/Objects/Drawables/DrawableSlider.cs b/osu.Game.Rulesets.Osu/Objects/Drawables/DrawableSlider.cs index 844e95aa7a..3f91a31066 100644 --- a/osu.Game.Rulesets.Osu/Objects/Drawables/DrawableSlider.cs +++ b/osu.Game.Rulesets.Osu/Objects/Drawables/DrawableSlider.cs @@ -78,22 +78,22 @@ namespace osu.Game.Rulesets.Osu.Objects.Drawables Tracking.BindValueChanged(updateSlidingSample); } - public override void Free() + protected override void OnApply(HitObject hitObject) { - PathVersion.UnbindFrom(HitObject.Path.Version); - - base.Free(); - } - - public override void Apply(HitObject hitObject) - { - base.Apply(hitObject); + base.OnApply(hitObject); // Ensure that the version will change after the upcoming BindTo(). pathVersion.Value = int.MaxValue; PathVersion.BindTo(HitObject.Path.Version); } + protected override void OnFree(HitObject hitObject) + { + base.OnFree(hitObject); + + PathVersion.UnbindFrom(HitObject.Path.Version); + } + private PausableSkinnableSound slidingSample; protected override void LoadSamples() diff --git a/osu.Game/Rulesets/Objects/Drawables/DrawableHitObject.cs b/osu.Game/Rulesets/Objects/Drawables/DrawableHitObject.cs index 8e56b07420..ecd345f1a9 100644 --- a/osu.Game/Rulesets/Objects/Drawables/DrawableHitObject.cs +++ b/osu.Game/Rulesets/Objects/Drawables/DrawableHitObject.cs @@ -160,10 +160,9 @@ namespace osu.Game.Rulesets.Objects.Drawables /// Applies a new to be represented by this . /// /// The to apply. - public virtual void Apply(HitObject hitObject) + public void Apply(HitObject hitObject) { - if (hasHitObjectApplied) - Free(); + free(); HitObject = hitObject ?? throw new InvalidOperationException($"Cannot apply a null {nameof(HitObject)}."); @@ -202,14 +201,18 @@ namespace osu.Game.Rulesets.Objects.Drawables if (IsLoaded) Schedule(() => updateState(ArmedState.Idle, true)); + OnApply(hitObject); hasHitObjectApplied = true; } /// /// Removes the currently applied /// - public virtual void Free() + private void free() { + if (!hasHitObjectApplied) + return; + StartTimeBindable.UnbindFrom(HitObject.StartTimeBindable); if (HitObject is IHasComboInformation combo) comboIndexBindable.UnbindFrom(combo.ComboIndexBindable); @@ -234,8 +237,10 @@ namespace osu.Game.Rulesets.Objects.Drawables } HitObject.DefaultsApplied -= onDefaultsApplied; - HitObject = null; + OnFree(HitObject); + + HitObject = null; hasHitObjectApplied = false; } @@ -243,10 +248,27 @@ namespace osu.Game.Rulesets.Objects.Drawables { base.FreeAfterUse(); + // Freeing while not in a pool would cause the DHO to not be usable elsewhere in the hierarchy without being re-applied. if (!IsInPool) return; - Free(); + free(); + } + + /// + /// Invoked for this to take on any values from a newly-applied . + /// + /// The being applied. + protected virtual void OnApply(HitObject hitObject) + { + } + + /// + /// Invoked for this to revert any values previously taken on from the currently-applied . + /// + /// The currently-applied . + protected virtual void OnFree(HitObject hitObject) + { } /// @@ -285,7 +307,6 @@ namespace osu.Game.Rulesets.Objects.Drawables private void onDefaultsApplied(HitObject hitObject) { - Free(); Apply(hitObject); DefaultsApplied?.Invoke(this); } From 4a07a7e757ce283e2445abe4522cfb80bdad6bc6 Mon Sep 17 00:00:00 2001 From: smoogipoo Date: Sat, 7 Nov 2020 00:40:41 +0900 Subject: [PATCH 10/19] Refactor test --- osu.Game.Rulesets.Osu.Tests/TestSceneSliderApplication.cs | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/osu.Game.Rulesets.Osu.Tests/TestSceneSliderApplication.cs b/osu.Game.Rulesets.Osu.Tests/TestSceneSliderApplication.cs index e0898dc95d..f76c7e2a3e 100644 --- a/osu.Game.Rulesets.Osu.Tests/TestSceneSliderApplication.cs +++ b/osu.Game.Rulesets.Osu.Tests/TestSceneSliderApplication.cs @@ -2,7 +2,6 @@ // See the LICENCE file in the repository root for full licence text. using NUnit.Framework; -using osu.Framework.Timing; using osu.Game.Beatmaps; using osu.Game.Beatmaps.ControlPoints; using osu.Game.Rulesets.Objects; @@ -25,16 +24,14 @@ namespace osu.Game.Rulesets.Osu.Tests { Position = new Vector2(256, 192), IndexInCurrentCombo = 0, + StartTime = Time.Current, Path = new SliderPath(PathType.Linear, new[] { Vector2.Zero, new Vector2(150, 100), new Vector2(300, 0), }) - })) - { - Clock = new FramedClock(new StopwatchClock(true)) - }); + }))); AddWaitStep("wait for progression", 1); @@ -42,6 +39,7 @@ namespace osu.Game.Rulesets.Osu.Tests { Position = new Vector2(256, 192), ComboIndex = 1, + StartTime = dho.HitObject.StartTime, Path = new SliderPath(PathType.Bezier, new[] { Vector2.Zero, From 91c627c22d2db9001668bfc51a87ae46849db59b Mon Sep 17 00:00:00 2001 From: smoogipoo Date: Sat, 7 Nov 2020 00:57:33 +0900 Subject: [PATCH 11/19] Revert HOC changes --- osu.Game/Rulesets/UI/HitObjectContainer.cs | 89 ++++++++++------------ 1 file changed, 42 insertions(+), 47 deletions(-) diff --git a/osu.Game/Rulesets/UI/HitObjectContainer.cs b/osu.Game/Rulesets/UI/HitObjectContainer.cs index 4666aa9211..4cadfa9ad4 100644 --- a/osu.Game/Rulesets/UI/HitObjectContainer.cs +++ b/osu.Game/Rulesets/UI/HitObjectContainer.cs @@ -16,7 +16,7 @@ namespace osu.Game.Rulesets.UI public IEnumerable Objects => InternalChildren.Cast().OrderBy(h => h.HitObject.StartTime); public IEnumerable AliveObjects => AliveInternalChildren.Cast().OrderBy(h => h.HitObject.StartTime); - private readonly Dictionary startTimeMap = new Dictionary(); + private readonly Dictionary bindable, double timeAtAdd)> startTimeMap = new Dictionary, double)>(); public HitObjectContainer() { @@ -25,7 +25,10 @@ namespace osu.Game.Rulesets.UI public virtual void Add(DrawableHitObject hitObject) { - bindStartTime(hitObject); + // Added first for the comparer to remain ordered during AddInternal + startTimeMap[hitObject] = (hitObject.HitObject.StartTimeBindable.GetBoundCopy(), hitObject.HitObject.StartTime); + startTimeMap[hitObject].bindable.BindValueChanged(_ => onStartTimeChanged(hitObject)); + AddInternal(hitObject); } @@ -34,19 +37,54 @@ namespace osu.Game.Rulesets.UI if (!RemoveInternal(hitObject)) return false; - unbindStartTime(hitObject); + // Removed last for the comparer to remain ordered during RemoveInternal + startTimeMap[hitObject].bindable.UnbindAll(); + startTimeMap.Remove(hitObject); return true; } + protected override void Dispose(bool isDisposing) + { + base.Dispose(isDisposing); + unbindStartTimeMap(); + } + public virtual void Clear(bool disposeChildren = true) { ClearInternal(disposeChildren); - unbindAllStartTimes(); + unbindStartTimeMap(); + } + + private void unbindStartTimeMap() + { + foreach (var kvp in startTimeMap) + kvp.Value.bindable.UnbindAll(); + startTimeMap.Clear(); } public int IndexOf(DrawableHitObject hitObject) => IndexOfInternal(hitObject); + private void onStartTimeChanged(DrawableHitObject hitObject) + { + if (!RemoveInternal(hitObject)) + return; + + // Update the stored time, preserving the existing bindable + startTimeMap[hitObject] = (startTimeMap[hitObject].bindable, hitObject.HitObject.StartTime); + AddInternal(hitObject); + } + + protected override int Compare(Drawable x, Drawable y) + { + if (!(x is DrawableHitObject xObj) || !(y is DrawableHitObject yObj)) + return base.Compare(x, y); + + // Put earlier hitobjects towards the end of the list, so they handle input first + int i = startTimeMap[yObj].timeAtAdd.CompareTo(startTimeMap[xObj].timeAtAdd); + return i == 0 ? CompareReverseChildID(x, y) : i; + } + protected override void OnChildLifetimeBoundaryCrossed(LifetimeBoundaryCrossedEvent e) { if (!(e.Child is DrawableHitObject hitObject)) @@ -58,48 +96,5 @@ namespace osu.Game.Rulesets.UI hitObject.OnKilled(); } } - - #region Comparator + StartTime tracking - - private void bindStartTime(DrawableHitObject hitObject) - { - var bindable = hitObject.StartTimeBindable.GetBoundCopy(); - bindable.BindValueChanged(_ => onStartTimeChanged(hitObject)); - - startTimeMap[hitObject] = bindable; - } - - private void unbindStartTime(DrawableHitObject hitObject) - { - startTimeMap[hitObject].UnbindAll(); - startTimeMap.Remove(hitObject); - } - - private void unbindAllStartTimes() - { - foreach (var kvp in startTimeMap) - kvp.Value.UnbindAll(); - startTimeMap.Clear(); - } - - private void onStartTimeChanged(DrawableHitObject hitObject) => SortInternal(); - - protected override int Compare(Drawable x, Drawable y) - { - if (!(x is DrawableHitObject xObj) || !(y is DrawableHitObject yObj)) - return base.Compare(x, y); - - // Put earlier hitobjects towards the end of the list, so they handle input first - int i = yObj.HitObject.StartTime.CompareTo(xObj.HitObject.StartTime); - return i == 0 ? CompareReverseChildID(x, y) : i; - } - - #endregion - - protected override void Dispose(bool isDisposing) - { - base.Dispose(isDisposing); - unbindAllStartTimes(); - } } } From 33c3b07101c4ca61c7496b89bb087b56d161142a Mon Sep 17 00:00:00 2001 From: Dan Balasescu Date: Mon, 9 Nov 2020 19:06:48 +0900 Subject: [PATCH 12/19] Fix events not being bound correctly MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Bartłomiej Dach --- osu.Game/Rulesets/Objects/Drawables/DrawableHitObject.cs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/osu.Game/Rulesets/Objects/Drawables/DrawableHitObject.cs b/osu.Game/Rulesets/Objects/Drawables/DrawableHitObject.cs index ecd345f1a9..6bb22018ec 100644 --- a/osu.Game/Rulesets/Objects/Drawables/DrawableHitObject.cs +++ b/osu.Game/Rulesets/Objects/Drawables/DrawableHitObject.cs @@ -180,9 +180,9 @@ namespace osu.Game.Rulesets.Objects.Drawables { var drawableNested = CreateNestedHitObject(h) ?? throw new InvalidOperationException($"{nameof(CreateNestedHitObject)} returned null for {h.GetType().ReadableName()}."); - drawableNested.OnNewResult += (d, r) => OnNewResult?.Invoke(d, r); - drawableNested.OnRevertResult += (d, r) => OnRevertResult?.Invoke(d, r); - drawableNested.ApplyCustomUpdateState += (d, j) => ApplyCustomUpdateState?.Invoke(d, j); + drawableNested.OnNewResult += onNewResult; + drawableNested.OnRevertResult += onRevertResult; + drawableNested.ApplyCustomUpdateState += onApplyCustomUpdateState; nestedHitObjects.Value.Add(drawableNested); AddNestedHitObject(drawableNested); From ec8b726ea80461024ad3f642b9c4301d00cf8109 Mon Sep 17 00:00:00 2001 From: smoogipoo Date: Mon, 9 Nov 2020 21:51:58 +0900 Subject: [PATCH 13/19] Re-privatise start time bindable --- osu.Game/Rulesets/Objects/Drawables/DrawableHitObject.cs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/osu.Game/Rulesets/Objects/Drawables/DrawableHitObject.cs b/osu.Game/Rulesets/Objects/Drawables/DrawableHitObject.cs index 6bb22018ec..58b24cb7b2 100644 --- a/osu.Game/Rulesets/Objects/Drawables/DrawableHitObject.cs +++ b/osu.Game/Rulesets/Objects/Drawables/DrawableHitObject.cs @@ -100,7 +100,7 @@ namespace osu.Game.Rulesets.Objects.Drawables /// protected virtual float SamplePlaybackPosition => 0.5f; - public readonly Bindable StartTimeBindable = new Bindable(); + private readonly Bindable startTimeBindable = new Bindable(); private readonly BindableList samplesBindable = new BindableList(); private readonly Bindable userPositionalHitSounds = new Bindable(); private readonly Bindable comboIndexBindable = new Bindable(); @@ -150,7 +150,7 @@ namespace osu.Game.Rulesets.Objects.Drawables { base.LoadComplete(); - StartTimeBindable.BindValueChanged(_ => updateState(State.Value, true)); + startTimeBindable.BindValueChanged(_ => updateState(State.Value, true)); comboIndexBindable.BindValueChanged(_ => updateComboColour(), true); updateState(ArmedState.Idle, true); @@ -188,7 +188,7 @@ namespace osu.Game.Rulesets.Objects.Drawables AddNestedHitObject(drawableNested); } - StartTimeBindable.BindTo(HitObject.StartTimeBindable); + startTimeBindable.BindTo(HitObject.StartTimeBindable); if (HitObject is IHasComboInformation combo) comboIndexBindable.BindTo(combo.ComboIndexBindable); @@ -213,7 +213,7 @@ namespace osu.Game.Rulesets.Objects.Drawables if (!hasHitObjectApplied) return; - StartTimeBindable.UnbindFrom(HitObject.StartTimeBindable); + startTimeBindable.UnbindFrom(HitObject.StartTimeBindable); if (HitObject is IHasComboInformation combo) comboIndexBindable.UnbindFrom(combo.ComboIndexBindable); From ac47399e6e0c57044d800f464a419f85a656f31d Mon Sep 17 00:00:00 2001 From: smoogipoo Date: Tue, 10 Nov 2020 00:30:23 +0900 Subject: [PATCH 14/19] Update state after OnApply() --- osu.Game/Rulesets/Objects/Drawables/DrawableHitObject.cs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/osu.Game/Rulesets/Objects/Drawables/DrawableHitObject.cs b/osu.Game/Rulesets/Objects/Drawables/DrawableHitObject.cs index 58b24cb7b2..77c4ea42df 100644 --- a/osu.Game/Rulesets/Objects/Drawables/DrawableHitObject.cs +++ b/osu.Game/Rulesets/Objects/Drawables/DrawableHitObject.cs @@ -197,11 +197,12 @@ namespace osu.Game.Rulesets.Objects.Drawables HitObject.DefaultsApplied += onDefaultsApplied; + OnApply(hitObject); + // If not loaded, the state update happens in LoadComplete(). Otherwise, the update is scheduled to allow for lifetime updates. if (IsLoaded) Schedule(() => updateState(ArmedState.Idle, true)); - OnApply(hitObject); hasHitObjectApplied = true; } From d4d3a6621e6f512ccfbcf378ab6500d8acc07db1 Mon Sep 17 00:00:00 2001 From: smoogipoo Date: Tue, 10 Nov 2020 01:30:25 +0900 Subject: [PATCH 15/19] Disable automatic lifetime management --- osu.Game/Rulesets/Objects/Drawables/DrawableHitObject.cs | 2 ++ 1 file changed, 2 insertions(+) diff --git a/osu.Game/Rulesets/Objects/Drawables/DrawableHitObject.cs b/osu.Game/Rulesets/Objects/Drawables/DrawableHitObject.cs index 77c4ea42df..05ef9b162f 100644 --- a/osu.Game/Rulesets/Objects/Drawables/DrawableHitObject.cs +++ b/osu.Game/Rulesets/Objects/Drawables/DrawableHitObject.cs @@ -109,6 +109,8 @@ namespace osu.Game.Rulesets.Objects.Drawables public override bool RemoveCompletedTransforms => false; protected override bool RequiresChildrenUpdate => true; + public override bool ResetLifetimeWhenAssigned => false; // DHOs do their own lifetime management. + public override bool IsPresent => base.IsPresent || (State.Value == ArmedState.Idle && Clock?.CurrentTime >= LifetimeStart); private readonly Bindable state = new Bindable(); From f5076fe3b8f2e46100c4c13a6e96996523345bc5 Mon Sep 17 00:00:00 2001 From: smoogipoo Date: Tue, 10 Nov 2020 18:15:11 +0900 Subject: [PATCH 16/19] Revert unnecessary change --- osu.Game/Rulesets/Objects/Drawables/DrawableHitObject.cs | 2 -- 1 file changed, 2 deletions(-) diff --git a/osu.Game/Rulesets/Objects/Drawables/DrawableHitObject.cs b/osu.Game/Rulesets/Objects/Drawables/DrawableHitObject.cs index 05ef9b162f..77c4ea42df 100644 --- a/osu.Game/Rulesets/Objects/Drawables/DrawableHitObject.cs +++ b/osu.Game/Rulesets/Objects/Drawables/DrawableHitObject.cs @@ -109,8 +109,6 @@ namespace osu.Game.Rulesets.Objects.Drawables public override bool RemoveCompletedTransforms => false; protected override bool RequiresChildrenUpdate => true; - public override bool ResetLifetimeWhenAssigned => false; // DHOs do their own lifetime management. - public override bool IsPresent => base.IsPresent || (State.Value == ArmedState.Idle && Clock?.CurrentTime >= LifetimeStart); private readonly Bindable state = new Bindable(); From 88112801eba65f5602bc8f5fe16d8c61390f3d79 Mon Sep 17 00:00:00 2001 From: smoogipoo Date: Tue, 10 Nov 2020 18:56:09 +0900 Subject: [PATCH 17/19] Remove result storage from hitobject --- osu.Game/Rulesets/Objects/Drawables/DrawableHitObject.cs | 6 ------ osu.Game/Rulesets/Objects/HitObject.cs | 6 ------ 2 files changed, 12 deletions(-) diff --git a/osu.Game/Rulesets/Objects/Drawables/DrawableHitObject.cs b/osu.Game/Rulesets/Objects/Drawables/DrawableHitObject.cs index 77c4ea42df..7a4e136553 100644 --- a/osu.Game/Rulesets/Objects/Drawables/DrawableHitObject.cs +++ b/osu.Game/Rulesets/Objects/Drawables/DrawableHitObject.cs @@ -166,16 +166,10 @@ namespace osu.Game.Rulesets.Objects.Drawables HitObject = hitObject ?? throw new InvalidOperationException($"Cannot apply a null {nameof(HitObject)}."); - // Copy any existing result from the hitobject (required for rewind / judgement revert). - Result = HitObject.Result; - // Ensure this DHO has a result. Result ??= CreateResult(HitObject.CreateJudgement()) ?? throw new InvalidOperationException($"{GetType().ReadableName()} must provide a {nameof(JudgementResult)} through {nameof(CreateResult)}."); - // Ensure the hitobject has a result. - HitObject.Result = Result; - foreach (var h in HitObject.NestedHitObjects) { var drawableNested = CreateNestedHitObject(h) ?? throw new InvalidOperationException($"{nameof(CreateNestedHitObject)} returned null for {h.GetType().ReadableName()}."); diff --git a/osu.Game/Rulesets/Objects/HitObject.cs b/osu.Game/Rulesets/Objects/HitObject.cs index 9ef3ff9c4a..826d411822 100644 --- a/osu.Game/Rulesets/Objects/HitObject.cs +++ b/osu.Game/Rulesets/Objects/HitObject.cs @@ -12,7 +12,6 @@ using osu.Game.Audio; using osu.Game.Beatmaps; using osu.Game.Beatmaps.ControlPoints; using osu.Game.Rulesets.Judgements; -using osu.Game.Rulesets.Objects.Drawables; using osu.Game.Rulesets.Objects.Types; using osu.Game.Rulesets.Scoring; @@ -169,11 +168,6 @@ namespace osu.Game.Rulesets.Objects /// [NotNull] protected virtual HitWindows CreateHitWindows() => new HitWindows(); - - /// - /// The result this was judged with. Used internally for rewinding within . - /// - internal JudgementResult Result; } public static class HitObjectExtensions From e1dcac4d8ba4feb73c0fc2088edccc368d2698f3 Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Tue, 10 Nov 2020 20:29:29 +0900 Subject: [PATCH 18/19] Update framework --- osu.Android.props | 2 +- osu.Game/osu.Game.csproj | 2 +- osu.iOS.props | 4 ++-- 3 files changed, 4 insertions(+), 4 deletions(-) diff --git a/osu.Android.props b/osu.Android.props index e3285222f8..bbe8426316 100644 --- a/osu.Android.props +++ b/osu.Android.props @@ -52,6 +52,6 @@ - + diff --git a/osu.Game/osu.Game.csproj b/osu.Game/osu.Game.csproj index 832722c729..8f0cc58594 100644 --- a/osu.Game/osu.Game.csproj +++ b/osu.Game/osu.Game.csproj @@ -26,7 +26,7 @@ - + diff --git a/osu.iOS.props b/osu.iOS.props index ad6dd2a0b5..f766e0ec03 100644 --- a/osu.iOS.props +++ b/osu.iOS.props @@ -70,7 +70,7 @@ - + @@ -88,7 +88,7 @@ - + From 4ea823e4dc85365822fcde88e4d8175ce1a6532d Mon Sep 17 00:00:00 2001 From: smoogipoo Date: Tue, 10 Nov 2020 22:02:33 +0900 Subject: [PATCH 19/19] Fix test failures --- osu.Game.Rulesets.Taiko.Tests/DrawableTestHit.cs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/osu.Game.Rulesets.Taiko.Tests/DrawableTestHit.cs b/osu.Game.Rulesets.Taiko.Tests/DrawableTestHit.cs index 4eeb4a1475..fb0917341e 100644 --- a/osu.Game.Rulesets.Taiko.Tests/DrawableTestHit.cs +++ b/osu.Game.Rulesets.Taiko.Tests/DrawableTestHit.cs @@ -1,7 +1,6 @@ // Copyright (c) ppy Pty Ltd . Licensed under the MIT Licence. // See the LICENCE file in the repository root for full licence text. -using osu.Framework.Allocation; using osu.Game.Beatmaps; using osu.Game.Beatmaps.ControlPoints; using osu.Game.Rulesets.Scoring; @@ -28,9 +27,10 @@ namespace osu.Game.Rulesets.Taiko.Tests // suppress locally to allow hiding the visuals wherever necessary. } - [BackgroundDependencyLoader] - private void load() + protected override void LoadComplete() { + base.LoadComplete(); + Result.Type = Type; }