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 memory leak in PlayGlobal when filter is empty #5549

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

ewokswagger
Copy link

Short circuits when the player filter is empty before the SetupAudio entity is made.

Copy link
Member

@PJB3005 PJB3005 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Surely you should add this to the other APIs that are running AddAudioFilter too?

Also, can you elaborate on how this fixes a memory leak? The change seems fine to me on its own (is it possibly any code can freak out at the sudden null return from this function?), but I'm not sure what memory leak this fixes.

@ewokswagger
Copy link
Author

Is was at least indicated to be the case in the entity integration tests. I didn't test if this also happens in live environment since its a bit harder to test for, it may just have to do with the integration test itself.

@ewokswagger
Copy link
Author

This is the pr that caused this behavior to arise. DeltaV-Station/Delta-v#2351 Locally running with this change fixes the test failure.

@slarticodefast
Copy link
Member

Could it be that this PR also fixes sounds like the WEWLAD, zombie infection or other antag specific ones that use PlayGlobal being heard for every player in replays? They are restricted to a certain player session but that does not seem to get filtered in replays correctly.

@PJB3005
Copy link
Member

PJB3005 commented Dec 6, 2024

Could it be that this PR also fixes sounds like the WEWLAD, zombie infection or other antag specific ones that use PlayGlobal being heard for every player in replays? They are restricted to a certain player session but that does not seem to get filtered in replays correctly.

Unlikely.

@PJB3005
Copy link
Member

PJB3005 commented Dec 6, 2024

This is the pr that caused this behavior to arise. DeltaV-Station/Delta-v#2351 Locally running with this change fixes the test failure.

The problem here seems to be that SpawnAndDeleteEntityCountTest should ignore Audio components. Paging @ElectroJr to see if that's valid

@ewokswagger
Copy link
Author

Do you guys still want the change even if its not fixing a memory leak? Or should I close?

@ElectroJr
Copy link
Member

Do you guys still want the change even if its not fixing a memory leak? Or should I close?

I agree with PJB that if the change is made, it should also be made to the other audio methods for consistency (e.g., the PlayEntity() method directly beneath the one you changed).

The problem here seems to be that SpawnAndDeleteEntityCountTest should ignore Audio components. Paging @ElectroJr to see if that's valid

The test was created before audio-entities existed and was never updated. I guess it should be updated to ignore all audio entities. Though even with that change the test is still pretty janky and I'm not sure if it should exist. It was a crude test meant to try catch some problematic entities that accidentally dump other entities on the floor or spawn null-space ents that aren't cleaned up when the entity that spawned them gets deleted, but there are always going to be quite a few entities where the test just isn't applicable.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants