Unity-Technologies / multiplayer-community-contributions

Community contributions to Unity Multiplayer Networking products and services.
MIT License
421 stars 161 forks source link

fix: WebSocketTransport Re-use and Compatibility #188

Closed nicholas-maltbie closed 1 year ago

nicholas-maltbie commented 1 year ago

Adjusted web socket transport so it can be re-used multiple times and not rely on static fields to manage state. This way a player can connect, disconnect, and reconnect without having to re-create the web socket transport.

Also included changes from PR #174 to change references in JSWebSocketClient.jslib to use Module['dynCall_vii'] instead of Runtime.dynCall('v', _, []); to get around the error: ReferenceError: Runtime is not defined

Found that the variable Runtime no longer exists in more recent versions of the WebGL build for unity. Would it be best to make two versions of these calls based on a flag like if the unity build is before/after v2021.2 to decide if that flag should be used? https://stackoverflow.com/questions/70411564/unity-webgl-throws-error-referenceerror-runtime-is-not-defined

It seams that in unity 2021.2 variable Runtime doesn't exist and can be replaced with Module['dynCall_*'].

In webSocket.jslib change all Runtime.dynCall('1', 2, [3, 4]) for Module['dynCall_1'](2, 3, 4)

Example instance.ws.onopen function in WebSocket.jslib:

change Runtime.dynCall('vi', webSocketState.onOpen, [ instanceId ]);
for
Module['dynCall_vi'](webSocketState.onOpen, instanceId);

Ran into an error with the NativeWebSocket.cs being marked as closed so avoided that error as it didn't seem to catch any edge cases when closing an already closed native session. Any feedback on this part is appreciated. It seems like the existing case of connecting one still works so it's not "more broken" than it was before but if there is a more graceful solution, I'm happy to adjust based on feedback. It seems like the close method of the socket was being called after the socket was already closed and I'm not sure if this is because the websocket transport was originally designed to be run once or if it's just some compatibility issues with netcode v1.1.0.

I have a working example using this websocket transport in my repo https://github.com/nicholas-maltbie/NetworkStateMachineUnity An example of the project running in the browser is hosted here via github pages: https://nickmaltbie.com/NetworkStateMachineUnity/

I specifically referenced my branch in the changes to ensure they work as expected:

"com.community.netcode.transport.websocket": "git+https://github.com/nicholas-maltbie/multiplayer-community-contributions?path=/Transports/com.community.netcode.transport.websocket#webgl-patch",

I was able to have multiple browser sessions connect to a host I execute locally. The project doesn't have much documentation but there will be buttons to start host/server mode when running in a non WebGL client. The WebGL client doesn't support server mode so it can only function as a client.

unity-cla-assistant commented 1 year ago

CLA assistant check
All committers have signed the CLA.

simon-lemay-unity commented 1 year ago

Found that the variable Runtime no longer exists in more recent versions of the WebGL build for unity. Would it be best to make two versions of these calls based on a flag like if the unity build is before/after v2021.2 to decide if that flag should be used?

If the change causes the transport to be broken for some supported LTS version of Unity (currently that would be 2020.3 and 2021.3), then yes it would be preferable to guard this with some flag based on the version.

Or put alternatively, does replacing Runtime with Module like the PR is doing also work on 2020.3? If so, then I'd say it's fine to leave it as it is.

nicholas-maltbie commented 1 year ago

Found that the variable Runtime no longer exists in more recent versions of the WebGL build for unity. Would it be best to make two versions of these calls based on a flag like if the unity build is before/after v2021.2 to decide if that flag should be used?

If the change causes the transport to be broken for some supported LTS version of Unity (currently that would be 2020.3 and 2021.3), then yes it would be preferable to guard this with some flag based on the version.

Or put alternatively, does replacing Runtime with Module like the PR is doing also work on 2020.3? If so, then I'd say it's fine to leave it as it is.

I'll setup a quick test to verify this behaviour. If it doesn't work as expected i'll add a flag to the build and variable that will select between the old or new method based on which version is used by the player :)

nicholas-maltbie commented 1 year ago

Hi @simon-lemay-unity Thanks for reviewing the PR earlier.

I created a test project to verify that it works with unity v2020.3 https://github.com/nicholas-maltbie/NetworkTest-Unity-WebSocket-Verification. I used version 2020.3.43f1 specifically.

The webgl build seemed to work fine without throwing any errors. Although to make sure it's not just me you may want to verify and checkout the project yourself. I have it importing the project using my fork's branch

git+https://github.com/nicholas-maltbie/multiplayer-community-contributions?path=/Transports/com.community.netcode.transport.websocket#webgl-patch

The changes to using the new syntax via dynCall_v works in the version 2020.3.x as well. I didn't upload a build like I did with the last one, but you can download the repo and verify locally if interested.

Module['dynCall_vi'](webSocketState.onOpen, instanceId);

if you have any other suggestions or comments regarding the change let me know.