From 0bf8291388f740ed1b854c545eee4a6baa107eef Mon Sep 17 00:00:00 2001 From: mat Date: Thu, 28 Sep 2023 21:57:36 -0500 Subject: [PATCH] check for entity duplication before spawning this fixes behavior where in swarms entities in the world might sometimes have a duplicate that gets spawned and despawned immediately --- azalea-client/src/client.rs | 23 +- .../src/packet_handling/configuration.rs | 3 +- azalea-client/src/packet_handling/game.rs | 139 ++++++++---- azalea-entity/src/plugin/indexing.rs | 208 ++++-------------- azalea-entity/src/plugin/mod.rs | 20 +- azalea-nbt/Cargo.toml | 8 - azalea-world/Cargo.toml | 3 - 7 files changed, 155 insertions(+), 249 deletions(-) diff --git a/azalea-client/src/client.rs b/azalea-client/src/client.rs index cd191e0f1..6438cf06a 100644 --- a/azalea-client/src/client.rs +++ b/azalea-client/src/client.rs @@ -25,7 +25,7 @@ use azalea_buf::McBufWritable; use azalea_chat::FormattedText; use azalea_core::{ResourceLocation, Vec3}; use azalea_entity::{ - indexing::{EntityIdIndex, Loaded}, + indexing::{EntityIdIndex, EntityUuidIndex}, metadata::Health, EntityPlugin, EntityUpdateSet, EyeHeight, LocalEntity, Position, }; @@ -208,8 +208,6 @@ impl Client { resolved_address: &SocketAddr, run_schedule_sender: mpsc::UnboundedSender<()>, ) -> Result<(Self, mpsc::UnboundedReceiver), JoinError> { - let entity = ecs_lock.lock().spawn(account.to_owned()).id(); - let conn = Connection::new(resolved_address).await?; let (mut conn, game_profile) = Self::handshake(conn, account, address).await?; @@ -236,6 +234,22 @@ impl Client { let mut ecs = ecs_lock.lock(); + // check if an entity with our uuid already exists in the ecs and if so then + // just use that + let entity = { + let entity_uuid_index = ecs.resource::(); + if let Some(entity) = entity_uuid_index.get(&game_profile.uuid) { + debug!("Reusing entity {entity:?} for client"); + entity + } else { + let entity = ecs.spawn_empty().id(); + debug!("Created new entity {entity:?} for client"); + // add to the uuid index + let mut entity_uuid_index = ecs.resource_mut::(); + entity_uuid_index.insert(game_profile.uuid, entity); + entity + } + }; // we got the ConfigurationConnection, so the client is now connected :) let client = Client::new( game_profile.clone(), @@ -256,6 +270,7 @@ impl Client { received_registries: ReceivedRegistries::default(), local_player_events: LocalPlayerEvents(tx), game_profile: GameProfileComponent(game_profile), + account: account.to_owned(), }, InConfigurationState, )); @@ -578,6 +593,7 @@ pub struct LocalPlayerBundle { pub received_registries: ReceivedRegistries, pub local_player_events: LocalPlayerEvents, pub game_profile: GameProfileComponent, + pub account: Account, } /// A bundle for the components that are present on a local player that is @@ -603,7 +619,6 @@ pub struct JoinedClientBundle { pub attack: attack::AttackBundle, pub _local_entity: LocalEntity, - pub _loaded: Loaded, } /// A marker component for local players that are currently in the diff --git a/azalea-client/src/packet_handling/configuration.rs b/azalea-client/src/packet_handling/configuration.rs index 6930e739d..df1b10256 100644 --- a/azalea-client/src/packet_handling/configuration.rs +++ b/azalea-client/src/packet_handling/configuration.rs @@ -1,7 +1,7 @@ use std::io::Cursor; use std::sync::Arc; -use azalea_entity::indexing::{EntityIdIndex, Loaded}; +use azalea_entity::indexing::EntityIdIndex; use azalea_protocol::packets::configuration::serverbound_finish_configuration_packet::ServerboundFinishConfigurationPacket; use azalea_protocol::packets::configuration::serverbound_keep_alive_packet::ServerboundKeepAlivePacket; use azalea_protocol::packets::configuration::serverbound_pong_packet::ServerboundPongPacket; @@ -149,7 +149,6 @@ pub fn process_packet_events(ecs: &mut World) { attack: crate::attack::AttackBundle::default(), _local_entity: azalea_entity::LocalEntity, - _loaded: Loaded, }); } ClientboundConfigurationPacket::KeepAlive(p) => { diff --git a/azalea-client/src/packet_handling/game.rs b/azalea-client/src/packet_handling/game.rs index e0a8b017e..670d37576 100644 --- a/azalea-client/src/packet_handling/game.rs +++ b/azalea-client/src/packet_handling/game.rs @@ -194,11 +194,13 @@ pub fn process_packet_events(ecs: &mut World) { &ClientInformation, &ReceivedRegistries, Option<&mut InstanceName>, + Option<&mut LoadedBy>, &mut EntityIdIndex, &mut InstanceHolder, )>, EventWriter, ResMut, + ResMut, EventWriter, )> = SystemState::new(ecs); let ( @@ -206,6 +208,7 @@ pub fn process_packet_events(ecs: &mut World) { mut query, mut instance_loaded_events, mut instance_container, + mut entity_uuid_index, mut send_packet_events, ) = system_state.get_mut(ecs); let ( @@ -213,6 +216,7 @@ pub fn process_packet_events(ecs: &mut World) { client_information, received_registries, instance_name, + loaded_by, mut entity_id_index, mut instance_holder, ) = query.get_mut(player_entity).unwrap(); @@ -277,9 +281,10 @@ pub fn process_packet_events(ecs: &mut World) { ), metadata: PlayerMetadataBundle::default(), }; + let entity_id = MinecraftEntityId(p.player_id); // insert our components into the ecs :) commands.entity(player_entity).insert(( - MinecraftEntityId(p.player_id), + entity_id, LocalGameMode { current: p.common.game_type, previous: p.common.previous_game_type.into(), @@ -289,8 +294,23 @@ pub fn process_packet_events(ecs: &mut World) { player_bundle, )); - // add our own player to our index - entity_id_index.insert(MinecraftEntityId(p.player_id), player_entity); + azalea_entity::indexing::add_entity_to_indexes( + entity_id, + player_entity, + Some(game_profile.uuid), + &mut entity_id_index, + &mut entity_uuid_index, + &mut instance_holder.instance.write(), + ); + + // update or insert loaded_by + if let Some(mut loaded_by) = loaded_by { + loaded_by.insert(player_entity); + } else { + commands + .entity(player_entity) + .insert(LoadedBy(HashSet::from_iter(vec![player_entity]))); + } } // send the client information that we have set @@ -595,10 +615,7 @@ pub fn process_packet_events(ecs: &mut World) { if !this_client_has_chunk { if let Some(shared_chunk) = shared_chunk { - trace!( - "Skipping parsing chunk {:?} because we already know about it", - pos - ); + trace!("Skipping parsing chunk {pos:?} because we already know about it"); partial_world.chunks.set_with_shared_reference( &pos, Some(shared_chunk.clone()), @@ -608,12 +625,7 @@ pub fn process_packet_events(ecs: &mut World) { } } - let heightmaps = p - .chunk_data - .heightmaps - .as_compound() - .and_then(|c| c.get("")) - .and_then(|c| c.as_compound()); + let heightmaps = p.chunk_data.heightmaps.as_compound(); // necessary to make the unwrap_or work let empty_nbt_compound = NbtCompound::default(); let heightmaps = heightmaps.unwrap_or(&empty_nbt_compound); @@ -624,7 +636,7 @@ pub fn process_packet_events(ecs: &mut World) { heightmaps, &mut world.chunks, ) { - error!("Couldn't set chunk data: {}", e); + error!("Couldn't set chunk data: {e}"); } } ClientboundGamePacket::AddEntity(p) => { @@ -634,50 +646,83 @@ pub fn process_packet_events(ecs: &mut World) { let mut system_state: SystemState<( Commands, Query<(&mut EntityIdIndex, Option<&InstanceName>, Option<&TabList>)>, + Query<&mut LoadedBy>, + Query, Res, ResMut, )> = SystemState::new(ecs); - let (mut commands, mut query, instance_container, mut entity_uuid_index) = - system_state.get_mut(ecs); + let ( + mut commands, + mut query, + mut loaded_by_query, + entity_query, + instance_container, + mut entity_uuid_index, + ) = system_state.get_mut(ecs); let (mut entity_id_index, instance_name, tab_list) = query.get_mut(player_entity).unwrap(); - if let Some(instance_name) = instance_name { - let bundle = p.as_entity_bundle((**instance_name).clone()); - let mut spawned = commands.spawn(( - MinecraftEntityId(p.id), - LoadedBy(HashSet::from([player_entity])), - bundle, - )); - entity_id_index.insert(MinecraftEntityId(p.id), spawned.id()); + let entity_id = MinecraftEntityId(p.id); - { - // add it to the indexes immediately so if there's a packet that references - // it immediately after it still works - let instance = instance_container.get(instance_name).unwrap(); - instance - .write() - .entity_by_id - .insert(MinecraftEntityId(p.id), spawned.id()); - entity_uuid_index.insert(p.uuid, spawned.id()); - } + let Some(instance_name) = instance_name else { + warn!("got add player packet but we haven't gotten a login packet yet"); + continue; + }; + + // check if the entity already exists, and if it does then only add to LoadedBy + let instance = instance_container.get(instance_name).unwrap(); + if let Some(&ecs_entity) = instance.read().entity_by_id.get(&entity_id) { + // entity already exists + let Ok(mut loaded_by) = loaded_by_query.get_mut(ecs_entity) else { + // LoadedBy for this entity isn't in the ecs! figure out what went wrong + // and print an error - if let Some(tab_list) = tab_list { - // technically this makes it possible for non-player entities to have - // GameProfileComponents but the server would have to be doing something - // really weird - if let Some(player_info) = tab_list.get(&p.uuid) { - spawned.insert(GameProfileComponent(player_info.profile.clone())); + let entity_in_ecs = entity_query.get(ecs_entity).is_ok(); + + if entity_in_ecs { + error!("LoadedBy for entity {entity_id:?} ({ecs_entity:?}) isn't in the ecs, but the entity is in entity_by_id"); + } else { + error!("Entity {entity_id:?} ({ecs_entity:?}) isn't in the ecs, but the entity is in entity_by_id"); } - } + continue; + }; + loaded_by.insert(player_entity); - // the bundle doesn't include the default entity metadata so we add that - // separately - p.apply_metadata(&mut spawned); - } else { - warn!("got add player packet but we haven't gotten a login packet yet"); + // per-client id index + entity_id_index.insert(entity_id, ecs_entity); + continue; + }; + + // entity doesn't exist in the global index! + + let bundle = p.as_entity_bundle((**instance_name).clone()); + let mut spawned = + commands.spawn((entity_id, LoadedBy(HashSet::from([player_entity])), bundle)); + let ecs_entity = spawned.id(); + + azalea_entity::indexing::add_entity_to_indexes( + entity_id, + ecs_entity, + Some(p.uuid), + &mut entity_id_index, + &mut entity_uuid_index, + &mut instance.write(), + ); + + // add the GameProfileComponent if the uuid is in the tab list + if let Some(tab_list) = tab_list { + // (technically this makes it possible for non-player entities to have + // GameProfileComponents but the server would have to be doing something + // really weird) + if let Some(player_info) = tab_list.get(&p.uuid) { + spawned.insert(GameProfileComponent(player_info.profile.clone())); + } } + // the bundle doesn't include the default entity metadata so we add that + // separately + p.apply_metadata(&mut spawned); + system_state.apply(ecs); } ClientboundGamePacket::SetEntityData(p) => { @@ -901,6 +946,8 @@ pub fn process_packet_events(ecs: &mut World) { ); continue; }; + // the [`remove_despawned_entities_from_indexes`] system will despawn the entity + // if it's not loaded by anything anymore loaded_by.remove(&player_entity); } } diff --git a/azalea-entity/src/plugin/indexing.rs b/azalea-entity/src/plugin/indexing.rs index e3b1fccb6..f9c210a34 100644 --- a/azalea-entity/src/plugin/indexing.rs +++ b/azalea-entity/src/plugin/indexing.rs @@ -1,19 +1,19 @@ //! Stuff related to entity indexes and keeping track of entities in the world. use azalea_core::ChunkPos; -use azalea_world::{InstanceContainer, InstanceName, MinecraftEntityId}; +use azalea_world::{Instance, InstanceContainer, InstanceName, MinecraftEntityId}; use bevy_ecs::{ component::Component, entity::Entity, - query::{Changed, With, Without}, + query::Changed, system::{Commands, Query, Res, ResMut, Resource}, }; -use log::{debug, error, info, warn}; +use log::{debug, warn}; use nohash_hasher::IntMap; use std::{collections::HashMap, fmt::Debug}; use uuid::Uuid; -use crate::{EntityUuid, LastSentPosition, LocalEntity, Position}; +use crate::{EntityUuid, LastSentPosition, Position}; use super::LoadedBy; @@ -50,6 +50,10 @@ impl EntityUuidIndex { pub fn insert(&mut self, uuid: Uuid, entity: Entity) { self.entity_by_uuid.insert(uuid, entity); } + + pub fn remove(&mut self, uuid: &Uuid) -> Option { + self.entity_by_uuid.remove(uuid) + } } impl EntityIdIndex { @@ -76,180 +80,29 @@ impl Debug for EntityUuidIndex { } } -/// A marker component for entities that are in the world and aren't temporary -/// duplicates of other ones. This is meant to be used as a query filter -/// `Added` (since if you do `Added` with another component it might -/// trigger multiple times when in a swarm due to how entities are handled for -/// swarms). -#[derive(Component)] -pub struct Loaded; +// TODO: this should keep track of chunk position changes in a better way +// instead of relying on LastSentPosition -/// Remove new entities that have the same id as an existing entity, and -/// increase the reference counts. +/// Update the chunk position indexes in [`Instance::entities_by_chunk`]. /// -/// This is the reason why spawning entities into the ECS when you get a spawn -/// entity packet is okay. This system will make sure the new entity gets -/// combined into the old one. -#[allow(clippy::type_complexity)] -pub fn deduplicate_entities( - mut commands: Commands, - mut query: Query< - (Entity, &MinecraftEntityId, &InstanceName, Option<&Loaded>), - (Changed, Without), - >, - mut loaded_by_query: Query<&mut LoadedBy>, - mut entity_id_index_query: Query<&mut EntityIdIndex>, - instance_container: Res, -) { - // if this entity already exists, remove it and keep the old one - for (new_entity, id, world_name, loaded) in query.iter_mut() { - let Some(world_lock) = instance_container.get(world_name) else { - error!("Entity was inserted into a world that doesn't exist."); - continue; - }; - let world = world_lock.write(); - let Some(old_entity) = world.entity_by_id.get(id) else { - // not in index yet, so it's good - if loaded.is_none() { - commands.entity(new_entity).insert(Loaded); - } - continue; - }; - if old_entity == &new_entity { - if loaded.is_none() { - commands.entity(new_entity).insert(Loaded); - } - continue; - } - - // this entity already exists!!! remove the one we just added but increase - // the reference count - let new_loaded_by = loaded_by_query - .get(new_entity) - .expect("Entities should always have the LoadedBy component ({new_entity:?} did not)") - .clone(); - - // update the `EntityIdIndex`s of the local players that have this entity loaded - for local_player in new_loaded_by.iter() { - let mut entity_id_index = entity_id_index_query - .get_mut(*local_player) - .expect("Local players should always have the EntityIdIndex component ({local_player:?} did not)"); - entity_id_index.insert(*id, *old_entity); - } - - let old_loaded_by = loaded_by_query.get_mut(*old_entity); - // merge them if possible - if let Ok(mut old_loaded_by) = old_loaded_by { - old_loaded_by.extend(new_loaded_by.iter()); - } - commands.entity(new_entity).despawn(); - info!( - "Entity with id {id:?} / {new_entity:?} already existed in the world, merging it with {old_entity:?}" - ); - continue; - } -} - -// when a local entity is added, if there was already an entity with the same id -// then delete the old entity -#[allow(clippy::type_complexity)] -pub fn deduplicate_local_entities( - mut commands: Commands, - mut query: Query< - (Entity, &MinecraftEntityId, &InstanceName), - (Changed, With), - >, - instance_container: Res, -) { - // if this entity already exists, remove the old one - for (new_entity, id, world_name) in query.iter_mut() { - let Some(world_lock) = instance_container.get(world_name) else { - error!("Entity was inserted into a world that doesn't exist."); - continue; - }; - let world = world_lock.write(); - let Some(old_entity) = world.entity_by_id.get(id) else { - continue; - }; - if old_entity == &new_entity { - // lol - continue; - } - - commands.entity(*old_entity).despawn(); - debug!( - "Added local entity {id:?} / {new_entity:?} but already existed in world as {old_entity:?}, despawning {old_entity:?}" - ); - break; - } -} - -pub fn update_uuid_index( - mut entity_infos: ResMut, - query: Query<(Entity, &EntityUuid, Option<&LocalEntity>), Changed>, -) { - for (entity, &uuid, local) in query.iter() { - // only add it if it doesn't already exist in - // entity_infos.entity_by_uuid - if local.is_none() { - if let Some(old_entity) = entity_infos.entity_by_uuid.get(&uuid) { - debug!( - "Entity with UUID {uuid:?} already existed in the world, not adding to index (old ecs id: {old_entity:?} / new ecs id: {entity:?})" - ); - continue; - } - } - entity_infos.entity_by_uuid.insert(*uuid, entity); - } -} - -/// System to keep the entity_by_id index up-to-date. -pub fn update_entity_by_id_index( - mut query: Query< - ( - Entity, - &MinecraftEntityId, - &InstanceName, - Option<&LocalEntity>, - ), - Changed, - >, - instance_container: Res, -) { - for (entity, id, world_name, local) in query.iter_mut() { - let world_lock = instance_container.get(world_name).unwrap(); - let mut world = world_lock.write(); - if local.is_none() { - if let Some(old_entity) = world.entity_by_id.get(id) { - debug!( - "Entity with ID {id:?} already existed in the world, not adding to index (old ecs id: {old_entity:?} / new ecs id: {entity:?})" - ); - continue; - } - } - world.entity_by_id.insert(*id, entity); - debug!("Added {entity:?} to {world_name:?} with {id:?}."); - } -} - -/// Update the chunk position indexes in [`EntityUuidIndex`]. +/// [`Instance::entities_by_chunk`]: azalea_world::Instance::entities_by_chunk pub fn update_entity_chunk_positions( - mut query: Query<(Entity, &Position, &mut LastSentPosition, &InstanceName), Changed>, + mut query: Query<(Entity, &Position, &LastSentPosition, &InstanceName), Changed>, instance_container: Res, ) { for (entity, pos, last_pos, world_name) in query.iter_mut() { - let world_lock = instance_container.get(world_name).unwrap(); - let mut world = world_lock.write(); + let instance_lock = instance_container.get(world_name).unwrap(); + let mut instance = instance_lock.write(); let old_chunk = ChunkPos::from(*last_pos); let new_chunk = ChunkPos::from(*pos); if old_chunk != new_chunk { // move the entity from the old chunk to the new one - if let Some(entities) = world.entities_by_chunk.get_mut(&old_chunk) { + if let Some(entities) = instance.entities_by_chunk.get_mut(&old_chunk) { entities.remove(&entity); } - world + instance .entities_by_chunk .entry(new_chunk) .or_default() @@ -262,7 +115,7 @@ pub fn update_entity_chunk_positions( #[allow(clippy::type_complexity)] pub fn remove_despawned_entities_from_indexes( mut commands: Commands, - mut entity_infos: ResMut, + mut entity_uuid_index: ResMut, instance_container: Res, query: Query< ( @@ -282,7 +135,7 @@ pub fn remove_despawned_entities_from_indexes( debug!( "Despawned entity {entity:?} because it's in an instance that isn't loaded anymore" ); - if entity_infos.entity_by_uuid.remove(uuid).is_none() { + if entity_uuid_index.entity_by_uuid.remove(uuid).is_none() { warn!( "Tried to remove entity {entity:?} from the uuid index but it was not there." ); @@ -317,7 +170,7 @@ pub fn remove_despawned_entities_from_indexes( debug!("Tried to remove entity {entity:?} from chunk {chunk:?} but the chunk was not found."); } // remove it from the uuid index - if entity_infos.entity_by_uuid.remove(uuid).is_none() { + if entity_uuid_index.entity_by_uuid.remove(uuid).is_none() { warn!("Tried to remove entity {entity:?} from the uuid index but it was not there."); } if instance.entity_by_id.remove(minecraft_id).is_none() { @@ -326,6 +179,25 @@ pub fn remove_despawned_entities_from_indexes( // and now remove the entity from the ecs commands.entity(entity).despawn(); debug!("Despawned entity {entity:?} because it was not loaded by anything."); - continue; + } +} + +pub fn add_entity_to_indexes( + entity_id: MinecraftEntityId, + ecs_entity: Entity, + entity_uuid: Option, + entity_id_index: &mut EntityIdIndex, + entity_uuid_index: &mut EntityUuidIndex, + instance: &mut Instance, +) { + // per-client id index + entity_id_index.insert(entity_id, ecs_entity); + + // per-instance id index + instance.entity_by_id.insert(entity_id, ecs_entity); + + if let Some(uuid) = entity_uuid { + // per-instance uuid index + entity_uuid_index.insert(uuid, ecs_entity); } } diff --git a/azalea-entity/src/plugin/mod.rs b/azalea-entity/src/plugin/mod.rs index 1f5fd3bde..0e24a1a71 100644 --- a/azalea-entity/src/plugin/mod.rs +++ b/azalea-entity/src/plugin/mod.rs @@ -5,7 +5,7 @@ use std::collections::HashSet; use azalea_core::{BlockPos, ChunkPos, Vec3}; use azalea_world::{InstanceContainer, InstanceName, MinecraftEntityId}; -use bevy_app::{App, Plugin, PostUpdate, PreUpdate, Update}; +use bevy_app::{App, Plugin, PreUpdate, Update}; use bevy_ecs::prelude::*; use derive_more::{Deref, DerefMut}; use log::debug; @@ -20,9 +20,6 @@ pub use relative_updates::RelativeEntityUpdate; /// A Bevy [`SystemSet`] for various types of entity updates. #[derive(SystemSet, Debug, Hash, Eq, PartialEq, Clone)] pub enum EntityUpdateSet { - /// Remove ECS entities that refer to an entity that was already in the ECS - /// before. - Deduplicate, /// Create search indexes for entities. Index, /// Remove despawned entities from search indexes. @@ -41,23 +38,10 @@ impl Plugin for EntityPlugin { PreUpdate, indexing::remove_despawned_entities_from_indexes.in_set(EntityUpdateSet::Deindex), ) - .add_systems( - PostUpdate, - ( - indexing::deduplicate_entities, - indexing::deduplicate_local_entities, - ) - .in_set(EntityUpdateSet::Deduplicate), - ) .add_systems( Update, ( - ( - indexing::update_entity_chunk_positions, - indexing::update_uuid_index, - indexing::update_entity_by_id_index, - ) - .in_set(EntityUpdateSet::Index), + (indexing::update_entity_chunk_positions).in_set(EntityUpdateSet::Index), ( relative_updates::debug_detect_updates_received_on_local_entities, debug_new_entity, diff --git a/azalea-nbt/Cargo.toml b/azalea-nbt/Cargo.toml index f10cc6e14..993a1f4f9 100644 --- a/azalea-nbt/Cargo.toml +++ b/azalea-nbt/Cargo.toml @@ -28,14 +28,6 @@ fastnbt = "2.4.4" default = [] serde = ["dep:serde"] -[profile.release] -lto = true -debug = true - -[profile.bench] -lto = true -debug = true - [[bench]] harness = false name = "nbt" diff --git a/azalea-world/Cargo.toml b/azalea-world/Cargo.toml index 1fcb65f86..b71c1854a 100644 --- a/azalea-world/Cargo.toml +++ b/azalea-world/Cargo.toml @@ -27,8 +27,5 @@ parking_lot = "^0.12.1" thiserror = "1.0.48" uuid = "1.4.1" -[profile.release] -lto = true - [dev-dependencies] azalea-client = { path = "../azalea-client" }