C# lambda call in for loop references to last object

Question

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

Answer 1

Answer by Bunny83 · May 25, 2015 at 10:06 PM

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"

Add comment · Show 3 · Share

TeroHeiskanen · May 26, 2015 at 05:50 AM 0

Share

But I would have to create the lambda function within CreateClosure am I correct? I would have to make a couple of those CreateClosure functions for different needs. Using the solution I have shown in the end of my question I could make a function that takes in a System.Action and an Entity and gives out just the function which in turn would return the Entity. It would make for a much more general solution and I could use it everywhere in my code.

Bunny83 · May 26, 2015 at 08:29 AM 0

Share

Of course you can create two anonymous methods and execute one immediately. This should work as well:

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

Even when it works it's quite ugly. Since your actions seems to be bound to a certain player, wouldn't it make more sense to use an Action ins$$anonymous$$d and pass the player itself when you invoke the action.

TeroHeiskanen · May 26, 2015 at 08:44 AM 0

Share

Could you elaborate a bit more? Are we talking about Action or System.Action now? I should maybe change my na$$anonymous$$g a bit. I named the class Action before I knew I would use System.Action or even knew such a class existed in the first place.

Answer 2

Answer by BoaNeo · Oct 21, 2016 at 09:49 AM

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:

http://stackoverflow.com/questions/22200248/enumerating-over-lambdas-does-not-bind-the-scope-correctly

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?

Add comment · Share

Answer 3

Answer by Zodiarc · Oct 21, 2016 at 12:48 PM

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.

Add comment · Show 4 · Share

BoaNeo · Oct 21, 2016 at 01:09 PM 0

Share

Sorry, but you are wrong. It's a bug.

1) The VS run-time handles it correctly, newer versions of the $$anonymous$$ono runtime handles it correctly. The version of $$anonymous$$ono shipped with Unity does not.

2) It has nothing to do with references - it has to do with whether or not the local variables get captured when you create the lambda. The problem occurs with value types as well.

BoaNeo · Oct 21, 2016 at 01:12 PM 0

Share

(FWIW, the problem you are (probably) thinking of where an iterator isn't captured is caused by C#'s silly scope rules and is not a bug, true, but that's not the problem here - here the problem is that the loop behaves differently if it's in a co-routine).

Zodiarc BoaNeo · Oct 21, 2016 at 01:18 PM 0

Share

Oh I overread the coroutine part. $$anonymous$$y mistake.

BoaNeo Zodiarc · Oct 21, 2016 at 01:26 PM 0

Share

No worries :)

It's not mentioned in the first code snippet, so it's easy to miss, but you can see OP does the following:

Entity temp = player;

To get around the iterator problem, but it still doesn't work, and that's the bug.

I really prefer Java's way of handling this - forcing me to declare variables "final" if I want to use them in a lambda makes it impossible to make these kinds of mistakes and also makes it very obvious what is going on. In C# it just looks bloody mysterious.

C# lambda call in for loop references to last object

3 Replies

Your answer

Follow this Question

Related Questions