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

Ensure audio filters can't be attached before load (or post-disposal) #27226

Merged
merged 1 commit into from
Feb 19, 2024

Conversation

peppy
Copy link
Member

@peppy peppy commented Feb 18, 2024

Will probably fix #27225?

@peppy peppy added the audio label Feb 18, 2024
@frenzibyte frenzibyte self-requested a review February 18, 2024 22:22
@frenzibyte
Copy link
Member

frenzibyte commented Feb 18, 2024

This doesn't look like it would help. The only game usage of the Cutoff property is via transforms, and all transforms are being processed by the AudioFilter components themselves, so I would imagine they would never run post disposal, unless something is fundamentally wrong with the drawable hierarchy (which I hope is not the case?).

I'm wondering if async disposal has something to do with this, like the update thread processing through the AudioFilter.UpdateSubTree() method after Dispose has been called already...I'm not really sure if that's the case (I can't say I know async disposal too well), but if what I'm saying has some sense in it, I think you could guard against that by never allowing an audio filter to be attached after Dispose is called (right now in this PR as long as Update/UpdateSubTree is ever called, an audio filter will get attached).

EDIT: Now that I look at it again (I didn't pay enough attention at the title), I realised AudioFilter on master will be attached as soon as the component is created, which is something that this PR fixes, so I take back my very first sentence here.

@peppy
Copy link
Member Author

peppy commented Feb 19, 2024

unless something is fundamentally wrong with the drawable hierarchy (which I hope is not the case?).

as you seem to have figured out, cutoff being set in BDL can definitely cause this, and we've seen it happen before. it could run after disposal.

maybe just edit out the part of your comment completely rather than having us read 3 paragraphs of outdated minddump ;)

@frenzibyte
Copy link
Member

I'm not sure. Disposal running after BDL makes sense, but I can't see how it applies here. For that to occur, the initial cutoff of an AudioFilter must be set to a value that has an effect on the audio, and the only use case that matches this criteria is the following line in PlayerLoader:

highPassFilter.CutoffTo(300).Then().CutoffTo(0, 1250); // 1250 is to line up with the appearance of MetadataInfo (750 delay + 500 fade-in)

But then, that means PlayerLoader.OnEntering would have been called while the screen is disposed for the entire issue to occur, right? But that event is invoked when Screen.LoadComplete is invoked first, so the pieces aren't connecting here.

I'm willing to disregard looking into this any further and call it "async disposal weirdness" as the changes in this PR are valid to my eyes already and I'm not sure sparring a discussion for this is worth anyone's time right now.

@bdach
Copy link
Collaborator

bdach commented Feb 19, 2024

Maybe also does something for #24100?

@bdach bdach merged commit d7b1e3b into ppy:master Feb 19, 2024
15 of 17 checks passed
@peppy peppy deleted the no-audio-filter-funny-business branch February 19, 2024 14:50
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.

Song audio gets really muffled overtime in lazer
3 participants