From 7118b80b4f02a56510ba15a6fc039eae99d580a1 Mon Sep 17 00:00:00 2001 From: illiux Date: Sat, 21 Jan 2023 16:18:40 -0800 Subject: [PATCH 01/18] Support mapping dead references in MapEntities --- crates/bevy_ecs/src/entity/map_entities.rs | 70 +++++++++++++++++-- crates/bevy_ecs/src/entity/mod.rs | 16 +++++ crates/bevy_ecs/src/reflect.rs | 12 ++-- crates/bevy_ecs/src/world/mod.rs | 32 ++++++--- .../bevy_hierarchy/src/components/children.rs | 6 +- .../bevy_hierarchy/src/components/parent.rs | 10 +-- crates/bevy_render/src/mesh/mesh/skinning.rs | 6 +- 7 files changed, 123 insertions(+), 29 deletions(-) diff --git a/crates/bevy_ecs/src/entity/map_entities.rs b/crates/bevy_ecs/src/entity/map_entities.rs index 2a8dbfbc468c8..9bd686790174f 100644 --- a/crates/bevy_ecs/src/entity/map_entities.rs +++ b/crates/bevy_ecs/src/entity/map_entities.rs @@ -1,4 +1,4 @@ -use crate::entity::Entity; +use crate::{entity::Entity, world::World}; use bevy_utils::{Entry, HashMap}; use std::fmt; @@ -33,7 +33,7 @@ impl fmt::Display for MapEntitiesError { /// /// ```rust /// use bevy_ecs::prelude::*; -/// use bevy_ecs::entity::{EntityMap, MapEntities, MapEntitiesError}; +/// use bevy_ecs::entity::{EntityMapper, MapEntities, MapEntitiesError}; /// /// #[derive(Component)] /// struct Spring { @@ -42,7 +42,7 @@ impl fmt::Display for MapEntitiesError { /// } /// /// impl MapEntities for Spring { -/// fn map_entities(&mut self, entity_map: &EntityMap) -> Result<(), MapEntitiesError> { +/// fn map_entities(&mut self, entity_map: &mut EntityMapper) -> Result<(), MapEntitiesError> { /// self.a = entity_map.get(self.a)?; /// self.b = entity_map.get(self.b)?; /// Ok(()) @@ -56,7 +56,7 @@ pub trait MapEntities { /// /// 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>; + fn map_entities(&mut self, entity_map: &mut EntityMapper) -> Result<(), MapEntitiesError>; } /// A mapping from one set of entities to another. @@ -70,6 +70,55 @@ pub struct EntityMap { map: HashMap, } +pub struct EntityMapper<'m> { + map: &'m mut EntityMap, + dead_start: Entity, + generations: u32, +} + +impl<'m> EntityMapper<'m> { + /// Reserves the allocated dead entities within the world. + pub fn save(mut self, world: &mut World) { + if self.generations == 0 { + return; + } + + assert!(world.reserve_generations(self.dead_start, self.generations)); + self.generations = 0; + } + + /// Returns the corresponding mapped entity. + pub fn get(&self, entity: Entity) -> Result { + self.map.get(entity) + } + + /// Returns the corresponding mapped entity or allocates a new dead entity if it is absent. + pub fn get_or_alloc(&mut self, entity: Entity) -> Entity { + if let Ok(mapped) = self.map.get(entity) { + return mapped; + } + + let new = Entity { + generation: self.dead_start.generation + self.generations, + index: self.dead_start.index, + }; + self.generations += 1; + + self.map.insert(entity, new); + + new + } +} + +impl<'m> Drop for EntityMapper<'m> { + fn drop(&mut self) { + assert_eq!( + 0, self.generations, + "EntityMapper not saved before being dropped" + ); + } +} + impl EntityMap { /// Inserts an entities pair into the map. /// @@ -122,4 +171,17 @@ impl EntityMap { pub fn iter(&self) -> impl Iterator + '_ { self.map.iter().map(|(from, to)| (*from, *to)) } + + /// Gets an [`EntityMapper`] for allocating new dead entities. + /// + /// # Safety + /// If this function is called, [`EntityMapper::save()`] _must_ be subsequently called after mapping. + /// Otherwise, a panic may occur when the [`EntityMapper`] is dropped. + pub unsafe fn get_mapper<'m>(&'m mut self, world: &mut World) -> EntityMapper<'m> { + EntityMapper { + map: self, + dead_start: world.spawn_empty().id(), + generations: 0, + } + } } diff --git a/crates/bevy_ecs/src/entity/mod.rs b/crates/bevy_ecs/src/entity/mod.rs index afcba27bb68c1..f9f857a64bebb 100644 --- a/crates/bevy_ecs/src/entity/mod.rs +++ b/crates/bevy_ecs/src/entity/mod.rs @@ -590,6 +590,22 @@ impl Entities { self.meta.get_unchecked_mut(index as usize).location = location; } + /// Increments the generation of a despawned [`Entity`]. This may only be called on an entity + /// index corresponding to an entity that once existed and has since been freed. + 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 /// diff --git a/crates/bevy_ecs/src/reflect.rs b/crates/bevy_ecs/src/reflect.rs index 88e25dfaf86b5..6eb459c12285f 100644 --- a/crates/bevy_ecs/src/reflect.rs +++ b/crates/bevy_ecs/src/reflect.rs @@ -406,14 +406,14 @@ impl_from_reflect_value!(Entity); #[derive(Clone)] pub struct ReflectMapEntities { - map_entities: fn(&mut World, &EntityMap) -> Result<(), MapEntitiesError>, + map_entities: fn(&mut World, &mut EntityMap) -> Result<(), MapEntitiesError>, } impl ReflectMapEntities { pub fn map_entities( &self, world: &mut World, - entity_map: &EntityMap, + entity_map: &mut EntityMap, ) -> Result<(), MapEntitiesError> { (self.map_entities)(world, entity_map) } @@ -423,11 +423,15 @@ impl FromType for ReflectMapEntities { fn from_type() -> Self { ReflectMapEntities { map_entities: |world, entity_map| { - for entity in entity_map.values() { + let entities = entity_map.values().collect::>(); + // SAFETY: We save to the world just after calling entity mappers + let mut mapper = unsafe { entity_map.get_mapper(world) }; + for entity in entities.iter().cloned() { if let Some(mut component) = world.get_mut::(entity) { - component.map_entities(entity_map)?; + component.map_entities(&mut mapper)?; } } + mapper.save(world); Ok(()) }, } diff --git a/crates/bevy_ecs/src/world/mod.rs b/crates/bevy_ecs/src/world/mod.rs index dd09a5341aaa4..36629fd14ed67 100644 --- a/crates/bevy_ecs/src/world/mod.rs +++ b/crates/bevy_ecs/src/world/mod.rs @@ -608,6 +608,22 @@ impl World { } } + /// Despawns the given `entity`, if it exists, and reserves a number of subsequent generations. + /// + /// This function serves an extremely narrow use case of allocating a series of entity IDs that are + /// guaranteed to never refer to a live entity. This functionality is useful primarily for mapping references + /// to dead entities into a new world alongside a [`crate::entity::MapEntities`] implementation. + /// + /// See [`Self::despawn()`] for usage. + pub(crate) fn reserve_generations(&mut self, entity: Entity, generations: u32) -> bool { + if self.despawn(entity) { + self.entities + .reserve_generations(entity.index(), generations) + } else { + false + } + } + /// Clears the internal component tracker state. /// /// The world maintains some internal state about changed and removed components. This state @@ -946,8 +962,8 @@ impl World { match self.get_resource() { Some(x) => x, None => panic!( - "Requested resource {} does not exist in the `World`. - Did you forget to add it using `app.insert_resource` / `app.init_resource`? + "Requested resource {} does not exist in the `World`. + Did you forget to add it using `app.insert_resource` / `app.init_resource`? Resources are also implicitly added via `app.add_event`, and can be added by plugins.", std::any::type_name::() @@ -970,8 +986,8 @@ impl World { match self.get_resource_mut() { Some(x) => x, None => panic!( - "Requested resource {} does not exist in the `World`. - Did you forget to add it using `app.insert_resource` / `app.init_resource`? + "Requested resource {} does not exist in the `World`. + Did you forget to add it using `app.insert_resource` / `app.init_resource`? Resources are also implicitly added via `app.add_event`, and can be added by plugins.", std::any::type_name::() @@ -1050,8 +1066,8 @@ impl World { match self.get_non_send_resource() { Some(x) => x, None => panic!( - "Requested non-send resource {} does not exist in the `World`. - Did you forget to add it using `app.insert_non_send_resource` / `app.init_non_send_resource`? + "Requested non-send resource {} does not exist in the `World`. + Did you forget to add it using `app.insert_non_send_resource` / `app.init_non_send_resource`? Non-send resources can also be be added by plugins.", std::any::type_name::() ), @@ -1072,8 +1088,8 @@ impl World { match self.get_non_send_resource_mut() { Some(x) => x, None => panic!( - "Requested non-send resource {} does not exist in the `World`. - Did you forget to add it using `app.insert_non_send_resource` / `app.init_non_send_resource`? + "Requested non-send resource {} does not exist in the `World`. + Did you forget to add it using `app.insert_non_send_resource` / `app.init_non_send_resource`? Non-send resources can also be be added by plugins.", std::any::type_name::() ), diff --git a/crates/bevy_hierarchy/src/components/children.rs b/crates/bevy_hierarchy/src/components/children.rs index 541b48d68a07f..701291e0facd7 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, MapEntitiesError}, prelude::FromWorld, reflect::{ReflectComponent, ReflectMapEntities}, world::World, @@ -21,9 +21,9 @@ 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_map: &mut EntityMapper) -> Result<(), MapEntitiesError> { for entity in &mut self.0 { - *entity = entity_map.get(*entity)?; + *entity = entity_map.get_or_alloc(*entity); } Ok(()) diff --git a/crates/bevy_hierarchy/src/components/parent.rs b/crates/bevy_hierarchy/src/components/parent.rs index 3e573be5a6952..845b3a901e1e4 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, MapEntitiesError}, reflect::{ReflectComponent, ReflectMapEntities}, world::{FromWorld, World}, }; @@ -36,12 +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; - } + fn map_entities(&mut self, entity_map: &mut EntityMapper) -> Result<(), MapEntitiesError> { + self.0 = entity_map.get_or_alloc(self.0); Ok(()) } } diff --git a/crates/bevy_render/src/mesh/mesh/skinning.rs b/crates/bevy_render/src/mesh/mesh/skinning.rs index 3c3cef062de06..51b2ef6a2a418 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, MapEntitiesError}, prelude::ReflectComponent, reflect::ReflectMapEntities, }; @@ -17,9 +17,9 @@ pub struct SkinnedMesh { } impl MapEntities for SkinnedMesh { - fn map_entities(&mut self, entity_map: &EntityMap) -> Result<(), MapEntitiesError> { + fn map_entities(&mut self, entity_map: &mut EntityMapper) -> Result<(), MapEntitiesError> { for joint in &mut self.joints { - *joint = entity_map.get(*joint)?; + *joint = entity_map.get_or_alloc(*joint); } Ok(()) From 5228f02aebae34723096d0dc4f0fe1af665d02a6 Mon Sep 17 00:00:00 2001 From: illiux Date: Sat, 21 Jan 2023 16:18:40 -0800 Subject: [PATCH 02/18] Save entity generation in scenes --- crates/bevy_scene/src/dynamic_scene.rs | 4 +- .../bevy_scene/src/dynamic_scene_builder.rs | 36 ++++--- crates/bevy_scene/src/scene.rs | 2 +- crates/bevy_scene/src/serde.rs | 93 +++++++++++++++++-- 4 files changed, 110 insertions(+), 25 deletions(-) diff --git a/crates/bevy_scene/src/dynamic_scene.rs b/crates/bevy_scene/src/dynamic_scene.rs index c0eb067b2ca8b..658cb57e25a91 100644 --- a/crates/bevy_scene/src/dynamic_scene.rs +++ b/crates/bevy_scene/src/dynamic_scene.rs @@ -29,7 +29,7 @@ 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`. - pub entity: u32, + pub entity: u64, /// A vector of boxed components that belong to the given entity and /// implement the `Reflect` trait. pub components: Vec>, @@ -69,7 +69,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(bevy_ecs::entity::Entity::from_bits(scene_entity.entity)) .or_insert_with(|| world.spawn_empty().id()); // Apply/ add each component to the given entity. diff --git a/crates/bevy_scene/src/dynamic_scene_builder.rs b/crates/bevy_scene/src/dynamic_scene_builder.rs index 98bf10e27f5b8..c1fe425ef9ac5 100644 --- a/crates/bevy_scene/src/dynamic_scene_builder.rs +++ b/crates/bevy_scene/src/dynamic_scene_builder.rs @@ -31,7 +31,7 @@ use std::collections::BTreeMap; /// let dynamic_scene = builder.build(); /// ``` pub struct DynamicSceneBuilder<'w> { - extracted_scene: BTreeMap, + extracted_scene: BTreeMap, type_registry: AppTypeRegistry, original_world: &'w World, } @@ -113,14 +113,14 @@ impl<'w> DynamicSceneBuilder<'w> { let type_registry = self.type_registry.read(); for entity in entities { - let index = entity.index(); + let index = entity.to_bits(); if self.extracted_scene.contains_key(&index) { continue; } let mut entry = DynamicEntity { - entity: index, + entity: entity.to_bits(), components: Vec::new(), }; @@ -180,7 +180,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.to_bits()); assert_eq!(scene.entities[0].components.len(), 1); assert!(scene.entities[0].components[0].represents::()); } @@ -201,7 +201,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.to_bits()); assert_eq!(scene.entities[0].components.len(), 1); assert!(scene.entities[0].components[0].represents::()); } @@ -225,7 +225,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.to_bits()); assert_eq!(scene.entities[0].components.len(), 2); assert!(scene.entities[0].components[0].represents::()); assert!(scene.entities[0].components[1].represents::()); @@ -252,10 +252,22 @@ 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.to_bits(), + entities.next().map(|e| e.entity).unwrap() + ); + assert_eq!( + entity_b.to_bits(), + entities.next().map(|e| e.entity).unwrap() + ); + assert_eq!( + entity_c.to_bits(), + entities.next().map(|e| e.entity).unwrap() + ); + assert_eq!( + entity_d.to_bits(), + entities.next().map(|e| e.entity).unwrap() + ); } #[test] @@ -282,7 +294,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.to_bits(), entity_a.to_bits()]); } #[test] @@ -302,6 +314,6 @@ 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.to_bits()); } } diff --git a/crates/bevy_scene/src/scene.rs b/crates/bevy_scene/src/scene.rs index 4df757aa5838f..496b6aac7fc0c 100644 --- a/crates/bevy_scene/src/scene.rs +++ b/crates/bevy_scene/src/scene.rs @@ -93,7 +93,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) + .map_entities(world, &mut instance_info.entity_map) .unwrap(); } } diff --git a/crates/bevy_scene/src/serde.rs b/crates/bevy_scene/src/serde.rs index 51b8a5ebce0e7..bba7778ecdf1e 100644 --- a/crates/bevy_scene/src/serde.rs +++ b/crates/bevy_scene/src/serde.rs @@ -229,7 +229,7 @@ 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(id) = map.next_key::()? { let entity = map.next_value_seed(SceneEntityDeserializer { id, type_registry: self.type_registry, @@ -242,7 +242,7 @@ impl<'a, 'de> Visitor<'de> for SceneEntitiesVisitor<'a> { } pub struct SceneEntityDeserializer<'a> { - pub id: u32, + pub id: u64, pub type_registry: &'a TypeRegistry, } @@ -265,7 +265,7 @@ impl<'a, 'de> DeserializeSeed<'de> for SceneEntityDeserializer<'a> { } struct SceneEntityVisitor<'a> { - pub id: u32, + pub id: u64, pub registry: &'a TypeRegistry, } @@ -393,8 +393,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, MapEntitiesError}; use bevy_ecs::prelude::{Component, ReflectComponent, 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; @@ -429,6 +432,23 @@ mod tests { }, } + #[derive(Component, Reflect, PartialEq)] + #[reflect(Component, MapEntities, PartialEq)] + struct MyEntityRef(Entity); + + impl MapEntities for MyEntityRef { + fn map_entities(&mut self, entity_map: &mut EntityMapper) -> Result<(), MapEntitiesError> { + self.0 = entity_map.get_or_alloc(self.0); + Ok(()) + } + } + + 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(); @@ -443,6 +463,8 @@ mod tests { registry.register_type_data::(); registry.register::<[usize; 3]>(); registry.register::<(f32, f32)>(); + registry.register::(); + registry.register::(); } world.insert_resource(registry); world @@ -535,6 +557,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) + .unwrap() + .0; + let foo = dst_world + .query_filtered::>() + .get_single(&dst_world) + .unwrap(); + + assert_eq!(foo, bar_to_foo); + 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(); @@ -635,12 +708,12 @@ mod tests { assert_eq!( vec![ - 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 + 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 ); From 752d282195456e51ad91cd0be342551cd4802ba1 Mon Sep 17 00:00:00 2001 From: illiux Date: Sun, 22 Jan 2023 12:35:06 -0800 Subject: [PATCH 03/18] Update WindowRef's MapEntities impl --- crates/bevy_window/src/window.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/crates/bevy_window/src/window.rs b/crates/bevy_window/src/window.rs index ec29a7e9692aa..e138ba316f537 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, MapEntitiesError}, prelude::{Component, ReflectComponent}, }; use bevy_math::{DVec2, IVec2, Vec2}; @@ -55,10 +55,10 @@ impl WindowRef { } impl MapEntities for WindowRef { - fn map_entities(&mut self, entity_map: &EntityMap) -> Result<(), MapEntitiesError> { + fn map_entities(&mut self, entity_map: &mut EntityMapper) -> Result<(), MapEntitiesError> { match self { Self::Entity(entity) => { - *entity = entity_map.get(*entity)?; + *entity = entity_map.get_or_alloc(*entity); Ok(()) } Self::Primary => Ok(()), From c9bac3b70fcfec0ea66ffd29873c878c8f73a40c Mon Sep 17 00:00:00 2001 From: illiux Date: Sun, 22 Jan 2023 14:37:45 -0800 Subject: [PATCH 04/18] Address review feedback --- crates/bevy_ecs/src/entity/map_entities.rs | 80 ++++++++++++++-------- crates/bevy_ecs/src/reflect.rs | 14 ++-- crates/bevy_ecs/src/world/mod.rs | 16 ++--- crates/bevy_scene/src/dynamic_scene.rs | 3 +- 4 files changed, 67 insertions(+), 46 deletions(-) diff --git a/crates/bevy_ecs/src/entity/map_entities.rs b/crates/bevy_ecs/src/entity/map_entities.rs index 9bd686790174f..6c7ff6f7ca383 100644 --- a/crates/bevy_ecs/src/entity/map_entities.rs +++ b/crates/bevy_ecs/src/entity/map_entities.rs @@ -56,7 +56,7 @@ pub trait MapEntities { /// /// 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: &mut EntityMapper) -> Result<(), MapEntitiesError>; + fn map_entities(&mut self, entity_mapper: &mut EntityMapper) -> Result<(), MapEntitiesError>; } /// A mapping from one set of entities to another. @@ -70,23 +70,22 @@ pub struct EntityMap { map: HashMap, } +/// 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> { - /// Reserves the allocated dead entities within the world. - pub fn save(mut self, world: &mut World) { - if self.generations == 0 { - return; - } - - assert!(world.reserve_generations(self.dead_start, self.generations)); - self.generations = 0; - } - /// Returns the corresponding mapped entity. pub fn get(&self, entity: Entity) -> Result { self.map.get(entity) @@ -108,14 +107,37 @@ impl<'m> EntityMapper<'m> { new } -} -impl<'m> Drop for EntityMapper<'m> { - fn drop(&mut self) { - assert_eq!( - 0, self.generations, - "EntityMapper not saved before being dropped" - ); + /// 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, + dead_start: world.spawn_empty().id(), + generations: 0, + } + } + + /// Reserves the allocated references to dead entities within the world. This despawns the temporary base + /// [`Entity`] while reserving extra generations via [`World::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 save(self, world: &mut World) { + if self.generations == 0 { + assert!(world.despawn(self.dead_start)); + return; + } + + assert!(world.reserve_generations(self.dead_start, self.generations)); } } @@ -172,16 +194,16 @@ impl EntityMap { self.map.iter().map(|(from, to)| (*from, *to)) } - /// Gets an [`EntityMapper`] for allocating new dead entities. - /// - /// # Safety - /// If this function is called, [`EntityMapper::save()`] _must_ be subsequently called after mapping. - /// Otherwise, a panic may occur when the [`EntityMapper`] is dropped. - pub unsafe fn get_mapper<'m>(&'m mut self, world: &mut World) -> EntityMapper<'m> { - EntityMapper { - map: self, - dead_start: world.spawn_empty().id(), - generations: 0, - } + /// Calls the provided closure with an [`EntityMapper`] created from this [`EntityMap`]. This allows the closure + /// to allocate new entity references in the provided [`World`] that will never point at a living entity. + pub fn with_mapper( + &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.save(world); + result } } diff --git a/crates/bevy_ecs/src/reflect.rs b/crates/bevy_ecs/src/reflect.rs index 6eb459c12285f..1e9b4960d6dd3 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, MapEntitiesError}, system::Resource, world::{FromWorld, World}, }; @@ -406,7 +406,7 @@ impl_from_reflect_value!(Entity); #[derive(Clone)] pub struct ReflectMapEntities { - map_entities: fn(&mut World, &mut EntityMap) -> Result<(), MapEntitiesError>, + map_entities: fn(&mut World, &mut EntityMapper) -> Result<(), MapEntitiesError>, } impl ReflectMapEntities { @@ -415,23 +415,21 @@ impl ReflectMapEntities { world: &mut World, entity_map: &mut EntityMap, ) -> Result<(), MapEntitiesError> { - (self.map_entities)(world, entity_map) + entity_map.with_mapper(world, self.map_entities) } } impl FromType for ReflectMapEntities { fn from_type() -> Self { ReflectMapEntities { - map_entities: |world, entity_map| { - let entities = entity_map.values().collect::>(); + map_entities: |world, entity_mapper| { + let entities = entity_mapper.get_map().values().collect::>(); // SAFETY: We save to the world just after calling entity mappers - let mut mapper = unsafe { entity_map.get_mapper(world) }; for entity in entities.iter().cloned() { if let Some(mut component) = world.get_mut::(entity) { - component.map_entities(&mut mapper)?; + component.map_entities(entity_mapper)?; } } - mapper.save(world); Ok(()) }, } diff --git a/crates/bevy_ecs/src/world/mod.rs b/crates/bevy_ecs/src/world/mod.rs index 36629fd14ed67..4dd10bf3531a4 100644 --- a/crates/bevy_ecs/src/world/mod.rs +++ b/crates/bevy_ecs/src/world/mod.rs @@ -962,8 +962,8 @@ impl World { match self.get_resource() { Some(x) => x, None => panic!( - "Requested resource {} does not exist in the `World`. - Did you forget to add it using `app.insert_resource` / `app.init_resource`? + "Requested resource {} does not exist in the `World`. + Did you forget to add it using `app.insert_resource` / `app.init_resource`? Resources are also implicitly added via `app.add_event`, and can be added by plugins.", std::any::type_name::() @@ -986,8 +986,8 @@ impl World { match self.get_resource_mut() { Some(x) => x, None => panic!( - "Requested resource {} does not exist in the `World`. - Did you forget to add it using `app.insert_resource` / `app.init_resource`? + "Requested resource {} does not exist in the `World`. + Did you forget to add it using `app.insert_resource` / `app.init_resource`? Resources are also implicitly added via `app.add_event`, and can be added by plugins.", std::any::type_name::() @@ -1066,8 +1066,8 @@ impl World { match self.get_non_send_resource() { Some(x) => x, None => panic!( - "Requested non-send resource {} does not exist in the `World`. - Did you forget to add it using `app.insert_non_send_resource` / `app.init_non_send_resource`? + "Requested non-send resource {} does not exist in the `World`. + Did you forget to add it using `app.insert_non_send_resource` / `app.init_non_send_resource`? Non-send resources can also be be added by plugins.", std::any::type_name::() ), @@ -1088,8 +1088,8 @@ impl World { match self.get_non_send_resource_mut() { Some(x) => x, None => panic!( - "Requested non-send resource {} does not exist in the `World`. - Did you forget to add it using `app.insert_non_send_resource` / `app.init_non_send_resource`? + "Requested non-send resource {} does not exist in the `World`. + Did you forget to add it using `app.insert_non_send_resource` / `app.init_non_send_resource`? Non-send resources can also be be added by plugins.", std::any::type_name::() ), diff --git a/crates/bevy_scene/src/dynamic_scene.rs b/crates/bevy_scene/src/dynamic_scene.rs index 658cb57e25a91..8089c54ef6555 100644 --- a/crates/bevy_scene/src/dynamic_scene.rs +++ b/crates/bevy_scene/src/dynamic_scene.rs @@ -28,7 +28,8 @@ 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`. + /// The identifier of the entity, unique within a scene (and the world it may have been generated from). + /// Components containing entitiy references will identify their referant via this identifier. pub entity: u64, /// A vector of boxed components that belong to the given entity and /// implement the `Reflect` trait. From d9ac92a6020503572b0819fc1904ca32802bfaee Mon Sep 17 00:00:00 2001 From: illiux Date: Sun, 22 Jan 2023 15:13:41 -0800 Subject: [PATCH 05/18] Remove superfluous safety comment --- crates/bevy_ecs/src/reflect.rs | 1 - 1 file changed, 1 deletion(-) diff --git a/crates/bevy_ecs/src/reflect.rs b/crates/bevy_ecs/src/reflect.rs index 1e9b4960d6dd3..8895d68d69715 100644 --- a/crates/bevy_ecs/src/reflect.rs +++ b/crates/bevy_ecs/src/reflect.rs @@ -424,7 +424,6 @@ impl FromType for ReflectMapEntities { ReflectMapEntities { map_entities: |world, entity_mapper| { let entities = entity_mapper.get_map().values().collect::>(); - // SAFETY: We save to the world just after calling entity mappers for entity in entities.iter().cloned() { if let Some(mut component) = world.get_mut::(entity) { component.map_entities(entity_mapper)?; From 6e7e2b22a1a428674f64af5cd41d554baf7425dc Mon Sep 17 00:00:00 2001 From: illiux Date: Sun, 22 Jan 2023 16:14:32 -0800 Subject: [PATCH 06/18] Use Entity instead of u64 for entity ids in scenes --- assets/scenes/load_scene_example.scn.ron | 10 +++- crates/bevy_scene/src/dynamic_scene.rs | 6 +- .../bevy_scene/src/dynamic_scene_builder.rs | 32 ++++------- crates/bevy_scene/src/serde.rs | 57 ++++++++++++------- 4 files changed, 59 insertions(+), 46 deletions(-) diff --git a/assets/scenes/load_scene_example.scn.ron b/assets/scenes/load_scene_example.scn.ron index de4d3ce9280a0..4c3d3aac1e127 100644 --- a/assets/scenes/load_scene_example.scn.ron +++ b/assets/scenes/load_scene_example.scn.ron @@ -1,6 +1,9 @@ ( entities: { - 0: ( + ( + generation: 0, + index: 0, + ): ( components: { "bevy_transform::components::transform::Transform": ( translation: ( @@ -24,7 +27,10 @@ ), }, ), - 1: ( + ( + generation: 0, + index: 1, + ): ( components: { "scene::ComponentA": ( x: 3.0, diff --git a/crates/bevy_scene/src/dynamic_scene.rs b/crates/bevy_scene/src/dynamic_scene.rs index 8089c54ef6555..4c75334462f33 100644 --- a/crates/bevy_scene/src/dynamic_scene.rs +++ b/crates/bevy_scene/src/dynamic_scene.rs @@ -2,7 +2,7 @@ use crate::{DynamicSceneBuilder, Scene, SceneSpawnError}; use anyhow::Result; use bevy_app::AppTypeRegistry; use bevy_ecs::{ - entity::EntityMap, + entity::{Entity, EntityMap}, reflect::{ReflectComponent, ReflectMapEntities}, world::World, }; @@ -30,7 +30,7 @@ pub struct DynamicScene { pub struct DynamicEntity { /// The identifier of the entity, unique within a scene (and the world it may have been generated from). /// Components containing entitiy references will identify their referant via this identifier. - pub entity: u64, + pub entity: Entity, /// A vector of boxed components that belong to the given entity and /// implement the `Reflect` trait. pub components: Vec>, @@ -70,7 +70,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_bits(scene_entity.entity)) + .entry(scene_entity.entity) .or_insert_with(|| world.spawn_empty().id()); // Apply/ add each component to the given entity. diff --git a/crates/bevy_scene/src/dynamic_scene_builder.rs b/crates/bevy_scene/src/dynamic_scene_builder.rs index c1fe425ef9ac5..ba59ac1d6cf90 100644 --- a/crates/bevy_scene/src/dynamic_scene_builder.rs +++ b/crates/bevy_scene/src/dynamic_scene_builder.rs @@ -120,7 +120,7 @@ impl<'w> DynamicSceneBuilder<'w> { } let mut entry = DynamicEntity { - entity: entity.to_bits(), + entity, components: Vec::new(), }; @@ -180,7 +180,7 @@ mod tests { let scene = builder.build(); assert_eq!(scene.entities.len(), 1); - assert_eq!(scene.entities[0].entity, entity.to_bits()); + assert_eq!(scene.entities[0].entity, entity); assert_eq!(scene.entities[0].components.len(), 1); assert!(scene.entities[0].components[0].represents::()); } @@ -201,7 +201,7 @@ mod tests { let scene = builder.build(); assert_eq!(scene.entities.len(), 1); - assert_eq!(scene.entities[0].entity, entity.to_bits()); + assert_eq!(scene.entities[0].entity, entity); assert_eq!(scene.entities[0].components.len(), 1); assert!(scene.entities[0].components[0].represents::()); } @@ -225,7 +225,7 @@ mod tests { let scene = builder.build(); assert_eq!(scene.entities.len(), 1); - assert_eq!(scene.entities[0].entity, entity.to_bits()); + 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::()); @@ -252,22 +252,10 @@ mod tests { let mut entities = builder.build().entities.into_iter(); // Assert entities are ordered - assert_eq!( - entity_a.to_bits(), - entities.next().map(|e| e.entity).unwrap() - ); - assert_eq!( - entity_b.to_bits(), - entities.next().map(|e| e.entity).unwrap() - ); - assert_eq!( - entity_c.to_bits(), - entities.next().map(|e| e.entity).unwrap() - ); - assert_eq!( - entity_d.to_bits(), - 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] @@ -294,7 +282,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.to_bits(), entity_a.to_bits()]); + assert_eq!(scene_entities, [entity_a_b, entity_a]); } #[test] @@ -314,6 +302,6 @@ mod tests { let scene = builder.build(); assert_eq!(scene.entities.len(), 1); - assert_eq!(scene.entities[0].entity, entity_a.to_bits()); + assert_eq!(scene.entities[0].entity, entity_a); } } diff --git a/crates/bevy_scene/src/serde.rs b/crates/bevy_scene/src/serde.rs index bba7778ecdf1e..f30e65f2c553b 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}, @@ -229,7 +230,7 @@ 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(id) = map.next_key::()? { let entity = map.next_value_seed(SceneEntityDeserializer { id, type_registry: self.type_registry, @@ -242,7 +243,7 @@ impl<'a, 'de> Visitor<'de> for SceneEntitiesVisitor<'a> { } pub struct SceneEntityDeserializer<'a> { - pub id: u64, + pub id: Entity, pub type_registry: &'a TypeRegistry, } @@ -265,7 +266,7 @@ impl<'a, 'de> DeserializeSeed<'de> for SceneEntityDeserializer<'a> { } struct SceneEntityVisitor<'a> { - pub id: u64, + pub id: Entity, pub registry: &'a TypeRegistry, } @@ -484,18 +485,27 @@ mod tests { let expected = r#"( entities: { - 0: ( + ( + generation: 0, + index: 0, + ): ( components: { "bevy_scene::serde::tests::Foo": (123), }, ), - 1: ( + ( + generation: 0, + index: 1, + ): ( components: { "bevy_scene::serde::tests::Foo": (123), "bevy_scene::serde::tests::Bar": (345), }, ), - 2: ( + ( + generation: 0, + index: 2, + ): ( components: { "bevy_scene::serde::tests::Foo": (123), "bevy_scene::serde::tests::Bar": (345), @@ -516,18 +526,27 @@ mod tests { let input = r#"( entities: { - 0: ( + ( + generation: 0, + index: 0, + ): ( components: { "bevy_scene::serde::tests::Foo": (123), }, ), - 1: ( + ( + generation: 0, + index: 1, + ): ( components: { "bevy_scene::serde::tests::Foo": (123), "bevy_scene::serde::tests::Bar": (345), }, ), - 2: ( + ( + generation: 0, + index: 2, + ): ( components: { "bevy_scene::serde::tests::Foo": (123), "bevy_scene::serde::tests::Bar": (345), @@ -627,10 +646,10 @@ mod tests { assert_eq!( vec![ - 1, 0, 1, 37, 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, 2, 3, 102, 102, 166, 63, 205, 204, 108, 64, 1, 12, 72, 101, - 108, 108, 111, 32, 87, 111, 114, 108, 100, 33 + 1, 0, 0, 1, 37, 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, 2, 3, 102, 102, 166, 63, 205, 204, 108, 64, 1, 12, 72, + 101, 108, 108, 111, 32, 87, 111, 114, 108, 100, 33 ], serialized_scene ); @@ -667,11 +686,11 @@ mod tests { assert_eq!( vec![ - 145, 129, 0, 145, 129, 217, 37, 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, 147, 147, 1, 2, 3, 146, 202, 63, 166, 102, - 102, 202, 64, 108, 204, 205, 129, 165, 84, 117, 112, 108, 101, 172, 72, 101, 108, - 108, 111, 32, 87, 111, 114, 108, 100, 33 + 145, 129, 146, 0, 0, 145, 129, 217, 37, 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, 147, 147, 1, 2, 3, 146, 202, 63, + 166, 102, 102, 202, 64, 108, 204, 205, 129, 165, 84, 117, 112, 108, 101, 172, 72, + 101, 108, 108, 111, 32, 87, 111, 114, 108, 100, 33 ], buf ); @@ -744,7 +763,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",); From b2bb7bd46dd88fc504035a0b5bc0efaf25a3f950 Mon Sep 17 00:00:00 2001 From: illiux Date: Sun, 22 Jan 2023 17:37:22 -0800 Subject: [PATCH 07/18] Serialize Entity refs as u64 --- assets/scenes/load_scene_example.scn.ron | 10 +---- crates/bevy_ecs/src/entity/mod.rs | 21 ++++++++++- crates/bevy_scene/src/serde.rs | 48 ++++++++---------------- 3 files changed, 37 insertions(+), 42 deletions(-) diff --git a/assets/scenes/load_scene_example.scn.ron b/assets/scenes/load_scene_example.scn.ron index 4c3d3aac1e127..de4d3ce9280a0 100644 --- a/assets/scenes/load_scene_example.scn.ron +++ b/assets/scenes/load_scene_example.scn.ron @@ -1,9 +1,6 @@ ( entities: { - ( - generation: 0, - index: 0, - ): ( + 0: ( components: { "bevy_transform::components::transform::Transform": ( translation: ( @@ -27,10 +24,7 @@ ), }, ), - ( - generation: 0, - index: 1, - ): ( + 1: ( components: { "scene::ComponentA": ( x: 3.0, diff --git a/crates/bevy_ecs/src/entity/mod.rs b/crates/bevy_ecs/src/entity/mod.rs index f9f857a64bebb..a2469a9ace73d 100644 --- a/crates/bevy_ecs/src/entity/mod.rs +++ b/crates/bevy_ecs/src/entity/mod.rs @@ -110,7 +110,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, @@ -222,6 +222,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) diff --git a/crates/bevy_scene/src/serde.rs b/crates/bevy_scene/src/serde.rs index f30e65f2c553b..30c3641e1f1ed 100644 --- a/crates/bevy_scene/src/serde.rs +++ b/crates/bevy_scene/src/serde.rs @@ -485,27 +485,18 @@ mod tests { let expected = r#"( entities: { - ( - generation: 0, - index: 0, - ): ( + 0: ( components: { "bevy_scene::serde::tests::Foo": (123), }, ), - ( - generation: 0, - index: 1, - ): ( + 1: ( components: { "bevy_scene::serde::tests::Foo": (123), "bevy_scene::serde::tests::Bar": (345), }, ), - ( - generation: 0, - index: 2, - ): ( + 2: ( components: { "bevy_scene::serde::tests::Foo": (123), "bevy_scene::serde::tests::Bar": (345), @@ -526,27 +517,18 @@ mod tests { let input = r#"( entities: { - ( - generation: 0, - index: 0, - ): ( + 0: ( components: { "bevy_scene::serde::tests::Foo": (123), }, ), - ( - generation: 0, - index: 1, - ): ( + 1: ( components: { "bevy_scene::serde::tests::Foo": (123), "bevy_scene::serde::tests::Bar": (345), }, ), - ( - generation: 0, - index: 2, - ): ( + 2: ( components: { "bevy_scene::serde::tests::Foo": (123), "bevy_scene::serde::tests::Bar": (345), @@ -646,10 +628,10 @@ mod tests { assert_eq!( vec![ - 1, 0, 0, 1, 37, 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, 2, 3, 102, 102, 166, 63, 205, 204, 108, 64, 1, 12, 72, - 101, 108, 108, 111, 32, 87, 111, 114, 108, 100, 33 + 1, 0, 1, 37, 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, 2, 3, 102, 102, 166, 63, 205, 204, 108, 64, 1, 12, 72, 101, + 108, 108, 111, 32, 87, 111, 114, 108, 100, 33 ], serialized_scene ); @@ -686,11 +668,11 @@ mod tests { assert_eq!( vec![ - 145, 129, 146, 0, 0, 145, 129, 217, 37, 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, 147, 147, 1, 2, 3, 146, 202, 63, - 166, 102, 102, 202, 64, 108, 204, 205, 129, 165, 84, 117, 112, 108, 101, 172, 72, - 101, 108, 108, 111, 32, 87, 111, 114, 108, 100, 33 + 145, 129, 0, 145, 129, 217, 37, 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, 147, 147, 1, 2, 3, 146, 202, 63, 166, 102, + 102, 202, 64, 108, 204, 205, 129, 165, 84, 117, 112, 108, 101, 172, 72, 101, 108, + 108, 111, 32, 87, 111, 114, 108, 100, 33 ], buf ); From b78c47992e9dcdba0ce9b887f320bb8be3edb4d0 Mon Sep 17 00:00:00 2001 From: illiux Date: Mon, 23 Jan 2023 09:31:22 -0800 Subject: [PATCH 08/18] Remove serde macro dependency from bevy_ecs --- crates/bevy_ecs/Cargo.toml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/bevy_ecs/Cargo.toml b/crates/bevy_ecs/Cargo.toml index cff59621cf887..b6f88e29cc551 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" fxhash = "0.2" downcast-rs = "1.2" -serde = { version = "1", features = ["derive"] } +serde = "1" [dev-dependencies] rand = "0.8" From ed197b53725f444e14aa0ff5dcdd877f7dca3fbb Mon Sep 17 00:00:00 2001 From: illiux Date: Tue, 24 Jan 2023 09:48:13 -0800 Subject: [PATCH 09/18] Prefix reserve_generations with try_ --- crates/bevy_ecs/src/entity/map_entities.rs | 4 ++-- crates/bevy_ecs/src/world/mod.rs | 18 +++++++++--------- 2 files changed, 11 insertions(+), 11 deletions(-) diff --git a/crates/bevy_ecs/src/entity/map_entities.rs b/crates/bevy_ecs/src/entity/map_entities.rs index 6c7ff6f7ca383..a4e50fcb18ecd 100644 --- a/crates/bevy_ecs/src/entity/map_entities.rs +++ b/crates/bevy_ecs/src/entity/map_entities.rs @@ -128,7 +128,7 @@ impl<'m> EntityMapper<'m> { } /// Reserves the allocated references to dead entities within the world. This despawns the temporary base - /// [`Entity`] while reserving extra generations via [`World::reserve_generations`]. Because this renders the + /// [`Entity`] while reserving extra generations via [`World::try_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 save(self, world: &mut World) { @@ -137,7 +137,7 @@ impl<'m> EntityMapper<'m> { return; } - assert!(world.reserve_generations(self.dead_start, self.generations)); + assert!(world.try_reserve_generations(self.dead_start, self.generations)); } } diff --git a/crates/bevy_ecs/src/world/mod.rs b/crates/bevy_ecs/src/world/mod.rs index 4dd10bf3531a4..771ae5011707a 100644 --- a/crates/bevy_ecs/src/world/mod.rs +++ b/crates/bevy_ecs/src/world/mod.rs @@ -615,7 +615,7 @@ impl World { /// to dead entities into a new world alongside a [`crate::entity::MapEntities`] implementation. /// /// See [`Self::despawn()`] for usage. - pub(crate) fn reserve_generations(&mut self, entity: Entity, generations: u32) -> bool { + pub(crate) fn try_reserve_generations(&mut self, entity: Entity, generations: u32) -> bool { if self.despawn(entity) { self.entities .reserve_generations(entity.index(), generations) @@ -962,8 +962,8 @@ impl World { match self.get_resource() { Some(x) => x, None => panic!( - "Requested resource {} does not exist in the `World`. - Did you forget to add it using `app.insert_resource` / `app.init_resource`? + "Requested resource {} does not exist in the `World`. + Did you forget to add it using `app.insert_resource` / `app.init_resource`? Resources are also implicitly added via `app.add_event`, and can be added by plugins.", std::any::type_name::() @@ -986,8 +986,8 @@ impl World { match self.get_resource_mut() { Some(x) => x, None => panic!( - "Requested resource {} does not exist in the `World`. - Did you forget to add it using `app.insert_resource` / `app.init_resource`? + "Requested resource {} does not exist in the `World`. + Did you forget to add it using `app.insert_resource` / `app.init_resource`? Resources are also implicitly added via `app.add_event`, and can be added by plugins.", std::any::type_name::() @@ -1066,8 +1066,8 @@ impl World { match self.get_non_send_resource() { Some(x) => x, None => panic!( - "Requested non-send resource {} does not exist in the `World`. - Did you forget to add it using `app.insert_non_send_resource` / `app.init_non_send_resource`? + "Requested non-send resource {} does not exist in the `World`. + Did you forget to add it using `app.insert_non_send_resource` / `app.init_non_send_resource`? Non-send resources can also be be added by plugins.", std::any::type_name::() ), @@ -1088,8 +1088,8 @@ impl World { match self.get_non_send_resource_mut() { Some(x) => x, None => panic!( - "Requested non-send resource {} does not exist in the `World`. - Did you forget to add it using `app.insert_non_send_resource` / `app.init_non_send_resource`? + "Requested non-send resource {} does not exist in the `World`. + Did you forget to add it using `app.insert_non_send_resource` / `app.init_non_send_resource`? Non-send resources can also be be added by plugins.", std::any::type_name::() ), From 63f934daac7422522d22c7ee06811ad8e832ee4e Mon Sep 17 00:00:00 2001 From: illiux Date: Sat, 11 Feb 2023 14:28:19 -0800 Subject: [PATCH 10/18] Address PR feedback --- crates/bevy_ecs/src/entity/map_entities.rs | 72 ++++++++----------- crates/bevy_ecs/src/reflect.rs | 15 ++-- crates/bevy_ecs/src/world/mod.rs | 6 +- .../bevy_hierarchy/src/components/children.rs | 8 +-- .../bevy_hierarchy/src/components/parent.rs | 7 +- crates/bevy_render/src/mesh/mesh/skinning.rs | 8 +-- crates/bevy_scene/src/dynamic_scene.rs | 8 +-- .../bevy_scene/src/dynamic_scene_builder.rs | 4 +- crates/bevy_scene/src/scene.rs | 4 +- crates/bevy_scene/src/serde.rs | 7 +- crates/bevy_window/src/window.rs | 13 ++-- 11 files changed, 62 insertions(+), 90 deletions(-) diff --git a/crates/bevy_ecs/src/entity/map_entities.rs b/crates/bevy_ecs/src/entity/map_entities.rs index a4e50fcb18ecd..e08930fd070d4 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, 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::{EntityMapper, 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: &mut EntityMapper) -> 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_alloc(self.a); +/// self.b = entity_mapper.get_or_alloc(self.b); /// } /// } /// ``` @@ -56,15 +36,21 @@ pub trait MapEntities { /// /// 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_mapper: &mut EntityMapper) -> Result<(), MapEntitiesError>; + 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, @@ -86,14 +72,9 @@ pub struct EntityMapper<'m> { } impl<'m> EntityMapper<'m> { - /// Returns the corresponding mapped entity. - pub fn get(&self, entity: Entity) -> Result { - self.map.get(entity) - } - /// Returns the corresponding mapped entity or allocates a new dead entity if it is absent. pub fn get_or_alloc(&mut self, entity: Entity) -> Entity { - if let Ok(mapped) = self.map.get(entity) { + if let Some(mapped) = self.map.get(entity) { return mapped; } @@ -131,7 +112,7 @@ impl<'m> EntityMapper<'m> { /// [`Entity`] while reserving extra generations via [`World::try_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 save(self, world: &mut World) { + fn finish(self, world: &mut World) { if self.generations == 0 { assert!(world.despawn(self.dead_start)); return; @@ -162,11 +143,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).cloned() } /// An iterator visiting all keys in arbitrary order. @@ -194,16 +172,24 @@ impl EntityMap { self.map.iter().map(|(from, to)| (*from, *to)) } - /// Calls the provided closure with an [`EntityMapper`] created from this [`EntityMap`]. This allows the closure - /// to allocate new entity references in the provided [`World`] that will never point at a living entity. - pub fn with_mapper( + /// 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. + /// + /// # Arguments + /// + /// * `world` - The world the entities are being mapped into. + /// * `f` - A function to call 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.save(world); + mapper.finish(world); result } } diff --git a/crates/bevy_ecs/src/reflect.rs b/crates/bevy_ecs/src/reflect.rs index 3c567e9773caf..9fd36f213ded1 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, EntityMapper, MapEntities, MapEntitiesError}, + entity::{Entity, EntityMap, EntityMapper, MapEntities}, system::Resource, world::{ unsafe_world_cell::{UnsafeWorldCell, UnsafeWorldCellEntityRef}, @@ -409,16 +409,12 @@ impl_from_reflect_value!(Entity); #[derive(Clone)] pub struct ReflectMapEntities { - map_entities: fn(&mut World, &mut EntityMapper) -> Result<(), MapEntitiesError>, + map_entities: fn(&mut World, &mut EntityMapper), } impl ReflectMapEntities { - pub fn map_entities( - &self, - world: &mut World, - entity_map: &mut EntityMap, - ) -> Result<(), MapEntitiesError> { - entity_map.with_mapper(world, self.map_entities) + pub fn map_entities(&self, world: &mut World, entity_map: &mut EntityMap) { + entity_map.world_scope(world, self.map_entities); } } @@ -429,10 +425,9 @@ impl FromType for ReflectMapEntities { let entities = entity_mapper.get_map().values().collect::>(); for entity in entities.iter().cloned() { if let Some(mut component) = world.get_mut::(entity) { - component.map_entities(entity_mapper)?; + component.map_entities(entity_mapper); } } - Ok(()) }, } } diff --git a/crates/bevy_ecs/src/world/mod.rs b/crates/bevy_ecs/src/world/mod.rs index a8f86189a966b..c56f58bbca546 100644 --- a/crates/bevy_ecs/src/world/mod.rs +++ b/crates/bevy_ecs/src/world/mod.rs @@ -633,13 +633,13 @@ impl World { } } - /// Despawns the given `entity`, if it exists, and reserves a number of subsequent generations. + /// Despawns the given `entity`, if it exists, and reserves a number of subsequent generations. This will also + /// remove all of the entity's [Component]s. Returns `true` if the `entity` is successfully despawned and the + /// generations successfully reserved, and `false` if the `entity` does not exist or reservation failed. /// /// This function serves an extremely narrow use case of allocating a series of entity IDs that are /// guaranteed to never refer to a live entity. This functionality is useful primarily for mapping references /// to dead entities into a new world alongside a [`crate::entity::MapEntities`] implementation. - /// - /// See [`Self::despawn()`] for usage. pub(crate) fn try_reserve_generations(&mut self, entity: Entity, generations: u32) -> bool { if self.despawn(entity) { self.entities diff --git a/crates/bevy_hierarchy/src/components/children.rs b/crates/bevy_hierarchy/src/components/children.rs index 701291e0facd7..309ea9374065f 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, EntityMapper, 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: &mut EntityMapper) -> Result<(), MapEntitiesError> { + fn map_entities(&mut self, entity_mapper: &mut EntityMapper) { for entity in &mut self.0 { - *entity = entity_map.get_or_alloc(*entity); + *entity = entity_mapper.get_or_alloc(*entity); } - - Ok(()) } } diff --git a/crates/bevy_hierarchy/src/components/parent.rs b/crates/bevy_hierarchy/src/components/parent.rs index 845b3a901e1e4..e90c24a278fcf 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, EntityMapper, MapEntities, MapEntitiesError}, + entity::{Entity, EntityMapper, MapEntities}, reflect::{ReflectComponent, ReflectMapEntities}, world::{FromWorld, World}, }; @@ -36,9 +36,8 @@ impl FromWorld for Parent { } impl MapEntities for Parent { - fn map_entities(&mut self, entity_map: &mut EntityMapper) -> Result<(), MapEntitiesError> { - self.0 = entity_map.get_or_alloc(self.0); - Ok(()) + fn map_entities(&mut self, entity_mapper: &mut EntityMapper) { + self.0 = entity_mapper.get_or_alloc(self.0); } } diff --git a/crates/bevy_render/src/mesh/mesh/skinning.rs b/crates/bevy_render/src/mesh/mesh/skinning.rs index 51b2ef6a2a418..f58cb3827a3da 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, EntityMapper, 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: &mut EntityMapper) -> Result<(), MapEntitiesError> { + fn map_entities(&mut self, entity_mapper: &mut EntityMapper) { for joint in &mut self.joints { - *joint = entity_map.get_or_alloc(*joint); + *joint = entity_mapper.get_or_alloc(*joint); } - - Ok(()) } } diff --git a/crates/bevy_scene/src/dynamic_scene.rs b/crates/bevy_scene/src/dynamic_scene.rs index 3a6f958953bd9..b3a824a963223 100644 --- a/crates/bevy_scene/src/dynamic_scene.rs +++ b/crates/bevy_scene/src/dynamic_scene.rs @@ -29,7 +29,9 @@ pub struct DynamicScene { /// A reflection-powered serializable representation of an entity and its components. pub struct DynamicEntity { /// The identifier of the entity, unique within a scene (and the world it may have been generated from). - /// Components containing entitiy references will identify their referant via this identifier. + /// + /// Components in the scene that reference other entities reference them through this identifier, and so this + /// identifier must be consistent with the references contained within components. pub entity: Entity, /// A vector of boxed components that belong to the given entity and /// implement the `Reflect` trait. @@ -97,9 +99,7 @@ impl DynamicScene { for registration in type_registry.iter() { if let Some(map_entities_reflect) = registration.data::() { - map_entities_reflect - .map_entities(world, entity_map) - .unwrap(); + map_entities_reflect.map_entities(world, entity_map); } } diff --git a/crates/bevy_scene/src/dynamic_scene_builder.rs b/crates/bevy_scene/src/dynamic_scene_builder.rs index 5749b0e50b1d9..6dbdceeea4c9f 100644 --- a/crates/bevy_scene/src/dynamic_scene_builder.rs +++ b/crates/bevy_scene/src/dynamic_scene_builder.rs @@ -31,7 +31,7 @@ use std::collections::BTreeMap; /// let dynamic_scene = builder.build(); /// ``` pub struct DynamicSceneBuilder<'w> { - extracted_scene: BTreeMap, + extracted_scene: BTreeMap, type_registry: AppTypeRegistry, original_world: &'w World, } @@ -113,7 +113,7 @@ impl<'w> DynamicSceneBuilder<'w> { let type_registry = self.type_registry.read(); for entity in entities { - let index = entity.to_bits(); + let index = entity; if self.extracted_scene.contains_key(&index) { continue; diff --git a/crates/bevy_scene/src/scene.rs b/crates/bevy_scene/src/scene.rs index 496b6aac7fc0c..ffd2c7fa521d2 100644 --- a/crates/bevy_scene/src/scene.rs +++ b/crates/bevy_scene/src/scene.rs @@ -92,9 +92,7 @@ impl Scene { } for registration in type_registry.iter() { if let Some(map_entities_reflect) = registration.data::() { - map_entities_reflect - .map_entities(world, &mut 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 30c3641e1f1ed..0dce3c68c1f5a 100644 --- a/crates/bevy_scene/src/serde.rs +++ b/crates/bevy_scene/src/serde.rs @@ -394,7 +394,7 @@ mod tests { use crate::serde::{SceneDeserializer, SceneSerializer}; use crate::{DynamicScene, DynamicSceneBuilder}; use bevy_app::AppTypeRegistry; - use bevy_ecs::entity::{Entity, EntityMap, EntityMapper, MapEntities, MapEntitiesError}; + use bevy_ecs::entity::{Entity, EntityMap, EntityMapper, MapEntities}; use bevy_ecs::prelude::{Component, ReflectComponent, World}; use bevy_ecs::query::{With, Without}; use bevy_ecs::reflect::ReflectMapEntities; @@ -438,9 +438,8 @@ mod tests { struct MyEntityRef(Entity); impl MapEntities for MyEntityRef { - fn map_entities(&mut self, entity_map: &mut EntityMapper) -> Result<(), MapEntitiesError> { - self.0 = entity_map.get_or_alloc(self.0); - Ok(()) + fn map_entities(&mut self, entity_mapper: &mut EntityMapper) { + self.0 = entity_mapper.get_or_alloc(self.0); } } diff --git a/crates/bevy_window/src/window.rs b/crates/bevy_window/src/window.rs index 16c1ea02e20b5..bdcb609a7a9bd 100644 --- a/crates/bevy_window/src/window.rs +++ b/crates/bevy_window/src/window.rs @@ -1,5 +1,5 @@ use bevy_ecs::{ - entity::{Entity, EntityMapper, 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: &mut EntityMapper) -> Result<(), MapEntitiesError> { + fn map_entities(&mut self, entity_mapper: &mut EntityMapper) { match self { Self::Entity(entity) => { - *entity = entity_map.get_or_alloc(*entity); - Ok(()) + *entity = entity_mapper.get_or_alloc(*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. From 2836525bd1e60e12595f991bda022f0c57484497 Mon Sep 17 00:00:00 2001 From: illiux Date: Sat, 11 Feb 2023 15:17:02 -0800 Subject: [PATCH 11/18] Add some unit tests --- crates/bevy_ecs/src/entity/map_entities.rs | 48 ++++++++++++++++++++++ crates/bevy_ecs/src/entity/mod.rs | 9 ++++ crates/bevy_ecs/src/world/mod.rs | 24 +++++++++++ 3 files changed, 81 insertions(+) diff --git a/crates/bevy_ecs/src/entity/map_entities.rs b/crates/bevy_ecs/src/entity/map_entities.rs index e08930fd070d4..b2e6faffa2ccd 100644 --- a/crates/bevy_ecs/src/entity/map_entities.rs +++ b/crates/bevy_ecs/src/entity/map_entities.rs @@ -193,3 +193,51 @@ impl EntityMap { result } } + +#[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_alloc(mapped_ent); + + // Should persist the allocated mapping from the previous line + assert_eq!(dead_ref, mapper.get_or_alloc(mapped_ent)); + // Should re-use the same index for further dead refs + assert_eq!( + mapper.get_or_alloc(Entity::new(SECOND_IDX, 0)).index(), + dead_ref.index() + ); + + 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_alloc(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 a2469a9ace73d..f535a97d13415 100644 --- a/crates/bevy_ecs/src/entity/mod.rs +++ b/crates/bevy_ecs/src/entity/mod.rs @@ -862,4 +862,13 @@ 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)); + } } diff --git a/crates/bevy_ecs/src/world/mod.rs b/crates/bevy_ecs/src/world/mod.rs index c56f58bbca546..89eac89c6b6c5 100644 --- a/crates/bevy_ecs/src/world/mod.rs +++ b/crates/bevy_ecs/src/world/mod.rs @@ -2194,4 +2194,28 @@ mod tests { let mut world = World::new(); world.spawn(()); } + + #[test] + fn try_reserve_generations_on_despawned() { + let mut world = World::new(); + let entity = world.spawn_empty().id(); + world.despawn(entity); + + assert!(!world.try_reserve_generations(entity, 1)); + } + + #[test] + fn reserve_generations_and_alloc() { + const GENERATIONS: u32 = 10; + + let mut world = World::new(); + let entity = world.spawn_empty().id(); + + assert!(world.try_reserve_generations(entity, GENERATIONS)); + + // The very next entity allocated should be a further generation on the same index + let next_entity = world.spawn_empty().id(); + assert_eq!(next_entity.index(), entity.index()); + assert!(next_entity.generation() > entity.generation() + GENERATIONS); + } } From 151628d2ca0cfc9905227a36ce49678bad021b7f Mon Sep 17 00:00:00 2001 From: Illiux Date: Sat, 25 Feb 2023 13:15:28 -0800 Subject: [PATCH 12/18] Apply suggestions from code review Co-authored-by: Nicola Papale --- crates/bevy_ecs/src/entity/map_entities.rs | 2 +- crates/bevy_ecs/src/reflect.rs | 4 ++-- crates/bevy_window/src/window.rs | 2 +- 3 files changed, 4 insertions(+), 4 deletions(-) diff --git a/crates/bevy_ecs/src/entity/map_entities.rs b/crates/bevy_ecs/src/entity/map_entities.rs index b2e6faffa2ccd..94cc77b960f46 100644 --- a/crates/bevy_ecs/src/entity/map_entities.rs +++ b/crates/bevy_ecs/src/entity/map_entities.rs @@ -144,7 +144,7 @@ impl EntityMap { /// Returns the corresponding mapped entity. pub fn get(&self, entity: Entity) -> Option { - self.map.get(&entity).cloned() + self.map.get(&entity).copied() } /// An iterator visiting all keys in arbitrary order. diff --git a/crates/bevy_ecs/src/reflect.rs b/crates/bevy_ecs/src/reflect.rs index e4ba6da2756b1..01c76fbc5df4e 100644 --- a/crates/bevy_ecs/src/reflect.rs +++ b/crates/bevy_ecs/src/reflect.rs @@ -422,8 +422,8 @@ impl FromType for ReflectMapEntities { ReflectMapEntities { map_entities: |world, entity_mapper| { let entities = entity_mapper.get_map().values().collect::>(); - for entity in entities.iter().cloned() { - if let Some(mut component) = world.get_mut::(entity) { + for entity in &entities { + if let Some(mut component) = world.get_mut::(*entity) { component.map_entities(entity_mapper); } } diff --git a/crates/bevy_window/src/window.rs b/crates/bevy_window/src/window.rs index bdcb609a7a9bd..f68e0571b79a7 100644 --- a/crates/bevy_window/src/window.rs +++ b/crates/bevy_window/src/window.rs @@ -60,7 +60,7 @@ impl MapEntities for WindowRef { Self::Entity(entity) => { *entity = entity_mapper.get_or_alloc(*entity); } - Self::Primary => (), + Self::Primary => {}, }; } } From cff1d80879b5c38d4acc10ddea0965ebe03325fe Mon Sep 17 00:00:00 2001 From: illiux Date: Sat, 25 Feb 2023 13:15:13 -0800 Subject: [PATCH 13/18] Address PR feedback --- crates/bevy_ecs/src/entity/map_entities.rs | 163 +++++++++--------- crates/bevy_ecs/src/entity/mod.rs | 23 ++- crates/bevy_ecs/src/world/mod.rs | 40 ----- .../bevy_hierarchy/src/components/children.rs | 2 +- .../bevy_hierarchy/src/components/parent.rs | 2 +- crates/bevy_render/src/mesh/mesh/skinning.rs | 2 +- crates/bevy_scene/src/serde.rs | 10 +- crates/bevy_window/src/window.rs | 2 +- 8 files changed, 111 insertions(+), 133 deletions(-) diff --git a/crates/bevy_ecs/src/entity/map_entities.rs b/crates/bevy_ecs/src/entity/map_entities.rs index 94cc77b960f46..11af4b5f867cc 100644 --- a/crates/bevy_ecs/src/entity/map_entities.rs +++ b/crates/bevy_ecs/src/entity/map_entities.rs @@ -24,8 +24,8 @@ use bevy_utils::{Entry, HashMap}; /// /// impl MapEntities for Spring { /// fn map_entities(&mut self, entity_mapper: &mut EntityMapper) { -/// self.a = entity_mapper.get_or_alloc(self.a); -/// self.b = entity_mapper.get_or_alloc(self.b); +/// self.a = entity_mapper.get_or_reserve(self.a); +/// self.b = entity_mapper.get_or_reserve(self.b); /// } /// } /// ``` @@ -56,72 +56,6 @@ pub struct EntityMap { map: HashMap, } -/// 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 allocates a new dead entity if it is absent. - pub fn get_or_alloc(&mut self, entity: Entity) -> Entity { - if let Some(mapped) = self.map.get(entity) { - return mapped; - } - - 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, - dead_start: world.spawn_empty().id(), - generations: 0, - } - } - - /// Reserves the allocated references to dead entities within the world. This despawns the temporary base - /// [`Entity`] while reserving extra generations via [`World::try_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) { - if self.generations == 0 { - assert!(world.despawn(self.dead_start)); - return; - } - - assert!(world.try_reserve_generations(self.dead_start, self.generations)); - } -} - impl EntityMap { /// Inserts an entities pair into the map. /// @@ -175,13 +109,9 @@ impl EntityMap { /// 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. - /// - /// # Arguments - /// - /// * `world` - The world the entities are being mapped into. - /// * `f` - A function to call within the scope of the passed world. Its return value is then returned from - /// `world_scope` as the generic type parameter `R`. + /// 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, @@ -194,6 +124,72 @@ impl EntityMap { } } +/// 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}; @@ -209,14 +205,17 @@ mod tests { let mut mapper = EntityMapper::new(&mut map, &mut world); let mapped_ent = Entity::new(FIRST_IDX, 0); - let dead_ref = mapper.get_or_alloc(mapped_ent); + let dead_ref = mapper.get_or_reserve(mapped_ent); - // Should persist the allocated mapping from the previous line - assert_eq!(dead_ref, mapper.get_or_alloc(mapped_ent)); - // Should re-use the same index for further dead refs assert_eq!( - mapper.get_or_alloc(Entity::new(SECOND_IDX, 0)).index(), - dead_ref.index() + 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); @@ -232,7 +231,7 @@ mod tests { let mut world = World::new(); let dead_ref = map.world_scope(&mut world, |_, mapper| { - mapper.get_or_alloc(Entity::new(0, 0)) + mapper.get_or_reserve(Entity::new(0, 0)) }); // Next allocated entity should be a further generation on the same index diff --git a/crates/bevy_ecs/src/entity/mod.rs b/crates/bevy_ecs/src/entity/mod.rs index 7ed106af499b1..22f7e5c80a995 100644 --- a/crates/bevy_ecs/src/entity/mod.rs +++ b/crates/bevy_ecs/src/entity/mod.rs @@ -615,8 +615,11 @@ impl Entities { self.meta.get_unchecked_mut(index as usize).location = location; } - /// Increments the generation of a despawned [`Entity`]. This may only be called on an entity - /// index corresponding to an entity that once existed and has since been freed. + /// 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; @@ -883,4 +886,20 @@ mod tests { 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/world/mod.rs b/crates/bevy_ecs/src/world/mod.rs index 9647ce1b6b9bf..a6a25e28bb3cd 100644 --- a/crates/bevy_ecs/src/world/mod.rs +++ b/crates/bevy_ecs/src/world/mod.rs @@ -633,22 +633,6 @@ impl World { } } - /// Despawns the given `entity`, if it exists, and reserves a number of subsequent generations. This will also - /// remove all of the entity's [Component]s. Returns `true` if the `entity` is successfully despawned and the - /// generations successfully reserved, and `false` if the `entity` does not exist or reservation failed. - /// - /// This function serves an extremely narrow use case of allocating a series of entity IDs that are - /// guaranteed to never refer to a live entity. This functionality is useful primarily for mapping references - /// to dead entities into a new world alongside a [`crate::entity::MapEntities`] implementation. - pub(crate) fn try_reserve_generations(&mut self, entity: Entity, generations: u32) -> bool { - if self.despawn(entity) { - self.entities - .reserve_generations(entity.index(), generations) - } else { - false - } - } - /// Clears the internal component tracker state. /// /// The world maintains some internal state about changed and removed components. This state @@ -2192,28 +2176,4 @@ mod tests { let mut world = World::new(); world.spawn(()); } - - #[test] - fn try_reserve_generations_on_despawned() { - let mut world = World::new(); - let entity = world.spawn_empty().id(); - world.despawn(entity); - - assert!(!world.try_reserve_generations(entity, 1)); - } - - #[test] - fn reserve_generations_and_alloc() { - const GENERATIONS: u32 = 10; - - let mut world = World::new(); - let entity = world.spawn_empty().id(); - - assert!(world.try_reserve_generations(entity, GENERATIONS)); - - // The very next entity allocated should be a further generation on the same index - let next_entity = world.spawn_empty().id(); - assert_eq!(next_entity.index(), entity.index()); - assert!(next_entity.generation() > entity.generation() + GENERATIONS); - } } diff --git a/crates/bevy_hierarchy/src/components/children.rs b/crates/bevy_hierarchy/src/components/children.rs index 309ea9374065f..7a76d91b4c679 100644 --- a/crates/bevy_hierarchy/src/components/children.rs +++ b/crates/bevy_hierarchy/src/components/children.rs @@ -23,7 +23,7 @@ pub struct Children(pub(crate) SmallVec<[Entity; 8]>); impl MapEntities for Children { fn map_entities(&mut self, entity_mapper: &mut EntityMapper) { for entity in &mut self.0 { - *entity = entity_mapper.get_or_alloc(*entity); + *entity = entity_mapper.get_or_reserve(*entity); } } } diff --git a/crates/bevy_hierarchy/src/components/parent.rs b/crates/bevy_hierarchy/src/components/parent.rs index e90c24a278fcf..7bccf33ed6e1e 100644 --- a/crates/bevy_hierarchy/src/components/parent.rs +++ b/crates/bevy_hierarchy/src/components/parent.rs @@ -37,7 +37,7 @@ impl FromWorld for Parent { impl MapEntities for Parent { fn map_entities(&mut self, entity_mapper: &mut EntityMapper) { - self.0 = entity_mapper.get_or_alloc(self.0); + 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 f58cb3827a3da..a124655df15c7 100644 --- a/crates/bevy_render/src/mesh/mesh/skinning.rs +++ b/crates/bevy_render/src/mesh/mesh/skinning.rs @@ -19,7 +19,7 @@ pub struct SkinnedMesh { impl MapEntities for SkinnedMesh { fn map_entities(&mut self, entity_mapper: &mut EntityMapper) { for joint in &mut self.joints { - *joint = entity_mapper.get_or_alloc(*joint); + *joint = entity_mapper.get_or_reserve(*joint); } } } diff --git a/crates/bevy_scene/src/serde.rs b/crates/bevy_scene/src/serde.rs index 0dce3c68c1f5a..14e7e10604bbf 100644 --- a/crates/bevy_scene/src/serde.rs +++ b/crates/bevy_scene/src/serde.rs @@ -433,13 +433,13 @@ mod tests { }, } - #[derive(Component, Reflect, PartialEq)] + #[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_alloc(self.0); + self.0 = entity_mapper.get_or_reserve(self.0); } } @@ -594,14 +594,14 @@ mod tests { let bar_to_foo = dst_world .query_filtered::<&MyEntityRef, Without>() .get_single(&dst_world) - .unwrap() - .0; + .cloned() + .unwrap(); let foo = dst_world .query_filtered::>() .get_single(&dst_world) .unwrap(); - assert_eq!(foo, bar_to_foo); + assert_eq!(foo, bar_to_foo.0); assert!(dst_world .query_filtered::<&MyEntityRef, With>() .iter(&dst_world) diff --git a/crates/bevy_window/src/window.rs b/crates/bevy_window/src/window.rs index f68e0571b79a7..6eab3ee139534 100644 --- a/crates/bevy_window/src/window.rs +++ b/crates/bevy_window/src/window.rs @@ -58,7 +58,7 @@ impl MapEntities for WindowRef { fn map_entities(&mut self, entity_mapper: &mut EntityMapper) { match self { Self::Entity(entity) => { - *entity = entity_mapper.get_or_alloc(*entity); + *entity = entity_mapper.get_or_reserve(*entity); } Self::Primary => {}, }; From 2e1dcc2efdda20d75980ba7f6437f9fe5b4c106f Mon Sep 17 00:00:00 2001 From: illiux Date: Sat, 25 Feb 2023 13:25:56 -0800 Subject: [PATCH 14/18] Address comments --- crates/bevy_ecs/src/entity/map_entities.rs | 2 +- crates/bevy_window/src/window.rs | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/crates/bevy_ecs/src/entity/map_entities.rs b/crates/bevy_ecs/src/entity/map_entities.rs index 11af4b5f867cc..739635f5325ab 100644 --- a/crates/bevy_ecs/src/entity/map_entities.rs +++ b/crates/bevy_ecs/src/entity/map_entities.rs @@ -35,7 +35,7 @@ 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`. + /// update them to the mapped values via `entity_mapper`. fn map_entities(&mut self, entity_mapper: &mut EntityMapper); } diff --git a/crates/bevy_window/src/window.rs b/crates/bevy_window/src/window.rs index 6eab3ee139534..4d4124b33ffad 100644 --- a/crates/bevy_window/src/window.rs +++ b/crates/bevy_window/src/window.rs @@ -60,7 +60,7 @@ impl MapEntities for WindowRef { Self::Entity(entity) => { *entity = entity_mapper.get_or_reserve(*entity); } - Self::Primary => {}, + Self::Primary => {} }; } } From 953fd8a7aeac80a46b1167f66a310eb990eca0ac Mon Sep 17 00:00:00 2001 From: illiux Date: Thu, 2 Mar 2023 20:40:39 -0800 Subject: [PATCH 15/18] Address feedback --- crates/bevy_scene/src/dynamic_scene.rs | 3 +-- crates/bevy_scene/src/dynamic_scene_builder.rs | 8 +++----- 2 files changed, 4 insertions(+), 7 deletions(-) diff --git a/crates/bevy_scene/src/dynamic_scene.rs b/crates/bevy_scene/src/dynamic_scene.rs index b3a824a963223..bd387348f15e8 100644 --- a/crates/bevy_scene/src/dynamic_scene.rs +++ b/crates/bevy_scene/src/dynamic_scene.rs @@ -30,8 +30,7 @@ pub struct DynamicScene { pub struct DynamicEntity { /// The identifier of the entity, unique within a scene (and the world it may have been generated from). /// - /// Components in the scene that reference other entities reference them through this identifier, and so this - /// identifier must be consistent with the references contained within components. + /// 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. diff --git a/crates/bevy_scene/src/dynamic_scene_builder.rs b/crates/bevy_scene/src/dynamic_scene_builder.rs index 6dbdceeea4c9f..60500b7962ba6 100644 --- a/crates/bevy_scene/src/dynamic_scene_builder.rs +++ b/crates/bevy_scene/src/dynamic_scene_builder.rs @@ -112,19 +112,17 @@ impl<'w> DynamicSceneBuilder<'w> { pub fn extract_entities(&mut self, entities: impl Iterator) -> &mut Self { let type_registry = self.type_registry.read(); - for entity in entities { - let index = entity; - + for index in entities { if self.extracted_scene.contains_key(&index) { continue; } let mut entry = DynamicEntity { - entity, + entity: index, components: Vec::new(), }; - let entity = self.original_world.entity(entity); + let entity = self.original_world.entity(index); for component_id in entity.archetype().components() { let reflect_component = self .original_world From 066c36ce328db1ff73bcd1914f1dde68cae9d917 Mon Sep 17 00:00:00 2001 From: illiux Date: Sun, 30 Apr 2023 16:01:31 -0700 Subject: [PATCH 16/18] Undo changes in world/mod.rs --- crates/bevy_ecs/src/world/mod.rs | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/crates/bevy_ecs/src/world/mod.rs b/crates/bevy_ecs/src/world/mod.rs index 19e47bf08899a..f270e85105915 100644 --- a/crates/bevy_ecs/src/world/mod.rs +++ b/crates/bevy_ecs/src/world/mod.rs @@ -972,8 +972,8 @@ impl World { match self.get_resource() { Some(x) => x, None => panic!( - "Requested resource {} does not exist in the `World`. - Did you forget to add it using `app.insert_resource` / `app.init_resource`? + "Requested resource {} does not exist in the `World`. + Did you forget to add it using `app.insert_resource` / `app.init_resource`? Resources are also implicitly added via `app.add_event`, and can be added by plugins.", std::any::type_name::() @@ -996,8 +996,8 @@ impl World { match self.get_resource_mut() { Some(x) => x, None => panic!( - "Requested resource {} does not exist in the `World`. - Did you forget to add it using `app.insert_resource` / `app.init_resource`? + "Requested resource {} does not exist in the `World`. + Did you forget to add it using `app.insert_resource` / `app.init_resource`? Resources are also implicitly added via `app.add_event`, and can be added by plugins.", std::any::type_name::() @@ -1067,8 +1067,8 @@ impl World { match self.get_non_send_resource() { Some(x) => x, None => panic!( - "Requested non-send resource {} does not exist in the `World`. - Did you forget to add it using `app.insert_non_send_resource` / `app.init_non_send_resource`? + "Requested non-send resource {} does not exist in the `World`. + Did you forget to add it using `app.insert_non_send_resource` / `app.init_non_send_resource`? Non-send resources can also be be added by plugins.", std::any::type_name::() ), @@ -1089,8 +1089,8 @@ impl World { match self.get_non_send_resource_mut() { Some(x) => x, None => panic!( - "Requested non-send resource {} does not exist in the `World`. - Did you forget to add it using `app.insert_non_send_resource` / `app.init_non_send_resource`? + "Requested non-send resource {} does not exist in the `World`. + Did you forget to add it using `app.insert_non_send_resource` / `app.init_non_send_resource`? Non-send resources can also be be added by plugins.", std::any::type_name::() ), From 327ba49e8c73427dc4370855c8e32fbcd88a14bd Mon Sep 17 00:00:00 2001 From: illiux Date: Sun, 30 Apr 2023 16:15:32 -0700 Subject: [PATCH 17/18] Rename various variables from "index" to "entity" --- crates/bevy_scene/src/dynamic_scene_builder.rs | 14 +++++++------- crates/bevy_scene/src/serde.rs | 8 ++++---- 2 files changed, 11 insertions(+), 11 deletions(-) diff --git a/crates/bevy_scene/src/dynamic_scene_builder.rs b/crates/bevy_scene/src/dynamic_scene_builder.rs index 8cf5cb769c528..b19d0a0993999 100644 --- a/crates/bevy_scene/src/dynamic_scene_builder.rs +++ b/crates/bevy_scene/src/dynamic_scene_builder.rs @@ -122,18 +122,18 @@ impl<'w> DynamicSceneBuilder<'w> { pub fn extract_entities(&mut self, entities: impl Iterator) -> &mut Self { let type_registry = self.type_registry.read(); - for index in entities { - if self.extracted_scene.contains_key(&index) { + for entity in entities { + if self.extracted_scene.contains_key(&entity) { continue; } let mut entry = DynamicEntity { - entity: index, + entity, components: Vec::new(), }; - let entity = self.original_world.entity(index); - 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 @@ -143,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); diff --git a/crates/bevy_scene/src/serde.rs b/crates/bevy_scene/src/serde.rs index 5d59a3c31ad53..f386f1a4809c0 100644 --- a/crates/bevy_scene/src/serde.rs +++ b/crates/bevy_scene/src/serde.rs @@ -261,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); @@ -274,7 +274,7 @@ impl<'a, 'de> Visitor<'de> for SceneEntitiesVisitor<'a> { } pub struct SceneEntityDeserializer<'a> { - pub id: Entity, + pub entity: Entity, pub type_registry: &'a TypeRegistry, } @@ -289,7 +289,7 @@ impl<'a, 'de> DeserializeSeed<'de> for SceneEntityDeserializer<'a> { ENTITY_STRUCT, &[ENTITY_FIELD_COMPONENTS], SceneEntityVisitor { - id: self.id, + id: self.entity, registry: self.type_registry, }, ) From f5ea4d93009d5c0d1e3e1fb6373cf1d959732a1e Mon Sep 17 00:00:00 2001 From: illiux Date: Sun, 30 Apr 2023 18:59:54 -0700 Subject: [PATCH 18/18] Rename vars in serde.rs --- crates/bevy_scene/src/serde.rs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/crates/bevy_scene/src/serde.rs b/crates/bevy_scene/src/serde.rs index f386f1a4809c0..635feee649230 100644 --- a/crates/bevy_scene/src/serde.rs +++ b/crates/bevy_scene/src/serde.rs @@ -289,7 +289,7 @@ impl<'a, 'de> DeserializeSeed<'de> for SceneEntityDeserializer<'a> { ENTITY_STRUCT, &[ENTITY_FIELD_COMPONENTS], SceneEntityVisitor { - id: self.entity, + entity: self.entity, registry: self.type_registry, }, ) @@ -297,7 +297,7 @@ impl<'a, 'de> DeserializeSeed<'de> for SceneEntityDeserializer<'a> { } struct SceneEntityVisitor<'a> { - pub id: Entity, + pub entity: Entity, pub registry: &'a TypeRegistry, } @@ -319,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, }) } @@ -347,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, }) }