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

Marker-based API #227

merged 68 commits into from
Apr 20, 2024

Conversation

Shatur
Copy link
Contributor

@Shatur Shatur commented Apr 4, 2024

This PR have multiple purposes, but unfortunately they highly related, so I can't split it into multiple PRs.

Right for prediction crates like https://github.com/Bendzae/bevy_replicon_snap or https://github.com/RJ/bevy_timewarp we rely on overriding deserialization and remove functions. But this approach have 3 major drawbacks:

  1. In order to customize serialization for interpolated or predicted components, user need to copy the code from the crate or use some crate-provided helpers.
  2. Serialization function is is unsafe due to pointer manipulation.
  3. If you want to override writing or removal only for some entities, it's costly to check if a component present on each writing.

Marker-based API solves all of the above. Now for replication user register only serialize, deserialize and newly added deserialize_in_place. These functions assigned to rules as before. So instead of ComponentFns, we now have SerdeFns. And these function now only do one thing - serialization/deserialization.

All other logic now represented by CommandFns. Unlike SerdeFns, this struct created for each component only once and automatically when user register a SerdeFns for a component. It uses default write and remove functions. To override it, user can register a marker and assign a custom functions for each component. We use markers instead of archetypes because clients do not know in advance the archetype of the entity being deserialized.

Then on receive we collect a reusable Vec<bool> for each marker (by checking if a marker is present) and based on it pick write/remove functions for each component. We pick first marker that have a registration for the current component and present on an entity (true in the Vec). Markers are sorted by priority customizable by user.

Since for deserialization we call two functions now (write and then deserialize), we can do a nice trick to remove unsafe from ser/de customization and make it even more flexible!
Since write knows the component type of deserialize, we can store a type-erased function pointers and let write "restore" the type. "restoration" is done by calling transmute, but we abstract it inside SerdeFns and check the type if debug_assertions enabled.

So serialize and deserialize now just a normal functions with a very simple signature. This also unlocks the usage of deserialize_in_place that fallback into deserialize if not overridden by user.
Similar trick is done for serialize, except read is non-overridable by user and only used to remove extra unsafety from the public API.

The mentioned read and write are still unsafe since it's possible to pass SerdeFns that was created with a different type.

And lastly, instead of using EntityWorldMut for writing and removal, we use EntityMut and Commands. Using EntityWorldMut have 2 major drawbacks:

  • You can't borrow a component from EntityWorldMut for in-place deserialization and ClientMapper at the same time.
  • Each insertion creates a new archetype.

With commands we can spawn new entities inside ClientMapper and it will be possible to batch insertions in the future, see: bevyengine/bevy#10154

Huge thanks to @NiseVoid for the idea!

Copy link

codecov bot commented Apr 4, 2024

Codecov Report

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

Project coverage is 91.51%. Comparing base (70db508) to head (44f8e97).
Report is 1 commits behind head on master.

Files Patch % Lines
src/core/replication_fns/serde_fns.rs 86.48% 5 Missing ⚠️
src/client.rs 98.03% 1 Missing ⚠️
src/core/replication_fns/command_fns.rs 98.03% 1 Missing ⚠️
src/server.rs 83.33% 1 Missing ⚠️
src/server/replicated_archetypes.rs 85.71% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #227      +/-   ##
==========================================
+ Coverage   90.62%   91.51%   +0.88%     
==========================================
  Files          28       31       +3     
  Lines        1718     1909     +191     
==========================================
+ Hits         1557     1747     +190     
- Misses        161      162       +1     

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

@Shatur Shatur force-pushed the split-deserialize branch from afc96be to f5aa6c8 Compare April 4, 2024 20:15
Into separate `write` function to customize it independently.
@Shatur Shatur force-pushed the split-deserialize branch from f5aa6c8 to f6a640f Compare April 4, 2024 20:20
@Shatur Shatur marked this pull request as ready for review April 16, 2024 08:50
@Shatur Shatur requested a review from UkoeHB April 16, 2024 09:12
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.

This PR feels like it's working against ReplicationRules a bit. Replication rules now provide quite a bit less power since deserialize can only be used for serialization optimizations, without any additional logic.

Maybe you could somehow tie replication rule ids to which write command gets selected.

src/client.rs Outdated

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.

src/client.rs Outdated
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

src/client.rs Outdated
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();

src/client.rs Outdated Show resolved Hide resolved
src/client.rs Outdated Show resolved Hide resolved
src/core/replication_rules.rs Outdated Show resolved Hide resolved
src/core/replication_rules.rs Outdated Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
src/server/replication_messages.rs Outdated Show resolved Hide resolved
src/server/replication_messages.rs Outdated Show resolved Hide resolved
@Shatur
Copy link
Contributor Author

Shatur commented Apr 18, 2024

Maybe you could somehow tie replication rule ids to which write command gets selected.

My original plan was to just separate writing and just store it in ReplicationRule. So users would just need to supply additional write function.
But for prediction and rollback, the write function should be picked dynamically on client. And it can be guessed from server archetype because prediction and rollback are client-specific things. For example, local client player could be predicted and other players - interpolated. This is why we went with a different mechanism for writing and it should be based on client entities. And we can't use archetypes here since we don't know the archetype in advance when we deserialize.

Yes, it loses a little bit of power since writing is now component-based, rather then rule-based. But I think that it's an acceptable trade-off? Feel free to suggest anything about how we can improve it. Also in the comments I answered one of your questions about your use case. Let me know if the proposed solution works for you.

@UkoeHB
Copy link
Collaborator

UkoeHB commented Apr 18, 2024

And it can be guessed from server archetype because prediction and rollback are client-specific things.

The problem is it can't be guessed until after the first tick of replication, because CommandMarkers are filtered based on the client archetype. So you always have one tick of ambiguity.

And we can't use archetypes here since we don't know the archetype in advance when we deserialize.

This isn't totally true - we know the replication rule FnsId, which represents a segment of the server's archetype.

If you want to override writing or removal only for some entities, it's costly to check if a component present on each writing. (from the PR comment)

Now you are checking all entities for all marker components.

@Shatur Shatur force-pushed the split-deserialize branch from d51fb72 to ffe51df Compare April 18, 2024 21:35
@Shatur
Copy link
Contributor Author

Shatur commented Apr 18, 2024

Discussed in Discord. We decided to provide set_command_fns.
There have also been some suggestions to improve the safety of public API, but these will be addressed in a separate PR.

Shatur added 3 commits April 19, 2024 01:40
Open resource-based access to the newly added API.
I also bring back `ReplicationRules` resource to public. It was hidden
in this PR earlier, but after the separation rework it can be returned
back.
@Shatur Shatur force-pushed the split-deserialize branch from 6a50ffa to a56912e Compare April 19, 2024 12:28
@Shatur Shatur requested a review from UkoeHB April 19, 2024 16:50
src/core/command_markers.rs Outdated Show resolved Hide resolved
src/core/command_markers.rs Outdated Show resolved Hide resolved
@Shatur Shatur merged commit de53d66 into master Apr 20, 2024
5 checks passed
@Shatur Shatur deleted the split-deserialize branch April 20, 2024 17:10
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.

2 participants