From b1e039bcec1c45c66f7e622d20b1dbb8c104fea9 Mon Sep 17 00:00:00 2001
From: smoogipoo <smoogipoo@smgi.me>
Date: Sat, 7 Nov 2020 00:40:26 +0900
Subject: [PATCH] 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 <see cref="HitObject"/> to be represented by this <see cref="DrawableHitObject"/>.
         /// </summary>
         /// <param name="hitObject">The <see cref="HitObject"/> to apply.</param>
-        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;
         }
 
         /// <summary>
         /// Removes the currently applied <see cref="HitObject"/>
         /// </summary>
-        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();
+        }
+
+        /// <summary>
+        /// Invoked for this <see cref="DrawableHitObject"/> to take on any values from a newly-applied <see cref="HitObject"/>.
+        /// </summary>
+        /// <param name="hitObject">The <see cref="HitObject"/> being applied.</param>
+        protected virtual void OnApply(HitObject hitObject)
+        {
+        }
+
+        /// <summary>
+        /// Invoked for this <see cref="DrawableHitObject"/> to revert any values previously taken on from the currently-applied <see cref="HitObject"/>.
+        /// </summary>
+        /// <param name="hitObject">The currently-applied <see cref="HitObject"/>.</param>
+        protected virtual void OnFree(HitObject hitObject)
+        {
         }
 
         /// <summary>
@@ -285,7 +307,6 @@ namespace osu.Game.Rulesets.Objects.Drawables
 
         private void onDefaultsApplied(HitObject hitObject)
         {
-            Free();
             Apply(hitObject);
             DefaultsApplied?.Invoke(this);
         }