ppy / osu

rhythm is just a *click* away!
https://osu.ppy.sh
MIT License
14.92k stars 2.2k forks source link

Exiting the manage collections screen while editing an existing collection leaves the low-pass filter active #24100

Closed Bruno5430 closed 6 months ago

Bruno5430 commented 1 year ago

Type

Game behaviour

Bug description

Screenshots or videos

2023-07-01 16-37-48.webm

Version

2023.621.0

Logs

-

Joehuu commented 1 year ago

@Bruno5430 can you reliably reproduce this with the steps you provided? Can't seem to reproduce well. I only hit this once after 5 minutes of trying.

Something has gone really wrong with the PopOut() call (FadeOut() and ScaleTo() animations don't happen in the video too): https://github.com/ppy/osu/blob/7b3cb5b410f653e966c29167dbbc7d039fd3c020/osu.Game/Collections/ManageCollectionsDialog.cs#L122-L133

Bruno5430 commented 1 year ago

@Bruno5430 can you reliably reproduce this with the steps you provided? Can't seem to reproduce well. I only hit this once after 5 minutes of trying.

Yeah i can, but i should clarify some conditions

  • Exit the manage collections screen by either pressing Esc or clicking outside of the screen

Pressing Esc unselects the textbox and doesn't leave the filter active, i will edit it in op

  • Select the title of any existing collection (it's not necessary to be modified)

It has to be an existing collection (It's not necessary to have any beatmaps), It doesn't work if you select the "Create new collection" textbox

Also I'm in linux, so idk if It's platform specific or locale specific (I't's on es_AR)

Bruno5430 commented 1 year ago

Ok, it isn't entirely consistent but it's still very common and the conditions i noted before still apply 2023-07-02 15-58-02.webm

This time submitting clean logs from the session shown in the video legacy-ipc.log performance-draw.log performance-update.log performance-audio.log performance-input.log database.log input.log performance.log network.log runtime.log

peppy commented 1 year ago

I have no idea how this can happen.

A starting point is to figure out what state the overlay is in when in this state. Somehow it is not visible but skipped all state change logic (so State.Value must either still be visible, or it got dismissed so brutally that it couldn't execute the callbacks / animations).

I can't seem to repro locally, so if someone manages it, use the draw visualiser to examine the ManageCollectionsDialog's state.

Bruno5430 commented 1 year ago

Apparently the overlay does hide, but the animations do this:

2023-07-04 17-12-17.webm

peppy commented 6 months ago

@Bruno5430 please re-test this in the next release (coming today or tomorrow) thanks!

Bruno5430 commented 6 months ago

Still happens

2024-02-19 19-25-23.webm

frenzibyte commented 6 months ago

The key to this happening is probably in the sudden spike you're getting while trying to exit the manage collections screen.

frenzibyte commented 6 months ago

Thanks to the information in https://github.com/ppy/osu/issues/24100#issuecomment-1620716473, and in addition to what I observed above, it's very likely that the issue is happening from the following scenario:

  1. ManageCollectionsDialog.PopOut adds transformation for alpha, scale, and AudioFilter transforming cutoff
  2. Game gets frozen for whatever reason (as can be noticed in all of the videos attached above).
  3. Game regains control, and on the next iteration of ManageCollectionsDialog.UpdateSubTree, both alpha and scale transforms are processed to final value immediately (since time has passed already)
  4. ManageCollectionsDialog becomes non-present after processing the alpha transform, so it doesn't update its children due to this specific conditional, leaving AudioFilter's transform stuck indefinitely until the dialog is visible again.

I can replicate this as well by putting the update thread to ~300ms sleep in PopOut after all transforms are added.

This questions the friendliness between the AudioFilter component and the rest of the drawable system, especially due to the conditional I linked in point 4. CompositeDrawable assumes that if the parent drawable already became non-present, then there's no point to process children anymore since it doesn't matter visually (everything will be hidden), but AudioFilter is an audible component, therefore being an exception to that assumption.

I'm not quite sure what's the best approach here, but this undoubtedly shows a flaw in the nature of AudioFilter with the rest of the drawable system. I had thought of a separate system where audio filters are managed by a general component and the cutoff values are managed by a bindable that's transformed by the drawable class that controls its presence (i.e. ManageCollectionsDialog doing the bindable transform using this.TransformBindableTo), but as I try to pursue through this direction, I feel further unsure as to whether it's a good direction or not, as it comes with its own levels of complexity, and I'm frankly not that good at designing a reliable system.

For the time being, we can easily work around this issue by either making ManageCollectionsDialog always present using AlwaysPresent = true, or override the IsPresent boolean to be true when the filter has a non-default cutoff.

bdach commented 6 months ago

I'd probably do something like override RequiresChildrenUpdate on the collections dialog and call it a day.

frenzibyte commented 6 months ago

If we continue to use AudioFilter in more places, we're very likely to hit multiple kinds of this issue in those places.

bdach commented 6 months ago

We work around presence weirdness in so many places now that I consider that argument to be mostly moot.

frenzibyte commented 6 months ago

It's not about workarounds, it's about predicting how the transform can fail to be processed to avoid that from occurring with the user, because again, AudioFilter is an audible component. If presence is taken away, audio will not return back to normal (unlike normal drawables where loss of presence means nothing displayed)

bdach commented 6 months ago

No I find that presence is the core issue here and don't see this as any different to any e.g. scheduling issue caused by presence wonkiness. The only difference that the breakage is audible and not visible.

arialp commented 6 months ago

I experienced the same issue today for build 2024.221.0-lazer and although it looks like you found potential fixes, I would like to add my findings.

While creating a new collection via right click on beatmap > Collections > Manage and then clicking off the menu to go back, the game occasionally stutters, causing the beatmap music to stay muffled everywhere in-game. I was able to reproduce this issue at times: 0:14, 1:06, and 1:26 in the uploaded video. I also noticed that longer and more complex collection names cause higher probability of stutters upon leaving the menu, and subsequently, higher reproducibility of the bug. The temporary fix is to simply open the aforementioned menu again and close without doing anything.

I installed osu!lazer on a spinning HDD and thought this was the main cause as I experience frequent stutters in song select already because of a slow disk.

Logs: compressed-logs.zip

https://github.com/ppy/osu/assets/36759240/694a4b87-c032-43d4-bcd8-1f14d06a9dd7