Rename SyncFrom::AllEntities to SyncFrom::AnyEntity for more clarity, add more comments for component syncing code, address MR comment

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"))]

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<T> 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<PhantomData<T>> for EcsCompPhantom
+        // for each variant EcsCompPhantom::T(PhantomData<T>).
         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);

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::UpdateFrom> = 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,
 }

common/net/src/sync/packet.rs

@@ -184,6 +184,8 @@ impl<P: CompPacket> CompSyncPackage<P> {
         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<C>,

common/net/src/sync/track.rs

@@ -139,17 +139,17 @@ impl<C: Component + Clone + Send + Sync> UpdateTracker<C> {
         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) {
+        if self.modified.contains(id) {
-            storage
-                .get(entity)
-                .map(|comp| CompUpdateKind::Inserted(P::from(comp.clone())))
-        } else 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::<C>)))
         } else {

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<Mount>;
             pub type IsRider = Is<Rider>;
         }
 
+        // 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;

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);

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);
             }
         }

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);

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<Uid>,
@@ -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