bevy icon indicating copy to clipboard operation
bevy copied to clipboard

Better tools for working with dynamic collections of components

Open alice-i-cecile opened this issue 4 years ago • 23 comments

What problem does this solve or what need does it fill?

Working with dynamic collections of components (as might be used in advanced spawning patterns) is very painful at the moment: every straightforward path is blocked due to missing impls. This cannot be fixed in end user code, as they do not own the Component trait, or the common wrapper types like Vec and Box.

What solution would you like?

  1. Implement Component for Box<dyn Component>, which uses the box's data as the component.
  2. Do similarly for Rc and Arc (and any other common smart pointers).
  3. Implement Bundle for IntoIter<Item=Component>, which combined with the above would allow us to use structs like Vec<Box<dyn Component>> as bundles.

Notes:

  1. This will need to be done for each of the standard storage types, as we cannot vary the associated type found within the trait object.
  2. In practice, we may need DynClone for this pattern to actually work, which would probably force a DynComponent trait layer.

What alternative(s) have you considered?

Other possible approaches:

  • make Bundle itself object-safe, obviating 3. I don't think this will be feasible.
  • implement Component for &Component, since we can get these out of our boxes. This doesn't work nicely though, due to the need to clone components out of a standard storage.

#1515 would provide an alternate path to some of the use cases.

Some very limited workarounds may exist using commands (and entity commands) in certain use cases.

alice-i-cecile avatar Nov 30 '21 20:11 alice-i-cecile

One existing tool for this is DynamicScenes, and the bevy::scene::Entity type they contain (which is not the Entity you're thinking about ;-)

Davier avatar Nov 30 '21 21:11 Davier

I don't see what's impractical about making Bundle object safe. We'd need to add a NonNull<Self> version of to_components, but Bundle is already unsafe.

Your description of something to add arbitrary Components is exactly what a Bundle is

DJMcNab avatar Nov 30 '21 21:11 DJMcNab

This would be great for what I'm working on now. I have a simple prefab system set up where you can specify components in a scene-like way but with way less boilerplate:

BuiltIn (
    Transform {
        translation : Vec3 {
            x: 15.0, y: 10.5,
        },
    },
    Visible,
    Draw,
)

If we could iterate over bundles in a generic way I could just drop them right in.

I poked at it a little but I know next to nothing about the unsafe land of rust so sadly beyond me to implement:

Code_TtyIY8B0Kx

sarkahn avatar Dec 04 '21 05:12 sarkahn

Talking it over, the more aggressive direction would be to attempt to replace the Bundle trait entirely:

  • create a type alias type Bundle = IntoIter<Item = impl Component>
  • implement Component for Box<dyn Component>

This allows Vec<Box<dyn Component>> and the like to just be a bundle of components directly

Along with that, you'd:

  • keep the derive macro the same
  • ~~cut the bundle tuple macro, it adds significant compile time, is unidiomatic, and I don't think it's actually useful in end-user code. Users can use an array of boxed components, a vector of components or a bundle-derived struct for when they need it~~
  • keep internal uses around (at least for now), but rename BundleId and BundleInfo to something else

Effectively, this would serve as an alternative to #2974. It would also make issues like #792 much easier.

alice-i-cecile avatar Dec 05 '21 05:12 alice-i-cecile

Also related to #2157, which is another way we could make bundles more flexible in more constrained ways. I don't immediately see any direct overlap, but I'll chew on it.

alice-i-cecile avatar Dec 05 '21 06:12 alice-i-cecile

  • cut the bundle tuple macro, it adds significant compile time, is unidiomatic

Please don't, a tuple doesn't add any allocations, unlike a potential vec solution. Also, it doesn't really add compile time. Just checked, bevy_ecs with up to 16 part tuples (current config) takes 8.8s, bumping the bundle tuple macro to 30 takes 8.9 seconds, lowering it to 1 takes 9.0 seconds. Plus, is it unidiomatic when it's how the rust std lib is built?

mockersf avatar Dec 05 '21 15:12 mockersf

Please don't, a tuple doesn't add any allocations, unlike a potential vec solution.

Deal: you've convinced me on this point here (and other have explained more use cases on Discord).

alice-i-cecile avatar Dec 05 '21 17:12 alice-i-cecile

Have you tried using heterogenous lists (I know them from the shapeless lib in Scala)? There seems to be a frunk crate in Rust that implement those.

Or it should be possible to add an extend method to tuples that would extend a tuple with an additional element: https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=54ac9ca50c1ac210dde4752868d58514

mockersf avatar Dec 06 '21 14:12 mockersf

I don't see what's impractical about making Bundle object safe. We'd need to add a NonNull<Self> version of to_components, but Bundle is already unsafe.

This is my preferred path forward: if we can get this working, we should just use this approach.

alice-i-cecile avatar Dec 12 '21 18:12 alice-i-cecile

More detailed error messages:

    |
117 |     fn component_ids(components: &mut Components, storages: &mut Storages) -> Vec<ComponentId>;
    |        ^^^^^^^^^^^^^ the trait cannot be made into an object because associated function `component_ids` has no `self` parameter
...
132 |     fn get_components(self, func: impl FnMut(*mut u8));
    |        ^^^^^^^^^^^^^^ the trait cannot be made into an object because method `get_components` has generic type parameters
    ```

alice-i-cecile avatar Dec 16 '21 03:12 alice-i-cecile

Also note you cannot have a method take self by value from dyn Bundle as there isn't a way to call such a method. But the compiler won't complain as it is treated kind of like where Self : Sized, doing it makes it impossible to call but you can define it as long as you don't try to use it.

pub unsafe trait Bundle: Send + Sync + 'static {
    /// Gets this [Bundle]'s component ids, in the order of this bundle's Components
    fn component_ids(components: &mut Components, storages: &mut Storages) -> Vec<ComponentId>
    where
        Self: Sized;

    /// Trait object form of component_ids
    fn component_ids_trait(&self, components: &mut Components, storages: &mut Storages) -> Vec<ComponentId>;

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

    /// Calls `func` on each value, in the order of this bundle's Components. This will
    /// "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))
    where
        Self: Sized;

    /// Trait object form of get_components
    fn get_components_trait(&mut self, func: &mut dyn FnMut(*mut u8));
}

I expect everything else to be mechanical but I am against the duplication here (even if they are codegened) unfortunately a quick pass didn't lead to an obvious removal of component_ids given it is used without an instance (by design of course). But maybe this will prove useful to someone else so figured I would pass it along.

Guvante avatar Jan 08 '22 20:01 Guvante

As a heads up, I have a WIP prototype for this (coauthored with @alercah) on https://github.com/alice-i-cecile/bevy/tree/dyn-bundle-mad-science. Feel free to poke at it, steal from it, shamelessly clean it up and submit a PR or so on. I'm a little busy, but the initial direction here is promising.

alice-i-cecile avatar Jan 08 '22 20:01 alice-i-cecile

After #12311, Component is no longer a object-safe trait. That PR might need to be reverted if this is something we want to support.

james7132 avatar Mar 17 '24 23:03 james7132

I've been using Bevy and Rust for a few days and have bumped into exactly this issue. I'm wondering if there are any new potential solutions.

James-Fraser-Jones avatar Jul 26 '24 02:07 James-Fraser-Jones

I am seeing bad performance upon receiving a lot of components from game server that need to be added to entity for the first time for hundreds of entities.

As there could be multiple possible combinations of components for entity I can not use bundles.

Pseudocode:

for entity_data in entities_data_from_server {
     let entity = get_entity_or_spawn(...., entity_data.entity_id);
     for component in entity_data.components {
         match component {
                ComponentA(comp) => { entity.insert(comp); }     
                ComponentB(comp) => { entity.insert(comp); }         
               ....
         }
    }
}

As much as I understand: Each new component insert (directly to world or through command) results in entity being moved from one archetype to another. It would be good if all these operations could be grouped together in dynamic ComponentSet data structure that could be inserted into entity in one operation. In this case no unneeded intermediate archetypes would be initialized, no redundant data moves of entity between different archetypes would be needed.

It looks like something similar can be done with unsafe: https://docs.rs/bevy/latest/bevy/ecs/prelude/struct.EntityWorldMut.html#method.insert_by_ids It would be nice if there would be a safe variant that is easy to use or bundles with optional components (https://github.com/bevyengine/bevy/issues/2157#issuecomment-2376302215) .

PPakalns avatar Sep 26 '24 10:09 PPakalns

I have noticed the problem as well, I hope we can automatically merge the following commands:

let mut deferred = DeferredEntityBuilder::new(entity); // or it is a dynamic bundle
deferred.insert(component1); 
deferred.insert(component2);
deferred.insert_by_id(comp_id,component_dynamic);
deferred.commit(); // inserts everything in one pass

~~In addition, it is unclear how to represent a dynamic component since it currently use a OwningPtr, which seems to fail for many types of structs.~~

I guess it can be designed like this:

pub struct DeferredEntityBuilder<'a> {
    world: &'a mut World,
    entity: Entity,
    ids: Vec<ComponentId>,
    ptrs: Vec<OwningPtr<'a>>,
    bump: &'a Bump,
}

impl<'a> DeferredEntityBuilder<'a> {
    pub fn new(world: &'a mut World, bump: &'a Bump, entity: Entity) -> Self {
        Self {
            world,
            entity,
            ids: vec![],
            ptrs: vec![],
            bump,
        }
    }

    pub fn insert<T: Component>(&mut self, value: T)
    {
        let id = self
            .world
            .component_id::<T>()
            .unwrap_or(self.world.register_component::<T>());
        let ptr = self.bump.alloc(value) as *mut T;
        let ptr = unsafe { OwningPtr::new(NonNull::new_unchecked(ptr.cast())) };
        self.ids.push(id);
        self.ptrs.push(ptr);
    }

    pub fn insert_by_id(&mut self, id: ComponentId, ptr: OwningPtr<'a>) {
        self.ids.push(id);
        self.ptrs.push(ptr);
    }

    pub fn commit(mut self) { //we can also bind to a world at the last minute (comp ids are deferred in that case)
        let mut entity = self.world.entity_mut(self.entity); 
        unsafe { entity.insert_by_ids(&self.ids, self.ptrs.drain(..)) };
    }
}

chengts95 avatar May 13 '25 13:05 chengts95

I used the DeferredEntityBuilder I wrote in my repo here; in this example, I got ✅ Original archetypes (deferred): 13 ✅ Reloaded archetypes (snapshot): 13 ⚠️ Archetypes with commands.insert(): 44 ⚠️ Old way reloaded archetypes (snapshot): 46 The new method works as intended.

chengts95 avatar May 14 '25 16:05 chengts95

Has the state-of-the-art changed since this write-up? https://github.com/bevyengine/bevy/discussions/11409

Consider what axum (with the concrete Response type and IntoResponse trait) or leptos (with various solutions built on top of IntoView trait) does to tackle the same problem. https://book.leptos.dev/view/06_control_flow.html#note-type-conversions

There are two ways to get yourself out of this situation:

  1. Use the enum Either (and EitherOf3, EitherOf4, etc.) to convert the different types to the same type.
  2. Use .into_any() to convert multiple types into one typed-erased AnyView.
// (bevy-provided code)

/// Bundle with 4 variants
pub enum EitherOf4<A, B, C, D> {
    A(A),
    B(B),
    C(C),
    D(D),
}

impl<A: Bundle, B: Bundle, C: Bundle, D: Bundle> Bundle for EitherOf4<A, B, C, D> {}

// -----------

// (user-code)

fn returns_either(a: bool, b: bool) -> impl Bundle {
    // actual return type: EitherOf4<(A, B), A, B, ()>;

    // provided the construction is exhaustive, Rust infers the return type
    match (a, b) {
        (true, true) => EitherOf4::A((A, B)),
        (true, false) => EitherOf4::B(A),
        (false, true) => EitherOf4::C(B),
        (false, false) => EitherOf4::D(()),
    }
}
// (bevy-provided code)

/// Type-erased bundle
pub struct DynamicBundle {
    component_ids: Vec<ComponentId>,
    data: Vec<Box<dyn Any>>, // or something like this
}

// -----------

// (user-code)

fn returns_dynamic(a: bool, b: bool) -> impl Bundle {
    // actual return type: DynamicBundle;

    // type erase all bundles
    match (a, b) {
        (true, true) => (A, B).into_dyn_bundle(),
        (true, false) => A.into_dyn_bundle(),
        (false, true) => B.into_dyn_bundle(),
        (false, false) => ().into_dyn_bundle(),
    }
}

Beyond the above, right now, #[derive(Bundle)] on an enum is a compile-time error, so you could potentially make it not a compile error, basically producing the same code as you'd have for Eitherof4 above.

// (user-code)
fn returns_custom(a: bool, b: bool) -> impl Bundle {
    // actual return type: Custom;

    #[derive(Bundle)]
    enum Custom {
        Ab((A, B)),
        A(A),
        B(B),
        None(()),
    }

    match (a, b) {
        (true, true) => Custom::Ab((A, B)),
        (true, false) => Custom::A(A),
        (false, true) => Custom::B(B),
        (false, false) => Custom::None(()),
    }
}

tigregalis avatar May 31 '25 10:05 tigregalis

Use the enum Either (and EitherOf3, EitherOf4, etc.) to convert the different types to the same type.

That's incredibly ugly code. The axum solution is much cleaner.

teohhanhui avatar May 31 '25 10:05 teohhanhui

Use the enum Either (and EitherOf3, EitherOf4, etc.) to convert the different types to the same type.

That's incredibly ugly code. The axum solution is much cleaner.

EitherOf4 would be provided by Bevy. This is the code you write:

fn returns_either(a: bool, b: bool) -> impl Bundle {
    match (a, b) {
        (true, true) => EitherOf4::A((A, B)),
        (true, false) => EitherOf4::B(A),
        (false, true) => EitherOf4::C(B),
        (false, false) => EitherOf4::D(()),
    }
}

Similarly, DynamicBundle would be provided by Bevy, so you write:

fn returns_dynamic(a: bool, b: bool) -> impl Bundle {
    match (a, b) {
        (true, true) => (A, B).into_dyn_bundle(),
        (true, false) => A.into_dyn_bundle(),
        (false, true) => B.into_dyn_bundle(),
        (false, false) => ().into_dyn_bundle(),
    }
}

Let's look at the axum solution.

async fn handle(Path(path): Path<String>) -> Response {
    if path.len() < 5 {
        (StatusCode::BAD_REQUEST, "Short path").into_response()
    } else {
        StatusCode::OK.into_response()
    }
}

This is identical to the DynamicBundle.

tigregalis avatar May 31 '25 10:05 tigregalis

This is the code you write:

Yeah, having to write EitherOf4 in my code is ugly.

teohhanhui avatar May 31 '25 10:05 teohhanhui

This is the code you write:

Yeah, having to write EitherOf4 in my code is ugly.

Honestly I don't disagree but I'd just rename the import if I cared enough.

fn returns_either(a: bool, b: bool) -> impl Bundle {
    use EitherOf4 as E;
    // you could do `use EitherOf4::*` if there weren't a name conflict in
    //   the variant names and the component names in this example
    match (a, b) {
        (true, true) => E::A((A, B)),
        (true, false) => E::B(A),
        (false, true) => E::C(B),
        (false, false) => E::D(()),
    }
}

tigregalis avatar May 31 '25 10:05 tigregalis

Looking into this, for both the enum and fully dynamic bundles that return impl Bundle, you need to add a self parameter to the component_ids so you can use an instance of the Bundle to determine the component IDs and similar dynamically, so you run into the same issue as making Bundle dyn-compatible anyway (adding self).

However, too many methods internally use <B as Bundle>::component_ids(.. /*no self*/) with no way to pass down an instance of B. From what I can tell, these aren't necessarily operating on bundles for spawning purposes, but using the concept of a Bundle as a static collection of components (e.g. for the purposes of querying data out of the ECS). Essentially Bundle is used in two separate but until now similar ways, and the Bundle infrastructure has been reused for both. So actual usages of Bundle (as a static collection of components) are a bit too tied to being static.

You would need to separate these two usages into something like StaticComponentList and Bundle, and anything that implemented StaticComponentList could also automatically implement Bundle without using the self parameter in its implementations. Then all current usages of Bundle outside of spawning, would use StaticComponentList instead.

It might simply be easier to offer a bridge type DynBundle and custom Command and do something like commands.spawn_dynamic(some_bundle) like a lot of the solutions so far, and have this largely separated from the Bundle architecture, like a lot of the existing workarounds.

Summary:

  1. trait StaticComponentList (static), used for ECS infrastructure, e.g. <B as StaticComponentList>::component_ids
  2. trait Bundle (dyn-compatible), used for spawning
  3. impl<T: StaticComponentList> Bundle for T (or make it part of the derive)
  4. impl Bundle for EnumBundle but impl !StaticComponentList for EnumBundle
  5. impl Bundle for FullyDynamicBundle but impl !StaticComponentList for FullyDynamicBundle
  6. Alternatively, new trait DynBundle and new command spawn_dynamic(impl DynBundle) to allow fn -> impl DynBundle

tigregalis avatar Jun 01 '25 04:06 tigregalis