Skip to content
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

[$50 Bounty] RP completely broken with il2cpp (Unity 2018.2 Stable): Needs dll marshaling #243

Closed
dylanh724 opened this issue Oct 19, 2018 · 30 comments

Comments

@dylanh724
Copy link

dylanh724 commented Oct 19, 2018

Reviving this issue:

#186

Background

Unity 2018.2 now only builds il2cpp for Windows UWP and now also gives the option for il2cpp builds for Windows & Mac standalone (which is definitely the future for security+performance) -- this version is not a beta. Very soon, you'll see tons of people raising this issue: a permanent fix in a newer version with cross-compatibility would be ideal~

The Issue

The answer within the #186 was not explained in detail for those not too familiar with DLL madness (very little interaction with DLL's from Unity users).

I found where to put this attribute, but don't know what to do with the errors that follow.

@mdsitton was the original person that answered #186 and @franckmolto seemed to resolve it from that answer: Any of you guys up to sharing your result?

Bugs reported from logs

[Discord] init @ OnEnable
UnityEngine.DebugLogHandler:LogFormat(LogType, Object, String, Object[])
UnityEngine.Logger:Log(LogType, Object)
UnityEngine.Debug:Log(Object)
DiscordController:OnEnable()

(Filename: C:\buildslave\unity\build\Runtime/Export/Debug.bindings.h Line: 43)

NotSupportedException: IL2CPP does not support marshaling delegates that point to instance methods to native code.
  at DiscordController.OnEnable () [0x00000] in <00000000000000000000000000000000>:0 

Original Code

[MonoPInvokeCallback(typeof(DiscordRpc.ErrorCallback))]

So you have this @ DiscordRpc:

[UnmanagedFunctionPointer(CallingConvention.Cdecl)]
public delegate void ErrorCallback(int errorCode, string message);

public struct EventHandlers
{
    public ErrorCallback errorCallback;
    [...]
}

Suggested Change from #186

If you change it to this:

using AOT; // Parent of MonoPInvokeCallback

[MonoPInvokeCallback(typeof(DiscordRpc.ErrorCallback))]

So you have this @ DiscordRpc:

[MonoPInvokeCallback(typeof(DiscordRpc.ErrorCallback))] // 
public static void ErrorCallback(int errorCode, string message);

public struct EventHandlers
{
    public ErrorCallback errorCallback;
    [...]
}

image
image

It starts getting confusing, starting with ErrorCallback not being a type

Cross-Compatibility

From my research (someone else can confirm), converting to dll marshaling using the [MonoPInvokeCallback] attribute and static callbacks has claims of cross-compatibility, exampled at steamworks.net and facepunch.steamworks repo's (or a quick Google search).

@dylanh724
Copy link
Author

The keyword is AOT (Ahead of Time) - we need AOT-friendly delivery of the dll reads.

@dylanh724 dylanh724 changed the title il2cpp marshaling: Dominant bug in Unity 2018 il2cpp marshaling: Dominant bug in Unity 2018 - Need permanent fix Oct 21, 2018
@msciotti
Copy link
Collaborator

Responded to the closed issue without seeing this open first. Sure, if il2cpp is the new standard in Unity, we definitely need to come up with a fix and ship a new version. No problem.

If the static class method implementation in the closed issue is a valid fix, I'm more than happy to accept a PR that does as much. It won't break any existing implementations, as the native stuff here is pretty well abstracted from what actually "happens" in JS land in Discord, but I do sense some unknowns around how this plays with older Unity versions, which many folks will still use.

Plenty of folks asking questions around VS2010 oddities when it comes to our newer game SDK 😛

@dylanh724
Copy link
Author

dylanh724 commented Oct 23, 2018

Thread Merge

Responded to the closed issue without seeing this open first
To Consolidate:
I'm perfectly happy accepting a PR for the static class member changes if it won't break existing implementations (and it shouldn't?).

Unless you have suggestions for a more "real" solution. As mentioned above, neither myself nor snowyote are well-versed in this il2cpp stuff, so we're going a bit on community knowledge here.

Alas, I'm not good, either. I tried a few things, but failed. Something to do with turning the callbacks into static callbacks tagged with (for ErrorCallback's example) [MonoPInvokeCallback(typeof(DiscordRpc.ErrorCallback))]

Cross Compatible? Yes!

I read that it's cross-compatible. Steamworks.net had to do the same thing and confirms cross-compatability: Perhaps see here for clues @ rlabrecque/Steamworks.NET#227 and https://gist.github.com/GMMan/7a8a475b703363d2911fd20f238caef0

Then the importdll[] tags probably have to be changed to something similar, too. Maybe? I tried researching, but I wasn't even familiar with dll madness to begin with :P I mostly use native C# libraries, personally.

Anyone able to come to the rescue?

@dylanh724
Copy link
Author

dylanh724 commented Oct 23, 2018

💰 Bounty!

$50 PayPal to whoever first posts the correct il2cpp-friendly solution below (that I can confirm works).

🔍 Hints:

  • There are tons of links and examples in this issue that may show possible patterns that would work.
  • The solution has something to do with turning a dll callback delegate into a static one and with use of the [[MonoPInvokeCallback(typeof(DiscordRpc.ErrorCallback))]] MonoPInvokeCallback attribute (exampled for the ErrorCallback).
  • If you use that attribute above, it's said that the statement below needs to be a full() function (not end with a ; like a delegate)
  • Importing the importdll()'s may need something extra too.
  • All of the changes should be found at this single file: https://github.com/discordapp/discord-rpc/blob/master/examples/button-clicker/Assets/DiscordRpc.cs
  • You may find further hints at steamworks.net's WIP implementation that uses similar methods. Their open issue also has hints.

💡 If you complete the bounty...

  1. Reply here with the solution before someone else does!
  2. Reach out to me @ https://discord.gg/tol as i42-Xblade with your email

@pipliz
Copy link

pipliz commented Oct 24, 2018

I'd like to point out that IL2CPP is the new standard for UWP builds only - "standalone" windows builds using mono/.net are not being deprecated.

See the section about it at https://blogs.unity3d.com/2018/07/10/2018-2-is-now-available/

@msciotti
Copy link
Collaborator

@pipliz thank you for that documentation. That's mainly what I'm referring to. Unity deprecating .NET altogether seemed a bit fishy to me :p I think we can definitely come up with a solve for folks who want to make the switch, and just ensure it doesn't break other stuff.

@dylanh724
Copy link
Author

Ah, my bad -- I updated OP

@dylanh724 dylanh724 changed the title il2cpp marshaling: Dominant bug in Unity 2018 - Need permanent fix [$50 Bounty] RP completely broken with il2cpp (Unity 2018.2): Needs dll marshaling Oct 31, 2018
@dylanh724 dylanh724 changed the title [$50 Bounty] RP completely broken with il2cpp (Unity 2018.2): Needs dll marshaling [$50 Bounty] RP completely broken with il2cpp (Unity 2018.2 Stable): Needs dll marshaling Oct 31, 2018
@dylanh724
Copy link
Author

dylanh724 commented Nov 1, 2018

@msciotti
Copy link
Collaborator

msciotti commented Nov 1, 2018

Based on the solution offered in #186, this seems like something that is handled more in the use of the methods, rather than the underlying source code. That is to say, handled in DiscordController.cs which is an example, rather than DiscordRpc.cs which is the "header" file.

Perhaps @mdsitton would be able to share with us a bit more of his code, but in the meantime, I am trying to finagle the example to get it working.

EDIT: I do not know that for sure, but trying to parse his short response, that's what I am assuming. I could be, and most certainly am, wrong.

@dylanh724
Copy link
Author

dylanh724 commented Nov 2, 2018

It has to do with the initial DLL callback -- it must be a static callback and handled as a non-delegate (ending with () instead of ;). Then swap an attribute tag.

This file should be the culprit: https://github.com/discordapp/discord-rpc/blob/master/examples/button-clicker/Assets/DiscordRpc.cs


So we have this (of 7 funcs); I'm just going to use ready examples:

[UnmanagedFunctionPointer(CallingConvention.Cdecl)]
public delegate void ReadyCallback(ref DiscordUser connectedUser);

public struct EventHandlers
{
	public ReadyCallback readyCallback;
}

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

This needs to changed to look something like this (again, I'm still getting that error, but for directions sake):

// DiscordRpc.cs
using AOT;

public delegate void OnReadyInfo(ref DiscordUser connectedUser);

[MonoPInvokeCallback(typeof(OnReadyInfo))]
public static void ReadyCallback(ref DiscordUser connectedUser)
{
      UnityEngine.Debug.Log("[DiscordRpc] @ ReadyCallback");
}

public struct EventHandlers
{
	public ReadyCallback readyCallback; // Unchanged?
}

// From the examples I've seen, I don't think this line is the issue -- but the callback, rather.
[DllImport("discord-rpc", EntryPoint = "Discord_Initialize", CallingConvention = CallingConvention.Cdecl)]
public static extern void Initialize(string applicationId, ref EventHandlers handlers, bool autoRegister, string optionalSteamId);

The actual err does occur at DiscordController.cs, however, the true error comes from DiscordRpc: Here is what is happening @ DiscordController -- after OnEnable, it goes straight to DiscordRpc.cs before the err

// DiscordController.cs
void OnEnable()
{
        handlers = new DiscordRpc.EventHandlers(); // << This is where the il2cpp err happens: Inside DiscordRpc. Stacktrace will only show it's from DiscordController because the parent starts at OnEnable.

        handlers.onReadyInfo = ReadyCallback;
        [...]
}

AOT Requirements TL;DR Seem to Be:

  • When you import a DLL, the callback must be static (original ReadyCallback was not static)

  • The initial callback must not be a delegate and must end with (), not ; (ReadyCallback was a delegate)

  • The callback must have the attribute [MonoPInvokeCallback] (via using AOT;), recommended to include the typeof (ReadyCallback used [UnmanagedFunctionPointer])

@dylanh724
Copy link
Author

dylanh724 commented Nov 2, 2018

Porting a recent comment over from #186 from @joshpeterson

image

Hey, I'm a dev at Unity working on IL2CPP, so I thought I might help here.

The use of the MonoPInvokeCallback attribute for cases where a managed delegate is marshaled to native code is required for both IL2CPP and Mono AOT. As it turns out, Unity won't be supporting Mono AOT on many platforms soon (and currently we don't support too many), but this is a more general AOT issue.

The AOT code generator needs to know which managed methods could be called from native code, as it needs to generate marshaling wrappers. Doing this for all managed methods is not feasible, so the MonoPInvokeCallback attribute guides the AOT code generator.

While IL2CPP is now an option for desktop standalone player builds in Unity 2018.2 and later, we plan to continue to support Mono JIT for those platforms. We don't plan to require IL2CPP as is the case on iOS, for example.

Still, using MonoPInvokeCallback is the correct solution here, and should have no impact for other cases. The chances of it breaking something else are pretty low (or zero).

@joshpeterson
Copy link

I think this is on the right track.

When you import a DLL, the callback must be static (original ReadyCallback was not static)

The callback must have the attribute [MonoPInvokeCallback] (via using AOT;), recommended to include the typeof (ReadyCallback used [UnmanagedFunctionPointer])

This is correct. This code is trying to get a function pointer to the managed method ReadyCallback, and give it is native code via the Initialize method, so the native code can call back into the managed code at some point later.

These two requirements are correct. The method which will be called back from native code needs to be a static method and it needs to have the MonoPInvokeCallback attribute.

The initial callback must not be a delegate and must end with (), not ; (ReadyCallback was a delegate)

I'm not entirely sure what this requirement means.

Based on the code above though, I think you need one modification:

// DiscordRpc.cs
using AOT;

public delegate void OnReadyInfo(ref DiscordUser connectedUser);

[MonoPInvokeCallback(typeof(OnReadyInfo))]
public static void ReadyCallback(ref DiscordUser connectedUser)
{
      UnityEngine.Debug.Log("[DiscordRpc] @ ReadyCallback");
}

public struct EventHandlers
{
        // I think this is the change we need.
	// public ReadyCallback readyCallback; // Unchanged?
        public OnReadyInfo readyCallback; // The type of this field should be OnReadyInfo
}

// From the examples I've seen, I don't think this line is the issue -- but the callback, rather.
[DllImport("discord-rpc", EntryPoint = "Discord_Initialize", CallingConvention = CallingConvention.Cdecl)]
public static extern void Initialize(string applicationId, ref EventHandlers handlers, bool autoRegister, string optionalSteamId);

If this does not fix the issue, can you provide more details about what the error is? Thanks!

@msciotti
Copy link
Collaborator

msciotti commented Nov 2, 2018

Hi josh! Thanks a ton for this helpful info and for chiming in. Based on your and Dylan's notes this definitely seems like a change we need to make. No problem at all there.

The second piece will be updating the example code in DiscordController.cs to work with the new "header" file. As we swap these callbacks to statics, the code example throws a number of errors about requiring object references when Invoke()ing the UnityEvents. I've refactored them into a singleton GameManager:

public class GameManager {
  private static GameManager instance = null;
  public static GameManager Instance {
    get
    {
      if (instance == null)
      {
        instance = new GameManager();
      }
      return instance;
    }
  }
  public UnityEngine.Events.UnityEvent onConnect;
  public UnityEngine.Events.UnityEvent onDisconnect;
  public UnityEngine.Events.UnityEvent hasResponded;
}

And changing the invokes to:

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

This makes the compiler happy, but seems to cause some runtime crashes in the player. Starting the player will result in a freeze after Discord: init without any errors thrown. So, that's where I am at as of this morning 😄

EDIT: I realize this is now an implementation issue rather than a native code issue, but if we've got the example up, we need to make sure it's as easy as Open -> Play for people learning.

@msciotti
Copy link
Collaborator

msciotti commented Nov 2, 2018

Can confirm that UnityEvent.Invoke() seems to be the culprit. This freezes the player:

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

This works successfully and the Debug Log is printed:

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

@joshpeterson
Copy link

I'm curious about the specific errors you get when calling Invoke here. Can you provide more details?

@msciotti
Copy link
Collaborator

msciotti commented Nov 2, 2018

It actually isn't throwing any errors on my end; the Unity player freezes up and needs to be force quit. Perhaps there's some logging I can attach?

@joshpeterson
Copy link

Is there any information in the Unity player log?

@msciotti
Copy link
Collaborator

msciotti commented Nov 2, 2018

Not much from what I've seen, but here's a couple videos.

Not working when invoking events: https://gfycat.com/ShrillDigitalAnglerfish
Working when removing the Invoke(): https://gfycat.com/QuickDearDogwoodtwigborer

Sorry for the gfycat links; github doesn't like mp4 embeds apparently!

If there's some logging I can attach that I might be missing, would love to know. I'm a Unity newbie for the most part. My changes are also reflected in #249 if anyone would like to pull it down themself.

@msciotti
Copy link
Collaborator

msciotti commented Nov 2, 2018

Think I found my own issue. The callbacks in DiscordController.cs did not need to be static; that was my misunderstanding. So there's no need for a singleton and the "normal" code works just fine. Updating PR.

@msciotti
Copy link
Collaborator

msciotti commented Nov 2, 2018

@dylanh724 could you pull down my PR and see if it fixes the issue for you? It works on my machine for il2cpp and Mono, but would love a second confirmation before merging.

@dylanh724
Copy link
Author

dylanh724 commented Nov 4, 2018

@msciotti Thanks for progressing on this! I copy+pasted your DiscordRpc changes without changing it. I then moved my custom logic into its own file, leaving DiscordController a 99% copy+paste for init sequence:

Works fine in editor,

However, I was unable to get this working in a standalone build, getting the same error:

image

NotSupportedException: IL2CPP does not support marshaling delegates that point to instance methods to native code.
  at DiscordController.OnEnable () [0x00000] in <00000000000000000000000000000000>:0 

What's your build info? Mine is:

  • Unity 2018.2.12f1
  • il2cpp (not Mono)
  • .NET 4.x
  • Windows Standalone x64 build

image

@msciotti
Copy link
Collaborator

msciotti commented Nov 5, 2018

We can continue tracking this over in the PR. But my settings are as follows:

image

image

I'm testing over on macOS. Unity build 2018.3.0b8

@msciotti
Copy link
Collaborator

This didn't get auto-closed with the PR, so closing.

@mdsitton
Copy link

mdsitton commented Dec 6, 2018

I'm a little late to the party, but glad this issue was figured out eventually 👍

@derwodaso
Copy link
Contributor

derwodaso commented Dec 7, 2018

Is there an official solution here now? I am still getting the IL2CPP does not support marshaling delegates that point to instance methods to native code. error in the build when compiling the button-clicker example with IL2CPP:

My solution after trying around a bit was the following (not sure if that all makes sense but it seems to work):

In DiscordRpc.cs:

// public EventHandlers are set from the DiscordController.cs instead of passing in the handlers when initializing:
public static EventHandlers Callbacks;

[MonoPInvokeCallback(typeof(OnReadyInfo))]
public static void ReadyCallback(ref DiscordUser connectedUser) { Callbacks.readyCallback(ref connectedUser); }
public delegate void OnReadyInfo(ref DiscordUser connectedUser);

[ ... ]

public static void Initialize(string applicationId, bool autoRegister, string optionalSteamId)
{
	EventHandlers eventHandlers = new EventHandlers();
	// asign the static methods above, which invoke the Callbacks
	eventHandlers.readyCallback += DiscordRpc.ReadyCallback;
	eventHandlers.disconnectedCallback += DiscordRpc.DisconnectedCallback;
	eventHandlers.errorCallback += DiscordRpc.ErrorCallback;
	eventHandlers.joinCallback += DiscordRpc.JoinCallback;
	eventHandlers.spectateCallback += DiscordRpc.SpectateCallback;
	eventHandlers.requestCallback += DiscordRpc.RequestCallback;
	InitializeInternal(applicationId, ref eventHandlers, autoRegister, optionalSteamId);
}
[ ... ]

In DiscordController.cs I replaced the OnEnable() with this:

void OnEnable()
{
	Debug.Log("Discord: init");
	DiscordRpc.Callbacks.readyCallback = ReadyCallback;
	DiscordRpc.Callbacks.disconnectedCallback = DisconnectedCallback;
	DiscordRpc.Callbacks.errorCallback = ErrorCallback;
	DiscordRpc.Callbacks.joinCallback = JoinCallback;
	DiscordRpc.Callbacks.spectateCallback = SpectateCallback;
	DiscordRpc.Callbacks.requestCallback = RequestCallback;
        DiscordRpc.Initialize(applicationId, true, optionalSteamId);
}

@msciotti
Copy link
Collaborator

msciotti commented Dec 7, 2018

@derwodaso Hm, the example as-is in the repo works fine for me? Here are my steps:

  1. Get the repo fresh (git reset --hard HEAD && git pull && git clean -xdf)
  2. python build.py
  3. Open unity -> button-clicker
  4. Swap to il2cpp compiler -> restart
  5. Press play

I get my ready callback with my userid in it:

image

This does not work for you?

@franckmolto
Copy link

franckmolto commented Dec 7, 2018 via email

@derwodaso
Copy link
Contributor

derwodaso commented Dec 8, 2018

@franckmolto is correct, the problem only occurs in the build. The editor worked right away.

Also I got the relevant DLL's directly (most recent), not building them. Might that be another issue @msciotti?

@msciotti
Copy link
Collaborator

Hm, OK can confirm, I didn't realize it was failing on a built app. @derwodaso if your changes work and you'd like to PR them, I'm happy to review and test and merge.

@derwodaso
Copy link
Contributor

@msciotti done! Did some minor changes to my previous version to keep the signature of the Initialize method the same. So no changes are required in DiscordController.cs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

7 participants