mirror of
https://gitlab.com/veloren/veloren.git
synced 2024-08-30 18:12:32 +00:00
When sync_me is false avoid:
* Logging a warning when deleting the entity and it is not in any Region. * Searching every single region for an entity that is in none of them. Also: * Add workaround for bug in specs where deleting any entity clears the components before checking that the generation is correct (surprised that we haven't encounted bugs from this yet). * Properly update `tracked_entities` inside `RegionMap` when deleting an entity. Previously, we just relied on this being updated in `RegionMap::tick` by the absence of the `Pos` component at that index. However, this means if a new entity is created at that index after deletion and before calling `RegionMap::tick`, then this can be interpreted as an entity moving between regions rather than one being deleted and another created. It seems like this could lead to synchronization bugs like not creating the new entity on the client (although I haven't noticed this before, I think maybe we use newly inserted `Uid`s to detect new entities rather than the region system?). I think it could at least lead to sending redundant messages to synchronize the new entity.
This commit is contained in:
parent
cdca700297
commit
ecb27deeae
@ -2257,17 +2257,10 @@ impl Client {
|
|||||||
.apply_entity_sync_package(entity_sync_package, uid);
|
.apply_entity_sync_package(entity_sync_package, uid);
|
||||||
},
|
},
|
||||||
ServerGeneral::CompSync(comp_sync_package, force_counter) => {
|
ServerGeneral::CompSync(comp_sync_package, force_counter) => {
|
||||||
// TODO: Test with and without sync_me = true commented out to see if results are
|
|
||||||
// as expected. (client shouldn't normally receive updates if client physics is
|
|
||||||
// enabled)
|
|
||||||
let b = self.position();
|
|
||||||
self.force_update_counter = force_counter;
|
self.force_update_counter = force_counter;
|
||||||
self.state
|
self.state
|
||||||
.ecs_mut()
|
.ecs_mut()
|
||||||
.apply_comp_sync_package(comp_sync_package);
|
.apply_comp_sync_package(comp_sync_package);
|
||||||
if b != self.position() {
|
|
||||||
println!("{b:?}");
|
|
||||||
}
|
|
||||||
},
|
},
|
||||||
ServerGeneral::CreateEntity(entity_package) => {
|
ServerGeneral::CreateEntity(entity_package) => {
|
||||||
self.state.ecs_mut().apply_entity_package(entity_package);
|
self.state.ecs_mut().apply_entity_package(entity_package);
|
||||||
|
@ -209,6 +209,13 @@ impl RegionMap {
|
|||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
|
/// Must be called immediately after succesfully deleting an entity from the
|
||||||
|
/// ecs (i.e. when deleting the entity did not generate a WrongGeneration
|
||||||
|
/// error).
|
||||||
|
pub fn entity_deleted(&mut self, entity: specs::Entity) {
|
||||||
|
self.tracked_entities.remove(entity.id());
|
||||||
|
}
|
||||||
|
|
||||||
fn add_entity(&mut self, id: u32, pos: Vec3<i32>, from: Option<Vec2<i32>>) {
|
fn add_entity(&mut self, id: u32, pos: Vec3<i32>, from: Option<Vec2<i32>>) {
|
||||||
let key = Self::pos_key(pos);
|
let key = Self::pos_key(pos);
|
||||||
if let Some(region) = self.regions.get_mut(&key) {
|
if let Some(region) = self.regions.get_mut(&key) {
|
||||||
@ -247,6 +254,8 @@ impl RegionMap {
|
|||||||
}
|
}
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
} else if !self.tracked_entities.contains(id) {
|
||||||
|
return None;
|
||||||
} else {
|
} else {
|
||||||
// Check neighbors
|
// Check neighbors
|
||||||
for o in &NEIGHBOR_OFFSETS {
|
for o in &NEIGHBOR_OFFSETS {
|
||||||
@ -269,26 +278,9 @@ impl RegionMap {
|
|||||||
None
|
None
|
||||||
}
|
}
|
||||||
|
|
||||||
/// Checks if this entity is located in the `RegionMap` using the provided
|
/// Checks if this entity is located in the `RegionMap`.
|
||||||
/// position to limit which regions are checked.
|
pub fn in_region_map(&self, entity: specs::Entity) -> bool {
|
||||||
///
|
self.tracked_entities.contains(entity.id())
|
||||||
/// May produce false negatives (e.g. if for some reason the entity is not
|
|
||||||
/// in a region near the provided position).
|
|
||||||
pub fn in_region_map_relaxed(&self, entity: specs::Entity, pos: Vec3<f32>) -> bool {
|
|
||||||
let id = entity.id();
|
|
||||||
let pos = pos.map(|e| e as i32);
|
|
||||||
|
|
||||||
// Compute key for most likely region
|
|
||||||
let key = Self::pos_key(pos);
|
|
||||||
if let Some(region) = self.regions.get(&key) && region.entities().contains(id) { return true }
|
|
||||||
|
|
||||||
// Get the base of the four nearest regions.
|
|
||||||
let quad_base = Self::pos_key(pos - (REGION_SIZE / 2) as i32);
|
|
||||||
[(0, 0), (0, 1), (1, 0), (1, 1)]
|
|
||||||
.iter()
|
|
||||||
.map(|&offset| Vec2::<i32>::from(offset) + quad_base)
|
|
||||||
// skip key we already checked
|
|
||||||
.any(|k| k != key && self.regions.get(&key).is_some_and(|r| r.entities().contains(id)))
|
|
||||||
}
|
}
|
||||||
|
|
||||||
fn key_index(&self, key: Vec2<i32>) -> Option<usize> {
|
fn key_index(&self, key: Vec2<i32>) -> Option<usize> {
|
||||||
|
@ -127,15 +127,16 @@ pub fn handle_exit_ingame(server: &mut Server, entity: EcsEntity, skip_persisten
|
|||||||
error!("handle_exit_ingame called with entity that is missing expected components");
|
error!("handle_exit_ingame called with entity that is missing expected components");
|
||||||
}
|
}
|
||||||
|
|
||||||
let maybe_character = state
|
let (maybe_character, sync_me) = state
|
||||||
.read_storage::<Presence>()
|
.read_storage::<Presence>()
|
||||||
.get(entity)
|
.get(entity)
|
||||||
.and_then(|p| p.kind.character_id());
|
.map(|p| (p.kind.character_id(), p.sync_me))
|
||||||
|
.unzip();
|
||||||
let maybe_rtsim = state.read_component_copied::<common::rtsim::RtSimEntity>(entity);
|
let maybe_rtsim = state.read_component_copied::<common::rtsim::RtSimEntity>(entity);
|
||||||
state.mut_resource::<IdMaps>().remove_entity(
|
state.mut_resource::<IdMaps>().remove_entity(
|
||||||
Some(entity),
|
Some(entity),
|
||||||
None, // Uid re-mapped, we don't want to remove the mapping
|
None, // Uid re-mapped, we don't want to remove the mapping
|
||||||
maybe_character,
|
maybe_character.flatten(),
|
||||||
maybe_rtsim,
|
maybe_rtsim,
|
||||||
);
|
);
|
||||||
|
|
||||||
@ -143,7 +144,9 @@ pub fn handle_exit_ingame(server: &mut Server, entity: EcsEntity, skip_persisten
|
|||||||
// Uid to a new entity (and e.g. don't want it to be unmapped).
|
// Uid to a new entity (and e.g. don't want it to be unmapped).
|
||||||
//
|
//
|
||||||
// Delete old entity
|
// Delete old entity
|
||||||
if let Err(e) = crate::state_ext::delete_entity_common(state, entity, maybe_uid) {
|
if let Err(e) =
|
||||||
|
crate::state_ext::delete_entity_common(state, entity, maybe_uid, sync_me.unwrap_or(true))
|
||||||
|
{
|
||||||
error!(
|
error!(
|
||||||
?e,
|
?e,
|
||||||
?entity,
|
?entity,
|
||||||
|
@ -709,7 +709,7 @@ impl StateExt for State {
|
|||||||
self.ecs()
|
self.ecs()
|
||||||
.write_resource::<IdMaps>()
|
.write_resource::<IdMaps>()
|
||||||
.add_character(id, entity);
|
.add_character(id, entity);
|
||||||
//presence.sync_me = true;
|
presence.sync_me = true;
|
||||||
Ok(())
|
Ok(())
|
||||||
} else {
|
} else {
|
||||||
Err("PresenceKind is not LoadingCharacter")
|
Err("PresenceKind is not LoadingCharacter")
|
||||||
@ -1196,20 +1196,21 @@ impl StateExt for State {
|
|||||||
// NOTE: We expect that these 3 components are never removed from an entity (nor
|
// NOTE: We expect that these 3 components are never removed from an entity (nor
|
||||||
// mutated) (at least not without updating the relevant mappings)!
|
// mutated) (at least not without updating the relevant mappings)!
|
||||||
let maybe_uid = self.read_component_copied::<Uid>(entity);
|
let maybe_uid = self.read_component_copied::<Uid>(entity);
|
||||||
let maybe_character = self
|
let (maybe_character, sync_me) = self
|
||||||
.read_storage::<Presence>()
|
.read_storage::<Presence>()
|
||||||
.get(entity)
|
.get(entity)
|
||||||
.and_then(|p| p.kind.character_id());
|
.map(|p| (p.kind.character_id(), p.sync_me))
|
||||||
|
.unzip();
|
||||||
let maybe_rtsim = self.read_component_copied::<RtSimEntity>(entity);
|
let maybe_rtsim = self.read_component_copied::<RtSimEntity>(entity);
|
||||||
|
|
||||||
self.mut_resource::<IdMaps>().remove_entity(
|
self.mut_resource::<IdMaps>().remove_entity(
|
||||||
Some(entity),
|
Some(entity),
|
||||||
maybe_uid,
|
maybe_uid,
|
||||||
maybe_character,
|
maybe_character.flatten(),
|
||||||
maybe_rtsim,
|
maybe_rtsim,
|
||||||
);
|
);
|
||||||
|
|
||||||
delete_entity_common(self, entity, maybe_uid)
|
delete_entity_common(self, entity, maybe_uid, sync_me.unwrap_or(true))
|
||||||
}
|
}
|
||||||
|
|
||||||
fn entity_as_actor(&self, entity: EcsEntity) -> Option<Actor> {
|
fn entity_as_actor(&self, entity: EcsEntity) -> Option<Actor> {
|
||||||
@ -1247,6 +1248,7 @@ pub(crate) fn delete_entity_common(
|
|||||||
state: &mut State,
|
state: &mut State,
|
||||||
entity: EcsEntity,
|
entity: EcsEntity,
|
||||||
maybe_uid: Option<Uid>,
|
maybe_uid: Option<Uid>,
|
||||||
|
sync_me: bool,
|
||||||
) -> Result<(), specs::error::WrongGeneration> {
|
) -> Result<(), specs::error::WrongGeneration> {
|
||||||
if maybe_uid.is_none() {
|
if maybe_uid.is_none() {
|
||||||
// For now we expect all entities have a Uid component.
|
// For now we expect all entities have a Uid component.
|
||||||
@ -1254,8 +1256,24 @@ pub(crate) fn delete_entity_common(
|
|||||||
}
|
}
|
||||||
let maybe_pos = state.read_component_copied::<comp::Pos>(entity);
|
let maybe_pos = state.read_component_copied::<comp::Pos>(entity);
|
||||||
|
|
||||||
let res = state.ecs_mut().delete_entity(entity);
|
// TODO: workaround for https://github.com/amethyst/specs/pull/766
|
||||||
|
let actual_gen = state.ecs().entities().entity(entity.id()).gen();
|
||||||
|
let res = if actual_gen == entity.gen() {
|
||||||
|
state.ecs_mut().delete_entity(entity)
|
||||||
|
} else {
|
||||||
|
Err(specs::error::WrongGeneration {
|
||||||
|
action: "delete",
|
||||||
|
actual_gen,
|
||||||
|
entity,
|
||||||
|
})
|
||||||
|
};
|
||||||
|
|
||||||
if res.is_ok() {
|
if res.is_ok() {
|
||||||
|
let region_map = state.mut_resource::<common::region::RegionMap>();
|
||||||
|
let uid_pos_region_key = maybe_uid
|
||||||
|
.zip(maybe_pos)
|
||||||
|
.map(|(uid, pos)| (uid, pos, region_map.find_region(entity, pos.0)));
|
||||||
|
region_map.entity_deleted(entity);
|
||||||
// Note: Adding the `Uid` to the deleted list when exiting "in-game" relies on
|
// Note: Adding the `Uid` to the deleted list when exiting "in-game" relies on
|
||||||
// the client not being able to immediately re-enter the game in the
|
// the client not being able to immediately re-enter the game in the
|
||||||
// same tick (since we could then mix up the ordering of things and
|
// same tick (since we could then mix up the ordering of things and
|
||||||
@ -1263,16 +1281,14 @@ pub(crate) fn delete_entity_common(
|
|||||||
//
|
//
|
||||||
// The client will ignore requests to delete its own entity that are triggered
|
// The client will ignore requests to delete its own entity that are triggered
|
||||||
// by this.
|
// by this.
|
||||||
if let Some((uid, pos)) = maybe_uid.zip(maybe_pos) {
|
if let Some((uid, pos, region_key)) = uid_pos_region_key {
|
||||||
let region_key = state
|
|
||||||
.ecs()
|
|
||||||
.read_resource::<common::region::RegionMap>()
|
|
||||||
.find_region(entity, pos.0);
|
|
||||||
if let Some(region_key) = region_key {
|
if let Some(region_key) = region_key {
|
||||||
state
|
state
|
||||||
.mut_resource::<DeletedEntities>()
|
.mut_resource::<DeletedEntities>()
|
||||||
.record_deleted_entity(uid, region_key);
|
.record_deleted_entity(uid, region_key);
|
||||||
} else {
|
// If there is a position and sync_me is true, but the entity is not
|
||||||
|
// in a region, something might be wrong.
|
||||||
|
} else if sync_me {
|
||||||
// Don't panic if the entity wasn't found in a region, maybe it was just created
|
// Don't panic if the entity wasn't found in a region, maybe it was just created
|
||||||
// and then deleted before the region manager had a chance to assign it a region
|
// and then deleted before the region manager had a chance to assign it a region
|
||||||
warn!(
|
warn!(
|
||||||
|
@ -325,8 +325,7 @@ impl<'a> System<'a> for Sys {
|
|||||||
// Include additional components for clients that aren't in a region (e.g. due
|
// Include additional components for clients that aren't in a region (e.g. due
|
||||||
// to having no position or have sync_me as `false`) since those
|
// to having no position or have sync_me as `false`) since those
|
||||||
// won't be synced above.
|
// won't be synced above.
|
||||||
let include_all_comps =
|
let include_all_comps = region_map.in_region_map(entity);
|
||||||
!maybe_pos.is_some_and(|pos| region_map.in_region_map_relaxed(entity, pos.0));
|
|
||||||
|
|
||||||
let mut comp_sync_package = trackers.create_sync_from_client_package(
|
let mut comp_sync_package = trackers.create_sync_from_client_package(
|
||||||
&tracked_storages,
|
&tracked_storages,
|
||||||
|
Loading…
Reference in New Issue
Block a user