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

Convenient way to end a session #93

Open
ndev-rs opened this issue Nov 15, 2023 · 5 comments
Open

Convenient way to end a session #93

ndev-rs opened this issue Nov 15, 2023 · 5 comments
Labels
enhancement New feature or request

Comments

@ndev-rs
Copy link

ndev-rs commented Nov 15, 2023

Is your feature request related to a problem? Please describe.
When a player joins or leaves a server, we must remake the P2P session with the appropriate amount of people (gschup/ggrs#23). In bevy_ggrs v0.13, this worked easily by just removing the Session resource before creating a new one:

commands.remove_resource::<Session<Config>>();

In v0.14, we must also reset the GgrsTime resource. Otherwise the app will panic once synchronization is completed for the 2nd time in an app's lifetime:

commands.insert_resource(Time::new_with(GgrsTime::default()));

I also noticed that the new resources LocalInputs and PlayerInputs are not removed or reset, but this doesn't appear to cause any issues for me at the moment.

Describe the solution you'd like
A convenient way to end a session. Something like:

commands.end_session::<Config>();

Describe alternatives you've considered
It's just a few lines of code for now, so this is only for convenience. Though it certainly caused me a headache to diagnose :)

@ndev-rs ndev-rs added the enhancement New feature or request label Nov 15, 2023
@johanhelsing
Copy link
Collaborator

johanhelsing commented Nov 16, 2023

Thanks for reporting, I'd like a convenient way to do this as well.

I think it's best if session starting and closing mirrors each other. So perhaps best to change how we start from inserting a resource to commands as well. Perhaps:

let session = session_builder
    .start_p2p_session(socket)
    .expect("failed to start session, check config");

commands.start_session(session);

Or perhaps take a session_builder and socket instead?

commands.start_p2p_session(session_builder, socket);

@ndev-rs
Copy link
Author

ndev-rs commented Nov 21, 2023

I think it's best if session starting and closing mirrors each other. So perhaps best to change how we start from inserting a resource to commands as well. Perhaps:

let session = session_builder
    .start_p2p_session(socket)
    .expect("failed to start session, check config");

commands.start_session(session);

That would be a nice abstraction. Would it be possible to have the socket returned to us when we end the session? Something like:

let socket = commands.end_p2p_session()
    .expect("failed to return socket");

@johanhelsing
Copy link
Collaborator

Not with commands, no. It could perhaps be put into a resource where we could collect it... Not the nicest api, though

@ndev-rs
Copy link
Author

ndev-rs commented Nov 21, 2023

Not with commands, no. It could perhaps be put into a resource where we could collect it... Not the nicest api, though

Yeah, that sounds pretty ugly. I like your idea of having the api work entirely thru commands, but perhaps there could be a separate way to take the socket which would make the session inactive until we clean it up via commands. Though that's probably a separate Issue.

let socket = session.take_socket();
commands.end_p2p_session();

@johanhelsing
Copy link
Collaborator

johanhelsing commented Nov 21, 2023

Btw, my workaround for this is to create a newtype around an Arc<RwLock>, so i don't actually give the socket to ggrs

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

No branches or pull requests

2 participants