Make a dynamically applicable version of Bundle

[DJMcNab]

Objective

• Fixes Better tools for working with dynamic collections of components #3227 (I think? @alice-i-cecile please confirm)

Solution

• For dynamic bundles, you can now use Box<dyn ApplicableBundle>
• Naming nitpicks welcome

[bjorn3]

Maybe call this DynBundle?

[DJMcNab]

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.

[alice-i-cecile]

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.

[DJMcNab]

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.

[alice-i-cecile]

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.

[DJMcNab]

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

[DJMcNab]

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.

[alice-i-cecile]

Because we can't nest dynamic bundles, could we get a few simple helper methods on ApplicableBundle?

In particular, I would like the ability to iterate over it, and the ability to merge (union) two ApplicableBundle.

[alice-i-cecile]

I'm going to be submitting a PR to this branch to add a solid example explaining how and why you might use this functionality.

[DJMcNab]

I'm not sure what you mean by iterating over a dynamic bundle? The whole point of the Bundle api is iterating over the contained components.

And the whole reason for this design which doesn't allow nesting is because a union style operation would break our bundle caching features; this currently neatly sidesteps that issue by (effectively) requiring that a 'real' Bundle implementation is backing the ApplicableBundle. A union operation would require significantly more work.

[hlb8122]

use bevy::prelude::*;

fn main() {
    App::new()
        .add_plugins(DefaultPlugins)
        .add_startup_system(setup)
        .run();
}

#[derive(Bundle)]
pub struct BasicBundle {}

fn setup(mut commands: Commands) {
    let normal_bundle: Box<dyn ApplicableBundle> = Box::new(BasicBundle {});
    commands.spawn_bundle(normal_bundle);
}

[package]
name = "dyn-test"
version = "0.1.0"
edition = "2021"

[dependencies]
bevy = "0.6.0"

[patch.crates-io]
bevy = { git = "https://github.com/DJMcNab/bevy.git", branch = "breaker-of-bundles" }

[[package]]
name = "bevy"
version = "0.6.0"
source = "git+https://github.com/DJMcNab/bevy.git?branch=breaker-of-bundles#57caadc215d449fc06c5371f22c03a009d261fe8"
dependencies = [
 "bevy_internal",
]

OS Name:                   Microsoft Windows 11 Pro N
OS Version:                10.0.22000 N/A Build 22000

rustc 1.58.0 (02072b482 2022-01-11)

I get thread 'main' has overflowed its stack as soon as the spawn occurs.

Seems that init_bundle_info is calling itself recursively.

[DJMcNab]

Hmm, fair enough, thanks

That's not super unexpected :p - I didn't actually test the code

[DJMcNab]

I've pushed a fix, I hope.

[alice-i-cecile]

Awesome, I'm thrilled that this was so easy! The doc example is solid.

ApplicableBundle isn't my favorite name, but I'll punt to @cart to decide on a naming convention.

[hlb8122]

@DJMcNab that fixed #3694 (comment) 👍

[alice-i-cecile]

FYI, I have an example branch for this feature. I could use some feedback on the direction; it's a bit elaborate, but showcases some of the magic that this enables.

[jakyle]

I think in the interest of reducing boiler plate code and creating collections of dynamically dispatched components, this would be a great feature. I understand that this may not be as "performant" and will introduce unsafe code behind the scene; However, developer ergonomic goes a long way in this case and I know a big selling point of Bevy is just that.

[cart]

I'd like to explore motivating use cases a bit more before we decide to embrace this pattern / complicate bundle internals.

@alice-i-cecile

FYI, I have an example branch for this feature. I could use some feedback on the direction; it's a bit elaborate, but showcases some of the magic that this enables.

I'm not seeing anything in there that actually benefits from this abstraction. The dynamic bundle creation bit could easily be replaced by an associated type, which is both more efficient (because it can use the dyn-stack Command impl for inserts / it can be fully monomorphized) and clearer (because users don't need to think about boxing their bundles):

trait Widget: Component {
  type Bundle: Bundle;
  fn new() -> Self::Bundle;
}


fn spawn_widget<W: Widget>(mut commands: Commands) {
    // `W::n_to_spawn` calls the trait method on `Widget` for the concrete type `W`
    for i in 0..W::n_to_spawn() {
        commands.spawn()
             /* other commands here */
            .insert_bundle(W::new());
    }
}

Additionally, to properly compare / contrast designs + patterns, I think we should come up with illustrative examples that actually compile. There are a number of issues with the linked impl that would need to be fixed if we want to actually compile it / test what works.

[alice-i-cecile]

Sounds good. I'll put some time into the game that wants this, and see if I can come up with workarounds or a solid minimal reproduction of the value.

[alice-i-cecile]

Following work in #5513 and #5503, I'm strongly favoring a generic builder pattern for bundles that looks like

trait UiComponent;

trait BuildUi {
   fn with<C: UiComponent>(self, component: C) -> Self;
}

Obviously this has non-UI implications, but if you're working on an open set you want to be able to extend the bundle if needed. This PR would enable that pattern, and allow us to expose this ergonomic pattern in a consistent way to the ECS as a whole.

[DJMcNab]

How would this pr affect the ability to create that pattern?

This seems to be another instance of this API being an attractive nuisance (with people trying to make it do things it intentionally cannot do).

[alice-i-cecile]

After sleeping on it, you're right @DJMcNab. The correct pattern here IMO is just chaining .insert and using the overwriting behavior.

We just need #5074 to make sure that's not a performance footgun.

[alice-i-cecile]

Closing this out due to lack of use cases. We can revive it if this ever recurs. @PROMETHIA-27 suggests that most users will be better off with the use of ReflectComponent instead.