Make Component opt-in

[concave-sphere]

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

Currently, Component is auto implemented for any type that is Send + Sync + 'static. This has some benefits:

1. Convenience
2. Allows direct use of pre-existing types as components

But there are also some downsides:

1. There's no way to prevent Bundles from being used with Component APIs, such as in commands.insert(bundle). This results in confusing and hard to track down bugs for newbies, and the number of "danger spots" you have to learn to step around as part of the Bevy learning curve.
2. It's easy to create an unclear architecture where it's not obvious what's supposed to be a component.
3. Using foreign types as components is likely a land mine / attractive nuisance, because another package could come along with the same idea and conflict with your use. It would IMO be better to discourage this pattern so users don't shoot themselves in the foot.

Number one is hopefully obvious. I haven't written a large Bevy program yet, but based on my experience in other domains, numbers 2 and 3 are likely to become a nuisance. Allowing arbitrary types to implicitly get used as a channel for passing data sideways works great for small programs, and rapidly becomes unmanageable as the system size expands. At my workplace we can mitigate the danger with strict code policies, but I don't think that strategy will work with crates.io.

What solution would you like?

Don't auto implement Component. Instead, provide a derive macro:

#[derive(Component)]
struct MyCounter(i32);

The trait impl it expands to is trivial:

impl Component for MyCounter {}

This is almost as convenient as the existing code, while ensuring that types can only be used as Components if they were intended for that purpose. By default, it prevents using a Component as a Bundle (since you'd have to opt in to both types).

There is a question of what to do with generic types. I would suggest this:

#[derive(Component)]
struct MyGenericComponent<T>(T);

// Expands to:
impl<T> Component for MyGenericComponent<T>
    where T: Send+Sync+'static {
}

I didn't require <T> to be a component in this case: by wrapping T in MyGenericComponent, we've declared our intent to make it a Component. The orphan rule ensures that only the owner of MyGenericComponent<T> can implement Component on it (either as a blanket impl or as an impl for one instance). It would be possible to be stricter, but I think this is a good compromise between convenience and safety.

What alternative(s) have you considered?

I don't see any way to fix the problems listed above as long as Component is auto implemented, but there may be some other trickery that my Rust type fu is too weak to see.

[alice-i-cecile]

This has been brought up in other contexts before, and your arguments are valid. Avoiding the bundle-component confusion would be particularly nice, and negative trait bounds will be an eternity to hit stable.

The main concerns are:

1. The addition of required boilerplate.
2. Using foreign types as components. You can bypass this with a wrapper type.

Perhaps we can soften the boilerplate blow by eliminating the need to register components to specify their storage type as part of this change; which would allow that information to be specified much more locally.

This change may also enable interesting features like "immutable components" down the line, or soften the cost of requiring Clone (#1515) on components.

I'm personally in favor of this, but it's a change that should be made deliberately. This is a prime candidate for an RFC once #1662 has landed :)

[DJMcNab]

I am also in favour of this. My 'planned' solution would:

• Change Component to look like:

trait Component {
     fn maybe_register_as_reflect(...)->Option<...>;
     fn storage_type() ->Option<StorageType> {None}
}

• This makes reflecting a key part of components directly, which means that most component types will implement reflect.
  • This is good for scene saving (not gnarly hot reloading 👀) and for the editor
• Add derive(Component), with attributes to support all cases ({no reflect, automatic reflect [default], manual reflect}\times Option<StorageType> [default to 'None', which will probably be dense, although we might not guarantee that (e.g. moving to sparse automatically based on access patterns)])
• Remove the default impls of Component
• Implement component for a specific set of default types (not tuples of T0, T1,T2: Component, not &'static T: Component, yes [T: Component; N], yes Box<T: Component> , yes Arc<T: Component>)

This would fix #392, (and probably #1794)

[BoxyUwU]

Doing this would allow for kinded entities #1634 to be as the target type of a relation #1627 as we could have an associated type for the target type of the relation kind.

[alice-i-cecile]

We should consider whether we want to do this for Resources as well. This would enable a proper solution to #1891 for instance, which would be very useful for the editor.

[DJMcNab]

In my mind, the default state is applying it for resources. We'd have to have some very good reasons not to.

[alice-i-cecile]

In my designs, I'm finding I would also like opt-in an opt-in Event trait. This seems like a natural extension of this idea in a similar way.

[alice-i-cecile]

@BoxyUwU points out that if we have this we should be able to make change detection tracking more easily opt-in or opt-out, resulting in significant memory savings.

[NathanSWard]

Is there any specific reason to not just to the super easy implementation where #[derive(Component)] expands to impl Component for MyType {}.
While this doesn't do anything now, introducing it sooner rather than later would help with the massive number of projects this sort of change would break.

This also allows us to add onto the implementation in the future.

[cart]

I would prefer to wait until we:

1. go through the RFC process, have the derive + manual impl apis well defined, and outline as many benefits + future possibilities as we can
2. have some value-add functionality implemented.

We haven't seen the Bundle vs Component confusion as much since the 0.5 release (thanks to api renames), so I don't feel the pressure to "solve" that problem as much as I did before. And adding boilerplate without a solid "now you can do this" feels like a great way to annoy people.

I don't want to just dive in and break peoples' code before we've built a full and compelling "story" around this.

[NathanSWard]

I don't want to just dive in and break peoples' code before we've built a full and compelling "story" around this.

That's all fair! I was mostly just poking the bear to see where we were standing on this and the best way to proceed forward.

[Byteron]

For completions sake, I might add that personally I tripped over the insert(bundle) issue a number of times.
I know it but I keep falling into that trap anyways.
So while I do very much like the idea of minimal boilerplate, I do think adding derived here is justifiable to prevent such pitfalls. Especially if it makes other things easier as well, such as storage definition / reflect etc.

[speakk]

One of my first uses of components I stumbled into this. It wasn't in the context of Bundles, but instead:
pub struct Position(pub Vec2)

And inserting this into an entity by accident with:
.insert(Position) instead of the correct .insert(Position(Vec2::default())

There was no compile or runtime error, I was just left wondering why my Queries for Position weren't returning any results.

[speakk]

I also like this for its self documenting implications.

I sometimes have some utility structs and whatnot near where I define my components. This change would make it 100% clear what is a component and what is not, at a glance.

[Frizi]

There are many positives from adding #[derive(Component)]. While at first glance it seems like an unnecessary boilerplate, it has potential to massively reduce boilerplate for any feature related to components type, as well as remove many pitfalls (e.g. inserting of bundles).

I also like the "self-documenting" argument for this, and I see it being a correctness argument as well.
It's not really good idea to use random non-component struct as a component type. And if you do, there are possible problems with different semantics attached to that type conflicting with each other. Imagine if two different libs use a random type from shared dependency as a component in their system, meaning different things. Their systems will compile, but behave incorrectly due to interaction between them. If you want to create a component type with specific semantic, you are better off defining a newtype for it anyway.

Apart from nice marker trait that does all of the above, we can also use this derive for more things. One particularily attractive example (previously mentioned in this thread) is the storage type. By making storages an associated type, we make them known statically at compile time. This is a huge deal for iteration speed - right now we generate a lot of code and have some extra branches for every single query iterator. Being able to optimize those out through static typing is likely to increase performance significantly, as well as make the iterator implementation way less fragile to code changes (we are at the limit of compiler right now and already inline a lot of complicated code manually because of that). This would also allow for easier experimentation with different storage implementations without impacting the performance of existing code.

The above argument might seems like a bad idea for dynamic components in the future, but I think it's not really the case. There is nothing wrong with having a DecidedAtRuntime "storage type" which does manual dynamic dispatch based on runtime info (basically what we do right now for every query anyway).

There is also reflection or other features necessary for good editor experience. There is not much hope in implementing that without some kind of derive macro. The next best thing is global code analysis, but it is just a terrible idea.

[Frizi]

Implementation of opt-in Component marker trait derive in #2254

[alice-i-cecile]

Another idea from @BoxyUwU on this note to generalize the registration of events:

trait [Component | Resource]: Send + Sync + 'static {
  pub fn on_register(world: &mut World) {}
}

I've needed this pattern in my Styles and Hooks proposals, and I would like to have a clean mechanism. Note that we'd want to solve the event cleanup issues with fixed time steps in one way or another to do this.

This may also spare us from needing to call .init_resource at all, which would be a massive ergonomics win (if a bit magic).

[Frizi]

At this point you probably want trait Event as well. Actually due to namespace conflicts, it would have to be EventData, unless we rename Event struct (i've hit the same problem with State in the derive impl PR).

I like the idea of registration hook though. Still, the details would have to be considered in context of derive(Component) not having a natural place to impl that. I guess we would need something like this:

#[derive(Component)]
#[on_register(my_register_fn)]
struct MyComponent { .. }

fn my_register_fn(world: &mut World) {
	// ...
}

I wonder if we can make it more natural, especially if we will add more hooks.

[alice-i-cecile]

@TheRawMeatball wants an Event trait already for #2073 to configure event storage :)

[TheRawMeatball]

An easy registration hook would be one of the major advantages of this design imo, as this means we can use the existing implicit registration infrastructure for more things, such as Reflect registration and possibly more.

[cart]

I think we should seriously consider "real trait impls" over building a complicated derive(Component) dsl.

Ex: derive(Component) provides the "defaults" (maybe with a few configuration options for the most common things, such as storage type) . If you need something custom / less common you would do the full Component trait impl. This improves documentation/feature discoverability and makes us more "rust native".

trait Component {
  fn on_register(world: &mut World) {
  }
}


#[derive(Component)]
struct A;

struct B;

// identical to A impl
impl Component for B {
  type Storage = Table; // associated type defaults aren't stable yet :(
}

// once associated type defaults stabilize it would just be this
impl Component for B {}

struct C;
impl Component for C {
  type Storage = Table;
  fn on_register(world: &mut World) {
     // ...
  }
}

But we should enumerate the scenarios we want to cover before picking a path.

[Frizi]

I'm trying to figure out the best way to implement the storage type declaration DSL.
Right now I did this:

#[derive(Component)]
#[storage(sparse)]
struct A;

I don't like that though, because every single extra declaration will have to be another attribute. Also there is no clear connection between attribute and the Component derive. So, I'm thinking about doing similar syntax to what serde does, and use single meta attribute with multiple "named parameters", like so

#[derive(Component)]
#[component(storage = SparseSet)]
struct A;

That would allow us to later add other common parameters if needed.

Also the manual trait impl must be easy. My plan is to use associated const STORAGE. So the impl would look like this:

impl Component for B {
  const STORAGE: StorageType = StorageType::SparseSet;
}

Alternatively an associated type could be used (this is in the current impl), but does require importing extra types that doesn't directly correspond to the parameters used in the attribute.

impl Component for B {
  type Storage = bevy::component::SparseSetStorage;
}

This opens up the possibility of maybe allowing custom storage impls, but right now it's not really possible due to high level of coupling betweeen storages and ecs internals (those are basically one and the same right now). So for now the ComponentStorage trait (guard for the associated type) would be sealed anyway.

[alice-i-cecile]

Solved in #2254.