Adding And Removal from List Error Makes no sense.

So Here is the Problem, You Cant add and remove Units to a Specific List Under the Current Circumstances:

  1. Removing and adding in same function Input.GetKey(KeyCode.). if u use the same KeyCode
  2. Removing Using KeyCodeDown(KeyCode.) and Adding Using KeyCodeUp(KeyCode.), if its in the same function
  3. Removing Using KeyCodeDown(KeyCode.) and Adding Using KeyCodeUp(KeyCode.), even if its in a different Function
  4. Removing Using KeyCodeDown(KeyCode.) and Adding Using KeyCodeUp(KeyCode.), even if its in a different Function and you wait a few seconds Before you Let go of the Key
  5. And this is the Worst Part: Even if you use “IEnumerator” functions to add a time gap between the two Via “yield return new WaitForSeconds((5))”, no matter the duration.
  6. Even if you use “IEnumerator” functions to add a “waitUntill” the Purged list becomes empty “yield return new WaitUntil(() => SelectionManager.ctrlGroup1.Count == 0);”
  7. If you try to Remove Object A and B and C, while Adding Different Objects D, E and F. It still wont work using any of the Above methods!

NOTE: All of the Above work, if you Initially attempted to Purge/empty a List that was Initially Empty!

Now, I can Understand why method 1 - 2 and 3 dont work. but for the following reasons, I do not Understand why 4 - 5 and 6 Dont work.

Reason 1) if you Separate the two functions Via 2 separate keys, it always works even if you click them very fast in succession.
Reason 2) if you are removing Object A and B and C and trying to Add the Same Objects with the same function, thats something I can Understand. but I am not adding the same functions!
Reason 3) if I use “IEnumerator” to separate the actions by time it still doesn’t work, but if I do it manually using different KeyCode it works no matter how full the list is, and no matter how fast I click the two keys in succession. even if I am adding and removing the exact same items.

Now the way I am going to have to go around this to make my script work, Is make 2 Lists with around 4 loops that must work Like clock work. Though Theoretically I can make this work, It opens so much room for error/Bugs.

I am Obviously Missing Something!. Please help!

This is the Code of my Last Attempt! but I honestly tried this over 20-30 Different ways, It always comes up with the same Error if the above points apply!


IEnumerator SaveToList1()
    {
        if (Input.GetKey("left shift"))
        {
            foreach (Unit myUnit in SelectionManager.SelectedUnitList)
            {
                SelectionManager.ctrlGroup1.Add(myUnit);
                Debug.Log("FurtherUnits Have Been Added to group 1");
            }
           
        }
        else

        foreach (Unit myUnit in SelectionManager.ctrlGroup1)  //Unless Shift is pressed
            {
                if (myUnit.tag == ("UnSelected"))
                {
                    SelectionManager.ctrlGroup1.Remove(myUnit);
                    Debug.Log("Purge-All-Unselected Units-Add");
                }
               
           
        }

       
        if (SelectionManager.ctrlGroup1.Count != 0)
        {
            yield return new WaitUntil(() => SelectionManager.ctrlGroup1.Count == 0);    //Even This Doesnt work "yield return new WaitForSeconds((5)), desipt the list getting purged almost instantly

            SaveToList1Part2();
        }
        else

            SaveToList1Part2();
                      
    }

    void SaveToList1Part2()
    {
        foreach (Unit myUnit in SelectionManager.SelectedUnitList)
        {
            SelectionManager.ctrlGroup1.Add(myUnit);
            Debug.Log("All-Selected Units Have been Saved to Group1");
        }

    }

Error:

You cannot remove items from or add items to a List that your are iterating with “foreach”.

To delete items, create a new list list, and copy all items over except those that you want to delete.

To add items, create a new list, add the items that you want to add to that new list, and at the end, add the new list to the old.

@ csofranz, Thanks for reply, Problem is, I would then Have to Delete Items from the new list for the next time that function is used. and its possible that this function can be used very very repetitively. worst Part is, I would need this to Happens when only one keyboard button is used. so it will have to be a loop within a loop. And though I can see this working. It feels like so much work that Its hard to believe there is no other way for something so simple.

Now maybe this is the normal way and I “as a noob” is just shocked at how inefficient this is.

Well, it’s not really that bad, as it adds two lines to the code, and it’s usually much faster than iteratively deleting items (adding a list to a list is also fast)

so this loop

foreach (Unit myUnit in SelectionManager.ctrlGroup1)  //Unless Shift is pressed
{
    if (myUnit.tag == ("UnSelected"))  {
        SelectionManager.ctrlGroup1.Remove(myUnit);
        Debug.Log("Purge-All-Unselected Units-Add");
     }
}

becomes

List<Unit> shortenedList = new List<Unit>();
foreach (Unit myUnit in SelectionManager.ctrlGroup1)  //Unless Shift is pressed
{
    if (myUnit.tag != ("UnSelected")) {
        shortenedList.Add(myUnit);
        Debug.Log("Purge-All-Selected: Adding selected");
     }
}
SelectionManager.ctrlGroup1 = shortenedList

Which isn’t too bad, and safe.

@csofranz , and what about in the next click, I want to further “shorten” the “shortenedList”. By using the exact same keyCode?

I cant Infinity create new lists.

The purpose of this is, in RTS games, if you press CTRL+1,2,3,4,5,6,7,8,9

You Will store your Units to those numbers, and by clicking them, will instantly recall them all. This is the easy Part

Its Also very easy to Further Add more units to those Groups by just selecting New units and Ctrl+Numbers (though I have assigned shift for this because that the norm: Shift + CTRL + Numbers)

– The Hard Part is, when you want to CTRL+1, but you do not want to add to the current List, you want to replace it.

Now if I make a new list, 1) I would Have to use some sort of a loop to select it using the same keyboard Numbers, and 2) I would have to make another Loop, that will re-Use the First List so that I dont endlessly create New Lists.

Your explanations are very hard to follow, tbh, but if you want to do control groups like in RTS games, you should just have a List of 10 Lists of units. When the player presses Ctrl and a number, you make a list of the selected units and replace the existing list with the new one.

List<List<Unit>> ctrlGroups = new List<List<Unit>>(10);

// In your function
if(/*Ctrl down*/) {

    int selectedCtrlGroup = // The pressed number
   
    ctrlGroups[selectedCtrlGroup] = // Get selected units   
}

I’d probably make some sort of control group wrapper class, but that should be enough to start.

@Boz0r , That sounds Like a very good idea, I didnt even know you can Override a List with another List… btw what does that “(10)” stand for?

This is How my Selection Manager Looks Like atm

It’s just the initial size of the list. It’s still dynamic, though, so the size increases if you add more. So it’s a List of Lists of Units. That way you can always control how many control groups you want the player to have. You could annoy them by only allowing them 2-3 groups :slight_smile:

Oh I understand what you mean, but your not actually Overwriting the list, just the reference to it while creating a new one? but isnt that a problem in terms of performance and memory storage?

Also I remember in my days of Empire Earth (Great game), I use to use Ctrl+Numbers to create groups over 200-300 times per game on average, maybe even more. I was Super Competitive. Does that mean using this method I would have made 300 Lists and only overwriting the name so they would still all exists?

In theory, yes, you’re making 200-300 lists, but when you don’t have any references to the old list, it gets garbage collected.

I have been trying to avoid that, I was told it can cause Lag spikes?

Weather I use this Method or not, I must say this has been extremely Helpful! I thank you very much, Btw how long do these posts Last before they expire? because I need to download them Otherwise lol

Just dereferencing a list of references probably won’t cause any noticable performance impact. But don’t worry too much about performance improvements for such simple things that are going to make your code much more complex.

Edit: Some threads on this board are 10+ years old, so I think you’re good.

1 Like

Don’t worry about GC at this point in development. Do that when the game is finished, and doesn’t perform well. Then use the profiler to find out what is causing the problem. I can virtually guarantee to you that it won’t be creating and/or reallocating lists. :slight_smile:

1 Like

@csofranz , @Boz0r , thx guys

Now can someone explain why in hell this code Also gets the same Error??

 void SaveToList1()
    {
            //if (Input.GetKey("left shift"))
            // {
            //  foreach (Unit myUnit in SelectionManager.SelectedUnitList)
            //  {
            //      SelectionManager.ctrlGroup1.Add(myUnit);
            //      Debug.Log("FurtherUnits Have Been Added to group 1");
            // }

            //  }
            //  else



        if (removalActiveA == 1)
        {
           
            foreach (Unit myUnit in SelectionManager.ctrlGroup1)
            {
                if (myUnit.tag == ("UnSelected"))
                {
                    SelectionManager.ctrlGroup1.Remove(myUnit);
                    Debug.Log("Purge-All-Unselected Units-Add group group1");
                }

               
            }
        }
        else
        {
            foreach (Unit myUnit in SelectionManager.SelectedUnitList)
            {
                SelectionManager.ctrlGroup1.Add(myUnit);
                Debug.Log("All-Selected Units Have been Saved to Group1");
            }
        }


        if (removalActiveA == 0)
        {
            removalActiveA = 1;
        }

        else
        {
            removalActiveA = 0;
        }


    }

You’re still adding / removing items during iteration. This is not allowed in foreach-loops if the iterator belongs to collection that you’re modifying, and even though it works with normal for-loops, it’s also not recommended.

Reason being that this would quickly lead to bugs, which can be difficult to find.

Instead, you should follow the advice that was given earlier, with one small side-note… don’t create new lists all the time if you do this frequently, instead, allocate one or a few, and re-use them.

1 Like

You’re doing the same thing that was explained above earlier: looping through a List and changing the contents of its collection at the same time.

Add all the required elements to a temporary List, then once the loop is complete, perform the operation on those filtered items.

Rough Example:

//Setting a fixed count based on the real List's count, as it's not possible to go over this count.
List<Unit> removedUnits = new List<Unit>(SelectionManager.ctrlGroup1.Count);

foreach (Unit myUnit in SelectionManager.ctrlGroup1) {
   if (myUnit.tag == ("UnSelected")) {
      removedUnits.Add(myUnit);
   }
}

foreach (Unit unit in removedUnits) {
   SelectionManager.ctrlGroup1.Remove(unit);
}

Edit:
And yes, do what @Suddoha suggested. Create one reusable List of temporary units if this is something that is done frequently.

1 Like

@Suddoha , @Vryken , Thx for replies, I have already found a way around it, but the reason why I keep experimenting with it is because I am trying to Understand it. Btw, If you check the last Code I sent Carefully, the script never added Units Nor Deleted Units at the same time. It had to be done between 2 different calls of the function. Thx to the following line of code, this is why I am so confused. I am only doing one action with it. And though I have already found a way around it. I feel like as a noob I should try to understand the problem then to dodge it if you know what I mean?

if (removalActiveA == 0)
        {
            removalActiveA = 1;
        }
        else
        {
            removalActiveA = 0;
        }

It’s late here, but I’ll give it a try.

The assumptions or guesses that you had already written in your first post, just like the one I quoted here, have nothing to do with the problem at hand.

The problem resides whithin the foreach loop, or more specifcally, within the collection’s implementation of its IEnumerator, which is used to iterate the collection. You can separate the logic and wait as long as you want, it won’t help at all as long as the code does it that way.

Why?
The built-in collections keep track of a version number. It changes when the collection is modified via any operation like add, insert, remove …
When the foreach loop starts, it calls GetEnumerator on the instance which is on the right of the in-keyword, that is, your list instance in this particular case.
These collection types return IEnumerators which know the version number that was set when they were created.

Suppose your list was modified a few times and now has version number 10.
You enter a foreach loop, and the enumerator is created and knows version number 10 was the version it’s been created for.
While the loop executes, any changes would increase the collections version number and when the loop enters the next iteration via the enumerator, the change will be detected, as the enumerator compares the version number it knows about (10) with the most recent version number of the collection it originates from, which is 11 or greater by now.

These don’t match, an exception is thrown.

It’s an implementation detail, and it’s not necessarily required. I mean, you can just do whatever you want to do with a for-loop. Or write your own enumerable list without it. But it makes iterating collections less error-prone.

Imagine this detail wouldn’t exist: you’d most-likely sit there scratching your head because your code wouldn’t work as expected.

I think I’ll have to repeat that I think you’re trying to solve a non-issue. Unless you’re doing this a million times a second it doesn’t matter if you make a new list or reuse an old list. You’ll end up spending way too much time overthinking trivial stuff and make your code into a mess.

Optimize if there’s a measured performance concern and instead focus on more important stuff like making your code readable and clean.