Unity il2cpp support

[msciotti]

Trying to support #243

[msciotti]

@dylanh724 and @joshpeterson: This seems to be on the path to working. All the Discord-related stuff works just fine in both Mono and il2cpp. Still working on the UnityEvent stuff.

[snowyote]

lgtm!

[dylanh724]

Hmm, I could only get this working in the editor -- what is your build info?

[joshpeterson]

The error message here from IL2CPP is not too helpful. It would be nice if it happened to mention the name of the method that is causing the problem. I'm going to add this to the error message for a future Unity release.

@dylanh724: It is possible for you to patch the source code in your local Unity installation to add this information to the error message. Here is how to do it:

Look for the Data\il2cpp\libil2cpp\vm\PlatformInvoke.cpp file in your editor installation. In the function named PlatformInvoke::MarshalDelegate in that file, look for the code snippet which looks like this:

// Okay, we cannot marshal it for some reason. Figure out why.
if (Method::IsInstance(d->method))
    vm::Exception::Raise(vm::Exception::GetNotSupportedException("IL2CPP does not support marshaling delegates that point to instance methods to native code."));

You can replace that code with this code:

std::string methodName = il2cpp::vm::Method::GetFullName(d->method);
// Okay, we cannot marshal it for some reason. Figure out why.
if (Method::IsInstance(d->method))
{
    std::string errorMessage = "IL2CPP does not support marshaling delegates that point to instance methods to native code. The method we're attempting to marshal is: " + methodName;
    vm::Exception::Raise(vm::Exception::GetNotSupportedException(errorMessage.c_str()));
}

Then build the player for IL2CPP in the editor again. When the error happens, you should see the full name of the method that is supposed to be marshaled, but is not able to be marshaled correctly. Hopefully that will help track down the remaining issue.

[dylanh724]

Heads up - We just had a patch in our game and may need a couple days to get the time to test this again~

[dylanh724]

@joshpeterson Anything special I need to do to get this code replacement working? I'm still seeing the old error. Is there a 2nd place where this can be with the same error?

EDIT: OH. The backup file wasn't renamed to .bak, so maybe c++ files pick up classes no matter what the file name? I'm unfamiliar. Renamed backup to .bak and trying again. Also restarting Unity.

EDIT: Ok, I haven't edited my Unity non-project files for a long time. I was accessing my old 5.6 one ;p I forgot about the Hub's new hierarchy and forgot to remove 5.6. Oops! Trying again.

[joshpeterson]

@dylanh724 No, I think this is the only place this error message occurs. Regarding .bak files, Unity will only compile and use .cpp files, so that should not have an impact.

Is it possible for you to send me the project or submit it via a bug report to us at Unity? I can have a look at it and try to determine what is wrong.

[dylanh724]

@joshpeterson the change reflects now -- however, I am getting this error that seems to trace back straight to that file changed: https://pastebin.com/gaH0Ue3p

il2cpp.exe didn't catch exception: Unity.IL2CPP.Building.BuilderFailedException: PlatformInvoke.cpp

C:\Program Files\Unity\Hub\Editor\2018.2.12f1\Editor\Data\il2cpp\libil2cpp\vm\PlatformInvoke.cpp(406): error C2065: 'methodName': undeclared identifier

For clarity, here is what was changed including a bit of the block around it:

Il2CppMethodPointer reversePInvokeWrapper = MetadataCache::GetReversePInvokeWrapperFromIndex(d->method->methodDefinition->reversePInvokeWrapperIndex);
if (reversePInvokeWrapper == NULL)
{
	// Okay, we cannot marshal it for some reason. Figure out why.
	if (Method::IsInstance(d->method))
	{
		std::string errorMessage = "IL2CPP does not support marshaling delegates that point to instance methods to native code. The method we're attempting to marshal is: " + methodName;
		vm::Exception::Raise(vm::Exception::GetNotSupportedException(errorMessage.c_str()));
	}

	vm::Exception::Raise(vm::Exception::GetNotSupportedException("To marshal a managed method, please add an attribute named 'MonoPInvokeCallback' to the method definition."));
}

Is it supposed to be method.Name instead of methodName? (I noticed (d->method))

[joshpeterson]

@dylanh724 I think you need one more line:

std::string methodName = il2cpp::vm::Method::GetFullName(d->method);

This should be just above this line:

std::string errorMessage = "IL2CPP does not support marshaling delegates that point to instance methods to native code. The method we're attempting to marshal is: " + methodName;

[dylanh724]

@joshpeterson Oh man, missed that copy+paste. Got it:

NotSupportedException: IL2CPP does not support marshaling delegates that point to instance methods to native code. The method we're attempting to marshal is: DiscordMgr::OnReady
  at DiscordController.OnEnable () [0x00000] in <00000000000000000000000000000000>:0 

DiscordMgr::OnReady

[joshpeterson]

Cool! So that indicates the OnReady method in the DiscordMgr class is still an instance method. That should be changed to be a static method as well.

[msciotti]

Hm, that's odd. OnReady is what I was mainly testing with, as it's the first callback fired after connecting.

To compare two calls:

// Not working?
[MonoPInvokeCallback(typeof(OnReadyInfo))]
 public static void ReadyCallback(ref DiscordUser connectedUser) { }
 public delegate void OnReadyInfo(ref DiscordUser connectedUser);

// Working
[MonoPInvokeCallback(typeof(OnErrorInfo))]
 public static void ErrorCallback(int errorCode, string message) { }
 public delegate void OnErrorInfo(int errorCode, string message);

// Handlers

public struct EventHandlers
{
  public OnReadyInfo readyCallback;
  public OnDisconnectedInfo disconnectedCallback;
  public OnErrorInfo errorCallback;
  public OnJoinInfo joinCallback;
  public OnSpectateInfo spectateCallback;
  public OnRequestInfo requestCallback;
}

It all seems to look legit? I did just realize that the two ready functions had different names for their variable, which I just fixed, but I feel like that shouldn't cause this error?

[joshpeterson]

That does all look correct. However, the problem might be elsewhere. Is there a class named DiscordMgr in the discord-rpc code? I don't see one. I wonder if this comes from the code @dylanh724 is working in.

[dylanh724]

@joshpeterson The main reason why this is awkward is because they have a public struct EventHandlers - it makes things a bit awkward. I can't redirect the callbacks straight to my own because Discord uses the EventHandlers for some sealed class.

// DiscordController.cs
handlers = new DiscordRpc.EventHandlers();
handlers.readyCallback += s_discordMgr.OnReady;
[...]

This is what I tried before -- if I could just skip the handlers, I could probably figure this out.

---

So we now have this:

// discordRpc.cs
[MonoPInvokeCallback(typeof(OnReadyInfo))]
public static void ReadyCallback(ref DiscordUser user) { }
public delegate void OnReadyInfo(ref DiscordUser connectedUser);
[ ... ]

public struct EventHandlers
{
	public OnReadyInfo readyCallback; // THESE should be static?
        [ ... ]
}

[DllImport("discord-rpc", EntryPoint = "Discord_Initialize", CallingConvention = CallingConvention.Cdecl)]
public static extern void Initialize(string applicationId, ref EventHandlers handlers, bool autoRegister, string optionalSteamId);
[ ... ]

// discordController.cs
void OnEnable()
{
        handlers = new DiscordRpc.EventHandlers();
        handlers.readyCallback += s_discordMgr.OnReady;
        [ ... ]
}

public void OnReady(ref DiscordRpc.DiscordUser connectedDiscordUser)
{
         // The goal is to get here.
}

[joshpeterson]

@dylanh724 I don't think the readyCallback field in the EventHandlers struct needs to be static. But the OnReady method in the DiscordMgr class does need to be static. Can you provide the code for the DiscordMgr class here so that I can have a look at it?

[dylanh724]

DiscordMgr is my own script (it was getting a bit confusing with the statics, leaving DiscordMgr my final callback). I called that through the static callback via a singleton. Only the first callback needs to be static, right?

Here are the scripts I use, watered down a bit as best as my 3am mind can do @joshpeterson
drpc-scripts.zip

I'll check here again in the morning (GMT+8)

[joshpeterson]

@dylanh724 This is rather confusing. It looks like the code you have should work. I don't see OnReady being marshaled as a callback for native code anywhere. When you have a chance, can you send me the full Unity project? I might be able to debug it and see what is wrong.

[dylanh724]

@joshpeterson GOT IT WORKING! I had a bad build, so tested an old build by accident. I really shouldn't be doing this at 3am ;D

But yeah, works (the exact code I sent you)! So this PR needs their callbacks to be static with that [] mono attribute.

@msciotti Check out the zip file above for a working build example. My code has a DiscordMgr which you can decide if you want or don't want. I added this because the static callbacks get confusing and you want to send it to a singleton / non-static instance somewhere. If you don't touch the callbacks, this makes UPDATING wayy easier (and the overall flow less confusing) if you leave DiscordMgr as the end-users logic. This also means they can name their callbacks whatever they want with minimal master repo changes. Anyway, feel free to cherry pick what you feel is best:

• DiscordRpc is untouched
• DiscordController handles init + the Handler callbacks. The handler callbacks at OnEnable need to be STATIC with that [] attribute above it. It then needs to be sent to a SINGLETON (since it's static), assuming you want to cache things and save things in an instance. In my example, I then callback to my DiscordMgr custom logic that handles the non-static (final) callbacks. You could optionally keep it in the current DiscordController, but this would encourage end-users to make custom logic, which makes updating very difficult.
• DiscordMgr has a singleton to call from the DiscordController static callback. It has the traditional callbacks you saw in the original (non-static) DiscordController before the il2cpp changes. Moved to this file to separate custom logic. Can be named something arbitrary.

About that Bounty

How to handle that one? Josh and msci both helped -- seems like a multi-effort thing. I toss $25 to each of you? To claim your bounty piece, DM me on Discord ( https://discord.gg/tol i42-Xblade ) with a PayPal email and I'll toss it over.

Good job, folks.

[joshpeterson]

@dylanh724 That is great to hear! I'm happy you have this working now.

About that Bounty

I'd rather not accept it - I'm just doing my job at Unity. :) If you still want to pay it though, I'd suggest a charity, maybe Girls Who Code?

[msciotti]

Seconded. If you really feel like it, Girls Who Code is fantastic. Thank you both for your help!

[msciotti]

@dylanh724 it seems like you ran into a similar issue that I did, which was errors when trying to assign the callbacks to methods in a singleton GameManager class.

I think we may want to get somewhere in the middle here. The existing code does work as is, and i don't want to implement a specific pattern in the example for folks who may want an example a little more basic.

Is there a way that we can structure things where the existing code works, and it's also easily extended to a use case like yours?

[dylanh724]

Is there a way that we can structure things where the existing code works, and it's also easily extended to a use case like yours?

The alternative would be in DiscordController to put the static callbacks at the top (or above each callback -- but maybe better at top since the average user won't ever touch them) and make a singleton for DiscordController (if there's not 1 already - I don't think so). Then change the non-static callbacks to private. And import AOT.

Or use an anonymous callback () =>, but it could get bloaty. Probably not. Something like ...

// ############################################################
// DiscordController
// ############################################################
using UnityEngine;
using AOT;

#region "Core Vars"
public string applicationId;
public string optionalSteamId;
public static DiscordController s_discordCtrl;
private DiscordRpc.EventHandlers handlers;
public DiscordRpc.RichPresence presence = new DiscordRpc.RichPresence();
public DiscordRpc.DiscordUser joinRequest;
#endregion 

#region "Demo Vars"
public int clickCounter;
public DiscordJoinEvent onJoin;
public DiscordJoinEvent onSpectate;
public DiscordJoinRequestEvent onJoinRequest;
public UnityEngine.Events.UnityEvent onConnect;
public UnityEngine.Events.UnityEvent onDisconnect;
public UnityEngine.Events.UnityEvent hasResponded;
// <And those serialization fields - seems like I deleted them>
#endregion

#region "Core Static IL2CPP Callbacks"
[MonoPInvokeCallback(typeof(DiscordRpc.OnReadyInfo))]
public static void OnReadyStatic(ref DiscordRpc.DiscordUser connectedDiscordUser)
{ s_discordCtrl.OnReady(ref connectedDiscordUser); }

[MonoPInvokeCallback(typeof(DiscordRpc.OnDisconnectedInfo))]
public static void OnDisconnectedStatic(int errorCode, string message)
{ DiscordController.s_discordCtrl.OnDisconnected(errorCode, message); }

[MonoPInvokeCallback(typeof(DiscordRpc.OnErrorInfo))]
public static void OnErrStatic(int errorCode, string message)
{ DiscordController.s_discordCtrl.OnErr(errorCode, message); }

[MonoPInvokeCallback(typeof(DiscordRpc.OnJoinInfo))]
public static void OnOtherJoinByPublicInvStatic(string secret)
{ DiscordController.s_discordCtrl.OnOtherJoinByPublicInv(secret); }

[MonoPInvokeCallback(typeof(DiscordRpc.OnRequestInfo))]
public static void OnMyReqToJoinOtherResStatic(ref DiscordRpc.DiscordUser request)
{ DiscordController.s_discordCtrl.OnMyReqToJoinOtherRes(ref request); }

[MonoPInvokeCallback(typeof(DiscordRpc.OnSpectateInfo))]
public static void OnOtherSpectateStatic(secret)
{ DiscordController.s_discordCtrl.OnOtherSpectate(secret); }
#endregion

#region "Demo Callbacks"
private void OnReady(ref connectedDiscordUser)
{
    Debug.Log("[DiscordCtrl] @ OnReady"];
}

// <...and the other callbacks that are normally here - just changed callback to private>
#endregion 

#region "Init/End"
private void Awake()
{
    Debug.Log("[DiscordController] @ Awake: Singleton+Persist");

    // Singleton + Persist
    if (s_discordCtrl == null)
    {
        s_discordCtrl = this;
        DontDestroyOnLoad(this.gameObject);
    }
    else if (this != s_discordCtrl)
        Destroy(gameObject);
}

private void OnEnable()
{
    // <The usual stuff>
}

private void OnDisable()
{
    // <The usual shutdown stuff>
}
#endregion 

^ This is cherry picked from both of my scripts and added back some of the demo items, but I may be missing some or syntax may be slightly off: Needs some verification. Something like this should work, though. May also want to confirm naming conventions, this was quick n dirty.

Apologies for not including everything, may seem sorta lazy - but alas... lacking some time today. Hopefully this helps.

---

It may be more accurate to use your own code and just move things around using the patterns above:

1. using AOT
2. Make a static singleton for DiscordController. It can probably be private, but I'm bad with static callbacks, so just made it public for now.
3. Make a static callback with the marshaling [] attribute. Inside{}, directly call your existing demo callbacks() through the singleton
4. Final callbacks (from demo) don't need to be public anymore. Make them private.
5. Optional: Use regions so users know what they can and cannot delete/change. Keywords like demo vs core for unfamiliar elements may help hint direction.

[MichalPetryka]

When will we get new release with this included?

[msciotti]

I wanted to try and roll a release together with this + choosing which pipe number to open to support choosing the Discord client to connect to. However, the Windows side of things hung me up on that other PR. Let me take some of Dylan's suggestions above and work on the example, and I can merge + release this this week.

[msciotti]

Alright, so I've been trying to poke at this a lot. I've gotten to more or less the same place that I was at before, which I'm not sure is the fault of the DiscordRpc stuff but perhaps just my unfamiliarity with doing this stuff in Unity the right way.

I was going about refactoring some stuff into a DiscordManager singleton. It looks roughly like this:

public class DiscordManager
{
    private static DiscordManager _instance;
    public static DiscordManager Instance
    {
        get
        {
            if (_instance == null)
            {
                _instance = new DiscordManager();
            }
            return _instance;
        }
    }

    public void Ready(ref DiscordRpc.DiscordUser connectedUser)
    {
        Debug.Log(string.Format("Discord: connected to {0}#{1}: {2}", connectedUser.username, connectedUser.discriminator, connectedUser.userId));
        // onConnect.Invoke();
    }
}

I can assign callbacks without issue:

 handlers = new DiscordRpc.EventHandlers();
 handlers.readyCallback += DiscordManager.Instance.Ready;

All this code runs perfectly fine, and callbacks fire. No need to change where the MonoPInvoke stuff is done. Where I run into issues that I cannot debug is when I try to invoke the UnityEvent from within the singleton. I get no errors; Unity just freezes until I force close it.

For the time being, I'm planning to merge this as-is. It will support compiling with il2cpp, and the example runs as expected. I do not know why the UnityEvent.Invoke() stuff is crashing. Perhaps @joshpeterson can shed some light there? I'm more or less out of ideas when it comes to trying to support a singleton GameManager type implementation properly 😕

[dylanh724]

@msciotti Try changing this:

if (_instance == null)
    _instance = new DiscordManager();

to this:

if (_instance == null)
    _instance = this;

I doubt it contributes to the freeze, but usually you want the instance to be this current instance, unless you're doing something beyond me or any previous vars set will be unset. With this, you have 2 instances running: The local script that sets this, and the new instance. This means that the handlers may be set in the current instance, but the singleton (3rd party) instance may not have it set.

....but if it's set at start/awake/onenable, then it'll set itself. Which means you'd have two callbacks. Maybe that's what's causing the freeze? Strange callback happenings with 2 instances?

No matter what code you put [that's not a stack overflow], you probably shouldn't get freezes... did you try stepping through with VS in the Unity editor?

---

While you're there,

Debug.Log(string.Format("

can be simplified to

Debug.LogFormat("

[MichalPetryka]

Unity freezes when you have infinite loop

[joshpeterson]

@msciotti Sorry, I don't have any ideas about why this causes a freeze.

[msciotti]

@dylanh724 I'll give that a go, see if it makes a difference. I'll try this out tomorrow and see where I get. If I can't make much more forward progress on that, I'll merge and roll a new release for the sake of at least actually supporting the il2cpp compiling stuff 👍

[msciotti]

Merging this to at least have a supporting release. I will continue to tinker with examples when I have time.

[dylanh724]

As an update, none of my callbacks are firing in Unity. Was something changed in the backend so it won't work when testing in the editor?

EDIT: Seems likely unrelated, moved this here: #261