From 13d970bf6f4779b3a8d2a064527e5fb521415bd2 Mon Sep 17 00:00:00 2001 From: Imbris Date: Wed, 19 Jan 2022 00:56:26 -0500 Subject: [PATCH] Rename SyncFrom::AllEntities to SyncFrom::AnyEntity for more clarity, add more comments for component syncing code, address MR comment --- common/base/src/lib.rs | 2 + common/net/src/msg/ecs_packet.rs | 18 +++++++ common/net/src/sync/net_sync.rs | 10 ++-- common/net/src/sync/packet.rs | 2 + common/net/src/sync/track.rs | 12 ++--- common/net/src/synced_components.rs | 67 +++++++++++++++--------- common/systems/src/character_behavior.rs | 10 +++- common/systems/src/stats.rs | 4 ++ server/src/sys/entity_sync.rs | 4 +- server/src/sys/sentinel.rs | 35 +++++++++++-- 10 files changed, 123 insertions(+), 41 deletions(-) diff --git a/common/base/src/lib.rs b/common/base/src/lib.rs index f9e3743287..f221ba6503 100644 --- a/common/base/src/lib.rs +++ b/common/base/src/lib.rs @@ -6,6 +6,8 @@ pub use userdata_dir::userdata_dir; #[cfg(feature = "tracy")] pub use tracy_client; +/// Allows downstream crates to conditionally do things based on whether tracy +/// is enabled without having to expose a cargo feature themselves. pub const TRACY_ENABLED: bool = cfg!(feature = "tracy"); #[cfg(not(feature = "tracy"))] diff --git a/common/net/src/msg/ecs_packet.rs b/common/net/src/msg/ecs_packet.rs index 00d6994a50..87f24567a1 100644 --- a/common/net/src/msg/ecs_packet.rs +++ b/common/net/src/msg/ecs_packet.rs @@ -3,18 +3,32 @@ use common::comp; use serde::{Deserialize, Serialize}; use std::marker::PhantomData; +/// This macro defines [`EcsCompPacke`] +/// +/// It is meant to be passed to the `synced_components!` macro which will call +/// it with a list of components. macro_rules! comp_packet { ($($component_name:ident: $component_type:ident,)*) => { + + // `sum_type!` will automatically derive From for EcsCompPacket + // for each variant EcsCompPacket::T(T). sum_type::sum_type! { #[derive(Clone, Debug, Serialize, Deserialize)] pub enum EcsCompPacket { + // Note: also use the component_type identifier + // to name the enum variant that contains the component. $($component_type($component_type),)* + // These aren't included in the "synced_components" because the way + // we determine if they are changed and when to send them is different + // from the other components. Pos(comp::Pos), Vel(comp::Vel), Ori(comp::Ori), } } + // `sum_type!` will automatically derive From> for EcsCompPhantom + // for each variant EcsCompPhantom::T(PhantomData). sum_type::sum_type! { #[derive(Clone, Debug, Serialize, Deserialize)] pub enum EcsCompPhantom { @@ -84,5 +98,9 @@ macro_rules! comp_packet { } } +// Import all the component types so they will be available when expanding the +// macro below. use crate::synced_components::*; +// Pass `comp_packet!` macro to this "x macro" which will invoke it with a list +// of components. This will declare the types defined in the macro above. crate::synced_components!(comp_packet); diff --git a/common/net/src/sync/net_sync.rs b/common/net/src/sync/net_sync.rs index 72535aec02..eac929057c 100644 --- a/common/net/src/sync/net_sync.rs +++ b/common/net/src/sync/net_sync.rs @@ -1,5 +1,5 @@ //! Types of syncing: -//! * synced from all entities +//! * synced from any entity (within range) //! * synced only from the client's entity //! //! Types of updating @@ -25,6 +25,10 @@ where //type UpdateFrom = Self; //type Update: From = Self; + /// Determines what for entities this component is synced to the client. + /// + /// For example, [`SyncFrom::ClientEntity`] can be used to only sync the + /// components for the client's own entity. const SYNC_FROM: SyncFrom; // sync::handle_modify(comp, entity, world) @@ -38,9 +42,9 @@ where fn pre_modify(&mut self, world: &specs::World) { let _world = world; } } -/// Whether a component is synced to the client for all entities or for just the +/// Whether a component is synced to the client for any entity or for just the /// client's own entity. pub enum SyncFrom { - AllEntities, + AnyEntity, ClientEntity, } diff --git a/common/net/src/sync/packet.rs b/common/net/src/sync/packet.rs index 7b022567e9..3dc1e642bd 100644 --- a/common/net/src/sync/packet.rs +++ b/common/net/src/sync/packet.rs @@ -184,6 +184,8 @@ impl CompSyncPackage

{ tracker.get_updates_for(uids, storage, filter, &mut self.comp_updates); } + /// If there was an update to the component `C` on the provided entity this + /// will add the update to this package. pub fn add_component_update<'a, C: Component + Clone + Send + Sync>( &mut self, tracker: &UpdateTracker, diff --git a/common/net/src/sync/track.rs b/common/net/src/sync/track.rs index 6e5772e962..fa18d18466 100644 --- a/common/net/src/sync/track.rs +++ b/common/net/src/sync/track.rs @@ -139,17 +139,17 @@ impl UpdateTracker { C::Storage: specs::storage::Tracked, { let id = entity.id(); - // Generate inserted update if one exists + // Generate update if one exists. // // Note: presence of the id in these bitsets should be mutually exclusive - if self.inserted.contains(id) { - storage - .get(entity) - .map(|comp| CompUpdateKind::Inserted(P::from(comp.clone()))) - } else if self.modified.contains(id) { + if self.modified.contains(id) { storage .get(entity) .map(|comp| CompUpdateKind::Modified(P::from(comp.clone()))) + } else if self.inserted.contains(id) { + storage + .get(entity) + .map(|comp| CompUpdateKind::Inserted(P::from(comp.clone()))) } else if self.removed.contains(id) { Some(CompUpdateKind::Removed(P::Phantom::from(PhantomData::))) } else { diff --git a/common/net/src/synced_components.rs b/common/net/src/synced_components.rs index de9e5e0f6c..d8d5963b35 100644 --- a/common/net/src/synced_components.rs +++ b/common/net/src/synced_components.rs @@ -14,6 +14,8 @@ //! when using the x macro defined here which requires this. /// This provides a lowercase name and the component type. +/// +/// See [module](self) level docs for more details. #[macro_export] macro_rules! synced_components { ($macro:ident) => { @@ -70,13 +72,26 @@ macro_rules! reexport_comps { use common::link::Is; use common::mounting::{Mount, Rider}; + // We alias these because the identifier used for the + // component's type is reused as an enum variant name + // in the macro's that we pass to `synced_components!`. + // + // This is also the reason we need this inner module, since + // we can't just re-export all the types directly from `common::comp`. pub type IsMount = Is; pub type IsRider = Is; } + // Re-export all the component types. So that uses of `synced_components!` outside this + // module can bring them into scope with a single glob import. $(pub use inner::$type;)* } } +// Pass `reexport_comps` macro to the "x macro" which will invoke it with a list +// of components. +// +// Note: this brings all these components into scope for the implementations +// below. synced_components!(reexport_comps); // =============================== @@ -85,28 +100,30 @@ synced_components!(reexport_comps); use crate::sync::{NetSync, SyncFrom}; +// These are synced from any entity within range. + impl NetSync for Body { - const SYNC_FROM: SyncFrom = SyncFrom::AllEntities; + const SYNC_FROM: SyncFrom = SyncFrom::AnyEntity; } impl NetSync for Stats { - const SYNC_FROM: SyncFrom = SyncFrom::AllEntities; + const SYNC_FROM: SyncFrom = SyncFrom::AnyEntity; } impl NetSync for Buffs { - const SYNC_FROM: SyncFrom = SyncFrom::AllEntities; + const SYNC_FROM: SyncFrom = SyncFrom::AnyEntity; } impl NetSync for Auras { - const SYNC_FROM: SyncFrom = SyncFrom::AllEntities; + const SYNC_FROM: SyncFrom = SyncFrom::AnyEntity; } impl NetSync for Energy { - const SYNC_FROM: SyncFrom = SyncFrom::AllEntities; + const SYNC_FROM: SyncFrom = SyncFrom::AnyEntity; } impl NetSync for Health { - const SYNC_FROM: SyncFrom = SyncFrom::AllEntities; + const SYNC_FROM: SyncFrom = SyncFrom::AnyEntity; fn pre_insert(&mut self, world: &specs::World) { use common::resources::Time; @@ -128,78 +145,78 @@ impl NetSync for Health { } impl NetSync for Poise { - const SYNC_FROM: SyncFrom = SyncFrom::AllEntities; + const SYNC_FROM: SyncFrom = SyncFrom::AnyEntity; } impl NetSync for LightEmitter { - const SYNC_FROM: SyncFrom = SyncFrom::AllEntities; + const SYNC_FROM: SyncFrom = SyncFrom::AnyEntity; } impl NetSync for Item { - const SYNC_FROM: SyncFrom = SyncFrom::AllEntities; + const SYNC_FROM: SyncFrom = SyncFrom::AnyEntity; } impl NetSync for Scale { - const SYNC_FROM: SyncFrom = SyncFrom::AllEntities; + const SYNC_FROM: SyncFrom = SyncFrom::AnyEntity; } impl NetSync for Group { - const SYNC_FROM: SyncFrom = SyncFrom::AllEntities; + const SYNC_FROM: SyncFrom = SyncFrom::AnyEntity; } impl NetSync for IsMount { - const SYNC_FROM: SyncFrom = SyncFrom::AllEntities; + const SYNC_FROM: SyncFrom = SyncFrom::AnyEntity; } impl NetSync for IsRider { - const SYNC_FROM: SyncFrom = SyncFrom::AllEntities; + const SYNC_FROM: SyncFrom = SyncFrom::AnyEntity; } impl NetSync for Mass { - const SYNC_FROM: SyncFrom = SyncFrom::AllEntities; + const SYNC_FROM: SyncFrom = SyncFrom::AnyEntity; } impl NetSync for Density { - const SYNC_FROM: SyncFrom = SyncFrom::AllEntities; + const SYNC_FROM: SyncFrom = SyncFrom::AnyEntity; } impl NetSync for Collider { - const SYNC_FROM: SyncFrom = SyncFrom::AllEntities; + const SYNC_FROM: SyncFrom = SyncFrom::AnyEntity; } impl NetSync for Sticky { - const SYNC_FROM: SyncFrom = SyncFrom::AllEntities; + const SYNC_FROM: SyncFrom = SyncFrom::AnyEntity; } impl NetSync for CharacterState { - const SYNC_FROM: SyncFrom = SyncFrom::AllEntities; + const SYNC_FROM: SyncFrom = SyncFrom::AnyEntity; } impl NetSync for Shockwave { - const SYNC_FROM: SyncFrom = SyncFrom::AllEntities; + const SYNC_FROM: SyncFrom = SyncFrom::AnyEntity; } impl NetSync for BeamSegment { - const SYNC_FROM: SyncFrom = SyncFrom::AllEntities; + const SYNC_FROM: SyncFrom = SyncFrom::AnyEntity; } impl NetSync for Alignment { - const SYNC_FROM: SyncFrom = SyncFrom::AllEntities; + const SYNC_FROM: SyncFrom = SyncFrom::AnyEntity; } impl NetSync for Player { - const SYNC_FROM: SyncFrom = SyncFrom::AllEntities; + const SYNC_FROM: SyncFrom = SyncFrom::AnyEntity; } impl NetSync for Inventory { - const SYNC_FROM: SyncFrom = SyncFrom::AllEntities; + const SYNC_FROM: SyncFrom = SyncFrom::AnyEntity; } impl NetSync for SkillSet { - const SYNC_FROM: SyncFrom = SyncFrom::AllEntities; + const SYNC_FROM: SyncFrom = SyncFrom::AnyEntity; } -// SyncFrom::ClientEntity +// These are synced only from the client's own entity. impl NetSync for Combo { const SYNC_FROM: SyncFrom = SyncFrom::ClientEntity; diff --git a/common/systems/src/character_behavior.rs b/common/systems/src/character_behavior.rs index 96a01739a5..b46b214cb9 100644 --- a/common/systems/src/character_behavior.rs +++ b/common/systems/src/character_behavior.rs @@ -238,7 +238,13 @@ impl Sys { mut state_update: StateUpdate, output_events: &mut OutputEvents, ) { - // TODO: if checking equality is expensive use optional field in StateUpdate + // Here we check for equality with the previous value of these components before + // updating them so that the modification detection will not be + // triggered unnecessarily. This is important for minimizing updates + // sent to the clients (and thus keeping bandwidth usage down). + // + // TODO: if checking equality is expensive for char_state use optional field in + // StateUpdate if *join.char_state != state_update.character { *join.char_state = state_update.character } @@ -249,9 +255,11 @@ impl Sys { *join.energy = state_update.energy; }; + // These components use a different type of change detection. *join.pos = state_update.pos; *join.vel = state_update.vel; *join.ori = state_update.ori; + join.controller .queued_inputs .append(&mut state_update.queued_inputs); diff --git a/common/systems/src/stats.rs b/common/systems/src/stats.rs index 0d58c6a9e6..52e277071b 100644 --- a/common/systems/src/stats.rs +++ b/common/systems/src/stats.rs @@ -93,6 +93,8 @@ impl<'a> System<'a> for Sys { let stat = stats; if let Some(new_max) = health.needs_maximum_update(stat.max_health_modifiers) { + // Only call this if we need to since mutable access will trigger sending an + // update to the client. health.update_internal_integer_maximum(new_max); } @@ -104,6 +106,8 @@ impl<'a> System<'a> for Sys { }; if let Some(new_max) = energy.needs_maximum_update(energy_mods) { + // Only call this if we need to since mutable access will trigger sending an + // update to the client. energy.update_internal_integer_maximum(new_max); } } diff --git a/server/src/sys/entity_sync.rs b/server/src/sys/entity_sync.rs index 293ff1d99b..59046f83d2 100644 --- a/server/src/sys/entity_sync.rs +++ b/server/src/sys/entity_sync.rs @@ -82,6 +82,8 @@ impl<'a> System<'a> for Sys { ) { let tick = tick.0; + // Storages already provided in `TrackedStorages` that we need to use + // for other things besides change detection. let uids = &tracked_storages.uid; let colliders = &tracked_storages.collider; let inventories = &tracked_storages.inventory; @@ -355,7 +357,7 @@ impl<'a> System<'a> for Sys { )); } - // Sync components that just sync to the client's entity + // Sync components that are only synced for the client's own entity. for (entity, client) in (&entities, &clients).join() { let comp_sync_package = trackers.create_sync_from_client_package(&tracked_storages, entity); diff --git a/server/src/sys/sentinel.rs b/server/src/sys/sentinel.rs index e3f48838c9..68f2cf9762 100644 --- a/server/src/sys/sentinel.rs +++ b/server/src/sys/sentinel.rs @@ -37,16 +37,26 @@ impl<'a> System<'a> for Sys { /// Holds state like modified bitsets, modification event readers macro_rules! trackers { + // Every place where we have `$( /* ... */ )*` will be repeated for each synced component. ($($component_name:ident: $component_type:ident,)*) => { #[derive(SystemData)] pub struct TrackedStorages<'a> { + // Uids are tracked to detect created entities that should be synced over the network. + // Additionally we need access to the uids when generating packets to send to the clients. pub uid: ReadStorage<'a, Uid>, $(pub $component_name: ReadStorage<'a, $component_type>,)* + // TODO: these may be used to duplicate items when we attempt to remove + // cloning them. pub _ability_map: ReadExpect<'a, AbilityMap>, pub _msm: ReadExpect<'a, MaterialStatManifest>, } impl TrackedStorages<'_> { + /// Create a package containing all the synced components for this entity. This is + /// used to initialized the entity's representation on the client (e.g. used when a new + /// entity is within the area synced to the client). + /// + /// Note: This is only for components that are synced to the client for all entities. pub fn create_entity_package( &self, entity: EcsEntity, @@ -62,10 +72,10 @@ macro_rules! trackers { // if the number of optional components sent is less than 1/8 of the number of component types then // then the suggested approach would no longer be favorable $( - + // Only add components that are synced from any entity. if matches!( <$component_type as NetSync>::SYNC_FROM, - SyncFrom::AllEntities, + SyncFrom::AnyEntity, ) { self .$component_name @@ -88,6 +98,9 @@ macro_rules! trackers { } } + /// Contains an [`UpdateTracker`] for every synced component (that uses this method of + /// change detection). + /// /// This should be inserted into the ecs as a Resource pub struct UpdateTrackers { pub uid: UpdateTracker, @@ -108,14 +121,18 @@ macro_rules! trackers { // TODO: if we held copies of components for doing diffing, the components that hold that data could be registered here } - /// Update trackers + /// Records updates to components that are provided from the tracked storages as a series of events into bitsets + /// that can later be joined on. fn record_changes(&mut self, comps: &TrackedStorages) { self.uid.record_changes(&comps.uid); $( self.$component_name.record_changes(&comps.$component_name); )* + // Enable for logging of counts of component update events. const LOG_COUNTS: bool = false; + // Plotting counts via tracy. Env var provided to toggle on so there's no need to + // recompile if you are already have a tracy build. let plot_counts = common_base::TRACY_ENABLED && matches!(std::env::var("PLOT_UPDATE_COUNTS").as_deref(), Ok("1")); macro_rules! log_counts { @@ -142,6 +159,10 @@ macro_rules! trackers { $(log_counts!($component_name, concat!(stringify!($component_name), 's'));)* } + /// Create a [`EntitySyncPackage`] and a [`CompSyncPackage`] to provide updates + /// for the set entities specified by the provided filter (e.g. for a region). + /// + /// A deleted entities must be externally constructed and provided here. pub fn create_sync_packages( &self, comps: &TrackedStorages, @@ -155,7 +176,7 @@ macro_rules! trackers { $( if matches!( <$component_type as NetSync>::SYNC_FROM, - SyncFrom::AllEntities, + SyncFrom::AnyEntity, ) { comp_sync_package.add_component_updates( &comps.uid, @@ -170,7 +191,7 @@ macro_rules! trackers { } - /// Create sync package for components that are only synced to the client entity + /// Create sync package for components that are only synced for the client's entity. pub fn create_sync_from_client_package( &self, comps: &TrackedStorages, @@ -208,7 +229,11 @@ macro_rules! trackers { } } +// Import all the component types so they will be available when expanding the +// macro below. use common_net::synced_components::*; +// Pass `trackers!` macro to this "x macro" which will invoke it with a list +// of components. This will declare the types defined in the macro above. common_net::synced_components!(trackers); /// Deleted entities grouped by region