From e480f322653b69e32b90e735e4703e0b827aa981 Mon Sep 17 00:00:00 2001 From: TheDarkSword Date: Sat, 11 Jul 2026 18:59:37 +0200 Subject: [PATCH] fix(pumpkin): stop entities from duplicating on chunk reload (#2342) * fix(pumpkin): stop entities from duplicating on chunk reload Entities lived in two places at once: the live World::entities list and the serialized NBT in the entity chunk's data. On load the saved NBT was turned into live entities but never cleared, and on unload each live entity was appended back onto that still-populated list - so the persisted entity count doubled every load/unload (reconnect) cycle. Freshly spawned entities hit the same trap: add_entity_silent pushed their NBT into the chunk immediately, so they were both live and serialized, doubling on the first unload too. Make the live entity the single source of truth, matching vanilla: - on load, take (clear) the chunk's serialized entities as they become live, and restore their persisted UUID so they keep their identity; - a second watcher of an already-loaded chunk is sent spawn packets built from the live entities, not the stale NBT; - entities are serialized fresh, from their current live state, only when their chunk unloads (save_entity); - add_entity_silent no longer serializes on spawn. Because the live entity is serialized fresh on unload, any change made to it while loaded (health, effects, ...) is persisted automatically, without having to be written back to the chunk data by hand. * Move UUID int-array NBT helpers into pumpkin-nbt Review feedback: the UUID read helper doesn't belong in world/mod.rs. NbtCompound now has put_uuid/get_uuid for the vanilla 4-int-array layout, used by both Entity::write_nbt and the entity chunk loader. Serialized bytes are unchanged. --- Cargo.lock | 1 + pumpkin-nbt/Cargo.toml | 1 + pumpkin-nbt/src/compound.rs | 51 ++++++++++ pumpkin/src/entity/mod.rs | 15 +-- pumpkin/src/world/mod.rs | 193 ++++++++++++++---------------------- 5 files changed, 128 insertions(+), 133 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index a824df029..c6c7ef867 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -2775,6 +2775,7 @@ dependencies = [ "tempfile", "thiserror 2.0.18", "tracing", + "uuid", ] [[package]] diff --git a/pumpkin-nbt/Cargo.toml b/pumpkin-nbt/Cargo.toml index 592357132..636bfeb37 100644 --- a/pumpkin-nbt/Cargo.toml +++ b/pumpkin-nbt/Cargo.toml @@ -10,6 +10,7 @@ pumpkin-codecs.workspace = true serde.workspace = true thiserror.workspace = true bytes.workspace = true +uuid.workspace = true cesu8.workspace = true flate2.workspace = true diff --git a/pumpkin-nbt/src/compound.rs b/pumpkin-nbt/src/compound.rs index eb30addcb..67c2926d0 100644 --- a/pumpkin-nbt/src/compound.rs +++ b/pumpkin-nbt/src/compound.rs @@ -1,6 +1,7 @@ use serde::{Deserialize, Serialize}; use std::collections::HashMap; use std::fmt::{Display, Formatter}; +use uuid::Uuid; use crate::deserializer::NbtReadHelper; use crate::serializer::NbtWriteHelper; @@ -146,6 +147,21 @@ impl NbtCompound { self.put(name, NbtTag::Compound(value)); } + /// Stores a UUID as a 4-element int array, most significant bits first + /// (the vanilla `UUID` layout). + pub fn put_uuid(&mut self, name: &str, value: Uuid) { + let value = value.as_u128(); + self.put( + name, + NbtTag::IntArray(vec![ + (value >> 96) as i32, + ((value >> 64) & 0xFFFF_FFFF) as i32, + ((value >> 32) & 0xFFFF_FFFF) as i32, + (value & 0xFFFF_FFFF) as i32, + ]), + ); + } + #[must_use] pub fn get_byte(&self, name: &str) -> Option { self.get(name).and_then(super::tag::NbtTag::extract_byte) @@ -217,6 +233,21 @@ impl NbtCompound { pub fn get_long_array(&self, name: &str) -> Option<&[i64]> { self.get(name).and_then(|tag| tag.extract_long_array()) } + + /// Reads a UUID stored as a 4-element int array, most significant bits + /// first (the vanilla `UUID` layout written by [`Self::put_uuid`]). + #[must_use] + pub fn get_uuid(&self, name: &str) -> Option { + let &[a, b, c, d] = self.get_int_array(name)? else { + return None; + }; + Some(Uuid::from_u128( + ((a as u32 as u128) << 96) + | ((b as u32 as u128) << 64) + | ((c as u32 as u128) << 32) + | (d as u32 as u128), + )) + } } impl From for NbtCompound { @@ -388,3 +419,23 @@ impl Display for NbtTag { } } } + +#[cfg(test)] +mod tests { + use super::NbtCompound; + use uuid::Uuid; + + #[test] + fn uuid_int_array_round_trip() { + let original = Uuid::from_u128(0x0123_4567_89ab_cdef_fedc_ba98_7654_3210); + let mut nbt = NbtCompound::new(); + nbt.put_uuid("UUID", original); + assert_eq!(nbt.get_uuid("UUID"), Some(original)); + + // Missing or malformed entries fall back to None. + assert_eq!(nbt.get_uuid("missing"), None); + let mut short = NbtCompound::new(); + short.put("UUID", crate::tag::NbtTag::IntArray(vec![1, 2, 3])); + assert_eq!(short.get_uuid("UUID"), None); + } +} diff --git a/pumpkin/src/entity/mod.rs b/pumpkin/src/entity/mod.rs index c1876ed9f..d7d38d8d0 100644 --- a/pumpkin/src/entity/mod.rs +++ b/pumpkin/src/entity/mod.rs @@ -785,8 +785,6 @@ pub struct Entity { /// The age of the entity in ticks. Negative values indicate a baby. pub age: AtomicI32, - pub first_loaded_chunk_position: AtomicCell>>, - pub current_biome: ArcSwap<&'static Biome>, pub last_biome_update_pos: AtomicCell, @@ -900,7 +898,6 @@ impl Entity { pitch: AtomicCell::new(0.0), velocity: AtomicCell::new(Vector3::new(0.0, 0.0, 0.0)), pose: AtomicCell::new(EntityPose::Standing), - first_loaded_chunk_position: AtomicCell::new(None), bounding_box: AtomicCell::new(BoundingBox::new_from_pos( position.x, position.y, @@ -3265,16 +3262,7 @@ impl NBTStorage for Entity { "id", format!("minecraft:{}", self.entity_type.resource_name), ); - let uuid = self.entity_uuid.as_u128(); - nbt.put( - "UUID", - NbtTag::IntArray(vec![ - (uuid >> 96) as i32, - ((uuid >> 64) & 0xFFFF_FFFF) as i32, - ((uuid >> 32) & 0xFFFF_FFFF) as i32, - (uuid & 0xFFFF_FFFF) as i32, - ]), - ); + nbt.put_uuid("UUID", self.entity_uuid); nbt.put( "Pos", NbtTag::List(vec![ @@ -3324,7 +3312,6 @@ impl NBTStorage for Entity { let pos = Vector3::new(x, y, z); self.set_pos(pos); self.last_sent_pos.store(pos); - self.first_loaded_chunk_position.store(Some(pos.to_i32())); let velocity = nbt.get_list("Motion").unwrap(); let x = velocity[0].extract_double().unwrap_or(0.0); let y = velocity[1].extract_double().unwrap_or(0.0); diff --git a/pumpkin/src/world/mod.rs b/pumpkin/src/world/mod.rs index 709ed100f..d4b4fc7bb 100644 --- a/pumpkin/src/world/mod.rs +++ b/pumpkin/src/world/mod.rs @@ -374,32 +374,17 @@ impl World { self.level.shutdown().await; } + /// Serializes a live entity into its current chunk's entity data. The live + /// entity list is the source of truth while a chunk is loaded (its saved NBT + /// is consumed on load), so this simply appends the entity to the chunk it is + /// currently in; the chunk is rewritten from scratch every unload cycle, so + /// there is nothing stale to deduplicate. async fn save_entity(&self, entity: &Arc) { - // First lets see if the entity was saved on an other chunk, and if the current chunk does not match we remove it - // Otherwise we just update the nbt data - let base_entity = entity.get_entity(); - let current_chunk_coordinate = base_entity.block_pos.load().chunk_position(); + let current_chunk = entity.get_entity().block_pos.load().chunk_position(); let mut nbt = NbtCompound::new(); entity.write_nbt(&mut nbt).await; - if let Some(old_chunk) = base_entity.first_loaded_chunk_position.load() { - let old_chunk = old_chunk.to_vec2_i32(); - - let chunk = self.level.get_entity_chunk(old_chunk).await; - chunk.mark_dirty(true); - let mut data = chunk.data.lock().await; - if old_chunk == current_chunk_coordinate { - data.push(nbt); - return; - } - - // The chunk has changed, lets remove the entity from the old chunk - // TODO? - data.clear(); - } - // We did not continue, so lets save data in a new chunk - let chunk = self.level.get_entity_chunk(current_chunk_coordinate).await; - let mut data = chunk.data.lock().await; - data.push(nbt); + let chunk = self.level.get_entity_chunk(current_chunk).await; + chunk.data.lock().await.push(nbt); chunk.mark_dirty(true); } @@ -3546,7 +3531,6 @@ impl World { // NOTE: This function doesn't actually await on anything, it just spawns two tokio tasks /// IMPORTANT: Chunks have to be non-empty - #[expect(clippy::too_many_lines)] fn spawn_world_entity_chunks( self: &Arc, player: Arc, @@ -3591,104 +3575,80 @@ impl World { let position = Vector2::new(chunk.x, chunk.z); if !level.is_chunk_watched(&position) { + // No longer watched: don't make its entities live. Leave the + // serialized data untouched so the normal unload path persists + // it as-is (nothing went live, so there is nothing to save). trace!( - "Received chunk {:?}, but it is no longer watched... cleaning", + "Received entity chunk {:?}, but it is no longer watched; leaving it for the unload path", &position ); - - if first_load { - for entity_nbt in chunk.data.lock().await.iter() { - let Some(id) = entity_nbt.get_string("id") else { - continue; - }; - let Some(entity_type) = - EntityType::from_name(id.strip_prefix("minecraft:").unwrap_or(id)) - else { - continue; - }; - - let entity = from_type( - entity_type, - Vector3::new(0.0, 0.0, 0.0), - &world, - Uuid::new_v4(), - ); - entity.read_nbt_non_mut(entity_nbt).await; - let base_entity = entity.get_entity(); - - let mut nbt = NbtCompound::new(); - entity.write_nbt(&mut nbt).await; - - if let Some(old_chunk) = base_entity.first_loaded_chunk_position.load() - { - let old_chunk = old_chunk.to_vec2_i32(); - let chunk = world.level.get_entity_chunk(old_chunk).await; - chunk.mark_dirty(true); - let current_chunk_coordinate = - base_entity.block_pos.load().chunk_position(); - - let mut data = chunk.data.lock().await; - if old_chunk == current_chunk_coordinate { - data.push(nbt); - continue; - } - - // The chunk has changed, lets remove the entity from the old chunk - data.clear(); - } - } - } continue 'main; } - // Add all new Entities to the world - let mut entities_to_add: Vec> = Vec::new(); - for entity_nbt in chunk.data.lock().await.iter() { - let Some(id) = entity_nbt.get_string("id") else { - debug!("Entity has no ID"); - continue; - }; - let Some(entity_type) = - EntityType::from_name(id.strip_prefix("minecraft:").unwrap_or(id)) - else { - warn!("Entity has no valid Entity Type {id}"); - continue; - }; + if first_load { + // First watcher: consume the serialized entities and make them + // live. The live entity list becomes the single source of + // truth, so the chunk's NBT is taken (cleared) to avoid keeping + // a duplicate copy that would be re-appended on the next unload + // and doubled on every reload. + let entity_nbts = std::mem::take(&mut *chunk.data.lock().await); + let mut entities_to_add: Vec> = + Vec::with_capacity(entity_nbts.len()); + for entity_nbt in &entity_nbts { + let Some(id) = entity_nbt.get_string("id") else { + debug!("Entity has no ID"); + continue; + }; + let Some(entity_type) = + EntityType::from_name(id.strip_prefix("minecraft:").unwrap_or(id)) + else { + warn!("Entity has no valid Entity Type {id}"); + continue; + }; - // Pos is zero since it will read from nbt - let entity = from_type( - entity_type, - Vector3::new(0.0, 0.0, 0.0), - &world, - Uuid::new_v4(), - ); - entity.read_nbt_non_mut(entity_nbt).await; + // Keep the persisted UUID so the entity keeps its identity + // across reloads (matching vanilla); only fall back to a + // fresh one if it is missing/corrupt. + let uuid = entity_nbt.get_uuid("UUID").unwrap_or_else(Uuid::new_v4); + // Pos is zero since it will be read from nbt. + let entity = + from_type(entity_type, Vector3::new(0.0, 0.0, 0.0), &world, uuid); + entity.read_nbt_non_mut(entity_nbt).await; + entity.init_data_tracker().await; - entity.init_data_tracker().await; + let base_entity = entity.get_entity(); + // Clear velocity so the client does not replay the drop + // animation; residual velocity from the original drop is + // stale data. + base_entity.velocity.store(Vector3::default()); - let base_entity = entity.get_entity(); - - // Clear velocity so the client does not replay the drop animation. - // Items at rest on the ground should have zero velocity; any - // residual velocity from the original drop is stale data. - base_entity.velocity.store(Vector3::default()); - - player - .client - .enqueue_packet(&base_entity.create_spawn_packet()) - .await; - - if first_load { + player + .client + .enqueue_packet(&base_entity.create_spawn_packet()) + .await; entities_to_add.push(entity); } - } - if first_load && !entities_to_add.is_empty() { - world.entities.rcu(|current_entities| { - let mut new_entities = (**current_entities).clone(); - new_entities.extend(entities_to_add.iter().cloned()); - new_entities - }); + if !entities_to_add.is_empty() { + world.entities.rcu(|current_entities| { + let mut new_entities = (**current_entities).clone(); + new_entities.extend(entities_to_add.iter().cloned()); + new_entities + }); + } + } else { + // The chunk's entities are already live (another watcher loaded + // them). Just send this player the spawn packets for the live + // entities currently in this chunk. + for entity in world.entities.load().iter() { + let base_entity = entity.get_entity(); + if base_entity.chunk_pos.load() == position { + player + .client + .enqueue_packet(&base_entity.create_spawn_packet()) + .await; + } + } } } @@ -4143,6 +4103,7 @@ impl World { } } + #[allow(clippy::unused_async)] pub async fn add_entity_silent(&self, entity: Arc) { let base_entity = entity.get_entity(); @@ -4158,17 +4119,11 @@ impl World { return; } - let block_pos = base_entity.block_pos.load(); - let chunk_coordinate = block_pos.chunk_position(); - let mut nbt = NbtCompound::new(); - entity.write_nbt(&mut nbt).await; - + // The entity stays live-only: it is written to its chunk's saved data on + // unload (see `save_entity`), never at spawn, so it can't be both live and + // serialized at once (which would double it on the next reload). self.spawn_state.load().add_entity(self, entity.as_ref()); - let chunk = self.level.get_entity_chunk(chunk_coordinate).await; - chunk.data.lock().await.push(nbt); - chunk.mark_dirty(true); - self.entities.rcu(|current_entities| { let mut new_entities = (**current_entities).clone(); new_entities.push(entity.clone());