Skip to content

Commit

Permalink
Use EntityHashMap for EntityMapper (#10415)
Browse files Browse the repository at this point in the history
# Objective

- There is a specialized hasher for entities:
[`EntityHashMap`](https://docs.rs/bevy/latest/bevy/utils/type.EntityHashMap.html)
- [`EntityMapper`] currently uses a normal `HashMap<Entity, Entity>`
- Fixes #10391

## Solution

- Replace the normal `HashMap` with the more performant `EntityHashMap`

## Questions

- This does change public API. Should a system be implemented to help
migrate code?
  - Perhaps an `impl From<HashMap<K, V, S>> for EntityHashMap<K, V>`
- I updated to docs for each function that I changed, but I may have
missed something

---

## Changelog

- Changed `EntityMapper` to use `EntityHashMap` instead of normal
`HashMap`

## Migration Guide

If you are using the following types, update their listed methods to use
the new `EntityHashMap`. `EntityHashMap` has the same methods as the
normal `HashMap`, so you just need to replace the name.

### `EntityMapper`

- `get_map`
- `get_mut_map`
- `new`
- `world_scope`

### `ReflectMapEntities`

- `map_all_entities`
- `map_entities`
- `write_to_world`

### `InstanceInfo`

- `entity_map`
  - This is a property, not a method.

---

This is my first time contributing in a while, and I'm not familiar with
the usage of `EntityMapper`. I changed the type definition and fixed all
errors, but there may have been things I've missed. Please keep an eye
out for me!
  • Loading branch information
BD103 authored Nov 7, 2023
1 parent 6ce33d0 commit 04ceb46
Show file tree
Hide file tree
Showing 6 changed files with 43 additions and 39 deletions.
30 changes: 15 additions & 15 deletions crates/bevy_ecs/src/entity/map_entities.rs
Original file line number Diff line number Diff line change
@@ -1,11 +1,11 @@
use crate::{entity::Entity, world::World};
use bevy_utils::HashMap;
use bevy_utils::EntityHashMap;

/// Operation to map all contained [`Entity`] fields in a type to new values.
///
/// As entity IDs are valid only for the [`World`] they're sourced from, using [`Entity`]
/// as references in components copied from another world will be invalid. This trait
/// allows defining custom mappings for these references via [`HashMap<Entity, Entity>`]
/// allows defining custom mappings for these references via [`EntityHashMap<Entity, Entity>`]
///
/// Implementing this trait correctly is required for properly loading components
/// with entity references from scenes.
Expand Down Expand Up @@ -39,7 +39,7 @@ pub trait MapEntities {
fn map_entities(&mut self, entity_mapper: &mut EntityMapper);
}

/// A wrapper for [`HashMap<Entity, Entity>`], augmenting it with the ability to allocate new [`Entity`] references in a destination
/// A wrapper for [`EntityHashMap<Entity, Entity>`], 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
Expand All @@ -52,9 +52,9 @@ pub struct EntityMapper<'m> {
/// 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, a [`HashMap<Entity, Entity>`] is not capable of allocating new entity identifiers, which is needed to map references
/// On its own, a [`EntityHashMap<Entity, Entity>`] is not capable of allocating new entity identifiers, which is needed to map references
/// to entities that lie outside the source entity set. This functionality can be accessed through [`EntityMapper::world_scope()`].
map: &'m mut HashMap<Entity, Entity>,
map: &'m mut EntityHashMap<Entity, Entity>,
/// A base [`Entity`] used to allocate new references.
dead_start: Entity,
/// The number of generations this mapper has allocated thus far.
Expand All @@ -80,18 +80,18 @@ impl<'m> EntityMapper<'m> {
new
}

/// Gets a reference to the underlying [`HashMap<Entity, Entity>`].
pub fn get_map(&'m self) -> &'m HashMap<Entity, Entity> {
/// Gets a reference to the underlying [`EntityHashMap<Entity, Entity>`].
pub fn get_map(&'m self) -> &'m EntityHashMap<Entity, Entity> {
self.map
}

/// Gets a mutable reference to the underlying [`HashMap<Entity, Entity>`].
pub fn get_map_mut(&'m mut self) -> &'m mut HashMap<Entity, Entity> {
/// Gets a mutable reference to the underlying [`EntityHashMap<Entity, Entity>`].
pub fn get_map_mut(&'m mut self) -> &'m mut EntityHashMap<Entity, Entity> {
self.map
}

/// Creates a new [`EntityMapper`], spawning a temporary base [`Entity`] in the provided [`World`]
fn new(map: &'m mut HashMap<Entity, Entity>, world: &mut World) -> Self {
fn new(map: &'m mut EntityHashMap<Entity, Entity>, world: &mut World) -> Self {
Self {
map,
// SAFETY: Entities data is kept in a valid state via `EntityMapper::world_scope`
Expand All @@ -111,14 +111,14 @@ impl<'m> EntityMapper<'m> {
assert!(entities.reserve_generations(self.dead_start.index, self.generations));
}

/// Creates an [`EntityMapper`] from a provided [`World`] and [`HashMap<Entity, Entity>`], then calls the
/// Creates an [`EntityMapper`] from a provided [`World`] and [`EntityHashMap<Entity, Entity>`], then calls the
/// provided function with it. This allows one to allocate new entity references in this [`World`] that are
/// guaranteed to never point at a living entity now or in the future. This functionality is useful for safely
/// mapping entity identifiers that point at entities outside the source world. The passed function, `f`, is called
/// within the scope of this world. Its return value is then returned from `world_scope` as the generic type
/// parameter `R`.
pub fn world_scope<R>(
entity_map: &'m mut HashMap<Entity, Entity>,
entity_map: &'m mut EntityHashMap<Entity, Entity>,
world: &mut World,
f: impl FnOnce(&mut World, &mut Self) -> R,
) -> R {
Expand All @@ -131,7 +131,7 @@ impl<'m> EntityMapper<'m> {

#[cfg(test)]
mod tests {
use bevy_utils::HashMap;
use bevy_utils::EntityHashMap;

use crate::{
entity::{Entity, EntityMapper},
Expand All @@ -143,7 +143,7 @@ mod tests {
const FIRST_IDX: u32 = 1;
const SECOND_IDX: u32 = 2;

let mut map = HashMap::default();
let mut map = EntityHashMap::default();
let mut world = World::new();
let mut mapper = EntityMapper::new(&mut map, &mut world);

Expand All @@ -170,7 +170,7 @@ mod tests {

#[test]
fn world_scope_reserves_generations() {
let mut map = HashMap::default();
let mut map = EntityHashMap::default();
let mut world = World::new();

let dead_ref = EntityMapper::world_scope(&mut map, &mut world, |_, mapper| {
Expand Down
20 changes: 12 additions & 8 deletions crates/bevy_ecs/src/reflect/map_entities.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ use crate::{
world::World,
};
use bevy_reflect::FromType;
use bevy_utils::HashMap;
use bevy_utils::EntityHashMap;

/// For a specific type of component, this maps any fields with values of type [`Entity`] to a new world.
/// Since a given `Entity` ID is only valid for the world it came from, when performing deserialization
Expand All @@ -18,29 +18,33 @@ pub struct ReflectMapEntities {
}

impl ReflectMapEntities {
/// A general method for applying [`MapEntities`] behavior to all elements in an [`HashMap<Entity, Entity>`].
/// A general method for applying [`MapEntities`] behavior to all elements in an [`EntityHashMap<Entity, Entity>`].
///
/// Be mindful in its usage: Works best in situations where the entities in the [`HashMap<Entity, Entity>`] are newly
/// Be mindful in its usage: Works best in situations where the entities in the [`EntityHashMap<Entity, Entity>`] are newly
/// created, before systems have a chance to add new components. If some of the entities referred to
/// by the [`HashMap<Entity, Entity>`] might already contain valid entity references, you should use [`map_entities`](Self::map_entities).
/// by the [`EntityHashMap<Entity, Entity>`] might already contain valid entity references, you should use [`map_entities`](Self::map_entities).
///
/// An example of this: A scene can be loaded with `Parent` components, but then a `Parent` component can be added
/// to these entities after they have been loaded. If you reload the scene using [`map_all_entities`](Self::map_all_entities), those `Parent`
/// components with already valid entity references could be updated to point at something else entirely.
pub fn map_all_entities(&self, world: &mut World, entity_map: &mut HashMap<Entity, Entity>) {
pub fn map_all_entities(
&self,
world: &mut World,
entity_map: &mut EntityHashMap<Entity, Entity>,
) {
EntityMapper::world_scope(entity_map, world, self.map_all_entities);
}

/// A general method for applying [`MapEntities`] behavior to elements in an [`HashMap<Entity, Entity>`]. Unlike
/// A general method for applying [`MapEntities`] behavior to elements in an [`EntityHashMap<Entity, Entity>`]. Unlike
/// [`map_all_entities`](Self::map_all_entities), this is applied to specific entities, not all values
/// in the [`HashMap<Entity, Entity>`].
/// in the [`EntityHashMap<Entity, Entity>`].
///
/// This is useful mostly for when you need to be careful not to update components that already contain valid entity
/// values. See [`map_all_entities`](Self::map_all_entities) for more details.
pub fn map_entities(
&self,
world: &mut World,
entity_map: &mut HashMap<Entity, Entity>,
entity_map: &mut EntityHashMap<Entity, Entity>,
entities: &[Entity],
) {
EntityMapper::world_scope(entity_map, world, |world, mapper| {
Expand Down
10 changes: 5 additions & 5 deletions crates/bevy_scene/src/dynamic_scene.rs
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ use bevy_ecs::{
world::World,
};
use bevy_reflect::{Reflect, TypePath, TypeRegistryArc};
use bevy_utils::HashMap;
use bevy_utils::{EntityHashMap, HashMap};
use std::any::TypeId;

#[cfg(feature = "serialize")]
Expand Down Expand Up @@ -65,7 +65,7 @@ impl DynamicScene {
pub fn write_to_world_with(
&self,
world: &mut World,
entity_map: &mut HashMap<Entity, Entity>,
entity_map: &mut EntityHashMap<Entity, Entity>,
type_registry: &AppTypeRegistry,
) -> Result<(), SceneSpawnError> {
let type_registry = type_registry.read();
Expand Down Expand Up @@ -163,7 +163,7 @@ impl DynamicScene {
pub fn write_to_world(
&self,
world: &mut World,
entity_map: &mut HashMap<Entity, Entity>,
entity_map: &mut EntityHashMap<Entity, Entity>,
) -> Result<(), SceneSpawnError> {
let registry = world.resource::<AppTypeRegistry>().clone();
self.write_to_world_with(world, entity_map, &registry)
Expand Down Expand Up @@ -193,7 +193,7 @@ where
mod tests {
use bevy_ecs::{reflect::AppTypeRegistry, system::Command, world::World};
use bevy_hierarchy::{AddChild, Parent};
use bevy_utils::HashMap;
use bevy_utils::EntityHashMap;

use crate::dynamic_scene_builder::DynamicSceneBuilder;

Expand Down Expand Up @@ -222,7 +222,7 @@ mod tests {
.extract_entity(original_parent_entity)
.extract_entity(original_child_entity)
.build();
let mut entity_map = HashMap::default();
let mut entity_map = EntityHashMap::default();
scene.write_to_world(&mut world, &mut entity_map).unwrap();

let &from_scene_parent_entity = entity_map.get(&original_parent_entity).unwrap();
Expand Down
6 changes: 3 additions & 3 deletions crates/bevy_scene/src/scene.rs
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ use bevy_ecs::{
world::World,
};
use bevy_reflect::TypePath;
use bevy_utils::HashMap;
use bevy_utils::EntityHashMap;

/// To spawn a scene, you can use either:
/// * [`SceneSpawner::spawn`](crate::SceneSpawner::spawn)
Expand All @@ -31,7 +31,7 @@ impl Scene {
type_registry: &AppTypeRegistry,
) -> Result<Scene, SceneSpawnError> {
let mut world = World::new();
let mut entity_map = HashMap::default();
let mut entity_map = EntityHashMap::default();
dynamic_scene.write_to_world_with(&mut world, &mut entity_map, type_registry)?;

Ok(Self { world })
Expand All @@ -57,7 +57,7 @@ impl Scene {
type_registry: &AppTypeRegistry,
) -> Result<InstanceInfo, SceneSpawnError> {
let mut instance_info = InstanceInfo {
entity_map: HashMap::default(),
entity_map: EntityHashMap::default(),
};

let type_registry = type_registry.read();
Expand Down
10 changes: 5 additions & 5 deletions crates/bevy_scene/src/scene_spawner.rs
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ use bevy_ecs::{
world::{Mut, World},
};
use bevy_hierarchy::{AddChild, Parent};
use bevy_utils::{tracing::error, HashMap, HashSet};
use bevy_utils::{tracing::error, EntityHashMap, HashMap, HashSet};
use thiserror::Error;
use uuid::Uuid;

Expand All @@ -25,7 +25,7 @@ pub struct SceneInstanceReady {
#[derive(Debug)]
pub struct InstanceInfo {
/// Mapping of entities from the scene world to the instance world.
pub entity_map: HashMap<Entity, Entity>,
pub entity_map: EntityHashMap<Entity, Entity>,
}

/// Unique id identifying a scene instance.
Expand Down Expand Up @@ -199,7 +199,7 @@ impl SceneSpawner {
world: &mut World,
id: impl Into<AssetId<DynamicScene>>,
) -> Result<(), SceneSpawnError> {
let mut entity_map = HashMap::default();
let mut entity_map = EntityHashMap::default();
let id = id.into();
Self::spawn_dynamic_internal(world, id, &mut entity_map)?;
let instance_id = InstanceId::new();
Expand All @@ -213,7 +213,7 @@ impl SceneSpawner {
fn spawn_dynamic_internal(
world: &mut World,
id: AssetId<DynamicScene>,
entity_map: &mut HashMap<Entity, Entity>,
entity_map: &mut EntityHashMap<Entity, Entity>,
) -> Result<(), SceneSpawnError> {
world.resource_scope(|world, scenes: Mut<Assets<DynamicScene>>| {
let scene = scenes
Expand Down Expand Up @@ -297,7 +297,7 @@ impl SceneSpawner {
let scenes_to_spawn = std::mem::take(&mut self.dynamic_scenes_to_spawn);

for (id, instance_id) in scenes_to_spawn {
let mut entity_map = HashMap::default();
let mut entity_map = EntityHashMap::default();

match Self::spawn_dynamic_internal(world, id, &mut entity_map) {
Ok(_) => {
Expand Down
6 changes: 3 additions & 3 deletions crates/bevy_scene/src/serde.rs
Original file line number Diff line number Diff line change
Expand Up @@ -504,7 +504,7 @@ mod tests {
use bevy_ecs::reflect::{AppTypeRegistry, ReflectMapEntities};
use bevy_ecs::world::FromWorld;
use bevy_reflect::{Reflect, ReflectSerialize};
use bevy_utils::HashMap;
use bevy_utils::EntityHashMap;
use bincode::Options;
use serde::de::DeserializeSeed;
use serde::Serialize;
Expand Down Expand Up @@ -678,7 +678,7 @@ mod tests {
"expected `entities` to contain 3 entities"
);

let mut map = HashMap::default();
let mut map = EntityHashMap::default();
let mut dst_world = create_world();
scene.write_to_world(&mut dst_world, &mut map).unwrap();

Expand Down Expand Up @@ -717,7 +717,7 @@ mod tests {

let deserialized_scene = scene_deserializer.deserialize(&mut deserializer).unwrap();

let mut map = HashMap::default();
let mut map = EntityHashMap::default();
let mut dst_world = create_world();
deserialized_scene
.write_to_world(&mut dst_world, &mut map)
Expand Down

0 comments on commit 04ceb46

Please sign in to comment.