-
Notifications
You must be signed in to change notification settings - Fork 38
Use Bevy state API for client and server states v2 #565
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
Conversation
92f62b6 to
b8fa30d
Compare
50a9742 to
a6b97fa
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #565 +/- ##
==========================================
+ Coverage 78.34% 78.45% +0.10%
==========================================
Files 57 56 -1
Lines 3187 3133 -54
==========================================
- Hits 2497 2458 -39
+ Misses 690 675 -15 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
This allows users to utilize things like `StateScoped` and run systems more efficiently using `OnEnter` or `OnExit` without evaluating conditions every frame. As a result, we now require `StatesPlugin` to be present. It's included by default in `DefaultPlugins`, but with `MinimalPlugins` you have to add it manually. In tests, I had to add `StatesPlugin` everywhere.
It's associated with both connected client on the server and the client itself.
Makes it more clear that it's only for clients.
Because it holds only messages now.
Because it holds only messages now.
|
Migrated Works great, no logical changes to any tests needed. |
|
The |
| set_stopped.run_if(resource_removed::<ExampleServer>), | ||
| ) | ||
| .chain(), | ||
| set_running.run_if(resource_added::<ExampleServer>), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should be before receive_packets.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not necessary, the packets can be passed into ServerMessages before we change the state.
| /// System that sends [`ProtocolHash`]. | ||
| /// | ||
| /// Can be used by backends to add custom logic before sending data, such as transition to a disconnected or connecting state. | ||
| PrepareSend, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd like to keep this for renet2 to use.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not against, but how do you plan to use it? I ported bevy_renet, and PrepareSend wasn't needed because of the states (I used it previously in bevy_renet).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No necessary anymore. Just like with the server everything can be done in ClientSet::ReceivePackets. Here is how I did it in renet:
https://github.com/simgine/bevy_replicon_renet/blob/9b45a0bb84f4d47d47cc595b5cfbb6309fc139db/src/client.rs#L23-L24
| add_measurements | ||
| .in_set(ClientSet::Diagnostics) | ||
| .run_if(client_connected), | ||
| .run_if(in_state(ClientState::Connected)), | ||
| ) | ||
| .add_systems( | ||
| OnEnter(ClientState::Connected), | ||
| add_measurements.in_set(ClientSet::Diagnostics), | ||
| ) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks very awkward to me. Adding states means there are systems in the main schedule and in state schedules, which is an increase in complexity not a decrease.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I ran Replicon systems upon entering the schedule and within the schedule. This lets me process packets immediately after connection. It's not ideal, but I think it's worth it.
| /// [`SyncRelatedAppExt::sync_related_entities`]. | ||
| /// | ||
| /// Can be used by backends to add custom logic before sending data, such as transition to a stopped state. | ||
| PrepareSend, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd like to keep this for use in renet2.
Recently I took a quick look at how backends handle system ordering, and they didn't do it the way I intended. For example, no one uses the
PrepareSendsets. This isn't the authors' fault - it just means our API sucks and we need to improve it.This PR resurrects (and extends) my previous attempt to migrate to states (#487), which solves the problem. I closed it last time because of the inevitable one-frame delay:
StateTransitionsruns afterPreUpdate, but backend receive systems (and ours) run inPreUpdate. Since our systems rely on the state, they wouldn't run until the next frame.But I found a simple solution: run our receive systems additionally in
OnEnter(ClientState::Connected)!Pros of this approach:
StateScoped, and run systems more efficiently usingOnEnterorOnExitinstead of evaluating conditions every frame.PrepareSendset or tricky state updates split betweenPreUpdateandPostUpdate. Backends just update the state inPreUpdateduringReceivePackets.RepliconServerandRepliconClient. Since they only hold messages, they're now renamed toServerMessagesandClientMessages.bevy_rewind.Cons:
bevy_state, which means we have to use it everywhere in our tests since we rely onMinimalPlugins. But I think that's fine.To make the review simpler, I split each change into a separate commit. I'd recommend reviewing them one by one. The only important change is 71e97b1. Everything else is just renames/refactors (which take most of the PR diff).