Possible for EventDelegate to be immune to added/removed handlers on execute?

[drjeats]

We're upgrading from NGUI 2.7 to 3.5.5. A big jump, but the most complex UIs in our game were already slated to be redone anyway, and the new sprite manipulation and widget anchoring tools were too good to pass up. Well done on that.

After upgrading, one of our older scripts seemed to be creating an event loop since it would EventDelegate.Remove itself from onChange, change the widget value, then EventDelegate.Add itself back to onChange.

This thread suggests this might have already been a problem before, but was handled by copying the EventDelegate list on execute:

http://www.tasharen.com/forum/index.php?topic=7740.msg37556#msg37556

Doesn't seem to handle this case though.

Removing then re-adding callbacks is kinda janky and gross, so that's getting refactored out of our code (and adding the event handler back after yield return new WaitForEndOfFrame provides a temporary solution until the refactoring gets done).

However, I think the EventDelegate API would be better off if it made a guarantee that the a list is always copied on execute, or if it used some other means to ensure that adding or removing delegates in a handler doesn't affect the execution of the EventDelegate list since this is closer to the behavior of regular C# multicast delegates.

I realize Unity serialization might make this tricky to do without the cost of a list copy, but is this something on your roadmap?

[ArenMook]

You're removing then re-adding callbacks? Dare I ask why you had to do something like this? If you're executing something once, you can add the event delegate with that in mind (EventDelegate.Add has an optional parameter to only execute something once). Otherwise what's the point of removing then adding something? Isn't next result void as a result? You remove something, then you re-add it, you end up exactly where you started.

[drjeats]

Hey man, I never said I was proud that this little nugget exists in the codebase. Just saying it's there, and that this use case isn't inconsistent with standard delegate behavior.

Also, the effect is not void:

EventDelegate.Remove itself from onChange, change the widget value, then EventDelegate.Add itself back to onChange

There's an if branch in onChanged handler where it had to change the widget value without re-entering itself. Like I said, this is being rewritten, but NGUI provides more value if it's robust in spite of QFE hacks like this one.

[EDIT]

For reference, here's roughly what it looks like pre-refactoring sans irrelevant details, with the coroutine workaround:

1. private void OnScrollBarChanged()
2. {
3.     MuckWithOtherViewObjectsAndStuff();
4.  
5.     if (extraScrollContentSize == 0) {
6.         EventDelegate.Remove(scrollBar.onChange, OnScrollBarChanged);
7.         scrollBar.scrollValue = 0f;
8.         StartCoroutine(ReAddScrollBarEvent());
9.     }
10. }
11.  
12. private IEnumerator ReAddScrollBarEvent()
13. {
14.     yield return new WaitForEndOfFrame();
15.     EventDelegate.Add(scrollBar.onChange, OnScrollBarChanged);
16. }

[ArenMook]

...or you can just set a local flag on that script, something like "ignoreValue", and in the callback function if "ignoreValue" is true then return out of the function.

No hacks needed.

[drjeats]

A flag in this case is really just a hack of a different sort. Adding another bit of state introduces a technical burden to the class using EventDelegate, while making EventDelegate robust against add/change/remove during dispatch adds a technical burden to EventDelegate. It's a wash.

I'm not seeking to use this behavior, but I think it's worth your consideration.

[ArenMook]

All callbacks must be in the same state as whatever class you have them registered with (scroll bar in this case). You are intentionally breaking this, setting the scroll bar's value to something, and you don't want to execute the associated callbacks. This -- is a hack. Ask yourself why you need to do this to begin with -- why you need to break the consistency.

[drjeats]

I don't seem to be making myself clear. The hack is no longer in our code (it was a temporary measure while the rest of our UI code was being refactored). There is no longer any need to break this consistency. There is no problem I need help solving here.

I'm providing feedback, suggesting that your API contract for EventDelegate,

All callbacks must be in the same state as whatever class you have them registered with (scroll bar in this case).

could be a little more flexible for the sake of usability and consistency with how .Net delegates behave.

Right now if someone, either inadvertently or through  deliberate recklessness (that would be me  ), does this little add/change/remove dance or any variation on it, there's a fairly strong chance that Unity will get caught in an infinite event dispatching loop and lock up.

How do you stop it from locking up? The answer is obviously, "Don't do that." Unfortunately, that doesn't really help someone once they've gotten themselves into this mess. That also doesn't mean that NGUI shouldn't be made resilient to this kind of silly user behavior.

I think making the EventDelegate.Execute immune to this problem is the right approach, but it's also reasonable for NGUI to detect the loop, stop executing callbacks, and throw an exception.

It's just a suggestion. Take it or leave it.

[ArenMook]

Gotcha. I can add a check to prevent the loop.