EventDelegate.Execute - order of operations

[dominus85]

When you trigger a delegate in EventDelegate.Execute, the order is this:

for (int i = 0; i < list.Count; )
         {
            EventDelegate del = list;

            if (del != null)
            {
               // DELEGATE GETS EXECUTED
               del.Execute();

               if (del.oneShot)
               {
                  // DELEGATE GETS REMOVED
                  list.RemoveAt(i);
                  continue;
               }
            }
            ++i;
         }

I have an use case that, when executing an onFinished delegate, I want to add another onFinished delegate on the same object..

The problem is that the code execution will trigger Execute() and I will try to re-add a similar delegate in the same execution stack..

This will cause the EventDelegate to check if it already has the delegate, it will see that it has it.. then the execution gets back to the above line, list.RemoveAt(i) and the delegate will get removed

Proposed solution 1:

Call the del.Execute() method after the list.RemoveAt(i)

Proposed solution 2 - more complex, but safer:

Save a list of the delegates that need to be triggered, let the execution flow remove all the delegates from the list.. And after the for loop is finished, call all the delegates in another for loop

[ArenMook]

I'd aim for #1 personally. Remind me about this after the 16th if you can (when I return from vacation).

[dominus85]

I saw you made some changes to onFinished in the 3.0.9f1 version.. Since I didn't had the time to test, I am just reminding you of this issue..

The #1 is simpler indeed, but I don't think that #2 adds too much of a performance drawback and it would definetely solve all the problems where multiple callbacks can be stacked for the event list..

Imagine i have 2 delegates.. where delegate 1 tries to re-add the delegate2.. this won't happen since you will detect it already exists

Cheers

[ArenMook]

Right, 3.0.9 changed it so that when onFinished gets executed, a copy is used instead of the original list.

[dominus85]

I managed to review the changes..

I saw you've chosen to keep a mTemp to the old onFinished list and reset the onFinished list..

I am not sure this is the best idea because:

- the onFinished list of delegates will clear, no matter if the EventDelegate is set to oneShot or not (this basically renders oneShot obsolete)
- this should fix the problem for the UITweener classes but not for the other classes that use EventDelegate.Execute
- I think the fix should be done in the EventDelegate.Execute method and the clone of the list should be done there

Please consider my arguments and let me know if I need to further explain the issue.

Here is a proposed solution for the EventDelegate.Execute method. This would eliminate the need of cloning the array in UITweener and the rest of the classes that use the Execute method. I think the performance decrease would be minimal since most of the time there are only 1 delegates added to the onFinished method.

1. static public void Execute (List<EventDelegate> list)
2.         {
3.                 if (list != null)
4.                 {
5.             // Creates a clone of the event delegate list
6.             List<EventDelegate> mTemp = new List<EventDelegate>();
7.             mTemp.AddRange(list);
8.  
9.             for (int i = 0; i < mTemp.Count; i++)
10.                         {
11.                 EventDelegate del = mTemp[i];
12.  
13.                 if (del != null)
14.                                 {
15.                                         del.Execute();
16.  
17.                                         if (del.oneShot)
18.                                         {
19.                         list.Remove(del);
20.                                         }
21.                                 }
22.                         }
23.                 }
24.         }

[ArenMook]

I've already fixed in the Pro version yesterday (there was another thread about it), so it will be fixed in f2.