-
Notifications
You must be signed in to change notification settings - Fork 72
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
Provide abstraction for a channel #160
Conversation
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.
If you were able to spend another 30 minutes trying to make this as idiomatic as the previous API, and add more documentation, what would the result look like? I'd like to see.
Overall I found this a little jarring of a change, because there's a ton of unwraps/borrows/etc. that aren't obvious to me on first glance, even on our simple examples. There's definitely some opportunity for documentation on some things, too.
Oops, this was supposed to be a draft. Just wanted to get something on the table before I headed out for training. Definitely needs some tidying up around the edges. Think I might return some error enum that has a couple variants like |
2d583d1
to
81b9b02
Compare
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.
Small nitpicks
Can you also explain to me what take_channel is used for? Why would someone take the channel out of the socket, and what are the effects if you do? I was surprised to see the channels is a Vec<Option> instead of just Vec. Is the method necessary, could we do without Option there? |
It's because ggrs needs to take ownership of the socket, and if you still want to be able to send/receive on the other channels outside ggrs, you need some way of turning a socket into separate parts. In this PR, it's done by removing the channels. I think they're options just so the errors can be nice (so you know there was a channel there with that id, but it's been taken). Other options for this solution would be:
Not having broadcast on the channels is probably fine for now. I'm sure we can find some other way of implementing it. Could for instance make the unbounded channel in enum Receiver {
Single(PeerId),
Broadcast, // or All?
} (and this would just be internal API). Another note. I'm wondering if it would make sense to add Bevy-like labels for channels. i.e.: #[derive(ChannelLabel)]
struct MyReliableChannel;
let (sock, fut) = WebRtcSocket::builder(url).add_reliable_channel(MyReliableChannel).build();
socket.channel(MyReliableChannel).send(packet, peer); |
I can't really confirm or deny whether I'd like the (SocketWithoutChannels, Vec) approach until I see how drastic the API change is. @johanhelsing +1 For labels, they could help us with this problem as well. For example, store channels as a You could remove the channel that way, and .send() can return several errors, like To me, that is better than |
One more suggestion, if the prior isn't favorable: Model the sending as a builder, e.g. |
Not sure I see how labels would solve this, we need to facilitate external code taking ownership of channels (i.e. the |
|
This is an interesting idea, will give it a go. Not sure how idiomatic it will feel though |
One option we have is to add back the convenience functions to sending and receiving on the pub fn recieve_on_channel(
&mut self,
channel: usize,
) -> Result<Vec<(PeerId, Packet)>, ChannelError> {
Ok(self.channel(channel)?.receive())
}
pub fn send_on_channel(
&mut self,
packet: Packet,
peer: PeerId,
channel: usize,
) -> Result<(), ChannelError> {
Ok(self.channel(channel)?.send(packet, peer))
} But I think we'd be better of sticking to having one way of doing this to avoid confusion |
I believe Hashmap::remove() returns the item removed, giving you ownership back. You can take it out of the hashmap. |
|
So it's not possible to combine them, e.g. |
It's possible to combine them regardless of whether we use a |
Having to always do the double unwrap on send/receive does not make for great ergonomics. At least not for simple cases, we're you're just interested in one channel, and don't need to decompose or hand off anything. As I see it, we have three options:
My third idea, which is still kind of on the draft stage, not sure if I like it or not, but thought I'd air it: Try to force it to be infallible by being generic over an iterable enum: #[derive(EnumIter, Debug)]
enum MyChannel {
Reliable,
Ggrs,
Custom,
}
impl ChannelConfig for MyChannel {
fn config(self) -> ChannelConfig {
match self {
Reliable => ChannelConfig::reliable(),
Ggrs => ChannelConfig::ggrs(),
Custom => ChannelConfig {
// ...
}
}
}
}
// type annotation added for clarity
let socket: WebRtcSocket<MyChannel> = WebRtcSocket::builder::<MyChannel>(url).build();
// or simply
let socket = WebRtcSocket::new::<MyChannel>(url);
socket.send(MyChannel::Reliable, data, peer); // infallible, can not panic
let (socket_control, data_channels) = socket.split();
start_ggrs_session(data_channels.remove(MyChannel::Ggrs).unwrap());
let my_game_socket = MyGameSocket {
socket_control,
data_channel: data_channels.remove(MyChannel::Reliable).unwrap()),
}
assert!(data_channels.empty()); Note, no This would make run-time channel config decisions more clunky to do, but it would also remove quite a few footguns. |
I'll be honest, I don't think this makes the right tradeoffs. Particularly because I think it's high boilerplate/glue code. #[derive(EnumIter, Debug)]
enum MyChannel {
Reliable,
Ggrs,
Custom,
}
impl ChannelConfig for MyChannel {
fn config(self) -> ChannelConfig {
match self {
Reliable => ChannelConfig::reliable(),
Ggrs => ChannelConfig::ggrs(),
Custom => ChannelConfig {
// ...
}
}
}
} I have a proposal which I think strikes the right balances: Keep the builder approach, with labels: Store the channels in a hashmap and mirror Bevy's API (per @johanhelsing) and if there's no reason ever that we would take a channel other than for ggrs, let's just make that a feature method like |
Guess we could also do a mix of the two: #[derive(EnumIter, Debug)]
enum MyChannel {
Chat,
Ggrs,
Custom,
Unspecified // <-- will be configured with ChannelConfig::default()
}
let socket = WebRtcSocket::builder::<MyChannel>(url)
.reliable_channel(MyChannel::Chat)
.ggrs_channel(MyChannel::Ggrs)
.custom_channel(MyChannel::Custom, ChannelConfig { /* ... */ })
.build(); We trade the complexity of an added generic for knowing that We could always start with the bevy-like |
I'm not sure this can ever be sufficient as I think I've got another solution which might be nice, but is to do with how we integrate with bevy. If we were to derive |
If we only have |
Surely we would then need to decompose this vector in some guaranteed way? Perhaps I'm missing something obvious here but I'm not sure I see how it would work |
if we iterate over the enum on init, e.g.: let channel_configs = MyChannel::iter()
.map(|c| explicit_configs.get(c).unwrap_or_default())
.collect();
// create socket And we have no way to remove channels from hashmap, then self.channels.get(channel).unwrap() cannot panic. I'm thinking Still I'm not sure I like my idea. Let's start with yours, it's always possible to revisit (and probably easier to explain if I just do it in a PR). And we need the channel abstraction in any case. |
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.
Some final nits/spelling stuff. If you're pressed on time, don't feel like you have to address them all. This looking really great :)
Love how much simpler the bevy_ggrs example got
Co-authored-by: Johan Klokkhammer Helsing <johanhelsing@gmail.com>
Co-authored-by: Johan Klokkhammer Helsing <johanhelsing@gmail.com>
Co-authored-by: Johan Klokkhammer Helsing <johanhelsing@gmail.com>
Co-authored-by: Johan Klokkhammer Helsing <johanhelsing@gmail.com>
Co-authored-by: Johan Klokkhammer Helsing <johanhelsing@gmail.com>
Co-authored-by: Johan Klokkhammer Helsing <johanhelsing@gmail.com>
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'm really happy with this, and have no more comments. @simbleau do you want to have a look before I merge?
I think most of my points are addressed regarding API complexity. :) Happy to see a good resolution. Do note: Please squash commits lol |
Can squash them down to something reasonable or you can "squash and merge". That being said, I am usually a strong advocate for only using rebase merges as it results in a much cleaner history |
Oops I already merged 👼 . I like linear history if every commit is readable and bisectable, but sometimes hard to achieve on small/midsize open source projects. |
Again, with a view towards making multi-channel sockets easier to use, I've abstracted channels by implementing a
WebRtcChannel
struct which providessend
&receive
. This should make the code from @johanhelsing's last devblog a lot cleaner.Unfortunately I've had to drop
broadcast
as it would require channels to know about their peers, it wasn't in use anywhere as far as I can tell, though maybe we should still figure out a way of re-adding it