Skip to content

Fix for AudioController.Update() at 2d game tutorial. #148

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

Merged
merged 3 commits into from
Jun 26, 2025

Conversation

nahaharo
Copy link
Contributor

Fix for the MonoGame/MonoGame#8851

This code fixed two things in AudiroController.Update() at 2D game tutorial.

  1. move "_activeSoundEffectInstances.RemoveAt(index)" into if block.
  2. add index++ at the end of while statement.
    public void Update()
    {
        int index = 0;
        while (index < _activeSoundEffectInstances.Count)
        {
            SoundEffectInstance instance = _activeSoundEffectInstances[index];

            if (instance.State == SoundState.Stopped && !instance.IsDisposed)
            {
                instance.Dispose();
                _activeSoundEffectInstances.RemoveAt(index);
            }
            index++;
        }
    }

@ThomasFOG
Copy link
Contributor

I think there is still an issue with this loop. If you remove an element from the list, you shouldn't increase the index, otherwise you would skip an element.

    public void Update()
    {
        int index = 0;
        while (index < _activeSoundEffectInstances.Count)
        {
            SoundEffectInstance instance = _activeSoundEffectInstances[index];

            if (instance.State == SoundState.Stopped && !instance.IsDisposed)
            {
                instance.Dispose();
                _activeSoundEffectInstances.RemoveAt(index);
            }
            else
                index++;
        }
    }

Alternatively, you can iterate backward to simplify this and have a predictable loop:

    public void Update()
    {
        int index = 0;
        for (int i = _activeSoundEffectInstances.Count - 1; i >= 0; i--)
        {
            SoundEffectInstance instance = _activeSoundEffectInstances[i];

            if (instance.State == SoundState.Stopped && !instance.IsDisposed)
            {
                instance.Dispose();
                _activeSoundEffectInstances.RemoveAt(i);
            }
        }
    }

@nahaharo
Copy link
Contributor Author

I think backward iteration is a better solution.
Gemini suggests an improvement to this code.
I think this version is a little long, but more appropriate.
Can I commit this version?

public void Update()
{
    for (int i = _activeSoundEffectInstances.Count - 1; i >= 0; i--)
    {
        SoundEffectInstance instance = _activeSoundEffectInstances[i];

        if (instance.State == SoundState.Stopped)
        {
            if (!instance.IsDisposed)
            {
                instance.Dispose();
            }
            _activeSoundEffectInstances.RemoveAt(i);
        }
    }
}

@SimonDarksideJ
Copy link
Collaborator

I think backward iteration is a better solution. Gemini suggests an improvement to this code. I think this version is a little long, but more appropriate. Can I commit this version?

public void Update()
{
    for (int i = _activeSoundEffectInstances.Count - 1; i >= 0; i--)
    {
        SoundEffectInstance instance = _activeSoundEffectInstances[i];

        if (instance.State == SoundState.Stopped)
        {
            if (!instance.IsDisposed)
            {
                instance.Dispose();
            }
            _activeSoundEffectInstances.RemoveAt(i);
        }
    }
}

Yes, this version would be more preferable, please update @nahaharo

nahaharo pushed a commit to nahaharo/MonoGame.Samples that referenced this pull request Jun 25, 2025
@SimonDarksideJ SimonDarksideJ merged commit 65e2f64 into MonoGame:main Jun 26, 2025
2 checks passed
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