asadm / playroom-unity

9 stars 1 forks source link

Refactor PlayroomKit #40

Open momintlh opened 5 months ago

momintlh commented 5 months ago

This isn't a feature/bug issue but rather an issue for refactoring/cleaning the SDK. As we are using callbacks in many parts of the C# wrapper, it is getting quite messy, we have the following pattern in multiple parts:

private static Action SomeCallback = null;

[DllImport("__Internal")]
private static extern void InternalJSLIBFunction(Action callback)

// Callback Invoker
[MonoPInvokeCallback(typeof(Action))]
private static void CallBackInvoker()
{
    SomeCallback?.Invoke();
}

// Exposed Function
public static void ExposedFunction(Action callback) 
{
SomeCallback = callback;
InternalJSLIBFunction(CallBackInvoker);
}

And to use the function we usually do something like this:

PlayroomKit.ExposedFunction(() = > {
   // Do Something when callback is invoked
})

or

PlayroomKit.ExposedFunction(callback);
void callback() 
{
// Do Something when callback is invoked
}

What we can do is to make a custom class for handling callbacks this will simplify the current codebase and make it easier for future updates/fixes.

SaadBazaz commented 5 months ago

Yeah a class for Callbacks (kind of like a CallbackFactory or something) might be cool.

Also, can use Macros for repetitive code, like the if playroom is not initialized guard.

momintlh commented 5 months ago

Side by side working of JSLIB & C# handling callbacks:

Untitled-2023-12-04-2253

SaadBazaz commented 4 months ago

We need better error handling to distinguish between JS errors and Unity errors. Inspired by #38

SaadBazaz commented 1 month ago

The current RPC implementation is unstable. Using setState to manage rpcEventNames can cause race conditions under high loads, and may not guarantee delivery (specially first delivery, as there will be network delay between setState and the RPC delivery)

This needs to be reliably managed. A way to do this is by delivering metadata alongside each RPC call, or use a predictable key generator to generate function IDs.

Reference: https://github.com/asadm/playroom-unity/pull/67

SaadBazaz commented 1 month ago

We also should separate our header and definition files, and give the project better structure and abstraction. Not only will it make our maintenance much easier, but later it may give us an opportunity to build C#-native functionality.

SaadBazaz commented 1 month ago

Thoughts on filestructure:

utils
|_ CallbackFactory.cs
|_ Errors.cs
modules
|_ PlayroomKit
  |_ PlayroomKit.h // <-- declaration file
  |_ PlayroomKit.cs // <-- definition file
  |_ PlayroomKit.mock.cs // <-- mockmode file (only overrides the functions which are defined here, other should be same as definition file
  |_ Player
    |_ ... // <-- similar 3 files here
    |_ Profile
      |_ Profile.cs // <-- similar 3 files here
      |_ Color.cs // <-- similar 3 files here
SaadBazaz commented 1 month ago

My thoughts on how we can implement Mock Mode in a more structured, cleaner manner. Code is in JS but you may get the gist:

image

momintlh commented 3 weeks ago

My thoughts on how we can implement Mock Mode in a more structured, cleaner manner. Code is in JS but you may get the gist:

Our base class is a static class with static members, inheritance and overriding isn't going to work with that.

SaadBazaz commented 3 weeks ago

Our base class is a static class with static members, inheritance and overriding isn't going to work with that.

Maybe create a common interface on top?

SaadBazaz commented 2 weeks ago

We need to decouple the NetworkManager from the PlayroomKit API. That will enable us to create an implementation for WebGL vs mobile vs desktop