EventDelegate and Inherited Methods in latest NGUI

[wishful_st]

Hi there,

I've recently upgraded NGUI from 3.5.4r2 to 3.5.8 and noticed some "Could not find method" errors in my project triggered from "onFinished" tweens.  e.g.

1. Could not find method '<Hide>m__2' on NewDialog
2. UnityEngine.Debug:LogError(Object, Object)
3. EventDelegate:Cache() (at Assets/NGUI/Scripts/Internal/EventDelegate.cs:362)
4. EventDelegate:Execute() (at Assets/NGUI/Scripts/Internal/EventDelegate.cs:417)
5. EventDelegate:Execute(List`1) (at Assets/NGUI/Scripts/Internal/EventDelegate.cs:566)
6. UITweener:Update() (at Assets/NGUI/Scripts/Tweening/UITweener.cs:224)

In my case, I've tracked down how to reproduce it and I've concocted a simple example to reproduce the issue (not a useful and real life example but should highlight the problem) - if Aren, or anyone else can take a look at it and comment, that'd be great.  (Whether I'm doing something wrong, or if this is a legit issue).

Summary: if an EventDelegate callback is added from a base class and refers to a method in the base class, any object that is inherited from that base class and triggers that EventDelegate callback won't be able to trigger it because it "Could not find method"

Thanks.

*Edit* Not sure if this matteres, but this was tested/reproduced in the editor, with Android as the target platform.

[Nicki]

Hmm, this is quite strange. Only the extended version makes the error.

Changing the

1. () => gameObject.SetActive(false)

to

1. () => Debug.Log("test")

(BaseDialog: line 25-26)
Makes both of them work, which leads me to believe the timing of the deactivation has an effect.

Making it virtual and overriding by just running base.Hide makes the same error.

Making it virtual and overriding with the full body of the BaseDialog.Hide and not running base.Hide in the overriden one makes it work fine.

I'll go with my gut and say Reflection is dark magic and you're angering the gods. Don't do that.

[wishful_st]

So I'm digging around to see if I can pinpoint it further.  I've figured this out:

1 - It probably has something to do with Reflection and how the callback cache is implemented
2 - @Nicki - when you replaced the lambda function to just do the Debug.Log(), it actually changes the Callback object; i.e., there's no "target" if gameObject is not referenced, hence everything is happy, so that's a red herring.  Not a timing issue.
3 - This issue is related to the anonymous function - if I change (in BaseDialog.cs)

1.                 EventDelegate.Add (mainTween.onFinished, () => {
2.                         gameObject.SetActive(false);
3.                 }, true);
4.  

To

1.                 EventDelegate.Add (mainTween.onFinished, DoStuff, true);
2.  

while adding:

1.         protected void DoStuff(){
2.                 gameObject.SetActive(false);
3.         }
4.  

everything is happy.

I've diagnosed the issue to be the following:

When the anonymous method is added to the EventDelegate list, the method name is "saved/cached" in the EventDelegate list as <Hide>m__2.  However, when EventDelegate goes through its list of callbacks to execute, when it pulls this particular function from the list, it tries to retrieve the method (via GetMethod in EventDelegate.cs:355) with the method name <Hide>m__2, which seems to be only valid for BaseDialog, but not NewDialog, so it fails if the triggering class is from the inherited class.  Hence, if the anonymous function was replaced with a named function, GetMethod seems to be able to find the method from the inherited class with no problems (provided the proper accessibility modifiers are in place).

I'm not sure how to fix this yet, I'm not extremely familiar with Reflection - maybe ArenMook has an idea.  In the meantime, I'll continue to dig around.  As stated above though, a work around would be to declare named methods instead of anonymous functions and this work around is only required if an inherited class is trying to execute a callback that was declared via a lambda function in the base class.

[wishful_st]

After a little bit more investigation, I think there may be a solution, though ArenMook would probably have a more complete picture as I haven't considered the impact on the editor at all (and legacy).

In EventDelegate.cs, when Setting the Callback (EventDelegate.cs:292), the reflected type for that method is actually available to you via call.Method.ReflectedType.  If the reflected type is saved in a member variable, when inside the Cache() method, at EventDelegate.cs:347, instead of:

1. System.Type type = mTarget.GetType();
2.  

you might be able to do something like:

1. System.Type type = (mReflectionType!=null)?mReflectionType:mTarget.GetType();
2.  

where mReflectionType is the type saved during Setting of the Callback.  This way, the proper type can be used when trying to get the method.  I've tested this, and it seems to work - but there definitely is more supporting code that needs to be written to cover all cases - the above modifications was just to prove that it's possible to support anonymous functions for this edge case.

[ArenMook]

You should never be getting that far in the Cache() code because the delegate would be a raw delegate, and 'mRawDelegate' would evaluate to true, exiting the function early. Question is why isn't that happening in your case?

[wishful_st]

Good question. Following your hint, I've done the legwork and found out why mRawDelegate is set to null, though I'm not exactly sure what mTarget's purpose is in EventDelegate.  The following is a stack trace of when the lambda is added to the EventDelegate list.  (line numbers in EventDelegate.cs will be off - I've got debug code in there):

1. EventDelegate.Set (call={EventDelegate.Callback}) in C:\TestProj\Assets\NGUI\Scripts\Internal\EventDelegate.cs:302
2. EventDelegate..ctor (call={EventDelegate.Callback}) in C:\TestProj\Assets\NGUI\Scripts\Internal\EventDelegate.cs:213
3. EventDelegate.Add (list=Count=0, callback={EventDelegate.Callback}, oneShot=true) in C:\TestProj\Assets\NGUI\Scripts\Internal\EventDelegate.cs:650
4. BaseDialog.Hide () in C:\TestProj\Assets\_Test\BaseDialog.cs:24
5.  

Basically, the lambda function is getting added like so in my code:

1.         EventDelegate.Add (mainTween.onFinished, () => {
2.                 gameObject.SetActive(false);
3.         }, true);
4.  

...when it gets into EventDelegate's Add function, which in turn, calls Set - call.Target is NOT null, hence mRawDelegate will be set as false.  This is the snippet in Set:

1.                         mTarget = call.Target as MonoBehaviour;
2.  
3.                         //call.Target is NOT NULL here - it is a NewDialog, hence the else branch gets executed, where mRawDelegate is set to false
4.  
5.                         if (mTarget == null)
6.                         {
7.                                 mRawDelegate = true;
8.                                 mCachedCallback = call;
9.                                 mMethodName = null;
10.                         }
11.                         else
12.                         {
13.                                 mMethodName = GetMethodName(call);
14.                                 mRawDelegate = false;
15.                         }
16.  

And as per your explanation, it would then go into Cache code.  This explains why mRawDelegate is false.  As to why call.Target is not null, that is outside of my experience .  Perhaps someone other than myself can shed some light?

*Edit* I've done a bit of research and I've found that call.Target is:

The object on which the current delegate invokes the instance method, if the delegate represents an instance method; null if the delegate represents a static method.

So - knowing that, what is the purpose of the mTarget == null check?  To separate the static calls from the instance calls?  If that's the case, then definitely that would explain why mRawDelegate would be false - the lambda is added from an instance of a class.

[wishful_st]

Not sure if this is a valid "fix" or not, but it works for the test case and my project.  In EventDelegate.cs, in the Set method, added additional clause:

1.                         if (mTarget == null)
2.                         {
3.                                 mRawDelegate = true;
4.                                 mCachedCallback = call;
5.                                 mMethodName = null;
6.                         }
7.                         else if (mTarget.GetType () != call.Method.ReflectedType)
8.                         {
9.                                 mRawDelegate = true;
10.                                 mCachedCallback = call;
11.                                 mMethodName = null;
12.                         }
13.                         else
14.                         {
15.                                 mMethodName = GetMethodName(call);
16.                                 mRawDelegate = false;
17.                         }
18.  

Though, this leads me to ask, in what cases would you not want to set mCachedCallback = call and mRawDelegate to be true all the time?

[ArenMook]

mTarget.GetType() would normally be something like YourScript. call.Method.ReflectedType refers to the method instead, not the class, does it not? That doesn't seem like a fix at all.

Also, is there a reason you can't use anonymous delegates instead?
Instead of:

1. EventDelegate.Add (mainTween.onFinished, () => { gameObject.SetActive(false); }, true);

Use:

1. EventDelegate.Add (mainTween.onFinished, delegate () { gameObject.SetActive(false); }, true);

[wishful_st]

using delegate() instead of () => {} doesn't make a difference, it "breaks" exactly the same way.

As stated earlier in the thread, this "breaking" will only happen if the delegate/lambda was declared in a base class and the lambda/delegate is triggered in an inherited class.  And the reason why you might need the ReflectedType is, yes the ReflectedType is the type of the object that invoked the method itself, but that's exactly what you'll need in this case, since the lambda/delegate seems to be bound only to the base type, and not the inherited type.  e.g., if you look at the example attached in my original post, when NewDialog calls Hide(), since it hasn't implemented Hide(), it calls BaseDialog's Hide(), which in turn, declares and executes the delegate/lambda method.  The ReflectedType would be BaseDialog in this case, instead of NewDialog.

That's why when it gets into your Cache() function

1. System.Type type = mTarget.GetType();  //mTarget.GetType() is NewDialog
2.  
3. mMethod = type.GetMethod(mMethodName, BindingFlags.Instance | BindingFlags.Public | BindingFlags.NonPublic);
4.  

mMethod evaluates to null (it doesn't throw exception for some reason), and the "LogError" happens when you check if mMethod is null.  I've even put in debug calls to list the methods for both BaseDialog and NewDialog via mMethod.GetMethods, and I've comfirmed that the compiler created function aka the delegate/lambda (<Hide>m__2) cannot be found for type NewDialog but is found for BaseDialog.  That's why I don't think you can use the target's type in this instance, since the target's type is a NewDialog, while the method is actually bound to BaseDialog.  In most cases, call.Target's type would be the same as the ReflectedType, but not in the cases where there's inheritance.

[ArenMook]

Alright, so how about we change the Cache method instead? Inside the try block, replace this:

1. mMethod = type.GetMethod(mMethodName, BindingFlags.Instance | BindingFlags.Public | BindingFlags.NonPublic);

...with this:

1.                                         for (mMethod = null; ; )
2.                                         {
3.                                                 mMethod = type.GetMethod(mMethodName, BindingFlags.Instance | BindingFlags.Public | BindingFlags.NonPublic);
4.                                                 if (mMethod != null) break;
5.                                                 type = type.BaseType;
6.                                                 if (type == null) break;
7.                                         }

Will this work? I don't want to add your functionality as-is because it's not going to work on WP8/WSA, so I'd like another solution.

[wishful_st]

Yep - this works for this test case.

The only case where I think this can break (though, I'm not sure if it's possible) is if the method can't be found in the first iteration, and if a collision happens with the method names in the inheritance tree, in which case the wrong method would be called.  However, this is probably highly unlikely, if not impossible - but I don't know how the compiler generates method names for anonymous methods *shrug* 

It also sucks that you have to iterate, but the only alternative I see is saving the reflected type of the method somewhere - though, I'm not sure if that's easy or WP8 friendly or not.  *shrug* no matter...it works.

Thanks for looking into this - this is probably scenario that doesn't happen often and I appreciate you spending time looking into this.

Cheers!