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

Better tools for working with dynamic collections of components #3227

Open
alice-i-cecile opened this issue Nov 30, 2021 · 14 comments · May be fixed by #14879
Open

Better tools for working with dynamic collections of components #3227

alice-i-cecile opened this issue Nov 30, 2021 · 14 comments · May be fixed by #14879
Labels
A-ECS Entities, components, systems, and events C-Usability A simple quality-of-life change that makes Bevy easier to use

Comments

@alice-i-cecile
Copy link
Member

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 alice-i-cecile added A-ECS Entities, components, systems, and events C-Usability A simple quality-of-life change that makes Bevy easier to use labels Nov 30, 2021
@Davier
Copy link
Contributor

Davier commented Nov 30, 2021

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

@DJMcNab
Copy link
Member

DJMcNab commented Nov 30, 2021

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

@alice-i-cecile alice-i-cecile changed the title Better tools for working with dynamic collections fo components Better tools for working with dynamic collections of components Dec 1, 2021
@sarkahn
Copy link
Contributor

sarkahn commented Dec 4, 2021

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

@alice-i-cecile
Copy link
Member Author

alice-i-cecile commented Dec 5, 2021

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
Copy link
Member Author

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.

@mockersf
Copy link
Member

mockersf commented Dec 5, 2021

  • 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?

@alice-i-cecile
Copy link
Member Author

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).

@mockersf
Copy link
Member

mockersf commented Dec 6, 2021

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

@alice-i-cecile alice-i-cecile added the S-Ready-For-Implementation This issue is ready for an implementation PR. Go for it! label Dec 12, 2021
@alice-i-cecile
Copy link
Member Author

I don't see what's impractical about making Bundle object safe. We'd need to add a NonNull 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
Copy link
Member Author

alice-i-cecile commented Dec 16, 2021

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
    ```

@Guvante
Copy link
Contributor

Guvante commented Jan 8, 2022

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.

@alice-i-cecile
Copy link
Member Author

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.

@james7132
Copy link
Member

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.

@James-Fraser-Jones
Copy link

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-ECS Entities, components, systems, and events C-Usability A simple quality-of-life change that makes Bevy easier to use
Projects
None yet
8 participants