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 slider head being incorrectly dimmed twice #27369

Merged
merged 1 commit into from
Feb 27, 2024

Conversation

Detze
Copy link
Contributor

@Detze Detze commented Feb 24, 2024

Resolves #25761.

Notice that the slider heads's approach circles are also dimmed in the footage in the issue. Slider heads are supposed to behave like hit circles, which dim their body, but not their approach circle.

The issue is that DrawableSlider dims its elements, including its HeadCircle, but DrawableSliderHead, inheriting from DrawableHitCircle, already dims itself (while also not dimming the approach circle), so the slider head is incorrectly being dimmed twice.

Thus, the solution is to remove the slider head from the list of slider elements to dim.

Not sure why it's an issue only most of the time (eg. from a few seconds into the map) and not always. The drawable hit object's state might not always be updating when it should? Either way, the addressed issue is gone now.

some data

In the footage, the cutoff point seems to be between the orange 5 and the orange 6. Times in the map:

  • offset: -16
  • first hit object: 18734
  • orange 5: 20984
  • orange 6: 21265

On my test map (offset 676) it would usually happen at 11497, but usually not at 11568.

Sometimes, moving the slider to a new time, or even just seeking away and back, would resolve or reintroduce the issue on the slider, or removing all objects before the tested slider and existing and re-entering the editor would resolve the issue. Changing offset didn't make a difference.

before after
before after

@bdach
Copy link
Collaborator

bdach commented Feb 27, 2024

Not sure why it's an issue only most of the time (eg. from a few seconds into the map) and not always.

There's something else there at play because I noticed when testing this that on some maps the slider tail won't dim correctly when it should. See attached video:

Untitled.mp4

Anyhow I'll look into that separately. This seems fine on its own.

bdach
bdach previously approved these changes Feb 27, 2024
@bdach bdach dismissed their stale review February 27, 2024 09:40

actually hold it, i'll investigate the tail issue first, because i have a feeling we may want to do something else here

@bdach
Copy link
Collaborator

bdach commented Feb 27, 2024

Upon further investigation tails not correctly dimming is a separate issue. On first use of every pooled object it would "fix" the double dim of the slider head, but break the single dim of the slider tail.

Working on fix, will be submitted separately.

@bdach bdach merged commit 087a2a7 into ppy:master Feb 27, 2024
15 of 17 checks passed
bdach added a commit to bdach/osu that referenced this pull request Feb 27, 2024
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`.
@Detze Detze deleted the incorrect-slider-head-dim branch February 29, 2024 20:49
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.

Object dimming not correctly applied to sliders
3 participants