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 OpenAlSound.Complete being incorrect when OpenAl source is reused. #19660

Merged
merged 2 commits into from Oct 1, 2021

Conversation

tovl
Copy link
Contributor

@tovl tovl commented Sep 10, 2021

I ran into this issue when trying to synchronize sequences to audio clips downstream. For that I needed to know when an audio clip has completed. When there are a lot of other sounds playing at the same time this would often go wrong.

The problem here is that OpenAlSound.Complete just looks at the status of the underlying OpenAl source. If another sound is started after the original sound has completed, the new sound might reuse the same underlying source. If the Complete method on the original sound is then evaluated again afterwards, it will return the status of the new sound instead of the original sound.

I added a simple boolean flag that gets set whenever the underlying source of the sound is reused, so that the Complete method will keep evaluating as true.

@Mailaender
Copy link
Member

This might fix #14153.

@RoosterDragon
Copy link
Member

This makes sense - reuse of sources would make OpenAlSound act on the wrong thing.

I think we may need to generalise this - if the source is going to be reclaimed, then OpenAlSound should handle that for any method, not just Completed. Maybe you could set Source to a sentential value and return early from any methods affected.

I notice things like the PauseSound method might also need adjusting. If you grab an outdated Source from a sound that is complete, then pausing might pause something else as well.

I think in the general case that might explain #14153 as @Mailaender noted - if things were pausing or setting volume of a sound and actually setting some other sound by accident.

@tovl
Copy link
Contributor Author

tovl commented Sep 28, 2021

I can't confirm that this is the root cause of #14153 but it wouldn't surprise me either.

In any case it makes sense to be thorough here. I've adjusted the other methods to short out when the source is freed for reuse as well.

Copy link
Member

@RoosterDragon RoosterDragon left a comment

Choose a reason for hiding this comment

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

Sounds seem fine and music was able to switch tracks without anything going off kilter.

FWIW I would be happy to close the hard-to-repro issue if this is merged as it seems like a good candidate to resolve it. We can reopen if this doesn't turn out to be the fix.

Copy link
Contributor

@reaperrr reaperrr left a comment

Choose a reason for hiding this comment

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

Looks good as far as I can tell and didn't encounter any in-game issues with sounds or music

@reaperrr reaperrr merged commit a428aaa into OpenRA:bleed Oct 1, 2021
@Smittytron Smittytron mentioned this pull request Oct 2, 2021
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.

None yet

5 participants