feat(packages/network): client and server for tcp/udp and networked pong as example #156
Conversation
# Conflicts: # packages/core/src/core/core.ts
github-actions
bot
added
packages:common
MartinFillon
left a comment
MartinFillon
left a comment
There was a problem hiding this comment.
A lot of things to clarify, especially on the .idea folder side
There was a problem hiding this comment.
To allow users that come on the project to have a correct idea configuration (for prettier and eslint for example)
There was a problem hiding this comment.
Please explain what it does, so its already done and we dont have to comeback
| @Expose() | ||
| @IsOptional() | ||
| @IsPort() | ||
| serverTcpPort?: string; |
There was a problem hiding this comment.
Why a string when its direclty converted later ? Why not directly a number ?
There was a problem hiding this comment.
IsPort is usefull but it checks only string port
If it's doesn't complain we can create another IsPort validator that checks int but it wasn't the goal
| @Expose() | ||
| @IsOptional() | ||
| @IsPort() | ||
| serverUdpPort?: string; |
There was a problem hiding this comment.
We gonna need some documentation for this, udp, and the utils file because otherwise if someone doesnt know how it works he is fucked. A markdown would be the best.
There was a problem hiding this comment.
Actually comment functions can be enough, we're not going to make a md for every file in the project (maybe some inline comments if it's very difficults)
There was a problem hiding this comment.
We still need a global design documentation on why we made such choice and how it works, not the specific functions but the architecture
| listeningUdpPort?: string; | ||
|
|
||
| @Expose() | ||
| @IsOptional() | ||
| @IsPort() | ||
| listeningTcpPort?: string; | ||
|
|
There was a problem hiding this comment.
Same as above if not used as string and directly converted use numbers
| throw new Error("No listenning address provided"); | ||
| } | ||
| if (config.listeningUdpPort === undefined && config.listeningTcpPort === undefined) { | ||
| throw new Error("No listenning port specified"); |
| } | ||
| } | ||
|
|
||
| public async __run(): Promise<void> {} |
There was a problem hiding this comment.
Why is run doing nothing ? Is it not suppose to listen and act ?
| emscripten::val registry = ctx["libs"].call<emscripten::val>("getComponentSystem")["registry"]; | ||
| std::vector<emscripten::val> systems_copy = _systems; | ||
| for (emscripten::val &system : systems_copy) |
There was a problem hiding this comment.
My shared_this solution didn't work ?
There was a problem hiding this comment.
Why changing the tests like that ? Your code should not be modifying those ?
There was a problem hiding this comment.
I did this to feat with the new working way of the Zipper.
I think the new working way is better than the older one as we don't have undefined values in the results.
I don't know very well ECS, it can be rollback if it causes perf issues but you need to see this with @Tchips46
There was a problem hiding this comment.
This is still out of scope of this PR, and not your pr. You can't just hack a change on a pr so it goes under the radar
Exeloo
force-pushed
the
feat/network/tcp
branch
from
a7c03f1 to
53f3895
Compare
PR Checklist
Please check if your PR fulfills the following requirements:
PR Type
What kind of change does this PR introduce?
Resolves #31
Resolves #44
Resolves #32
Resolves #63
Resolves #64
Resolves #65
Resolves #66
Does this PR introduce a breaking change?