Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix crash on changing skins when classic mod is enabled and game is rewound #29406

Merged
merged 3 commits into from
Aug 14, 2024

Conversation

peppy
Copy link
Sponsor Member

@peppy peppy commented Aug 13, 2024

I can't mentally figure out what is leading to the issue here, but in the case where prepareDrawables is called from JudgementBody.OnSkinChanged, things go very wrong. It's a weird recursive-like call that really shouldn't have existed in the first place. I think a smell test is enough for anyone to agree that the flow was very bad.

Removing this call doesn't seem to cause any issues. runAnimation should always be called in PrepareForUse (both pooled and non-pooled scenarios) so things should still always be in a correct state.

Closes #29398.


If anyone wishes to try and track down the actual underlying cause of this:

diff --git a/osu.Game/Rulesets/Judgements/DrawableJudgement.cs b/osu.Game/Rulesets/Judgements/DrawableJudgement.cs
index bdeadfd201..b37f855b41 100644
--- a/osu.Game/Rulesets/Judgements/DrawableJudgement.cs
+++ b/osu.Game/Rulesets/Judgements/DrawableJudgement.cs
@@ -120,7 +120,10 @@ protected override void PrepareForUse()
         private void runAnimation()
         {
             // is a no-op if the drawables are already in a correct state.
-            prepareDrawables();
+            if (prepareDrawables())
+            {
+                Console.WriteLine($"WARNING bad thing happened for {Result.Type} ({Result.HitObject})"); // breakpoint
+            }
 
             // undo any transforms applies in ApplyMissAnimations/ApplyHitAnimations to get a sane initial state.
             ApplyTransformsAt(double.MinValue, true);
@@ -161,13 +164,13 @@ private void runAnimation()
 
         private HitResult? currentDrawableType;
 
-        private void prepareDrawables()
+        private bool prepareDrawables()
         {
             var type = Result?.Type ?? HitResult.Perfect; //TODO: better default type from ruleset
 
             // todo: this should be removed once judgements are always pooled.
             if (type == currentDrawableType)
-                return;
+                return false;
 
             // sub-classes might have added their own children that would be removed here if .InternalChild was used.
             if (JudgementBody != null)
@@ -199,6 +202,8 @@ void proxyContent()
                         aboveHitObjectsContent.Add(proxiedContent);
                 }
             }
+
+            return true;
         }
 
         protected virtual Drawable CreateDefaultJudgement(HitResult result) => new DefaultJudgementPiece(result);
  • Apply classic mod
  • Start with autoplay.
  • Seek forward until after a slider
  • Seek backward until before slider
  • Change skin

Doesn't matter what skin before/after. It's clearly something to do with the classic skin slider logic, and probably something to do with different lifetimes making this only happen for said scenario. Alternatively test that this is fixed and not worry about why it happened (probably irrelevant).

I can't mentally figure out *what* is causing the issue here, but in the
case where `prepareDrawables` is called from
`JudgementBody.OnSkinChanged` (only happens in a non-pooled scenario),
things go very wrong.

I think a smell test is enough for anyone to agree that the flow was
very bad. Removing this call doesn't seem to cause any issues.

`runAnimation` should always be called in `PrepareForUse` (both pooled
and non-pooled scenarios) so things should still always be in a correct
state.

Closes ppy#29398.
@smoogipoo smoogipoo merged commit 8bfb5ce into ppy:master Aug 14, 2024
11 of 13 checks passed
@peppy peppy deleted the fix-skin-change-crash branch August 15, 2024 09:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Switch skin when playing replay may will cause crash
2 participants