Skip to content

Commit

Permalink
Add mappings to EntityMapper (#13727)
Browse files Browse the repository at this point in the history
# Objective

- Fixes #13703

## Solution

- Added `mappings` to the `EntityMapper` trait, which returns an
iterator over currently tracked `Entity` to `Entity` mappings.
- Added `DynEntityMapper` as an [object
safe](https://doc.rust-lang.org/reference/items/traits.html#object-safety)
alternative to `EntityMapper`.
- Added `assert_object_safe` as a helper for ensuring traits are object
safe.

## Testing

- Added new unit test `entity_mapper_iteration` which tests the
`SceneEntityMapper` implementation of `EntityMapper::mappings`.
- Added unit tests to ensure `DynEntityMapper`, `DynEq` and `DynHash`
are object safe.
- Passed CI on my Windows 10 development environment

---

## Changelog

- Added `mappings` to `EntityMapper` trait.

## Migration Guide

- If you are implementing `EntityMapper` yourself, you can use the below
as a stub implementation:

```rust
fn mappings(&self) -> impl Iterator<Item = (Entity, Entity)> {
    unimplemented!()
}
```

- If you were using `EntityMapper` as a trait object (`dyn
EntityMapper`), instead use `dyn DynEntityMapper` and its associated
methods.

## Notes

- The original issue proposed returning a `Vec` from `EntityMapper`
instead of an `impl Iterator` to preserve its object safety. This is a
simpler option, but also forces an allocation where it isn't strictly
needed. I've opted for this split into `DynEntityMapper` and
`EntityMapper` as it's been done several times across Bevy already, and
provides maximum flexibility to users.
- `assert_object_safe` is an empty function, since the assertion
actually happens once you try to use a `dyn T` for some trait `T`. I
have still added this function to clearly document what object safety is
within Bevy, and to create a standard way to communicate that a given
trait must be object safe.
- Other traits should have tests added to ensure object safety, but I've
left those off to avoid cluttering this PR further.

---------

Co-authored-by: Alice Cecile <alice.i.cecile@gmail.com>
  • Loading branch information
bushrat011899 and alice-i-cecile authored Jun 8, 2024
1 parent d38d8a1 commit 3bfc427
Show file tree
Hide file tree
Showing 4 changed files with 126 additions and 0 deletions.
78 changes: 78 additions & 0 deletions crates/bevy_ecs/src/entity/map_entities.rs
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,9 @@ pub trait MapEntities {
///
/// More generally, this can be used to map [`Entity`] references between any two [`Worlds`](World).
///
/// Note that this trait is _not_ [object safe](https://doc.rust-lang.org/reference/items/traits.html#object-safety).
/// Please see [`DynEntityMapper`] for an object safe alternative.
///
/// ## Example
///
/// ```
Expand All @@ -68,11 +71,55 @@ pub trait MapEntities {
/// fn map_entity(&mut self, entity: Entity) -> Entity {
/// self.map.get(&entity).copied().unwrap_or(entity)
/// }
///
/// fn mappings(&self) -> impl Iterator<Item = (Entity, Entity)> {
/// self.map.iter().map(|(&source, &target)| (source, target))
/// }
/// }
/// ```
pub trait EntityMapper {
/// Map an entity to another entity
fn map_entity(&mut self, entity: Entity) -> Entity;

/// Iterate over all entity to entity mappings.
///
/// # Examples
///
/// ```rust
/// # use bevy_ecs::entity::{Entity, EntityMapper};
/// # fn example(mapper: impl EntityMapper) {
/// for (source, target) in mapper.mappings() {
/// println!("Will map from {source} to {target}");
/// }
/// # }
/// ```
fn mappings(&self) -> impl Iterator<Item = (Entity, Entity)>;
}

/// An [object safe](https://doc.rust-lang.org/reference/items/traits.html#object-safety) version
/// of [`EntityMapper`]. This trait is automatically implemented for type that implements `EntityMapper`.
pub trait DynEntityMapper {
/// Map an entity to another entity.
///
/// This is an [object safe](https://doc.rust-lang.org/reference/items/traits.html#object-safety)
/// alternative to [`EntityMapper::map_entity`].
fn dyn_map_entity(&mut self, entity: Entity) -> Entity;

/// Iterate over all entity to entity mappings.
///
/// This is an [object safe](https://doc.rust-lang.org/reference/items/traits.html#object-safety)
/// alternative to [`EntityMapper::mappings`].
fn dyn_mappings(&self) -> Vec<(Entity, Entity)>;
}

impl<T: EntityMapper> DynEntityMapper for T {
fn dyn_map_entity(&mut self, entity: Entity) -> Entity {
<T as EntityMapper>::map_entity(self, entity)
}

fn dyn_mappings(&self) -> Vec<(Entity, Entity)> {
<T as EntityMapper>::mappings(self).collect()
}
}

impl EntityMapper for SceneEntityMapper<'_> {
Expand All @@ -95,6 +142,10 @@ impl EntityMapper for SceneEntityMapper<'_> {

new
}

fn mappings(&self) -> impl Iterator<Item = (Entity, Entity)> {
self.map.iter().map(|(&source, &target)| (source, target))
}
}

/// A wrapper for [`EntityHashMap<Entity>`], augmenting it with the ability to allocate new [`Entity`] references in a destination
Expand Down Expand Up @@ -171,10 +222,12 @@ impl<'m> SceneEntityMapper<'m> {

#[cfg(test)]
mod tests {
use crate::entity::DynEntityMapper;
use crate::{
entity::{Entity, EntityHashMap, EntityMapper, SceneEntityMapper},
world::World,
};
use bevy_utils::assert_object_safe;

#[test]
fn entity_mapper() {
Expand Down Expand Up @@ -220,4 +273,29 @@ mod tests {
assert_eq!(entity.index(), dead_ref.index());
assert!(entity.generation() > dead_ref.generation());
}

#[test]
fn entity_mapper_iteration() {
let mut old_world = World::new();
let mut new_world = World::new();

let mut map = EntityHashMap::default();
let mut mapper = SceneEntityMapper::new(&mut map, &mut new_world);

assert_eq!(mapper.mappings().collect::<Vec<_>>(), vec![]);

let old_entity = old_world.spawn_empty().id();

let new_entity = mapper.map_entity(old_entity);

assert_eq!(
mapper.mappings().collect::<Vec<_>>(),
vec![(old_entity, new_entity)]
);
}

#[test]
fn dyn_entity_mapper_object_safe() {
assert_object_safe::<dyn DynEntityMapper>();
}
}
16 changes: 16 additions & 0 deletions crates/bevy_ecs/src/label.rs
Original file line number Diff line number Diff line change
Expand Up @@ -197,3 +197,19 @@ macro_rules! define_label {
$crate::intern::Interner::new();
};
}

#[cfg(test)]
mod tests {
use super::{DynEq, DynHash};
use bevy_utils::assert_object_safe;

#[test]
fn dyn_eq_object_safe() {
assert_object_safe::<dyn DynEq>();
}

#[test]
fn dyn_hash_object_safe() {
assert_object_safe::<dyn DynHash>();
}
}
2 changes: 2 additions & 0 deletions crates/bevy_utils/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,8 @@ pub mod syncunsafecell;

mod cow_arc;
mod default;
mod object_safe;
pub use object_safe::assert_object_safe;
mod once;
mod parallel_queue;

Expand Down
30 changes: 30 additions & 0 deletions crates/bevy_utils/src/object_safe.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
/// Assert that a given `T` is [object safe](https://doc.rust-lang.org/reference/items/traits.html#object-safety).
/// Will fail to compile if that is not the case.
///
/// # Examples
///
/// ```rust
/// # use bevy_utils::assert_object_safe;
/// // Concrete types are always object safe
/// struct MyStruct;
/// assert_object_safe::<MyStruct>();
///
/// // Trait objects are where that safety comes into question.
/// // This trait is object safe...
/// trait ObjectSafe { }
/// assert_object_safe::<dyn ObjectSafe>();
/// ```
///
/// ```compile_fail
/// # use bevy_utils::assert_object_safe;
/// // ...but this trait is not.
/// trait NotObjectSafe {
/// const VALUE: usize;
/// }
/// assert_object_safe::<dyn NotObjectSafe>();
/// // Error: the trait `NotObjectSafe` cannot be made into an object
/// ```
pub fn assert_object_safe<T: ?Sized>() {
// This space is left intentionally blank. The type parameter T is sufficient to induce a compiler
// error without a function body.
}

0 comments on commit 3bfc427

Please sign in to comment.