Prevent duplicated OnPlayerJoin callbacks when rejoining different rooms

[SaadBazaz]

What happened?

Summary

Currently, everytime you call PlayroomKit.OnPlayerJoin(…) a new callback is added to the list. So when a player joins for the first time, only one callback is called. But if that player is kicked then joins another room, 2 callbacks are going to be called, and the player is going to be added twice. That keeps increasing everytime you kick and join.

There should be a way to prevent that.

Description

Part of the problem is due to the fact that PlayroomKit.OnPlayerJoin needs to be called to join the player. This is the only way to make the player to the room after InsertCoin. My first attempt to workaround that was to have a bool alreadySetCallback set to true after the first call to PlayroomKit.OnPlayerJoin. So the next time I call InsertCoin I would know I have already added the callback and not add it again. However, in this case I need to call PlayroomKit.OnPlayerJoin otherwise, the player would not join the room. So this workaround does not work. The second attempt was to call PlayroomKit.OnPlayerJoin normally, adding the callback again, but inside the callback check if the player is already added then in that case return. This works! However, this causes a memory leak (the list of callbacks can grow forever). Also, implementation wise, I believe that calling a callback in the same method/function that sets/adds it is a bad implementation. Those things should be separated.

Sample code

Let me try to explain the problem again, with a real case scenario: Consider a code similar to this

public class NetworkManager
{
    public void Initialize(string roomId)
    {
        PlayroomKit.InsertCoin(
        new PlayroomKit.InitOptions() {roomCode = roomId},
        () => { PlayroomKit.OnPlayerJoin(AddPlayer); }
        );
    }

    public void Deinitialize()
    {
        //    Call OnQuit for this player in each client
    }

    private void AddPlayer(PlayroomKit.Player player)
    {
        Debug.Log("Added Player " + player.id);

        player.OnQuit(RemovePlayer);
    }

    private void RemovePlayer(string id)
    {
        Debug.Log("Removed Player " + id);
    }
}

Let's say I want the player to be able to enter and exit rooms by calling Initialize and Deinitialize. The first time I call Initialize, InsertCoin is called and an OnPlayerJoin callback is added to PlayroomKit. If for whatever reason the player needs to exit the room and enter a new one, he needs to call Deinitialize then Initialize for a second time. That means a new OnPlayerJoin callback is added, but the previous one is still there. That causes a situation in which the same method is called twice every time a player enters the room. And the problem keeps increasing every time a new room is created. There should be a way to prevent that. Maybe the dev could manually remove a callback or clear the callback list. But that seems counterintuitive. Having only 1 callback instead of multiples could be a better option.

Version

0.0.18

What is your environment?

Unknown

Link to original discussion

https://discord.com/channels/997752993598419044/1224810795393613904

Relevant log output

N/A

[SaadBazaz]

There are multiple solutions on the top of my head:

1. Clearing out all existing state within the Unity SDK (callbacks, states, etcetera) in the first line of insertCoin

2. Adding some sort of check to see if a function Handle already exists in the callback (i.e. only keeping unique callbacks)

@momintlh

[momintlh]

Isn't this similar to this issue @SaadBazaz

callbacks are added again on joining a new room and are added again to the list.

[SaadBazaz]

Isn't this similar to this issue @SaadBazaz

callbacks are added again on joining a new room and are added again to the list.

I think it's eerily similar, if not the same. It could guide us to implement that PR better.

i.e. we need a mechanism to track duplicates, or should clear out callbacks on insertCoin.

[momintlh]

I'll reproduce this issue with the same scenario described above and test with the fixes I just pushed in #51.

[SaadBazaz]

We've released 0.0.19 with a possible solution to this problem.

OnPlayerJoin and OnQuit now return functions which you can call to unsubscribe your callbacks. That means, when you Deinitialize, just call these unsubscribers and you won't have repeating callbacks!

See the release here: https://github.com/asadm/playroom-unity/releases/tag/v0.0.19