bevy icon indicating copy to clipboard operation
bevy copied to clipboard

Required Components

Open cart opened this issue 1 year ago • 15 comments

Introduction

This is the first step in my Next Generation Scene / UI Proposal.

Fixes https://github.com/bevyengine/bevy/issues/7272 #14800.

Bevy's current Bundles as the "unit of construction" hamstring the UI user experience and have been a pain point in the Bevy ecosystem generally when composing scenes:

  • They are an additional object defining concept, which must be learned separately from components. Notably, Bundles are not present at runtime, which is confusing and limiting.
  • They can completely erase the defining component during Bundle init. For example, ButtonBundle { style: Style::default(), ..default() } makes no mention of the Button component symbol, which is what makes the Entity a "button"!
  • They are not capable of representing "dependency inheritance" without completely non-viable / ergonomically crushing nested bundles. This limitation is especially painful in UI scenarios, but it applies to everything across the board.
  • They introduce a bunch of additional nesting when defining scenes, making them ugly to look at
  • They introduce component name "stutter": SomeBundle { component_name: ComponentName::new() }
  • They require copious sprinklings of ..default() when spawning them in Rust code, due to the additional layer of nesting

Required Components solve this by allowing you to define which components a given component needs, and how to construct those components when they aren't explicitly provided.

This is what a ButtonBundle looks like with Bundles (the current approach):

#[derive(Component, Default)]
struct Button;

#[derive(Bundle, Default)]
struct ButtonBundle {
    pub button: Button,
    pub node: Node,
    pub style: Style,
    pub interaction: Interaction,
    pub focus_policy: FocusPolicy,
    pub border_color: BorderColor,
    pub border_radius: BorderRadius,
    pub image: UiImage,
    pub transform: Transform,
    pub global_transform: GlobalTransform,
    pub visibility: Visibility,
    pub inherited_visibility: InheritedVisibility,
    pub view_visibility: ViewVisibility,
    pub z_index: ZIndex,
}

commands.spawn(ButtonBundle {
    style: Style {
        width: Val::Px(100.0),
        height: Val::Px(50.0),
        ..default()
    },
    focus_policy: FocusPolicy::Block,
    ..default()
})

And this is what it looks like with Required Components:

#[derive(Component)]
#[require(Node, UiImage)]
struct Button;

commands.spawn((
    Button,
    Style { 
        width: Val::Px(100.0),
        height: Val::Px(50.0),
        ..default()
    },
    FocusPolicy::Block,
));

With Required Components, we mention only the most relevant components. Every component required by Node (ex: Style, FocusPolicy, etc) is automatically brought in!

Efficiency

  1. At insertion/spawn time, Required Components (including recursive required components) are initialized and inserted as if they were manually inserted alongside the given components. This means that this is maximally efficient: there are no archetype or table moves.
  2. Required components are only initialized and inserted if they were not manually provided by the developer. For the code example in the previous section, because Style and FocusPolicy are inserted manually, they will not be initialized and inserted as part of the required components system. Efficient!
  3. The "missing required components and constructors needed for an insertion" are cached in the "archetype graph edge", meaning they aren't computed per-insertion. When a component is inserted, the "missing required components" list is iterated (and that graph edge (AddBundle) is actually already looked up for us during insertion, because we need that for "normal" insert logic too).

IDE Integration

The #[require(SomeComponent)] macro has been written in such a way that Rust Analyzer can provide type-inspection-on-hover and F12 / go-to-definition for required components.

Custom Constructors

The require syntax expects a Default constructor by default, but it can be overridden with a custom constructor:

#[derive(Component)]
#[require(
    Node,
    Style(button_style),
    UiImage
)]
struct Button;

fn button_style() -> Style {
    Style {
        width: Val::Px(100.0),
        ..default()
    }
}

Multiple Inheritance

You may have noticed by now that this behaves a bit like "multiple inheritance". One of the problems that this presents is that it is possible to have duplicate requires for a given type at different levels of the inheritance tree:

#[derive(Component)
struct X(usize);

#[derive(Component)]
#[require(X(x1))
struct Y;

fn x1() -> X {
    X(1)
}

#[derive(Component)]
#[require(
    Y,
    X(x2),
)]
struct Z;

fn x2() -> X {
    X(2)
}

// What version of X is inserted for Z?
commands.spawn(Z);

This is allowed (and encouraged), although this doesn't appear to occur much in practice. First: only one version of X is initialized and inserted for Z. In the case above, I think we can all probably agree that it makes the most sense to use the x2 constructor for X, because Y's x1 constructor exists "beneath" Z in the inheritance hierarchy; Z's constructor is "more specific".

The algorithm is simple and predictable:

  1. Use all of the constructors (including default constructors) directly defined in the spawned component's require list
  2. In the order the requires are defined in #[require()], recursively visit the require list of each of the components in the list (this is a depth Depth First Search). When a constructor is found, it will only be used if one has not already been found.

From a user perspective, just think about this as the following:

  1. Specifying a required component constructor for Foo directly on a spawned component Bar will result in that constructor being used (and overriding existing constructors lower in the inheritance tree). This is the classic "inheritance override" behavior people expect.
  2. For cases where "multiple inheritance" results in constructor clashes, Components should be listed in "importance order". List a component earlier in the requirement list to initialize its inheritance tree earlier.

Required Components does generally result in a model where component values are decoupled from each other at construction time. Notably, some existing Bundle patterns use bundle constructors to initialize multiple components with shared state. I think (in general) moving away from this is necessary:

  1. It allows Required Components (and the Scene system more generally) to operate according to simple rules
  2. The "do arbitrary init value sharing in Bundle constructors" approach already causes data consistency problems, and those problems would be exacerbated in the context of a Scene/UI system. For cases where shared state is truly necessary, I think we are better served by observers / hooks.
  3. If a situation truly needs shared state constructors (which should be rare / generally discouraged), Bundles are still there if they are needed.

Next Steps

  • Require Construct-ed Components: I have already implemented this (as defined in the Next Generation Scene / UI Proposal. However I've removed Construct support from this PR, as that has not landed yet. Adding this back in requires relatively minimal changes to the current impl, and can be done as part of a future Construct pr.
  • Port Built-in Bundles to Required Components: This isn't something we should do right away. It will require rethinking our public interfaces, which IMO should be done holistically after the rest of Next Generation Scene / UI lands. I think we should merge this PR first and let people experiment inside their own code with their own Components while we wait for the rest of the new scene system to land.
  • Consider Automatic Required Component Removal: We should evaluate if automatic Required Component removal should be done. Ex: if all components that explicitly require a component are removed, automatically remove that component. This issue has been explicitly deferred in this PR, as I consider the insertion behavior to be desirable on its own (and viable on its own). I am also doubtful that we can find a design that has behavior we actually want. Aka: can we really distinguish between a component that is "only there because it was automatically inserted" and "a component that was necessary / should be kept". See my discussion response here for more details.

cart avatar Aug 17 '24 01:08 cart

One more question: it looks like the fallback constructor is Default. Why not make it FromWorld for both consistency and more expressiveness?

alice-i-cecile avatar Aug 17 '24 01:08 alice-i-cecile

Seems like this doesn't work with hooks/observers as the required components aren't in the added array in BundleInfo (which is the current source of truth).

Here's a test that fails:

#[test]
fn required_components_hooks() {
    #[derive(Component)]
    #[require(Y)]
    struct X;

    #[derive(Component, Default)]
    struct Y;

    #[derive(Resource)]
    struct R(usize);

    let mut world = World::new();
    world.insert_resource(R(0));
    world
        .register_component_hooks::<Y>()
        .on_add(|mut world, _, _| world.resource_mut::<R>().0 += 1);

    // Spawn entity and ensure Y was added
    assert!(world.spawn(X).contains::<Y>());

    assert_eq!(world.resource::<R>().0, 1);
}

james-j-obrien avatar Aug 17 '24 01:08 james-j-obrien

One more question: it looks like the fallback constructor is Default. Why not make it FromWorld for both consistency and more expressiveness?

Given that the constructors are called mid-insertion, this is dangerous. FromWorld could in theory access data that shouldn't be accessed during an insert. Ex: try to read components before they are initialized.

In theory I suspect we would be able to do this via careful ordering of operations, but it would also require some serious restructuring of entity initialization (and how we break up / borrow components from world), especially if we don't want to redundantly allocate outside of the "real" storage.

Worth considering, but imo as a separate PR, and with much more meticulous soundness analysis.

cart avatar Aug 17 '24 02:08 cart

Seems like this doesn't work with hooks/observers as the required components aren't in the added array in BundleInfo (which is the current source of truth).

Hmm yeah I'll look into this.

cart avatar Aug 17 '24 02:08 cart

Confirmed that go to definition works in rust-analyzer:

image

Unfortunately, it does not work in RustRover :( image

Not sure if there's prior art here that's already on JetBrains radar, but it might be worth filing an issue if not.

tychedelia avatar Aug 17 '24 04:08 tychedelia

I have a question regarding the ergnomics, using the Button example provided.

How in the following example would I find out that I can/need to spawn Style to change the look of my spawned Button, neither the Docs or the Language server would help me in this example. Traversing the tree seems like an easy way to get lost or walk in circles quite quickly...

  • Button
    • Node
      • NodeReq1
      • NodeReq2,
    • UiImage
      • UiImageReq2,
      • UiImageReq1
      • Node
#[derive(Component)]
#[require(Node, UiImage)]
struct Button;

commands.spawn((
    Button,
    Style { 
        width: Val::Px(100.0),
        height: Val::Px(50.0),
        ..default()
    },
    FocusPolicy::Block,

TotalKrill avatar Aug 17 '24 08:08 TotalKrill

Thank you for this! 🚀

Does this implementation allow users to query the required components of a component at runtime?

Context: In Moonshine Kind, I use a CastInto<T> trait which has to be implemented manually to allow "upcasting" safely between related components (i.e. a Button should always have a Node, so an Instance<Button> is safely convertible to Instance<Node>).

If we can query the required components, safe casting may be doable automatically.

Zeenobit avatar Aug 17 '24 14:08 Zeenobit

@Zeenobit still not safe, user can remove required component (e.g. Node) but the root component (e.g. Button) can stay

DasLixou avatar Aug 17 '24 14:08 DasLixou

I'll do a pass of the code later but 2 things about the API design:

  • Would this cause problems/be incompatible with private Components? Crates will have private Components they might expose Bundles & QueryDatas for.
  • What if I want a constructor that needs input? This scenario is better covered by relations & inheritance but just to illustrate the problem I might want a EntityRef so I could get a Style component from the parent.

iiYese avatar Aug 17 '24 15:08 iiYese

@Zeenobit still not safe, user can remove required component (e.g. Node) but the root component (e.g. Button) can stay

@DasLixou That's a known (and in my opinion, safely ignorable) issue: I just wouldn't use requirements and/or kind semantics on components that I expect to be removed post initialization.

Zeenobit avatar Aug 17 '24 15:08 Zeenobit

Added https://github.com/bevyengine/bevy/issues/14800 with a suggestion to also generate doc comments from this. (Didn't want to add this here so we can land this quicker.)

killercup avatar Aug 17 '24 17:08 killercup

Just slimmed down the size of AddBundle by only storing the Required Component constructor on it (instead of also including the ComponentId). Given that AddBundle will be high-frequency, better to cut down on init and memory costs.

cart avatar Aug 20 '24 02:08 cart

A collection of "big" todos for this PR:

  • [x] Support generic requires (this was on my pre-PR todo list but I missed it)
  • [x] Support hooks / observers (currently does not invoke them)
  • [x] Support dynamic components (currently not wired up)

cart avatar Aug 20 '24 19:08 cart

Just fixed hooks/observers, which included a refactor of BundleInfo + AddBundle to improve clarity about the scope of the components being accessed. Also added more tests.

cart avatar Aug 21 '24 20:08 cart

I've addressed all comments. The big changes since the opening the PR:

  • Generics and full type paths in #[requires()] are now supported
  • Hooks and observers now work with required components
  • Requires now work with dynamic bundles / components
  • Required Components can now be queried for a given component using let info = world.components().get_info(id).unwrap() followed by info.required_component.iter_ids()
  • The representation of required components has been slimmed down a bit
  • The derive macro now generates docs containing a list of the required components (with links to their docs). This is accomplished by adding the doc to the Component trait impl. Sadly we can't put the docs directly on the type's docs, but this still works pretty well:
    #[derive(Component)]
    #[require(B, C)]
    pub struct A;
    
    image

For some reason miri builds are failing because syn is missing some impls with that build config (not a soundness issue). Not quite sure what the deal is there yet. Once I've solved that I think this is good to go.

cart avatar Aug 24 '24 04:08 cart

It looks like the notes on multiple inheritance didn't really make it into the docs. Could we copy over the explanation from the PR description?

Done!

cart avatar Aug 26 '24 23:08 cart

@cart feel free to merge once CI is green: the new docs look good and it looks like you've resolved the remaining concerns for the MVP.

alice-i-cecile avatar Aug 27 '24 00:08 alice-i-cecile

I've joined the party a bit late here, but I'm wondering how this affects a certain pattern I'm using.

I'm currently using populated bundles as templates for specific types of entities, e.g. I have a resource: EnemyTemplate(EnemyBundle) which gets created by a system at some point after the assets have been loaded; it inserts the resource into the world containing the bundle with the correct sprite image and other configuration from the world. This is kind of like my own rudimentary "prefab" pattern. If the idea of "bundles" is deprecated, I don't really know how to achieve this, or at least it isn't clear to me. I understand that bundles are still here to stay, but I'd like to avoid them if I can in favour of whatever the new way is of doing this.

Jaso333 avatar Oct 08 '24 13:10 Jaso333

Bundles themselves are not deprecated, but the existing bundles are being removed from the engine's public api. Required components is actually kind of built on bundles.

In the future, the recommended approach to this will be through BSN. For now, you may have to write some custom bundles for yourself.

NthTensor avatar Oct 08 '24 14:10 NthTensor

Understood. At least I won't have bundle trees like I currently have, e.g. EnemyBundle contains a field for SpriteBundle.

Jaso333 avatar Oct 08 '24 14:10 Jaso333

Are component requirements enforced during scene deserialization?

i.e. if component A requires B, and I load a scene with just an A, would B get added dynamically?

Zeenobit avatar Oct 12 '24 16:10 Zeenobit

Yes :)

alice-i-cecile avatar Oct 12 '24 16:10 alice-i-cecile

Thank you to everyone involved with the authoring or reviewing of this PR! This work is relatively important and needs release notes! Head over to https://github.com/bevyengine/bevy-website/issues/1725 if you'd like to help out.

alice-i-cecile avatar Oct 20 '24 15:10 alice-i-cecile