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

Refactor PlayroomKit #40

Closed
momintlh opened this issue Jan 30, 2024 · 13 comments
Closed

Refactor PlayroomKit #40

momintlh opened this issue Jan 30, 2024 · 13 comments
Labels
enhancement New feature or request recurring

Comments

@momintlh
Copy link
Collaborator

momintlh commented Jan 30, 2024

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
Copy link
Collaborator

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.

@SaadBazaz SaadBazaz added the enhancement New feature or request label Jan 30, 2024
@momintlh
Copy link
Collaborator Author

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

Untitled-2023-12-04-2253

@SaadBazaz
Copy link
Collaborator

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

@SaadBazaz
Copy link
Collaborator

SaadBazaz commented May 11, 2024

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:
#67

@SaadBazaz
Copy link
Collaborator

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
Copy link
Collaborator

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

@momintlh momintlh mentioned this issue Jun 1, 2024
@SaadBazaz
Copy link
Collaborator

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
Copy link
Collaborator Author

momintlh commented Jun 8, 2024

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
Copy link
Collaborator

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?

@momintlh momintlh mentioned this issue Jun 13, 2024
@SaadBazaz
Copy link
Collaborator

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

@momintlh momintlh mentioned this issue Aug 29, 2024
3 tasks
@SaadBazaz
Copy link
Collaborator

PlayroomKit interface is yet to be made @momintlh

@momintlh
Copy link
Collaborator Author

momintlh commented Sep 5, 2024

PlayroomKit interface is yet to be made @momintlh

Base Interface / abstract class? Yes

@SaadBazaz
Copy link
Collaborator

We will now track this in the project, in the form of separate tasks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request recurring
Projects
None yet
Development

No branches or pull requests

2 participants