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

Don't require &mut self for WebRtcSocket::id, players #140

Merged
merged 1 commit into from
Mar 12, 2023

Conversation

johanhelsing
Copy link
Owner

in #111, id and players started taking &mut self and returning Option<PeerId> instead of &PeerId.

This made the socket a bit more awkward to use. This fixes the &mut self part of it, though you'd still have to deal with the case when socket.id() returns None.

Alternative to #136

in #111, id and players started taking `&mut self` and returning
`Option<PeerId>` instead of &PeerId.

This made the socket a bit more awkward to use. This fixes the &mut self
part of it, though you'd still have to deal with the case when
`socket.id()` returns `None`.
@johanhelsing johanhelsing added the enhancement New feature or request label Mar 12, 2023
@garryod garryod mentioned this pull request Mar 12, 2023
Copy link
Collaborator

@garryod garryod left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a nice improvement to the ergonomics. Whilst it would be nice to be able to guarantee an Id in the future this seems like a far better interim solution than the one I had

@johanhelsing
Copy link
Owner Author

Okay, merging this. I'm not necessarily opposed to #136 , but I'd like some solution that makes it less awkward to use with bevy, but not exactly sure what that would be...

@johanhelsing johanhelsing merged commit e8779ac into main Mar 12, 2023
@johanhelsing johanhelsing deleted the id-non-mut-2 branch March 12, 2023 12:34
@garryod
Copy link
Collaborator

garryod commented Mar 12, 2023

Okay, merging this. I'm not necessarily opposed to #136 , but I'd like some solution that makes it less awkward to use with bevy, but not exactly sure what that would be...

A few thoughts:

  • Ultimately there's always going to be the sync/async boundary to contend with when it comes to bevy.
  • Exposing async higher up is almost always nice when the user is working in an async context.

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

@johanhelsing
Copy link
Owner Author

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...

@simbleau
Copy link
Collaborator

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

@johanhelsing
Copy link
Owner Author

Discussion continued in #144

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

Successfully merging this pull request may close these issues.

3 participants