Placeholder-Software / Dissonance

Unity Voice Chat Asset
70 stars 5 forks source link

Mirror Support with custom transport #138

Closed MichalPetryka closed 5 years ago

MichalPetryka commented 5 years ago

I know that you said in #124 that you won't support Mirror because of TCP, but what about mirror with custom transport? There's UDP based transport for Mirror, Ignorance https://github.com/SoftwareGuy/Ignorance/ Could you consider supporting it?

martindevans commented 5 years ago

That looks like a good backend for Mirror. However it's fairly new so we'll wait at least a few months to make sure that development on it continues and it's stable etc before really considering supporting it.

Don't forget that you don't need to wait for us to "officially" support a networking system. Mirror is meant to be a replacement for HLAPI, so you can take our HLAPI network integration and adapt it to work with Mirror. The person who initially requested Mirror support in #120 only had ~20 errors when he swapped it over (fairly minor ones, by the look of it). So I wouldn't anticipate that taking very long to fix.

MichalPetryka commented 5 years ago

One big difference in Mirror that might cause issues is that it doesn't have channels

martindevans commented 5 years ago

Dissonance needs unreliable transport for voice packets and reliable transport for session control packets, so if Mirror doesn't support channels we won't ever be able to support it. I just had a quick look through Ignorance and Mirror on Github and it looks like there is at least partial support for channels. Ignorance has this (note channelId). Mirror has this (added just one month ago). It might be worth contacting the mirror dev and asking what he's planning for channels, and how to use it.

Even if mirror itself does not support reliable/unreliable channels another thing you could do is send Dissonance/Reliable packets through Mirror and send Dissonance/Unreliable packets directly through ignorance. Of course that ties your network implementation to Ignorance. If you decide to do that I would suggest implementing Dissonance on top of Mirror by adapting the HLAPI integration and then once that is working you just need to modify the SendUnreliable method to use Ignorance directly.

MichalPetryka commented 5 years ago

Looking at code makes me think that everything will be working without modifications, Dissonance gives channel number in NetworkConnection.SendBytes, which Mirror passes to Ignorance, which passes it into ENET. I think that in mirror hardcoding 0 as Reliable and 1 as unreliable would make everything working

MichalPetryka commented 5 years ago

Another issue is that mirror doesn't have NetworkConnection.lastError and they said that they aren't planning to add it and they also said "just override OnServerError and OnServerClient in your NetworkManager and do whatever you want with the exception."

martindevans commented 5 years ago

We only use lastError to detect if a connection has timed out. It should be fairly easy to override the method they suggest and set a flag if the connection has timed out - then where the code currently checks lastError == NetworkError.Timeout just check if the flag has been set instead.

pallasathena1 commented 5 years ago

Is anyone else working on this? It looks like mirror 1.3 and ignorance 2017 are reasonably stable releases, and Coburn has said that ignorance2017 is just going to be in maintenance mode, to receive updates to maintain compatibility with mirror, but that all major changes to the ignorance UDP library will be in the 2018 branch.

martindevans commented 5 years ago

This is probably going to be our next network integration. I've been keeping an eye on Ignorance development and it looks good. I think I'll open some PRs next week to fix some issues I've seen with the library, and then if they get merged I'll put some time aside to develop a Dissonance+Mirror+Ignorance integration.

@MichalPetryka how did you get on with implementing this?

MichalPetryka commented 5 years ago

Sorry for not responding, first, i've temporarly deferred moving, second i'm creating a custom LiteNetLib UDP-based transport for Mirror, which will probably also be ok. However, i've also noticed that in mirror sendbytes is protected and you have to create a custom message with bytes for sending.

SoftwareGuy commented 5 years ago

I merged the ArraySegment PR of Martin's into Ignorance 1.2; so that part of the puzzle is ready to go.

I still have a few bugs plus Mac OS compatibility that I need to address before I would call Ignorance 1.2 stable. Plus there's a lot of loose ends that another developer pointed out, so I need to get those addressed too.

martindevans commented 5 years ago

Right now I have a prototype version of a Mirror integration that seems to mostly work (one more potential bug to investigate next week) - thankyou to BigBoxVR for sponsoring development of this! I hope to release this with the next version of Dissonance within a couple of weeks.

That may use the Ignorance 1.2 ArraySegment sending system, depending upon the release schedule of that. If the initial release of the integration doesn't use it I'll update the integration as soon as Ignorance 1.2 gets released :)

MichalPetryka commented 5 years ago

@martindevans could we get those methods as an interface for transport, so that other transports will also work with it?

martindevans commented 5 years ago

Nope, Mirror rejected my PR which would add that. Until something like that is added I can either send packets through Mirror (lots of allocations) or directly through Ignorance (less allocations, tied to Ignorance transport) :(

MichalPetryka commented 5 years ago

Could you take a look at https://github.com/SoftwareGuy/Ignorance/pull/25 ?

martindevans commented 5 years ago

I've looked at it. With Dissonance I plan to add a runtime test which would simply check:

if (NetworkManager.instance.transport is IgnoranceTransport) {
    // Use zero alloc methods
} else {
    // Fallback to standard methods
}

So your change does help with plugging in new backends to Dissonance+Mirror. If it's rejected then of course it's easy enough to add a new check into that branch for whatever transport you specifically want to use.

martindevans commented 5 years ago

Dissonance 6.3.1 just went live on the asset store, check out the release notes for all the details including the download link for the Mirror+Ignorance integration!