C# lambda call in for loop references to last object

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

It turns out there is a bug in Unity with coroutines and anonymous lambda callbacks. Check this link for more.

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

Something doesn't make sense. The use of local `temp` is the standard way to "Capture" (closure style) the variable and prevent it from changing in the next iteration. Is there anything else in the code we're not seeing? — Amit, May 25 '15 at 10:46
@Amit I edited in the whole loop structure. It really doesn't make sense which is why I posted this here. — Tero Heiskanen, May 25 '15 at 11:21
What happens if you capture the name itself (I assume that's not really what you want, but might be useful to understand)?: `string tempName = player.name;` ... `Debug.Log(tempName...` — Amit, May 25 '15 at 11:30
Try using the debugger to see what the values of temp are and compare them to players. Maybe the players list has changed in the meantime? — Daniel Rose, May 25 '15 at 11:30
@Amit Prints `Player2` and `Player2` so same thing. @DanielRose `temp` is defined correctly when the loop runs (checked with debugger). — Tero Heiskanen, May 25 '15 at 11:43
Try this: `Action a = () => Debug.Log(temp.name); a(); player.actions.Dequeue().Execute(a);` The idea is to test if the lambda captured `temp.name` BEFORE going into the `Execute` call, and if that somehow changed afterwards. — Amit, May 25 '15 at 12:02
@Amit It prints the following: `Player1 started action` `Player1` `Player2 started action` `Player2` `Waiting for Player1` `Player2` `Player2` `Waiting for Player1` and then repeats the last. So it works just fine when I call it right away, but not later on when the loop has progressed. — Tero Heiskanen, May 25 '15 at 13:20
@publicgk I added the last parts in. This should show the whole flow of the code. — Tero Heiskanen, May 25 '15 at 13:26
Makes no sense :-). looks like your callback is being overwritten and the problem isn't with the capture, or the lambda. Final validation: `Action a = (player.name == 'player1') ? (() => Debug.Log("Player1")) : (() => Debug.Log("not Player1"));`. No longer using capture. If this fails, it's definitely the callback being reset. — Amit, May 25 '15 at 13:29
@Amit `yield return null` seem to work fine but there's an alternative which is `yield return new WaitForSeconds(0f)` which I think does the exact same thing. I updated my question once again to answer to your earlier comment. The comment box is getting too crowded for my liking :P — Tero Heiskanen, May 25 '15 at 13:50
@Amit Check this [links](http://answers.unity3d.com/questions/755196/yield-return-null-vs-yield-return-waitforendoffram.html) answer. — Tero Heiskanen, May 25 '15 at 13:54
I'm pretty much out of ideas. It looks like the delegate (`callback`) isn't changed, but the captures variables are. That's too weird and I can only suggest you contact Unity Technologies for assistance. Sorry — Amit, May 25 '15 at 15:51
@Amit No worries! You were really helpful and helped with troubleshooting. I'll contact Unity. Thank you :) — Tero Heiskanen, May 25 '15 at 16:06

score 6 · Accepted Answer · answered May 25 '15 at 16:59

6

You can capture the value the following way:

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

answered May 25 '15 at 16:59

Dzienny

3,295
1
20
30

1

@Dzienny - Other than providing an answer, are you able to explain the problem with the OP approach and how the solution avoids the issue? – Amit May 25 '15 at 21:35

score 3 · Answer 2 · answered May 25 '15 at 10:32

3

Try

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

answered May 25 '15 at 10:32

shapiro yaacov

2,308
2
26
39

1

That's not what the original code tries to do. `temp` is changed and no longer hold a reference to `player`. – Amit May 25 '15 at 10:47
1

The method is defined as `void Execute(System.Action callback)` so that won't work I think? I was thinking if making a delegate that takes an Entity as parameter would work but I haven't tried yet. Also I would like to keep the solution as general as possible. – Tero Heiskanen May 25 '15 at 11:14

C# lambda call in for loop references to last object

2 Answers2

Linked