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

Abstract I/O #204

Merged
merged 52 commits into from
Mar 5, 2024
Merged

Abstract I/O #204

merged 52 commits into from
Mar 5, 2024

Conversation

Shatur
Copy link
Contributor

@Shatur Shatur commented Feb 26, 2024

With the current implementation in order to disconnect user need to write library-specific code. For example, in case of renet user will need to call RenetClient::disconnect or remove RenetClient resource.

I think we can do better by hiding RepliconServer::set_running and RepliconClient::set_status and providing events for messaging libraries and users to control it. But I will probably do it in a follow-up PR. I decided to keep only minimal functionality in RepliconServer and RepliconClient required for replicon to function.

@Shatur Shatur requested a review from UkoeHB February 26, 2024 23:43
With the current implementation in order to disconnect user need to
write library-specific code. For example, in case of renet user will
need to call `RenetClient::disconnect` or remove `RenetClient` resource.

I think we can do better by hiding `RepliconServer::set_active` and
`RepliconClient::set_status` and providing events for messaging
libraries and users to control it. But I will probably do it in a
follow-up PR.

We can include more library-independent things into `RepliconServer`,
but I would also do it in a follow-up PRs.
Copy link

codecov bot commented Feb 27, 2024

Codecov Report

Attention: Patch coverage is 94.08740% with 23 lines in your changes are missing coverage. Please review.

Project coverage is 90.72%. Comparing base (d4c2876) to head (7c0d8f3).

Files Patch % Lines
src/client/replicon_client.rs 87.23% 6 Missing ⚠️
src/core/common_conditions.rs 76.19% 5 Missing ⚠️
bevy_replicon_renet/src/lib.rs 96.42% 3 Missing ⚠️
src/core/replicon_channels.rs 87.50% 2 Missing ⚠️
src/network_event/server_event.rs 83.33% 2 Missing ⚠️
src/client.rs 93.33% 1 Missing ⚠️
src/network_event/client_event.rs 91.66% 1 Missing ⚠️
src/server.rs 97.29% 1 Missing ⚠️
src/server/connected_clients.rs 80.00% 1 Missing ⚠️
src/server/replicon_server.rs 97.72% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #204      +/-   ##
==========================================
+ Coverage   89.37%   90.72%   +1.35%     
==========================================
  Files          21       28       +7     
  Lines        1421     1672     +251     
==========================================
+ Hits         1270     1517     +247     
- Misses        151      155       +4     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

This was linked to issues Feb 27, 2024
Copy link
Collaborator

@UkoeHB UkoeHB left a comment

Choose a reason for hiding this comment

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

Review part 1

Shatur and others added 2 commits March 3, 2024 12:22
Co-authored-by: UkoeHB <37489173+UkoeHB@users.noreply.github.com>
Copy link
Collaborator

@UkoeHB UkoeHB left a comment

Choose a reason for hiding this comment

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

Part 3

Copy link
Collaborator

@UkoeHB UkoeHB left a comment

Choose a reason for hiding this comment

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

Part 4 (done). Overall it's a clean abstraction, nice job!

@@ -662,7 +663,7 @@ fn write_with<'a>(
}

fn can_pack(header_size: usize, base: usize, add: usize) -> bool {
const MAX_PACKET_SIZE: usize = 1200; // https://github.com/lucaspoffo/renet/blob/acee8b470e34c70d35700d96c00fb233d9cf6919/renet/src/packet.rs#L7
const MAX_PACKET_SIZE: usize = 1200;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe this should be configurable by the backend, can be done in a future PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added TODO.

Shatur and others added 3 commits March 4, 2024 23:47
Co-authored-by: UkoeHB <37489173+UkoeHB@users.noreply.github.com>
Co-authored-by: UkoeHB <37489173+UkoeHB@users.noreply.github.com>
@Shatur Shatur requested a review from UkoeHB March 4, 2024 22:32
client.filter(|client| client.is_connected()).is_some()
}

/// Returns `true` if the server stopped on this tick.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@UkoeHB maybe clarity that it's not exactly on this tick, but more like since the condition run? Because if you run this condition again on the same "stopped" tick, it will return false.

Copy link
Collaborator

Choose a reason for hiding this comment

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

  1. Bevy runs all run conditions every tick.
  2. Separate instances of this run condition will have different locals, so individual instances won't be run twice on the same tick.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, it's Bevy's tick. I assumed that we meant replicon tick as in other places.

@Shatur Shatur merged commit 3dd0a1e into master Mar 5, 2024
6 checks passed
@Shatur Shatur deleted the io-abstraction branch March 5, 2024 08:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support for custom renet transports More modularity
2 participants