Foreach loop returning last item despite using local variable

Unity Engine
1

I have a list of enemies and a method in which I iterate over that list and Invoke another method that instantiates a GameObject for each item in the list.

        foreach (Enemy e in enemies)
        {
            Enemy currentEnemy = e;
            Invoke("Spawn", currentEnemy.Timer);
        }

Using the local variable should have fixed this, but instead of instantiating all enemies, it instantiates the last enemy enemies.Count times.

I also tried using enemies.ForEach but it didn’t fixed it.

A fix to this issue, or another way to instantiate all enemies in the list with a given delay would be appreciated, thanks.

2

Have you checked out that enemies list in your inspector during run-time to see if the list actually has all unique entries?

3

Yes, they are different.

4

The problem is that you’ve set “Spawn” to call after currentEnemy.Timer has elapsed. By then, your loop has finished and so I’m guessing your “Spawn” method grabs the enemy object from somewhere?

What you’re trying to do sounds like Coroutines would be a better solution.

5

How does your Spawn function know which enemy to instantiate? There isn’t anything in the code you’ve shown that shows how it would know. The Spawn function can’t know anything about your local variable because it only exists inside of that for loop.

Can you show some additional code? Also, an explanation of what you’re trying to do would be helpful as well. The timer thing makes it difficult to diagnose based on what you’ve given us so far.

1 Like
6

Here’s an example of how to do it with a coroutine, if I understand correctly about what you want to do.

    void SpawnEnemies()
    {
        foreach(Enemy enemy in enemies){
            StartCoroutine(DelayedSpawn(enemy));
        }
    }

    IEnumerator DelayedSpawn(Enemy enemy){
        float timer = 0;

        while(timer < enemy.Timer){
            timer += Time.deltaTime; //Increase our timer by frame-time
            yield return null; //Wait for next frame before calculating again
        }
        //Now that our while loop has finished, that means time is up, so Spawn!
        Spawn(enemy);
    }
2 Likes
7

Or rather than the hand rolled timer…

IEnumerator DelayedSpawn(Enemy enemy){
        yield return new WaitForSeconds(enemy.Timer);
        Spawn(enemy);
    }
1 Like
8

Sure, if you want to be lazy and generate garbage :wink:

9

:slight_smile: By that logic you shouldn’t be using Coroutines either since they create an enumerator each time they’re started. Lazy always trumps premature optimization! :smile:

10

That doesn’t make sense, you’re saying “oh, well I’m generating some garbage, so let’s just generate even more!”. WaitForSeconds is just another generated Enumerator that your current enumerator is yielding to. It’s doing what I wrote, but you’re launching another enumerator just to do that simple function.

That’s not pre-mature optimization, that’s recognizing efficient coding practices. It takes very little extra time to just write it the more efficient way, and you also provide yourself with the ability to make things happen during that wait time if you so feel like it.

11

Unless you’re calling this sort of thing a dozen times per frame it’s mostly irrelevant. In any case, not really what OP was caring about, eh?

12

Indeed, thus no reason for you to have made that comment when perfectly valid and optimal code was given :slight_smile:

13

I don’t really agree with you. Simplicity when providing assistance to someone is generally the better way to go. Your hand coded timer is more complex than necessary to solve OP’s problem and would likely be more confusing to a newer coder. But hey, if you want to leave this discussion being “right” then I’ll leave you to it. :slight_smile:

14

I would agree this sounds like an instance of premature optimization. There’s nothing wrong with using WaitForSeconds and generating a little garbage in a small, fun personal project. I use it all the time even in production projects and the performance impact is negligible on small scales, with the trade-off being excellent readability and code simplicity.

Besides, if you’re really concerned with creating new objects, you can cache the yield instruction and reuse the same one.

YieldInstruction enemySpawnDelay = new WaitForSeconds(5);

private IEnumerator DelayedSpawn(Enemy enemy) {
    yield return enemySpawnDelay;
    Spawn(enemy);
}
2 Likes
15

I actually think the garbage is fixed now. I know a while ago they fixed a lot of the garbage generation from enumerators and coroutines. I know the cases I was seeing garbage were fixed back then.
I haven’t looked again recently.

For the question we need to see more code. Invoke takes a function name and a time.
I never use invoke but my guess is the scope is not every monobehavior in the scene.
And from the examples it looks like it calls it on the same gameobject.

You might need something like
currenEnemy.invoke(“spawn”, currentEnemy.timer);

16

Problem here might be with Invoke and reference type that foreach uses when iterating the object.

Meaning that at the point where the invoke method is called, the reference variable enemy is already pointing to the last entry in the collection because foreach has already gone through all the enemies in the collection by the time it’s called.

for (int i = 0; i < enemies.Count; i++)
{
      Invoke("Spawn", enemies[i].Timer);
}

I recommend using foreach only for Dictionaries because the good old for is easier and better for almost everything else. Just type for and press tab twice and most editors will give you a ready to use code-snippet for for.

For is also multiple times faster than foreach.

17

Actually, I think Invoke is called on each timer…however, what “Spawn” does is beyond our knowledge, which is where the problem actually lies.

Invoke doesn’t take a parameter with the method call(you can’t pass something to spawn using Invoke). All it does is say call a method after x amount of time. So if “Spawn” component simply spawns an object, it has no clue what object to spawn unless it’s set somewhere.

In otherwords, nothing is being set in that loop to indicate what needs to be spawned after “Spawn” is called. Thus, the coroutine is a better solution as you can pass an enemy object to it, thus knowning what to spawn and when.

1 Like
18

First off, on the subject of Garbage. it doesn’t matter until you can actually see the impact in your game.
Secondly, @Invertex , your examples still produce garbage (because you use that foreach lol).

And no, Foreach can still produce garbage, it depends mostly on the collection or if you’re accessing a collection via an interface. Foreach on IEnumerable and IEnumerable always produces garbage, except on arrays (which are given special treatment). Even if the collection is using a struct as its Enumerator, if you access it via an IEnumerable, it will get boxed and generate garbage.

Again, because I think this is important to get across, it doesn’t matter until you can feel it! More often than not you’ll never feel it, or the code you’ve wasted time to pre-optimize ended up changing to something else. Optimizing ForEach is like the last thing you have to work on for performance leaks.

Finally you can use Closures inside a loop to carbon-copy the reference, even if the reference is changed later take this for example.

int count = 0;
var actions = new List<System.Action>();

for(int i =0; i< 10; i++)
{
    count = i;
    actions.Add( () => {Debug.Log(count)});
}

//this will print "9" ten times
for(int i =0; i< 10; i++)
{
    actions[i].invoke();
}

Here, the variable thats used is in an external scope from where the lambada was defined which means the variable is shared on all delegates that use it.

However shift the code just so slightly…

var actions = new List<System.Action>();

for(int i =0; i< 10; i++)
{
    int count = i; // closure reference!
    actions.Add( () => {Debug.Log(count)});
}

//this prints "0" through "9"
for(int i =0; i< 10; i++)
{
    actions[i].invoke();
}

suddenly you’re using closures. The compiler detects that the defined lambada is using a variable defined in the same environment (i.e. same scope), so it made a “carbon-copy” of said variable and that lambada brought that variable instance with them.

I love using closures when I get the chance, it allows me to pack extra data into an event handler. that said I always comment that I’m using closures, because its not always instantly obvious why some random variable was declared there for, especially for people who don’t know what closures are.

1 Like
19

I used @Invertex 's answer and it’s working now.
And with the spawn method: currentEnemy was a class variable.
Thank you all for your help :slight_smile:

3 Likes