From ee33f62809067247049f03d6900eb7e7077bf55f Mon Sep 17 00:00:00 2001
From: Dean Herbert <pe@ppy.sh>
Date: Tue, 24 Nov 2020 15:19:51 +0900
Subject: [PATCH] Fix DrawableJudgement not always animating correctly on skin
 change

If the skin is changed before gameplay has started (at the loading
screen) it is possible for a sequence of events to occur which results
in the animation not being played:

- `SkinReloadableDrawable` runs its BDL load (and calls `OnSkinChanged`
  once)
- User changes skin, triggering `DrawableJudgement`'s skin change
  handling (binding directly on the `SkinSource` locally)
- This will call `PrepareDrawables` and reinitialise the
  `SkinnableDrawable` child hierarchy, then immediately apply the
animations to it.
- The new `SkinnableDrawable` will then get the `SkinChanged` event and
  schedule a handler for it, which will run on its first Update call.
- Any added animations will be lost as a result.

Fixed by binding directly to the `SkinnableDrawable`'s `OnSkinChanged`.
This has the added bonus of not needing to reinitialise the child
hierarchy on skin change (which felt a bit weird in the first place).
---
 .../Rulesets/Judgements/DrawableJudgement.cs  | 63 ++++++++-----------
 1 file changed, 27 insertions(+), 36 deletions(-)

diff --git a/osu.Game/Rulesets/Judgements/DrawableJudgement.cs b/osu.Game/Rulesets/Judgements/DrawableJudgement.cs
index 889e748a4a..45129c17ea 100644
--- a/osu.Game/Rulesets/Judgements/DrawableJudgement.cs
+++ b/osu.Game/Rulesets/Judgements/DrawableJudgement.cs
@@ -32,9 +32,6 @@ namespace osu.Game.Rulesets.Judgements
 
         private readonly Container aboveHitObjectsContent;
 
-        [Resolved]
-        private ISkinSource skinSource { get; set; }
-
         /// <summary>
         /// Duration of initial fade in.
         /// </summary>
@@ -78,29 +75,6 @@ namespace osu.Game.Rulesets.Judgements
 
         public Drawable GetProxyAboveHitObjectsContent() => aboveHitObjectsContent.CreateProxy();
 
-        protected override void LoadComplete()
-        {
-            base.LoadComplete();
-            skinSource.SourceChanged += onSkinChanged;
-        }
-
-        private void onSkinChanged()
-        {
-            // on a skin change, the child component will update but not get correctly triggered to play its animation.
-            // we need to trigger a reinitialisation to make things right.
-            currentDrawableType = null;
-
-            PrepareForUse();
-        }
-
-        protected override void Dispose(bool isDisposing)
-        {
-            base.Dispose(isDisposing);
-
-            if (skinSource != null)
-                skinSource.SourceChanged -= onSkinChanged;
-        }
-
         /// <summary>
         /// Apply top-level animations to the current judgement when successfully hit.
         /// If displaying components which require lifetime extensions, manually adjusting <see cref="Drawable.LifetimeEnd"/> is required.
@@ -142,16 +116,17 @@ namespace osu.Game.Rulesets.Judgements
 
             Debug.Assert(Result != null);
 
-            prepareDrawables();
-
             runAnimation();
         }
 
         private void runAnimation()
         {
+            // is a no-op if the drawables are already in a correct state.
+            prepareDrawables();
+
             // undo any transforms applies in ApplyMissAnimations/ApplyHitAnimations to get a sane initial state.
-            ApplyTransformsAt(double.MinValue, true);
-            ClearTransforms(true);
+            // ApplyTransformsAt(double.MinValue, true);
+            // ClearTransforms(true);
 
             LifetimeStart = Result.TimeAbsolute;
 
@@ -193,6 +168,8 @@ namespace osu.Game.Rulesets.Judgements
 
         private void prepareDrawables()
         {
+            Logger.Log("prepareDrawables on DrawableJudgement");
+
             var type = Result?.Type ?? HitResult.Perfect; //TODO: better default type from ruleset
 
             // todo: this should be removed once judgements are always pooled.
@@ -203,7 +180,6 @@ namespace osu.Game.Rulesets.Judgements
             if (JudgementBody != null)
                 RemoveInternal(JudgementBody);
 
-            aboveHitObjectsContent.Clear();
             AddInternal(JudgementBody = new SkinnableDrawable(new GameplaySkinComponent<HitResult>(type), _ =>
                 CreateDefaultJudgement(type), confineMode: ConfineMode.NoScaling)
             {
@@ -211,14 +187,29 @@ namespace osu.Game.Rulesets.Judgements
                 Origin = Anchor.Centre,
             });
 
-            if (JudgementBody.Drawable is IAnimatableJudgement animatable)
+            JudgementBody.OnSkinChanged += () =>
             {
-                var proxiedContent = animatable.GetAboveHitObjectsProxiedContent();
-                if (proxiedContent != null)
-                    aboveHitObjectsContent.Add(proxiedContent);
-            }
+                // on a skin change, the child component will update but not get correctly triggered to play its animation (or proxy the newly created content).
+                // we need to trigger a reinitialisation to make things right.
+                proxyContent();
+                runAnimation();
+            };
+
+            proxyContent();
 
             currentDrawableType = type;
+
+            void proxyContent()
+            {
+                aboveHitObjectsContent.Clear();
+
+                if (JudgementBody.Drawable is IAnimatableJudgement animatable)
+                {
+                    var proxiedContent = animatable.GetAboveHitObjectsProxiedContent();
+                    if (proxiedContent != null)
+                        aboveHitObjectsContent.Add(proxiedContent);
+                }
+            }
         }
 
         protected virtual Drawable CreateDefaultJudgement(HitResult result) => new DefaultJudgementPiece(result);