1
0
mirror of https://github.com/ppy/osu.git synced 2024-11-15 16:27:43 +08:00
osu-lazer/osu.Game.Rulesets.Osu/Objects/Drawables/DrawableOsuHitObject.cs
Bartłomiej Dach 89509ea49e
Fix DrawableOsuHitObject not properly cleaning up dim application callbacks
Should fix https://github.com/ppy/osu/issues/28629.

First of all, to support the claim that this does fix the issue -
reproduction is rather difficult, but I believe I found a way to
maximise the chances of it reproducing by performing the following
steps:

1. Apply the following diff:

diff --git a/osu.Game.Rulesets.Osu/Objects/Drawables/DrawableSlider.cs b/osu.Game.Rulesets.Osu/Objects/Drawables/DrawableSlider.cs
index eacd2b3e75..4c00da031a 100644
--- a/osu.Game.Rulesets.Osu/Objects/Drawables/DrawableSlider.cs
+++ b/osu.Game.Rulesets.Osu/Objects/Drawables/DrawableSlider.cs
@@ -6,6 +6,7 @@
 using System;
 using System.Collections.Generic;
 using System.Linq;
+using System.Threading;
 using JetBrains.Annotations;
 using osu.Framework.Allocation;
 using osu.Framework.Bindables;
@@ -95,6 +96,8 @@ public DrawableSlider([CanBeNull] Slider s = null)
         [BackgroundDependencyLoader]
         private void load()
         {
+            Thread.Sleep(100);
+
             tailContainer = new Container<DrawableSliderTail> { RelativeSizeAxes = Axes.Both };

             AddRangeInternal(new Drawable[]

2. Download https://osu.ppy.sh/beatmapsets/1470790#osu/3023028 and open
   it in the editor.

3. Select all objects using Ctrl-A. Yes, it'll take a while, especially
   so with the patch above.

4. Rotate the selection by any amount using the right toolbox.

5. Press undo. The game should crash. If it doesn't spam redo and undo
   until it does.

Now to explain what the fix is.

In the issue thread I spent a considerable time hemming and hawing about
which of the dimmable pieces was null, which was a complete miss and a
failure at reading. Let's see the stack trace again:

	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

Line 101, you say? What could be null here?

	bd8addfb5f/osu.Game.Rulesets.Osu/Objects/Drawables/DrawableOsuHitObject.cs (L101)

Okay, what's `InitialLifetimeOffset`, then?

	bd8addfb5f/osu.Game.Rulesets.Osu/Objects/Drawables/DrawableOsuHitObject.cs (L108)

Yes, that's right. It's `HitObject` that is null here.

Now why does *that* happen? First, let's note that all stacks where this
died went through `UpdateState()`, which means that the problematic
`applyDim()` calls had to be `ApplyCustomUpdateState` event callbacks.
Meaning that the pieces where `HitObject` was null were DHOs themselves.

Recall that parent DHOs and child DHOs are pooled separately. Therefore,
there is no guarantee that any parent and child DHOs will remain
associated with each other for the entire duration of a gameplay
session; it is quite the contrary, and nobody should rely on that.
Unfortunately for us, adding a `applyDimToDrawableHitObject` callback to
a child object's `ApplyCustomUpdateState` *implicitly creates* such an
association, because it ends up allocating a closure that captures
`this` (meaning the parent in this context).

Therefore, this now creates a situation where a child DHO can attempt to
read state from a former parent DHO which can be in an indeterminate
state, and in fact, when this crashes, the former parent DHO is most
likely not even in use - hence the null `HitObject`.

Thus, the fix is to clear the association by unsubscribing from the
event when nested objects are cleared.

My hypothesis why the reproduction scenario is like it is, is that both
the sleep and the increased pressure on the pool (by way of selecting
all objects and therefore forcing the DHOs to be materialised beyond
pool capacity) increases the likelihood of getting a crosslink.
When pool pressure is low, it is much more likely that a parent DHO
*will* get the same child DHO again on re-application, even though
that is not guaranteed.

Just as an additional detail, note that the sentry issue for this lists
the "first seen" version as 2024.312.0, which is the release that
included https://github.com/ppy/osu/pull/27401 which would be directly
responsible for this mess.
2024-09-19 14:30:02 +02:00

154 lines
7.0 KiB
C#

// 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.
#nullable disable
using System;
using System.Collections.Generic;
using System.Linq;
using osu.Framework.Allocation;
using osu.Framework.Bindables;
using osu.Framework.Graphics;
using osu.Framework.Graphics.Primitives;
using osu.Game.Rulesets.Judgements;
using osu.Game.Rulesets.Objects.Drawables;
using osu.Game.Rulesets.Osu.Judgements;
using osu.Game.Rulesets.Osu.Scoring;
using osu.Game.Rulesets.Osu.UI;
using osu.Game.Rulesets.Scoring;
using osuTK;
using osuTK.Graphics;
namespace osu.Game.Rulesets.Osu.Objects.Drawables
{
public abstract partial class DrawableOsuHitObject : DrawableHitObject<OsuHitObject>
{
public readonly IBindable<Vector2> PositionBindable = new Bindable<Vector2>();
public readonly IBindable<int> StackHeightBindable = new Bindable<int>();
public readonly IBindable<float> ScaleBindable = new BindableFloat();
public readonly IBindable<int> IndexInCurrentComboBindable = new Bindable<int>();
// Must be set to update IsHovered as it's used in relax mod to detect osu hit objects.
public override bool HandlePositionalInput => true;
protected override float SamplePlaybackPosition => CalculateDrawableRelativePosition(this);
/// <summary>
/// What action this <see cref="DrawableOsuHitObject"/> should take in response to a
/// click at the given time value.
/// If non-null, judgements will be ignored for return values of <see cref="ClickAction.Ignore"/>
/// and <see cref="ClickAction.Shake"/>, and this hit object will be shaken for return values of
/// <see cref="ClickAction.Shake"/>.
/// </summary>
public Func<DrawableHitObject, double, HitResult, ClickAction> CheckHittable;
protected DrawableOsuHitObject(OsuHitObject hitObject)
: base(hitObject)
{
}
[BackgroundDependencyLoader]
private void load()
{
Alpha = 0;
}
protected override void OnApply()
{
base.OnApply();
IndexInCurrentComboBindable.BindTo(HitObject.IndexInCurrentComboBindable);
PositionBindable.BindTo(HitObject.PositionBindable);
StackHeightBindable.BindTo(HitObject.StackHeightBindable);
ScaleBindable.BindTo(HitObject.ScaleBindable);
}
protected override void OnFree()
{
base.OnFree();
IndexInCurrentComboBindable.UnbindFrom(HitObject.IndexInCurrentComboBindable);
PositionBindable.UnbindFrom(HitObject.PositionBindable);
StackHeightBindable.UnbindFrom(HitObject.StackHeightBindable);
ScaleBindable.UnbindFrom(HitObject.ScaleBindable);
}
protected virtual IEnumerable<Drawable> DimmablePieces => Enumerable.Empty<Drawable>();
protected override void UpdateInitialTransforms()
{
base.UpdateInitialTransforms();
foreach (var piece in DimmablePieces)
{
// if the specified dimmable piece is a DHO, it is generally not safe to tack transforms onto it directly
// as they may be cleared via the `updateState()` DHO flow,
// so use `ApplyCustomUpdateState` instead. which does not have this pitfall.
if (piece is DrawableHitObject drawableObjectPiece)
{
// this method can be called multiple times, and we don't want to subscribe to the event more than once,
// so this is what it is going to have to be...
drawableObjectPiece.ApplyCustomUpdateState -= applyDimToDrawableHitObject;
drawableObjectPiece.ApplyCustomUpdateState += applyDimToDrawableHitObject;
}
// but at the end apply the transforms now regardless of whether this is a DHO or not.
// the above is just to ensure they don't get overwritten later.
applyDim(piece);
}
}
protected override void ClearNestedHitObjects()
{
base.ClearNestedHitObjects();
// any dimmable pieces that are DHOs will be pooled separately.
// `applyDimToDrawableHitObject` is a closure that implicitly captures `this`,
// and because of separate pooling of parent and child objects, there is no guarantee that the pieces will be associated with `this` again on re-use.
// therefore, clean up the subscription here to avoid crosstalk.
// not doing so can result in the callback attempting to read things from `this` when it is in a completely bogus state (not in use or similar).
foreach (var piece in DimmablePieces.OfType<DrawableHitObject>())
piece.ApplyCustomUpdateState -= applyDimToDrawableHitObject;
}
private void applyDim(Drawable piece)
{
piece.FadeColour(new Color4(195, 195, 195, 255));
using (piece.BeginDelayedSequence(InitialLifetimeOffset - OsuHitWindows.MISS_WINDOW))
piece.FadeColour(Color4.White, 100);
}
private void applyDimToDrawableHitObject(DrawableHitObject dho, ArmedState _) => applyDim(dho);
protected sealed override double InitialLifetimeOffset => HitObject.TimePreempt;
private OsuInputManager osuActionInputManager;
internal OsuInputManager OsuActionInputManager => osuActionInputManager ??= GetContainingInputManager() as OsuInputManager;
/// <summary>
/// Shake the hit object in case it was clicked far too early or late (aka "note lock").
/// </summary>
public virtual void Shake() { }
/// <summary>
/// Causes this <see cref="DrawableOsuHitObject"/> to get hit, disregarding all conditions in implementations of <see cref="DrawableHitObject.CheckForResult"/>.
/// </summary>
public void HitForcefully() => ApplyMaxResult();
/// <summary>
/// Causes this <see cref="DrawableOsuHitObject"/> to get missed, disregarding all conditions in implementations of <see cref="DrawableHitObject.CheckForResult"/>.
/// </summary>
public void MissForcefully() => ApplyMinResult();
private RectangleF parentScreenSpaceRectangle => ((DrawableOsuHitObject)ParentHitObject)?.parentScreenSpaceRectangle ?? Parent!.ScreenSpaceDrawQuad.AABBFloat;
/// <summary>
/// Calculates the position of the given <paramref name="drawable"/> relative to the playfield area.
/// </summary>
/// <param name="drawable">The drawable to calculate its relative position.</param>
protected float CalculateDrawableRelativePosition(Drawable drawable) => (drawable.ScreenSpaceDrawQuad.Centre.X - parentScreenSpaceRectangle.X) / parentScreenSpaceRectangle.Width;
protected override JudgementResult CreateResult(Judgement judgement) => new OsuJudgementResult(HitObject, judgement);
}
}