From eebc92a7d4ca818bbbb1f032c011bcff855cb625 Mon Sep 17 00:00:00 2001 From: Illiux Date: Mon, 1 May 2023 08:49:27 -0700 Subject: [PATCH] Make scene handling of entity references robust (#7335) # Objective - Handle dangling entity references inside scenes - Handle references to entities with generation > 0 inside scenes - Fix a latent bug in `Parent`'s `MapEntities` implementation, which would, if the parent was outside the scene, cause the scene to be loaded into the new world with a parent reference potentially pointing to some random entity in that new world. - Fixes #4793 and addresses #7235 ## Solution - DynamicScenes now identify entities with a `Entity` instead of a u32, therefore including generation - `World` exposes a new `reserve_generations` function that despawns an entity and advances its generation by some extra amount. - `MapEntities` implementations have a new `get_or_reserve` function available that will always return an `Entity`, establishing a new mapping to a dead entity when the entity they are called with is not in the `EntityMap`. Subsequent calls with that same `Entity` will return the same newly created dead entity reference, preserving equality semantics. - As a result, after loading a scene containing references to dead entities (or entities otherwise outside the scene), those references will all point to different generations on a single entity id in the new world. --- ## Changelog ### Changed - In serialized scenes, entities are now identified by a u64 instead of a u32. - In serialized scenes, components with entity references now have those references serialize as u64s instead of structs. ### Fixed - Scenes containing components with entity references will now deserialize and add to a world reliably. ## Migration Guide - `MapEntities` implementations must change from a `&EntityMap` parameter to a `&mut EntityMapper` parameter and can no longer return a `Result`. Finally, they should switch from calling `EntityMap::get` to calling `EntityMapper::get_or_reserve`. --------- Co-authored-by: Nicola Papale --- crates/bevy_ecs/Cargo.toml | 2 +- crates/bevy_ecs/src/entity/map_entities.rs | 185 ++++++++++++++---- crates/bevy_ecs/src/entity/mod.rs | 65 +++++- crates/bevy_ecs/src/reflect.rs | 37 ++-- .../bevy_hierarchy/src/components/children.rs | 8 +- .../bevy_hierarchy/src/components/parent.rs | 11 +- crates/bevy_render/src/mesh/mesh/skinning.rs | 8 +- crates/bevy_scene/src/dynamic_scene.rs | 15 +- .../bevy_scene/src/dynamic_scene_builder.rs | 34 ++-- crates/bevy_scene/src/scene.rs | 4 +- crates/bevy_scene/src/serde.rs | 103 ++++++++-- crates/bevy_window/src/window.rs | 13 +- 12 files changed, 360 insertions(+), 125 deletions(-) diff --git a/crates/bevy_ecs/Cargo.toml b/crates/bevy_ecs/Cargo.toml index cfcea53b3644c..c19073b2c97ae 100644 --- a/crates/bevy_ecs/Cargo.toml +++ b/crates/bevy_ecs/Cargo.toml @@ -26,7 +26,7 @@ thread_local = "1.1.4" fixedbitset = "0.4.2" rustc-hash = "1.1" downcast-rs = "1.2" -serde = { version = "1", features = ["derive"] } +serde = "1" thiserror = "1.0" [dev-dependencies] diff --git a/crates/bevy_ecs/src/entity/map_entities.rs b/crates/bevy_ecs/src/entity/map_entities.rs index 2a8dbfbc468c8..739635f5325ab 100644 --- a/crates/bevy_ecs/src/entity/map_entities.rs +++ b/crates/bevy_ecs/src/entity/map_entities.rs @@ -1,24 +1,5 @@ -use crate::entity::Entity; +use crate::{entity::Entity, world::World}; use bevy_utils::{Entry, HashMap}; -use std::fmt; - -/// The errors that might be returned while using [`MapEntities::map_entities`]. -#[derive(Debug)] -pub enum MapEntitiesError { - EntityNotFound(Entity), -} - -impl std::error::Error for MapEntitiesError {} - -impl fmt::Display for MapEntitiesError { - fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { - match self { - MapEntitiesError::EntityNotFound(_) => { - write!(f, "the given entity does not exist in the map") - } - } - } -} /// Operation to map all contained [`Entity`] fields in a type to new values. /// @@ -33,7 +14,7 @@ impl fmt::Display for MapEntitiesError { /// /// ```rust /// use bevy_ecs::prelude::*; -/// use bevy_ecs::entity::{EntityMap, MapEntities, MapEntitiesError}; +/// use bevy_ecs::entity::{EntityMapper, MapEntities}; /// /// #[derive(Component)] /// struct Spring { @@ -42,10 +23,9 @@ impl fmt::Display for MapEntitiesError { /// } /// /// impl MapEntities for Spring { -/// fn map_entities(&mut self, entity_map: &EntityMap) -> Result<(), MapEntitiesError> { -/// self.a = entity_map.get(self.a)?; -/// self.b = entity_map.get(self.b)?; -/// Ok(()) +/// fn map_entities(&mut self, entity_mapper: &mut EntityMapper) { +/// self.a = entity_mapper.get_or_reserve(self.a); +/// self.b = entity_mapper.get_or_reserve(self.b); /// } /// } /// ``` @@ -55,16 +35,22 @@ pub trait MapEntities { /// Updates all [`Entity`] references stored inside using `entity_map`. /// /// Implementors should look up any and all [`Entity`] values stored within and - /// update them to the mapped values via `entity_map`. - fn map_entities(&mut self, entity_map: &EntityMap) -> Result<(), MapEntitiesError>; + /// update them to the mapped values via `entity_mapper`. + fn map_entities(&mut self, entity_mapper: &mut EntityMapper); } /// A mapping from one set of entities to another. /// /// The API generally follows [`HashMap`], but each [`Entity`] is returned by value, as they are [`Copy`]. /// -/// This is typically used to coordinate data transfer between sets of entities, such as between a scene and the world or over the network. -/// This is required as [`Entity`] identifiers are opaque; you cannot and do not want to reuse identifiers directly. +/// This is typically used to coordinate data transfer between sets of entities, such as between a scene and the world +/// or over the network. This is required as [`Entity`] identifiers are opaque; you cannot and do not want to reuse +/// identifiers directly. +/// +/// On its own, an `EntityMap` is not capable of allocating new entity identifiers, which is needed to map references +/// to entities that lie outside the source entity set. To do this, an `EntityMap` can be wrapped in an +/// [`EntityMapper`] which scopes it to a particular destination [`World`] and allows new identifiers to be allocated. +/// This functionality can be accessed through [`Self::world_scope()`]. #[derive(Default, Debug)] pub struct EntityMap { map: HashMap, @@ -91,11 +77,8 @@ impl EntityMap { } /// Returns the corresponding mapped entity. - pub fn get(&self, entity: Entity) -> Result { - self.map - .get(&entity) - .cloned() - .ok_or(MapEntitiesError::EntityNotFound(entity)) + pub fn get(&self, entity: Entity) -> Option { + self.map.get(&entity).copied() } /// An iterator visiting all keys in arbitrary order. @@ -122,4 +105,138 @@ impl EntityMap { pub fn iter(&self) -> impl Iterator + '_ { self.map.iter().map(|(from, to)| (*from, *to)) } + + /// Creates an [`EntityMapper`] from this [`EntityMap`] and scoped to the provided [`World`], then calls the + /// provided function with it. This allows one to allocate new entity references in the provided `World` that are + /// guaranteed to never point at a living entity now or in the future. This functionality is useful for safely + /// mapping entity identifiers that point at entities outside the source world. The passed function, `f`, is called + /// within the scope of the passed world. Its return value is then returned from `world_scope` as the generic type + /// parameter `R`. + pub fn world_scope( + &mut self, + world: &mut World, + f: impl FnOnce(&mut World, &mut EntityMapper) -> R, + ) -> R { + let mut mapper = EntityMapper::new(self, world); + let result = f(world, &mut mapper); + mapper.finish(world); + result + } +} + +/// A wrapper for [`EntityMap`], augmenting it with the ability to allocate new [`Entity`] references in a destination +/// world. These newly allocated references are guaranteed to never point to any living entity in that world. +/// +/// References are allocated by returning increasing generations starting from an internally initialized base +/// [`Entity`]. After it is finished being used by [`MapEntities`] implementations, this entity is despawned and the +/// requisite number of generations reserved. +pub struct EntityMapper<'m> { + /// The wrapped [`EntityMap`]. + map: &'m mut EntityMap, + /// A base [`Entity`] used to allocate new references. + dead_start: Entity, + /// The number of generations this mapper has allocated thus far. + generations: u32, +} + +impl<'m> EntityMapper<'m> { + /// Returns the corresponding mapped entity or reserves a new dead entity ID if it is absent. + pub fn get_or_reserve(&mut self, entity: Entity) -> Entity { + if let Some(mapped) = self.map.get(entity) { + return mapped; + } + + // this new entity reference is specifically designed to never represent any living entity + let new = Entity { + generation: self.dead_start.generation + self.generations, + index: self.dead_start.index, + }; + self.generations += 1; + + self.map.insert(entity, new); + + new + } + + /// Gets a reference to the underlying [`EntityMap`]. + pub fn get_map(&'m self) -> &'m EntityMap { + self.map + } + + /// Gets a mutable reference to the underlying [`EntityMap`] + pub fn get_map_mut(&'m mut self) -> &'m mut EntityMap { + self.map + } + + /// Creates a new [`EntityMapper`], spawning a temporary base [`Entity`] in the provided [`World`] + fn new(map: &'m mut EntityMap, world: &mut World) -> Self { + Self { + map, + // SAFETY: Entities data is kept in a valid state via `EntityMap::world_scope` + dead_start: unsafe { world.entities_mut().alloc() }, + generations: 0, + } + } + + /// Reserves the allocated references to dead entities within the world. This frees the temporary base + /// [`Entity`] while reserving extra generations via [`crate::entity::Entities::reserve_generations`]. Because this + /// renders the [`EntityMapper`] unable to safely allocate any more references, this method takes ownership of + /// `self` in order to render it unusable. + fn finish(self, world: &mut World) { + // SAFETY: Entities data is kept in a valid state via `EntityMap::world_scope` + let entities = unsafe { world.entities_mut() }; + assert!(entities.free(self.dead_start).is_some()); + assert!(entities.reserve_generations(self.dead_start.index, self.generations)); + } +} + +#[cfg(test)] +mod tests { + use super::{EntityMap, EntityMapper}; + use crate::{entity::Entity, world::World}; + + #[test] + fn entity_mapper() { + const FIRST_IDX: u32 = 1; + const SECOND_IDX: u32 = 2; + + let mut map = EntityMap::default(); + let mut world = World::new(); + let mut mapper = EntityMapper::new(&mut map, &mut world); + + let mapped_ent = Entity::new(FIRST_IDX, 0); + let dead_ref = mapper.get_or_reserve(mapped_ent); + + assert_eq!( + dead_ref, + mapper.get_or_reserve(mapped_ent), + "should persist the allocated mapping from the previous line" + ); + assert_eq!( + mapper.get_or_reserve(Entity::new(SECOND_IDX, 0)).index(), + dead_ref.index(), + "should re-use the same index for further dead refs" + ); + + mapper.finish(&mut world); + // Next allocated entity should be a further generation on the same index + let entity = world.spawn_empty().id(); + assert_eq!(entity.index(), dead_ref.index()); + assert!(entity.generation() > dead_ref.generation()); + } + + #[test] + fn world_scope_reserves_generations() { + let mut map = EntityMap::default(); + let mut world = World::new(); + + let dead_ref = map.world_scope(&mut world, |_, mapper| { + mapper.get_or_reserve(Entity::new(0, 0)) + }); + + // Next allocated entity should be a further generation on the same index + let entity = world.spawn_empty().id(); + assert_eq!(entity.index(), dead_ref.index()); + assert!(entity.generation() > dead_ref.generation()); + } } diff --git a/crates/bevy_ecs/src/entity/mod.rs b/crates/bevy_ecs/src/entity/mod.rs index 0dcbe44581beb..38fecdbd51944 100644 --- a/crates/bevy_ecs/src/entity/mod.rs +++ b/crates/bevy_ecs/src/entity/mod.rs @@ -115,7 +115,7 @@ type IdCursor = isize; /// [`EntityCommands`]: crate::system::EntityCommands /// [`Query::get`]: crate::system::Query::get /// [`World`]: crate::world::World -#[derive(Clone, Copy, Deserialize, Eq, Hash, Ord, PartialEq, PartialOrd, Serialize)] +#[derive(Clone, Copy, Eq, Hash, Ord, PartialEq, PartialOrd)] pub struct Entity { generation: u32, index: u32, @@ -227,6 +227,25 @@ impl Entity { } } +impl Serialize for Entity { + fn serialize(&self, serializer: S) -> Result + where + S: serde::Serializer, + { + serializer.serialize_u64(self.to_bits()) + } +} + +impl<'de> Deserialize<'de> for Entity { + fn deserialize(deserializer: D) -> Result + where + D: serde::Deserializer<'de>, + { + let id: u64 = serde::de::Deserialize::deserialize(deserializer)?; + Ok(Entity::from_bits(id)) + } +} + impl fmt::Debug for Entity { fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { write!(f, "{}v{}", self.index, self.generation) @@ -604,6 +623,25 @@ impl Entities { self.meta.get_unchecked_mut(index as usize).location = location; } + /// Increments the `generation` of a freed [`Entity`]. The next entity ID allocated with this + /// `index` will count `generation` starting from the prior `generation` + the specified + /// value + 1. + /// + /// Does nothing if no entity with this `index` has been allocated yet. + pub(crate) fn reserve_generations(&mut self, index: u32, generations: u32) -> bool { + if (index as usize) >= self.meta.len() { + return false; + } + + let meta = &mut self.meta[index as usize]; + if meta.location.archetype_id == ArchetypeId::INVALID { + meta.generation += generations; + true + } else { + false + } + } + /// Get the [`Entity`] with a given id, if it exists in this [`Entities`] collection /// Returns `None` if this [`Entity`] is outside of the range of currently reserved Entities /// @@ -847,4 +885,29 @@ mod tests { const C4: u32 = Entity::from_bits(0x00dd_00ff_0000_0000).generation(); assert_eq!(0x00dd_00ff, C4); } + + #[test] + fn reserve_generations() { + let mut entities = Entities::new(); + let entity = entities.alloc(); + entities.free(entity); + + assert!(entities.reserve_generations(entity.index, 1)); + } + + #[test] + fn reserve_generations_and_alloc() { + const GENERATIONS: u32 = 10; + + let mut entities = Entities::new(); + let entity = entities.alloc(); + entities.free(entity); + + assert!(entities.reserve_generations(entity.index, GENERATIONS)); + + // The very next entitiy allocated should be a further generation on the same index + let next_entity = entities.alloc(); + assert_eq!(next_entity.index(), entity.index()); + assert!(next_entity.generation > entity.generation + GENERATIONS); + } } diff --git a/crates/bevy_ecs/src/reflect.rs b/crates/bevy_ecs/src/reflect.rs index 91781e74fc90e..c80fb9ae586fa 100644 --- a/crates/bevy_ecs/src/reflect.rs +++ b/crates/bevy_ecs/src/reflect.rs @@ -3,7 +3,7 @@ use crate::{ change_detection::Mut, component::Component, - entity::{Entity, EntityMap, MapEntities, MapEntitiesError}, + entity::{Entity, EntityMap, EntityMapper, MapEntities}, system::Resource, world::{ unsafe_world_cell::{UnsafeEntityCell, UnsafeWorldCell}, @@ -412,8 +412,8 @@ impl_from_reflect_value!(Entity); #[derive(Clone)] pub struct ReflectMapEntities { - map_entities: fn(&mut World, &EntityMap) -> Result<(), MapEntitiesError>, - map_specific_entities: fn(&mut World, &EntityMap, &[Entity]) -> Result<(), MapEntitiesError>, + map_entities: fn(&mut World, &mut EntityMapper), + map_specific_entities: fn(&mut World, &mut EntityMapper, &[Entity]), } impl ReflectMapEntities { @@ -426,12 +426,8 @@ impl ReflectMapEntities { /// An example of this: A scene can be loaded with `Parent` components, but then a `Parent` component can be added /// to these entities after they have been loaded. If you reload the scene using [`map_entities`](Self::map_entities), those `Parent` /// components with already valid entity references could be updated to point at something else entirely. - pub fn map_entities( - &self, - world: &mut World, - entity_map: &EntityMap, - ) -> Result<(), MapEntitiesError> { - (self.map_entities)(world, entity_map) + pub fn map_entities(&self, world: &mut World, entity_map: &mut EntityMap) { + entity_map.world_scope(world, self.map_entities); } /// This is like [`map_entities`](Self::map_entities), but only applied to specific entities, not all values @@ -442,31 +438,32 @@ impl ReflectMapEntities { pub fn map_specific_entities( &self, world: &mut World, - entity_map: &EntityMap, + entity_map: &mut EntityMap, entities: &[Entity], - ) -> Result<(), MapEntitiesError> { - (self.map_specific_entities)(world, entity_map, entities) + ) { + entity_map.world_scope(world, |world, mapper| { + (self.map_specific_entities)(world, mapper, entities); + }); } } impl FromType for ReflectMapEntities { fn from_type() -> Self { ReflectMapEntities { - map_specific_entities: |world, entity_map, entities| { + map_specific_entities: |world, entity_mapper, entities| { for &entity in entities { if let Some(mut component) = world.get_mut::(entity) { - component.map_entities(entity_map)?; + component.map_entities(entity_mapper); } } - Ok(()) }, - map_entities: |world, entity_map| { - for entity in entity_map.values() { - if let Some(mut component) = world.get_mut::(entity) { - component.map_entities(entity_map)?; + map_entities: |world, entity_mapper| { + let entities = entity_mapper.get_map().values().collect::>(); + for entity in &entities { + if let Some(mut component) = world.get_mut::(*entity) { + component.map_entities(entity_mapper); } } - Ok(()) }, } } diff --git a/crates/bevy_hierarchy/src/components/children.rs b/crates/bevy_hierarchy/src/components/children.rs index 541b48d68a07f..7a76d91b4c679 100644 --- a/crates/bevy_hierarchy/src/components/children.rs +++ b/crates/bevy_hierarchy/src/components/children.rs @@ -1,6 +1,6 @@ use bevy_ecs::{ component::Component, - entity::{Entity, EntityMap, MapEntities, MapEntitiesError}, + entity::{Entity, EntityMapper, MapEntities}, prelude::FromWorld, reflect::{ReflectComponent, ReflectMapEntities}, world::World, @@ -21,12 +21,10 @@ use std::ops::Deref; pub struct Children(pub(crate) SmallVec<[Entity; 8]>); impl MapEntities for Children { - fn map_entities(&mut self, entity_map: &EntityMap) -> Result<(), MapEntitiesError> { + fn map_entities(&mut self, entity_mapper: &mut EntityMapper) { for entity in &mut self.0 { - *entity = entity_map.get(*entity)?; + *entity = entity_mapper.get_or_reserve(*entity); } - - Ok(()) } } diff --git a/crates/bevy_hierarchy/src/components/parent.rs b/crates/bevy_hierarchy/src/components/parent.rs index 3e573be5a6952..7bccf33ed6e1e 100644 --- a/crates/bevy_hierarchy/src/components/parent.rs +++ b/crates/bevy_hierarchy/src/components/parent.rs @@ -1,6 +1,6 @@ use bevy_ecs::{ component::Component, - entity::{Entity, EntityMap, MapEntities, MapEntitiesError}, + entity::{Entity, EntityMapper, MapEntities}, reflect::{ReflectComponent, ReflectMapEntities}, world::{FromWorld, World}, }; @@ -36,13 +36,8 @@ impl FromWorld for Parent { } impl MapEntities for Parent { - fn map_entities(&mut self, entity_map: &EntityMap) -> Result<(), MapEntitiesError> { - // Parent of an entity in the new world can be in outside world, in which case it - // should not be mapped. - if let Ok(mapped_entity) = entity_map.get(self.0) { - self.0 = mapped_entity; - } - Ok(()) + fn map_entities(&mut self, entity_mapper: &mut EntityMapper) { + self.0 = entity_mapper.get_or_reserve(self.0); } } diff --git a/crates/bevy_render/src/mesh/mesh/skinning.rs b/crates/bevy_render/src/mesh/mesh/skinning.rs index 3c3cef062de06..a124655df15c7 100644 --- a/crates/bevy_render/src/mesh/mesh/skinning.rs +++ b/crates/bevy_render/src/mesh/mesh/skinning.rs @@ -1,7 +1,7 @@ use bevy_asset::Handle; use bevy_ecs::{ component::Component, - entity::{Entity, EntityMap, MapEntities, MapEntitiesError}, + entity::{Entity, EntityMapper, MapEntities}, prelude::ReflectComponent, reflect::ReflectMapEntities, }; @@ -17,12 +17,10 @@ pub struct SkinnedMesh { } impl MapEntities for SkinnedMesh { - fn map_entities(&mut self, entity_map: &EntityMap) -> Result<(), MapEntitiesError> { + fn map_entities(&mut self, entity_mapper: &mut EntityMapper) { for joint in &mut self.joints { - *joint = entity_map.get(*joint)?; + *joint = entity_mapper.get_or_reserve(*joint); } - - Ok(()) } } diff --git a/crates/bevy_scene/src/dynamic_scene.rs b/crates/bevy_scene/src/dynamic_scene.rs index 0a45e0e82b9d8..7bbdb07cb9f20 100644 --- a/crates/bevy_scene/src/dynamic_scene.rs +++ b/crates/bevy_scene/src/dynamic_scene.rs @@ -4,8 +4,7 @@ use crate::{DynamicSceneBuilder, Scene, SceneSpawnError}; use anyhow::Result; use bevy_app::AppTypeRegistry; use bevy_ecs::{ - entity::EntityMap, - prelude::Entity, + entity::{Entity, EntityMap}, reflect::{ReflectComponent, ReflectMapEntities}, world::World, }; @@ -36,8 +35,10 @@ pub struct DynamicScene { /// A reflection-powered serializable representation of an entity and its components. pub struct DynamicEntity { - /// The transiently unique identifier of a corresponding [`Entity`](bevy_ecs::entity::Entity). - pub entity: u32, + /// The identifier of the entity, unique within a scene (and the world it may have been generated from). + /// + /// Components that reference this entity must consistently use this identifier. + pub entity: Entity, /// A vector of boxed components that belong to the given entity and /// implement the [`Reflect`] trait. pub components: Vec>, @@ -101,7 +102,7 @@ impl DynamicScene { // or spawn a new entity with a transiently unique id if there is // no corresponding entry. let entity = *entity_map - .entry(bevy_ecs::entity::Entity::from_raw(scene_entity.entity)) + .entry(scene_entity.entity) .or_insert_with(|| world.spawn_empty().id()); let entity_mut = &mut world.entity_mut(entity); @@ -141,9 +142,7 @@ impl DynamicScene { "we should be getting TypeId from this TypeRegistration in the first place", ); if let Some(map_entities_reflect) = registration.data::() { - map_entities_reflect - .map_specific_entities(world, entity_map, &entities) - .unwrap(); + map_entities_reflect.map_specific_entities(world, entity_map, &entities); } } diff --git a/crates/bevy_scene/src/dynamic_scene_builder.rs b/crates/bevy_scene/src/dynamic_scene_builder.rs index 0f25b1e484ba3..b19d0a0993999 100644 --- a/crates/bevy_scene/src/dynamic_scene_builder.rs +++ b/crates/bevy_scene/src/dynamic_scene_builder.rs @@ -38,7 +38,7 @@ use std::collections::BTreeMap; /// ``` pub struct DynamicSceneBuilder<'w> { extracted_resources: BTreeMap>, - extracted_scene: BTreeMap, + extracted_scene: BTreeMap, type_registry: AppTypeRegistry, original_world: &'w World, } @@ -123,19 +123,17 @@ impl<'w> DynamicSceneBuilder<'w> { let type_registry = self.type_registry.read(); for entity in entities { - let index = entity.index(); - - if self.extracted_scene.contains_key(&index) { + if self.extracted_scene.contains_key(&entity) { continue; } let mut entry = DynamicEntity { - entity: index, + entity, components: Vec::new(), }; - let entity = self.original_world.entity(entity); - for component_id in entity.archetype().components() { + let original_entity = self.original_world.entity(entity); + for component_id in original_entity.archetype().components() { let mut extract_and_push = || { let type_id = self .original_world @@ -145,13 +143,13 @@ impl<'w> DynamicSceneBuilder<'w> { let component = type_registry .get(type_id)? .data::()? - .reflect(entity)?; + .reflect(original_entity)?; entry.components.push(component.clone_value()); Some(()) }; extract_and_push(); } - self.extracted_scene.insert(index, entry); + self.extracted_scene.insert(entity, entry); } drop(type_registry); @@ -243,7 +241,7 @@ mod tests { let scene = builder.build(); assert_eq!(scene.entities.len(), 1); - assert_eq!(scene.entities[0].entity, entity.index()); + assert_eq!(scene.entities[0].entity, entity); assert_eq!(scene.entities[0].components.len(), 1); assert!(scene.entities[0].components[0].represents::()); } @@ -264,7 +262,7 @@ mod tests { let scene = builder.build(); assert_eq!(scene.entities.len(), 1); - assert_eq!(scene.entities[0].entity, entity.index()); + assert_eq!(scene.entities[0].entity, entity); assert_eq!(scene.entities[0].components.len(), 1); assert!(scene.entities[0].components[0].represents::()); } @@ -288,7 +286,7 @@ mod tests { let scene = builder.build(); assert_eq!(scene.entities.len(), 1); - assert_eq!(scene.entities[0].entity, entity.index()); + assert_eq!(scene.entities[0].entity, entity); assert_eq!(scene.entities[0].components.len(), 2); assert!(scene.entities[0].components[0].represents::()); assert!(scene.entities[0].components[1].represents::()); @@ -315,10 +313,10 @@ mod tests { let mut entities = builder.build().entities.into_iter(); // Assert entities are ordered - assert_eq!(entity_a.index(), entities.next().map(|e| e.entity).unwrap()); - assert_eq!(entity_b.index(), entities.next().map(|e| e.entity).unwrap()); - assert_eq!(entity_c.index(), entities.next().map(|e| e.entity).unwrap()); - assert_eq!(entity_d.index(), entities.next().map(|e| e.entity).unwrap()); + assert_eq!(entity_a, entities.next().map(|e| e.entity).unwrap()); + assert_eq!(entity_b, entities.next().map(|e| e.entity).unwrap()); + assert_eq!(entity_c, entities.next().map(|e| e.entity).unwrap()); + assert_eq!(entity_d, entities.next().map(|e| e.entity).unwrap()); } #[test] @@ -345,7 +343,7 @@ mod tests { assert_eq!(scene.entities.len(), 2); let mut scene_entities = vec![scene.entities[0].entity, scene.entities[1].entity]; scene_entities.sort(); - assert_eq!(scene_entities, [entity_a_b.index(), entity_a.index()]); + assert_eq!(scene_entities, [entity_a_b, entity_a]); } #[test] @@ -365,7 +363,7 @@ mod tests { let scene = builder.build(); assert_eq!(scene.entities.len(), 1); - assert_eq!(scene.entities[0].entity, entity_a.index()); + assert_eq!(scene.entities[0].entity, entity_a); } #[test] diff --git a/crates/bevy_scene/src/scene.rs b/crates/bevy_scene/src/scene.rs index 462c1ec41e49f..2e5a342e40e77 100644 --- a/crates/bevy_scene/src/scene.rs +++ b/crates/bevy_scene/src/scene.rs @@ -120,9 +120,7 @@ impl Scene { for registration in type_registry.iter() { if let Some(map_entities_reflect) = registration.data::() { - map_entities_reflect - .map_entities(world, &instance_info.entity_map) - .unwrap(); + map_entities_reflect.map_entities(world, &mut instance_info.entity_map); } } diff --git a/crates/bevy_scene/src/serde.rs b/crates/bevy_scene/src/serde.rs index debe7c54515f3..635feee649230 100644 --- a/crates/bevy_scene/src/serde.rs +++ b/crates/bevy_scene/src/serde.rs @@ -1,5 +1,6 @@ use crate::{DynamicEntity, DynamicScene}; use anyhow::Result; +use bevy_ecs::entity::Entity; use bevy_reflect::serde::{TypedReflectDeserializer, TypedReflectSerializer}; use bevy_reflect::{ serde::{TypeRegistrationDeserializer, UntypedReflectDeserializer}, @@ -260,9 +261,9 @@ impl<'a, 'de> Visitor<'de> for SceneEntitiesVisitor<'a> { A: MapAccess<'de>, { let mut entities = Vec::new(); - while let Some(id) = map.next_key::()? { + while let Some(entity) = map.next_key::()? { let entity = map.next_value_seed(SceneEntityDeserializer { - id, + entity, type_registry: self.type_registry, })?; entities.push(entity); @@ -273,7 +274,7 @@ impl<'a, 'de> Visitor<'de> for SceneEntitiesVisitor<'a> { } pub struct SceneEntityDeserializer<'a> { - pub id: u32, + pub entity: Entity, pub type_registry: &'a TypeRegistry, } @@ -288,7 +289,7 @@ impl<'a, 'de> DeserializeSeed<'de> for SceneEntityDeserializer<'a> { ENTITY_STRUCT, &[ENTITY_FIELD_COMPONENTS], SceneEntityVisitor { - id: self.id, + entity: self.entity, registry: self.type_registry, }, ) @@ -296,7 +297,7 @@ impl<'a, 'de> DeserializeSeed<'de> for SceneEntityDeserializer<'a> { } struct SceneEntityVisitor<'a> { - pub id: u32, + pub entity: Entity, pub registry: &'a TypeRegistry, } @@ -318,7 +319,7 @@ impl<'a, 'de> Visitor<'de> for SceneEntityVisitor<'a> { .ok_or_else(|| Error::missing_field(ENTITY_FIELD_COMPONENTS))?; Ok(DynamicEntity { - entity: self.id, + entity: self.entity, components, }) } @@ -346,7 +347,7 @@ impl<'a, 'de> Visitor<'de> for SceneEntityVisitor<'a> { .take() .ok_or_else(|| Error::missing_field(ENTITY_FIELD_COMPONENTS))?; Ok(DynamicEntity { - entity: self.id, + entity: self.entity, components, }) } @@ -424,8 +425,11 @@ mod tests { use crate::serde::{SceneDeserializer, SceneSerializer}; use crate::{DynamicScene, DynamicSceneBuilder}; use bevy_app::AppTypeRegistry; - use bevy_ecs::entity::EntityMap; + use bevy_ecs::entity::{Entity, EntityMap, EntityMapper, MapEntities}; use bevy_ecs::prelude::{Component, ReflectComponent, ReflectResource, Resource, World}; + use bevy_ecs::query::{With, Without}; + use bevy_ecs::reflect::ReflectMapEntities; + use bevy_ecs::world::FromWorld; use bevy_reflect::{FromReflect, Reflect, ReflectSerialize}; use bincode::Options; use serde::de::DeserializeSeed; @@ -466,6 +470,22 @@ mod tests { foo: i32, } + #[derive(Clone, Component, Reflect, PartialEq)] + #[reflect(Component, MapEntities, PartialEq)] + struct MyEntityRef(Entity); + + impl MapEntities for MyEntityRef { + fn map_entities(&mut self, entity_mapper: &mut EntityMapper) { + self.0 = entity_mapper.get_or_reserve(self.0); + } + } + + impl FromWorld for MyEntityRef { + fn from_world(_world: &mut World) -> Self { + Self(Entity::PLACEHOLDER) + } + } + fn create_world() -> World { let mut world = World::new(); let registry = AppTypeRegistry::default(); @@ -480,6 +500,8 @@ mod tests { registry.register_type_data::(); registry.register::<[usize; 3]>(); registry.register::<(f32, f32)>(); + registry.register::(); + registry.register::(); registry.register::(); } world.insert_resource(registry); @@ -596,6 +618,57 @@ mod tests { assert_eq!(1, dst_world.query::<&Baz>().iter(&dst_world).count()); } + #[test] + fn should_roundtrip_with_later_generations_and_obsolete_references() { + let mut world = create_world(); + + world.spawn_empty().despawn(); + + let a = world.spawn_empty().id(); + let foo = world.spawn(MyEntityRef(a)).insert(Foo(123)).id(); + world.despawn(a); + world.spawn(MyEntityRef(foo)).insert(Bar(123)); + + let registry = world.resource::(); + + let scene = DynamicScene::from_world(&world, registry); + + let serialized = scene + .serialize_ron(&world.resource::().0) + .unwrap(); + let mut deserializer = ron::de::Deserializer::from_str(&serialized).unwrap(); + let scene_deserializer = SceneDeserializer { + type_registry: ®istry.0.read(), + }; + + let deserialized_scene = scene_deserializer.deserialize(&mut deserializer).unwrap(); + + let mut map = EntityMap::default(); + let mut dst_world = create_world(); + deserialized_scene + .write_to_world(&mut dst_world, &mut map) + .unwrap(); + + assert_eq!(2, deserialized_scene.entities.len()); + assert_scene_eq(&scene, &deserialized_scene); + + let bar_to_foo = dst_world + .query_filtered::<&MyEntityRef, Without>() + .get_single(&dst_world) + .cloned() + .unwrap(); + let foo = dst_world + .query_filtered::>() + .get_single(&dst_world) + .unwrap(); + + assert_eq!(foo, bar_to_foo.0); + assert!(dst_world + .query_filtered::<&MyEntityRef, With>() + .iter(&dst_world) + .all(|r| world.get_entity(r.0).is_none())); + } + #[test] fn should_roundtrip_postcard() { let mut world = create_world(); @@ -696,12 +769,12 @@ mod tests { assert_eq!( vec![ - 0, 0, 0, 0, 0, 0, 0, 0, 1, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 1, 0, 0, 0, 0, 0, 0, 0, - 37, 0, 0, 0, 0, 0, 0, 0, 98, 101, 118, 121, 95, 115, 99, 101, 110, 101, 58, 58, - 115, 101, 114, 100, 101, 58, 58, 116, 101, 115, 116, 115, 58, 58, 77, 121, 67, 111, - 109, 112, 111, 110, 101, 110, 116, 1, 0, 0, 0, 0, 0, 0, 0, 2, 0, 0, 0, 0, 0, 0, 0, - 3, 0, 0, 0, 0, 0, 0, 0, 102, 102, 166, 63, 205, 204, 108, 64, 1, 0, 0, 0, 12, 0, 0, - 0, 0, 0, 0, 0, 72, 101, 108, 108, 111, 32, 87, 111, 114, 108, 100, 33 + 0, 0, 0, 0, 0, 0, 0, 0, 1, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 1, 0, 0, 0, + 0, 0, 0, 0, 37, 0, 0, 0, 0, 0, 0, 0, 98, 101, 118, 121, 95, 115, 99, 101, 110, 101, + 58, 58, 115, 101, 114, 100, 101, 58, 58, 116, 101, 115, 116, 115, 58, 58, 77, 121, + 67, 111, 109, 112, 111, 110, 101, 110, 116, 1, 0, 0, 0, 0, 0, 0, 0, 2, 0, 0, 0, 0, + 0, 0, 0, 3, 0, 0, 0, 0, 0, 0, 0, 102, 102, 166, 63, 205, 204, 108, 64, 1, 0, 0, 0, + 12, 0, 0, 0, 0, 0, 0, 0, 72, 101, 108, 108, 111, 32, 87, 111, 114, 108, 100, 33 ], serialized_scene ); @@ -732,7 +805,7 @@ mod tests { .entities .iter() .find(|dynamic_entity| dynamic_entity.entity == expected.entity) - .unwrap_or_else(|| panic!("missing entity (expected: `{}`)", expected.entity)); + .unwrap_or_else(|| panic!("missing entity (expected: `{:?}`)", expected.entity)); assert_eq!(expected.entity, received.entity, "entities did not match",); diff --git a/crates/bevy_window/src/window.rs b/crates/bevy_window/src/window.rs index ada0387a4d9fc..96e43a7c59c3b 100644 --- a/crates/bevy_window/src/window.rs +++ b/crates/bevy_window/src/window.rs @@ -1,5 +1,5 @@ use bevy_ecs::{ - entity::{Entity, EntityMap, MapEntities, MapEntitiesError}, + entity::{Entity, EntityMapper, MapEntities}, prelude::{Component, ReflectComponent}, }; use bevy_math::{DVec2, IVec2, Vec2}; @@ -55,14 +55,13 @@ impl WindowRef { } impl MapEntities for WindowRef { - fn map_entities(&mut self, entity_map: &EntityMap) -> Result<(), MapEntitiesError> { + fn map_entities(&mut self, entity_mapper: &mut EntityMapper) { match self { Self::Entity(entity) => { - *entity = entity_map.get(*entity)?; - Ok(()) + *entity = entity_mapper.get_or_reserve(*entity); } - Self::Primary => Ok(()), - } + Self::Primary => {} + }; } } @@ -145,7 +144,7 @@ pub struct Window { /// The "html canvas" element selector. /// /// If set, this selector will be used to find a matching html canvas element, - /// rather than creating a new one. + /// rather than creating a new one. /// Uses the [CSS selector format](https://developer.mozilla.org/en-US/docs/Web/API/Document/querySelector). /// /// This value has no effect on non-web platforms.