Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
334 changes: 334 additions & 0 deletions crates/bevy_ecs/src/entity_rc.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,334 @@
//! This module holds utilities for reference-counting of entities, similar to [`Arc`]. This enables
//! automatic cleanup of entities that can be referenced in multiple places.

use core::{
fmt::{Debug, Formatter},
ops::Deref,
};

use bevy_platform::sync::{Arc, Weak};
use concurrent_queue::ConcurrentQueue;

use crate::{entity::Entity, system::Commands};

/// A reference count for an entity.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a bad definition, since it doesn't explain what this does to someone who isn't familiar with reference counting.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The first line of docs also need to capture that it can store data of type T.

///
/// This "handle" also stores some optional data, allowing users to customize any shared data
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Of type T, right? This should be explicitly clarified.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The model needs to be laid out more explicitly. Which entity owns the actual data? How is the shared data stored? How do you generate handles? Is there a distinction between the entity which "owns" the data (currently Assets) and entities which "reference" the data (currently handles)?

Doc tests may be helpful to explain usage :)

/// between all references to the entity.
///
/// Once all [`EntityRc`] instances have been dropped, the entity will be queued for destruction.
/// This means it is possible for the entity to still exist, while its [`EntityRc`] has been
/// dropped.
///
/// The reverse is also true: a held [`EntityRc`] does not guarantee that the entity still exists.
/// It can still be explicitly despawned, so users should try to be resilient to this.
///
/// This type has similar semantics to [`Arc`].
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This note needs a bit more explanation: what semantics or properties are held in common?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This analogy might be a helpful jumping off point to explain how this is intended to be used more broadly. You should not assume that your audience is familiar with Arc, especially at a level beyond "IDK it can be used to get around the borrow checker in a thread-safe way".

#[derive(Debug)]
pub struct EntityRc<T: Send + Sync + 'static = ()>(Arc<EntityRcInner<T>>);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should mention that T defaults to (), and that this means that no data is shared.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think that this terminology is particularly clear. The fact that it's an entity is somewhat incidental: the point is that this is a component which holds shared, reference-counted data.

Something like RcData<T> or SharedData<T> makes much more sense to me.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I disagree - the point is that the entity is the thing that should be considered "reference-counted". The fact that there is data is what I consider "incidental". We could just as easily have thrown out the T, but it just makes it more convenient to include some data so all handles can access it.

If the entity part weren't important, we would just tell people to use Arc, since that has exactly the same semantics.


impl<T: Send + Sync + 'static> Clone for EntityRc<T> {
fn clone(&self) -> Self {
Self(self.0.clone())
}
}

impl<T: Send + Sync + 'static> EntityRc<T> {
/// Creates a new [`EntityWeak`] referring to the same entity (and reference count).
pub fn downgrade(this: &Self) -> EntityWeak<T> {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's surprising to me that a method called downgrade takes &self rather than self. Doesn't this leave the original strong EntityRc alive?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This surprised me as well! This is exactly what https://doc.rust-lang.org/stable/std/sync/struct.Arc.html#method.downgrade does. This does indeed leave the original EntityRc alive, but I guess in a way that makes sense. Otherwise it also implicitly turns it into a drop, which could just immediately return you a Weak that is already invalid.

Also Weak has their own reference counting, so I'm guessing it's no faster to take an owned Arc.

EntityWeak {
entity: this.0.entity,
weak: Arc::downgrade(&this.0),
}
}

/// Returns the entity this reference count refers to.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably worth inlining these just in case.

pub fn entity(&self) -> Entity {
self.0.entity
}
}

impl<T: Send + Sync + 'static> Deref for EntityRc<T> {
type Target = T;

fn deref(&self) -> &Self::Target {
&self.0.payload
}
}

/// A "non-owning" reference to a reference-counted entity.
///
/// Holding this handle does not guarantee that the entity will not be cleaned up. This handle
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The double negative here makes it very hard for me to parse this sentence.

/// allows "upgrading" to an [`EntityRc`], if the reference count is still positive, which **will**
/// avoid clean ups.
///
/// This type has similar semantics to [`Weak`].
#[derive(Debug)]
pub struct EntityWeak<T: Send + Sync + 'static = ()> {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See this comment complaining about the names.

/// The entity being referenced.
///
/// This allows the entity to be referenced even if the reference count has expired. This is
/// generally useful for cleanup operations.
entity: Entity,
/// The underlying weak reference.
weak: Weak<EntityRcInner<T>>,
}

impl<T: Send + Sync + 'static> Clone for EntityWeak<T> {
fn clone(&self) -> Self {
Self {
entity: self.entity,
weak: self.weak.clone(),
}
}
}

impl<T: Send + Sync + 'static> EntityWeak<T> {
/// Attempts to upgrade the weak reference into an [`EntityRc`], which can keep the entity alive
/// if successful.
///
/// Returns [`None`] if all [`EntityRc`]s were previously dropped. This does not necessarily
/// mean that the entity has been despawned yet.
pub fn upgrade(&self) -> Option<EntityRc<T>> {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is returning an Option traditional here? The semantics are fairly non-intuitive just reading the signature to me; I would definitely prefer a Result.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

self.weak.upgrade().map(EntityRc)
}

/// Returns the entity this weak reference count refers to.
///
/// The entity may or may not have been despawned (since the [`EntityRc`]s may have all been
/// dropped). In order to guarantee the entity remains alive, use [`Self::upgrade`] first. This
/// accessor exists to support cleanup operations.
pub fn entity(&self) -> Entity {
self.entity
}
}

/// Data stored inside the shared data for [`EntityRc`].
struct EntityRcInner<T: Send + Sync + 'static> {
/// The concurrent queue to notify when dropping this type.
drop_notifier: Arc<ConcurrentQueue<Entity>>,
/// The entity this reference count refers to.
entity: Entity,
/// The data that is shared with all reference counts for easy access.
payload: T,
}

// Manual impl of Debug to avoid debugging the drop_notifier.
impl<T: Send + Sync + 'static + Debug> Debug for EntityRcInner<T> {
fn fmt(&self, f: &mut Formatter<'_>) -> core::fmt::Result {
f.debug_struct("EntityRcInner")
.field("entity", &self.entity)
.field("payload", &self.payload)
.finish()
}
}

impl<T: Send + Sync + 'static> Drop for EntityRcInner<T> {
fn drop(&mut self) {
// Try to push the entity. If the notifier is closed for some reason, that's ok.
let _ = self.drop_notifier.push(self.entity);
}
}

/// Allows creating [`EntityRc`] and handles syncing them with the world.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This needs another paragraph of explanation below explaining how this actually works in practice, and why you might want to do this.

///
/// Note: this can produce [`EntityRc`] containing any "payload", since the payload is not
/// accessible during despawn time. This is because it's possible for the entity to be despawned
/// explicitly even though an [`EntityRc`] is still held - callers should be resilient to this.
pub struct EntityRcSource {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remember to update this name too if the naming convention is changed.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there any reason this isn't Clone?

Is there any reason someone would want to have two different EntityRcSources? Maybe to have two different clean up cycles? But that would only be useful if the program isn't resilient to the entities being despawned. If there's not a strong reason to have multiple of these for one world, can we just put this in World and handle_dropped_rcs directly in World::flush or something similar?

/// The concurrent queue used for communicating drop events of [`EntityRcInner`]s.
// Note: this could be a channel, but `bevy_ecs` already depends on `concurrent_queue`, so use
// it as a simple channel.
drop_notifier: Arc<ConcurrentQueue<Entity>>,
}

impl Default for EntityRcSource {
fn default() -> Self {
Self::new()
}
}

impl EntityRcSource {
/// Creates a new source of [`EntityRc`]s.
///
/// Generally, only one [`EntityRcSource`] is needed, but having separate ones allows clean up
/// operations to occur at different times or different rates.
pub fn new() -> Self {
Self {
drop_notifier: Arc::new(ConcurrentQueue::unbounded()),
}
}

/// Creates a new [`EntityRc`] for `entity`, storing the given `payload` in that [`EntityRc`].
///
/// It is up to the caller to ensure that the provided `entity` does not already have an
/// [`EntityRc`] associated with it. Providing an `entity` which already has an [`EntityRc`]
/// will result in two reference counts tracking the same entity and both attempting to despawn
/// the entity (and more importantly, for a held [`EntityRc`] to have its entity despawned
/// anyway).
///
/// Providing an `entity` allows this method to be compatible with regular entity allocation
/// ([`EntityAllocator`](crate::entity::EntityAllocator)), remote entity allocation
/// ([`RemoteAllocator`](crate::entity::RemoteAllocator)), or even taking an existing entity and
/// making it reference counted.
pub fn create_rc<T: Send + Sync + 'static>(&self, entity: Entity, payload: T) -> EntityRc<T> {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we expect T to almost always be (), might be worth making a convenience method here.

EntityRc(Arc::new(EntityRcInner {
drop_notifier: self.drop_notifier.clone(),
entity,
payload,
}))
}

/// Handles any dropped [`EntityRc`]s and despawns the corresponding entities.
///
/// This must be called regularly in order for reference-counted entities to actually be cleaned
/// up.
///
/// Note: if you have exclusive world access (`&mut World`), you can use
/// [`World::commands`](crate::world::World::commands) to get an instance of [`Commands`].
pub fn handle_dropped_rcs(&self, commands: &mut Commands) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't like the commands indirection here, but I'm not terribly opposed to it either. But it would be faster (fewer commands, tighter loop, etc) if this took &mut World. Maybe then have a single command that could call that &mut World function?

for entity in self.drop_notifier.try_iter() {
let Ok(mut entity) = commands.get_entity(entity) else {
// We intended to despawn the entity - and the entity is despawned. Someone did our
// work for us!
continue;
};
// Also only try to despawn here - if the entity is despawned when this is run, it's not
// a problem.
entity.try_despawn();
}
}
}

#[cfg(test)]
mod tests {
use crate::{
entity_rc::{EntityRc, EntityRcSource},
world::World,
};

/// Handles any dropped entities, and flushes the world.
fn handle_drops(world: &mut World, source: &EntityRcSource) {
source.handle_dropped_rcs(&mut world.commands());
world.flush();
}

#[test]
fn simple_counting() {
let mut world = World::new();
let source = EntityRcSource::new();

let entity_1 = world.spawn_empty().id();
let rc_1_1 = source.create_rc(entity_1, ());

let entity_2 = world.spawn_empty().id();
let rc_2_1 = source.create_rc(entity_2, ());

let entity_3 = world.spawn_empty().id();
let rc_3_1 = source.create_rc(entity_3, ());

handle_drops(&mut world, &source);

assert!(world.get_entity(entity_1).is_ok());
assert!(world.get_entity(entity_2).is_ok());
assert!(world.get_entity(entity_3).is_ok());

drop(rc_2_1);

// Dropping the rc doesn't do anything until we handle the drops.
assert!(world.get_entity(entity_1).is_ok());
assert!(world.get_entity(entity_2).is_ok());
assert!(world.get_entity(entity_3).is_ok());

handle_drops(&mut world, &source);

// entity_2 is despawned.
assert!(world.get_entity(entity_1).is_ok());
assert!(world.get_entity(entity_2).is_err());
assert!(world.get_entity(entity_3).is_ok());

// Cloning the rc and then dropping the original doesn't drop the entity.
let rc_1_2 = rc_1_1.clone();
drop(rc_1_1);
handle_drops(&mut world, &source);

assert!(world.get_entity(entity_1).is_ok());
assert!(world.get_entity(entity_3).is_ok());

// Dropping all handles will.
drop(rc_1_2);
handle_drops(&mut world, &source);

assert!(world.get_entity(entity_1).is_err());
assert!(world.get_entity(entity_3).is_ok());

// Cloning the handle many times doesn't do anything.
let rc_3_2 = rc_3_1.clone();
let rc_3_3 = rc_3_1.clone();
let rc_3_4 = rc_3_1.clone();
let rc_3_5 = rc_3_1.clone();
handle_drops(&mut world, &source);

assert!(world.get_entity(entity_3).is_ok());

// Dropping the rc fewer times than clones still does nothing.
for rc in [rc_3_1, rc_3_2, rc_3_3, rc_3_4] {
drop(rc);
handle_drops(&mut world, &source);
assert!(world.get_entity(entity_3).is_ok());
}

// Dropping the last rc finally drops the entity.
drop(rc_3_5);
handle_drops(&mut world, &source);
assert!(world.get_entity(entity_3).is_err());
}

#[test]
fn weak_handles_dont_keep_entity_alive() {
let mut world = World::new();
let source = EntityRcSource::new();

let entity = world.spawn_empty().id();
let rc = source.create_rc(entity, ());

let weak_1 = EntityRc::downgrade(&rc);
let _weak_2 = EntityRc::downgrade(&rc);
let _weak_3 = EntityRc::downgrade(&rc);
handle_drops(&mut world, &source);
assert!(world.get_entity(entity).is_ok());

// Dropping the one rc is enough to despawn the entity.
drop(rc);

// Bonus: trying to get an rc out of an expired weak doesn't work.
assert!(weak_1.upgrade().is_none());

handle_drops(&mut world, &source);
assert!(world.get_entity(entity).is_err());
}

#[test]
fn weak_handles_can_upgrade() {
let mut world = World::new();
let source = EntityRcSource::new();

let entity = world.spawn_empty().id();
let rc = source.create_rc(entity, ());

let weak = EntityRc::downgrade(&rc);

let upgraded_weak = weak.upgrade();
assert!(upgraded_weak.is_some());

// Dropping the original rc does nothing, since we upgraded the weak.
drop(rc);
handle_drops(&mut world, &source);
assert!(world.get_entity(entity).is_ok());

// Also dropping the upgraded weak (now just an rc) will drop the entity.
drop(upgraded_weak);
handle_drops(&mut world, &source);
assert!(world.get_entity(entity).is_err());
}
}
1 change: 1 addition & 0 deletions crates/bevy_ecs/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@ pub mod change_detection;
pub mod component;
pub mod entity;
pub mod entity_disabling;
pub mod entity_rc;
pub mod error;
pub mod event;
pub mod hierarchy;
Expand Down
Loading