Make judgements follow DrawableHitObjects and enable them in magnetised, repel and depth

[DavidBeh]

Addresses #20139

Makes judgements appear based on the DrawableHitobjects position instead of the Hitobjects position which is not updated by mods moving single Hitobjects. They move with hitobjects as long as the drawables are alive, so they move with the shadow of hits and stay on misses.

Judgements are enabled for magnetised, repel and depth.

[bdach]

I don't think this modifier change achieves anything useful?

[bdach]

This is the type of comment that just really makes me question "why aren't we doing that though?"

I'm not sure anyone knows the answer to that but does that do anything to fix the issue? Why was this comment added here?

[bdach]

So about the entirety of this...

• Not sure why PrepareForUse() does this too if the position update is just gonna be placed in Update() like this. The first one can probably be removed.
• What's this Parent != null check?
• The osuObject.HitObject != null - and generally associating with a potentially-stale JudgedObject - makes me rather uneasy. Connections across pooled object instances are generally quite dodgy to begin with and best avoided if can be, but I don't see how to avoid it in this case.

All in all, I'd see all of the above addressed by something like the following patch:

diff --git a/osu.Game.Rulesets.Osu.Tests/TestSceneDrawableJudgement.cs b/osu.Game.Rulesets.Osu.Tests/TestSceneDrawableJudgement.cs
index 5f5596cbb3..a239f671af 100644
--- a/osu.Game.Rulesets.Osu.Tests/TestSceneDrawableJudgement.cs
+++ b/osu.Game.Rulesets.Osu.Tests/TestSceneDrawableJudgement.cs
@@ -108,7 +108,7 @@ private void showResult(HitResult result)
         private partial class TestDrawableOsuJudgement : DrawableOsuJudgement
         {
             public new SkinnableSprite Lighting => base.Lighting;
-            public new SkinnableDrawable JudgementBody => base.JudgementBody;
+            public new SkinnableDrawable? JudgementBody => base.JudgementBody;
         }
     }
 }
diff --git a/osu.Game.Rulesets.Osu/Objects/Drawables/DrawableOsuJudgement.cs b/osu.Game.Rulesets.Osu/Objects/Drawables/DrawableOsuJudgement.cs
index ffbf45291f..98fb99609a 100644
--- a/osu.Game.Rulesets.Osu/Objects/Drawables/DrawableOsuJudgement.cs
+++ b/osu.Game.Rulesets.Osu/Objects/Drawables/DrawableOsuJudgement.cs
@@ -1,8 +1,6 @@
 // Copyright (c) ppy Pty Ltd <[email protected]>. Licensed under the MIT Licence.
 // See the LICENCE file in the repository root for full licence text.
 
-#nullable disable
-
 using osu.Framework.Allocation;
 using osu.Framework.Graphics;
 using osu.Game.Configuration;
@@ -14,10 +12,10 @@ namespace osu.Game.Rulesets.Osu.Objects.Drawables
 {
     public partial class DrawableOsuJudgement : DrawableJudgement
     {
-        internal SkinnableLighting Lighting { get; private set; }
+        internal SkinnableLighting Lighting { get; private set; } = null!;
 
         [Resolved]
-        private OsuConfigManager config { get; set; }
+        private OsuConfigManager config { get; set; } = null!;
 
         [BackgroundDependencyLoader]
         private void load()
@@ -38,19 +36,13 @@ protected override void PrepareForUse()
 
             Lighting.ResetAnimation();
             Lighting.SetColourFrom(JudgedObject, Result);
-
-            if (JudgedObject is DrawableOsuHitObject osuObject)
-            {
-                Position = osuObject.ToSpaceOfOtherDrawable(osuObject.OriginPosition, Parent!);
-                Scale = new Vector2(osuObject.HitObject.Scale);
-            }
         }
 
         protected override void Update()
         {
             base.Update();
 
-            if (JudgedObject is DrawableOsuHitObject osuObject && Parent != null && osuObject.HitObject != null)
+            if (JudgedObject is DrawableOsuHitObject osuObject && JudgedObject.IsInUse)
             {
                 Position = osuObject.ToSpaceOfOtherDrawable(osuObject.OriginPosition, Parent!);
                 Scale = new Vector2(osuObject.HitObject.Scale);
diff --git a/osu.Game/Rulesets/Judgements/DrawableJudgement.cs b/osu.Game/Rulesets/Judgements/DrawableJudgement.cs
index b4686c52f3..37a9766b71 100644
--- a/osu.Game/Rulesets/Judgements/DrawableJudgement.cs
+++ b/osu.Game/Rulesets/Judgements/DrawableJudgement.cs
@@ -1,11 +1,8 @@
 // Copyright (c) ppy Pty Ltd <[email protected]>. Licensed under the MIT Licence.
 // See the LICENCE file in the repository root for full licence text.
 
-#nullable disable
-
 using System;
 using System.Diagnostics;
-using JetBrains.Annotations;
 using osu.Framework.Allocation;
 using osu.Framework.Graphics;
 using osu.Framework.Graphics.Containers;
@@ -24,13 +21,13 @@ public partial class DrawableJudgement : PoolableDrawable
     {
         private const float judgement_size = 128;
 
-        public JudgementResult Result { get; private set; }
+        public JudgementResult? Result { get; private set; }
 
-        public DrawableHitObject JudgedObject { get; private set; }
+        public DrawableHitObject? JudgedObject { get; private set; }
 
         public override bool RemoveCompletedTransforms => false;
 
-        protected SkinnableDrawable JudgementBody { get; private set; }
+        protected SkinnableDrawable? JudgementBody { get; private set; }
 
         private readonly Container aboveHitObjectsContent;
 
@@ -97,12 +94,19 @@ protected virtual void ApplyMissAnimations()
         /// </summary>
         /// <param name="result">The applicable judgement.</param>
         /// <param name="judgedObject">The drawable object.</param>
-        public void Apply([NotNull] JudgementResult result, [CanBeNull] DrawableHitObject judgedObject)
+        public void Apply(JudgementResult result, DrawableHitObject? judgedObject)
         {
             Result = result;
             JudgedObject = judgedObject;
         }
 
+        protected override void FreeAfterUse()
+        {
+            base.FreeAfterUse();
+
+            JudgedObject = null;
+        }
+
         protected override void PrepareForUse()
         {
             base.PrepareForUse();
@@ -121,6 +125,8 @@ private void runAnimation()
             ApplyTransformsAt(double.MinValue, true);
             ClearTransforms(true);
 
+            Debug.Assert(Result != null && JudgementBody != null);
+
             LifetimeStart = Result.TimeAbsolute;
 
             using (BeginAbsoluteSequence(Result.TimeAbsolute))

which:

• removes the seemingly unnecessary extra update from PrepareForUse()
• removes the Parent != null check (to seemingly no bad effects?)
• NRT-annotates DrawableJudgement, annotates JudgedObject as nullable and makes it eagerly null it on free, hopefully ensuring that there is no stale poolable reference read (and also replaces that last HitObject != null check with an actual poolable check).

I'd probably want @ppy/team-client to sound off on this in general since it's a bit of a change that I'm not exactly feeling comfortable making solo.

[DavidBeh]

This sounds good.

• The access modifier in DrawableSliderTick was changed for testing and I forgot to revert it.
• The Parent != null check comes from NullReferenceExceptions where the Update methods state was not debugable so I added null checks until it was fixed.
• Updating the Code to use modern nullability language features is a good.
• I am less familiar with the codebase so I can't really comment on the stuff about Poolables, but I don't see a problem here.

Thanks for reviewing my pull request.