Merge pull request #10844 from bdach/spinner-rotation-tracker-state

Fix rotation tracker state not being reset on drawable spinner re-use

osu.Game.Rulesets.Osu.Tests/TestSceneSpinnerApplication.cs

@@ -15,7 +15,7 @@ namespace osu.Game.Rulesets.Osu.Tests
     public class TestSceneSpinnerApplication : OsuTestScene
     {
         [Test]
-        public void TestApplyNewCircle()
+        public void TestApplyNewSpinner()
         {
             DrawableSpinner dho = null;
 
@@ -23,18 +23,23 @@ namespace osu.Game.Rulesets.Osu.Tests
             {
                 Position = new Vector2(256, 192),
                 IndexInCurrentCombo = 0,
-                Duration = 0,
+                Duration = 500,
             }))
             {
                 Clock = new FramedClock(new StopwatchClock())
             });
 
+            AddStep("rotate some", () => dho.RotationTracker.AddRotation(180));
+            AddAssert("rotation is set", () => dho.Result.RateAdjustedRotation == 180);
+
             AddStep("apply new spinner", () => dho.Apply(prepareObject(new Spinner
             {
                 Position = new Vector2(256, 192),
                 ComboIndex = 1,
                 Duration = 1000,
             }), null));
+
+            AddAssert("rotation is reset", () => dho.Result.RateAdjustedRotation == 0);
         }
 
         private Spinner prepareObject(Spinner circle)

osu.Game.Rulesets.Osu.Tests/TestSceneSpinnerRotation.cs

@@ -62,11 +62,11 @@ namespace osu.Game.Rulesets.Osu.Tests
                 trackerRotationTolerance = Math.Abs(drawableSpinner.RotationTracker.Rotation * 0.1f);
             });
             AddAssert("is disc rotation not almost 0", () => !Precision.AlmostEquals(drawableSpinner.RotationTracker.Rotation, 0, 100));
-            AddAssert("is disc rotation absolute not almost 0", () => !Precision.AlmostEquals(drawableSpinner.RotationTracker.RateAdjustedRotation, 0, 100));
+            AddAssert("is disc rotation absolute not almost 0", () => !Precision.AlmostEquals(drawableSpinner.Result.RateAdjustedRotation, 0, 100));
 
             addSeekStep(0);
             AddAssert("is disc rotation almost 0", () => Precision.AlmostEquals(drawableSpinner.RotationTracker.Rotation, 0, trackerRotationTolerance));
-            AddAssert("is disc rotation absolute almost 0", () => Precision.AlmostEquals(drawableSpinner.RotationTracker.RateAdjustedRotation, 0, 100));
+            AddAssert("is disc rotation absolute almost 0", () => Precision.AlmostEquals(drawableSpinner.Result.RateAdjustedRotation, 0, 100));
         }
 
         [Test]
@@ -87,7 +87,7 @@ namespace osu.Game.Rulesets.Osu.Tests
                 finalSpinnerSymbolRotation = spinnerSymbol.Rotation;
                 spinnerSymbolRotationTolerance = Math.Abs(finalSpinnerSymbolRotation * 0.05f);
             });
-            AddStep("retrieve cumulative disc rotation", () => finalCumulativeTrackerRotation = drawableSpinner.RotationTracker.RateAdjustedRotation);
+            AddStep("retrieve cumulative disc rotation", () => finalCumulativeTrackerRotation = drawableSpinner.Result.RateAdjustedRotation);
 
             addSeekStep(2500);
             AddAssert("disc rotation rewound",
@@ -99,7 +99,7 @@ namespace osu.Game.Rulesets.Osu.Tests
                 () => Precision.AlmostEquals(spinnerSymbol.Rotation, finalSpinnerSymbolRotation / 2, spinnerSymbolRotationTolerance));
             AddAssert("is cumulative rotation rewound",
                 // cumulative rotation is not damped, so we're treating it as the "ground truth" and allowing a comparatively smaller margin of error.
-                () => Precision.AlmostEquals(drawableSpinner.RotationTracker.RateAdjustedRotation, finalCumulativeTrackerRotation / 2, 100));
+                () => Precision.AlmostEquals(drawableSpinner.Result.RateAdjustedRotation, finalCumulativeTrackerRotation / 2, 100));
 
             addSeekStep(5000);
             AddAssert("is disc rotation almost same",
@@ -107,7 +107,7 @@ namespace osu.Game.Rulesets.Osu.Tests
             AddAssert("is symbol rotation almost same",
                 () => Precision.AlmostEquals(spinnerSymbol.Rotation, finalSpinnerSymbolRotation, spinnerSymbolRotationTolerance));
             AddAssert("is cumulative rotation almost same",
-                () => Precision.AlmostEquals(drawableSpinner.RotationTracker.RateAdjustedRotation, finalCumulativeTrackerRotation, 100));
+                () => Precision.AlmostEquals(drawableSpinner.Result.RateAdjustedRotation, finalCumulativeTrackerRotation, 100));
         }
 
         [Test]
@@ -145,7 +145,7 @@ namespace osu.Game.Rulesets.Osu.Tests
             {
                 // multipled by 2 to nullify the score multiplier. (autoplay mod selected)
                 var totalScore = ((ScoreExposedPlayer)Player).ScoreProcessor.TotalScore.Value * 2;
-                return totalScore == (int)(drawableSpinner.RotationTracker.RateAdjustedRotation / 360) * new SpinnerTick().CreateJudgement().MaxNumericResult;
+                return totalScore == (int)(drawableSpinner.Result.RateAdjustedRotation / 360) * new SpinnerTick().CreateJudgement().MaxNumericResult;
             });
 
             addSeekStep(0);

osu.Game.Rulesets.Osu/Judgements/OsuSpinnerJudgementResult.cs

@@ -0,0 +1,52 @@
+// Copyright (c) ppy Pty Ltd <contact@ppy.sh>. Licensed under the MIT Licence.
+// See the LICENCE file in the repository root for full licence text.
+
+using osu.Game.Rulesets.Judgements;
+using osu.Game.Rulesets.Objects;
+using osu.Game.Rulesets.Osu.Objects;
+
+namespace osu.Game.Rulesets.Osu.Judgements
+{
+    public class OsuSpinnerJudgementResult : OsuJudgementResult
+    {
+        /// <summary>
+        /// The <see cref="Spinner"/>.
+        /// </summary>
+        public Spinner Spinner => (Spinner)HitObject;
+
+        /// <summary>
+        /// The total rotation performed on the spinner disc, disregarding the spin direction,
+        /// adjusted for the track's playback rate.
+        /// </summary>
+        /// <remarks>
+        /// <para>
+        /// This value is always non-negative and is monotonically increasing with time
+        /// (i.e. will only increase if time is passing forward, but can decrease during rewind).
+        /// </para>
+        /// <para>
+        /// The rotation from each frame is multiplied by the clock's current playback rate.
+        /// The reason this is done is to ensure that spinners give the same score and require the same number of spins
+        /// regardless of whether speed-modifying mods are applied.
+        /// </para>
+        /// </remarks>
+        /// <example>
+        /// Assuming no speed-modifying mods are active,
+        /// if the spinner is spun 360 degrees clockwise and then 360 degrees counter-clockwise,
+        /// this property will return the value of 720 (as opposed to 0).
+        /// If Double Time is active instead (with a speed multiplier of 1.5x),
+        /// in the same scenario the property will return 720 * 1.5 = 1080.
+        /// </example>
+        public float RateAdjustedRotation;
+
+        /// <summary>
+        /// Time instant at which the spinner has been completed (the user has executed all required spins).
+        /// Will be null if all required spins haven't been completed.
+        /// </summary>
+        public double? TimeCompleted;
+
+        public OsuSpinnerJudgementResult(HitObject hitObject, Judgement judgement)
+            : base(hitObject, judgement)
+        {
+        }
+    }
+}

osu.Game.Rulesets.Osu/Objects/Drawables/DrawableSpinner.cs

@@ -10,8 +10,10 @@ using osu.Framework.Bindables;
 using osu.Framework.Graphics;
 using osu.Framework.Graphics.Containers;
 using osu.Game.Graphics;
+using osu.Game.Rulesets.Judgements;
 using osu.Game.Rulesets.Objects;
 using osu.Game.Rulesets.Objects.Drawables;
+using osu.Game.Rulesets.Osu.Judgements;
 using osu.Game.Rulesets.Osu.Objects.Drawables.Pieces;
 using osu.Game.Rulesets.Osu.Skinning;
 using osu.Game.Rulesets.Scoring;
@@ -24,6 +26,8 @@ namespace osu.Game.Rulesets.Osu.Objects.Drawables
     {
         public new Spinner HitObject => (Spinner)base.HitObject;
 
+        public new OsuSpinnerJudgementResult Result => (OsuSpinnerJudgementResult)base.Result;
+
         public SpinnerRotationTracker RotationTracker { get; private set; }
         public SpinnerSpmCounter SpmCounter { get; private set; }
 
@@ -197,15 +201,18 @@ namespace osu.Game.Rulesets.Osu.Objects.Drawables
                     // these become implicitly hit.
                     return 1;
 
-                return Math.Clamp(RotationTracker.RateAdjustedRotation / 360 / HitObject.SpinsRequired, 0, 1);
+                return Math.Clamp(Result.RateAdjustedRotation / 360 / HitObject.SpinsRequired, 0, 1);
             }
         }
 
+        protected override JudgementResult CreateResult(Judgement judgement) => new OsuSpinnerJudgementResult(HitObject, judgement);
+
         protected override void CheckForResult(bool userTriggered, double timeOffset)
         {
             if (Time.Current < HitObject.StartTime) return;
 
-            RotationTracker.Complete.Value = Progress >= 1;
+            if (Progress >= 1)
+                Result.TimeCompleted ??= Time.Current;
 
             if (userTriggered || Time.Current < HitObject.EndTime)
                 return;
@@ -244,7 +251,7 @@ namespace osu.Game.Rulesets.Osu.Objects.Drawables
 
             if (!SpmCounter.IsPresent && RotationTracker.Tracking)
                 SpmCounter.FadeIn(HitObject.TimeFadeIn);
-            SpmCounter.SetRotation(RotationTracker.RateAdjustedRotation);
+            SpmCounter.SetRotation(Result.RateAdjustedRotation);
 
             updateBonusScore();
         }
@@ -256,7 +263,7 @@ namespace osu.Game.Rulesets.Osu.Objects.Drawables
             if (ticks.Count == 0)
                 return;
 
-            int spins = (int)(RotationTracker.RateAdjustedRotation / 360);
+            int spins = (int)(Result.RateAdjustedRotation / 360);
 
             if (spins < wholeSpins)
             {

osu.Game.Rulesets.Osu/Objects/Drawables/Pieces/DefaultSpinnerDisc.cs

@@ -3,6 +3,7 @@
 
 using System;
 using osu.Framework.Allocation;
+using osu.Framework.Bindables;
 using osu.Framework.Extensions.Color4Extensions;
 using osu.Framework.Graphics;
 using osu.Framework.Graphics.Containers;
@@ -28,6 +29,7 @@ namespace osu.Game.Rulesets.Osu.Objects.Drawables.Pieces
         private SpinnerTicks ticks;
 
         private int wholeRotationCount;
+        private readonly BindableBool complete = new BindableBool();
 
         private SpinnerFill fill;
         private Container mainContainer;
@@ -89,7 +91,7 @@ namespace osu.Game.Rulesets.Osu.Objects.Drawables.Pieces
         {
             base.LoadComplete();
 
-            drawableSpinner.RotationTracker.Complete.BindValueChanged(complete => updateComplete(complete.NewValue, 200));
+            complete.BindValueChanged(complete => updateComplete(complete.NewValue, 200));
             drawableSpinner.ApplyCustomUpdateState += updateStateTransforms;
 
             updateStateTransforms(drawableSpinner, drawableSpinner.State.Value);
@@ -99,7 +101,9 @@ namespace osu.Game.Rulesets.Osu.Objects.Drawables.Pieces
         {
             base.Update();
 
-            if (drawableSpinner.RotationTracker.Complete.Value)
+            complete.Value = Time.Current >= drawableSpinner.Result.TimeCompleted;
+
+            if (complete.Value)
             {
                 if (checkNewRotationCount)
                 {
@@ -194,7 +198,7 @@ namespace osu.Game.Rulesets.Osu.Objects.Drawables.Pieces
         {
             get
             {
-                int rotations = (int)(drawableSpinner.RotationTracker.RateAdjustedRotation / 360);
+                int rotations = (int)(drawableSpinner.Result.RateAdjustedRotation / 360);
 
                 if (wholeRotationCount == rotations) return false;
 

osu.Game.Rulesets.Osu/Objects/Drawables/Pieces/SpinnerRotationTracker.cs

@@ -8,6 +8,7 @@ using osu.Framework.Graphics;
 using osu.Framework.Graphics.Containers;
 using osu.Framework.Input.Events;
 using osu.Framework.Utils;
+using osu.Game.Rulesets.Objects.Drawables;
 using osu.Game.Screens.Play;
 using osuTK;
 
@@ -22,6 +23,7 @@ namespace osu.Game.Rulesets.Osu.Objects.Drawables.Pieces
         public SpinnerRotationTracker(DrawableSpinner drawableSpinner)
         {
             this.drawableSpinner = drawableSpinner;
+            drawableSpinner.HitObjectApplied += resetState;
 
             RelativeSizeAxes = Axes.Both;
         }
@@ -30,32 +32,6 @@ namespace osu.Game.Rulesets.Osu.Objects.Drawables.Pieces
 
         public bool Tracking { get; set; }
 
-        public readonly BindableBool Complete = new BindableBool();
-
-        /// <summary>
-        /// The total rotation performed on the spinner disc, disregarding the spin direction,
-        /// adjusted for the track's playback rate.
-        /// </summary>
-        /// <remarks>
-        /// <para>
-        /// This value is always non-negative and is monotonically increasing with time
-        /// (i.e. will only increase if time is passing forward, but can decrease during rewind).
-        /// </para>
-        /// <para>
-        /// The rotation from each frame is multiplied by the clock's current playback rate.
-        /// The reason this is done is to ensure that spinners give the same score and require the same number of spins
-        /// regardless of whether speed-modifying mods are applied.
-        /// </para>
-        /// </remarks>
-        /// <example>
-        /// Assuming no speed-modifying mods are active,
-        /// if the spinner is spun 360 degrees clockwise and then 360 degrees counter-clockwise,
-        /// this property will return the value of 720 (as opposed to 0 for <see cref="Drawable.Rotation"/>).
-        /// If Double Time is active instead (with a speed multiplier of 1.5x),
-        /// in the same scenario the property will return 720 * 1.5 = 1080.
-        /// </example>
-        public float RateAdjustedRotation { get; private set; }
-
         /// <summary>
         /// Whether the spinning is spinning at a reasonable speed to be considered visually spinning.
         /// </summary>
@@ -131,7 +107,24 @@ namespace osu.Game.Rulesets.Osu.Objects.Drawables.Pieces
             currentRotation += angle;
             // rate has to be applied each frame, because it's not guaranteed to be constant throughout playback
             // (see: ModTimeRamp)
-            RateAdjustedRotation += (float)(Math.Abs(angle) * (gameplayClock?.TrueGameplayRate ?? Clock.Rate));
+            drawableSpinner.Result.RateAdjustedRotation += (float)(Math.Abs(angle) * (gameplayClock?.TrueGameplayRate ?? Clock.Rate));
+        }
+
+        private void resetState(DrawableHitObject obj)
+        {
+            Tracking = false;
+            IsSpinning.Value = false;
+            mousePosition = default;
+            lastAngle = currentRotation = Rotation = 0;
+            rotationTransferred = false;
+        }
+
+        protected override void Dispose(bool isDisposing)
+        {
+            base.Dispose(isDisposing);
+
+            if (drawableSpinner != null)
+                drawableSpinner.HitObjectApplied -= resetState;
         }
     }
 }

osu.Game.Rulesets.Osu/Skinning/LegacySpinner.cs

@@ -60,7 +60,6 @@ namespace osu.Game.Rulesets.Osu.Skinning
         {
             base.LoadComplete();
 
-            completed.BindTo(DrawableSpinner.RotationTracker.Complete);
             completed.BindValueChanged(onCompletedChanged, true);
 
             DrawableSpinner.ApplyCustomUpdateState += UpdateStateTransforms;
@@ -93,6 +92,12 @@ namespace osu.Game.Rulesets.Osu.Skinning
             }
         }
 
+        protected override void Update()
+        {
+            base.Update();
+            completed.Value = Time.Current >= DrawableSpinner.Result.TimeCompleted;
+        }
+
         protected virtual void UpdateStateTransforms(DrawableHitObject drawableHitObject, ArmedState state)
         {
             switch (drawableHitObject)

osu.Game/Rulesets/Objects/Drawables/DrawableHitObject.cs

@@ -285,6 +285,7 @@ namespace osu.Game.Rulesets.Objects.Drawables
             OnFree(HitObject);
 
             HitObject = null;
+            Result = null;
             lifetimeEntry = null;
 
             clearExistingStateTransforms();