search

futile / enet-rs

High-level bindings for the ENet networking library (http://enet.bespin.org)
Other
39 stars 21 forks source link

Huge update #6

Closed htrefil closed 4 years ago

htrefil commented 4 years ago

This is a huge, backwards incompatible update aimed at improving ergonomics of the library and performance.

The following changes have been made:

I hope this isn't too much. Please review these changes and let me know. :)

futile commented 4 years ago

Hey! Thanks for the changes, reading your description these all sound like useful API improvements that are in the spirit of the library! I haven't been using this library myself much lately, but reviewing this shouldn't be too hard, and your summary will certainly help. I'll try to get the review done over the next couple of days!

It seems like you merged in this repo's master branch into your fork, could you maybe instead rebase your new commits on top of this repo's master? I'd like to merge it that way because that avoids a merge commit in this repo and keeps the history linear.

Also, out of curiosity: Have you checked out amethyst's laminar crate? I'm just interested in what your use-case for working on this is, i.e., why you decided to use enet. Just if you want to share ;)

Thanks for your contributions! :)

htrefil commented 4 years ago

Hey, thanks for the feedback. I will try to rebase it. I'm not that good with git and it shows.

I have checked out the laminar crate and it looks interesting for sure. I personally do not use ENet anywhere (at least in my Rust projects), but I might find it useful later for interfacing with already existing applications that use ENet (namely a Cube 2: Sauerbraten chatbot is on my TODO list), and I believe someone else might find it useful for their projects when they need a simple networking solution. Lastly, I made the asnet library for TCP networking which uses a similar API to the one I "created" here, which seemed to work well, so I decided to contribute with my insight here.

BTW, I made a simple game with asnet (however the performance is not optimal due to the fact that it's built on TCP and clients send their position each 16ms), you can check it out here.

futile commented 4 years ago

Ok, I got around to reviewing your changes! First of all, thanks for your work again! :) There are some changes I definitely like, a few others that need a bit of iteration I think, and some that I (currently) don't like.

In order:

  • Peers are now borrowed directly from the host and are always behind references. This is implemented in a manner similar to how std::path::Path is implemented in the standard library. This is done in order to be able to implement std::ops::{Index, IndexMut} for the Host structure to allow access to the peers. The peers still can not be stored anywhere, but their indexes can, so that should improve usability. As long as Peer does not implement Copy or Clone, this is safe. We also intentionally don't expose the peers as a mutable slice because it would be possible to change the order of elements.

This is mostly nice, I think. Especially only handing out references because we never own them makes sense. I'll leave some more questions as comments in the code if I have some. One thing I don't like is implementing Index and IndexMut for Host. Semantically, what does it mean to "index" a host? I think it would make more sense to just stick with peer(idx) and peer_mut(idx) here, which have clear semantics. Do you have a certain use-case or something else in mind for these impls?

  • Packet::new now takes a Vec<u8> to reduce copying and allocation. This could have a huge performance impact on some applications. The userData field of ENetPacket is abused to store the capacity of the vector so that it's possible to free it from the freeCallback function once the packet is freed by ENet.

This is really nice! Avoiding a copy of the data in the enet internals definitely is desirable. Nice :) Since we don't really need capacity anymore at this point, what do you think about taking just a Box<[u8]> instead of a Vec<u8> here? Vec has a method Vec::into_boxed_slice so for users of the API this should be simple enough. This way we only need to reconstruct a Box<[u8]> and not a Vec<u8>, meaning we don't need to keep track of the capacity for dropping.

  • Event has been refactored as a struct and EventKind has been added.

I see this was to extract the common Peer field? In general, I like this, but I'm not 100% sure I like the current names. Do you have another suggestion here? Maybe PeerEvent might be a fitting name for one of them?

  • Peer data is now dropped on disconnect, peer reset and host drop (fixes #2)

Nice :) Why did you decide to drop data inside process_event over the existing Drop implementation? This feels a bit unnatural and unintuitive from a user's point of view to me.

  • Address::from_hostname takes a std::ffi::CStr rather than the owned counterpart

Nice, good catch :)

  • Host::service takes a std::time::Duration as the timeout parameter for consistency with the rest of the Rust ecosystem

Nice, good catch :)

  • Docs and examples are updated accordingly, the code compiles without warnings

Couldn't check yet, but nice :)

Thanks again for your PR! :) I'll be on vacation for the next week, so I'll probably not reply until the week after that, just to let you know!

P.S.: Also, this still needs a rebase so I can merge it (but probably only makes sense after everything else is done).

LeonMatthes commented 4 years ago

Hi, I'd just like to give this PR a bump. I'm currently using laminar for one of my projects, and am not really happy with it. It doesn't support package fragmentation, and is still missing a lot of features (latency detection, etc.). I would much rather use enet, and this PR seems to fix many of the remaining problems with it. Would love it to be integrated.

htrefil commented 4 years ago

@futile sorry for the delay, I had other things going on and kind of forgot about this. I will try to look into it in the near future and correct all problems.

LeonMatthes commented 4 years ago

In hopes of being able to use enet in rust in the near future, I've created PR #9 which further improves on these changes so they can hopefully get them merged.

htrefil commented 4 years ago

Closing this, please merge #9 instead.