Replace RemovedComponents<T> backing with Events<Entity> (#5680)

# Objective
Removal events are unwieldy and require some knowledge of when to put systems that need to catch events for them, it is very easy to end up missing one and end up with memory leak-ish issues where you don't clean up after yourself.

## Solution
Consolidate removals with the benefits of `Events<...>` (such as double buffering and per system ticks for reading the events) and reduce the special casing of it, ideally I was hoping to move the removals to a `Resource` in the world, but that seems a bit more rough to implement/maintain because of double mutable borrowing issues.

This doesn't go the full length of change detection esque removal detection a la https://github.com/bevyengine/rfcs/pull/44.
Just tries to make the current workflow a bit more user friendly so detecting removals isn't such a scheduling nightmare.

---

## Changelog
- RemovedComponents<T> is now backed by an `Events<Entity>` for the benefits of double buffering.

## Migration Guide
- Add a `mut` for `removed: RemovedComponents<T>` since we are now modifying an event reader internally.
- Iterating over removed components now requires `&mut removed_components` or `removed_components.iter()` instead of `&removed_components`.
This commit is contained in:
Aceeri 2023-02-04 20:53:37 +00:00
parent 12f30f5667
commit 67826b21d4
10 changed files with 273 additions and 132 deletions

View file

@ -3,7 +3,8 @@
use crate::{
change_detection::MAX_CHANGE_AGE,
storage::{SparseSetIndex, Storages},
system::Resource,
system::{Local, Resource},
world::{FromWorld, World},
};
pub use bevy_ecs_macros::Component;
use bevy_ptr::{OwningPtr, UnsafeCellDeref};
@ -12,6 +13,7 @@ use std::{
alloc::Layout,
any::{Any, TypeId},
borrow::Cow,
marker::PhantomData,
mem::needs_drop,
};
@ -698,3 +700,48 @@ impl ComponentTicks {
self.changed.set_changed(change_tick);
}
}
/// Initialize and fetch a [`ComponentId`] for a specific type.
///
/// # Example
/// ```rust
/// # use bevy_ecs::{system::Local, component::{Component, ComponentId, ComponentIdFor}};
/// #[derive(Component)]
/// struct Player;
/// fn my_system(component_id: Local<ComponentIdFor<Player>>) {
/// let component_id: ComponentId = component_id.into();
/// // ...
/// }
/// ```
pub struct ComponentIdFor<T: Component> {
component_id: ComponentId,
phantom: PhantomData<T>,
}
impl<T: Component> FromWorld for ComponentIdFor<T> {
fn from_world(world: &mut World) -> Self {
Self {
component_id: world.init_component::<T>(),
phantom: PhantomData,
}
}
}
impl<T: Component> std::ops::Deref for ComponentIdFor<T> {
type Target = ComponentId;
fn deref(&self) -> &Self::Target {
&self.component_id
}
}
impl<T: Component> From<ComponentIdFor<T>> for ComponentId {
fn from(to_component_id: ComponentIdFor<T>) -> ComponentId {
*to_component_id
}
}
impl<'s, T: Component> From<Local<'s, ComponentIdFor<T>>> for ComponentId {
fn from(to_component_id: Local<ComponentIdFor<T>>) -> ComponentId {
**to_component_id
}
}

View file

@ -13,6 +13,7 @@ pub mod event;
pub mod query;
#[cfg(feature = "bevy_reflect")]
pub mod reflect;
pub mod removal_detection;
pub mod schedule;
pub mod schedule_v3;
pub mod storage;
@ -34,6 +35,7 @@ pub mod prelude {
entity::Entity,
event::{EventReader, EventWriter, Events},
query::{Added, AnyOf, ChangeTrackers, Changed, Or, QueryState, With, Without},
removal_detection::RemovedComponents,
schedule::{
IntoSystemDescriptor, RunCriteria, RunCriteriaDescriptorCoercion, RunCriteriaLabel,
Schedule, Stage, StageLabel, State, SystemLabel, SystemSet, SystemStage,
@ -42,7 +44,7 @@ pub mod prelude {
adapter as system_adapter,
adapter::{dbg, error, ignore, info, unwrap, warn},
Commands, In, IntoPipeSystem, IntoSystem, Local, NonSend, NonSendMut, ParallelCommands,
ParamSet, Query, RemovedComponents, Res, ResMut, Resource, System, SystemParamFunction,
ParamSet, Query, Res, ResMut, Resource, System, SystemParamFunction,
},
world::{FromWorld, World},
};

View file

@ -0,0 +1,169 @@
//! Alerting events when a component is removed from an entity.
use crate::{
self as bevy_ecs,
component::{Component, ComponentId, ComponentIdFor},
entity::Entity,
event::{Events, ManualEventIterator, ManualEventReader},
prelude::Local,
storage::SparseSet,
system::{ReadOnlySystemParam, SystemMeta, SystemParam},
world::World,
};
use std::{
fmt::Debug,
iter,
marker::PhantomData,
ops::{Deref, DerefMut},
option,
};
/// Wrapper around a [`ManualEventReader<Entity>`] so that we
/// can differentiate events between components.
#[derive(Debug)]
pub struct RemovedComponentReader<T>
where
T: Component,
{
reader: ManualEventReader<Entity>,
marker: PhantomData<T>,
}
impl<T: Component> Default for RemovedComponentReader<T> {
fn default() -> Self {
Self {
reader: Default::default(),
marker: PhantomData,
}
}
}
impl<T: Component> Deref for RemovedComponentReader<T> {
type Target = ManualEventReader<Entity>;
fn deref(&self) -> &Self::Target {
&self.reader
}
}
impl<T: Component> DerefMut for RemovedComponentReader<T> {
fn deref_mut(&mut self) -> &mut Self::Target {
&mut self.reader
}
}
/// Wrapper around a map of components to [`Events<Entity>`].
/// So that we can find the events without naming the type directly.
#[derive(Default, Debug)]
pub struct RemovedComponentEvents {
event_sets: SparseSet<ComponentId, Events<Entity>>,
}
impl RemovedComponentEvents {
pub fn new() -> Self {
Self::default()
}
pub fn update(&mut self) {
for (_component_id, events) in self.event_sets.iter_mut() {
events.update();
}
}
pub fn get(&self, component_id: impl Into<ComponentId>) -> Option<&Events<Entity>> {
self.event_sets.get(component_id.into())
}
pub fn send(&mut self, component_id: impl Into<ComponentId>, entity: Entity) {
self.event_sets
.get_or_insert_with(component_id.into(), Default::default)
.send(entity);
}
}
/// A [`SystemParam`] that grants access to the entities that had their `T` [`Component`] removed.
///
/// Note that this does not allow you to see which data existed before removal.
/// If you need this, you will need to track the component data value on your own,
/// using a regularly scheduled system that requests `Query<(Entity, &T), Changed<T>>`
/// and stores the data somewhere safe to later cross-reference.
///
/// If you are using `bevy_ecs` as a standalone crate,
/// note that the `RemovedComponents` list will not be automatically cleared for you,
/// and will need to be manually flushed using [`World::clear_trackers`](crate::world::World::clear_trackers)
///
/// For users of `bevy` and `bevy_app`, this is automatically done in `bevy_app::App::update`.
/// For the main world, [`World::clear_trackers`](crate::world::World::clear_trackers) is run after the main schedule is run and after
/// `SubApp`'s have run.
///
/// # Examples
///
/// Basic usage:
///
/// ```
/// # use bevy_ecs::component::Component;
/// # use bevy_ecs::system::IntoSystem;
/// # use bevy_ecs::removal_detection::RemovedComponents;
/// #
/// # #[derive(Component)]
/// # struct MyComponent;
/// fn react_on_removal(mut removed: RemovedComponents<MyComponent>) {
/// removed.iter().for_each(|removed_entity| println!("{:?}", removed_entity));
/// }
/// # bevy_ecs::system::assert_is_system(react_on_removal);
/// ```
#[derive(SystemParam)]
pub struct RemovedComponents<'w, 's, T: Component> {
component_id: Local<'s, ComponentIdFor<T>>,
reader: Local<'s, RemovedComponentReader<T>>,
event_sets: &'w RemovedComponentEvents,
}
/// Iterator over entities that had a specific component removed.
///
/// See [`RemovedComponents`].
pub type RemovedIter<'a> =
iter::Flatten<option::IntoIter<iter::Cloned<ManualEventIterator<'a, Entity>>>>;
impl<'w, 's, T: Component> RemovedComponents<'w, 's, T> {
pub fn iter(&mut self) -> RemovedIter<'_> {
self.event_sets
.get(**self.component_id)
.map(|events| self.reader.iter(events).cloned())
.into_iter()
.flatten()
}
}
impl<'a, 'w, 's: 'a, T> IntoIterator for &'a mut RemovedComponents<'w, 's, T>
where
T: Component,
{
type Item = Entity;
type IntoIter = RemovedIter<'a>;
fn into_iter(self) -> Self::IntoIter {
self.iter()
}
}
// SAFETY: Only reads World removed component events
unsafe impl<'a> ReadOnlySystemParam for &'a RemovedComponentEvents {}
// SAFETY: no component value access, removed component events can be read in parallel and are
// never mutably borrowed during system execution
unsafe impl<'a> SystemParam for &'a RemovedComponentEvents {
type State = ();
type Item<'w, 's> = &'w RemovedComponentEvents;
fn init_state(_world: &mut World, _system_meta: &mut SystemMeta) -> Self::State {}
#[inline]
unsafe fn get_param<'w, 's>(
_state: &'s mut Self::State,
_system_meta: &SystemMeta,
world: &'w World,
_change_tick: u32,
) -> Self::Item<'w, 's> {
world.removed_components()
}
}

View file

@ -84,7 +84,7 @@
//! - [`NonSend`] and `Option<NonSend>`
//! - [`NonSendMut`] and `Option<NonSendMut>`
//! - [`&World`](crate::world::World)
//! - [`RemovedComponents`]
//! - [`RemovedComponents`](crate::removal_detection::RemovedComponents)
//! - [`SystemName`]
//! - [`SystemChangeTick`]
//! - [`Archetypes`](crate::archetype::Archetypes) (Provides Archetype metadata)
@ -139,10 +139,11 @@ mod tests {
entity::{Entities, Entity},
prelude::{AnyOf, StageLabel},
query::{Added, Changed, Or, With, Without},
removal_detection::RemovedComponents,
schedule::{Schedule, Stage, SystemStage},
system::{
Commands, IntoSystem, Local, NonSend, NonSendMut, ParamSet, Query, QueryComponentError,
RemovedComponents, Res, ResMut, Resource, System, SystemState,
Res, ResMut, Resource, System, SystemState,
},
world::{FromWorld, World},
};
@ -602,7 +603,7 @@ mod tests {
world.entity_mut(spurious_entity).despawn();
fn validate_despawn(
removed_i32: RemovedComponents<W<i32>>,
mut removed_i32: RemovedComponents<W<i32>>,
despawned: Res<Despawned>,
mut n_systems: ResMut<NSystems>,
) {
@ -627,13 +628,16 @@ mod tests {
world.entity_mut(entity_to_remove_w_from).remove::<W<i32>>();
fn validate_remove(
removed_i32: RemovedComponents<W<i32>>,
mut removed_i32: RemovedComponents<W<i32>>,
despawned: Res<Despawned>,
removed: Res<Removed>,
mut n_systems: ResMut<NSystems>,
) {
// The despawned entity from the previous frame was
// double buffered so we now have it in this system as well.
assert_eq!(
removed_i32.iter().collect::<Vec<_>>(),
&[removed.0],
&[despawned.0, removed.0],
"removing a component causes the correct entity to show up in the 'RemovedComponent' system parameter."
);

View file

@ -3,8 +3,8 @@ use crate::{
archetype::{Archetype, Archetypes},
bundle::Bundles,
change_detection::{Ticks, TicksMut},
component::{Component, ComponentId, ComponentTicks, Components},
entity::{Entities, Entity},
component::{ComponentId, ComponentTicks, Components},
entity::Entities,
query::{
Access, FilteredAccess, FilteredAccessSet, QueryState, ReadOnlyWorldQuery, WorldQuery,
},
@ -19,7 +19,6 @@ use bevy_utils::synccell::SyncCell;
use std::{
borrow::Cow,
fmt::Debug,
marker::PhantomData,
ops::{Deref, DerefMut},
};
@ -787,89 +786,6 @@ unsafe impl<'a, T: FromWorld + Send + 'static> SystemParam for Local<'a, T> {
}
}
/// A [`SystemParam`] that grants access to the entities that had their `T` [`Component`] removed.
///
/// Note that this does not allow you to see which data existed before removal.
/// If you need this, you will need to track the component data value on your own,
/// using a regularly scheduled system that requests `Query<(Entity, &T), Changed<T>>`
/// and stores the data somewhere safe to later cross-reference.
///
/// If you are using `bevy_ecs` as a standalone crate,
/// note that the `RemovedComponents` list will not be automatically cleared for you,
/// and will need to be manually flushed using [`World::clear_trackers`]
///
/// For users of `bevy` and `bevy_app`, this is automatically done in `bevy_app::App::update`.
/// For the main world, [`World::clear_trackers`] is run after the main schedule is run and after
/// `SubApp`'s have run.
///
/// # Examples
///
/// Basic usage:
///
/// ```
/// # use bevy_ecs::component::Component;
/// # use bevy_ecs::system::IntoSystem;
/// # use bevy_ecs::system::RemovedComponents;
/// #
/// # #[derive(Component)]
/// # struct MyComponent;
///
/// fn react_on_removal(removed: RemovedComponents<MyComponent>) {
/// removed.iter().for_each(|removed_entity| println!("{:?}", removed_entity));
/// }
///
/// # bevy_ecs::system::assert_is_system(react_on_removal);
/// ```
pub struct RemovedComponents<'a, T: Component> {
world: &'a World,
component_id: ComponentId,
marker: PhantomData<T>,
}
impl<'a, T: Component> RemovedComponents<'a, T> {
/// Returns an iterator over the entities that had their `T` [`Component`] removed.
pub fn iter(&self) -> std::iter::Cloned<std::slice::Iter<'_, Entity>> {
self.world.removed_with_id(self.component_id)
}
}
impl<'a, T: Component> IntoIterator for &'a RemovedComponents<'a, T> {
type Item = Entity;
type IntoIter = std::iter::Cloned<std::slice::Iter<'a, Entity>>;
fn into_iter(self) -> Self::IntoIter {
self.iter()
}
}
// SAFETY: Only reads World components
unsafe impl<'a, T: Component> ReadOnlySystemParam for RemovedComponents<'a, T> {}
// SAFETY: no component access. removed component entity collections can be read in parallel and are
// never mutably borrowed during system execution
unsafe impl<'a, T: Component> SystemParam for RemovedComponents<'a, T> {
type State = ComponentId;
type Item<'w, 's> = RemovedComponents<'w, T>;
fn init_state(world: &mut World, _system_meta: &mut SystemMeta) -> Self::State {
world.init_component::<T>()
}
#[inline]
unsafe fn get_param<'w, 's>(
&mut component_id: &'s mut Self::State,
_system_meta: &SystemMeta,
world: &'w World,
_change_tick: u32,
) -> Self::Item<'w, 's> {
RemovedComponents {
world,
component_id,
marker: PhantomData,
}
}
}
/// Shared borrow of a non-[`Send`] resource.
///
/// Only `Send` resources may be accessed with the [`Res`] [`SystemParam`]. In case that the
@ -1482,6 +1398,7 @@ mod tests {
query::{ReadOnlyWorldQuery, WorldQuery},
system::Query,
};
use std::marker::PhantomData;
// Compile test for https://github.com/bevyengine/bevy/pull/2838.
#[derive(SystemParam)]

View file

@ -6,7 +6,8 @@ use crate::{
Component, ComponentId, ComponentStorage, ComponentTicks, Components, StorageType,
},
entity::{Entities, Entity, EntityLocation},
storage::{SparseSet, Storages},
removal_detection::RemovedComponentEvents,
storage::Storages,
world::{Mut, World},
};
use bevy_ptr::{OwningPtr, Ptr};
@ -439,9 +440,7 @@ impl<'w> EntityMut<'w> {
let entity = self.entity;
for component_id in bundle_info.component_ids.iter().cloned() {
if old_archetype.contains(component_id) {
removed_components
.get_or_insert_with(component_id, Vec::new)
.push(entity);
removed_components.send(component_id, entity);
// Make sure to drop components stored in sparse sets.
// Dense components are dropped later in `move_to_and_drop_missing_unchecked`.
@ -480,13 +479,11 @@ impl<'w> EntityMut<'w> {
.expect("entity should exist at this point.");
let table_row;
let moved_entity;
{
let archetype = &mut world.archetypes[location.archetype_id];
for component_id in archetype.components() {
let removed_components = world
.removed_components
.get_or_insert_with(component_id, Vec::new);
removed_components.push(self.entity);
world.removed_components.send(component_id, self.entity);
}
let remove_result = archetype.swap_remove(location.archetype_row);
if let Some(swapped_entity) = remove_result.swapped_entity {
@ -839,15 +836,14 @@ pub(crate) unsafe fn get_mut_by_id(
pub(crate) unsafe fn take_component<'a>(
storages: &'a mut Storages,
components: &Components,
removed_components: &mut SparseSet<ComponentId, Vec<Entity>>,
removed_components: &mut RemovedComponentEvents,
component_id: ComponentId,
entity: Entity,
location: EntityLocation,
) -> OwningPtr<'a> {
// SAFETY: caller promises component_id to be valid
let component_info = components.get_info_unchecked(component_id);
let removed_components = removed_components.get_or_insert_with(component_id, Vec::new);
removed_components.push(entity);
removed_components.send(component_id, entity);
match component_info.storage_type() {
StorageType::Table => {
let table = &mut storages.tables[location.table_id];

View file

@ -19,7 +19,8 @@ use crate::{
entity::{AllocAtWithoutReplacement, Entities, Entity, EntityLocation},
event::{Event, Events},
query::{DebugCheckedUnwrap, QueryState, ReadOnlyWorldQuery, WorldQuery},
storage::{Column, ComponentSparseSet, ResourceData, SparseSet, Storages, TableRow},
removal_detection::RemovedComponentEvents,
storage::{Column, ComponentSparseSet, ResourceData, Storages, TableRow},
system::Resource,
};
use bevy_ptr::{OwningPtr, Ptr};
@ -62,7 +63,7 @@ pub struct World {
pub(crate) archetypes: Archetypes,
pub(crate) storages: Storages,
pub(crate) bundles: Bundles,
pub(crate) removed_components: SparseSet<ComponentId, Vec<Entity>>,
pub(crate) removed_components: RemovedComponentEvents,
/// Access cache used by [WorldCell]. Is only accessed in the `Drop` impl of `WorldCell`.
pub(crate) archetype_component_access: UnsafeCell<ArchetypeComponentAccess>,
pub(crate) change_tick: AtomicU32,
@ -165,6 +166,12 @@ impl World {
&self.bundles
}
/// Retrieves this world's [`RemovedComponentEvents`] collection
#[inline]
pub fn removed_components(&self) -> &RemovedComponentEvents {
&self.removed_components
}
/// Retrieves a [`WorldCell`], which safely enables multiple mutable World accesses at the same
/// time, provided those accesses do not conflict with each other.
#[inline]
@ -666,12 +673,9 @@ impl World {
/// assert!(!transform.is_changed());
/// ```
///
/// [`RemovedComponents`]: crate::system::RemovedComponents
/// [`RemovedComponents`]: crate::removal_detection::RemovedComponents
pub fn clear_trackers(&mut self) {
for entities in self.removed_components.values_mut() {
entities.clear();
}
self.removed_components.update();
self.last_change_tick = self.increment_change_tick();
}
@ -768,12 +772,12 @@ impl World {
/// Returns an iterator of entities that had components of type `T` removed
/// since the last call to [`World::clear_trackers`].
pub fn removed<T: Component>(&self) -> std::iter::Cloned<std::slice::Iter<'_, Entity>> {
if let Some(component_id) = self.components.get_id(TypeId::of::<T>()) {
self.removed_with_id(component_id)
} else {
[].iter().cloned()
}
pub fn removed<T: Component>(&self) -> impl DoubleEndedIterator<Item = Entity> + '_ {
self.components
.get_id(TypeId::of::<T>())
.map(|component_id| self.removed_with_id(component_id))
.into_iter()
.flatten()
}
/// Returns an iterator of entities that had components with the given `component_id` removed
@ -781,12 +785,12 @@ impl World {
pub fn removed_with_id(
&self,
component_id: ComponentId,
) -> std::iter::Cloned<std::slice::Iter<'_, Entity>> {
if let Some(removed) = self.removed_components.get(component_id) {
removed.iter().cloned()
} else {
[].iter().cloned()
}
) -> impl DoubleEndedIterator<Item = Entity> + '_ {
self.removed_components
.get(component_id)
.map(|removed| removed.iter_current_update_events().cloned())
.into_iter()
.flatten()
}
/// Initializes a new resource and returns the [`ComponentId`] created for it.

View file

@ -6,7 +6,8 @@ use bevy_ecs::{
entity::Entity,
event::EventReader,
query::{Changed, ReadOnlyWorldQuery, With, Without},
system::{Query, RemovedComponents, Res, ResMut, Resource},
removal_detection::RemovedComponents,
system::{Query, Res, ResMut, Resource},
};
use bevy_hierarchy::{Children, Parent};
use bevy_log::warn;
@ -233,9 +234,9 @@ pub fn flex_node_system(
(With<Node>, Changed<CalculatedSize>),
>,
children_query: Query<(Entity, &Children), (With<Node>, Changed<Children>)>,
removed_children: RemovedComponents<Children>,
mut removed_children: RemovedComponents<Children>,
mut node_transform_query: Query<(Entity, &mut Node, &mut Transform, Option<&Parent>)>,
removed_nodes: RemovedComponents<Node>,
mut removed_nodes: RemovedComponents<Node>,
) {
// assume one window for time being...
// TODO: Support window-independent scaling: https://github.com/bevyengine/bevy/issues/5621
@ -280,13 +281,13 @@ pub fn flex_node_system(
}
// clean up removed nodes
flex_surface.remove_entities(&removed_nodes);
flex_surface.remove_entities(removed_nodes.iter());
// update window children (for now assuming all Nodes live in the primary window)
flex_surface.set_window_children(primary_window_entity, root_node_query.iter());
// update and remove children
for entity in &removed_children {
for entity in removed_children.iter() {
flex_surface.try_remove_children(entity);
}
for (entity, children) in &children_query {

View file

@ -2,7 +2,8 @@ use bevy_ecs::{
entity::Entity,
event::EventWriter,
prelude::{Changed, Component, Resource},
system::{Commands, NonSendMut, Query, RemovedComponents},
removal_detection::RemovedComponents,
system::{Commands, NonSendMut, Query},
world::Mut,
};
use bevy_utils::{
@ -89,7 +90,7 @@ pub(crate) fn create_window<'a>(
pub struct WindowTitleCache(HashMap<Entity, String>);
pub(crate) fn despawn_window(
closed: RemovedComponents<Window>,
mut closed: RemovedComponents<Window>,
window_entities: Query<&Window>,
mut close_events: EventWriter<WindowClosed>,
mut winit_windows: NonSendMut<WinitWindows>,

View file

@ -51,10 +51,10 @@ fn remove_component(
}
}
fn react_on_removal(removed: RemovedComponents<MyComponent>, mut query: Query<&mut Sprite>) {
fn react_on_removal(mut removed: RemovedComponents<MyComponent>, mut query: Query<&mut Sprite>) {
// `RemovedComponents<T>::iter()` returns an iterator with the `Entity`s that had their
// `Component` `T` (in this case `MyComponent`) removed at some point earlier during the frame.
for entity in removed.iter() {
for entity in &mut removed {
if let Ok(mut sprite) = query.get_mut(entity) {
sprite.color.set_r(0.0);
}