-
Notifications
You must be signed in to change notification settings - Fork 70
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Allow static method injection #95
base: master
Are you sure you want to change the base?
Conversation
51af7b5
to
6eeeb3c
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have tested these changes
Have you? As is the PR had no chance of working at all.
@@ -559,10 +558,12 @@ private static bool IsMethodEligible(MethodInfo method) | |||
foreach (var parameter in method.GetParameters()) | |||
{ | |||
var parameterType = parameter.ParameterType; | |||
if (!IsTypeSupported(parameterType)) | |||
if (!IsTypeSupported(parameterType) || | |||
method.IsStatic && parameterType.IsGenericType && parameterType.ContainsGenericParameters) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's this check for?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't remember exactly, but best guess would be that somewhere there was a static method with one of the parameters being a generic type, and likely that was causing issues.
My test code: public class Test : Il2CppSystem.Object
{
public Test() : base(ClassInjector.DerivedConstructorPointer<Test>())
{
ClassInjector.DerivedConstructorBody(this);
}
public void InstanceFoo(int a, int b)
{
System.Console.WriteLine($"InstanceFoo {a} {b}");
}
public static void StaticFoo(int a, int b)
{
System.Console.WriteLine($"StaticFoo {a} {b}");
}
} ClassInjector.RegisterTypeInIl2Cpp<Test>();
var type = Il2CppType.Of<Test>();
var methods = type.GetMethods(BindingFlags.DeclaredOnly | BindingFlags.Public | BindingFlags.NonPublic | BindingFlags.Instance | BindingFlags.Static);
foreach (var methodInfo in methods)
{
Log.LogWarning(methodInfo.Name);
if (methodInfo.Name.EndsWith("Foo"))
{
var o = methodInfo.IsStatic ? null : new Test();
methodInfo.Invoke(o, new Il2CppReferenceArray<Object>([1, 2]));
}
} |
@limoka can you rebase? |
6eeeb3c
to
b13fc2c
Compare
@ds5678 rebased as you asked. Sorry it took a while, I was busy with IRL things. |
@@ -583,10 +582,12 @@ private static bool IsMethodEligible(MethodInfo method) | |||
foreach (var parameter in method.GetParameters()) | |||
{ | |||
var parameterType = parameter.ParameterType; | |||
if (!IsTypeSupported(parameterType)) | |||
if (!IsTypeSupported(parameterType) || | |||
method.IsStatic && parameterType.IsGenericType && parameterType.ContainsGenericParameters) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why does it matter here if the method is static or not?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I mean Il2cpp Interop supports normal methods that are generic. If I don't include static part, those won't work.
This check is here so that only non generic static methods are included.
Most likely I didn't want to bother supporting generic static methods.
This PR enables static methods to be injected. This mostly allows to pass such methods to il2cpp delegates, which can be useful in some cases.
My main use of this is for interacting with Burst compiler. It expects us to pass delegates pointing to valid il2cpp methods to consider them for Burst compilation.
I have tested these changes on Core Keeper Unity 2021.3.14.