Placeholder-Software / Dissonance

Unity Voice Chat Asset
71 stars 5 forks source link

[bug] mirror client isReady breaks dissonance plugin integration #253

Closed RichNextGen closed 2 years ago

RichNextGen commented 2 years ago

Context

I have a project in which I load and unload multiple scenes using mirror while multiple clients are connected.

Expected behaviour

I expect the new scene to load and Dissonance to continue working for voice chat functionality with the connect clients, once the scene has finished loading. It is okay if the clients temporarily stop hearing each other while isReady is false.

Actual behaviour

As soon as isReady is set to false for a client, MirrorIgnoranceServer.cs's update checks IsConnected(), which returns false if the client is not ready. This then calls ClientDisconnected() and that client is disconnected and no longer sends or receives audio.

Workaround / Fix

I attempted various workarounds by creating a new Server inheriting from the MirrorIgnoranceServer, however due to the way MirrorIgnoranceServer is set up, I was unable to just disable this functionality.

Because of that, I modified the source code, changing IsConnected() to not check isReady, which successfully fixed my problem. However, it does cause a lot more dissonance warnings to be thrwon during the time that isReady is false.

Your Environment

Include as many relevant details about the environment you experienced the bug in

martindevans commented 2 years ago

Once IsReady becomes true again Dissonance should notice this and reconnect, is that not what happens?

RichNextGen commented 2 years ago

It does not reconnect. I will add that I am manually calling NetworkServer.SetClientNotReady, and later NetworkServer.SetClientReady, which was necessary to support addressable scene loading. However, the same method calls are used by Mirror's internal NetworkManager.ServerChangeScene method, so I'm not sure why or if that would be an issue.

martindevans commented 2 years ago

Do you ever see IsReady become true or is it permanently stuck on false? If you do see it become true, which other thing is blocking it from reconnecting (see the code in Assets/Dissonance/Integrations/MirrorIgnorance/MirrorIgnoranceCommsNetwork.cs around Line 41)?

RichNextGen commented 2 years ago

So in this case it's not NetworkClient.connection.isReady that is the issue (as referenced on line 41), as that is on the client, rather the issue is the server-side conn.isReady called for the server's connection to the client, on line 83 of MirrorIgnoranceServer.cs. That said, I will go ahead and debug that line in case I am misunderstanding.

Edit: image NetworkClient.connection.isReady is true, but as mentioned above I don't think the client-side isReady is the important one, rather the server-side isReady.

martindevans commented 2 years ago

Sorry, I lost track of this because I didn't get a notification for your edit with more info.

Looking back on it I think IsReady isn't the issue but possibly a mismatch between the server and the client. The server sees the client become non-ready and calls ClientDisconnected, removing it from the session. However, since the client is not actually disconnected it presumably still thinks it is in the session and so it never tries to reconnect.

Changing IsConnected to not check IsReady is a workaround for this, but it will cause Dissonance to startup too soon (before the connection is actually ready) which is what triggers all the warnings you mention.

The best way to handle this is probably to simply destroy/disable Dissonance on the client side before the scene changed and recreate/enable it afterwards. Connecting to the server is very fast (2 roundtrips).