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

Replace std sockets with laminar sockets #1137

Merged
merged 1 commit into from Nov 14, 2018

Conversation

Projects
None yet
4 participants
@TimonPost
Member

TimonPost commented Nov 13, 2018

I've implemented laminar for amethyst_network.

Solves: #1136

@LucioFranco

LucioFranco requested changes Nov 13, 2018 edited

Looking good so far. Have you run the examples at all?

Show resolved Hide resolved amethyst_network/src/network_socket.rs Outdated
Show resolved Hide resolved amethyst_network/src/lib.rs
Show resolved Hide resolved amethyst_network/src/network_socket.rs Outdated
Show resolved Hide resolved amethyst_network/Cargo.toml Outdated

@LucioFranco LucioFranco changed the title from Implemented laminar to Replace std sockets with laminar sockets Nov 13, 2018

@LucioFranco

looking a lot better 👍

Show resolved Hide resolved amethyst_network/src/network_socket.rs
Show resolved Hide resolved amethyst_network/src/network_socket.rs Outdated
Show resolved Hide resolved amethyst_network/src/network_socket.rs Outdated

@LucioFranco LucioFranco requested review from jojolepro and fhaynes Nov 13, 2018

@jojolepro

👍

@@ -58,4 +61,4 @@ where
T: DeserializeOwned,
{
deserialize::<NetEvent<T>>(data)
}
}

This comment has been minimized.

@jojolepro

jojolepro Nov 14, 2018

Member

Missing newline at the end of the file

This comment has been minimized.

@TimonPost

TimonPost Nov 14, 2018

Member

Not sure what you mean here.... Like deserialize stops at 63 and the brace is at 64. That's normal right?

Show resolved Hide resolved amethyst_network/src/network_socket.rs

@TimonPost TimonPost referenced this pull request Nov 14, 2018

Closed

Implement laminar #1136

@LucioFranco

This comment has been minimized.

Member

LucioFranco commented Nov 14, 2018

@TimonPost I think we're overall happy with this, can you rustfmt your code and make CI happy?

@LucioFranco

Looks great! 👍

bors r=jojolepro,LucioFranco

use serde::{de::DeserializeOwned, Serialize};
/// Sends an event to the target NetConnection using the provided network Socket.
/// The socket has to be bound.
pub fn send_event<T>(event: &NetEvent<T>, target: &SocketAddr, socket: &UdpSocket)
pub fn send_event<T>(event: &NetEvent<T>, addr: &SocketAddr, socket: &mut UdpSocket)

This comment has been minimized.

@LucioFranco

LucioFranco Nov 14, 2018

Member

Just a thought but this would be a good thing to turn into a trait.

trait Transport {
  type Event;
  type Error;

  fn send_event(&mut self, addr: &SocketAddr, event: &Event);

  fn receive_event(&self) -> Result<Event, Error>;
}
@LucioFranco

This comment has been minimized.

Member

LucioFranco commented Nov 14, 2018

bors r-

bors bot added a commit that referenced this pull request Nov 14, 2018

Merge #1137
1137: Replace std sockets with laminar sockets r=jojolepro,LucioFranco a=TimonPost

I've implemented laminar for `amethyst_network`. 

Solves: #1136

Co-authored-by: Timon Post <timonpost@hotmail.nl>
@bors

This comment has been minimized.

Contributor

bors bot commented Nov 14, 2018

Canceled

@LucioFranco

This comment has been minimized.

Member

LucioFranco commented Nov 14, 2018

@TimonPost can you squash your commits btw?

@TimonPost TimonPost force-pushed the implemented_laminar branch from f6982d1 to 084c3c4 Nov 14, 2018

@amethyst amethyst deleted a comment from bors bot Nov 14, 2018

@TimonPost

This comment has been minimized.

Member

TimonPost commented Nov 14, 2018

bors r=jojolepro,LucioFranco

bors bot added a commit that referenced this pull request Nov 14, 2018

Merge #1137
1137: Replace std sockets with laminar sockets r=jojolepro,LucioFranco a=TimonPost

I've implemented laminar for `amethyst_network`. 

Solves: #1136

Co-authored-by: Timon Post <timonpost@hotmail.nl>
@bors

This comment has been minimized.

Contributor

bors bot commented Nov 14, 2018

@bors bors bot merged commit 084c3c4 into master Nov 14, 2018

4 checks passed

bors Build succeeded
Details
concourse-ci/linux-book-tests Concourse CI build success
Details
concourse-ci/linux-tests Concourse CI build success
Details
concourse-ci/rustfmt Concourse CI build success
Details

@LucioFranco LucioFranco deleted the implemented_laminar branch Nov 30, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment