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 slider tails sometimes not dimming correctly #27401
Conversation
Originally noticed during review of another change: ppy#27369 (comment). `DrawableOsuHitObject` tries to solve the initial dimming of objects by applying transform to a list of dimmable parts. For plain drawables this is safe, but if one of the parts is a DHO, it is not safe, because drawable transforms can be cleared at will. In particular, on first use of a drawable slider, `UpdateInitialTransforms()` would fire via `LoadComplete()` on the `DrawableSlider`, but *then*, also via `LoadComplete()`, the `DrawableSliderTail` would update its own state and by doing so inadvertently clear the dim transform just added by the slider. To fix, ensure dim transforms are applied to DHOs via `ApplyCustomUpdateState`.
// 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) | ||
drawableObjectPiece.ApplyCustomUpdateState += (dho, _) => applyDim(dho); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, my concern is that UpdateInitialTransforms
seems like it can be called more than once, which would bind this each time.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's a good spot but I don't necessarily know what the best option forward is here.
It's either this atrocity:
diff --git a/osu.Game.Rulesets.Osu/Objects/Drawables/DrawableOsuHitObject.cs b/osu.Game.Rulesets.Osu/Objects/Drawables/DrawableOsuHitObject.cs
index 35cab6459b..2e2831f9ae 100644
--- a/osu.Game.Rulesets.Osu/Objects/Drawables/DrawableOsuHitObject.cs
+++ b/osu.Game.Rulesets.Osu/Objects/Drawables/DrawableOsuHitObject.cs
@@ -85,11 +85,16 @@ protected override void UpdateInitialTransforms()
// 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)
- drawableObjectPiece.ApplyCustomUpdateState += (dho, _) => applyDim(dho);
+ {
+ drawableObjectPiece.ApplyCustomUpdateState -= applyDimToDrawableObject;
+ drawableObjectPiece.ApplyCustomUpdateState += applyDimToDrawableObject;
+ }
else
applyDim(piece);
}
+ void applyDimToDrawableObject(DrawableHitObject drawableHitObject, ArmedState _) => applyDim(drawableHitObject);
+
void applyDim(Drawable piece)
{
piece.FadeColour(new Color4(195, 195, 195, 255));
or splitting the application in two for DHOs and non-DHOs? which also feels pretty crap...
diff --git a/osu.Game.Rulesets.Osu/Objects/Drawables/DrawableOsuHitObject.cs b/osu.Game.Rulesets.Osu/Objects/Drawables/DrawableOsuHitObject.cs
index 35cab6459b..6bdf7ca82b 100644
--- a/osu.Game.Rulesets.Osu/Objects/Drawables/DrawableOsuHitObject.cs
+++ b/osu.Game.Rulesets.Osu/Objects/Drawables/DrawableOsuHitObject.cs
@@ -75,29 +75,43 @@ protected override void OnFree()
protected virtual IEnumerable<Drawable> DimmablePieces => Enumerable.Empty<Drawable>();
- protected override void UpdateInitialTransforms()
+ protected override void LoadComplete()
{
- base.UpdateInitialTransforms();
+ base.LoadComplete();
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)
- drawableObjectPiece.ApplyCustomUpdateState += (dho, _) => applyDim(dho);
- else
- applyDim(piece);
+ if (piece is not DrawableHitObject drawableObjectPiece)
+ continue;
+
+ drawableObjectPiece.ApplyCustomUpdateState += (dho, _) => applyDim(dho);
}
+ }
- void applyDim(Drawable piece)
+ protected override void UpdateInitialTransforms()
+ {
+ base.UpdateInitialTransforms();
+
+ foreach (var piece in DimmablePieces)
{
- piece.FadeColour(new Color4(195, 195, 195, 255));
- using (piece.BeginDelayedSequence(InitialLifetimeOffset - OsuHitWindows.MISS_WINDOW))
- piece.FadeColour(Color4.White, 100);
+ // see `LoadComplete()`.
+ if (piece is DrawableHitObject)
+ continue;
+
+ applyDim(piece);
}
}
+ 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);
+ }
+
protected sealed override double InitialLifetimeOffset => HitObject.TimePreempt;
private OsuInputManager osuActionInputManager;
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the former diff works it makes most sense to me, I would go with that to keep the ball rolling.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Have applied it. I tend to agree it's the less bad option.
Originally noticed during review of another change: #27369 (comment).
DrawableOsuHitObject
tries to solve the initial dimming of objects by applying transform to a list of dimmable parts. For plain drawables this is safe, but if one of the parts is a DHO, it is not safe, because drawable transforms can be cleared at will.In particular, on first use of a drawable slider,
UpdateInitialTransforms()
would fire viaLoadComplete()
on theDrawableSlider
, but then, also viaLoadComplete()
, theDrawableSliderTail
would update its own state and by doing so inadvertently clear the dim transform just added by the slider.To fix, ensure dim transforms are applied to DHOs via
ApplyCustomUpdateState
.Untitled.mp4