How to fix these Unity errors?

Question

I recently switched out some arrays for Lists in most of my scripts as they are easier to use. The problem though is that brought out some new errors. They are :

1. InvalidOperationException: Collection was modified; enumeration operation may not execute.
2. ArgumentOutOfRangeException: Index was out of range. Must be non-negative and less than the size of the collection.

According to the errors, the problem is on line 96 of the Gorean Turret and line 37 of the POW_Camp. A couple other scrips also have that issue, but if I figure out what's wrong with these two I should be able to figure out how to fix the rest.

I searched up what to do about these errors and found a couple solutions. I changed the foreach loops to for (var i=0;) style loops. This was supposed to work, but unfortunately not.

My problem is how do I fix these errors. How do I need to change my scripts so there are no errors. As a side note, if you find something that can be shortened, please tell me. I have a feeling that the code is also longer than it needs to be.

Here is the POW_Camp Script :

using System.Collections;
using System.Collections.Generic;
using UnityEngine;

public class POW_Camp : MonoBehaviour
{
    public GameObject AmmunitionsDump;
    public GameObject Explosion;
    public GameObject Fences;
    public List<GameObject> Prisoners;

    // Start is called before the first frame update
    void Start()
    {
        
    }

    // Update is called once per frame
    void Update()
    {
        if (AmmunitionsDump.GetComponent<Health>().CurrentHealth<=0)
        {
            AmmunitionsDump.SetActive(false);
            Explosion.SetActive(true);
        }
        //If the ammunitiondump is destroyed then destroy fences
        if (AmmunitionsDump.activeSelf == false)
        {
            Destroy(Fences);
            //Activate POWs
            for (var i=0;i<Prisoners.Count; i++)
            {
                if (Prisoners[i] == null)
                {
                    Prisoners.Remove(Prisoners[i]);
                }
                if (Prisoners[i] != null)
                {
                    Prisoners[i].GetComponent<PlayerData>().Captured = false;
                }
            }
        }
    }
}

Here is the GoreanTurret Script:

using System.Collections;
using System.Collections.Generic;
using UnityEngine;
using UnityEngine.UI;

public class GoreanTurret : MonoBehaviour
{
    public GameObject Projectile;
    public GameObject FiringPoint;
    public GameObject GunTower;

    public GameObject HealthBar;
    public List<GameObject> TargetsWithinRange = new List<GameObject>();
    public float FiringRange = 100;
    public GameObject CurrentTarget;
    public bool TargetLocked;

    // Start is called before the first frame update
    void Start()
    {
        InvokeRepeating("Shoot", 1, 0.5f);
        HealthBar.GetComponent<Slider>().maxValue = gameObject.GetComponent<Health>().MaxHealth;
        HealthBar.GetComponent<Slider>().value = gameObject.GetComponent<Health>().CurrentHealth;
    }

    // Update is called once per frame
    void Update()
    {
        FindTargetsWithinRange(FiringRange, TargetsWithinRange);
        TargetManager(TargetsWithinRange, FiringRange);
        HealthBar.GetComponent<Slider>().value = gameObject.GetComponent<Health>().CurrentHealth;
        //Look at Target if Target is Locked
        if (TargetLocked)
        {
            GunTower.transform.LookAt(CurrentTarget.transform);
        }
        // If target is destroyed then set targetLocked to false and target to null;
        if (CurrentTarget != null)
        {
            if (CurrentTarget.GetComponent<Health>().CurrentHealth <= 0)
            {
                TargetLocked = false;
                CurrentTarget = null;
            }
        }
        // If one of the targets in the target list is destroyed then remove that
        foreach (GameObject possibleTarget in TargetsWithinRange)
        {
            if (possibleTarget == null || possibleTarget.GetComponent<Health>().CurrentHealth<=0 || possibleTarget.GetComponent<Health>()==null)
            {
                TargetsWithinRange.Remove(possibleTarget);
                
            }
        }
        if (CurrentTarget == null)
        {
            TargetLocked = false;
            CurrentTarget = null;

        }
        // If target is lost and there are still targets, then switch target
        if (TargetLocked == false && (CurrentTarget == null || CurrentTarget.GetComponent<Health>().CurrentHealth<0) && TargetsWithinRange.Count>0)
        {
            if (TargetsWithinRange[0] != null)
            {
                CurrentTarget = TargetsWithinRange[0];
                TargetLocked = true;
            }
        }
    }



    void FindTargetsWithinRange(float range, List<GameObject> TargetList)
    {
        Collider[] colliders = Physics.OverlapSphere(gameObject.transform.position, range);
        foreach (Collider collider in colliders)
        {
            if (collider.gameObject.GetComponent<PlayerData>())
            {
                if (collider.gameObject.GetComponent<PlayerData>().Captured == false && TargetList.Contains(collider.gameObject) == false)
                {
                    TargetList.Add(collider.gameObject);
                }
            }
        }
    }
    void TargetManager(List<GameObject> TargetList, float MaxRange)
    {
        for (var i = 0; i < TargetList.Count; i++)
        {
            //If the Target is null or inactive then remove that target or if it has no health
            if (TargetList[i] == null || TargetList[i].activeSelf == false || TargetList[i].GetComponent<Health>().CurrentHealth <= 0)
            {
                TargetList.Remove(TargetList[i]);
                if (TargetList[i] == CurrentTarget)
                {
                    TargetLocked = false;
                }
            }
            //If the target is too far
            if (Vector3.Distance(gameObject.transform.position, TargetList[i].transform.position) > MaxRange)
            {
                TargetList.Remove(TargetList[i]);
                if (TargetList[i] == CurrentTarget)
                {
                    CurrentTarget = null;
                    TargetLocked = false;
                }
            }
        }

        //If there is no target and the TargetLocked is false then Set a new target
        if (CurrentTarget == null && TargetLocked == false && TargetList.Count > 0)
        {
            CurrentTarget = TargetList[0];
            TargetLocked = true;
        }

        // If target is destroyed then set targetLocked to false and target to null;
        if (CurrentTarget != null && CurrentTarget.activeSelf == true && CurrentTarget.GetComponent<Health>().CurrentHealth > 0)
        {
            if (CurrentTarget.GetComponent<Health>().CurrentHealth <= 0)
            {
                TargetLocked = false;
                CurrentTarget = null;
            }
        }
    }

    public void Shoot()
    {
        if (TargetLocked == true)
        {
            Instantiate(Projectile, FiringPoint.transform.position, FiringPoint.transform.rotation);
        }
    }
}

Answer 1

In both your classes you have loops where you are removing elements from your collections while you are iterating over them.

The first one causes the ArgumentOutOfRangeException because after removing certain amount of elements you will iterate until reaching an index that is too high since your list/array is smaller now!

The other exception is pretty self-explanatory: You alter the collection while iterating over it with a foreach.

You can solve both cases using Linq Where in order to filter out the null entries (requires using System.Linq; on top of your file)

// in general use the bool operator of UnityEngine.Object
// never do check for "== null" !
Prisoners = Prisoners.Where(p =>  p).ToList();
// This basically equals doing something like
//var p = new List<GameObject>();
//foreach(var prisoner in Prisoners)
//{
//    if(prisoner) p.Add(prisoner);
//}
//Prisoners = p;

foreach (var prisoner in Prisoners.Count)
{
    prisoner.GetComponent<PlayerData>().Captured = false;
}

Exactly the same thing can be done in your second script

// so first again we use the bool operator to check if the element target "existed
// then directly use TryGetComponent instead of GetComponent twice
// your order to check also made little sense ;) you should first Check 
// the existence of the component before accessing a value
TargetsWithinRange = TargetsWithinRange.Where(target => target && target.TryGetComponent<Health>(out var health) && health.CurrentHealth > 0).ToList();

---

Alternatively if it is important that the collection itself stays the same reference you could use RemoveAll instead like

Prisoners.RemoveAll(prisoner => ! prisoner);

and accordingly in your other script

TargetsWithinRange.RemoveAll(target => !target || !target.TryGetComponent<Health>(out var h) || h.CurrentHealth <= 0);