Game crash when changing addition sample bank for multiple objects

[catsstuffs]

Type

Crash to desktop

Bug description

The game crashes after selecting multiple objects with different addition, then attempting changing the addition sample bank on a slider end with a hitsound. You can reproduce like in the video by doing CTRL-A to a map with multiple banks for additions, changing the addition bank of a slider end, deselecting, then adding a hitsound to the slider end. I've not been able to get it to consistently reproduce either, so I'm not sure what direction to give there

Screenshots or videos

2024-06-26.21-14-44.mp4

Version

2024.625.2-lazer

Logs

compressed-logs.zip

[catsstuffs]

After more testing, I'm getting mixed results where on some maps it will end up just changing the banks to the last letter I input on the addition sample box instead of crashing :o

[bdach]

Crash in the logs:

2024-06-27 02:15:20 [verbose]: Too many unhandled exceptions, crashing out.
2024-06-27 02:15:20 [error]: An unhandled error has occurred.
2024-06-27 02:15:20 [error]: System.NullReferenceException: Object reference not set to an instance of an object.
2024-06-27 02:15:20 [error]: at osu.Game.Rulesets.Osu.Objects.Drawables.DrawableOsuHitObject.<UpdateInitialTransforms>g__applyDim|15_0(Drawable piece) in /home/runner/work/osu-auth-client/osu-auth-client/osu/osu.Game.Rulesets.Osu/Objects/Drawables/DrawableOsuHitObject.cs:line 101
2024-06-27 02:15:20 [error]: at osu.Game.Rulesets.Objects.Drawables.DrawableHitObject.UpdateState(ArmedState newState, Boolean force) in /home/runner/work/osu-auth-client/osu-auth-client/osu/osu.Game/Rulesets/Objects/Drawables/DrawableHitObject.cs:line 492
2024-06-27 02:15:20 [error]: at osu.Game.Rulesets.Osu.Objects.Drawables.DrawableSliderTail.SuppressHitAnimations() in /home/runner/work/osu-auth-client/osu-auth-client/osu/osu.Game.Rulesets.Osu/Objects/Drawables/DrawableSliderTail.cs:line 135
2024-06-27 02:15:20 [error]: at osu.Framework.Graphics.Drawable.UpdateSubTree()
2024-06-27 02:15:20 [error]: at osu.Framework.Graphics.Containers.CompositeDrawable.UpdateSubTree()

Looking at sentry this doesn't to be specific to changing samples, or even the editor. I see several instances of this from different stacks. More likely that the method in question is just generally unsafe.

After more testing, I'm getting mixed results where on some maps it will end up just changing the banks to the last letter I input on the addition sample box instead of crashing :o

I can't reproduce this. Please provide a video of this showing, preferably with the link to the map(s) that exhibit this behaviour.

[ppy-sentryintegration]

Sentry issue: OSU-133H

[bdach]

I guess for the crash I would do this...

diff --git a/osu.Game.Rulesets.Osu/Objects/Drawables/DrawableOsuHitObject.cs b/osu.Game.Rulesets.Osu/Objects/Drawables/DrawableOsuHitObject.cs
index 5f5deca1ba..f71d0d1d62 100644
--- a/osu.Game.Rulesets.Osu/Objects/Drawables/DrawableOsuHitObject.cs
+++ b/osu.Game.Rulesets.Osu/Objects/Drawables/DrawableOsuHitObject.cs
@@ -6,6 +6,7 @@
 using System;
 using System.Collections.Generic;
 using System.Linq;
+using JetBrains.Annotations;
 using osu.Framework.Allocation;
 using osu.Framework.Bindables;
 using osu.Framework.Graphics;
@@ -73,6 +74,7 @@ protected override void OnFree()
             ScaleBindable.UnbindFrom(HitObject.ScaleBindable);
         }
 
+        [ItemCanBeNull]
         protected virtual IEnumerable<Drawable> DimmablePieces => Enumerable.Empty<Drawable>();
 
         protected override void UpdateInitialTransforms()
@@ -91,7 +93,7 @@ protected override void UpdateInitialTransforms()
                     drawableObjectPiece.ApplyCustomUpdateState -= applyDimToDrawableHitObject;
                     drawableObjectPiece.ApplyCustomUpdateState += applyDimToDrawableHitObject;
                 }
-                else
+                else if (piece?.IsLoaded == true)
                     applyDim(piece);
             }
 

but I can't reproduce this crashing for one, and secondly it's a bit of a dicey one. I'm pretty sure it should be safe because DrawableHitObjects.LoadComplete() calls UpdateState(force: true), but it'll be difficult to ascertain in the general...

@ppy/team-client thoughts on this?

[peppy]

hmm, what would you say to this solution instead?

diff --git a/osu.Game.Rulesets.Osu/Objects/Drawables/DrawableHitCircle.cs b/osu.Game.Rulesets.Osu/Objects/Drawables/DrawableHitCircle.cs
index 7d707dea6c..760e15190c 100644
--- a/osu.Game.Rulesets.Osu/Objects/Drawables/DrawableHitCircle.cs
+++ b/osu.Game.Rulesets.Osu/Objects/Drawables/DrawableHitCircle.cs
@@ -325,6 +325,9 @@ public ProxyableSkinnableDrawable(ISkinComponentLookup lookup, Func<ISkinCompone
 
         internal void SuppressHitAnimations()
         {
+            if (!IsLoaded)
+                return;
+
             UpdateState(ArmedState.Idle, true);
             UpdateComboColour();
 
diff --git a/osu.Game.Rulesets.Osu/Objects/Drawables/DrawableSlider.cs b/osu.Game.Rulesets.Osu/Objects/Drawables/DrawableSlider.cs
index 02d0ebee83..69ad126f7b 100644
--- a/osu.Game.Rulesets.Osu/Objects/Drawables/DrawableSlider.cs
+++ b/osu.Game.Rulesets.Osu/Objects/Drawables/DrawableSlider.cs
@@ -375,6 +375,9 @@ private partial class DefaultSliderBody : PlaySliderBody
 
         internal void SuppressHitAnimations()
         {
+            if (!IsLoaded)
+                return;
+
             UpdateState(ArmedState.Idle, true);
             HeadCircle.SuppressHitAnimations();
             TailCircle.SuppressHitAnimations();
diff --git a/osu.Game.Rulesets.Osu/Objects/Drawables/DrawableSliderTail.cs b/osu.Game.Rulesets.Osu/Objects/Drawables/DrawableSliderTail.cs
index 42abf41d6f..053ca25398 100644
--- a/osu.Game.Rulesets.Osu/Objects/Drawables/DrawableSliderTail.cs
+++ b/osu.Game.Rulesets.Osu/Objects/Drawables/DrawableSliderTail.cs
@@ -132,6 +132,9 @@ protected override void OnApply()
 
         internal void SuppressHitAnimations()
         {
+            if (!IsLoaded)
+                return;
+
             UpdateState(ArmedState.Idle, true);
             UpdateComboColour();
 

I believe this is specific to the new flow, where SuppressHitAnimations is called by a parent drawable potentially a frame before the child is fully loaded. So under normal circumstances things should never break (and the check you added in UpdateInitialTransforms is nor required).

[bdach]

I believe this is specific to the new flow

Except it isn't. Check "merged issues" under the sentry link above, you'll see more stacks than that.

The last one is especially freaky. I haven't seen that one before, not sure how that is even possible.

[peppy]

Okay that's very scary. I think I'd want to understand how these can happen before deciding on a fix..

[peppy]

The only way I can see the last of those screenshots happening is a DrawableSlider being loaded without its nested objects being added yet. Assuming nested objects aren't just being skipped completely, this suggests OnApply is being run after LoadComplete... which I guess is feasible since there's quite a few calls directly to Apply(), with some suspicious ones in the editor.

[catsstuffs]

I could recreate the crash and the issue from the 2nd screenshot on a map I made, and on this map. Another difficulty had no issues changing the addition banks, oddly enough. I've attached two videos of me doing so on all three, and the map on gofile (github hates olz)

compressed3.mp4

compressed2.mp4

[catsstuffs]

if the link to download the map ever dies, let me know and i can attempt to reupload it ^^