Skip to content

Commit

Permalink
Bugfix: Scene reload fix (nonbreaking) (bevyengine#7951)
Browse files Browse the repository at this point in the history 
Fix a bug with scene reload.

(This is a copy of bevyengine#7570 but without the breaking API change, in order
to allow the bugfix to be introduced in 0.10.1)

When a scene was reloaded, it was corrupting components that weren't
native to the scene itself. In particular, when a DynamicScene was
created on Entity (A), all components in the scene without parents are
automatically added as children of Entity (A). But if that scene was
reloaded and the same ID of Entity (A) was a scene ID as well*, that
parent component was corrupted, causing the hierarchy to become
malformed and bevy to panic.

*For example, if Entity (A)'s ID was 3, and the scene contained an
entity with ID 3

This issue could affect any components that:
* Implemented `MapEntities`, basically components that contained
references to other entities
* Were added to entities from a scene file but weren't defined in the
scene file

- Fixes bevyengine#7529

The solution was to keep track of entities+components that had
`MapEntities` functionality during scene load, and only apply the entity
update behavior to them. They were tracked with a HashMap from the
component's TypeID to a vector of entity ID's. Then the
`ReflectMapEntities` struct was updated to hold a function that took a
list of entities to be applied to, instead of naively applying itself to
all values in the EntityMap.

(See this PR comment
bevyengine#7570 (comment) for
a story-based explanation of this bug and solution)

- Components that implement `MapEntities` added to scene entities after
load are not corrupted during scene reload.
  • Loading branch information
@Testare @UkoeHB
Testare authored and UkoeHB committed Sep 10, 2023
1 parent 91b5c37 commit 3bec4a7
Show file tree
Hide file tree
Showing 2 changed files with 280 additions and 122 deletions.
332 changes: 254 additions & 78 deletions crates/bevy_ecs/src/reflect/component.rs
View file
Original file line number Diff line number Diff line change
@@ -1,58 +1,19 @@
//! Definitions for [`Component`] reflection.
//!
//! This module exports two types: [`ReflectComponentFns`] and [`ReflectComponent`].
//!
//! # Architecture
//!
//! [`ReflectComponent`] wraps a [`ReflectComponentFns`]. In fact, each method on
//! [`ReflectComponent`] wraps a call to a function pointer field in `ReflectComponentFns`.
//!
//! ## Who creates `ReflectComponent`s?
//!
//! When a user adds the `#[reflect(Component)]` attribute to their `#[derive(Reflect)]`
//! type, it tells the derive macro for `Reflect` to add the following single line to its
//! [`get_type_registration`] method (see the relevant code[^1]).
//!
//! ```ignore
//! registration.insert::<ReflectComponent>(FromType::<Self>::from_type());
//! ```
//!
//! This line adds a `ReflectComponent` to the registration data for the type in question.
//! The user can access the `ReflectComponent` for type `T` through the type registry,
//! as per the `trait_reflection.rs` example.
//!
//! The `FromType::<Self>::from_type()` in the previous line calls the `FromType<C>`
//! implementation of `ReflectComponent`.
//!
//! The `FromType<C>` impl creates a function per field of [`ReflectComponentFns`].
//! In those functions, we call generic methods on [`World`] and [`EntityMut`].
//!
//! The result is a `ReflectComponent` completely independent of `C`, yet capable
//! of using generic ECS methods such as `entity.get::<C>()` to get `&dyn Reflect`
//! with underlying type `C`, without the `C` appearing in the type signature.
//!
//! ## A note on code generation
//!
//! A downside of this approach is that monomorphized code (ie: concrete code
//! for generics) is generated **unconditionally**, regardless of whether it ends
//! up used or not.
//!
//! Adding `N` fields on `ReflectComponentFns` will generate `N × M` additional
//! functions, where `M` is how many types derive `#[reflect(Component)]`.
//!
//! Those functions will increase the size of the final app binary.
//!
//! [^1]: `crates/bevy_reflect/bevy_reflect_derive/src/registration.rs`
//!
//! [`get_type_registration`]: bevy_reflect::GetTypeRegistration::get_type_registration
//! Types that enable reflection support.

use crate::{
change_detection::Mut,
component::Component,
entity::Entity,
world::{unsafe_world_cell::UnsafeEntityCell, EntityMut, EntityRef, FromWorld, World},
entity::{Entity, EntityMap, MapEntities, MapEntitiesError},
system::Resource,
world::{
unsafe_world_cell::{UnsafeEntityCell, UnsafeWorldCell},
EntityMut, EntityRef, FromWorld, World,
},
};
use bevy_reflect::{
impl_from_reflect_value, impl_reflect_value, FromType, Reflect, ReflectDeserialize,
ReflectSerialize,
};
use bevy_reflect::{FromType, Reflect};

/// A struct used to operate on reflected [`Component`] of a type.
///
Expand Down Expand Up @@ -83,8 +44,6 @@ pub struct ReflectComponent(ReflectComponentFns);
/// world.
#[derive(Clone)]
pub struct ReflectComponentFns {
/// Function pointer implementing [`ReflectComponent::from_world()`].
pub from_world: fn(&mut World) -> Box<dyn Reflect>,
/// Function pointer implementing [`ReflectComponent::insert()`].
pub insert: fn(&mut EntityMut, &dyn Reflect),
/// Function pointer implementing [`ReflectComponent::apply()`].
Expand Down Expand Up @@ -120,11 +79,6 @@ impl ReflectComponentFns {
}

impl ReflectComponent {
/// Constructs default reflected [`Component`] from world using [`from_world()`](FromWorld::from_world).
pub fn from_world(&self, world: &mut World) -> Box<dyn Reflect> {
(self.0.from_world)(world)
}

/// Insert a reflected [`Component`] into the entity like [`insert()`](crate::world::EntityMut::insert).
pub fn insert(&self, entity: &mut EntityMut, component: &dyn Reflect) {
(self.0.insert)(entity, component);
Expand Down Expand Up @@ -211,32 +165,11 @@ impl ReflectComponent {
pub fn new(fns: ReflectComponentFns) -> Self {
Self(fns)
}

/// The underlying function pointers implementing methods on `ReflectComponent`.
///
/// This is useful when you want to keep track locally of an individual
/// function pointer.
///
/// Calling [`TypeRegistry::get`] followed by
/// [`TypeRegistration::data::<ReflectComponent>`] can be costly if done several
/// times per frame. Consider cloning [`ReflectComponent`] and keeping it
/// between frames, cloning a `ReflectComponent` is very cheap.
///
/// If you only need a subset of the methods on `ReflectComponent`,
/// use `fn_pointers` to get the underlying [`ReflectComponentFns`]
/// and copy the subset of function pointers you care about.
///
/// [`TypeRegistration::data::<ReflectComponent>`]: bevy_reflect::TypeRegistration::data
/// [`TypeRegistry::get`]: bevy_reflect::TypeRegistry::get
pub fn fn_pointers(&self) -> &ReflectComponentFns {
&self.0
}
}

impl<C: Component + Reflect + FromWorld> FromType<C> for ReflectComponent {
fn from_type() -> Self {
ReflectComponent(ReflectComponentFns {
from_world: |world| Box::new(C::from_world(world)),
insert: |entity, reflected_component| {
let mut component = entity.world_scope(|world| C::from_world(world));
component.apply(reflected_component);
Expand Down Expand Up @@ -287,3 +220,246 @@ impl<C: Component + Reflect + FromWorld> FromType<C> for ReflectComponent {
})
}
}

/// A struct used to operate on reflected [`Resource`] of a type.
///
/// A [`ReflectResource`] for type `T` can be obtained via
/// [`bevy_reflect::TypeRegistration::data`].
#[derive(Clone)]
pub struct ReflectResource(ReflectResourceFns);

/// The raw function pointers needed to make up a [`ReflectResource`].
///
/// This is used when creating custom implementations of [`ReflectResource`] with
/// [`ReflectResource::new()`].
///
/// > **Note:**
/// > Creating custom implementations of [`ReflectResource`] is an advanced feature that most users
/// > will not need.
/// > Usually a [`ReflectResource`] is created for a type by deriving [`Reflect`]
/// > and adding the `#[reflect(Resource)]` attribute.
/// > After adding the component to the [`TypeRegistry`][bevy_reflect::TypeRegistry],
/// > its [`ReflectResource`] can then be retrieved when needed.
///
/// Creating a custom [`ReflectResource`] may be useful if you need to create new resource types at
/// runtime, for example, for scripting implementations.
///
/// By creating a custom [`ReflectResource`] and inserting it into a type's
/// [`TypeRegistration`][bevy_reflect::TypeRegistration],
/// you can modify the way that reflected resources of that type will be inserted into the bevy
/// world.
#[derive(Clone)]
pub struct ReflectResourceFns {
/// Function pointer implementing [`ReflectResource::insert()`].
pub insert: fn(&mut World, &dyn Reflect),
/// Function pointer implementing [`ReflectResource::apply()`].
pub apply: fn(&mut World, &dyn Reflect),
/// Function pointer implementing [`ReflectResource::apply_or_insert()`].
pub apply_or_insert: fn(&mut World, &dyn Reflect),
/// Function pointer implementing [`ReflectResource::remove()`].
pub remove: fn(&mut World),
/// Function pointer implementing [`ReflectResource::reflect()`].
pub reflect: fn(&World) -> Option<&dyn Reflect>,
/// Function pointer implementing [`ReflectResource::reflect_unchecked_mut()`].
///
/// # Safety
/// The function may only be called with an [`UnsafeWorldCell`] that can be used to mutably access the relevant resource.
pub reflect_unchecked_mut: unsafe fn(UnsafeWorldCell<'_>) -> Option<Mut<'_, dyn Reflect>>,
/// Function pointer implementing [`ReflectResource::copy()`].
pub copy: fn(&World, &mut World),
}

impl ReflectResourceFns {
/// Get the default set of [`ReflectResourceFns`] for a specific resource type using its
/// [`FromType`] implementation.
///
/// This is useful if you want to start with the default implementation before overriding some
/// of the functions to create a custom implementation.
pub fn new<T: Resource + Reflect + FromWorld>() -> Self {
<ReflectResource as FromType<T>>::from_type().0
}
}

impl ReflectResource {
/// Insert a reflected [`Resource`] into the world like [`insert()`](World::insert_resource).
pub fn insert(&self, world: &mut World, resource: &dyn Reflect) {
(self.0.insert)(world, resource);
}

/// Uses reflection to set the value of this [`Resource`] type in the world to the given value.
///
/// # Panics
///
/// Panics if there is no [`Resource`] of the given type.
pub fn apply(&self, world: &mut World, resource: &dyn Reflect) {
(self.0.apply)(world, resource);
}

/// Uses reflection to set the value of this [`Resource`] type in the world to the given value or insert a new one if it does not exist.
pub fn apply_or_insert(&self, world: &mut World, resource: &dyn Reflect) {
(self.0.apply_or_insert)(world, resource);
}

/// Removes this [`Resource`] type from the world. Does nothing if it doesn't exist.
pub fn remove(&self, world: &mut World) {
(self.0.remove)(world);
}

/// Gets the value of this [`Resource`] type from the world as a reflected reference.
pub fn reflect<'a>(&self, world: &'a World) -> Option<&'a dyn Reflect> {
(self.0.reflect)(world)
}

/// Gets the value of this [`Resource`] type from the world as a mutable reflected reference.
pub fn reflect_mut<'a>(&self, world: &'a mut World) -> Option<Mut<'a, dyn Reflect>> {
// SAFETY: unique world access
unsafe { (self.0.reflect_unchecked_mut)(world.as_unsafe_world_cell()) }
}

/// # Safety
/// This method does not prevent you from having two mutable pointers to the same data,
/// violating Rust's aliasing rules. To avoid this:
/// * Only call this method with an [`UnsafeWorldCell`] which can be used to mutably access the resource.
/// * Don't call this method more than once in the same scope for a given [`Resource`].
pub unsafe fn reflect_unchecked_mut<'w>(
&self,
world: UnsafeWorldCell<'w>,
) -> Option<Mut<'w, dyn Reflect>> {
// SAFETY: caller promises to uphold uniqueness guarantees
(self.0.reflect_unchecked_mut)(world)
}

/// Gets the value of this [`Resource`] type from `source_world` and [applies](Self::apply()) it to the value of this [`Resource`] type in `destination_world`.
///
/// # Panics
///
/// Panics if there is no [`Resource`] of the given type.
pub fn copy(&self, source_world: &World, destination_world: &mut World) {
(self.0.copy)(source_world, destination_world);
}

/// Create a custom implementation of [`ReflectResource`].
///
/// This is an advanced feature,
/// useful for scripting implementations,
/// that should not be used by most users
/// unless you know what you are doing.
///
/// Usually you should derive [`Reflect`] and add the `#[reflect(Resource)]` component
/// to generate a [`ReflectResource`] implementation automatically.
///
/// See [`ReflectResourceFns`] for more information.
pub fn new(&self, fns: ReflectResourceFns) -> Self {
Self(fns)
}
}

impl<C: Resource + Reflect + FromWorld> FromType<C> for ReflectResource {
fn from_type() -> Self {
ReflectResource(ReflectResourceFns {
insert: |world, reflected_resource| {
let mut resource = C::from_world(world);
resource.apply(reflected_resource);
world.insert_resource(resource);
},
apply: |world, reflected_resource| {
let mut resource = world.resource_mut::<C>();
resource.apply(reflected_resource);
},
apply_or_insert: |world, reflected_resource| {
if let Some(mut resource) = world.get_resource_mut::<C>() {
resource.apply(reflected_resource);
} else {
let mut resource = C::from_world(world);
resource.apply(reflected_resource);
world.insert_resource(resource);
}
},
remove: |world| {
world.remove_resource::<C>();
},
reflect: |world| world.get_resource::<C>().map(|res| res as &dyn Reflect),
reflect_unchecked_mut: |world| {
// SAFETY: all usages of `reflect_unchecked_mut` guarantee that there is either a single mutable
// reference or multiple immutable ones alive at any given point
unsafe {
world.get_resource_mut::<C>().map(|res| Mut {
value: res.value as &mut dyn Reflect,
ticks: res.ticks,
})
}
},
copy: |source_world, destination_world| {
let source_resource = source_world.resource::<C>();
let mut destination_resource = C::from_world(destination_world);
destination_resource.apply(source_resource);
destination_world.insert_resource(destination_resource);
},
})
}
}

impl_reflect_value!(Entity(Hash, PartialEq, Serialize, Deserialize));
impl_from_reflect_value!(Entity);

#[derive(Clone)]
pub struct ReflectMapEntities {
map_entities: fn(&mut World, &EntityMap) -> Result<(), MapEntitiesError>,
map_specific_entities: fn(&mut World, &EntityMap, &[Entity]) -> Result<(), MapEntitiesError>,
}

impl ReflectMapEntities {
/// A general method for applying [`MapEntities`] behavior to all elements in an [`EntityMap`].
///
/// Be mindful in its usage: Works best in situations where the entities in the [`EntityMap`] are newly
/// created, before systems have a chance to add new components. If some of the entities referred to
/// by the [`EntityMap`] 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_entities`](Self::map_entities), those `Parent`
/// components with already valid entity references could be updated to point at something else entirely.
pub fn map_entities(
&self,
world: &mut World,
entity_map: &EntityMap,
) -> Result<(), MapEntitiesError> {
(self.map_entities)(world, entity_map)
}

/// This is like [`map_entities`](Self::map_entities), but only applied to specific entities, not all values
/// in the [`EntityMap`].
///
/// This is useful mostly for when you need to be careful not to update components that already contain valid entity
/// values. See [`map_entities`](Self::map_entities) for more details.
pub fn map_specific_entities(
&self,
world: &mut World,
entity_map: &EntityMap,
entities: &[Entity],
) -> Result<(), MapEntitiesError> {
(self.map_specific_entities)(world, entity_map, entities)
}
}

impl<C: Component + MapEntities> FromType<C> for ReflectMapEntities {
fn from_type() -> Self {
ReflectMapEntities {
map_specific_entities: |world, entity_map, entities| {
for &entity in entities {
if let Some(mut component) = world.get_mut::<C>(entity) {
component.map_entities(entity_map)?;
}
}
Ok(())
},
map_entities: |world, entity_map| {
for entity in entity_map.values() {
if let Some(mut component) = world.get_mut::<C>(entity) {
component.map_entities(entity_map)?;
}
}
Ok(())
},
}
}
}
Loading

0 comments on commit 3bec4a7

Please sign in to comment.