RevenantX / LiteNetLib

Lite reliable UDP library for Mono and .NET
https://revenantx.github.io/LiteNetLib/index.html
MIT License
3k stars 489 forks source link

Replace UnitySocketFix with thread safe. #515

Closed FirstGearGames closed 1 year ago

FirstGearGames commented 1 year ago

This change replaces UnitySocketFix with PausedSocketFix, a thread safe and less complicated version.

UnitySocketFix would break when LiteNetLib was run on a thread since it's initialization involved creating a gameObject and adding a component.

PausedSocketFix uses thread safe events and a non-monobehaviour class. PausedSocketFix also brings enhancements to not drop the connection when the application loses focus, but rather only try to reconnect on focus gain if connection was lost.

RevenantX commented 1 year ago

@FirstGearGames thanks. But at first it doesn't compiled on Appveyor. And second Finalizer of PausedSocketFix will never be called because you subscribed to event. I think you should unsubscribe in NetManager.Stop.

FirstGearGames commented 1 year ago

I'm not sure why it's not compiling via Appveyor as it is in my project. I'll double check that ... You're right about the deinitializer though, does not seem to call when NetManager is destroyed. Will resolve.

Edit: not compiling because defines were stripping out needed code. I'll have a new PR in a few.

FirstGearGames commented 1 year ago

Resolved issues.

RevenantX commented 1 year ago

@FirstGearGames also this is really needed only on IOS devices. In other cases all things working fine.

FirstGearGames commented 1 year ago

Other devices can lose connection as well, WebGL depending on browser and Android too.

RevenantX commented 1 year ago

@FirstGearGames surely they can lose and this is normal behaviour that is expected. This created for bug in IOS Unity code that makes socket in some strange unusable state that requires such fix.

FirstGearGames commented 1 year ago

Normal behavior but not desired behavior. If the user wants to drop the connection they would call stop, which would prevent this from reconnecting. Otherwise this will try to re-establish a connection as that was the last intended state.

RevenantX commented 1 year ago

@FirstGearGames UDP socket doesn't have such thing as "connection". They always in "Listening"/"Sending" state. There was problem in IOS that closed socket by OS. I don't remember cases when Android destroys only socket (but not application entirely) and you needed to restart NetManager. Also WebGL doesn't support UDP (in normal meaning). And LiteNetLib doesn't support WebGL

FirstGearGames commented 1 year ago

I simply added UDP because I was throwing every potential define at it to detect a Unity project, since Unity doesn't have a 'UNITY' define, which is silly.

I'd like to see this included to support multi-threading but also because this does not just fix the connection it prevents the socket from automatically disconnecting on focus lost, which your version was.

RevenantX commented 1 year ago

I simply added UDP because I was throwing every potential define at it to detect a Unity project, since Unity doesn't have a 'UNITY' define, which is silly.

I'd like to see this included to support multi-threading but also because this does not just fix the connection it prevents the socket from automatically disconnecting on focus lost, which your version was.

Unity has define UNITY_X_Y_OR_NEWER. Library supports Unity 2018.3 and newer so define can be just

if UNITY_2018_3_OR_NEWER

https://docs.unity3d.com/Manual/PlatformDependentCompilation.html

it prevents the socket from automatically disconnecting on focus lost, which your version was.

There was specific disconnect lost only for Apple IOS. UnitySocketFix originaly under defines for IOS only. So it doesn't work when you just use Unity not on IOS. No disconnections on any focus lost Original code covered under #if UNITY_IOS && !UNITY_EDITOR

RevenantX commented 1 year ago

@FirstGearGames original issue https://github.com/RevenantX/LiteNetLib/issues/312

FirstGearGames commented 1 year ago

I looked for the 'or newer' define but certain versions do not trigger it. The one you provided does so I shall use that, thank you.

I understand this fix was for IOS initially but I promise you Android faces the same issue, perhaps only on certain devices. I've a large number of reports about the disconnects happening on IOS and Android. There's no harm in providing this for both anyway. Not to mention this supports threading where the previous solution did not. I'll do another push with the define changes.

I'm happy with it's state in my project and I hope to see this make it into the main branch so I may continue to work on the latest version. Cheers!