C# lambda call in for loop references to last object

Questions & Answers
1

I already asked about this in StackOverflow and they told me to ask it here instead (here’s the link to former question)

I’ve researched this problem and I’ve tried fixing the code as instructed in a few questions before (such as this one) but it still won’t work. Though I must say that all the answers I checked were from 2009-2010 so they might be obsolete.

This is the culprit code:

foreach(Entity player in players)
{
    if(player.actions.Count > 0)
    {
        Entity temp = player;

        player.isDoingAction = true;
        Debug.Log(player.name + " started action");

        player.actions.Dequeue().Execute(() => { temp.isDoingAction = false; Debug.Log(temp.name + " finished"); });
    }
}

This prints the following:

Player1 started action
Player2 started action
Player2 finished
Player2 finished

When it should print:

Player1 started action
Player2 started action
Player1 finished
Player2 finished

Or something similar.

This code runs in a Unity coroutine function.

A bit larger snip from the code:

GameManager.cs

private IEnumerator RunTurn()
{
    ...
    ...
    ...

    for(int i = 0; i < phases; i++)
    {
        //Assign action to each player
        foreach(Entity player in players)
        {
            if(player.actions.Count > 0)
            {
                Entity temp = player;
    
                player.isDoingAction = true;
                Debug.Log(player.name + " started action");
                player.actions.Dequeue().Execute(() => { temp.isDoingAction = false; Debug.Log(temp.name + " finished"); });
            }
        }
    
        //Wait for each player to finish action
        foreach(Entity player in players)
        {
            while(player.isDoingAction == true)
            {
                Debug.Log("Waiting for " + player.name);
                yield return null;
            }
        }
    }

    ...
    ...
    ...
}

Action.cs

public override void Execute(System.Action callback)
{
    Move(callback);             
}

private void Move(System.Action callback)
{
    ...
    ...
    ...

    //Move target entity
    target.MoveToPosition(newPosition, mSpeed, callback);
    target.location = newLocation;

    ...
    ...
    ...
}

Entity.cs

public void MoveToPosition(Vector3 position, float speed, System.Action callback)
{
    StartCoroutine(CoMoveToPosition(position, speed, callback));
}

//Move to position
private IEnumerator CoMoveToPosition(Vector3 position, float speed, System.Action callback)
{
    while(position != transform.position)
    {
        transform.position = Vector3.MoveTowards(transform.position, position, speed * Time.deltaTime);
        yield return null;
    }

    //Move finished so use callback
    callback();
}

Solution from StackOverflow

Working piece of code:

foreach(Entity player in players)
{
    if(player.actions.Count > 0)
    {
        player.isDoingAction = true;
        Debug.Log(player.name + " started action");

        System.Func<Entity, System.Action> action = new System.Func<Entity,System.Action>(p =>
        new System.Action(() => { p.isDoingAction = false; Debug.Log(p.name + " finished"); }));
        
        player.actions.Dequeue().Execute(action(player));
    }
}
2

Well, the problem here is that a coroutine itself is an implicit closure as well which moves all local variable to a seperate class as member variables. That means it’s impossible to create a “real” local variable inside a generator method. That’s why the explicit local variable declaration doesn’t work.

The only solution here would be to move the creation of the lambda into a seperate (non coroutine) method which you can call from your coroutine.

Either just move the creation of the delegate to a seperate method or the whole foreach loop. The simplest solution would be to only move the lambda into a seperate method as this wouldn’t need the explicit local variable at all since a method parameter is already a local variable (local to the new method).

player.actions.Dequeue().Execute(CreateClosure(player));

// ...

System.Action CreateClosure(Entity aPlayer)
{
    return () => { aPlayer.isDoingAction = false; Debug.Log(aPlayer.name + " finished"); }
}

edit
Just saw that you declared your own “Action” type. I replaced “Action” with “System.Action”

3

Just ran into this today and also assumed it was just a(nother) oddity in C#'s scope rules, but this SO post suggest that it works correctly in VS (am on a Mac so have not tested) and even that it’s fixed in newer releases of Mono:

So, I guess it’s a bug.

The workaround to create the lambda outside your co-routine works, but it’s not pretty.

@UnityDevs : Can you guys prioritise this?

4

If you’re working with objects, you’re working with references. It’s not a bug. While looping over an array of objects and calling methods on them using lambdas you’re always calling it on the last reference of the variable. The solution would be to implement the ICloneable interface, implement the Clone() method and create a clone of the object for each loop run. I had the same problem while dynamically adding buttons to the UI and solved it by using clones.