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 taiko swell ending samples playing at results sometimes #32085

Merged
merged 1 commit into from
Feb 25, 2025

Conversation

bdach
Copy link
Collaborator

@bdach bdach commented Feb 24, 2025

Closes #32052.

Sooooo... this is going to be a rant...

To understand why this is going to require a rant, dear reader, please do the following:

  1. Read the issue thread and follow the reproduction scenario (download map linked, fire up autoplay, seek near end, wait for results, hear the sample spam).
  2. Now exit out to song select, hide the toolbar, and attempt reproducing the issue again.
  3. Depending on ambient mood, laugh or cry.

Now, why on earth would the TOOLBAR have any bearing on anything?

Well, the chain of failure is something like this:

  • The toolbar hides for the duration of gameplay, naturally.
  • When progressing to results, the toolbar gets automatically unhidden.
  • This triggers invalidations on ScrollingHitObjectContainer. I'm not precisely sure which property it is that triggers the invalidations, but one clearly does. It may be position or size or whichever.
  • When the invalidation is triggered on layoutCache, the next Update() call is going to recompute lifetimes for ALL hitobject entries.
  • In case of swells, it happens that the calculated lifetime end of the swell is larger than what it actually ended up being determined as at the instant of judging the swell, and thus, the swell is resurrected, reassigned a DHO, and the DHO calls UpdateState() and plays the sample again despite the samplePlayed flag in LegacySwell, because that flag is ephemeral state that does not survive a hitobject getting resurrected.

Now I could just fix this locally to the swell, maybe, by having some time lenience check, but the fact that hitobjects can be resurrected by the toolbar appearing, of all possible causes in the world, feels just completely wrong. So I'm adding a local check in SHOC to not overwrite lifetime ends of judged object entries.

The reason why I'm making that check specific to end time is that I can see valid reasons why you would want to recompute lifetime start even on a judged object (such as playfield geometry changing in a significant way). I can't think of a valid reason to do that to lifetime end.

Closes ppy#32052.

Sooooo... this is going to be a rant...

To understand why this is going to require a rant, dear reader, please
do the following:

1. Read the issue thread and follow the reproduction scenario (download
   map linked, fire up autoplay, seek near end, wait for results, hear
   the sample spam).
2. Now exit out to song select, *hide the toolbar*, and attempt
   reproducing the issue again.
3. Depending on ambient mood, laugh or cry.

Now, *why on earth* would the *TOOLBAR* have any bearing on anything?

Well, the chain of failure is something like this:

- The toolbar hides for the duration of gameplay, naturally.
- When progressing to results, the toolbar gets automatically unhidden.
- This triggers invalidations on `ScrollingHitObjectContainer`. I'm not
  precisely sure which property it is that triggers the invalidations,
  but one clearly does. It may be position or size or whichever.
- When the invalidation is triggered on `layoutCache`, the next
  `Update()` call is going to recompute lifetimes for ALL hitobject
  entries.
- In case of swells, it happens that the calculated lifetime end of the
  swell is larger than what it actually ended up being determined as at
  the instant of judging the swell, and thus, the swell is *resurrected*,
  reassigned a DHO, and the DHO calls `UpdateState()` and plays the
  sample again despite the `samplePlayed` flag in `LegacySwell`, because
  all of that is ephemeral state that does not survive a hitobject
  getting resurrected.

Now I *could* just fix this locally to the swell, maybe, by having some
time lenience check, but the fact that hitobjects can be resurrected by
the *toolbar* appearing, of all possible causes in the world, feels
just completely wrong. So I'm adding a local check in SHOC to not
overwrite lifetime ends of judged object entries.

The reason why I'm making that check specific to end time is that I can
see valid reasons why you would want to recompute lifetime *start* even
on a judged object (such as playfield geometry changing in a significant
way). I can't think of a valid reason to do that to lifetime *end*.
@bdach bdach added ruleset/osu!taiko area:gameplay audio next release Pull requests which are almost there. We'll aim to get them in the next release, but no guarantees! labels Feb 24, 2025
@bdach bdach requested a review from smoogipoo February 24, 2025 14:38
@bdach bdach self-assigned this Feb 24, 2025
@smoogipoo smoogipoo merged commit 820821d into ppy:master Feb 25, 2025
8 of 10 checks passed
@bdach bdach deleted the i-dont-even branch February 25, 2025 06:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:gameplay audio next release Pull requests which are almost there. We'll aim to get them in the next release, but no guarantees! ruleset/osu!taiko size/XS
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Extra spinner-osu samples play if a swell is at the end of a map.
2 participants