Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: WebSocketTransport Re-use and Compatibility #188

Merged

Conversation

nicholas-maltbie
Copy link
Contributor

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
Copy link

unity-cla-assistant commented Nov 24, 2022

CLA assistant check
All committers have signed the CLA.

@simon-lemay-unity
Copy link
Contributor

simon-lemay-unity commented Dec 22, 2022

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.

Copy link
Contributor

@simon-lemay-unity simon-lemay-unity left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Aside from the Runtime/Module usage mentioned above, everything else looks good to me. Good work!

@nicholas-maltbie
Copy link
Contributor Author

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
Copy link
Contributor Author

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.

Copy link
Contributor

@simon-lemay-unity simon-lemay-unity left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for testing the compatibility with 2020.3! Merging this now!

@simon-lemay-unity simon-lemay-unity merged commit c7246c7 into Unity-Technologies:main Jan 4, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants