Skip to content
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

Matchbox Bevy Plugin #144

Closed
simbleau opened this issue Mar 12, 2023 · 15 comments · Fixed by #157
Closed

Matchbox Bevy Plugin #144

simbleau opened this issue Mar 12, 2023 · 15 comments · Fixed by #157
Labels
enhancement New feature or request help wanted Extra attention is needed

Comments

@simbleau
Copy link
Collaborator

Most people would agree the networking library support in Bevy is lacking. I'm really proud of what we've accomplished with Matchbox these past few weeks. I'd like to see matchbox become the go-to networking library for Bevy, for both P2P (likely) and Client/Server (less likely).

If we want to see that happen, the friction with Bevy (i.e. async) should probably be abstracted away from the user.

The approach I have is this simple:

  • Prepare a plugin for Bevy which translates socket updates into Bevy events
  • Provide a compatibility matrix on the README for versions, e.g.
bevy matchbox_bevy matchbox_socket
< 0.9 Not supported N/A
0.10 0.1 0.6

I'd also like to see perhaps over time, we prepare the signalling server as a Bevy plugin, and clean up the confusing path to client-server architecture. It would be wicked if it was as easy as adding the signalling server as a plugin to their game server (which also ran on Bevy), to bundle them together.

@simbleau simbleau added enhancement New feature or request help wanted Extra attention is needed labels Mar 12, 2023
@simbleau
Copy link
Collaborator Author

Some snippets on translating socket events into Bevy events is in #136

@garryod
Copy link
Collaborator

garryod commented Mar 12, 2023

This is almost exactly what I've been working on today 😆 And is certainly the approach we need to take if we want to make the project more widely accessible.

One thing that I've come to realise is we'll likely need to allow multiple channels in a bevy app - e.g. a reliable channel for Events and an unreliable one we can pass off to ggrs. This first requires that we implement ggrs::NonBlockingSocket for a specific channel on a WebRtcSocket

@simbleau
Copy link
Collaborator Author

Awesome! Happy to hear you've began on this. I don't know a lot about the ggrs side of things; I don't use it in my projects.

Bear in mind the user will want to have control over the WebRtcConfig, too. For example, if I want an unreliable channel for movement packets and a reliable channel for chat packets in my game.

Maybe the best approach is to initialize the plugin with one's own WebRtcSocketConfig, and then add 2 channels (and store their indexes) on top of what they specify.

@garryod
Copy link
Collaborator

garryod commented Mar 12, 2023

I don't either, only going by the example 😅

It might be nice to adopt the builder pattern with WebRtcSocketConfig, allowing the user - or another integration - to add various channels before socket creation.

@simbleau
Copy link
Collaborator Author

I don't either, only going by the example 😅

It might be nice to adopt the builder pattern with WebRtcSocketConfig, allowing the user - or another integration - to add various channels before socket creation.

I like that idea.

@johanhelsing
Copy link
Owner

One thing that I've come to realise is we'll likely need to allow multiple channels in a bevy app - e.g. a reliable channel for Events and an unreliable one we can pass off to ggrs. This first requires that we implement ggrs::NonBlockingSocket for a specific channel on a WebRtcSocket

I have a nearly done devlog that explains the solution I'm using. Here's the draft: https://johanhelsing.studio/posts/cargo-space-devlog-6

@johanhelsing
Copy link
Owner

Just picking up the thread from another thread :)

Perhaps the solution is to provide a bevy Plugin that creates the "socket" resource for you allowing us to hide the ugly async bits

Yes, something like this feels like the right thing to do, but I find it hard to decide until we've actually tried doing it...

Honestly I have this pretty much written already for my own game client. I could submit a PR with my abstraction layer. I put a snippet in #136 for viewing. Basically all I do is make the socket a resource and then have the plugin listen for Connect/Disconnect events. On connect, it attempts to create the socket, and if successful will start firing all the socket updates as events.

That basically means as a user you're responsible for 2 things:

* Sending the event when you want to connect/disconnect

* Listening for socket events in Bevy

I mean it's kind of unclear where to do the split. One option would be to create a nice, fully async API, not caring at all to be friendly for bevy. On top of that, we could either write a bevy-specific wrapper, or one that's just a non-async interface, very similar to the one we have today. We would just expose both interfaces in different modules.

Or it could be a callback-based API beneath instead of an async one, and provide two layers on top, one async (for fully async applications) and one for apps like bevy and ggrs. Personally, I have no use for the former though.

In any case, an ergonomic bevy plugin would probably make sense either way, but I'm still a bit undecided what the public and internal interfaces for matchbox_socket should be. It would be nice if it was something that makes the bevy wrapper crate be relatively clean and thin.

@simbleau
Copy link
Collaborator Author

simbleau commented Mar 14, 2023

Here's how I modelled a client plugin, if that helps, which I think is sufficiently "clean and thin."

Note this was done before Bevy stageless in 0.10...

use std::net::IpAddr;
use bevy::{prelude::*, tasks::IoTaskPool};
use events::SilkSocketEvent;
use matchbox_socket::{PeerId, WebRtcSocket};
use silk_common::{SilkSocket, SilkSocketConfig};

/// The socket client abstraction
pub struct SilkClientPlugin;

/// State of the socket
#[derive(Debug, Clone, Eq, PartialEq, Hash)]
enum ConnectionState {
    Disconnected,
    Connecting,
    Connected,
}

/// Socket events that are possible to subscribe to in Bevy
pub enum SilkSocketEvent {
    /// The signalling server assigned the socket a unique ID
    IdAssigned(PeerId),
    /// The socket has successfully connected to a host
    ConnectedToHost(PeerId),
    /// The socket disconnected from the host
    DisconnectedFromHost,
    /// A message was received from the host
    Message((PeerId, Box<[u8]>)),
}

impl Plugin for SilkClientPlugin {
    fn build(&self, app: &mut App) {
        app.insert_resource(SocketResource::default())
            .add_state(ConnectionState::Disconnected)
            .add_event::<ConnectionRequest>()
            .add_system(event_reader)
            .add_event::<SilkSocketEvent>()
            .add_system(event_writer)
            .add_system_set(
                SystemSet::on_enter(ConnectionState::Connecting)
                    .with_system(init_socket),
            )
            .add_system_set(
                SystemSet::on_enter(ConnectionState::Disconnected)
                    .with_system(reset_socket),
            );
    }
}

#[derive(Resource, Default)]
struct SocketResource {
    /// The ID given by the signalling server
    pub id: Option<PeerId>,
    /// The socket configuration, used for connecting/reconnecting
    pub silk_config: Option<SilkSocketConfig>,
    /// The underlying matchbox socket
    pub mb_socket: Option<WebRtcSocket>,
}

pub enum ConnectionRequest {
    /// A request to connect to the server through the signalling server; the
    /// ip and port are the signalling server
    Connect { ip: IpAddr, port: u16 },
    /// A request to disconnect from the signalling server; this will also
    /// disconnect from the server
    Disconnect,
}

/// Initialize the socket
fn init_socket(mut socket_res: ResMut<SocketResource>) {
    if let Some(silk_socket_config) = &socket_res.silk_config {
        debug!("silk config: {silk_socket_config:?}");

        // Crease silk socket
        let silk_socket = SilkSocket::new(silk_socket_config.clone());
        // Translate to matchbox parts
        let (socket, loop_fut) = silk_socket.into_parts();

        // The loop_fut runs the socket, and is async, so we use Bevy's polling.
        let task_pool = IoTaskPool::get();
        task_pool.spawn(loop_fut).detach();

        socket_res.mb_socket.replace(socket);
    } else {
        panic!("state set to connecting without config");
    }
}

/// Reset the internal socket
fn reset_socket(mut socket_res: ResMut<SocketResource>) {
    *socket_res = SocketResource::default();
}

/// Reads and handles connection request events
fn event_reader(
    mut cxn_event_reader: EventReader<ConnectionRequest>,
    mut socket_res: ResMut<SocketResource>,
    mut connection_state: ResMut<State<ConnectionState>>,
    mut silk_event_wtr: EventWriter<SilkSocketEvent>,
) {
    match cxn_event_reader.iter().next() {
        Some(ConnectionRequest::Connect { ip, port }) => {
            if let ConnectionState::Disconnected = connection_state.current() {
                let silk_socket_config =
                    SilkSocketConfig::RemoteSignallerClient {
                        ip: *ip,
                        port: *port,
                    };

                debug!(
                    previous = format!("{connection_state:?}"),
                    "set state: connecting"
                );
                socket_res.silk_config = Some(silk_socket_config);
                _ = connection_state.overwrite_set(ConnectionState::Connecting);
            }
        }
        Some(ConnectionRequest::Disconnect) => {
            if let ConnectionState::Connected = connection_state.current() {
                debug!(
                    previous = format!("{connection_state:?}"),
                    "set state: disconnected"
                );
                socket_res.mb_socket.take();
                silk_event_wtr.send(SilkSocketEvent::DisconnectedFromHost);
                _ = connection_state
                    .overwrite_set(ConnectionState::Disconnected);
            }
        }
        None => {}
    }
}

/// Translates socket updates into bevy events
fn event_writer(
    mut socket_res: ResMut<SocketResource>,
    mut event_wtr: EventWriter<SilkSocketEvent>,
    mut connection_state: ResMut<State<ConnectionState>>,
) {
    let socket_res = socket_res.as_mut();
    if let Some(ref mut socket) = socket_res.mb_socket {
        // Create socket events for Silk

        // Id changed events
        if let Some(id) = socket.id() {
            if socket_res.id.is_none() {
                socket_res.id.replace(id.clone());
                event_wtr.send(SilkSocketEvent::IdAssigned(id.to_string()));
            }
        }

        // Connection state updates
        for (id, state) in socket.update_peers() {
            match state {
                matchbox_socket::PeerState::Connected => {
                    _ = connection_state
                        .overwrite_set(ConnectionState::Connected);
                    event_wtr.send(SilkSocketEvent::ConnectedToHost(id));
                }
                matchbox_socket::PeerState::Disconnected => {
                    _ = connection_state
                        .overwrite_set(ConnectionState::Disconnected);
                    event_wtr.send(SilkSocketEvent::DisconnectedFromHost);
                }
            }
        }

        // Collect Unreliable, Reliable messages
        event_wtr.send_batch(
            socket
                .receive_on_channel(SilkSocketConfig::UNRELIABLE_CHANNEL_INDEX)
                .into_iter()
                .chain(socket.receive_on_channel(
                    SilkSocketConfig::RELIABLE_CHANNEL_INDEX,
                ))
                .map(SilkSocketEvent::Message),
        );
    }
}

This makes a few assumptions about the signalling server (since I adapted mine to be a server) but I think the general idea is the same.

@johanhelsing
Copy link
Owner

johanhelsing commented Mar 15, 2023

Yes, it will be thin as the API is now, but if we create a wrapper, we're free to do things like #136

I'm just worried that we'll be left with a really weird API for matchbox_socket itself, that's a mix of non-blocking polling and async. It will be neither be convenient as an async API nor a non-blocking sync API... For instance the simple async example currently require some ugly timeout hacks in order to regularly poll the socket.

The question is what kind of API matchbox_socket itself should have, to make both of these use-cases require a minimum of ugly hacks.

@johanhelsing
Copy link
Owner

That said, it might make sense to make a wrapper for Bevy right away anyway, but I'm wondering whether it would make sense to keep the raw example as well. I kind of feel like we need more direct use-cases of matchbox_socket itself, to make sure its API is good (so we don't forget it just because we hide ugly bits in the bevy wrapper).

Perhaps adding a non-bevy ggrs example (as suggested in #148) might actually make sense.

@garryod
Copy link
Collaborator

garryod commented Mar 15, 2023

Yeah, I would agree, we should do what we can to make the base matchbox_socket API as clean as possible. A non-bevy ggrs example might be a nice way of holding us to this.

As far as the matchbox_socket API goes, I think async is probably the play as it maps most nicely to our dependencies, even if this will require a few hacks to be brought back to a sync context

@simbleau
Copy link
Collaborator Author

Async is the best way to go for the socket, I agree.

And I can also add a terminal fullmesh chat app using only bevy (no ggrs) as well.

@garryod
Copy link
Collaborator

garryod commented Mar 15, 2023

Been playing around with this a fair bit today and have a few questions that could do with answering before I can move further.

  1. How do we want to pass our WebRtcSocketConfig? Options are; as a Plugin argument, as a startup_system argument, via a Resource, or via a bevy Event. My thinking is that it will be one of the two.
  2. Do we want to be able to retrieve channels from the WebRtcSocket? I noticed @johanhelsing used a bit of a hack to allow GGRS to own the socket whilst retaining access, would it be best to have WebRtcSocket be composed of a Vec of Channels and implement the ggrs trait for a channel?

@johanhelsing
Copy link
Owner

  1. Another option here is via commands
  2. I want to look into whether we could make WebRtcSocket implement Clone. But a way to split/decompose would also work

@garryod
Copy link
Collaborator

garryod commented Mar 15, 2023

  1. Another option here is via commands

Oooh, that would be really nice actually, hadn't considered that. Will give it a go and see how it looks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request help wanted Extra attention is needed
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants