Initial Apply()/FreeAfterUse() DHO implementation

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<DrawableHitObject> DefaultsApplied;
 
-        public readonly HitObject HitObject;
+        /// <summary>
+        /// The <see cref="HitObject"/> currently represented by this <see cref="DrawableHitObject"/>.
+        /// </summary>
+        public HitObject HitObject { get; private set; }
 
         /// <summary>
         /// The colour used for various elements of this DrawableHitObject.
@@ -96,10 +100,10 @@ namespace osu.Game.Rulesets.Objects.Drawables
         /// </remarks>
         protected virtual float SamplePlaybackPosition => 0.5f;
 
-        private BindableList<HitSampleInfo> samplesBindable;
-        private Bindable<double> startTimeBindable;
-        private Bindable<bool> userPositionalHitSounds;
-        private Bindable<int> comboIndexBindable;
+        public readonly Bindable<double> StartTimeBindable = new Bindable<double>();
+        private readonly BindableList<HitSampleInfo> samplesBindable = new BindableList<HitSampleInfo>();
+        private readonly Bindable<bool> userPositionalHitSounds = new Bindable<bool>();
+        private readonly Bindable<int> comboIndexBindable = new Bindable<int>();
 
         public override bool RemoveWhenNotAlive => false;
         public override bool RemoveCompletedTransforms => false;
@@ -111,52 +115,120 @@ namespace osu.Game.Rulesets.Objects.Drawables
 
         public IBindable<ArmedState> State => state;
 
-        protected DrawableHitObject([NotNull] HitObject hitObject)
+        /// <summary>
+        /// Creates a new <see cref="DrawableHitObject"/>.
+        /// </summary>
+        /// <param name="initialHitObject">
+        /// The <see cref="HitObject"/> to be initially applied to this <see cref="DrawableHitObject"/>.
+        /// If <c>null</c>, a hitobject is expected to be later applied via <see cref="Apply"/> (or automatically via pooling).
+        /// </param>
+        protected DrawableHitObject([CanBeNull] HitObject initialHitObject = null)
         {
-            HitObject = hitObject ?? throw new ArgumentNullException(nameof(hitObject));
+            HitObject = initialHitObject;
         }
 
         [BackgroundDependencyLoader]
         private void load(OsuConfigManager config)
         {
-            userPositionalHitSounds = config.GetBindable<bool>(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);
         }
 
+        /// <summary>
+        /// Removes the <see cref="HitObject"/> currently applied to this <see cref="DrawableHitObject"/>,
+        /// </summary>
+        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();
+        }
+
+        /// <summary>
+        /// Applies a new <see cref="HitObject"/> to be represented by this <see cref="DrawableHitObject"/>.
+        /// </summary>
+        /// <param name="hitObject"></param>
+        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));
+        }
+
         /// <summary>
         /// Invoked by the base <see cref="DrawableHitObject"/> to populate samples, once on initial load and potentially again on any change to the samples collection.
         /// </summary>
@@ -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);
-            }
-        }
-
         /// <summary>
         /// Invoked by the base <see cref="DrawableHitObject"/> to add nested <see cref="DrawableHitObject"/>s to the hierarchy.
         /// </summary>
@@ -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;
         }
     }
 

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
         /// </summary>
         [NotNull]
         protected virtual HitWindows CreateHitWindows() => new HitWindows();
+
+        /// <summary>
+        /// The result this <see cref="HitObject"/> was judged with. Used internally for rewinding within <see cref="DrawableHitObject"/>.
+        /// </summary>
+        internal JudgementResult Result;
     }
 
     public static class HitObjectExtensions

osu.Game/Rulesets/UI/HitObjectContainer.cs

@@ -16,7 +16,7 @@ namespace osu.Game.Rulesets.UI
         public IEnumerable<DrawableHitObject> Objects => InternalChildren.Cast<DrawableHitObject>().OrderBy(h => h.HitObject.StartTime);
         public IEnumerable<DrawableHitObject> AliveObjects => AliveInternalChildren.Cast<DrawableHitObject>().OrderBy(h => h.HitObject.StartTime);
 
-        private readonly Dictionary<DrawableHitObject, (IBindable<double> bindable, double timeAtAdd)> startTimeMap = new Dictionary<DrawableHitObject, (IBindable<double>, double)>();
+        private readonly Dictionary<DrawableHitObject, IBindable> startTimeMap = new Dictionary<DrawableHitObject, IBindable>();
 
         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();
+        }
     }
 }

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