Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make a dynamically applicable version of Bundle #3694

Closed
wants to merge 6 commits into from
Closed
Show file tree
Hide file tree
Changes from 3 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
106 changes: 94 additions & 12 deletions crates/bevy_ecs/src/bundle.rs
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,8 @@ use std::{any::TypeId, collections::HashMap};
/// particularly useful for spawning multiple empty entities by using
/// [`Commands::spawn_batch`](crate::system::Commands::spawn_batch).
///
/// To use a bundle dynamically, use [`Box<dyn ApplicableBundle>`](`ApplicableBundle`)
DJMcNab marked this conversation as resolved.
Show resolved Hide resolved
///
/// # Examples
///
/// Typically, you will simply use `#[derive(Bundle)]` when creating your own `Bundle`. Each
Expand Down Expand Up @@ -75,26 +77,106 @@ use std::{any::TypeId, collections::HashMap};
/// bundle, in the _exact_ order that [`Bundle::get_components`] is called.
/// - [`Bundle::from_components`] must call `func` exactly once for each [`ComponentId`] returned by
/// [`Bundle::component_ids`].
pub unsafe trait Bundle: Send + Sync + 'static {
/// Gets this [`Bundle`]'s component ids, in the order of this bundle's [`Component`]s
fn component_ids(components: &mut Components, storages: &mut Storages) -> Vec<ComponentId>;

pub unsafe trait Bundle: ApplicableBundle + Sized {
/// Calls `func`, which should return data for each component in the bundle, in the order of
/// this bundle's [`Component`]s
///
/// # Safety
/// Caller must return data for each component in the bundle, in the order of this bundle's
/// [`Component`]s
unsafe fn from_components(func: impl FnMut() -> *mut u8) -> Self
where
Self: Sized;
unsafe fn from_components(func: impl FnMut() -> *mut u8) -> Self;

/// Gets this [`Bundle`]'s component ids, in the order of this bundle's [`Component`]s
fn component_ids(components: &mut Components, storages: &mut Storages) -> Vec<ComponentId>;
DJMcNab marked this conversation as resolved.
Show resolved Hide resolved

/// Calls `func` on each value, in the order of this bundle's [`Component`]s. This will
/// [`std::mem::forget`] the bundle fields, so callers are responsible for dropping the fields
/// if that is desirable.
fn get_components(self, func: impl FnMut(*mut u8));
DJMcNab marked this conversation as resolved.
Show resolved Hide resolved
}

mod sealed {
use super::{ApplicableBundle, Bundle};

pub trait SealedApplicableBundle {}
impl<T> SealedApplicableBundle for T where T: Bundle {}
impl SealedApplicableBundle for Box<dyn ApplicableBundle> {}
}

/// A bundle which can be applied to a world
DJMcNab marked this conversation as resolved.
Show resolved Hide resolved
///
/// # Safety
/// The bundle returned from `init_bundle_info` is the same as used for `get_component_box`.
DJMcNab marked this conversation as resolved.
Show resolved Hide resolved
///
/// This trait is sealed and cannot be implemented outside of `bevy_ecs`
DJMcNab marked this conversation as resolved.
Show resolved Hide resolved
///
/// However, it is implemented for every type which implements [`Bundle`]
pub unsafe trait ApplicableBundle:
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe call this DynBundle?

Copy link
Member Author

Choose a reason for hiding this comment

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

Possibly? It is not limited to dynamic use cases; just cases where self is available. This is the main entry way to 'single bundle insertion'.

Normal Bundle is absolutely static, but that doesn't mean that ApplicableBundle is not static.

Copy link
Member

Choose a reason for hiding this comment

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

My preference here is either:

  1. Call this Bundle, and use StaticBundle for the other trait.
  2. Call this InsertableBundle or CreationBundle.

The former is nice, because in 99% of cases, users only need the methods from this trait to work with what they need. Removing bundles is typically not very useful. However it's a bit confusing, as deriving Bundle would derive a trait and its super trait.

The latter is just playing with synonyms to better reflect intent in terms of things users are familiar with.

Copy link
Member Author

Choose a reason for hiding this comment

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

I did consider option 1. My concern was that it requires any manual impls of Bundle to know that it's actually a different trait which gets implemented. This change is currently non-breaking (as far as I can tell), which is quite neat.

Copy link
Member

Choose a reason for hiding this comment

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

IMO I think that manual impls of Bundle are going to be exceedingly rare, and the compiler errors should be quite useful. The fact that Bundle would now be unsafe would also encourage people to Read the Docs.

Copy link
Member Author

Choose a reason for hiding this comment

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

? Bundle has always been unsafe. I do agree though that manual impls are going to be rare.

Copy link
Member Author

Choose a reason for hiding this comment

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

One other thing is that the renaming would require a careful audit of documentation etc. as Bundle can no longer be used for removal operations. I don't really want to do that in this PR if it can be avoided.

Send + Sync + 'static + sealed::SealedApplicableBundle
{
fn init_bundle_info<'a>(
&self,
bundles: &'a mut Bundles,
components: &mut Components,
storages: &mut Storages,
) -> &'a BundleInfo;
// Same requirements as [`Bundle::get_components`]
fn get_components_box(self: Box<Self>, func: &mut dyn FnMut(*mut u8));
// Same requirements as [`Bundle::get_components`]
fn get_components_self(self, func: impl FnMut(*mut u8))
where
Self: Sized;
}

unsafe impl<T> ApplicableBundle for T
where
T: Bundle,
{
fn init_bundle_info<'a>(
&self,
bundles: &'a mut Bundles,
components: &mut Components,
storages: &mut Storages,
) -> &'a BundleInfo {
bundles.init_info::<Self>(components, storages)
}

fn get_components_box(self: Box<Self>, func: &mut dyn FnMut(*mut u8)) {
Self::get_components(*self, func)
}

fn get_components_self(self, func: impl FnMut(*mut u8))
where
Self: Sized,
{
Self::get_components(self, func)
}
}

unsafe impl ApplicableBundle for Box<dyn ApplicableBundle> {
DJMcNab marked this conversation as resolved.
Show resolved Hide resolved
fn init_bundle_info<'a>(
&self,
bundles: &'a mut Bundles,
components: &mut Components,
storages: &mut Storages,
) -> &'a BundleInfo {
<dyn ApplicableBundle as ApplicableBundle>::init_bundle_info(
self, bundles, components, storages,
)
}

fn get_components_box(self: Box<Self>, func: &mut dyn FnMut(*mut u8)) {
<dyn ApplicableBundle as ApplicableBundle>::get_components_box(self, func)
DJMcNab marked this conversation as resolved.
Show resolved Hide resolved
}

fn get_components_self(self, mut func: impl FnMut(*mut u8))
where
Self: Sized,
{
<dyn ApplicableBundle as ApplicableBundle>::get_components_box(self, &mut func)
}
}

macro_rules! tuple_impl {
($($name: ident),*) => {
unsafe impl<$($name: Component),*> Bundle for ($($name,)*) {
Expand Down Expand Up @@ -261,7 +343,7 @@ impl BundleInfo {
/// `entity`, `bundle` must match this [`BundleInfo`]'s type
#[inline]
#[allow(clippy::too_many_arguments)]
unsafe fn write_components<T: Bundle>(
unsafe fn write_components<T: ApplicableBundle>(
&self,
table: &mut Table,
sparse_sets: &mut SparseSets,
Expand All @@ -274,7 +356,7 @@ impl BundleInfo {
// NOTE: get_components calls this closure on each component in "bundle order".
// bundle_info.component_ids are also in "bundle order"
let mut bundle_component = 0;
bundle.get_components(|component_ptr| {
bundle.get_components_self(|component_ptr| {
let component_id = *self.component_ids.get_unchecked(bundle_component);
match self.storage_types[bundle_component] {
StorageType::Table => {
Expand Down Expand Up @@ -412,7 +494,7 @@ impl<'a, 'b> BundleInserter<'a, 'b> {
/// `entity` must currently exist in the source archetype for this inserter. `archetype_index`
/// must be `entity`'s location in the archetype. `T` must match this [`BundleInfo`]'s type
#[inline]
pub unsafe fn insert<T: Bundle>(
pub unsafe fn insert<T: ApplicableBundle>(
&mut self,
entity: Entity,
archetype_index: usize,
Expand Down Expand Up @@ -540,7 +622,7 @@ impl<'a, 'b> BundleSpawner<'a, 'b> {
/// # Safety
/// `entity` must be allocated (but non-existent), `T` must match this [`BundleInfo`]'s type
#[inline]
pub unsafe fn spawn_non_existent<T: Bundle>(
pub unsafe fn spawn_non_existent<T: ApplicableBundle>(
&mut self,
entity: Entity,
bundle: T,
Expand All @@ -564,7 +646,7 @@ impl<'a, 'b> BundleSpawner<'a, 'b> {
/// # Safety
/// `T` must match this [`BundleInfo`]'s type
#[inline]
pub unsafe fn spawn<T: Bundle>(&mut self, bundle: T) -> Entity {
pub unsafe fn spawn<T: ApplicableBundle>(&mut self, bundle: T) -> Entity {
let entity = self.entities.alloc();
// SAFE: entity is allocated (but non-existent), `T` matches this BundleInfo's type
self.spawn_non_existent(entity, bundle);
Expand Down
2 changes: 1 addition & 1 deletion crates/bevy_ecs/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ pub mod prelude {
pub use crate::reflect::ReflectComponent;
#[doc(hidden)]
pub use crate::{
bundle::Bundle,
bundle::{ApplicableBundle, Bundle},
change_detection::DetectChanges,
component::Component,
entity::Entity,
Expand Down
25 changes: 14 additions & 11 deletions crates/bevy_ecs/src/system/commands/mod.rs
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
mod command_queue;

use crate::{
bundle::Bundle,
bundle::{ApplicableBundle, Bundle},
component::Component,
entity::{Entities, Entity},
world::World,
Expand Down Expand Up @@ -50,7 +50,7 @@ impl<'w, 's> Commands<'w, 's> {

/// Creates a new empty [`Entity`] and returns an [`EntityCommands`] builder for it.
///
/// To directly spawn an entity with a [`Bundle`] included, you can use
/// To directly spawn an entity with an [`ApplicableBundle`] included, you can use
/// [`spawn_bundle`](Self::spawn_bundle) instead of `.spawn().insert_bundle()`.
///
/// See [`World::spawn`] for more details.
Expand Down Expand Up @@ -104,9 +104,9 @@ impl<'w, 's> Commands<'w, 's> {
}
}

/// Spawns a [`Bundle`] without pre-allocating an [`Entity`]. The [`Entity`] will be allocated
/// Spawns an [`ApplicableBundle`] without pre-allocating an [`Entity`]. The [`Entity`] will be allocated
/// when this [`Command`] is applied.
pub fn spawn_and_forget(&mut self, bundle: impl Bundle) {
pub fn spawn_and_forget(&mut self, bundle: impl ApplicableBundle) {
self.queue.push(Spawn { bundle })
}

Expand All @@ -115,7 +115,7 @@ impl<'w, 's> Commands<'w, 's> {
/// This returns an [`EntityCommands`] builder, which enables inserting more components and
/// bundles using a "builder pattern".
///
/// Note that `bundle` is a [`Bundle`], which is a collection of components. [`Bundle`] is
/// Note that `bundle` is an [`ApplicableBundle`], which is a collection of components. [`ApplicableBundle`] is
/// automatically implemented for tuples of components. You can also create your own bundle
/// types by deriving [`derive@Bundle`].
///
Expand Down Expand Up @@ -158,7 +158,10 @@ impl<'w, 's> Commands<'w, 's> {
/// }
/// # example_system.system();
/// ```
pub fn spawn_bundle<'a, T: Bundle>(&'a mut self, bundle: T) -> EntityCommands<'w, 's, 'a> {
pub fn spawn_bundle<'a, T: ApplicableBundle>(
&'a mut self,
bundle: T,
) -> EntityCommands<'w, 's, 'a> {
let mut e = self.spawn();
e.insert_bundle(bundle);
e
Expand Down Expand Up @@ -376,7 +379,7 @@ impl<'w, 's, 'a> EntityCommands<'w, 's, 'a> {
self.entity
}

/// Adds a [`Bundle`] of components to the entity.
/// Adds an [`ApplicableBundle`] of components to the entity.
///
/// # Example
///
Expand Down Expand Up @@ -407,7 +410,7 @@ impl<'w, 's, 'a> EntityCommands<'w, 's, 'a> {
/// }
/// # add_combat_stats_system.system();
/// ```
pub fn insert_bundle(&mut self, bundle: impl Bundle) -> &mut Self {
pub fn insert_bundle(&mut self, bundle: impl ApplicableBundle) -> &mut Self {
self.commands.add(InsertBundle {
entity: self.entity,
bundle,
Expand Down Expand Up @@ -550,7 +553,7 @@ pub struct Spawn<T> {

impl<T> Command for Spawn<T>
where
T: Bundle,
T: ApplicableBundle,
{
fn write(self, world: &mut World) {
world.spawn().insert_bundle(self.bundle);
Expand All @@ -570,7 +573,7 @@ impl Command for GetOrSpawn {
pub struct SpawnBatch<I>
where
I: IntoIterator,
I::Item: Bundle,
I::Item: ApplicableBundle,
{
pub bundles_iter: I,
}
Expand Down Expand Up @@ -633,7 +636,7 @@ pub struct InsertBundle<T> {

impl<T> Command for InsertBundle<T>
where
T: Bundle + 'static,
T: ApplicableBundle + 'static,
{
fn write(self, world: &mut World) {
if let Some(mut entity) = world.get_entity_mut(self.entity) {
Expand Down
14 changes: 8 additions & 6 deletions crates/bevy_ecs/src/world/entity_ref.rs
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
use crate::{
archetype::{Archetype, ArchetypeId, Archetypes},
bundle::{Bundle, BundleInfo},
bundle::{ApplicableBundle, Bundle, BundleInfo},
change_detection::Ticks,
component::{Component, ComponentId, ComponentTicks, Components, StorageType},
entity::{Entities, Entity, EntityLocation},
Expand Down Expand Up @@ -189,12 +189,14 @@ impl<'w> EntityMut<'w> {
})
}

pub fn insert_bundle<T: Bundle>(&mut self, bundle: T) -> &mut Self {
pub fn insert_bundle<T: ApplicableBundle>(&mut self, bundle: T) -> &mut Self {
let change_tick = self.world.change_tick();
let bundle_info = self
.world
.bundles
.init_info::<T>(&mut self.world.components, &mut self.world.storages);
let bundle_info = bundle.init_bundle_info(
&mut self.world.bundles,
&mut self.world.components,
&mut self.world.storages,
);

let mut bundle_inserter = bundle_info.get_bundle_inserter(
&mut self.world.entities,
&mut self.world.archetypes,
Expand Down