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
Add NetworkingSockets, NetworkingUtils, GameServer #27
Conversation
Add socket module
I've fixed the documentation a bit and added poll group. I think I'm completely done. I might add more if I need it for my project or if anybody finds any bugs or issues. |
I've fixed some more documentation issues and cleaned up the code a bit. I also added the missing By now most of these methods are quite well tested on my side at least and the documentation is complete. |
Looking forward for approved reviews! |
I fixed an issue when trying to accept messages from an invalid poll group or connection handler (just return -1 instead of crashing). |
I ended up using all the auth ticket related code in a project of mine, so I fixed an issue with the implementation and added the missing methods on the server side. Works fine on my machine™. I would appreciate it, if somebody could take the time and motivation to look through this or alternatively promote me to maintainer. |
I'll have a look, but cannot tell when. Will take a while! |
Maybe this is bad etiquette, but I am going to shoot one of the maintainers an email about this. It would be a real shame if this wasn't merged. |
Hello, I'm the main maintainer here, sorry for the huge delay. Skimming this looks ok, but it's hard to review because it changed the steam API version and that breaks all CI (used for testing and uploading releases) and I need to fix it (which involves some hacky private repos). It's also possible Travis has changed in the meantime, as it often does, but I haven't checked that. I'm travelling currently and don't even have my usual setup, so I am not sure I can review this in the next couple weeks. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I skimmed over everything and it looks fine. I don't really understand a lot of these functions because I haven't used them, but they seem legit.
I'll be doing some very minor changes on top of this PR and then merging it.
The docs are mostly very good and consistent with the current stuff. The only things that are not very consistent are the usage of flags, in which we usually use strings and here it's using constants inside the API (which might be a better solution tbh), though sometimes it still uses strings in some return values. I didn't change this because I'm afraid of breaking the code and not being able to test it, and also because perhaps it does make sense to use integer constants in these places where speed is important, like sending messages.
Also, the commit history was a bit messed up, with merges, please try to avoid that in the future, a linear history (achieved with rebases) is clearer.
I tried to update the SDK to 1.57 but it has some breaking changes so I just did it to 1.55, but now it should be easier to do it for 1.57 (Travis integration was also broken and I fixed it).
//HAuthTicket GetAuthSessionTicket( void *pTicket, int cbMaxTicket, uint32 *pcbTicket ) | ||
EXTERN int luasteam_getAuthSessionTicket(lua_State *L) { | ||
uint32 pcbTicket = 0; | ||
void *pTicket = malloc(1024); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ideally we wouldn't manually allocate anything that needs freeing, and instead use something that automatically deallocates for us, to prevent future errors.
int64 *pOutMessageNumberOrResult = new int64[nMessages]; | ||
SteamNetworkingMessage_t **pMessages = new SteamNetworkingMessage_t *[nMessages]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IIUC from the API, steam takes ownership of these arrays and will free them, correct? It's a bit confusing when we see no free's anywhere. If so, it might be nice to at least add a comment explaining that.
I have merged this, and will soon do a release. @zorfmorf if you want to do further changes lmk, the review shouldn't take long now. Thanks for all of this!! 😎 |
@yancouto Thanks for looking through this, I appreciate it a lot and I know it took a lot of time! Sorry for the messy commit history, I don't know why I didn't rebase on my own actually. I'll be sure to create a cleaner history the next time. I am certain that the methods work as they are supposed to, however I am not a C++ expert, so I would not be surprised if I introduced a memory leak somewhere! I am travelling right now but I'll look into it again in the next few days and make sure that everything is alright to the best of my abilities. Yes, I decided not to introduce specific string return values in some situations where you would call the method very often. I am not sure if this actually helps performance wise, so maybe I should have just stayed consistent. |
Awesome! Thanks so much for implementing and reviewing! |
FYI the build is working on Travis but the release uploading is not, as |
If you want to use GitHub actions, feel free to look into NoitaMP workflows. Feel free to use it, but please add credits. Important part might be Fyi: I had to do it 'manually', because of modifications to dlls, but there a plenty marketplace actions to build stuff out of the box. If I can help, give me a shout. Preferably on discord. Edit/fyi2: Never used travis. |
Thank you for all the great work Yancouto! <3 |
I finally managed to fix CD and the latest release is working and has all the libraries uploaded! This is fully done. |
I might still add more to this butthe current state is tested quite extensively on linux and windows.Please let me know if you find any issues or want stuff done differently