search

PhilippvK / playforia-minigolf

Client & Server for Minigolf Game known from Playforia/Playray/Appeli. Written in Java.
84 stars 31 forks source link

Design new network protocol to be used for communication between server and client #70

Open pehala opened 3 years ago

pehala commented 3 years ago

Currently, the server-client communication is based on text messages separated by tab (e.g starttrack \t trackname...), processed by one big if statement across multiple methods. It is a mess and sadly totally unmaintanable since we cannot read most of it and do not understand how it works at all.

I would like to start designing a new proper protocol which would include all of the messages that are currently processed and that would allow extending it to for example enable user roles or even check version.

To do that I think we should have a separate branch because this is an effort that will take a long time a lot of work. WDYT @PhilippvK ?

PhilippvK commented 3 years ago

@pehala Great ideas! But definitely a huge workload which would be better done on a new branch. Feel free to work directly on this repository instead of your fork if its easier for you, I can give you write permissions if not already granted.

PhilippvK commented 3 years ago

I am not sure if it's really suited here but the server-client communications needs some sort of protection against conflics/race conditions when multiple messages come in at the same time. (f.e. synchronized blocks or something at a higher abstraction level)

pehala commented 3 years ago

No need, I would still like someone to review the code, to make sure it is correct. I would also like to discuss this with you in more detail to make sure the design is done right and if that would be possible I would like to ask you to help me with the mapping of existing "endpoints" from client because the code is totally a mess.

pehala commented 3 years ago

I am not sure if it's really suited here but the server-client communications needs some sort of protection against conflics/race conditions when multiple messages come in at the same time. (f.e. synchronized blocks or something at a higher abstraction level)

I definitely agree, that is taken care of netty (if we would use netty correctly) so it shouldn't be that hard. Also, we should also write networking tests to have at least some confidence in the new networking.

EDIT: Oh and we have to use special collections (CopyOnWriteArrayList etc) to make sure that it works with parallel access.

PhilippvK commented 3 years ago

No need, I would still like someone to review the code, to make sure it is correct. I would also like to discuss this with you in more detail to make sure the design is done right and if that would be possible I would like to ask you to help me with the mapping of existing "endpoints" from client because the code is totally a mess.

Fine! I do not really see myself writing any "new" high level Java code as my Java skills are way behind yours and I am tbh not really into Java. Reverse Engineering the current protocol stack and help design a new concept would be a more favorable tasks If I am able to invest time in it.

capsload2 commented 3 years ago

Is there any chance we can add more than 4 players?

PhilippvK commented 3 years ago

@capsload2 See #71!

PhilippvK commented 3 years ago

@pehala I digged a bit through the codebase and got a bit more familiar with some endpoints and how the connection works.

I have a few open questions to you:

pehala commented 3 years ago
pehala commented 3 years ago

Maybe adding NETWORK.md with all endpoints could be a good idea, we will document it in the process which is always a good idea

PhilippvK commented 3 years ago

@pehala Have you already found this document?

https://github.com/PhilippvK/playforia-minigolf/blob/master/doc/servertoclient.txt

Its not as helpful as I wished but it contains at least a subset of endpoints...

pehala commented 3 years ago

I did not, thanks!

PhilippvK commented 3 years ago

Do you think de-obfuscating the variable- and function-names in the client-side connection code would be a good initial/intermediate step or would it become obsolete when rewriting the protocol anyway?

We could f.e. migrate to netty 4 with the current protocol before we replace it with a new one.

I also found out that the protocol already supports server-client version verification in some way, but it is not really used at the moment :D

pehala commented 3 years ago

Do you think de-obfuscating the variable- and function-names in the client-side connection code would be a good initial/intermediate step or would it become obsolete when rewriting the protocol anyway?

Yes definitely

We could f.e. migrate to netty 4 with the current protocol before we replace it with a new one.

Probably not, it would require time to replace deprecated components which I think is not very useful since we want to scrap it all together.

I also found out that the protocol already supports server-client version verification in some way, but it is not really used at the moment :D

Lol, nice :D I would probably not enable it or use it right now, I think we can wait with this for the new one. I would rather have polished feature later than PoC early.

pehala commented 3 years ago

After spontaneous reserach I think we can base our protocol on top of WebSockets. Since it is using HTTP connection, we would be able to hide our server behind HTTP proxy, which is currently not possible (I have tried).

PhilippvK commented 3 years ago

Wow, I actually hd the same thoughs just a few days ago! I tried Porting the Client Java Applet to HTML5+JS via the CheerpJ Compiler. The problem is that Browsers can only communicate via HTTP/HTTPS/WebSockets which does not work at all with the Netty TCP Sockets. Unfortunately CheerpJ does only support native Java8 Components which means that java.net.http.WebSocket won't work, yet. As this means that the only remaining option right now to get CheerpJ working, would be replacing the Protocol with a HTTP REST API which sound terrible for a game engine...

Anyway, WebSockets would solve this issue if Java11 is supported by the tool in the future. Great Idea! Would you like to stick with Netty for realizing this approach?

pehala commented 3 years ago

Yes I would like to stick with Netty. Quick research showed that they are already some implementation of websockets with netty, even tho I didn't test them yet. I plan to test this with a prototype once I have some free time left (=most likely my exam period over)

pehala commented 3 years ago

Quick note: it might be possible to port existing TCP netty application to WebSockets by changing handler, which takes care of the actual protocol mapping.

PhilippvK commented 3 years ago

Good points!

But as far as I know only the server-side of the communication uses Netty at the moment. Client uses plain sockets. I assume that we should possibly port the client to use netty as well first.

PhilippvK commented 3 years ago

Similar situation for me. I hope that I manage to invest some time in the project in March because exams are more important at the moment.

pehala commented 3 years ago

Good points!

But as far as I know only the server-side of the communication uses Netty at the moment. Client uses plain sockets. I assume that we should possibly port the client to use netty as well first.

Yep that will definitely be the first pain point

PhilippvK commented 3 years ago

It might be not really related to the Issue but I managed to get the Client Applet running using CheerpJ to compile from JAR to HTML5+JS. As mentioned before I had to write a custom Adapter to java.net.Socket in JS to use Websockets on Port 80. The Server can be used without modifications using websockify 80 :4242. It should work with every desktop browser without any Addons and could be hosted f.e. on GitHub pages but I am not sure if I am going to deploy it because the code is quite a mess right now.

Screenshot 2021-01-18 at 05 50 13
pehala commented 3 years ago

Nice! I would like to still like to properly rewrite client networking since we have the option and time, but this is awesome for all other minigames.

pehala commented 3 years ago

I finally started with rewrite on top of latest Netty (4.1). Suprisingly, the pure socket client is not that hardly connected to the rest of the applet so I was able to replace it with my own.

I choose to design the communication from scratch because it seemed a lot of network messages currently in use.

I have currently a small PoC which so far is integrated as far as lobby select. Due to the nature of the rewrite (absolutely everything network related :D) it will be a very big PR, @PhilippvK would you like me to create draft PR which will be continuously updated so we can discuss changes early or do you want me to submit once I finish everything?

pehala commented 3 years ago

Also regarding the websockets, since we are using Netty, it is very easy to switch underlying protocol to pure websockets if we want to also embed it into HTML

PhilippvK commented 3 years ago

Due to the nature of the rewrite (absolutely everything network related :D) it will be a very big PR, @PhilippvK would you like me to create draft PR which will be continuously updated so we can discuss changes early or do you want me to submit once I finish everything?

BBoth is fine to me. But I would prefer if I can either check the current state out via the PR or by just using your fork of the repository.