Fix legacy mania note body animation not resetting sometimes

[bdach]

RFC. Hopefully closes #28284.

As far as I can tell this is a somewhat difficult one to reproduce because it relies on a specific set of circumstances (at least the reproduction case that I found does). The reset to frame 0 would previously be called explicitly when isHitting changed:

osu/osu.Game.Rulesets.Mania/Skinning/Legacy/LegacyBodyPiece.cs

Line 144 in 182ca14

bodyAnimation.GotoFrame(0);

However, it can be the case that bodyAnimation is not loaded at the point of this call. This is significant because
SkinnableTextureAnimation contains this logic:

osu/osu.Game/Skinning/LegacySkinExtensions.cs

Lines 192 to 211 in 182ca14

	protected override void LoadComplete()
	{
	base.LoadComplete();
	if (timeReference != null)
	{
	Clock = timeReference.Clock;
	animationStartTime.BindTo(timeReference.AnimationStartTime);
	}
	animationStartTime.BindValueChanged(_ => updatePlaybackPosition(), true);
	}
	private void updatePlaybackPosition()
	{
	if (timeReference == null)
	return;
	PlaybackPosition = timeReference.Clock.CurrentTime - timeReference.AnimationStartTime.Value;
	}

which cannot be moved any earlier (because any earlier the Clock may no longer be correct), and also causes the animation to be seeked forward while it is stopped.

I can't figure out a decent way to layer this otherwise (by scheduling or whatever), so this commit is just applying the nuclear option of just seeking back to frame 0 on every update frame in which the body piece is not being hit.

No tests because I hope it's obvious why no tests based on description above. The one reliable repro scenario that I found was to start https://osu.ppy.sh/beatmapsets/971561#mania/2034200 and then seek gameplay forward immediately via right arrow key and then pause, as per attached video:

2024-05-28.16-14-13.mp4

[smoogipoo]

This is kinda scary because this doesn't look like some obscure usage. As far as I can tell the scenario you describe is always happening in this component because GotoFrame(0) is done in a parent's LoadComplete(), which always occurs prior to a child's LoadComplete().

[bdach]

Well, to be more specific, it would happen on every first usage of LegacyBodyPiece which is inside a (pooled) DrawableHoldNote, right? It's not like the LegacyBodyPiece is reconstructed on every reuse, or is it?

[smoogipoo]

It should only be constructed once for every DHO but, weirdly enough, the issue repros every time:

2024-05-29.17-36-13.mp4

[bdach]

Oh... it's likely because of this thing:

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

Line 475 in ba3880a

AnimationStartTime.Value = initialTransformsTime;

which will trigger that same value change callback, just via a different path.

The nuclear Update() fix should still be working but it does make this entire situation more dodgy than I thought it was.

[smoogipoo]

Hmmm... In that case I have a little bit more faith in that this is more correct, actually. The premise being:

1. Suppose we've gone through the entire process once - IsHitting = true and then IsHitting = false.
2. On next use the DHO changes the animation start time, but IsHitting is still false.
3. If the animation start time callback cannot ensure PlaybackPosition = 0 on its own, then we would need a GotoFrame(0) at some point here, which again couldn't be fulfilled by the IsHitting callback.

It's a bit of a continuation of the initial case you described, but hinging on the fact that the callback may not always occur.

I still think that this is particularly scary and a bit of a hidden footgun, but I'm not sure how to fix it in a more general way...

[smoogipoo]

diff --git a/osu.Game.Rulesets.Mania/Skinning/Legacy/LegacyBodyPiece.cs b/osu.Game.Rulesets.Mania/Skinning/Legacy/LegacyBodyPiece.cs
index a8200e0144..a807ce8190 100644
--- a/osu.Game.Rulesets.Mania/Skinning/Legacy/LegacyBodyPiece.cs
+++ b/osu.Game.Rulesets.Mania/Skinning/Legacy/LegacyBodyPiece.cs
@@ -117,6 +117,12 @@ protected override void LoadComplete()
             isHitting.BindValueChanged(onIsHittingChanged, true);
             missFadeTime.BindValueChanged(onMissFadeTimeChanged, true);

+            holdNote.DefaultsApplied += _ =>
+            {
+                if (bodySprite is TextureAnimation bodyAnimation)
+                    bodyAnimation.GotoFrame(0);
+            };
+
             holdNote.ApplyCustomUpdateState += applyCustomUpdateState;
             applyCustomUpdateState(holdNote, holdNote.State.Value);
         }

Something like this also appears to fix it and may make more sense as a general way-of-using-this-component, but I think I'll go with the changes in this PR and adjust if this comes up again.