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

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

Conversation

nahaharo
Copy link

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
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);
        }
    }
}

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.

3 participants