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

Marker-based API #227

Merged
merged 68 commits into from
Apr 20, 2024
Merged
Show file tree
Hide file tree
Changes from 51 commits
Commits
Show all changes
68 commits
Select commit Hold shift + click to select a range
f6a640f
Split writing logic from `deserialize`
Shatur Apr 4, 2024
a002545
Use `MaybeUninit` and swap writing and deserialization
Shatur Apr 4, 2024
dc016c8
Improve docs
Shatur Apr 4, 2024
61217dc
Use commands [skip ci]
Shatur Apr 10, 2024
fbbcb8e
Initial working version with transmute [skip ci]
Shatur Apr 12, 2024
01aa915
Use safe API to create commands and `EntityMut`
Shatur Apr 12, 2024
d63c825
Test deserialize in place [skip ci]
Shatur Apr 12, 2024
61c3150
"Fix" UB [skip ci]
Shatur Apr 12, 2024
b6c0912
Revert ""Fix" UB [skip ci]"
Shatur Apr 12, 2024
430591e
Actually fix UB [skip ci]
Shatur Apr 12, 2024
57ff75b
Add bounds
Shatur Apr 12, 2024
7585c52
Make `deserialize_in_place` to default into `deserialize`
Shatur Apr 12, 2024
b553307
Store Ids inside `ReplicationFns`
Shatur Apr 12, 2024
e70ef65
Update naming [skip ci]
Shatur Apr 12, 2024
f83986e
Add markers API [skip ci]
Shatur Apr 13, 2024
d3d2e44
Pass `CommandMarkers` directly [skip ci]
Shatur Apr 13, 2024
2c2fbe3
Verify serde functions component
Shatur Apr 13, 2024
ac93b3d
Document serde functions
Shatur Apr 13, 2024
7c2d740
Refactor names
Shatur Apr 13, 2024
c8a5661
Add default mapped registration
Shatur Apr 13, 2024
e30e3fd
Remember number of slots
Shatur Apr 13, 2024
070d877
Document `CommandFns`
Shatur Apr 13, 2024
f198beb
Use `usize` instead of `CommandFnsId`
Shatur Apr 13, 2024
c61634a
Swap stored functions in `ReplicationFns`
Shatur Apr 13, 2024
fd46342
Refactor and document `ReplicationFns`
Shatur Apr 13, 2024
4fd415b
Rename `AppReplicationExt` into `AppRuleExt`
Shatur Apr 13, 2024
85ab9e7
Add documentation for command markers
Shatur Apr 13, 2024
0f1fe63
Minor comment correction [skip ci]
Shatur Apr 13, 2024
8d23bb8
More doc updates
Shatur Apr 13, 2024
bb6471a
Refactor unsafe comments and blocks
Shatur Apr 14, 2024
c3836dd
Improve docs for replicate_with
Shatur Apr 14, 2024
8df39cd
Add markers mention in the quick start guide
Shatur Apr 14, 2024
06145a6
Hide `ReplicationRules`
Shatur Apr 14, 2024
e371006
Update changelog
Shatur Apr 14, 2024
14f2372
Fix replication into scene docs
Shatur Apr 14, 2024
9a3d094
Add test for markers sorting
Shatur Apr 14, 2024
531da74
Add a test for non-registered marker
Shatur Apr 14, 2024
b1bb317
Use more consistent comment
Shatur Apr 14, 2024
53fa8d2
Add missing safety comment
Shatur Apr 14, 2024
abae767
Clarify why we don't use archetypes
Shatur Apr 14, 2024
5a1f4e7
Merge remote-tracking branch 'origin/master' into split-deserialize
Shatur Apr 14, 2024
b531891
Add small unit test for multiple serde fns
Shatur Apr 14, 2024
48f2a37
Panic on size mismatch
Shatur Apr 14, 2024
abc1339
Add unit test for panic
Shatur Apr 14, 2024
66e54ad
Adjust comment
Shatur Apr 14, 2024
e6abdb1
Swap generic params inside `register_marker_fns`
Shatur Apr 15, 2024
55cadc2
Add unit test for markers
Shatur Apr 15, 2024
578b944
Merge remote-tracking branch 'origin/master' into split-deserialize
Shatur Apr 15, 2024
e5ea886
Add integration tests
Shatur Apr 15, 2024
9a2a9c3
Fix safety comments
Shatur Apr 16, 2024
fdb524b
Fix history example
Shatur Apr 16, 2024
1455ab4
Apply suggestions from code review [skip ci]
Shatur Apr 18, 2024
a877b83
Update src/core/replication_rules.rs [skip ci]
Shatur Apr 18, 2024
121f2db
Swap order in `ReplicationFns::get`
Shatur Apr 18, 2024
4efa05c
Fix wrong name
Shatur Apr 18, 2024
8236587
Use checked access for `ReplicationFns::register_marker_fns`
Shatur Apr 18, 2024
753e3e1
Assert if marker fn is already set
Shatur Apr 18, 2024
870384d
Rewrite markers example
Shatur Apr 18, 2024
7dde4d3
Replace `unwrap` with `expect`
Shatur Apr 18, 2024
5117c38
Rename `register_marker_fns` into `set_marker_fns`
Shatur Apr 18, 2024
ffe51df
Add the ability to override default command fns
Shatur Apr 18, 2024
c5a1597
Extend public API
Shatur Apr 18, 2024
341bba0
Use more descriptive name for the default deserialize in place
Shatur Apr 19, 2024
a56912e
Use more descriptive names for default functions
Shatur Apr 19, 2024
6d9e725
Hide setters for resources to avoid users confusion
Shatur Apr 19, 2024
28da5eb
Fix docs
Shatur Apr 19, 2024
cf99bf2
Rename and clarify marker index
Shatur Apr 19, 2024
44f8e97
Apply suggestions from code review
UkoeHB Apr 20, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 14 additions & 8 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -9,23 +9,29 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0

### Added

- `AppReplicationExt::replicate_group` and `GroupRegistration` trait to register and customize component groups.
- `AppMarkerExt` to customize how components will be written based on markers present without overriding deserialization functions. Now third-party prediction crates could be integrated much easier.
- `AppRuleExt::replicate_group` and `GroupRegistration` trait to register and customize component groups. A group will be replicated only if all its components present on an entity.
- `ServerSet::StoreHierarchy` for systems that store hierarchy changes in `ParentSync`.
- `ReplicationRule` that describes how a component or a group of components will be serialized, deserialized and removed.

### Changed

- `AppReplicationExt::replicate_with` now accepts newly added `ReplicationFns` and the function is now `unsafe` (it was never "safe" before, caller must ensure that used `C` can be passed to the serialization function).
- `SerializeFn` now accepts regular `C` instead of `Ptr`.
- `DeserializeFn` now does only deserialization and returns `C`. Use the newly added marker-based API if you want to customize how component will be written. See docs for `AppMarkerExt` for details.
- Rename `AppReplicationExt` into `AppRuleExt`.
- `AppRuleExt::replicate_with` now no longer accepts `RemoveComponentFn`. Use the mentioned marker-based API to customize it instead.
- `AppRuleExt::replicate_with` now additionally accepts `deserialize_in_place`. You can use it to customize deserialization if a component is already present or just pass `command_fns::deserialize_in_place` to make it fallback to the passed `deserialize`.
- Writing to entities on client now done via `EntityMut` and `Commands` instead of `EntityWorldMut`. It was needed to support the mentioned in-place deserialization and will possibly allow batching insertions in the future (for details see https://github.com/bevyengine/bevy/issues/10154).
- Move `Replication` to `core` module.
- Move all functions-related logic from `ReplicationRules` into a new `ReplicationFns`.
- Rename `serialize_component` into `serialize` and move into `replication_fns` module.
- Rename `deserialize_component` into `deserialize` and move into `replication_fns` module.
- Rename `remove_component` into `remove` and move into `replication_fns` module.
- Move all functions-related logic from `ReplicationRules` into a new `ReplicationFns` and hide `ReplicationRules` from public API.
- Rename `serialize_component` into `serialize` and move into `serde_fns` module.
- Rename `deserialize_component` into `deserialize` and move into `serde_fns` module.
- Rename `deserialize_mapped_component` into `deserialize_mapped` and move into `serde_fns` module.
- Rename `remove_component` into `remove` and move into `command_fns` module.
- Move `despawn_recursive` into `replication_fns` module.

### Removed

- `dont_replicate` module. Use the newly added `AppReplicationExt::replicate_group` or newtypes.
- `dont_replicate` module. Use the newly added `AppRuleExt::replicate_group` or newtypes.

## [0.24.1] - 2024-03-07

Expand Down
150 changes: 116 additions & 34 deletions src/client.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4,12 +4,16 @@ pub mod replicon_client;

use std::io::Cursor;

use bevy::{ecs::entity::EntityHashMap, prelude::*};
use bevy::{
ecs::{entity::EntityHashMap, system::SystemState},
prelude::*,
};
use bincode::{DefaultOptions, Options};
use bytes::Bytes;
use varint_rs::VarintReader;

use crate::core::{
command_markers::CommandMarkers,
common_conditions::{client_connected, client_just_connected, client_just_disconnected},
replication_fns::ReplicationFns,
replicon_channels::{ReplicationChannel, RepliconChannels},
Expand Down Expand Up @@ -77,28 +81,35 @@ impl ClientPlugin {
/// Acknowledgments for received entity update messages are sent back to the server.
///
/// See also [`ReplicationMessages`](crate::server::replication_messages::ReplicationMessages).
pub(super) fn receive_replication(world: &mut World) -> bincode::Result<()> {
pub(super) fn receive_replication(
world: &mut World,
state: &mut ReceiveState,
) -> bincode::Result<()> {
world.resource_scope(|world, mut client: Mut<RepliconClient>| {
world.resource_scope(|world, mut entity_map: Mut<ServerEntityMap>| {
world.resource_scope(|world, mut entity_ticks: Mut<ServerEntityTicks>| {
world.resource_scope(|world, mut buffered_updates: Mut<BufferedUpdates>| {
world.resource_scope(|world, replication_fns: Mut<ReplicationFns>| {
let mut stats = world.remove_resource::<ClientStats>();
apply_replication(
world,
&mut client,
&mut entity_map,
&mut entity_ticks,
&mut buffered_updates,
stats.as_mut(),
&replication_fns,
)?;

if let Some(stats) = stats {
world.insert_resource(stats);
}

Ok(())
world.resource_scope(|world, command_markers: Mut<CommandMarkers>| {
world.resource_scope(|world, replication_fns: Mut<ReplicationFns>| {
let mut stats = world.remove_resource::<ClientStats>();
apply_replication(
world,
state,
&mut client,
&mut entity_map,
&mut entity_ticks,
&mut buffered_updates,
stats.as_mut(),
&command_markers,
&replication_fns,
)?;

if let Some(stats) = stats {
world.insert_resource(stats);
}

Ok(())
})
})
})
})
Expand All @@ -124,20 +135,24 @@ impl ClientPlugin {
/// Sends acknowledgments for update messages back.
fn apply_replication(
world: &mut World,
state: &mut ReceiveState,
client: &mut RepliconClient,
entity_map: &mut ServerEntityMap,
entity_ticks: &mut ServerEntityTicks,
buffered_updates: &mut BufferedUpdates,
mut stats: Option<&mut ClientStats>,
command_markers: &CommandMarkers,
replication_fns: &ReplicationFns,
) -> Result<(), Box<bincode::ErrorKind>> {
while let Some(message) = client.receive(ReplicationChannel::Init) {
apply_init_message(
&message,
world,
state,
entity_map,
entity_ticks,
stats.as_deref_mut(),
command_markers,
replication_fns,
)?;
}
Expand All @@ -147,10 +162,12 @@ fn apply_replication(
let index = apply_update_message(
message,
world,
state,
entity_map,
entity_ticks,
buffered_updates,
stats.as_deref_mut(),
command_markers,
replication_fns,
replicon_tick,
)?;
Expand All @@ -168,9 +185,11 @@ fn apply_replication(
if let Err(e) = apply_update_components(
&mut Cursor::new(&*update.message),
world,
state,
entity_map,
entity_ticks,
stats.as_deref_mut(),
command_markers,
replication_fns,
update.message_tick,
) {
Expand All @@ -188,9 +207,11 @@ fn apply_replication(
fn apply_init_message(
message: &[u8],
world: &mut World,
state: &mut ReceiveState,
entity_map: &mut ServerEntityMap,
entity_ticks: &mut ServerEntityTicks,
mut stats: Option<&mut ClientStats>,
command_markers: &CommandMarkers,
replication_fns: &ReplicationFns,
) -> bincode::Result<()> {
let end_pos: u64 = message.len().try_into().unwrap();
Expand Down Expand Up @@ -226,10 +247,12 @@ fn apply_init_message(
apply_init_components(
&mut cursor,
world,
state,
entity_map,
entity_ticks,
stats.as_deref_mut(),
ComponentsKind::Removal,
command_markers,
replication_fns,
replicon_tick,
)?;
Expand All @@ -240,10 +263,12 @@ fn apply_init_message(
apply_init_components(
&mut cursor,
world,
state,
entity_map,
entity_ticks,
stats,
ComponentsKind::Insert,
command_markers,
replication_fns,
replicon_tick,
)?;
Expand All @@ -260,10 +285,12 @@ fn apply_init_message(
fn apply_update_message(
message: Bytes,
world: &mut World,
state: &mut ReceiveState,
entity_map: &mut ServerEntityMap,
entity_ticks: &mut ServerEntityTicks,
buffered_updates: &mut BufferedUpdates,
mut stats: Option<&mut ClientStats>,
command_markers: &CommandMarkers,
replication_fns: &ReplicationFns,
replicon_tick: RepliconTick,
) -> bincode::Result<u16> {
Expand All @@ -289,9 +316,11 @@ fn apply_update_message(
apply_update_components(
&mut cursor,
world,
state,
entity_map,
entity_ticks,
stats,
command_markers,
replication_fns,
message_tick,
)?;
Expand Down Expand Up @@ -330,37 +359,62 @@ fn apply_entity_mappings(
fn apply_init_components(
cursor: &mut Cursor<&[u8]>,
world: &mut World,
state: &mut ReceiveState,
entity_map: &mut ServerEntityMap,
entity_ticks: &mut ServerEntityTicks,
mut stats: Option<&mut ClientStats>,
components_kind: ComponentsKind,
command_markers: &CommandMarkers,
replication_fns: &ReplicationFns,
replicon_tick: RepliconTick,
) -> bincode::Result<()> {
let entities_len: u16 = bincode::deserialize_from(&mut *cursor)?;
for _ in 0..entities_len {
let entity = deserialize_entity(cursor)?;
let server_entity = deserialize_entity(cursor)?;
let data_size: u16 = bincode::deserialize_from(&mut *cursor)?;
let mut entity = entity_map.get_by_server_or_spawn(world, entity);
entity_ticks.insert(entity.id(), replicon_tick);

let client_entity =
entity_map.get_by_server_or_insert(server_entity, || world.spawn(Replication).id());
entity_ticks.insert(client_entity, replicon_tick);

let (mut entity_markers, mut commands, mut query) = state.get_mut(world);
let mut server_entity = query.get_mut(client_entity).unwrap();
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
let mut server_entity = query.get_mut(client_entity).unwrap();
let mut client_entity = query.get_mut(client_entity).unwrap();

entity_markers.extend(command_markers.iter_contains(&server_entity));

let end_pos = cursor.position() + data_size as u64;
let mut components_len = 0u32;
while cursor.position() < end_pos {
let fns_id = DefaultOptions::new().deserialize_from(&mut *cursor)?;
let fns = replication_fns.component_fns(fns_id);
let (serde_fns, command_fns) = replication_fns.get(fns_id);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
let (serde_fns, command_fns) = replication_fns.get(fns_id);
let (command_fns, serde_fns) = replication_fns.get(fns_id);

Nit: this order seems more consistent with how they are used.

match components_kind {
ComponentsKind::Insert => {
(fns.deserialize)(&mut entity, entity_map, cursor, replicon_tick)?
}
ComponentsKind::Removal => (fns.remove)(&mut entity, replicon_tick),
ComponentsKind::Insert => unsafe {
// SAFETY: `serde_fns` and `command_fns` was created for the same type.
Shatur marked this conversation as resolved.
Show resolved Hide resolved
command_fns.write(
serde_fns,
&entity_markers,
&mut commands,
&mut server_entity,
cursor,
entity_map,
replicon_tick,
)?
},
ComponentsKind::Removal => command_fns.remove(
&entity_markers,
commands.entity(server_entity.id()),
replicon_tick,
),
}
components_len += 1;
}

if let Some(stats) = &mut stats {
stats.entities_changed += 1;
stats.components_changed += components_len;
}

entity_markers.clear();
state.apply(world);
}

Ok(())
Expand Down Expand Up @@ -403,44 +457,66 @@ fn apply_despawns(
fn apply_update_components(
cursor: &mut Cursor<&[u8]>,
world: &mut World,
state: &mut ReceiveState,
entity_map: &mut ServerEntityMap,
entity_ticks: &mut ServerEntityTicks,
mut stats: Option<&mut ClientStats>,
command_markers: &CommandMarkers,
replication_fns: &ReplicationFns,
message_tick: RepliconTick,
) -> bincode::Result<()> {
let message_end = cursor.get_ref().len() as u64;
while cursor.position() < message_end {
let entity = deserialize_entity(cursor)?;
let server_entity = deserialize_entity(cursor)?;
let data_size: u16 = bincode::deserialize_from(&mut *cursor)?;
let Some(mut entity) = entity_map.get_by_server(world, entity) else {

let Some(client_entity) = entity_map.get_by_server(server_entity) else {
// Update could arrive after a despawn from init message.
debug!("ignoring update received for unknown server's {entity:?}");
debug!("ignoring update received for unknown server's {server_entity:?}");
cursor.set_position(cursor.position() + data_size as u64);
continue;
};
let entity_tick = entity_ticks
.get_mut(&entity.id())
.get_mut(&client_entity)
.expect("all entities from update should have assigned ticks");
if message_tick <= *entity_tick {
trace!("ignoring outdated update for client's {:?}", entity.id());
trace!("ignoring outdated update for client's {client_entity:?}");
cursor.set_position(cursor.position() + data_size as u64);
continue;
}
*entity_tick = message_tick;

let (mut entity_markers, mut commands, mut query) = state.get_mut(world);
let mut client_entity = query.get_mut(client_entity).unwrap();
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is there a race condition where the unwrap can fire? I.e. despawning in PreUpdate.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We have test for combining insertions or updates with despawns, they pass.

But yes, if not server, but client itself despawn an entity in PreUpdate, it will panic. This is how it was before (World::entity_mut panics if there is no such entity).
I probably should replace it with expect with something like "replicated entities can be despawned only by server"?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Sure that would work

entity_markers.extend(command_markers.iter_contains(&client_entity));

let end_pos = cursor.position() + data_size as u64;
let mut components_count = 0u32;
while cursor.position() < end_pos {
let fns_id = DefaultOptions::new().deserialize_from(&mut *cursor)?;
let fns = replication_fns.component_fns(fns_id);
(fns.deserialize)(&mut entity, entity_map, cursor, message_tick)?;
let (serde_fns, command_fns) = replication_fns.get(fns_id);
// SAFETY: `serde_fns` and `command_fns` was created for the same type.
Shatur marked this conversation as resolved.
Show resolved Hide resolved
unsafe {
command_fns.write(
serde_fns,
&entity_markers,
&mut commands,
&mut client_entity,
cursor,
entity_map,
message_tick,
)?;
}
components_count += 1;
}

if let Some(stats) = &mut stats {
stats.entities_changed += 1;
stats.components_changed += components_count;
}

entity_markers.clear();
state.apply(world);
}

Ok(())
Expand All @@ -464,6 +540,12 @@ fn deserialize_entity(cursor: &mut Cursor<&[u8]>) -> bincode::Result<Entity> {
Ok(Entity::from_bits(bits))
}

type ReceiveState<'w, 's> = SystemState<(
Local<'s, Vec<bool>>,
Commands<'w, 's>,
Query<'w, 's, EntityMut<'w>>,
)>;

/// Type of components replication.
///
/// Parameter for [`apply_components`].
Expand Down
Loading