Skip to content

Commit

Permalink
chore!: remove removed filter
Browse files Browse the repository at this point in the history
Component removals for entities is error prone as it only tracks
entities which are still alive and they quickly fall prone to pitfalls
when used inside queries.

Use the subscription mechanism instead, which will also allow you to
store the value of the removed component
  • Loading branch information
ten3roberts committed Oct 28, 2023
1 parent 584b51e commit c803a34
Show file tree
Hide file tree
Showing 12 changed files with 76 additions and 260 deletions.
4 changes: 0 additions & 4 deletions src/archetype/changes.rs
Original file line number Diff line number Diff line change
Expand Up @@ -18,10 +18,6 @@ pub struct ChangeList {
}

impl ChangeList {
pub(crate) const fn new() -> Self {
Self { inner: Vec::new() }
}

// #[cfg(debug_assertions)]
// fn assert_normal(&self, msg: &str) {
// let this = self.iter().flat_map(|v| v.slice).collect_vec();
Expand Down
50 changes: 1 addition & 49 deletions src/archetype/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -267,8 +267,6 @@ impl Cell {
/// Stored as columns of contiguous component data.
pub struct Archetype {
cells: BTreeMap<ComponentKey, Cell>,
/// Stores removals of components which transferred the entities to this archetype
removals: BTreeMap<ComponentKey, ChangeList>,
/// Slot to entity id
pub(crate) entities: Vec<Entity>,

Expand All @@ -295,7 +293,6 @@ impl Archetype {
pub(crate) fn empty() -> Self {
Self {
cells: BTreeMap::new(),
removals: BTreeMap::new(),
incoming: BTreeMap::new(),
entities: Vec::new(),
children: Default::default(),
Expand All @@ -320,7 +317,6 @@ impl Archetype {

Self {
cells,
removals: BTreeMap::new(),
incoming: BTreeMap::new(),
entities: Vec::new(),
children: Default::default(),
Expand Down Expand Up @@ -376,10 +372,6 @@ impl Archetype {
debug_assert!(existing.is_none());
}

fn push_removed(&mut self, key: ComponentKey, change: Change) {
self.removals.entry(key).or_default().set(change);
}

pub(crate) fn borrow<T: ComponentValue>(
&self,
component: ComponentKey,
Expand Down Expand Up @@ -441,10 +433,6 @@ impl Archetype {
}
}

pub(crate) fn removals(&self, component: ComponentKey) -> Option<&ChangeList> {
self.removals.get(&component)
}

/// Get a component from the entity at `slot`
pub(crate) fn get_mut<T: ComponentValue>(
&self,
Expand Down Expand Up @@ -659,12 +647,9 @@ impl Archetype {
dst: &mut Self,
slot: Slot,
mut on_drop: impl FnMut(ComponentDesc, *mut u8),
tick: u32,
) -> (Slot, Option<(Entity, Slot)>) {
let id = self.entity(slot).expect("Invalid entity");

let last = self.len() - 1;

let dst_slot = dst.allocate(id);

for (&key, cell) in &mut self.cells {
Expand All @@ -682,21 +667,9 @@ impl Archetype {
});

cell.take(slot, &mut on_drop);
// Make the destination know of the component that was removed for the entity to
// get there
dst.push_removed(key, Change::new(Slice::single(dst_slot), tick));
}
}

// Make sure to carry over removed events
for (key, removed) in &mut self.removals {
let dst = dst.removals.entry(*key).or_default();
removed.swap_remove_with(slot, last, |mut v| {
v.slice = Slice::single(dst_slot);
dst.set(v);
})
}

let swapped = self.remove_slot(slot);

(dst_slot, swapped)
Expand Down Expand Up @@ -733,13 +706,6 @@ impl Archetype {
cell.take(slot, &mut on_move)
}

let last = self.len() - 1;

// Remove the component removals for slot
for removed in self.removals.values_mut() {
removed.swap_remove_with(slot, last, |_| {});
}

self.remove_slot(slot)
}

Expand All @@ -766,7 +732,7 @@ impl Archetype {
///
/// Leaves `self` empty.
/// Returns the new location of all entities
pub fn move_all(&mut self, dst: &mut Self, tick: u32) -> Vec<(Entity, Slot)> {
pub fn move_all(&mut self, dst: &mut Self) -> Vec<(Entity, Slot)> {
let len = self.len();
if len == 0 {
return Vec::new();
Expand Down Expand Up @@ -810,21 +776,9 @@ impl Archetype {
});

cell.clear();
dst.push_removed(*key, Change::new(dst_slots, tick))
}
}

// Make sure to carry over removed events
for (key, removed) in &mut self.removals {
let dst = dst.removals.entry(*key).or_default();
removed.inner.drain(..).for_each(|mut change| {
change.slice.start += dst_slots.start;
change.slice.end += dst_slots.start;

dst.set(change);
})
}

debug_assert_eq!(self.len(), 0);

entities.into_iter().zip_eq(dst_slots.iter()).collect_vec()
Expand Down Expand Up @@ -912,8 +866,6 @@ impl Archetype {
})
}

self.removals.clear();

ArchetypeDrain {
entities: mem::take(&mut self.entities),
cells: mem::take(&mut self.cells),
Expand Down
17 changes: 1 addition & 16 deletions src/component.rs
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ use crate::{
buffer::ComponentBuffer,
entity::EntityKind,
fetch::MaybeMut,
filter::{ChangeFilter, RemovedFilter, With, WithRelation, Without, WithoutRelation},
filter::{ChangeFilter, With, WithRelation, Without, WithoutRelation},
metadata::Metadata,
relation::RelationExt,
vtable::{ComponentVTable, UntypedVTable},
Expand Down Expand Up @@ -265,21 +265,6 @@ impl<T: ComponentValue> Component<T> {
ChangeFilter::new(self, kind)
}

/// Construct a fine grained component remove detection filter.
///
/// **Note**: This filter will yield entities **which are still alive** for which `component` was
/// removed, and the rest of the fetch matches.
///
/// Since a query only iterates the living world, this filter does not work for despawned entities.
///
/// In other words, queries only return valid entities.
///
/// To capture *all* removed components, including despawned entities, prefer
/// [`World::subscribe`](crate::World::subscribe).
pub fn removed(self) -> RemovedFilter<T> {
RemovedFilter::new(self)
}

/// Construct a new filter yielding entities without this component.
pub fn without(self) -> Without {
Without {
Expand Down
4 changes: 2 additions & 2 deletions src/fetch/ext.rs
Original file line number Diff line number Diff line change
Expand Up @@ -175,8 +175,8 @@ pub trait FetchExt: Sized {
self.transform_fetch(Modified)
}

/// Transform the fetch into a fetch where each constituent part tracks and yields for insert
/// events.
/// Transform the fetch into a fetch where each constituent part tracks and yields for
/// component addition events.
///
/// This is different from E.g; `(a().modified(), b().modified())` as it implies only when
/// *both* `a` and `b` are modified in the same iteration, which is seldom useful.
Expand Down
86 changes: 13 additions & 73 deletions src/filter/change.rs
Original file line number Diff line number Diff line change
Expand Up @@ -8,12 +8,10 @@ use crate::fetch::{FetchAccessData, FetchPrepareData, PreparedFetch, RandomFetch
use crate::system::Access;
use crate::util::Ptr;
use crate::{
archetype::{ChangeKind, ChangeList, Slice},
archetype::{ChangeKind, Slice},
Component, Fetch, FetchItem,
};

static EMPTY_CHANGELIST: ChangeList = ChangeList::new();

#[derive(Clone)]
/// Filter which only yields for change events
pub struct ChangeFilter<T> {
Expand Down Expand Up @@ -63,7 +61,7 @@ where

type Prepared = PreparedChangeFilter<'w, T>;

fn prepare(&'w self, data: crate::fetch::FetchPrepareData<'w>) -> Option<Self::Prepared> {
fn prepare(&'w self, data: FetchPrepareData<'w>) -> Option<Self::Prepared> {
let cell = data.arch.cell(self.component.key())?;
let guard = cell.borrow();

Expand Down Expand Up @@ -187,68 +185,15 @@ impl<'w, 'q, T: ComponentValue> PreparedFetch<'q> for PreparedChangeFilter<'w, T
}
}

#[derive(Clone)]
/// Filter which only yields removed components.
///
/// See: [`Component::removed`](crate::Component::removed)
pub struct RemovedFilter<T> {
component: Component<T>,
}

impl<T: ComponentValue> core::fmt::Debug for RemovedFilter<T> {
fn fmt(&self, f: &mut core::fmt::Formatter<'_>) -> core::fmt::Result {
f.debug_struct("RemovedFilter")
.field("component", &self.component)
.finish()
}
}

impl<T: ComponentValue> RemovedFilter<T> {
/// Create a new removed filter
pub(crate) fn new(component: Component<T>) -> Self {
Self { component }
}
}

impl<'q, T: ComponentValue> FetchItem<'q> for RemovedFilter<T> {
type Item = ();
}
impl<'w, T: ComponentValue> Fetch<'w> for RemovedFilter<T> {
const MUTABLE: bool = false;

type Prepared = PreparedRemoveFilter<'w>;

fn prepare(&self, data: FetchPrepareData<'w>) -> Option<Self::Prepared> {
let changes = data
.arch
.removals(self.component.key())
.unwrap_or(&EMPTY_CHANGELIST);

Some(PreparedRemoveFilter {
changes: changes.as_slice(),
cursor: ChangeCursor::new(data.old_tick),
})
}

fn filter_arch(&self, _: FetchAccessData) -> bool {
true
}

#[inline]
fn access(&self, _: FetchAccessData, _: &mut Vec<Access>) {}

fn describe(&self, f: &mut Formatter<'_>) -> core::fmt::Result {
write!(f, "removed {}", self.component.name())
}
}

#[doc(hidden)]
pub struct PreparedRemoveFilter<'w> {
#[cfg(test)]
pub struct ChangeFetch<'w> {
changes: &'w [Change],
cursor: ChangeCursor,
}

impl<'w> PreparedRemoveFilter<'w> {
#[cfg(test)]
impl<'w> ChangeFetch<'w> {
pub fn new(changes: &'w [Change], new_tick: u32) -> Self {
Self {
changes,
Expand All @@ -257,22 +202,17 @@ impl<'w> PreparedRemoveFilter<'w> {
}
}

impl<'w> core::fmt::Debug for PreparedRemoveFilter<'w> {
fn fmt(&self, f: &mut Formatter<'_>) -> core::fmt::Result {
f.debug_struct("PreparedRemoveFilter")
.finish_non_exhaustive()
}
}

impl<'q, 'w> RandomFetch<'q> for PreparedRemoveFilter<'w> {
#[cfg(test)]
impl<'q, 'w> RandomFetch<'q> for ChangeFetch<'w> {
#[inline]
unsafe fn fetch_shared(&'q self, _: Slot) -> Self::Item {}

#[inline]
unsafe fn fetch_shared_chunk(_: &Self::Chunk, _: Slot) -> Self::Item {}
}

impl<'w, 'q> PreparedFetch<'q> for PreparedRemoveFilter<'w> {
#[cfg(test)]
impl<'w, 'q> PreparedFetch<'q> for ChangeFetch<'w> {
type Item = ();
type Chunk = ();

Expand Down Expand Up @@ -311,7 +251,7 @@ mod test {
Change::new(Slice::new(100, 200), 4),
];

let mut filter = PreparedRemoveFilter {
let mut filter = ChangeFetch {
changes: &changes[..],
cursor: ChangeCursor::new(2),
};
Expand Down Expand Up @@ -342,7 +282,7 @@ mod test {
Change::new(Slice::new(100, 200), 4),
];

let filter = PreparedRemoveFilter {
let filter = ChangeFetch {
changes: &changes[..],
cursor: ChangeCursor::new(2),
};
Expand All @@ -369,7 +309,7 @@ mod test {
Change::new(Slice::new(100, 200), 4),
];

let filter = PreparedRemoveFilter {
let filter = ChangeFetch {
changes: &changes[..],
cursor: ChangeCursor::new(2),
};
Expand Down
15 changes: 7 additions & 8 deletions src/filter/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ use crate::{
ArchetypeSearcher, Entity, Fetch, FetchItem,
};

pub use change::{ChangeFilter, RemovedFilter};
pub use change::ChangeFilter;
pub use cmp::{Cmp, Equal, Greater, GreaterEq, Less, LessEq};
pub use constant::{All, Nothing};
pub use set::{And, Not, Or, Union};
Expand Down Expand Up @@ -165,7 +165,6 @@ gen_bitops! {
ChangeFilter[T];
Nothing[];
Or[T];
RemovedFilter[T];
WithObject[];
WithRelation[];
With[];
Expand Down Expand Up @@ -637,7 +636,7 @@ mod tests {
use crate::{
archetype::{ArchetypeId, Change, ChangeKind, ChangeList},
component,
filter::change::PreparedRemoveFilter,
filter::change::ChangeFetch,
World,
};

Expand All @@ -654,7 +653,7 @@ mod tests {
changes.set(Change::new(Slice::new(784, 800), 7));
changes.set(Change::new(Slice::new(945, 1139), 8));

let filter = PreparedRemoveFilter::new(changes.as_slice(), 2);
let filter = ChangeFetch::new(changes.as_slice(), 2);

// The whole "archetype"
let slots = Slice::new(0, 1238);
Expand Down Expand Up @@ -690,8 +689,8 @@ mod tests {
let slots = Slice::new(0, 1000);

// Or
let a = PreparedRemoveFilter::new(changes_1.as_slice(), 1);
let b = PreparedRemoveFilter::new(changes_2.as_slice(), 2);
let a = ChangeFetch::new(changes_1.as_slice(), 1);
let b = ChangeFetch::new(changes_2.as_slice(), 2);

let filter = Or((Some(a), Some(b)));

Expand All @@ -707,8 +706,8 @@ mod tests {

// And

let a = PreparedRemoveFilter::new(changes_1.as_slice(), 1);
let b = PreparedRemoveFilter::new(changes_2.as_slice(), 2);
let a = ChangeFetch::new(changes_1.as_slice(), 1);
let b = ChangeFetch::new(changes_2.as_slice(), 2);

let filter = And(a, b);

Expand Down
Loading

0 comments on commit c803a34

Please sign in to comment.